Discussion: should we enforce access control in C++ attributes?

A number of users have recently expressed frustration with fact that
access-control restrictions (i.e. public, private, etc.) are enforced
within thread safety attributes. I am considering whether or not I
should lift those restrictions, and this is not a decision that I want
to make unilaterally. There are some potentially far-reaching
ramifications here in terms of how attributes are handled in C++ in
general, so I want get feedback from cfe-dev before blindly forging
ahead. Be warned that this is a somewhat long post. I will try to lay
out the problem, along with the pros and cons as I seem them. I would
love to hear what the rest of you think.

Here's the problem. Thread-safety attributes are attached to fields
or methods, and name the protecting mutex for that field or method.
Most often, the protecting mutex is a private or protected data
member. For example:

class TaskQueue {
public:
  void lock() EXCLUSIVE_LOCK_FUNCTION(mu) {
    mu.Lock();
    logfile << "Task queue acquired by " << getCurrentThreadID() << "\n";
  }

  void unlock() UNLOCK_FUNCTION(mu) {
    mu.Unlock();
  }

  virtual void addTask(int taskID) EXCLUSIVE_LOCKS_REQUIRED(mu) {
    queue.push_back(taskID);
  }

  void addTaskSync(int taskID) {
    lock();
    addTask(taskID);
    unlock();
  }

private:
  Mutex mu;
  std::deque<int> queue GUARDED_BY(mu);
};

class CheckedTaskQueue : public TaskQueue {
public:
  bool validTask(int id);

  // error: TaskQueue::mu is private
  virtual void addTask(int taskID) EXCLUSIVE_LOCKS_REQUIRED(mu) {
    if (validTask(taskID)) TaskQueue::addTask(taskID);
  }
};

// error: q->mu is private.
void addTwoTasks(TaskQueue* q, int t1, int t2) EXCLUSIVE_LOCKS_REQUIRED(q->mu) {
  q->addTask(t1);
  q->addTask(t2);
}

This example illustrates how thread safety annotations are used, for
readers who are unfamiliar with them. TaskQueue provides two ways to
add tasks to a queue in a thread-safe manner. The addTaskSync()
method takes care of locking and unlocking automatically, while the
addTask() method requires the caller to lock and unlock the queue.
Because addTask() is marked with EXCLUSIVE_LOCKS_REQUIRED,
thread-safety analysis will issue a warning at compile time if
addTask() is called when the lock is not held.

A problem arises because mu is private. It is an error to override
addTask() in derived classes, because overriding requires mentioning
"mu" again in the attribute, and that violates access control
restrictions. Similarly, it is impossible to create external
functions like addTwoTasks(), because the locking requirements
propagate up, and "mu" isn't visible outside of TaskQueue.

Fortunately, there's a relatively simple workaround, which is to
declare a public getter method for the mutex. The main issue here is
that "mu" is private for a reason; we want clients of the class to go
through TaskQueue::lock() and unlock(), rather than acquiring the
mutex directly. There are two ways to keep the mutex from being
misused:

class Taskqueue {
public:
  // Option 1: const getter method.
  const Mutex& getMu() const LOCK_RETURNED(mu) { return mu; }

  // Option 2: fake getter that returns null.
  const Mutex* getMu() const LOCK_RETURNED(mu) { return 0; }

  ...
};

class CheckedTaskQueue : public TaskQueue {
public:
  // error: TaskQueue::mu is private
  virtual void addTask(int taskID) EXCLUSIVE_LOCKS_REQUIRED(getMu())

  ...
};

Option 1 follows standard OO design practice -- use a public getter
method to provide limited access to a private variable. Attributes
outside of TaskQueue can refer to getMu() rather than mu, and
LOCK_RETURNED tells the analysis that mu and getMu() refer to the same
lock.

However, some users don't like Option 1, because a const reference can
easily become a real reference with a simple const_cast, and they want
stronger protection. Option 2 provides the extra protection, but it's
a bit weird. It exploits the fact that thread-safety analysis only
looks at the LOCK_RETURNED attribute, not the body of the method.
Many users don't like the fact that their code becomes littered with
public getter methods that are never actually called, and have bogus
implementations.

