On Tue, Nov 10, 2020 at 8:09 AM Kristian Høgsberg <hoegsb...@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@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-dev _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
