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

[Julien Grall <julien.grall@xxxxxxx>]

Hi,

On 25/02/2019 11:49, Oleksandr Andrushchenko wrote:
> On 2/25/19 1:23 PM, Julien Grall wrote:
> > Hi,
> >
> > On 25/02/2019 09:50, Oleksandr Andrushchenko wrote:
> > > On 2/22/19 11:33 PM, 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.
> > > Will you be ok to turn this particular switch on by default?
> > > Or you suggest that anyone interested to do so on their own will?
> > > If we want to turn many more warnings a bit later?
> > If we decide to fully implement the rule in Xen, then it would be nice to get 
> > the compiler helping us spotting new switch without 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, then we need to get rid of BUG() from the existing code first,
> > > not trying to solve two issues at a time: rule 16.4 and BUG().
> > > I still do think these two changes are not strictly relevant,
> > > but 16.4 just suffers from consequences of BUG() being used.
> > Why would you add more BUG()? As I said above if this is an issue, then you 
> > don't add more. We can deal with other later on.
> > So let me be clearer, I will not accept any new defensive code using BUG(). If 
> > you need rework in order to avoid BUG() in your series, then please do it. 
> > Note, this is not a request to remove the existing BUG().
> I am not defending BUG() in any way which is obviously a no-go.
> I am just trying to say that BUG() usage in the existing code needs to be
> fixed first. Once done, we can then move to "default" w/o BUG()
I am sorry but it does not make any sense. Why do you first need to fix the 
BUG() in the code before moving to "default" without BUG()?
> > > > > > 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.
> > > Thank you for suggestions
> > > > > 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 */
> > > This is true...
> > > > What a simple comment doesn't do is avoid breaking -Wswitch.
> > > ... as well as this comment as well.
> > > > 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.
> > > This is a trade-off: if your compiler is buggy or not supporting
> > > the switches then the code itself defends from such cases, hence
> > > the "default".
> > As said before, how do you put the limit on what you need to protect against? 
> > If your compiler is buggy, then probably a default in switch is not going to 
> > help much.
> This is why defensive programming was invented, no? We at least try harder
> to defend ourselves from possible troubles.
You will never be able to fully workaround a buggy compiler. Imagine the default 
case is also miscompiled? My point here is we have to
Anyway, the chance of buggy compiler is slimmer than a programming error. Hence 
the defensive programming.
> Again, I am not an expert in this field. Neither I can defend MISRA and 
> corresponding
> requirements. But, I do believe that those rules do have some ground 
> underneath...
What I question is the way your patch implemented the rule. The goal of MISRA is 
to make the code more defensive, a few of your changes does not achieve that.
Cheers,

--
Julien Grall

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