Re: [PATCH 2/2] block/graph-lock: Make WITH_GRAPH_RDLOCK_GUARD() fully checked

[Manos Pitsidianakis]

On Thu, 27 Jun 2024 at 21:13, Kevin Wolf <kwolf@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 <kwolf@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.pitsidianakis@linaro.org>



-- 
Manos Pitsidianakis
Emulation and Virtualization Engineer at Linaro Ltd