[libc++] thread safety annotations for unique_lock

Sorry for the incomplete mail; hit the wrong shortcut. The mail continues down below.

Sorry for the incomplete mail; hit the wrong shortcut. The mail continues
down below.

>Hi everyone,
>
>this is my first post to this list, so please remind me if I’m violating
your netiquette in some way.
>
>I’m trying to introduce the thread safety analysis to one of our projects
and stumble upon the missing thread safety annotations for unique_lock. I
thought this might be easy, so let’s provide a patch. But now I’m stuck
with correctly annotating the move constructor and the lock() and unlock()
methods of unique_lock.
>
>Here is a very basic implementation of my annotated unique_lock:
>
>class SCOPED_CAPABILITY unique_lock {
> ::bluebox::mutex *mu;
> bool lock_held;
>public:
> unique_lock() = delete;
> unique_lock(unique_lock &&) = delete; // We cannot annotate move
semantics
>
> explicit unique_lock(::bluebox::mutex &m) ACQUIRE(m) : mu(&m),
lock_held(true) { m.lock(); }
> explicit unique_lock(::bluebox::mutex *m) ACQUIRE(m) : mu(m),
lock_held(true) { m->lock(); }

Change this to ACQUIRE(mu) instead of ACQUIRE(m). The ACQUIRE and RELEASE
annotations need to refer to the same variable for the thread safety
analysis to work.

Sorry for the incomplete mail; hit the wrong shortcut. The mail continues down below.

cfe-dev@lists.llvm.org <mailto:cfe-dev@lists.llvm.org>" <cfe-dev-bounces@lists.llvm.org im Auftrag von
cfe-dev@lists.llvm.org>:

Hi everyone,

this is my first post to this list, so please remind me if I’m violating your netiquette in some way.

I’m trying to introduce the thread safety analysis to one of our projects and stumble upon the missing thread safety annotations for unique_lock. I thought this might be easy, so let’s provide a patch. But now I’m stuck with correctly annotating the move constructor

and the lock() and unlock() methods of unique_lock.

Here is a very basic implementation of my annotated unique_lock:

class SCOPED_CAPABILITY unique_lock {
::bluebox::mutex *mu;
bool lock_held;
public:
unique_lock() = delete;
unique_lock(unique_lock &&) = delete; // We cannot annotate move semantics

explicit unique_lock(::bluebox::mutex &m) ACQUIRE(m) : mu(&m), lock_held(true) { m.lock(); }
explicit unique_lock(::bluebox::mutex *m) ACQUIRE(m) : mu(m), lock_held(true) { m->lock(); }

Change this to ACQUIRE(mu) instead of ACQUIRE(m). The ACQUIRE and RELEASE annotations need
to refer to the same variable for the thread safety analysis to work.

This doesn’t work either. When putting ACQUIRE(mu) in the constructor annotation, I get this error:

$ clang++-3.8 -Wthread-safety -O0 -std=c++11 mutex.cpp
mutex.cpp:65:12: warning: writing variable 'any_number' requires holding mutex 'mtx' exclusively [-Wthread-safety-analysis]
  return ++any_number;
           ^
mutex.cpp:70:3: warning: writing variable 'any_number' requires holding mutex 'mtx' exclusively [-Wthread-safety-analysis]
  any_number = 42;
  ^
2 warnings generated.

I’ve put together a very minimalistic example which can be found at https://dl.dropboxusercontent.com/u/13595729/mutex.cpp

The basic code fragment from the example is (with the mutex and unique locker as described in my previous mail):
mutex mtx;
int any_number GUARDED_BY(mtx) = 0;

int increment() EXCLUDES(mtx) {
  unique_lock lock(mtx);
  return ++any_number; // <- First warning here
}

int main(int argc, char const *argv[]) {
  unique_lock lock(mtx);
  any_number = 42; // <- Second warning here
  lock.unlock();
  int i = increment();
  lock.lock();
  return i;
}

To me it looks like the analyzer does not get it that unique_lock::mu is essentially the same mutex as mtx.

Just for reference, the warnings are exactly the same when using pointers instead of references in unique_lock.

It looks like you are running into the limits of what has been implemented
for thread safety analysis. It doesn't look like SCOPED_CAPABILITY
supports unlocking and relocking the mutex. In these cases, the usual way
to use a locking class is to do something like:

int main(int argc, char const *argv[]) {
  {
    unique_lock lock(mtx);
    any_number = 42;
  }
  int i = increment();

  unique_lock lock(mtx);
  return i;
}

The limitations section also says that it doesn't track pointer aliases:
http://clang.llvm.org/docs/ThreadSafetyAnalysis.html#no-alias-analysis

It looks like you are running into the limits of what has been implemented for thread safety analysis. It doesn't look like SCOPED_CAPABILITY supports unlocking and relocking the mutex. In these cases, the usual way to use a locking
class is to do something like:

int main(int argc, char const *argv[]) {
{
   unique_lock lock(mtx);
   any_number = 42;
}
int i = increment();

unique_lock lock(mtx);
return i;
}

Using just a basic lock like std::lock_guard (or the MutexLocker from the documentation), we have to use condition_variable_any instead of condition_variable and hand over the bare mutex instead of std::lock_guard (or the MutexLocker). Just for the sake of completeness, here is how this is supposed to be written, if I understand it right:

// Using the already annotated libc++ here
std::mutex mtx;
std::condition_variable_any cv;

void someFunction() {
  std::lock_guard<std::mutex> lock(mtx);
  while (!active) {
    cv.wait(mtx);
  }
  // Do something...
}

If this is the intended use case, maybe there should be something in the documentation. Because this is what boost and later C++11 have tought the people (Don’t use free mutexes), at least to my understanding. I will add something to the documentation and provide a patch.

I’ve tried to understand the thread safety analysis in clang, but after an hour of looking at the code, this is still a beast and I haven’t found the handle to start off. So from my point of view, there is no short fix available. However, the static analyzer is able to track references of variables, but implementing this into the thread safety analyzer is understanding both of them. I get back to the list with a patch proposal in a year or so... :relaxed:

The limitations section also says that it doesn't track pointer aliases:
http://clang.llvm.org/docs/ThreadSafetyAnalysis.html#no-alias-analysis

After re-reading this section, it states clearly that only the example MutexLocker pattern is implemented and nothing else. So my fault, sorry.

Thanks for your help here.

-Christoph

I haven't used condition variables with thread safety analysis so I can't
comment on how well they work together. If you want to work on the thread
safety analysis in Clang, you could possibly contact Delesley and see if he
has any good starter projects to work on.