Skip site navigation (1)Skip section navigation (2)
Date:      Thu, 28 Aug 2014 19:02:55 +0200
From:      Ed Schouten <ed@80386.nl>
To:        freebsd-arch@freebsd.org, freebsd-toolchain@freebsd.org
Cc:        Pawel Jakub Dawidek <pjd@freebsd.org>
Subject:   Lock annotations: enable them for libpthread, libstdthreads
Message-ID:  <CAJOYFBBbzu9ouwfUk1%2B1MLW0B2y%2B5JTD9-b0%2BkiymOT=xKU0uQ@mail.gmail.com>

next in thread | raw e-mail | index | archive | help
Hello all,

As some of you may know, Clang supports some form of thread safety
analysis by means of annotations:

http://clang.llvm.org/docs/ThreadSafetyAnalysis.html

To summarize, it works as follows:

- You annotate which functions in your code (in)directly
acquire/release/depend on which locks.
- You annotate global variables and structure members to specify which
locks should be held when accessing them (guards).

You can then invoke Clang with -Wthread-safety and it will throw a
compiler error in case these invariants don't hold. For example, in
case you acquire a lock in an unannotated function and don't unlock it
before the function returns, it will generate a warning.

Couple of random observations on this feature from my side:

- It seems that the thread safety stuff was mainly designed with C++
in mind. So far I haven't been able to get guarding annotations
working for mutexes residing in structures; only global mutexes.
Still, lock/unlock flow analysis on these mutexes works fine, which
should already be a game-changer.

- We can even use this feature to mark functions like
pthread_cond_wait() as requiring the lock to be held. That's pretty
sweet.

- In userspace, almost all of the code compiled out of the box with
-Wthread-safety turned on. I found one actual bug with this enabled
(see r270749) and had to patch up a small number of files in
auditdistd and hastd to annotate functions that acquire/release locks
(see synch.h).

I've written a patch that adds #defines for the attributes that are
relevant for us (read: things that are useful for C programs) to
<sys/cdefs.h> and patched up all functions in libpthread and
libstdthreads to use these attributes where possible:

http://80386.nl/pub/20140828-lock_annotate.txt

One of my observations is that if we want to use this in C code
seriously (which I hope), we would not use it in the same way as is
done in C++ canonically. In kernel space it seems that our
locking/unlocking strategy is a lot more asymmetric and non-trivial
compared to high-level C++ programs. It is more common to have
functions that pick up a lock and don't drop it or vice versa.
Especially free()-like functions that expect the object to be locked.

This is why I decided to go for a different naming scheme for the
macros that is a bit shorter than what's used on the Clang page
mentioned above. It is also a bit more accurate for our use case.
Instead of it being "THE exclusive lock member function of a mutex",
it is a function that just happens to "lock a mutex exclusively".

My intent is to slowly push bits and pieces of this patch into HEAD
within the next week. As the existing userspace only needs a very
small number of patches to build with this flag, I am also going to
add -Wthread-safety to WARNS=6, to make sure it won't regress. I'm
also planning on MFC'ing at least the changes to <sys/cdefs.h>, so it
is at least possible to write portable code.

Be sure to take a look at the diff, play around with it and let me
know what you think.

Thanks,
-- 
Ed Schouten <ed@80386.nl>



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAJOYFBBbzu9ouwfUk1%2B1MLW0B2y%2B5JTD9-b0%2BkiymOT=xKU0uQ>