[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: Top MISRA violations


  • To: Julien Grall <julien.grall.oss@xxxxxxxxx>
  • From: Stefano Stabellini <stefano.stabellini@xxxxxxxxxx>
  • Date: Wed, 22 Apr 2020 17:52:25 -0700 (PDT)
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass (sender ip is 149.199.60.83) smtp.rcpttodomain=gmail.com smtp.mailfrom=xilinx.com; dmarc=bestguesspass action=none header.from=xilinx.com; dkim=none (message not signed); arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=57i/82tmNKdG8KZjO1xqU42e1ZYDus+jCb5RyasGj/A=; b=ScTbg5jOR44/RyZBE7mCexmjmzftMTEZTbDXSEcG6EBBt66XWQ62QH1hciR3qmkGXigyEdP867Jw6GXnZhKJ5IN7BJOlcFkI0xvB+qzOBOZvnisKUGb4pUzcTYJFRImMoWdyEt3jmKbDcoqUhv29PBg3Scz6Cw9X4X2Dlq6X+oxhq/xeaQM1ny7MqeAIeWi/s4BnRAU9/6dc7mPU7nwuiZXtbPKumIJYM/BljnT38MQ/YlOY2v3QvlY3PomhMKZrxWS1sPmeNdCij0v7YqDVD32l0fxJlBqIbpn4oKEEUCYL+IA17NWTdWt6WI+3wQLB+FyhbAsu3IKSb9QfALktPQ==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=dhSD4IkWVJdf7B22JgsnwavZk3kRIFNW5pHI6kN7ut7zeO0sqFFs9xgAM9GqLRBf9r8EQ73REG5PdB78CMzXbpynzoRGLvYZ2TX8aJX1EvQEiCQr5fD3EjFmI6yHVe3VMu1Jz9Pa/bRJXCz3VJSV+olu/Dh3ycQznnr4NhAG/qfp2bHWqy7Tr7e4sQhqMzDetPjZs3qf1mkRsh+L6x5AO9iz+iDpnfDKLdi+sRMFVO18DLZqk6Za5T1yE73vVUYyFstmQ0IvPAvUFcjp6T87/3En+d+ZpFT/WgSanYsgtdFpMhswLQ3wwwcyKa2T+7md56dZXJv5io7MqGfB99zZ/g==
  • Authentication-results: spf=pass (sender IP is 149.199.60.83) smtp.mailfrom=xilinx.com; gmail.com; dkim=none (message not signed) header.d=none;gmail.com; dmarc=bestguesspass action=none header.from=xilinx.com;
  • Cc: Stefano Stabellini <stefano.stabellini@xxxxxxxxxx>, fusa-sig@xxxxxxxxxxxxxxxxxxxx
  • Delivery-date: Thu, 23 Apr 2020 00:52:33 +0000
  • List-id: This is a discussion list for members of the Xen Project FuSa SIG <fusa-sig.lists.xenproject.org>

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
> 



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.