Re: Top MISRA violations

[Julien Grall <julien.grall.oss@xxxxxxxxx>]

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?

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

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

Cheers,

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