[PATCH] disable lock annotation with clang 3.4.2

[PATCH] disable lock annotation with clang 3.4.2

[David Marchand]

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
diff --git a/lib/meson.build b/lib/meson.build
index 450c061d2b..2bc0932ad5 100644
--- a/lib/meson.build
+++ b/lib/meson.build
@@ -203,7 +203,7 @@ foreach l:libraries
         cflags += '-DRTE_USE_FUNCTION_VERSIONING'
     endif
     cflags += '-DRTE_LOG_DEFAULT_LOGTYPE=lib.' + l
-    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
-- 
2.39.1

Re: [PATCH] disable lock annotation with clang 3.4.2

[Bruce Richardson]

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

RE: [PATCH] disable lock annotation with clang 3.4.2

[Morten Brørup]

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

RE: [PATCH] disable lock annotation with clang 3.4.2

[Raslan Darawsheh]

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

Re: [PATCH] disable lock annotation with clang 3.4.2

[Thomas Monjalon]

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.