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 _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
