On Tue, Nov 10, 2020 at 8:00 PM Chia-I Wu <olva...@gmail.com> wrote: > > 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.
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@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
