static_assert, sfc, and clang issues

static_assert, sfc, and clang issues

[Stephen Hemminger]

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.

Re: static_assert, sfc, and clang issues

[Tyler Retzlaff]

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.

RE: static_assert, sfc, and clang issues

[Morten Brørup]

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

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

RE: static_assert, sfc, and clang issues

[Morten Brørup]

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

Re: static_assert, sfc, and clang issues

[Stephen Hemminger]

On Tue, 16 Jan 2024 23:14:36 +0100
Morten Brørup <mb@smartsharesystems.com> wrote:

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

In the patch series sent, already did that. Didn't need to do { } while(0) hack to make it work.
Reference is in commit message.

Re: static_assert, sfc, and clang issues

[Andrew Rybchenko]

On 1/17/24 01:49, Stephen Hemminger wrote:
> On Tue, 16 Jan 2024 23:14:36 +0100
> Morten Brørup <mb@smartsharesystems.com> wrote:
> 
>>> +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.
> 
> In the patch series sent, already did that. Didn't need to do { } while(0) hack to make it work.
> Reference is in commit message.

I'm OK with the solution in the patch series. Thanks a lot.

RE: static_assert, sfc, and clang issues

[Morten Brørup]

> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Tuesday, 16 January 2024 23.50
> 
> On Tue, 16 Jan 2024 23:14:36 +0100
> Morten Brørup <mb@smartsharesystems.com> wrote:
> 
> > > +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.
> 
> In the patch series sent, already did that. Didn't need to do { }
> while(0) hack to make it work.

It works in the problematic file, but not generally.

Without the do/while, "RTE_BUILD_BUG_ON(condition);" will expand to two lines:

{ static_assert(...); }
;

... which may be problematic when used with if/else without curly braces.

However, I do admit that this problem is probably purely theoretical.

> Reference is in commit message.

Yes, but if someone only looks at the source code, the workaround looks strange and unnecessary.

Personally, I prefer not having to dig into commit logs when reviewing code. Maybe it's just me. I'll leave the decision to you.