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

[Oleksandr Andrushchenko <andr2000@xxxxxxxxx>]

On 2/23/19 1:41 AM, Andrew Cooper wrote:
> On 22/02/2019 23:22, Julien Grall wrote:
> > Hi,
> >
> > On 22/02/2019 22:34, Andrew Cooper wrote:
> > > On 22/02/2019 22:11, Julien Grall wrote:
> > > > Hi Stefano,
> > > >
> > > > On 22/02/2019 21:58, Stefano Stabellini wrote:
> > > > > On Fri, 22 Feb 2019, 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.
> > > > > > > > > > > > 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.
> > > > > > > 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.
> > > > > That's a good suggestion.
> > > > >
> > > > >
> > > > > > > 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 */
> > > > > > What a simple comment doesn't do is avoid breaking -Wswitch.
> > > > > I don't know how to reconcile 16.4 with -Wswitch. One could argue that
> > > > > -Wswitch could be a good way to address 16.4, but then we introduce a
> > > > > compiler specific requirement. Typically gcc is not the compiler of
> > > > > choice for these environments, unfortunately forcing gcc is not an
> > > > > option.
> > > > Well, you could build with GCC and then build with your custom
> > > > compiler... But, GCC is pretty much the only choice for Xen on Arm today
> > > > as we don't build with clang and I pretty doubt we can build with compcert.
> > > So the suggestion I had was to have an overall CONFIG_MISRA which we can
> > > hide some of this nonsense behind, and then
> > >
> > > #ifdef CONFIG_MISRA
> > > #define MISRA_BLE_DEFAULT default:
> > > #else
> > > #define MISRA_BLE_DEFAULT
> > > #endif
> > This is pretty disgusting :). But then, it makes the code is bit more
> > obscure. So how that rule is making Xen more safe? Furthermore, one
> > default may not rule them all. So aren't we just adding code to make
> > MISRA happy at the risk of introducing more bug?
> Any switch statement where the default isn't empty
> (assert/bug/domain_crash/print) should probably be a regular default:
>
> I very much doubt MISRA intended for people to make themselves compliant
> by adding "default: break;" everywhere, but lethargy is a powerful
> driving force.
>
> An alternative, and substantially more ugly would be to have the else
> case be "if ( 0 )" or similar, so you do take out the full next block,
> but that requires people to member braces.
>
> Even more ugly would be to take (x) and construct the block manually,
> but then you get at minimum a set of brackets where you would expect to
> see braces in normal C, and this alone is confusing to read.
>
> > > So when you disable CONFIG_MISRA, your compiler starts being able to
> > > help you again.
> > >
> > > TBH, it would also be nice to hide the SYMBOL nonsense behind, so we can
> > > continue doing it the efficient way for ~100% of the time.
> > My main concern is we only looked at 2 MISRA rules and we already need
> > some nonsense code to spread everywhere. How much more are we going to
> > get like that?
(Un)fortunately many more...
> MISRA scanning is just like Coverity scanning.  The item classes with
> the highest seen-count tend to be the least interesting and relevant, as
> they tend to be the ones where we are systematically different.
>
> It is the one-off flagged issues which tend to be important.
I do agree. There are many more more serious issues on the list.
The "default" rule is just one to try and seemed very straight forward
to fix (I put aside BUG() issue - this was expected to be discussed)
> ~Andrew

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