[cfe-dev] Discussion: should we enforce access control in C++ attributes?

[Richard Smith]

On Wed, Oct 31, 2012 at 4:59 PM, John McCall <rjmccall at apple.com> wrote:
> On Oct 31, 2012, at 4:38 PM, Delesley Hutchins wrote:
>>> 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.  :-)
>
> 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.

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.