Re: [Xen-devel] [RFC PATCH 0/4] Add missing default labels to switch statements

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 22 Feb 2019, Andrew Cooper wrote:
> On 22/02/2019 21:00, Stefano Stabellini wrote:
> > On Fri, 22 Feb 2019, Julien Grall wrote:
> >>>>> BTW, I checked the series with -Wswitch-default:
> >>>>> -Wswitch-default
> >>>>> Warn whenever a switch statement does not have a default case.
> >>>>>> Furthermore, using BUG() is a pretty bad idea in switch. 
> >>>>> It is and not only in the switch. The reason I put BUG is that I tried
> >>>>> to follow
> >>>>> the existing "error handling" at those places.
> >>>> It is not because BUG() is been used today in some places that we need to
> >>>> continue to spread it.
> >>>>
> >>>>> Use of BUG() itself is another topic which will also need to be
> >>>>> addressed
> >>>> So we should not add more of them...
> >>> Again, I see this as a dedicated change. So, in the current series I think
> >>> it is
> >>> acceptable to use the existing way of error handling if any at all.
> >> That's not how it works in upstream. If you know some constructs are 
> >> wrong, it
> >> is best to try to address partially the problem directly then having so you
> >> reduce the amounts of change afterwards.
> >>
> >> So please try to not introduce more BUG() in the code base.
> > Hi Oleksandr, Julien,
> >
> > Julien's right that we should not introduce any more BUG()s. In fact,
> > each of them makes the code less safe, not more safe! The purpose of
> > MISRAC 16.4 is "defensive programming": write the code in a way that is
> > more (not less!) resilient to failure.
> >
> > So, I think it is a good idea to introduce a default label because it
> > can help us spot unexpected issues. Instead of calling BUG() in the
> > default handler, which is detrimental, we should return an error when
> > possible, or just print a warning.
> 
> domain_crash() is almost always better than BUG().  It is very obvious
> if it gets hit, and wont crash Xen.

That's a good suggestion.


> > As 16.4 clearly state, even a simple comment would be enough to address
> > the rule. We just need to explain why a default label is not needed.
> > Such as:
> >
> >   default:
> >   /* unreachable because blah and blah */
> 
> What a simple comment doesn't do is avoid breaking -Wswitch.

I don't know how to reconcile 16.4 with -Wswitch. One could argue that
-Wswitch could be a good way to address 16.4, but then we introduce a
compiler specific requirement. Typically gcc is not the compiler of
choice for these environments, unfortunately forcing gcc is not an
option.

But if there was a non-gcc way to do -Wswitch, then yes, that would be
the way to go.


> This requirement is actively hostile towards compilers trying to help
> you spot when you made a mistake and forgot to update one of the $N
> places you needed to.
> 
> In this case, I don't think "Because MISRA demand it" is a good enough
> justification to offset the increased error-prone-ness of the result.
> 
> ~Andrew
> 
> P.S. There is a solution here which could work, but IMO a better use of
> time and energy would be to get MISRA to update their rules to match
> this century, and stop getting in the way of compiler features intended
> to help the programmer avoid bugs.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel