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

[Oleksandr Andrushchenko <andr2000@xxxxxxxxx>]

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()
> > > > > 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.
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...
> Cheers,

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