> -----Original Message----- > From: Morten Brørup <m...@smartsharesystems.com> > Sent: Monday, February 13, 2023 6:09 PM > To: Bruce Richardson <bruce.richard...@intel.com>; David Marchand > <david.march...@redhat.com> > Cc: dev@dpdk.org; NBU-Contact-Thomas Monjalon (EXTERNAL) > <tho...@monjalon.net>; Raslan Darawsheh <rasl...@nvidia.com>; > Chenbo Xia <chenbo....@intel.com>; Maxime Coquelin > <maxime.coque...@redhat.com> > Subject: RE: [PATCH] disable lock annotation with clang 3.4.2 > > > From: Bruce Richardson [mailto:bruce.richard...@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.march...@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 <m...@smartsharesystems.com> Tested-by: Raslan Darawsheh <rasl...@nvidia.com>
Kindest regards, Raslan Darawsheh
