static_assert, sfc, and clang issues

[Morten Brørup]

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

The godbolt link in the clang issue seems happy with this workaround.