[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.