Re: Top MISRA violations

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi,

On 23 Apr 2020, at 01:52, Stefano Stabellini <stefano.stabellini@xxxxxxxxxx> wrote:

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

From my experience -Wall is neither or to much but more undefined as it changing with the versions of compiler.

So in the certification processes I went through the only solution is to explicitly list all of them so that it can be reviewed.

It would suggest to put all warning in one make include file with a comment for each of them and classify them with:

- mandatory

- recommended

- other

Mandatory should always be activated, recommended could be selectable by user and activated during internal tests.

Other could be a place holder for new flags or proposals.

It should be also possible to filter some of them depending on the compiler and version of compiler used, the architecture or configuration

There is some work the first time but once done that should not be to much overhead to maintain those.

Regards

Bertrand

# 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