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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 22 Feb 2019, 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
> 
> So when you disable CONFIG_MISRA, your compiler starts being able to
> help you again.

This is actually a good way to make progress which could make everybody
happy. (And it wouldn't require a slow back-and-forth with third parties
to ask difficult questions.)


> 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.

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