On Thu, 27 Jun 2024 at 21:13, Kevin Wolf <kw...@redhat.com> wrote:
>
> Upstream clang 18 (and backports to clang 17 in Fedora and RHEL)
> implemented support for __attribute__((cleanup())) in its Thread Safety
> Analysis, so we can now actually have a proper implementation of
> WITH_GRAPH_RDLOCK_GUARD() that understands when we acquire and when we
> release the lock.
>
> -Wthread-safety is now only enabled if the compiler is new enough to
> understand this pattern. In theory, we could have used some #ifdefs to
> keep the existing basic checks on old compilers, but as long as someone
> runs a newer compiler (and our CI does), we will catch locking problems,
> so it's probably not worth keeping multiple implementations for this.
>
> The implementation can't use g_autoptr any more because the glib macros
> define wrapper functions that don't have the right TSA attributes, so
> the compiler would complain about them. Just use the cleanup attribute
> directly instead.
>
> Signed-off-by: Kevin Wolf <kw...@redhat.com>
> ---
> include/block/graph-lock.h | 21 ++++++++++++++-------
> meson.build | 14 +++++++++++++-
> 2 files changed, 27 insertions(+), 8 deletions(-)
>
> diff --git a/include/block/graph-lock.h b/include/block/graph-lock.h
> index d7545e82d0..dc8d949184 100644
> --- a/include/block/graph-lock.h
> +++ b/include/block/graph-lock.h
> @@ -209,31 +209,38 @@ typedef struct GraphLockable { } GraphLockable;
> * unlocked. TSA_ASSERT_SHARED() makes sure that the following calls know
> that
> * we hold the lock while unlocking is left unchecked.
> */
> -static inline GraphLockable * TSA_ASSERT_SHARED(graph_lock) TSA_NO_TSA
> coroutine_fn
> +static inline GraphLockable * TSA_ACQUIRE_SHARED(graph_lock) coroutine_fn
> graph_lockable_auto_lock(GraphLockable *x)
> {
> bdrv_graph_co_rdlock();
> return x;
> }
>
> -static inline void TSA_NO_TSA coroutine_fn
> -graph_lockable_auto_unlock(GraphLockable *x)
> +static inline void TSA_RELEASE_SHARED(graph_lock) coroutine_fn
> +graph_lockable_auto_unlock(GraphLockable **x)
> {
> bdrv_graph_co_rdunlock();
> }
>
> -G_DEFINE_AUTOPTR_CLEANUP_FUNC(GraphLockable, graph_lockable_auto_unlock)
> +#define GRAPH_AUTO_UNLOCK
> __attribute__((cleanup(graph_lockable_auto_unlock)))
>
> +/*
> + * @var is only used to break the loop after the first iteration.
> + * @unlock_var can't be unlocked and then set to NULL because TSA wants the
> lock
> + * to be held at the start of every iteration of the loop.
> + */
> #define WITH_GRAPH_RDLOCK_GUARD_(var)
> \
> - for (g_autoptr(GraphLockable) var =
> graph_lockable_auto_lock(GML_OBJ_()); \
> + for (GraphLockable *unlock_var GRAPH_AUTO_UNLOCK =
> \
> + graph_lockable_auto_lock(GML_OBJ_()),
> \
> + *var = unlock_var;
> \
> var;
> \
> - graph_lockable_auto_unlock(var), var = NULL)
> + var = NULL)
>
> #define WITH_GRAPH_RDLOCK_GUARD() \
> WITH_GRAPH_RDLOCK_GUARD_(glue(graph_lockable_auto, __COUNTER__))
>
> #define GRAPH_RDLOCK_GUARD(x) \
> - g_autoptr(GraphLockable) \
> + GraphLockable * GRAPH_AUTO_UNLOCK \
> glue(graph_lockable_auto, __COUNTER__) G_GNUC_UNUSED = \
> graph_lockable_auto_lock(GML_OBJ_())
>
> diff --git a/meson.build b/meson.build
> index 97e00d6f59..b1d5ce5f1d 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -624,7 +624,19 @@ warn_flags = [
> ]
>
> if host_os != 'darwin'
> - warn_flags += ['-Wthread-safety']
> + tsa_has_cleanup = cc.compiles('''
> + struct __attribute__((capability("mutex"))) mutex {};
> + void lock(struct mutex *m) __attribute__((acquire_capability(m)));
> + void unlock(struct mutex *m) __attribute__((release_capability(m)));
> +
> + void test(void) {
> + struct mutex __attribute__((cleanup(unlock))) m;
> + lock(&m);
> + }
> + ''', args: ['-Wthread-safety', '-Werror'])
> + if tsa_has_cleanup
> + warn_flags += ['-Wthread-safety']
> + endif
> endif
>
> # Set up C++ compiler flags
> --
> 2.45.2
>
Reviewed-by: Manos Pitsidianakis <manos.pitsidiana...@linaro.org>
--
Manos Pitsidianakis
Emulation and Virtualization Engineer at Linaro Ltd
