disable lock annotation with clang 3.4.2

  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(-)
  

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
  

> From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> Sent: Monday, 13 February 2023 15.55
> 
> 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.

I prefer this over auto-detection in meson, for the reason mentioned by Bruce.

Furthermore, different compilers may use different syntax in the code, so it is impossible to detect generically; the auto-detection would be per compiler anyway.

Acked-by: Morten Brørup <mb@smartsharesystems.com>
  

> -----Original Message-----
> From: Morten Brørup <mb@smartsharesystems.com>
> Sent: Monday, February 13, 2023 6:09 PM
> To: Bruce Richardson <bruce.richardson@intel.com>; David Marchand
> <david.marchand@redhat.com>
> Cc: dev@dpdk.org; NBU-Contact-Thomas Monjalon (EXTERNAL)
> <thomas@monjalon.net>; Raslan Darawsheh <rasland@nvidia.com>;
> Chenbo Xia <chenbo.xia@intel.com>; Maxime Coquelin
> <maxime.coquelin@redhat.com>
> Subject: RE: [PATCH] disable lock annotation with clang 3.4.2
> 
> > From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> > Sent: Monday, 13 February 2023 15.55
> >
> > 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.
> 
> I prefer this over auto-detection in meson, for the reason mentioned by
> Bruce.
> 
> Furthermore, different compilers may use different syntax in the code, so it
> is impossible to detect generically; the auto-detection would be per compiler
> anyway.
> 
> Acked-by: Morten Brørup <mb@smartsharesystems.com>
Tested-by: Raslan Darawsheh <rasland@nvidia.com>

Kindest regards,
Raslan Darawsheh
  

13/02/2023 17:36, Raslan Darawsheh:
> From: Morten Brørup <mb@smartsharesystems.com>
> > From: Bruce Richardson [mailto: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>
[...]
> > > 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.
> > 
> > I prefer this over auto-detection in meson, for the reason mentioned by
> > Bruce.
> > 
> > Furthermore, different compilers may use different syntax in the code, so it
> > is impossible to detect generically; the auto-detection would be per compiler
> > anyway.
> > 
> > Acked-by: Morten Brørup <mb@smartsharesystems.com>
> Tested-by: Raslan Darawsheh <rasland@nvidia.com>

Applied, thanks.