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

From: Rahul Singh <Rahul.Singh@arm.com>
To: Stefano Stabellini <sstabellini@kernel.org>
Cc: Jan Beulich <jbeulich@suse.com>,
	Bertrand Marquis <Bertrand.Marquis@arm.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	George Dunlap <george.dunlap@citrix.com>,
	Ian Jackson <iwj@xenproject.org>, Julien Grall <julien@xen.org>,
	Wei Liu <wl@xen.org>,
	"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH v4 08/11] xen/compiler: import 'fallthrough' keyword from linux
Date: Fri, 15 Jan 2021 12:14:17 +0000	[thread overview]
Message-ID: <EA49F7DE-22AF-4F95-9158-0EE42519FE7A@arm.com> (raw)
In-Reply-To: <alpine.DEB.2.21.2101141546350.31265@sstabellini-ThinkPad-T480s>

Hello,

> On 14 Jan 2021, at 11:47 pm, Stefano Stabellini <sstabellini@kernel.org> 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.
If someone else can run a Coverity test than it will be very helpful.

[1] https://lore.kernel.org/lkml/20181021182926.GB6683@kroah.com/
[2] https://lore.kernel.org/patchwork/patch/1108577/

Regards,
Rahul