> From: Morten Brørup [mailto:[email protected]] > Sent: Tuesday, 16 January 2024 23.15 > > > From: Tyler Retzlaff [mailto:[email protected]] > > 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.
