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

[Matthieu Monrocq]

On Thu, Nov 1, 2012 at 2:04 AM, Richard Smith <richard at metafoo.co.uk> wrote:

> 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.
>
>
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<int> 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20121101/9a766da9/attachment.html>