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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 15 Jan 2021, Jan Beulich wrote:
> 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.

+1