Re: [PATCH] disable lock annotation with clang 3.4.2

[Bruce Richardson <bruce.richardson@intel.com>]

On Mon, Feb 13, 2023 at 03:44:55PM +0100, David Marchand wrote:
> Venerable RHEL7 clang 3.4.2 has (at least) two issues with lock
> annotations.
> 
> A first one with regards to the attribute position:
> ../lib/vhost/vhost.h:518:2: error: GCC does not allow
> 	assert_exclusive_lock attribute in this position on a
> 	function definition [-Werror,-Wgcc-compat]
>         __rte_assert_exclusive_lock(&vq->access_lock)
>         ^
> ../lib/eal/include/rte_lock_annotations.h:29:38: note: expanded
> 	from macro '__rte_assert_exclusive_lock'
>         __attribute__((assert_exclusive_lock(__VA_ARGS__)))
>                                             ^
> 
> This can be worked around by splitting and having the allocation on the
> function declaration.
> 
> But on the other hand, clang 3.4.2 does not seem to propagate those
> annotations in presence of a __builtin_expect (i.e. unlikely()), like
> for example when calling if (unlikely(rte_spinlock_trylock() == 0)).
> 
> Those annotations were only working with clang in any case, so restrict
> to clang versions newer than 3.5.0.
> 
> Fixes: 657a98f38940 ("eal: annotate spinlock, rwlock and seqlock")
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
>  drivers/meson.build | 2 +-
>  lib/meson.build     | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/meson.build b/drivers/meson.build
> index bddc4a6cc4..0618c31a69 100644
> --- a/drivers/meson.build
> +++ b/drivers/meson.build
> @@ -168,7 +168,7 @@ foreach subpath:subdirs
>          enabled_drivers += name
>          lib_name = '_'.join(['rte', class, name])
>          cflags += '-DRTE_LOG_DEFAULT_LOGTYPE=' + '.'.join([log_prefix, name])
> -        if annotate_locks and cc.has_argument('-Wthread-safety')
> +        if annotate_locks and cc.get_id() == 'clang' and cc.version().version_compare('>=3.5.0')
>              cflags += '-DRTE_ANNOTATE_LOCKS'
>              cflags += '-Wthread-safety'
>          endif

Are we likely to see any issues with this with any other compilers? Should
we look to do a built-test in meson to determine feature support rather
than checking clang versions explicitly?

On the plus side, checking clang version like this makes it clear when we
can drop the conditional.

/Bruce