From: Bruce Richardson <bruce.richardson@intel.com>
To: David Marchand <david.marchand@redhat.com>
Cc: dev@dpdk.org, thomas@monjalon.net, rasland@nvidia.com,
"Chenbo Xia" <chenbo.xia@intel.com>,
"Morten Brørup" <mb@smartsharesystems.com>,
"Maxime Coquelin" <maxime.coquelin@redhat.com>
Subject: Re: [PATCH] disable lock annotation with clang 3.4.2
Date: Mon, 13 Feb 2023 14:54:51 +0000 [thread overview]
Message-ID: <Y+pPO109xQPFk63Y@bricha3-MOBL.ger.corp.intel.com> (raw)
In-Reply-To: <20230213144455.520669-1-david.marchand@redhat.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
next prev parent reply other threads:[~2023-02-13 14:55 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-02-13 14:44 David Marchand
2023-02-13 14:54 ` Bruce Richardson [this message]
2023-02-13 16:09 ` Morten Brørup
2023-02-13 16:36 ` Raslan Darawsheh
2023-02-14 11:27 ` Thomas Monjalon
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Y+pPO109xQPFk63Y@bricha3-MOBL.ger.corp.intel.com \
--to=bruce.richardson@intel.com \
--cc=chenbo.xia@intel.com \
--cc=david.marchand@redhat.com \
--cc=dev@dpdk.org \
--cc=maxime.coquelin@redhat.com \
--cc=mb@smartsharesystems.com \
--cc=rasland@nvidia.com \
--cc=thomas@monjalon.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).