[Mesa-dev] Static thread safety checking with clang

[Chia-I Wu]

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.


>
> 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