I can see both sides of the argument here, so I'll do my best to
present a case both for and against enforcing access control
restrictions.

Argument in favor of disabling access control in attributes.

Hi Delesly,

Admittedly I've only briefly skimmed through this, but I did have a
few thoughts:

A number of users have recently expressed frustration with fact that
access-control restrictions (i.e. public, private, etc.) are enforced
within thread safety attributes. I am considering whether or not I
should lift those restrictions, and this is not a decision that I want
to make unilaterally. There are some potentially far-reaching
ramifications here in terms of how attributes are handled in C++ in
general, so I want get feedback from cfe-dev before blindly forging
ahead. Be warned that this is a somewhat long post. I will try to lay
out the problem, along with the pros and cons as I seem them. I would
love to hear what the rest of you think.

Here's the problem. Thread-safety attributes are attached to fields
or methods, and name the protecting mutex for that field or method.
Most often, the protecting mutex is a private or protected data
member. For example:

class TaskQueue {
public:
  void lock() EXCLUSIVE_LOCK_FUNCTION(mu) {
    mu.Lock();
    logfile << "Task queue acquired by " << getCurrentThreadID() << "\n";
  }

  void unlock() UNLOCK_FUNCTION(mu) {
    mu.Unlock();
  }

  virtual void addTask(int taskID) EXCLUSIVE_LOCKS_REQUIRED(mu) {
    queue.push_back(taskID);
  }

  void addTaskSync(int taskID) {
    lock();
    addTask(taskID);
    unlock();
  }

private:
  Mutex mu;
  std::deque<int> queue GUARDED_BY(mu);
};

class CheckedTaskQueue : public TaskQueue {
public:
  bool validTask(int id);

  // error: TaskQueue::mu is private
  virtual void addTask(int taskID) EXCLUSIVE_LOCKS_REQUIRED(mu) {
    if (validTask(taskID)) TaskQueue::addTask(taskID);
  }
};

// error: q->mu is private.
void addTwoTasks(TaskQueue* q, int t1, int t2) EXCLUSIVE_LOCKS_REQUIRED(q->mu) {
  q->addTask(t1);
  q->addTask(t2);
}

This example illustrates how thread safety annotations are used, for
readers who are unfamiliar with them. TaskQueue provides two ways to
add tasks to a queue in a thread-safe manner. The addTaskSync()
method takes care of locking and unlocking automatically, while the
addTask() method requires the caller to lock and unlock the queue.
Because addTask() is marked with EXCLUSIVE_LOCKS_REQUIRED,
thread-safety analysis will issue a warning at compile time if
addTask() is called when the lock is not held.

A problem arises because mu is private. It is an error to override
addTask() in derived classes, because overriding requires mentioning
"mu" again in the attribute, and that violates access control
restrictions. Similarly, it is impossible to create external
functions like addTwoTasks(), because the locking requirements
propagate up, and "mu" isn't visible outside of TaskQueue.

Fortunately, there's a relatively simple workaround, which is to
declare a public getter method for the mutex. The main issue here is
that "mu" is private for a reason; we want clients of the class to go
through TaskQueue::lock() and unlock(), rather than acquiring the
mutex directly. There are two ways to keep the mutex from being
misused:

class Taskqueue {
public:
  // Option 1: const getter method.
  const Mutex& getMu() const LOCK_RETURNED(mu) { return mu; }

  // Option 2: fake getter that returns null.
  const Mutex* getMu() const LOCK_RETURNED(mu) { return 0; }

Could this function not be defined? Possibly defined as deleted (=
delete)? (not sure whether it makes sense to allow calls to deleted
functions in the unevaluated context of a thread safety attribute (I
think/assume we don't allow calls to deleted functions in other
unevaluated contexts (which is where a deleted function would differ
from an undefined function))).

  ...
};

class CheckedTaskQueue : public TaskQueue {
public:
  // error: TaskQueue::mu is private
  virtual void addTask(int taskID) EXCLUSIVE_LOCKS_REQUIRED(getMu())

What about an Option 3?

Have you considered doing something like C++11's noexcept operator (
http://en.cppreference.com/w/cpp/language/noexcept )? In which you
could provide an expression & whatever locks are required by that
expression are required by this "Locks Required" clause (or other
properties you're interested in capturing in the locking annotations)

So in this case:

  virtual void addTask(int taskID)
EXCLUSIVE_LOCKS_REQUIRED(required_locks(TaskQueue::addTask(taskID)));

or something to that effect. This encapsulates the contract better &
without code duplication - if the contract of the base addTask
changes, so does the derived implementation (or the wrapper function,
or the whatever else needs this). Relying on the implementation
details of the functions you're calling & duplicating their contracts
explicitly seems sub-optimal compared to propagating the important
expressions up.

I'll admit to only skimming this. Please bear in mind that other people
have less time to read your messages than you have to write them.

That said, it seems very strange for a method to declare that it
requires a lock to be held when neither it nor its users can actually
access that lock. That's useless as user documentation because
it's not actionable — I can't actually modify my code in response to
your complaints.

You should instead allow the lock expression to more directly state
what the caller of this method needs to do in order to acquire the lock.
In this case, that means a reference to the lock() method, which is,
not coincidentally, actually public. Since that method is already
annotated with the lock it manipulates, this should be quite easy to
round-trip in the static analysis.

Going further, in principle, these expressions should really be
access-checked *from the perspective of a valid user of this method*:
that is, they should only be access-checked in the context of the class
when the annotated method is non-public. For a public method, they
should really be access-checked from a totally unprivileged context.

John.

Could this function not be defined? Possibly defined as deleted (=
delete)? (not sure whether it makes sense to allow calls to deleted
functions in the unevaluated context of a thread safety attribute (I
think/assume we don't allow calls to deleted functions in other
unevaluated contexts (which is where a deleted function would differ
from an undefined function))).

Yes, it can be left undefined, and that's what I used to do in some of
our code. However, a reviewer pointed out to me that if it's
undefined, anyone wanting the mutex can get it by supplying a
definition (perhaps even thinking that the lack of definition was
unintentional) so it was safer to define the method and return 0.

Marking it as deleted is an interesting idea that I hadn't thought of.
However, instead of turning off the access control checks inside
attributes, I'd have to turn off the deleted checks instead. Once
again, that would turn off the checks for every part of the expression
in an attribute, not just the parts that refer to the mutex. I'm not
sure that's a good idea.

Have you considered doing something like C++11's noexcept operator (
http://en.cppreference.com/w/cpp/language/noexcept )? In which you
could provide an expression & whatever locks are required by that
expression are required by this "Locks Required" clause (or other
properties you're interested in capturing in the locking annotations)

So in this case:

  virtual void addTask(int taskID)
EXCLUSIVE_LOCKS_REQUIRED(required_locks(TaskQueue::addTask(taskID)));

or something to that effect. This encapsulates the contract better &
without code duplication - if the contract of the base addTask
changes, so does the derived implementation (or the wrapper function,
or the whatever else needs this). Relying on the implementation
details of the functions you're calling & duplicating their contracts
explicitly seems sub-optimal compared to propagating the important
expressions up.

Another interesting idea. :slight_smile: Yes, I could do this. In the case of
virtual methods, the best thing is really to propogate LOCKS_REQUIRED
automatically, since it's actually a type safety violation to
redeclare it anyway. I included the virtual method primarily for
comparison with the private type example later. However, your
technique would also work for external functions like addTwoTasks, so
it's definitely a viable option.

I guess my main concern here is that
EXCLUSIVE_LOCKS_REQUIRED(required_locks(TaskQueue::addTask(taskID)))
is a lot less clear than EXCLUSIVE_LOCKS_REQUIRED(mu). Moreover, for
more complicated methods, it's not feasible to encapsulate the
contract in a completely general manner -- you'd have to list every
method that might require a mutex. I think this idea would be really
useful in certain cases, such as templates (where noexcept is also
most useful), but I'm pretty sure I'm going to get some major pushback
from users if I recommend this as the officially "blessed" way to do
thread annotations. :slight_smile:

Thanks for the suggestions,

  -DeLesley

Could this function not be defined? Possibly defined as deleted (=
delete)? (not sure whether it makes sense to allow calls to deleted
functions in the unevaluated context of a thread safety attribute (I
think/assume we don't allow calls to deleted functions in other
unevaluated contexts (which is where a deleted function would differ
from an undefined function))).

Yes, it can be left undefined, and that's what I used to do in some of
our code. However, a reviewer pointed out to me that if it's
undefined, anyone wanting the mutex can get it by supplying a
definition (perhaps even thinking that the lack of definition was
unintentional) so it was safer to define the method and return 0.

Hmm, I suppose I can see that (though the idea of a possible runtime
failure when someone tries to call that isn't exactly appealing). I'd
be inclined to leave it undefined with a comment rather than defined
with a possibly distant failure. (maybe an assert rather than a
silently invalid return). The function would still be surprising even
if it was defined rather than declared-but-not-defined, so I assume in
either case it needs a comment.

Marking it as deleted is an interesting idea that I hadn't thought of.
However, instead of turning off the access control checks inside
attributes, I'd have to turn off the deleted checks instead. Once
again, that would turn off the checks for every part of the expression
in an attribute, not just the parts that refer to the mutex. I'm not
sure that's a good idea.

Yeah, if = deleted fires in normal unevaluated contexts (sizeof, etc)
- which I assume it does, then it's probably not a great idea to have
it do something different/surprising in thread safety annotations.

Have you considered doing something like C++11's noexcept operator (
http://en.cppreference.com/w/cpp/language/noexcept )? In which you
could provide an expression & whatever locks are required by that
expression are required by this "Locks Required" clause (or other
properties you're interested in capturing in the locking annotations)

So in this case:

  virtual void addTask(int taskID)
EXCLUSIVE_LOCKS_REQUIRED(required_locks(TaskQueue::addTask(taskID)));

or something to that effect. This encapsulates the contract better &
without code duplication - if the contract of the base addTask
changes, so does the derived implementation (or the wrapper function,
or the whatever else needs this). Relying on the implementation
details of the functions you're calling & duplicating their contracts
explicitly seems sub-optimal compared to propagating the important
expressions up.

Another interesting idea. :slight_smile: Yes, I could do this. In the case of
virtual methods, the best thing is really to propogate LOCKS_REQUIRED
automatically, since it's actually a type safety violation to
redeclare it anyway.

What's the violation, sorry? In theory you should be able to /relax/
lock requirements in overrides (reducing the precondition contract),
but not tighten it further.

I included the virtual method primarily for
comparison with the private type example later. However, your
technique would also work for external functions like addTwoTasks, so
it's definitely a viable option.

I guess my main concern here is that
EXCLUSIVE_LOCKS_REQUIRED(required_locks(TaskQueue::addTask(taskID)))
is a lot less clear than EXCLUSIVE_LOCKS_REQUIRED(mu). Moreover, for
more complicated methods, it's not feasible to encapsulate the
contract in a completely general manner -- you'd have to list every
method that might require a mutex.

Yes, certainly in the most general case you would have to list the
entire function body, but that's perhaps excessive. Even getting a bit
of generality over the things you expect to require locks seems like
it's incrementally valuable.

I think this idea would be really
useful in certain cases, such as templates (where noexcept is also
most useful),

Certainly. It's probably going to be necessary there (shared_ptr<Mutex>, etc).

but I'm pretty sure I'm going to get some major pushback
from users if I recommend this as the officially "blessed" way to do
thread annotations. :slight_smile:

I'd say perhaps this can be combined with John's idea of focusing on
the client's actions rather than the implementation's. If a client can
call "lock()" to lock a thing & you need that to be done before you
call foo(), then foo() could say "LOCKS_REQUIRED(locks_of(x.lock()))"
or something. Easier to read/write/fix when you see the error.

Thanks for the comments! I know it was a long post. :slight_smile:

That said, it seems very strange for a method to declare that it
requires a lock to be held when neither it nor its users can actually
access that lock.

I tend to agree with you here, which is why I'm suspicious in general
when I see public methods referring to private mutexes.

That's useless as user documentation because
it's not actionable — I can't actually modify my code in response to
your complaints.

You should instead allow the lock expression to more directly state
what the caller of this method needs to do in order to acquire the lock.

The thread-safety analysis is based on published research in race-free
type systems, and it works by associating every piece of data with its
protecting mutex. There's no universal way to acquire a particular
mutex -- how the mutex is acquired and released depends entirely on
the interface being provided by the class. You'd be surprised by how
much variety I've seen in locking schemes. :slight_smile:

The only way to reference lock() instead of the mutex would be to use
some variation of the scheme proposed by David Blakie, but that won't
work in all cases.

In this case, that means a reference to the lock() method, which is,
not coincidentally, actually public.

In this case, the lock() method is public. However in many other
cases, there is no public lock() method. A pattern I've seen is where
some methods are supposedly "public," but they are are only intended
to be used from a limited context, and the protecting mutex is
available within that context. The thread safety analysis will
effectively prevent the methods from being called outside of their
intended context. Similarly, there may be an external helper function
or helper class that is only used from within private methods of a
different class, so it may legitimately need to refer to a private
mutex within that class.

Going further, in principle, these expressions should really be
access-checked *from the perspective of a valid user of this method*:
that is, they should only be access-checked in the context of the class
when the annotated method is non-public. For a public method, they
should really be access-checked from a totally unprivileged context.

I tend to agree with this, but that would be a stricter level of
access control than we currently use, and it is actually stricter than
what C++ normally does with private types.

  -DeLesley

That's useless as user documentation because
it's not actionable — I can't actually modify my code in response to
your complaints.

You should instead allow the lock expression to more directly state
what the caller of this method needs to do in order to acquire the lock.

The thread-safety analysis is based on published research in race-free
type systems, and it works by associating every piece of data with its
protecting mutex. There's no universal way to acquire a particular
mutex -- how the mutex is acquired and released depends entirely on
the interface being provided by the class. You'd be surprised by how
much variety I've seen in locking schemes. :slight_smile:

Sure.

The only way to reference lock() instead of the mutex would be to use
some variation of the scheme proposed by David Blakie, but that won't
work in all cases.

Your analysis recognizes arbitrary expressions in these contexts as long
as they resolve to something that works like a lock, right? Why not also,
as an alternative, permit arbitrary expressions that happen to resolve
to calls to methods with the THREAD_SAFETY_ACQUIRES_LOCK
attribute? e.g.
  THREAD_SAFETY_REQUIRES_LOCK(lock())
In other words, I am simply proposing that you accept an alternate way
of specifying the lock; this should not deeply affect your algorithm.

In this case, that means a reference to the lock() method, which is,
not coincidentally, actually public.

In this case, the lock() method is public. However in many other
cases, there is no public lock() method. A pattern I've seen is where
some methods are supposedly "public," but they are are only intended
to be used from a limited context, and the protecting mutex is
available within that context.

Okay, but you're checking from the context of that method (or at least its
class), so unless the method which relies on the lock being held actually
doesn't have access to the lock, this shouldn't be a problem.

I can imagine situations where that would be true, but only with some
sort of external locking where there probably isn't a static path from the
object to the lock in the first place. Otherwise, locks are overwhelmingly
likely to be privileged data.

Similarly, there may be an external helper function or helper class that
is only used from within private methods of a different class, so it may
legitimately need to refer to a private mutex within that class.

Sure, but why not befriend the helper class or function if it's going to
rely on internal details of this other class?

John.

This approach makes some sense to me.

It seems that the heart of the problem is that you have a region
controlled by a mutex, and you're using the name of that mutex as a
proxy for the name of the region, but access to the region does not
necessarily imply access to the mutex. I think the right solution is
to find another (accessible) way to name the region, not to provide
access to the mutex.

If your class only has one such region, it wouldn't seem unreasonable
to me to use the instance of the class as a proxy for your region name
in most cases (which I believe the thread safety annotations already
support). Otherwise (or if that's not semantically appropriate), it
seems that we should provide a mechanism for classes to expose a name
for their lockable regions as part of their public API, and I don't
think those names should be required to be tied to the names of the
mutexes (or via any other mechanism which leaks implementation
details). In principle, we could add another form of named declaration
for that; if we stick to existing language constructs, I think using a
member function as a proxy for the region name is the best option.

Are there reasonable cases where it's reasonable to put a
LOCKS_REQUIRED annotation on a function which doesn't have access to
the mutex it requires, and also doesn't have access to a mechanism
which acquires it? I could imagine such cases arising in templates:

class LockedData {
  Mutex mu;
public:
  template<typename F> void RunWithLockHeld(F f) {
    ScopedLock lock(mu);
    f(this);
  }
  int data GUARDED_BY(mu);
};

struct DoThing {
  void operator()(LockedData *p) const LOCKS_REQUIRED(p->mu) {
    p->data *= 42;
  }
};

LockedData data;
data.RunWithLockHeld(DoThing());

... though note that even in this case, we have both *p and p->data,
which could be used as proxies for the required mutex.

I think that from a user’s perspective, I prefer John’s approach.

Instead of stating the exact name of the mutex/region that I need to lock, it states what I need to do: this means that I don’t have to figure out how to lock the mutex => the safety annotation becomes documentation.

What would you think of this “revised” example:

class TaskQueue {
public:
void lock() EXCLUSIVE_LOCK_FUNCTION(mu) {
mu.Lock();
logfile << "Task queue acquired by " << getCurrentThreadID() << “\n”;
}

void unlock() UNLOCK_FUNCTION(mu) {
mu.Unlock();
}

virtual void addTask(int taskID) EXCLUSIVE_LOCKS_REQUIRED(this->lock()) {
queue.push_back(taskID);
}

void addTaskSync(int taskID) {
lock();
addTask(taskID);
unlock();
}

private:
Mutex mu;
std::deque queue GUARDED_BY(mu);
};

class CheckedTaskQueue : public TaskQueue {
public:
bool validTask(int id);

// OK: TaskQueue::lock() is public
virtual void addTask(int taskID) EXCLUSIVE_LOCKS_REQUIRED(this->lock()) {
if (validTask(taskID)) TaskQueue::addTask(taskID);
}
};

// OK: TaskQueue::lock() is public
void addTwoTasks(TaskQueue* q, int t1, int t2) EXCLUSIVE_LOCKS_REQUIRED(q->
lock()) {
q->addTask(t1);
q->addTask(t2);
}

Actually, it seems annoying to me that lock and unlock are exposing the name of the mutex, since it’s private, but then it would probably be exposed anyway so…

However, this made me realize there is a small issue with the safety analysis as it stands: I cannot see how to express the contracts of lock and unlock in the following PIMPL case.

class Queue {
public:
Queue(); Queue(Queue const&); Queue(Queue&&); Queue& operator=(Queue); ~Queue();

void lock() … ;

void unlock() … ;

void push(int) EXCLUSIVE_LOCKS_REQUIRED(this->lock()) ;

int pop() EXCLUSIVE_LOCKS_REQUIRED(this->lock()) ;

private:
struct Impl;
Impl* _impl;
};

Can I express the locks in terms of _impl ?

– Matthieu

Your analysis recognizes arbitrary expressions in these contexts as long
as they resolve to something that works like a lock, right? Why not also,
as an alternative, permit arbitrary expressions that happen to resolve
to calls to methods with the THREAD_SAFETY_ACQUIRES_LOCK
attribute? e.g.
  THREAD_SAFETY_REQUIRES_LOCK(lock())
In other words, I am simply proposing that you accept an alternate way
of specifying the lock; this should not deeply affect your algorithm.

Yes. I think this is a good idea, and it's similar to what David
Blakie proposed. However, I think this should be an alternative
mechanism for referring to locks, not the default. The biggest
problem with referencing the lock() function directly is that locking
is a side-effect. Functions often do much more than acquire a lock,
and multiple locks can be acquired in one function. The example I
gave was very simple. In more complex examples, you have functions
like:

  StatusCode initializeTableAndLock() EXCLUSIVE_LOCK_FUNCTION(mu1, mu2) { ... }

Calling initializeTableAndLock() directly within an attribute is
nonsense, since attribute expressions are supposed to be side-effect
free. The attribute syntax thus becomes something like:

  EXCLUSIVE_LOCKS_REQUIRED(locks_acquired_by(initializeTableAndLock))

Which is a bit unwieldy. We have a fairly large user base here at
google, and in my experience, it's usually best to separate the
mutexes from functions which lock them. In fact, in the vast majority
of my existing use cases, there's no lock method at all -- I put
lock() and unlock() methods in my example mainly for illustrative
purposes, so that people could see how the annotations work. In most
cases, the mutex is acquired and released directly from within
internal methods, like addTaskSync().

Sure, but why not befriend the helper class or function if it's going to
rely on internal details of this other class?

I think this is an argument in favor of keeping access control turned
on. Is that what you mean?

C++ does provide existing mechanisms (friends, public getters, etc.)
to provide access to other code where necessary. Right now I require
users to go through those mechanisms. However, I've received some
requests from users to turn off access control, because they have
found it to be a pain, and they believe thread-safety annotations are
a special case that should not be subject to access control
restrictions.

Part of the motivation is that users typically just want to annotate
their existing code. They want the annotations to work; they don't
want to refactor their code, make things public that were not
previously public, or add friend declarations just to make the
annotations happy.

  -DeLesley

It seems that the heart of the problem is that you have a region
controlled by a mutex, and you're using the name of that mutex as a
proxy for the name of the region, but access to the region does not
necessarily imply access to the mutex. I think the right solution is
to find another (accessible) way to name the region, not to provide
access to the mutex.

So I show a workaround that does exactly this in my original post,
using the LOCK_RETURNED attribute. Do you object to that particular
way of doing things?

If your class only has one such region, it wouldn't seem unreasonable
to me to use the instance of the class as a proxy for your region name
in most cases (which I believe the thread safety annotations already
support).

Yes, you can, and I used to advocate that as the correct solution for
simple cases. There is one problem, though. I have a few extensions
that let you quantify over a mutex where the instance object is not
available, in order to get around lexical scoping limitations. Those
extensions don't currently work if the mutex isn't explicitly named,
so I would need yet more fanciness in the mutex expression language.

we should provide a mechanism for classes to expose a name
for their lockable regions as part of their public API, and I don't
think those names should be required to be tied to the names of the
mutexes

We do this already, using the LOCK_RETURNED attribute. The real
question in my original post is whether LOCK_RETURNED is an acceptable
workaround.

Are there reasonable cases where it's reasonable to put a
LOCKS_REQUIRED annotation on a function which doesn't have access to
the mutex it requires, and also doesn't have access to a mechanism
which acquires it? I could imagine such cases arising in templates...

You don't need to go as far as templates. All you need is an external
helper function, that that does not have access to the lock itself,
but is always called from code which does have access to the lock.
Locking requirements propagate, so the helper function can be
arbitrarily far away from the code that calls it.

  -DeLesley

Actually, it seems annoying to me that lock and unlock are exposing the name
of the mutex, since it's private, but then it would probably be exposed
anyway so...

We don't need to expose the name of the mutex. If you provide a
public getter with LOCK_RETURNED, and always use the getter, then the
mutex is always named through a public method.

In general, I don't like using lock() as the way to name the mutex,
because in most of my real-world use cases, lock() doesn't exist, or
does something other than just locking. However, I am quite happy to
use a public getter method, and to require users to do so, if people
agree that that's the appropriate solution.

However, this made me realize there is a small issue with the safety
analysis as it stands: I cannot see how to express the contracts of lock and
unlock in the following PIMPL case.

Funny you should bring this up. I have this exact problem in a number
of real-world cases here at google. :slight_smile: You have to do something like
the following, and it is not always as easy to factor out as this
example would suggest:

class Queue {
public:
  Queue(); Queue(Queue const&); Queue(Queue&&); Queue& operator=(Queue);
  ~Queue();

  const Mutex& getMutex(Impl* i);

  void push(int) EXCLUSIVE_LOCKS_REQUIRED(getMutex(_impl));
  int pop() EXCLUSIVE_LOCKS_REQUIRED(getMutex(_impl));

  private:
    struct Impl;
    Impl* _impl;
};

// in .cpp file

struct Impl {
  ...
  Mutex mu;
};

const Mutex& Queue::getMutex(Impl* i) LOCK_RETURNED(i->mu) {
  return i->mu;
}

void Queue::push(int) { ... }
int Queue::pop() { ... }

  -DeLesley

You asked for opinions about the language design, and I offered mine.
Like you've already admitted, this is an artificial problem brought on by
the way that the attributes specify shared regions by constructing
references to a lock, rather than (e.g.) using an arbitrary label.
(Although of course an arbitrary-label design would have false-negative
problems.) Presumably your user base is already large enough that
you're stuck with this schema, though, and it's not worth presenting an
alternate one.

Ultimately, I don't have any responsibility to your users, and you do.
If you feel that it's just punishing your users to enable access control
in thread-safety annotations, then go ahead and turn it off.

John.

Thanks to everyone who has contributed to the discussion! Since I've
received a lot of suggestions for extensions that would avoid the need
to refer to private mutexes by name, I get the impression that people
are not comfortable with turning access control off. Is that a valid
perception? Does anybody want to take a stab at arguing that it
should be turned off? :slight_smile:

I also wanted to post a slightly different example, to further clarify
the issue. This example does not have lock() and unlock() functions,
because locking is always done internal to the class; it thus better
represents many of my real-world use cases. Also, my original
workaround (using LOCK_RETURNED) seems to have gotten buried in the
depths of my original post, so I show it in its entirety here. Note
in particular that TaskSet is not a friend class, so
TaskSet::addToQueue is not able to lock or unlock q.mu_. However, it
does require that q.mu_ is held, so the attribute uses the const
getter: q.getMu().

What do people think of this workaround? Does it seem reasonable?

class TaskSet;

class TaskQueue {
public:
  // makes mu_ visible, but does not allow external code to lock or unlock it.
  const Mutex& getMu() LOCK_RETURNED(mu_) { return mu_; }

  virtual void addTask(int taskID) EXCLUSIVE_LOCKS_REQUIRED(getMu()) {
    queue.push_back(taskID);
  }

  void addTaskSync(int taskID) {
    mu_.Lock();
    addTask(taskID);
    mu_.Unlock();
  }

  void addTaskSetSync(TaskSet& set);

private:
  Mutex mu_;
  std::deque<int> queue GUARDED_BY(mu_);
};

class TaskSet {
public:
  // Adds tasks to queue consecutively, without releasing the lock
between additions.
  void addToQueue(TaskQueue& q) EXCLUSIVE_LOCKS_REQUIRED(q.getMu()) {
    for (unsigned i=0, n=tasks_.size(); i<n; ++i)
      q.addTask(tasks_[i]);
  }

private:
  std::vector<int> tasks_;
};

void TaskQueue::addTaskSetSync(TaskSet& set) {
  mu_.Lock();
  set.addToQueue(*this);
  mu_.Unlock();
}

You asked for opinions about the language design, and I offered mine.

I appreciate your input -- I got some good ideas for new features from
the discussion. Thanks!

Ultimately, I don't have any responsibility to your users, and you do.
If you feel that it's just punishing your users to enable access control
in thread-safety annotations, then go ahead and turn it off.

I do have a responsibility to my users, but I also have a
responsibility to the open source community. I personally have mixed
feelings. I want my users to be happy, but I also want thread-safety
attributes to be a well-designed language extension that follows the
rules of C++. I figure cfe-dev is the right place to get opinions on
the latter; if the clang community thinks that keeping access control
turned on is the right idea, then I will keep it on, and tell my users
to use the workaround.

BTW, if it seems like I am arguing both sides of the case, I am; my
objective here is to foster discussion, and see what people think.
:slight_smile:

Thanks again,

  -DeLesley