[Mesa-dev] Static thread safety checking with clang

[Kristian Høgsberg]

Hi,

I wanted to call attention to

  https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/7529

which shows how we can use a new clang __attribute__ to statically
check locking invariants. It's probably not perfect, but more like
static type checking - there will always be cases where you can't
statically determine if the code is right or wrong, but it will be
useful and correct in most cases (like in the MR).  The attributes
have a few quirks and I suspect that they were mostly designed and
tested to work for C++ classes, but I found a way to make it work for
C.

For each type of lock, declare a lock capability:

  extern lock_cap_t fd_screen_lock_cap;

Functions that take and release that lock get annotated with:

  static inline void
  fd_screen_lock(struct fd_screen *screen)
    acquire_cap(fd_screen_lock_cap);

  static inline void
  fd_screen_unlock(struct fd_screen *screen)
    release_cap(fd_screen_lock_cap)

where acquire_cap and release_cap are #defines for an __attribute__.
One of the quirks is that the function doing the locking triggers a
warning about how it doesn't actually take the lock, so I silenced
that by adding a no_thread_safety_analysis in there.

Now whenever we have a function that expects to be called with that
lock held (we often call them foo_locked) we can add the requires_cap
annotation:

  static struct gmem_key *
  gmem_key_init(struct fd_batch *batch, bool assume_zs, bool no_scis_opt)
    requires_cap(fd_screen_lock_cap)

and calls to this function will warn or error if called from a point
where it's not statically determinable that you've taken the lock:

../../master/src/gallium/drivers/freedreno/freedreno_gmem.c:532:25:
error: calling function 'gmem_key_init' requires holding lock
'fd_screen_lock_cap' exclusively [-Werror,-Wthread-safety-analysis]
        struct gmem_key *key = gmem_key_init(batch, assume_zs, no_scis_opt);
                               ^
1 error generated.

Many functions that assert a lock is taken are better off using the
requires_cap annotation, which makes the check a compile failure
instead. For cases where it's not possible to determine statically, we
can still assert at runtime:

  static inline void
  fd_screen_assert_locked(struct fd_screen *screen)
    assert_cap(fd_screen_lock_cap)

which tells the checker to assume the lock is taken.  Finally, it's
possible to annotate struct members:

  struct fd_gmem_cache gmem_cache guarded_by(fd_screen_lock_cap);

such that any access to that field can only happen with the lock taken:

../../master/src/gallium/drivers/freedreno/freedreno_gmem.c:277:20:
error: reading variable 'gmem_cache' requires holding lock
'fd_screen_lock_cap' [-Werror,-Wthread-safety-analysis]
                        rzalloc(screen->gmem_cache.ht, struct fd_gmem_stateobj);
                                        ^
Having these annotations also helps human readers of the code by
spelling out the conventions explicitly instead of relying on _locked
suffix conventions or documentation that call out which parts of a
struct are protected by which lock. All in all this seems really
useful.

Kristian