Re: [PATCH] docs/misra: new rules addition

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 7 Jun 2023, Jan Beulich wrote:
> On 07.06.2023 03:38, Stefano Stabellini wrote:
> > From: Stefano Stabellini <stefano.stabellini@xxxxxxx>
> > 
> > For Dir 1.1, a document describing all implementation-defined behaviour
> > (i.e. gcc-specific behavior) will be added to docs/misra, also including
> > implementation-specific (gcc-specific) appropriate types for bit-field
> > relevant to Rule 6.1.
> > 
> > Rule 21.21 is lacking an example on gitlab but the rule is
> > straightforward: we don't use stdlib at all in Xen.
> > 
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxx>
> > ---
> > We also discussed Rules 2.1, 5.5 and 7.4 but they require more work
> > before we can decide one way or the other.
> 
> Since I wasn't able to attend yesterday's meeting, please forgive a
> couple of remarks:

No problem


> > @@ -133,6 +146,16 @@ existing codebase are work-in-progress.
> >         headers (xen/include/public/) are allowed to retain longer
> >         identifiers for backward compatibility.
> >  
> > +   * - `Rule 5.6 
> > <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_05_06.c>`_
> > +     - Required
> > +     - A typedef name shall be a unique identifier
> > +     -
> 
> Considering that the rule requires uniqueness across the entire code
> base (and hence precludes e.g. two functions having identically named
> local typedefs), I'm a little puzzled this was adopted. I for one
> question that a use like the one mentioned is really at risk of being
> confusing. Instead I think that the need to change at least one of
> the names is at risk of making the code less readable then, even if
> ever so slightly. (All of this said - I don't know if we have any
> violations of this rule.)

I don't think we have many local typedefs and I think we have only few
violations if I remember right. I'll let Roberto confirm how many. This
rule was considered not a difficult rule (some difficult rules were
found, namely 2.1, 5.5 and 7.4.)


> > +   * - `Rule 6.1 
> > <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_06_01.c>`_
> > +     - Required
> > +     - Bit-fields shall only be declared with an appropriate type
> > +     -
> 
> This requires you have settled on what "an appropriate type" is, i.e.
> whether our uses of e.g. types wider than int is meant to become a
> deviation, or will need eliminating. I suppose the outcome of this
> could do with mentioning as a remark here.

Yes, Roberto showed the "appropriate types" for gcc and there were
plenty including unsigned long if I remember right. I didn't write the
full list down.

Roberto do you have the list ready? I can add it in the Notes section
here.


> > @@ -143,6 +166,12 @@ existing codebase are work-in-progress.
> >       - Octal constants shall not be used
> >       -
> >  
> > +   * - `Rule 7.2 
> > <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_07_02.c>`_
> > +     - Required
> > +     - A "u" or "U" suffix shall be applied to all integer constants
> > +       that are represented in an unsigned type
> > +     -
> 
> "Represented in an unsigned type" means there is a dependency on the
> target architecture and compiler capabilities: Representation can only
> be determined once knowing the underlying binary ABI, and uses in #if
> and alike require knowing the widest integer type's size that the
> compiler supports. As a consequence this looks like it may require, in
> certain cases, to add u/U conditionally. Any such conditionals pose a
> risk of cluttering the code.

I don't think there is any plan to add u/U conditionally, and I can see
that would be undesirable. I think the idea is to add u/U to all
constants meant to be unsigned. But also here I'll Roberto chime in --
he said they already have a draft patch to fix this.


> > @@ -314,6 +343,11 @@ existing codebase are work-in-progress.
> >         used following a subsequent call to the same function
> >       -
> >  
> > +   * - Rule 21.21
> > +     - Required
> > +     - The Standard Library function system of <stdlib.h> shall not be used
> > +     -
> 
> I wish inapplicable (to us) rules would also be marked as such.
 
Yes, actually it could be a good idea to say "inapplicable" in the Notes
section for all the rules like this one. I can write a patch for it.