Re: Top MISRA violations

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxx>]

On Thu, 23 Apr 2020, Bertrand Marquis wrote:
>       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.

What you wrote makes sense.

Also, I think Julien meant that we should use -Wmisleading-indentation
only with GCC versions that include -Wmisleading-indentation in -Wall
(due to quality of the implemantation in GCC). Not necessarely use -Wall
instead of -Wmisleading-indentation.

So in this case, we could document something like:

  use GCC from version X to version Y with -Wmisleading-indentation
  to check for Rule 15.6


> 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.
 
Looks like a good and solid way of doing it