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

[John McCall]

On Nov 1, 2012, at 10:27 AM, Delesley Hutchins wrote:
>> 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.

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.