[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v4 08/11] xen/compiler: import 'fallthrough' keyword from linux



On 13.01.2021 00:30, Stefano Stabellini wrote:
> On Tue, 12 Jan 2021, Jan Beulich wrote:
>> On 08.01.2021 15:46, Rahul Singh wrote:
>>> -Wimplicit-fallthrough warns when a switch case falls through. Warning
>>> can be suppress by either adding a /* fallthrough */ comment, or by
>>> using a null statement: __attribute__ ((fallthrough))
>>
>> Why is the comment variant (which we use in many places already,
>> albeit with varying wording) not the route of choice?
> 
> See previous discussion:
> 
> https://marc.info/?l=xen-devel&m=160707274517270
> https://marc.info/?l=xen-devel&m=160733742810605
> https://marc.info/?l=xen-devel&m=160733852011023
> 
> We thought it would be best to introduce "fallthrough" and only resort
> to comments as a plan B. The usage of the keyword should allow GCC to do
> better checks.

Hmm, this earlier discussion was on an Arm-specific thread, and I
have to admit I can't see arguments there pro and/or con either
of the two alternatives.

>>> Define the pseudo keyword 'fallthrough' for the ability to convert the
>>> various case block /* fallthrough */ style comments to null statement
>>> "__attribute__((__fallthrough__))"
>>>
>>> In C mode, GCC supports the __fallthrough__ attribute since 7.1,
>>> the same time the warning and the comment parsing were introduced.
>>>
>>> fallthrough devolves to an empty "do {} while (0)" if the compiler
>>> version (any version less than gcc 7) does not support the attribute.
>>
>> What about Coverity? It would be nice if we wouldn't need to add
>> two separate constructs everywhere to make both compiler and static
>> code checker happy.
> 
> I don't think I fully understand your reply here: Coverity doesn't come
> into the picture. Given that GCC provides a special keyword to implement
> fallthrough, it makes sense to use it when available. When it is not
> available (e.g. clang or older GCC) we need to have an alternative to
> suppress the compiler warnings. Hence the need for this check:
> 
>   #if (!defined(__clang__) && (__GNUC__ >= 7))

I'm not sure how this interacts with Coverity. My point bringing up
that one is that whatever gets done here should _also_ result in
Coverity recognizing the fall-through as intentional, or else we'll
end up with many unwanted reports of new issues once the pseudo-
keyword gets made use of. The comment model is what we currently
use to "silence" Coverity; I'd like it to be clear up front that
any new alternative to be used is also going to "satisfy" it.

Jan



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.