From: "Morten Brørup" <mb@smartsharesystems.com>
To: "Tyler Retzlaff" <roretzla@linux.microsoft.com>,
"Stephen Hemminger" <stephen@networkplumber.org>
Cc: "Andrew Rybchenko" <andrew.rybchenko@oktetlabs.ru>, <dev@dpdk.org>
Subject: RE: static_assert, sfc, and clang issues
Date: Tue, 16 Jan 2024 23:18:20 +0100 [thread overview]
Message-ID: <98CBD80474FA8B44BF855DF32C47DC35E9F16A@smartserver.smartshare.dk> (raw)
In-Reply-To: <98CBD80474FA8B44BF855DF32C47DC35E9F169@smartserver.smartshare.dk>
> From: Morten Brørup [mailto:mb@smartsharesystems.com]
> Sent: Tuesday, 16 January 2024 23.15
>
> > From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> > Sent: Tuesday, 16 January 2024 22.51
> >
> > On Tue, Jan 16, 2024 at 09:03:01AM -0800, Stephen Hemminger wrote:
> > > Ran into a corner case issue, so sending to mailing list for wider
> > discussion.
> > >
> > > One improvement to DPDK code base planned is getting rid of
> variable
> > length arrays.
> > > VLA's can cause bugs and are not supported by the Windows compiler.
> > > Gcc and Clang have a flag to warn on use of VLA's (-Wvla).
> > >
> > > In DPDK one use of VLA's is in the RTE_BUILD_BUG_ON macro.
> > > The best path is to replace the undefined array access with a
> better
> > > static_assert() which is builtin to compilers.
> > >
> > > Using static_assert() is relatively easy, there are a few places
> > where
> > > it does detect use of non-constant expressions in existing code but
> > these
> > > are fixable.
> > >
> > > But there is one case where use static_assert() runs into a Clang
> bug
> > > which is not fixed in distros:
> > >
> > > https://github.com/llvm/llvm-project/issues/55821
> > >
> > > The code in question is in the sfc driver which for better or worse
> > > has lots of assertions. The problem is that clang (except in
> > unreleased trunk)
> > > can not handle static_assert in a switch leg.
> > >
> > > switch (actions->type) {
> > > case RTE_FLOW_ACTION_TYPE_VOID:
> > > SFC_BUILD_SET_OVERFLOW(RTE_FLOW_ACTION_TYPE_VOID,
> > > actions_set);
> > > break;
> > >
> > > ../drivers/net/sfc/sfc_flow.c:1635:4: error: expected expression
> > >
> > SFC_BUILD_SET_OVERFLOW(RTE_FLOW_ACTION_TYPE_VOID,
> > > ^
> > > ../drivers/net/sfc/sfc_flow.h:36:2: note: expanded from macro
> > 'SFC_BUILD_SET_OVERFLOW'
> > > RTE_BUILD_BUG_ON((_action) >= sizeof(_set) * CHAR_BIT)
> > > ^
> > >
> > >
> > > There are some workarounds:
> > > 0. Ignore it, works on Gcc, and Clang fix is pending.
> > > 1. Remove many of these RTE_BUILD_BUG_ON's. They are really not
> > that helpful.
> > > 2. Add additional { } to these switch cases so they become basic
> > blocks
> > > which works around the bug.
> > > 3. Move the assertions out of the switch
> > >
> > > My preference is #2 but open to other options.
> >
> > +1 for #2 just make it a block.
>
> I prefer that you implement the workaround in the RTE_BUILD_BUG_ON()
> macro, by surrounding it by "do { } while (0)", like this:
>
> #define RTE_BUILD_BUG_ON(condition) do { static_assert(!(condition),
> #condition); } while (0)
>
> And please mention the workaround reason, with the reference to the
> clang bug, in the source code comment describing the function.
Typo: "describing the function" -> "describing the RTE_BUILD_BUG_ON macro"
>
> The godbolt link in the clang issue seems happy with this workaround.
next prev parent reply other threads:[~2024-01-16 22:18 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-01-16 17:03 Stephen Hemminger
2024-01-16 21:50 ` Tyler Retzlaff
2024-01-16 22:14 ` Morten Brørup
2024-01-16 22:18 ` Morten Brørup [this message]
2024-01-16 22:49 ` Stephen Hemminger
2024-01-17 8:07 ` Andrew Rybchenko
2024-01-17 9:37 ` Morten Brørup
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=98CBD80474FA8B44BF855DF32C47DC35E9F16A@smartserver.smartshare.dk \
--to=mb@smartsharesystems.com \
--cc=andrew.rybchenko@oktetlabs.ru \
--cc=dev@dpdk.org \
--cc=roretzla@linux.microsoft.com \
--cc=stephen@networkplumber.org \
/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).