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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Mon, 25 Feb 2019, Julien Grall wrote:
> Hi Stefano,
> 
> On 25/02/2019 17:47, Stefano Stabellini wrote:
> > On Mon, 25 Feb 2019, George Dunlap wrote:
> > > On Mon, Feb 25, 2019 at 11:42 AM Jan Beulich <JBeulich@xxxxxxxx> wrote:
> > > > 
> > > > > > > On 22.02.19 at 22:33, <andrew.cooper3@xxxxxxxxxx> wrote:
> > > > > P.S. There is a solution here which could work, but IMO a better use
> > > > > of
> > > > > time and energy would be to get MISRA to update their rules to match
> > > > > this century, and stop getting in the way of compiler features
> > > > > intended
> > > > > to help the programmer avoid bugs.
> > > > 
> > > > As much as I'm with you in desiring the compiler aid given to not get
> > > > undermined, I think this MISRA rule isn't in need of modernizing: It's
> > > > one thing for the compiler to help with in-range enumerators, and it's
> > > > another to demand that unintentional out-of-range ones don't cause
> > > > actual harm (like crashing your car into the next tree). This is even
> > > > more so that iirc there's no warning if you pass a plain integer into a
> > > > function whose parameter specifies an enum, or if you assign a plain
> > > > integer to an enum types variable.
> > > 
> > > FWIW I was thinking the same thing.
> > 
> > Jan described exactly the scenario Rule 16.4 attempts to protect us
> > from. FYI MISRAC explains the reasons behind each rule, this is why I
> > think it would be great for you (you as in the x86 and Arm maintainers
> > mainly) to have access to it so you can read the explanation by
> > yourselves.
> > 
> > Also, all the alternative suggestions about using complier features to
> > check switch statements are based on the fact that one can use those
> > compilers. I don't think we can/should/want to mandate which compilers
> > are used with the Xen codebase. We don't want to get into the situation
> > where the code is "safe" only when it is built with gcc.
> 
> The compiler is not here to make it "safe", but helps you to find place where
> we forgot to handle a case when extending an enum.
> 
> Lets imagine for a moment that we decide to extend p2m_type_t. There are quite
> a few switch using this enum. You will no deny that finding out all the
> missing path during reviewing can be tricky. Here the compiler give you a bit
> of help to find to check all the places were handled.
> 
> Without the compiler aid, you may have to wait until someone hit the error
> path before actually discovering the missing bits. This may take quite
> sometimes (more than a release) depending on how much the code is exercised
> and potentially a pain to debug.
> 
> Regardless what the safety-certification folks are going to use, GCC is likely
> going to stay one of the main compiler used to build Xen. We are already
> struggling to keep up with the review, so I fail to see why we should make
> reviewer's life a bit more miserable by preventing us using "safety" options
> from GCC.
> 
> So I would like to find a way to keep that benefits for reviewers while also
> addressing MISRA rules. I suggested a GCC flag (-Wswitch-enum) on another
> e-mail [1] but Jan was not against such use.
> 
> I don't have a better suggestion so far.
> 
> Cheers,
> 
> [1] <5C73DB260200007800219CF1@xxxxxxxxxxxxxxxxxxxxxxxx>

I think it is fine to exploit compiler specific checks when available.
However, I don't think we should make any decisions on code correctness
based on the compiler checks that we introduce.

In other words, I think it is a good idea to use -Wswitch-enum if
possible, but it is not a reason for not having default labels in place
as suggested by 16.4. The code should be as correct and safe as
possible, without requiring external compiler-specific checks.

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