static_assert, sfc, and clang issues

[Morten Brørup]

> From: Morten Brørup [mailto:mb at smartsharesystems.com]
> Sent: Tuesday, 16 January 2024 23.15
> 
> > From: Tyler Retzlaff [mailto:roretzla at 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.