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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.01.2021 13:14, Rahul Singh wrote:
> Hello,
> 
>> On 14 Jan 2021, at 11:47 pm, Stefano Stabellini <sstabellini@xxxxxxxxxx> 
>> wrote:
>>
>> On Thu, 14 Jan 2021, Jan Beulich wrote:
>>> 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.
>>
>> That is a good point, and I agree with that. Rahul, do you have access
>> to a Coverity instance to run a test? 
> 
> No I don’t have access to Coverity to run a test.What I found out that from 
> the Linux kernel mailing list Coverity understand the 
> "__attribute__((__fallthrough__))” keyword.

Okay, thanks, looks sufficient afaic.

Jan

> [1] https://lore.kernel.org/lkml/20181021182926.GB6683@xxxxxxxxx/
> [2] https://lore.kernel.org/patchwork/patch/1108577/
> 
> Regards,
> Rahul
>