[Mesa-dev] Static thread safety checking with clang

[Kristian Høgsberg]

On Tue, Nov 10, 2020 at 8:00 PM Chia-I Wu <olvaffe at gmail.com> wrote:
>
> On Tue, Nov 10, 2020 at 8:09 AM Kristian Høgsberg <hoegsberg at gmail.com> wrote:
> >
> > 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.
>
> fd_screen_lock_cap seems to be a placeholder here.  Is it one of the
> quirks?  It would be nice if the error message reads "... requiring
> hold lock '&screen->lock' exclusively".
>
> From https://clang.llvm.org/docs/ThreadSafetyAnalysis.html#mutex-h,
> wrapping of the threading api is expected.  I wonder if we can
> annotate simple_mtx_t (and other in-tree locking wrappers), and
> perhaps kepp lock_cap_t only for code that uses mtx_t or
> pthread_mutex_t directly.

I couldn't get the guarded_by attribute to work with anything other
than a global variable. I suspect for C++ there's an implicit 'this'
in play that allows it to refer to class members. Similar for the
other attributes that refer to capabilities.

> > 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.
> 100% agreed.
>
> >
> > Kristian
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev