Re: Top MISRA violations

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxx>]

On Wed, 22 Apr 2020, Julien Grall wrote:
> Hi,
> 
> On Wed, 22 Apr 2020 at 21:50, Stefano Stabellini
> <stefano.stabellini@xxxxxxxxxx> wrote:
> >
> > Hi all,
> >
> > Francesco kindly shared his spreadsheet with the analysis of the MISRA
> > violations, see attached. I am sharing it here to the list as all the
> > members of fuse-sig have access to MISRA. I also want to take this
> > opportunity to thank Francesco and his team for their work.
> >
> >
> > Looking at the spreadsheet, the top required rules with more than 1000
> > violations are: 15.6, 21.2, 5.1, 8.4.
> >
> >
> > # Rule 15.6
> >
> > The body of an iteration-statement (while, for) or a selection-statement
> > (if, switch) shall be a compound statement.
> >
> > For example:
> >
> >     /* non compliant */
> >     if ( x )
> >         func();
> >
> >     /* compliant */
> >     if ( x )
> >     {
> >         func();
> >     }
> >
> > Where the real issue are cases like this:
> >
> >     if ( one )
> >         if ( two )
> >             foo();
> >         else
> >             bar();
> >
> >
> > ## Analysis & Potential Solution
> >
> > There are many places in the Xen code where we skip the curly brackets
> > when the statement under 'if' is a one-liner. A solution that was
> > suggested before was to use GCC automatic checking for these situations:
> >
> >     -Wmisleading-indentation (C and C++ only)
> >               Warn when the indentation of the code does not reflect the 
> > block
> >               structure.  Specifically, a warning is issued for "if", 
> > "else",
> >               "while", and "for" clauses with a guarded statement that does 
> > not
> >               use braces, followed by an unguarded statement with the same
> >               indentation.
> >
> >               In the following example, the call to "bar" is misleadingly
> >               indented as if it were guarded by the "if" conditional.
> >
> >                         if (some_condition ())
> >                           foo ();
> >                           bar ();  /* Gotcha: this is not guarded by the 
> > "if".  */
> >
> >               [...]
> >
> > We could document that -Wmisleading-indentation is required and should
> > be used as a check against this kind of issues. A patch to the Xen
> > Makefile to add -Wmisleading-indentation by default would also help. In
> > this context, we would be using GCC as a static code checker, not as a
> > compiler (GCC is not a safety certified compiler.)
> 
> A quick look at the man [1] suggests that -Wall already include
> -Wmisleading-indentation on recent GCC. Note that is has been known to
> bring some false positive in older version of GCC. So if we force on
> compiler that doesn't include it in -Wall, then we may end up to have
> more nuisance than needed.
> 
> How about documenting the version of GCC to use?

Yep, from my understanding that would be good enough. (I'd love for
Antonio and other experts to confirm.)


> >
> > # Rule 21.2
> >
> > A reserved identifier or macro name shall not be declared.
> >
> > Example, non-compliant:
> >
> >     extern void *memcpy(void *restrict s1, const void *restrict s2, size_t 
> > n);
> >
> > It is non-compliant because we are meant to #include <string.h> to get
> > the declaration of memcpy.
> >
> >
> > ## Analysis & Potential Solution
> >
> > In a kernel or a hypervisor it is not possible to #include <string.h>
> > because the standard library is not available.
> >
> > Probably it is just a matter of documenting this deviation?
> >
> >
> > # Rule 5.1
> >
> > External identifiers shall be distinct.
> >
> > /* non compliant */
> > int32_t abc = 0;
> > int32_t ABC = 0;
> >
> > /* non compliant: 31 significant chars are common */
> > int32_t engine_exhaust_gas_temperature_raw;
> > int32_t engine_exhaust_gas_temperature_scaled;
> >
> > - in C90 the minimum requirement is that the first 6 characters of
> >   external identifiers are significant but their case is not required to
> >   be significant;
> > - in C99 the minimum requirement is that the first 31 characters of
> >   external identifiers are significant, with each universal character or
> >   corresponding extended source character occupying between 6 and 10
> >   characters.
> >
> >
> > ## Analysis & Potential Solution
> >
> > In reality most implementations provide far greater limits. It is common
> > for external identifiers in C90 to be case-sensitive and for the first
> > 31 characters to be significant.
> >
> > Should we document that we require a C compiler that supports at least
> > the first 31 characters to be significant and case-sensitive? Then
> > configure the MISRA checker accordingly. Would it be good enough?
> 
> FWIW, we build Xen + tools using gnu99 (c99 + GNU extension), I don't
> think we would be able to compile using c90 as we rely on some c99
> features in Arm.

Good to know. Francesco, would it be possible to tell the checker to use
c99 in regards to Rule 5.1?


> > # Rule 8.4
> >
> > A compatible declaration shall be visible when an object or function
> > with external linkage is defined.
> >
> > Example:
> >
> > extern int16_t count;
> > int16_t count = 1; /* compliant */
> >
> > extern int16_t count = 1; /* non-compliant */
> >
> >
> > ## Analysis & Potential Solution
> >
> > This is not a rule I would expect Xen to be violating.
> 
> I am afraid Xen is violating this one in quite a few places. A good
> example is functions called from assembly such as start_xen().
> We also have a few places where the #include declaring the prototypes
> are not present before the implementation.
> 
> -Wall doesn't include -Wmissing-prototypes, I did attempt it a few
> years ago to add it but only part of the work made upstream so the
> flag was never added.

That is a good pointer, thank you. I can try to have a look at what
happens when adding -Wmissing-prototypes to the build.

>From my understanding, I don't think the functions called from assembly
would count as violations, but the others would.


> Cheers,
> 
> [1] http://man7.org/linux/man-pages/man1/gcc.1.html
>