| Message ID | 20220824153901.488576-16-irogers@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | Mutex wrapper, locking and memory leak fixes | expand |
| Context | Check | Description |
|---|---|---|
| bpf/vmtest-bpf-PR | fail | merge-conflict |
| netdev/tree_selection | success | Not a local patch |
On 24/08/22 18:38, Ian Rogers wrote: > Add thread safety annotations to struct mutex so that when compiled with > clang's -Wthread-safety warnings are generated for erroneous lock > patterns. NO_THREAD_SAFETY_ANALYSIS is needed for > mutex_lock/mutex_unlock as the analysis doesn't under pthread calls. > > Signed-off-by: Ian Rogers <irogers@google.com> > --- > tools/perf/util/mutex.c | 2 ++ > tools/perf/util/mutex.h | 72 ++++++++++++++++++++++++++++++++++++++--- > 2 files changed, 69 insertions(+), 5 deletions(-) > > diff --git a/tools/perf/util/mutex.c b/tools/perf/util/mutex.c > index 892294ac1769..ec813093276d 100644 > --- a/tools/perf/util/mutex.c > +++ b/tools/perf/util/mutex.c > @@ -50,11 +50,13 @@ void mutex_destroy(struct mutex *mtx) > } > > void mutex_lock(struct mutex *mtx) > + NO_THREAD_SAFETY_ANALYSIS > { > CHECK_ERR(pthread_mutex_lock(&mtx->lock)); > } > > void mutex_unlock(struct mutex *mtx) > + NO_THREAD_SAFETY_ANALYSIS > { > CHECK_ERR(pthread_mutex_unlock(&mtx->lock)); > } > diff --git a/tools/perf/util/mutex.h b/tools/perf/util/mutex.h > index c9e110a2b55e..48a2d87598f0 100644 > --- a/tools/perf/util/mutex.h > +++ b/tools/perf/util/mutex.h > @@ -5,11 +5,73 @@ > #include <pthread.h> > #include <stdbool.h> > > +/* > + * A function-like feature checking macro that is a wrapper around > + * `__has_attribute`, which is defined by GCC 5+ and Clang and evaluates to a > + * nonzero constant integer if the attribute is supported or 0 if not. > + */ > +#ifdef __has_attribute > +#define HAVE_ATTRIBUTE(x) __has_attribute(x) > +#else > +#define HAVE_ATTRIBUTE(x) 0 > +#endif > + > + Multiple blank lines > +#if HAVE_ATTRIBUTE(guarded_by) && HAVE_ATTRIBUTE(pt_guarded_by) && \ > + HAVE_ATTRIBUTE(lockable) && HAVE_ATTRIBUTE(exclusive_lock_function) && \ > + HAVE_ATTRIBUTE(exclusive_trylock_function) && HAVE_ATTRIBUTE(exclusive_locks_required) && \ > + HAVE_ATTRIBUTE(no_thread_safety_analysis) > + > +/* Documents if a shared field or global variable needs to be protected by a mutex. */ > +#define GUARDED_BY(x) __attribute__((guarded_by(x))) > + > +/* > + * Documents if the memory location pointed to by a pointer should be guarded by > + * a mutex when dereferencing the pointer. > + */ > +#define PT_GUARDED_BY(x) __attribute__((pt_guarded_by(x))) > + > +/* Documents if a type is a lockable type. */ > +#define LOCKABLE __attribute__((capability("lockable"))) > + > +/* Documents functions that acquire a lock in the body of a function, and do not release it. */ > +#define EXCLUSIVE_LOCK_FUNCTION(...) __attribute__((exclusive_lock_function(__VA_ARGS__))) > + > +/* > + * Documents functions that expect a lock to be held on entry to the function, > + * and release it in the body of the function. > + */ > +#define UNLOCK_FUNCTION(...) __attribute__((unlock_function(__VA_ARGS__))) > + > +/* Documents functions that try to acquire a lock, and return success or failure. */ > +#define EXCLUSIVE_TRYLOCK_FUNCTION(...) \ > + __attribute__((exclusive_trylock_function(__VA_ARGS__))) > + > + Multiple blank lines > +/* Documents a function that expects a mutex to be held prior to entry. */ > +#define EXCLUSIVE_LOCKS_REQUIRED(...) __attribute__((exclusive_locks_required(__VA_ARGS__))) > + > +/* Turns off thread safety checking within the body of a particular function. */ > +#define NO_THREAD_SAFETY_ANALYSIS __attribute__((no_thread_safety_analysis)) > + > +#else > + > +#define GUARDED_BY(x) > +#define PT_GUARDED_BY(x) > +#define LOCKABLE > +#define EXCLUSIVE_LOCK_FUNCTION(...) > +#define UNLOCK_FUNCTION(...) > +#define EXCLUSIVE_TRYLOCK_FUNCTION(...) > +#define EXCLUSIVE_LOCKS_REQUIRED(...) > +#define NO_THREAD_SAFETY_ANALYSIS > + > +#endif > + > /* > * A wrapper around the mutex implementation that allows perf to error check > * usage, etc. > */ > -struct mutex { > +struct LOCKABLE mutex { > pthread_mutex_t lock; > }; > > @@ -27,9 +89,9 @@ void mutex_init(struct mutex *mtx); > void mutex_init_pshared(struct mutex *mtx); > void mutex_destroy(struct mutex *mtx); > > -void mutex_lock(struct mutex *mtx); > -void mutex_unlock(struct mutex *mtx); > -bool mutex_trylock(struct mutex *mtx); > +void mutex_lock(struct mutex *mtx) EXCLUSIVE_LOCK_FUNCTION(*mtx); > +void mutex_unlock(struct mutex *mtx) UNLOCK_FUNCTION(*mtx); > +bool mutex_trylock(struct mutex *mtx) EXCLUSIVE_TRYLOCK_FUNCTION(true, *mtx); > > /* Default initialize the cond struct. */ > void cond_init(struct cond *cnd); > @@ -40,7 +102,7 @@ void cond_init(struct cond *cnd); > void cond_init_pshared(struct cond *cnd); > void cond_destroy(struct cond *cnd); > > -void cond_wait(struct cond *cnd, struct mutex *mtx); > +void cond_wait(struct cond *cnd, struct mutex *mtx) EXCLUSIVE_LOCKS_REQUIRED(mtx); > void cond_signal(struct cond *cnd); > void cond_broadcast(struct cond *cnd); >
Em Wed, Aug 24, 2022 at 08:38:58AM -0700, Ian Rogers escreveu: > Add thread safety annotations to struct mutex so that when compiled with > clang's -Wthread-safety warnings are generated for erroneous lock > patterns. NO_THREAD_SAFETY_ANALYSIS is needed for > mutex_lock/mutex_unlock as the analysis doesn't under pthread calls. So even having the guards checking if the attribute is available it seems at least clang 11.0 is needed, as the "lockable" arg was introduced there: 37 42.61 fedora:32 : FAIL clang version 10.0.1 (Fedora 10.0.1-3.fc32) In file included from /git/perf-6.0.0-rc2/tools/perf/util/../ui/ui.h:5: /git/perf-6.0.0-rc2/tools/perf/util/../ui/../util/mutex.h:74:8: error: invalid capability name 'lockable'; capability name must be 'mutex' or 'role' [-Werror,-Wthread-safety-attributes] struct LOCKABLE mutex { ^ /git/perf-6.0.0-rc2/tools/perf/util/../ui/../util/mutex.h:35:44: note: expanded from macro 'LOCKABLE' #define LOCKABLE __attribute__((capability("lockable"))) The next fedora releases are ok with it: 38 124.89 fedora:33 : Ok gcc (GCC) 10.3.1 20210422 (Red Hat 10.3.1-1) , clang version 11.0.0 (Fedora 11.0.0-3.fc33) 39 132.20 fedora:34 : Ok gcc (GCC) 11.3.1 20220421 (Red Hat 11.3.1-2) , clang version 12.0.1 (Fedora 12.0.1-1.fc34) 40 21.44 fedora:34-x-ARC-glibc : Ok arc-linux-gcc (ARC HS GNU/Linux glibc toolchain 2019.03-rc1) 8.3.1 20190225 41 19.43 fedora:34-x-ARC-uClibc : Ok arc-linux-gcc (ARCv2 ISA Linux uClibc toolchain 2019.03-rc1) 8.3.1 20190225 42 136.33 fedora:35 : Ok gcc (GCC) 11.3.1 20220421 (Red Hat 11.3.1-2) , clang version 13.0.0 (Fedora 13.0.0-3.fc35) 43 137.73 fedora:36 : Ok gcc (GCC) 12.1.1 20220507 (Red Hat 12.1.1-1) , clang version 14.0.0 (Fedora 14.0.0-1.fc36) 44 140.45 fedora:37 : Ok gcc (GCC) 12.1.1 20220628 (Red Hat 12.1.1-3) , clang version 14.0.5 (Fedora 14.0.5-6.fc37) 45 126.80 fedora:38 : Ok gcc (GCC) 12.1.1 20220810 (Red Hat 12.1.1-4) , clang version 14.0.5 (Fedora 14.0.5-6.fc38) 46 127.00 fedora:rawhide : Ok gcc (GCC) 12.1.1 20220810 (Red Hat 12.1.1-4) , clang version 14.0.5 (Fedora 14.0.5-6.fc38) Is there a way to check if a "capability" is available for the attribute? Otherwise we can additionally check the clang version? For my tests I'll make clang 11.0 to be the oldest supported compiler (i.e. just disable building with older versions), but wanted to let you know since you try to check if the attribute is available and fallback to doing nothing in that case. - Arnaldo > Signed-off-by: Ian Rogers <irogers@google.com> > --- > tools/perf/util/mutex.c | 2 ++ > tools/perf/util/mutex.h | 72 ++++++++++++++++++++++++++++++++++++++--- > 2 files changed, 69 insertions(+), 5 deletions(-) > > diff --git a/tools/perf/util/mutex.c b/tools/perf/util/mutex.c > index 892294ac1769..ec813093276d 100644 > --- a/tools/perf/util/mutex.c > +++ b/tools/perf/util/mutex.c > @@ -50,11 +50,13 @@ void mutex_destroy(struct mutex *mtx) > } > > void mutex_lock(struct mutex *mtx) > + NO_THREAD_SAFETY_ANALYSIS > { > CHECK_ERR(pthread_mutex_lock(&mtx->lock)); > } > > void mutex_unlock(struct mutex *mtx) > + NO_THREAD_SAFETY_ANALYSIS > { > CHECK_ERR(pthread_mutex_unlock(&mtx->lock)); > } > diff --git a/tools/perf/util/mutex.h b/tools/perf/util/mutex.h > index c9e110a2b55e..48a2d87598f0 100644 > --- a/tools/perf/util/mutex.h > +++ b/tools/perf/util/mutex.h > @@ -5,11 +5,73 @@ > #include <pthread.h> > #include <stdbool.h> > > +/* > + * A function-like feature checking macro that is a wrapper around > + * `__has_attribute`, which is defined by GCC 5+ and Clang and evaluates to a > + * nonzero constant integer if the attribute is supported or 0 if not. > + */ > +#ifdef __has_attribute > +#define HAVE_ATTRIBUTE(x) __has_attribute(x) > +#else > +#define HAVE_ATTRIBUTE(x) 0 > +#endif > + > + > +#if HAVE_ATTRIBUTE(guarded_by) && HAVE_ATTRIBUTE(pt_guarded_by) && \ > + HAVE_ATTRIBUTE(lockable) && HAVE_ATTRIBUTE(exclusive_lock_function) && \ > + HAVE_ATTRIBUTE(exclusive_trylock_function) && HAVE_ATTRIBUTE(exclusive_locks_required) && \ > + HAVE_ATTRIBUTE(no_thread_safety_analysis) > + > +/* Documents if a shared field or global variable needs to be protected by a mutex. */ > +#define GUARDED_BY(x) __attribute__((guarded_by(x))) > + > +/* > + * Documents if the memory location pointed to by a pointer should be guarded by > + * a mutex when dereferencing the pointer. > + */ > +#define PT_GUARDED_BY(x) __attribute__((pt_guarded_by(x))) > + > +/* Documents if a type is a lockable type. */ > +#define LOCKABLE __attribute__((capability("lockable"))) > + > +/* Documents functions that acquire a lock in the body of a function, and do not release it. */ > +#define EXCLUSIVE_LOCK_FUNCTION(...) __attribute__((exclusive_lock_function(__VA_ARGS__))) > + > +/* > + * Documents functions that expect a lock to be held on entry to the function, > + * and release it in the body of the function. > + */ > +#define UNLOCK_FUNCTION(...) __attribute__((unlock_function(__VA_ARGS__))) > + > +/* Documents functions that try to acquire a lock, and return success or failure. */ > +#define EXCLUSIVE_TRYLOCK_FUNCTION(...) \ > + __attribute__((exclusive_trylock_function(__VA_ARGS__))) > + > + > +/* Documents a function that expects a mutex to be held prior to entry. */ > +#define EXCLUSIVE_LOCKS_REQUIRED(...) __attribute__((exclusive_locks_required(__VA_ARGS__))) > + > +/* Turns off thread safety checking within the body of a particular function. */ > +#define NO_THREAD_SAFETY_ANALYSIS __attribute__((no_thread_safety_analysis)) > + > +#else > + > +#define GUARDED_BY(x) > +#define PT_GUARDED_BY(x) > +#define LOCKABLE > +#define EXCLUSIVE_LOCK_FUNCTION(...) > +#define UNLOCK_FUNCTION(...) > +#define EXCLUSIVE_TRYLOCK_FUNCTION(...) > +#define EXCLUSIVE_LOCKS_REQUIRED(...) > +#define NO_THREAD_SAFETY_ANALYSIS > + > +#endif > + > /* > * A wrapper around the mutex implementation that allows perf to error check > * usage, etc. > */ > -struct mutex { > +struct LOCKABLE mutex { > pthread_mutex_t lock; > }; > > @@ -27,9 +89,9 @@ void mutex_init(struct mutex *mtx); > void mutex_init_pshared(struct mutex *mtx); > void mutex_destroy(struct mutex *mtx); > > -void mutex_lock(struct mutex *mtx); > -void mutex_unlock(struct mutex *mtx); > -bool mutex_trylock(struct mutex *mtx); > +void mutex_lock(struct mutex *mtx) EXCLUSIVE_LOCK_FUNCTION(*mtx); > +void mutex_unlock(struct mutex *mtx) UNLOCK_FUNCTION(*mtx); > +bool mutex_trylock(struct mutex *mtx) EXCLUSIVE_TRYLOCK_FUNCTION(true, *mtx); > > /* Default initialize the cond struct. */ > void cond_init(struct cond *cnd); > @@ -40,7 +102,7 @@ void cond_init(struct cond *cnd); > void cond_init_pshared(struct cond *cnd); > void cond_destroy(struct cond *cnd); > > -void cond_wait(struct cond *cnd, struct mutex *mtx); > +void cond_wait(struct cond *cnd, struct mutex *mtx) EXCLUSIVE_LOCKS_REQUIRED(mtx); > void cond_signal(struct cond *cnd); > void cond_broadcast(struct cond *cnd); > > -- > 2.37.2.609.g9ff673ca1a-goog
On Fri, Aug 26, 2022 at 9:45 AM Arnaldo Carvalho de Melo <acme@kernel.org> wrote: > > Em Wed, Aug 24, 2022 at 08:38:58AM -0700, Ian Rogers escreveu: > > Add thread safety annotations to struct mutex so that when compiled with > > clang's -Wthread-safety warnings are generated for erroneous lock > > patterns. NO_THREAD_SAFETY_ANALYSIS is needed for > > mutex_lock/mutex_unlock as the analysis doesn't under pthread calls. > > So even having the guards checking if the attribute is available it > seems at least clang 11.0 is needed, as the "lockable" arg was > introduced there: > > 37 42.61 fedora:32 : FAIL clang version 10.0.1 (Fedora 10.0.1-3.fc32) > In file included from /git/perf-6.0.0-rc2/tools/perf/util/../ui/ui.h:5: > /git/perf-6.0.0-rc2/tools/perf/util/../ui/../util/mutex.h:74:8: error: invalid capability name 'lockable'; capability name must be 'mutex' or 'role' [-Werror,-Wthread-safety-attributes] > struct LOCKABLE mutex { > ^ > /git/perf-6.0.0-rc2/tools/perf/util/../ui/../util/mutex.h:35:44: note: expanded from macro 'LOCKABLE' > #define LOCKABLE __attribute__((capability("lockable"))) capability("lockable") can just be lockable, the capability generalization came in a later clang release. That is change: #define LOCKABLE __attribute__((capability("lockable"))) to: #define LOCKABLE __attribute__((lockable)) and later clangs take the earlier name. Do you want a v5 for this 1 liner? Thanks, Ian > The next fedora releases are ok with it: > > 38 124.89 fedora:33 : Ok gcc (GCC) 10.3.1 20210422 (Red Hat 10.3.1-1) , clang version 11.0.0 (Fedora 11.0.0-3.fc33) > 39 132.20 fedora:34 : Ok gcc (GCC) 11.3.1 20220421 (Red Hat 11.3.1-2) , clang version 12.0.1 (Fedora 12.0.1-1.fc34) > 40 21.44 fedora:34-x-ARC-glibc : Ok arc-linux-gcc (ARC HS GNU/Linux glibc toolchain 2019.03-rc1) 8.3.1 20190225 > 41 19.43 fedora:34-x-ARC-uClibc : Ok arc-linux-gcc (ARCv2 ISA Linux uClibc toolchain 2019.03-rc1) 8.3.1 20190225 > 42 136.33 fedora:35 : Ok gcc (GCC) 11.3.1 20220421 (Red Hat 11.3.1-2) , clang version 13.0.0 (Fedora 13.0.0-3.fc35) > 43 137.73 fedora:36 : Ok gcc (GCC) 12.1.1 20220507 (Red Hat 12.1.1-1) , clang version 14.0.0 (Fedora 14.0.0-1.fc36) > 44 140.45 fedora:37 : Ok gcc (GCC) 12.1.1 20220628 (Red Hat 12.1.1-3) , clang version 14.0.5 (Fedora 14.0.5-6.fc37) > 45 126.80 fedora:38 : Ok gcc (GCC) 12.1.1 20220810 (Red Hat 12.1.1-4) , clang version 14.0.5 (Fedora 14.0.5-6.fc38) > 46 127.00 fedora:rawhide : Ok gcc (GCC) 12.1.1 20220810 (Red Hat 12.1.1-4) , clang version 14.0.5 (Fedora 14.0.5-6.fc38) > > Is there a way to check if a "capability" is available for the > attribute? Otherwise we can additionally check the clang version? > > For my tests I'll make clang 11.0 to be the oldest supported compiler > (i.e. just disable building with older versions), but wanted to let you > know since you try to check if the attribute is available and fallback > to doing nothing in that case. > > - Arnaldo > > > Signed-off-by: Ian Rogers <irogers@google.com> > > --- > > tools/perf/util/mutex.c | 2 ++ > > tools/perf/util/mutex.h | 72 ++++++++++++++++++++++++++++++++++++++--- > > 2 files changed, 69 insertions(+), 5 deletions(-) > > > > diff --git a/tools/perf/util/mutex.c b/tools/perf/util/mutex.c > > index 892294ac1769..ec813093276d 100644 > > --- a/tools/perf/util/mutex.c > > +++ b/tools/perf/util/mutex.c > > @@ -50,11 +50,13 @@ void mutex_destroy(struct mutex *mtx) > > } > > > > void mutex_lock(struct mutex *mtx) > > + NO_THREAD_SAFETY_ANALYSIS > > { > > CHECK_ERR(pthread_mutex_lock(&mtx->lock)); > > } > > > > void mutex_unlock(struct mutex *mtx) > > + NO_THREAD_SAFETY_ANALYSIS > > { > > CHECK_ERR(pthread_mutex_unlock(&mtx->lock)); > > } > > diff --git a/tools/perf/util/mutex.h b/tools/perf/util/mutex.h > > index c9e110a2b55e..48a2d87598f0 100644 > > --- a/tools/perf/util/mutex.h > > +++ b/tools/perf/util/mutex.h > > @@ -5,11 +5,73 @@ > > #include <pthread.h> > > #include <stdbool.h> > > > > +/* > > + * A function-like feature checking macro that is a wrapper around > > + * `__has_attribute`, which is defined by GCC 5+ and Clang and evaluates to a > > + * nonzero constant integer if the attribute is supported or 0 if not. > > + */ > > +#ifdef __has_attribute > > +#define HAVE_ATTRIBUTE(x) __has_attribute(x) > > +#else > > +#define HAVE_ATTRIBUTE(x) 0 > > +#endif > > + > > + > > +#if HAVE_ATTRIBUTE(guarded_by) && HAVE_ATTRIBUTE(pt_guarded_by) && \ > > + HAVE_ATTRIBUTE(lockable) && HAVE_ATTRIBUTE(exclusive_lock_function) && \ > > + HAVE_ATTRIBUTE(exclusive_trylock_function) && HAVE_ATTRIBUTE(exclusive_locks_required) && \ > > + HAVE_ATTRIBUTE(no_thread_safety_analysis) > > + > > +/* Documents if a shared field or global variable needs to be protected by a mutex. */ > > +#define GUARDED_BY(x) __attribute__((guarded_by(x))) > > + > > +/* > > + * Documents if the memory location pointed to by a pointer should be guarded by > > + * a mutex when dereferencing the pointer. > > + */ > > +#define PT_GUARDED_BY(x) __attribute__((pt_guarded_by(x))) > > + > > +/* Documents if a type is a lockable type. */ > > +#define LOCKABLE __attribute__((capability("lockable"))) > > + > > +/* Documents functions that acquire a lock in the body of a function, and do not release it. */ > > +#define EXCLUSIVE_LOCK_FUNCTION(...) __attribute__((exclusive_lock_function(__VA_ARGS__))) > > + > > +/* > > + * Documents functions that expect a lock to be held on entry to the function, > > + * and release it in the body of the function. > > + */ > > +#define UNLOCK_FUNCTION(...) __attribute__((unlock_function(__VA_ARGS__))) > > + > > +/* Documents functions that try to acquire a lock, and return success or failure. */ > > +#define EXCLUSIVE_TRYLOCK_FUNCTION(...) \ > > + __attribute__((exclusive_trylock_function(__VA_ARGS__))) > > + > > + > > +/* Documents a function that expects a mutex to be held prior to entry. */ > > +#define EXCLUSIVE_LOCKS_REQUIRED(...) __attribute__((exclusive_locks_required(__VA_ARGS__))) > > + > > +/* Turns off thread safety checking within the body of a particular function. */ > > +#define NO_THREAD_SAFETY_ANALYSIS __attribute__((no_thread_safety_analysis)) > > + > > +#else > > + > > +#define GUARDED_BY(x) > > +#define PT_GUARDED_BY(x) > > +#define LOCKABLE > > +#define EXCLUSIVE_LOCK_FUNCTION(...) > > +#define UNLOCK_FUNCTION(...) > > +#define EXCLUSIVE_TRYLOCK_FUNCTION(...) > > +#define EXCLUSIVE_LOCKS_REQUIRED(...) > > +#define NO_THREAD_SAFETY_ANALYSIS > > + > > +#endif > > + > > /* > > * A wrapper around the mutex implementation that allows perf to error check > > * usage, etc. > > */ > > -struct mutex { > > +struct LOCKABLE mutex { > > pthread_mutex_t lock; > > }; > > > > @@ -27,9 +89,9 @@ void mutex_init(struct mutex *mtx); > > void mutex_init_pshared(struct mutex *mtx); > > void mutex_destroy(struct mutex *mtx); > > > > -void mutex_lock(struct mutex *mtx); > > -void mutex_unlock(struct mutex *mtx); > > -bool mutex_trylock(struct mutex *mtx); > > +void mutex_lock(struct mutex *mtx) EXCLUSIVE_LOCK_FUNCTION(*mtx); > > +void mutex_unlock(struct mutex *mtx) UNLOCK_FUNCTION(*mtx); > > +bool mutex_trylock(struct mutex *mtx) EXCLUSIVE_TRYLOCK_FUNCTION(true, *mtx); > > > > /* Default initialize the cond struct. */ > > void cond_init(struct cond *cnd); > > @@ -40,7 +102,7 @@ void cond_init(struct cond *cnd); > > void cond_init_pshared(struct cond *cnd); > > void cond_destroy(struct cond *cnd); > > > > -void cond_wait(struct cond *cnd, struct mutex *mtx); > > +void cond_wait(struct cond *cnd, struct mutex *mtx) EXCLUSIVE_LOCKS_REQUIRED(mtx); > > void cond_signal(struct cond *cnd); > > void cond_broadcast(struct cond *cnd); > > > > -- > > 2.37.2.609.g9ff673ca1a-goog > > -- > > - Arnaldo
Em Fri, Aug 26, 2022 at 10:00:43AM -0700, Ian Rogers escreveu: > On Fri, Aug 26, 2022 at 9:45 AM Arnaldo Carvalho de Melo > <acme@kernel.org> wrote: > > > > Em Wed, Aug 24, 2022 at 08:38:58AM -0700, Ian Rogers escreveu: > > > Add thread safety annotations to struct mutex so that when compiled with > > > clang's -Wthread-safety warnings are generated for erroneous lock > > > patterns. NO_THREAD_SAFETY_ANALYSIS is needed for > > > mutex_lock/mutex_unlock as the analysis doesn't under pthread calls. > > > > So even having the guards checking if the attribute is available it > > seems at least clang 11.0 is needed, as the "lockable" arg was > > introduced there: > > > > 37 42.61 fedora:32 : FAIL clang version 10.0.1 (Fedora 10.0.1-3.fc32) > > In file included from /git/perf-6.0.0-rc2/tools/perf/util/../ui/ui.h:5: > > /git/perf-6.0.0-rc2/tools/perf/util/../ui/../util/mutex.h:74:8: error: invalid capability name 'lockable'; capability name must be 'mutex' or 'role' [-Werror,-Wthread-safety-attributes] > > struct LOCKABLE mutex { > > ^ > > /git/perf-6.0.0-rc2/tools/perf/util/../ui/../util/mutex.h:35:44: note: expanded from macro 'LOCKABLE' > > #define LOCKABLE __attribute__((capability("lockable"))) > > > capability("lockable") can just be lockable, the capability > generalization came in a later clang release. > > That is change: > #define LOCKABLE __attribute__((capability("lockable"))) > to: > #define LOCKABLE __attribute__((lockable)) > > and later clangs take the earlier name. Do you want a v5 for this 1 liner? I did it and I'm now testing, thanks. - Arnaldo diff --git a/tools/perf/util/mutex.h b/tools/perf/util/mutex.h index 48a2d87598f0d725..29b5494b213a3fc9 100644 --- a/tools/perf/util/mutex.h +++ b/tools/perf/util/mutex.h @@ -32,7 +32,7 @@ #define PT_GUARDED_BY(x) __attribute__((pt_guarded_by(x))) /* Documents if a type is a lockable type. */ -#define LOCKABLE __attribute__((capability("lockable"))) +#define LOCKABLE __attribute__((lockable)) /* Documents functions that acquire a lock in the body of a function, and do not release it. */ #define EXCLUSIVE_LOCK_FUNCTION(...) __attribute__((exclusive_lock_function(__VA_ARGS__))) - Arnaldo
diff --git a/tools/perf/util/mutex.c b/tools/perf/util/mutex.c index 892294ac1769..ec813093276d 100644 --- a/tools/perf/util/mutex.c +++ b/tools/perf/util/mutex.c @@ -50,11 +50,13 @@ void mutex_destroy(struct mutex *mtx) } void mutex_lock(struct mutex *mtx) + NO_THREAD_SAFETY_ANALYSIS { CHECK_ERR(pthread_mutex_lock(&mtx->lock)); } void mutex_unlock(struct mutex *mtx) + NO_THREAD_SAFETY_ANALYSIS { CHECK_ERR(pthread_mutex_unlock(&mtx->lock)); } diff --git a/tools/perf/util/mutex.h b/tools/perf/util/mutex.h index c9e110a2b55e..48a2d87598f0 100644 --- a/tools/perf/util/mutex.h +++ b/tools/perf/util/mutex.h @@ -5,11 +5,73 @@ #include <pthread.h> #include <stdbool.h> +/* + * A function-like feature checking macro that is a wrapper around + * `__has_attribute`, which is defined by GCC 5+ and Clang and evaluates to a + * nonzero constant integer if the attribute is supported or 0 if not. + */ +#ifdef __has_attribute +#define HAVE_ATTRIBUTE(x) __has_attribute(x) +#else +#define HAVE_ATTRIBUTE(x) 0 +#endif + + +#if HAVE_ATTRIBUTE(guarded_by) && HAVE_ATTRIBUTE(pt_guarded_by) && \ + HAVE_ATTRIBUTE(lockable) && HAVE_ATTRIBUTE(exclusive_lock_function) && \ + HAVE_ATTRIBUTE(exclusive_trylock_function) && HAVE_ATTRIBUTE(exclusive_locks_required) && \ + HAVE_ATTRIBUTE(no_thread_safety_analysis) + +/* Documents if a shared field or global variable needs to be protected by a mutex. */ +#define GUARDED_BY(x) __attribute__((guarded_by(x))) + +/* + * Documents if the memory location pointed to by a pointer should be guarded by + * a mutex when dereferencing the pointer. + */ +#define PT_GUARDED_BY(x) __attribute__((pt_guarded_by(x))) + +/* Documents if a type is a lockable type. */ +#define LOCKABLE __attribute__((capability("lockable"))) + +/* Documents functions that acquire a lock in the body of a function, and do not release it. */ +#define EXCLUSIVE_LOCK_FUNCTION(...) __attribute__((exclusive_lock_function(__VA_ARGS__))) + +/* + * Documents functions that expect a lock to be held on entry to the function, + * and release it in the body of the function. + */ +#define UNLOCK_FUNCTION(...) __attribute__((unlock_function(__VA_ARGS__))) + +/* Documents functions that try to acquire a lock, and return success or failure. */ +#define EXCLUSIVE_TRYLOCK_FUNCTION(...) \ + __attribute__((exclusive_trylock_function(__VA_ARGS__))) + + +/* Documents a function that expects a mutex to be held prior to entry. */ +#define EXCLUSIVE_LOCKS_REQUIRED(...) __attribute__((exclusive_locks_required(__VA_ARGS__))) + +/* Turns off thread safety checking within the body of a particular function. */ +#define NO_THREAD_SAFETY_ANALYSIS __attribute__((no_thread_safety_analysis)) + +#else + +#define GUARDED_BY(x) +#define PT_GUARDED_BY(x) +#define LOCKABLE +#define EXCLUSIVE_LOCK_FUNCTION(...) +#define UNLOCK_FUNCTION(...) +#define EXCLUSIVE_TRYLOCK_FUNCTION(...) +#define EXCLUSIVE_LOCKS_REQUIRED(...) +#define NO_THREAD_SAFETY_ANALYSIS + +#endif + /* * A wrapper around the mutex implementation that allows perf to error check * usage, etc. */ -struct mutex { +struct LOCKABLE mutex { pthread_mutex_t lock; }; @@ -27,9 +89,9 @@ void mutex_init(struct mutex *mtx); void mutex_init_pshared(struct mutex *mtx); void mutex_destroy(struct mutex *mtx); -void mutex_lock(struct mutex *mtx); -void mutex_unlock(struct mutex *mtx); -bool mutex_trylock(struct mutex *mtx); +void mutex_lock(struct mutex *mtx) EXCLUSIVE_LOCK_FUNCTION(*mtx); +void mutex_unlock(struct mutex *mtx) UNLOCK_FUNCTION(*mtx); +bool mutex_trylock(struct mutex *mtx) EXCLUSIVE_TRYLOCK_FUNCTION(true, *mtx); /* Default initialize the cond struct. */ void cond_init(struct cond *cnd); @@ -40,7 +102,7 @@ void cond_init(struct cond *cnd); void cond_init_pshared(struct cond *cnd); void cond_destroy(struct cond *cnd); -void cond_wait(struct cond *cnd, struct mutex *mtx); +void cond_wait(struct cond *cnd, struct mutex *mtx) EXCLUSIVE_LOCKS_REQUIRED(mtx); void cond_signal(struct cond *cnd); void cond_broadcast(struct cond *cnd);
Add thread safety annotations to struct mutex so that when compiled with clang's -Wthread-safety warnings are generated for erroneous lock patterns. NO_THREAD_SAFETY_ANALYSIS is needed for mutex_lock/mutex_unlock as the analysis doesn't under pthread calls. Signed-off-by: Ian Rogers <irogers@google.com> --- tools/perf/util/mutex.c | 2 ++ tools/perf/util/mutex.h | 72 ++++++++++++++++++++++++++++++++++++++--- 2 files changed, 69 insertions(+), 5 deletions(-)