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

[Oleksandr Andrushchenko <andr2000@xxxxxxxxx>]

On 2/25/19 3:22 PM, Julien Grall wrote:
> Hi,
>
> On 25/02/2019 13:06, Oleksandr Andrushchenko wrote:
> > On 2/25/19 2:50 PM, Julien Grall wrote:
> > > Hi,
> > >
> > > On 25/02/2019 12:38, Oleksandr Andrushchenko wrote:
> > > > On 2/25/19 2:15 PM, Julien Grall wrote:
> > > > > 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()?
> > > > This order seems to be more consistent. Otherwise, we fix consequence
> > > > and then get back to the actual cause
> > > So you will add more badness to just be consistent. Doesn't it sound 
> > > not right for you? The more that you justify this patch series as 
> > > "defensive code"...
> > Why? If we *first* deal with BUG() and *then* introduce "defaults" patch
> > which will use the already fixed code?
> This is not how your e-mail came out. It came out as it is fine to add 
> "default" with BUG() and then fix it later.
Yes, you are correct. But I have an impression that couple of letters ago
I agreed that this *is* an issue and it *does need* a proper fix. And 
stated that
we might want to fix that separately. So, after that point I thought the 
sequence
will be:
1. Fix BUG()
2. Use fixed code as a basis for "defaults"
Hope we are on the same page now.
> However, I don't see any strict dependencies on the both. The more 
> than removing all BUG() is probably going to be more controversial 
> than this series. So maybe it is good to focus on one things and do it 
> right. Anyway, that's your call.
> > > > > > > > > > > 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.
> > > > This is a v1 of the patch. And one of the reasons to send it as is 
> > > > was to gather
> > > > community's view on how to solve such cases.
> > > If you are aware of potential issue, then it is custom to specify 
> > > them in the cover letter.
> > My bad, but the patch itself was expected to raise questions, so I 
> > was not sure
> > what needs to be highlighted
> > > This would allow us to directly focus on the meat of the series 
> > The meat is being discussed (IMO) and it is not only empty defaults
> > > rather than on sending an extra 40 e-mails to have more understanding.
> > And just imagine if one spends weeks of work trying to fix some other
> > non-trivial requirements and then receives >40 e-mails all requesting
> > non-trivial changes. We already have such an example with Stefano's
> > series which is now v9...
> > So, IMO it is better to discuss some common approaches before sending
> > something really complex which has all chances to have v15...
> > And you might have noticed that the series starts with "RFC" word
> My point is not about sending such code on the mailing list. My point 
> is you need to provide as much as possible details in your cover 
> letter so we can be more efficient when reviewing. For instance, many 
> of us does not have access to MISRA spec because it is not free...
While I agree that one has to provide as much supporting information as 
possible
while presenting some work to the community it is that I cannot disclose
MISRA rules here. As you said, MISRA spec is not free. And of course I 
cannot
expect anyone to by it for the reason that someone wants some patch to be
"securely" or blindly reviewed. (BTW, this is the topic that has already 
been
raised in our team internally and being discussed)
> Cheers,

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