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

[David Blaikie]

On Wed, Oct 31, 2012 at 4:12 PM, Delesley Hutchins <delesley at google.com> wrote:
>> 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.  :-)  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.  :-)

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.