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

[Julien Grall <julien.grall@xxxxxxx>]

Hi,

On 22/02/2019 12:01, Oleksandr Andrushchenko wrote:
> On 2/22/19 1:27 PM, Julien Grall wrote:
> > Hi Oleksandr,
> >
> > On 22/02/2019 11:13, Oleksandr Andrushchenko wrote:
> > > On 2/22/19 1:05 PM, Julien Grall wrote:
> > > > Hi,
> > > >
> > > > On 22/02/2019 10:27, Andrew Cooper wrote:
> > > > > On 22/02/2019 09:57, Oleksandr Andrushchenko wrote:
> > > > > > From: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
> > > > > >
> > > > > > Hello, everybody!
> > > > > >
> > > > > > We at EPAM Systems would like to present first series of patches targeting 
> > > > > > Xen
> > > > > > on ARM Functional Safety certification (ISO61508 based): implementation of
> > > > > > MISRA [1] C:2012 Rule 16.4 which requires that every switch statement has a
> > > > > > default label as a measure of defensive programming technique.
> > > > > Hang on - what?
> > > > >
> > > > > Can someone attempt to justify why actively breaking -Wswitch is going
> > > > > to result in safer/better code?
> > > > I was about to ask the same. There are quite a few cases where this series 
> > > > is going to make more difficult extending enum.
> > > Well, I am not sure I can truly defend MISRA requirements here, but I'll try
> > > to express my view on that.
> > >
> > > Let us have a look at gcc's options [1]: so, for what you are saying:
> > > "-Wswitch
> > > Warn whenever a switch statement has an index of enumerated type and lacks a 
> > > case for one or more of the named codes of that enumeration. (The presence of 
> > > a default label prevents this warning.) case labels outside the enumeration 
> > > range also provoke warnings when this option is used (even if there is a 
> > > default label). This warning is enabled by -Wall."
> > > So, if we do not have all the cases covered then this compiler's switch will 
> > > fire
> > > a warning (error). What if we have an integer as a switch's index, not an 
> > > enumeration,
> > > so then there is no easy way to handle all the cases and we have to provide 
> > > *default* statement.
> > You have a point for the integer switch.
> >
> > > What if with time enumeration changes, what if some of the case statements 
> > > get removed and so on.
> > In that case, the compiler will throw an error (Xen is built with -Werror). So 
> > for enumeration the compiler will help us to spot the missing places.
> > If you add a default case, you remove us a good way to check we actually add 
> > the new element everywhere.
> The answer is that what happens if we by any reason either by a mistake or any 
> other mean
> have a build which doesn't have -Wswitch or -Wall. I mean that everything 
> changes and having
> "default" in the source does guarantee the handling as it was intended to be.
If you consider that and ...
[...]

> >
> But what happens if you miss default handling and because of bugs in the code 
> you do not
> handle "compile time unexpected values"?
that. Then when do you put a limit between theoretical and real issue? For 
instance, what is the default case is introduced an unintended behavior?
> > > 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.

Cheers,

--
Julien Grall

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