Re: [PATCH 1/2] docs/misra: introduce rules.rst

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 25 May 2022, Jan Beulich wrote:
> On 25.05.2022 02:35, Stefano Stabellini wrote:
> > --- a/CODING_STYLE
> > +++ b/CODING_STYLE
> > @@ -235,3 +235,9 @@ callstack between the initial function call and the 
> > failure, no error
> >  is returned.  Using domain_crash() requires careful inspection and
> >  documentation of the code to make sure all callers at the stack handle
> >  a newly-dead domain gracefully.
> > +
> > +MISRA C
> > +-------
> > +
> > +The Xen Project hypervisor follows the MISRA C coding rules and
> > +directives listed under docs/misra/rules.rst.
> 
> Putting this at the very bottom isn't helpful, I'm afraid. I'd rather
> see this go directly after the initial paragraphs, before "Indentation".

I can do that


> > --- /dev/null
> > +++ b/docs/misra/rules.rst
> > @@ -0,0 +1,65 @@
> > +=====================
> > +MISRA C rules for Xen
> > +=====================
> > +
> > +**IMPORTANT** All MISRA C rules, text, and examples are copyrighted by the
> > +MISRA Consortium Limited and used with permission.
> > +
> > +Please refer to https://www.misra.org.uk/ to obtain a copy of MISRA C, or 
> > for
> > +licensing options for other use of the rules.
> > +
> > +The following is the list of MISRA C rules that apply to the Xen Project
> > +hypervisor.
> > +
> > +- Rule: Dir 2.1
> > +  - Severity:  Required
> > +  - Summary:  All source files shall compile without any compilation errors
> > +  - Link:  
> > https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/D_02_01.c
> > +- Rule: Dir 4.7
> > +  - Severity:  Required
> > +  - Summary:  If a function returns error information then that error 
> > information shall be tested
> > +  - Link:  
> > https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/D_04_07.c
> > +- Rule: Dir 4.10
> > +  - Severity:  Required
> > +  - Summary:  Precautions shall be taken in order to prevent the contents 
> > of a header file being included more than once
> > +  - Link:  
> > https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/D_04_10.c
> 
> Like Julien has already pointed out for 4.7, this and perhaps other ones
> also want clarifying somewhere that we expect certain exceptions. Without
> saying so explicitly, someone could come forward with a patch eliminating
> some uses (and perhaps crippling the code) just to satisfy such a rule.
> This would then be a waste of both their and our time.

Yes, and also Julien pointed out something similar. I'll add a statement
at the top of the file to say that there can be deviations as long as
they are commented.

I wouldn't go as far as adding a note to each specific rule where we
expect deviations because I actually imagine that many of them will end
up having at least one deviation or two. It would be easier to mark the
ones where we expect to have 100% compliance and intend to keep it that
way (once we reach 100% compliance).

We are still in the early days of this process. For now, what about
adding the following statement at the top of the file (in addition to
the one saying that deviations are permissible):

"""
The existing codebase is not 100% compliant with the rules. Some of the
violations are meant to be documented as deviations, while some others
should be fixed. Both compliance and documenting deviations on the
existing codebase is work-in-progress.
"""


> > +- Rule: Dir 4.14
> > +  - Severity:  Required
> > +  - Summary:  The validity of values received from external sources shall 
> > be checked
> > +  - Link:  
> > https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/D_04_14.c
> > +- Rule: Rule 1.3
> > +  - Severity:  Required
> > +  - Summary:  There shall be no occurrence of undefined or critical 
> > unspecified behaviour
> > +  - Link:  
> > https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_01_03.c
> > +- Rule: Rule 3.2
> > +  - Severity:  Required
> > +  - Summary:  Line-splicing shall not be used in // comments
> > +  - Link:  
> > https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_03_02.c
> 
> To aid easily looking up presence of a rule here, I think the table wants
> sorting numerically.

They are sorted numerically, first the "Dir" (directives) then the
"Rules".


> > +- Rule: Rule 6.2
> > +  - Severity:  Required
> > +  - Summary:  Single-bit named bit fields shall not be of a signed type
> > +  - Link:  
> > https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_06_02.c
> > +- Rule: Rule 8.1
> > +  - Severity:  Required
> > +  - Summary:  Types shall be explicitly specified
> > +  - Link:  
> > https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_08_01.c
> > +- Rule: Rule 8.4
> > +  - Severity:  Required
> > +  - Summary:  A compatible declaration shall be visible when an object or 
> > function with external linkage is defined
> > +  - Link:  
> > https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_08_04.c
> > +- Rule: Rule 8.5
> > +  - Severity:  Required
> > +  - Summary:  An external object or function shall be declared once in one 
> > and only one file
> > +  - Link:  
> > https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_08_05_2.c
> > +- Rule: Rule 8.6
> > +  - Severity:  Required
> > +  - Summary:  An identifier with external linkage shall have exactly one 
> > external definition
> > +  - Link:  
> > https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_08_06_2.c
> 
> I don't think this was uncontroversial, as we've got a lot of uses of
> declarations when we expect DCE to actually take out all uses. There
> are also almost a thousand violations, which - imo - by itself speaks
> against adoption.

Ah yes, good catch. We spoke about DCE in the context of Rule 2.1, not
this one. My preference would be to keep Rule 8.6 with a note allowing
DCE:

- Note: declarations without definitions are allowed (specifically when
  the definition is compiled-out or optimized-out by the compiler)

But if this is controversial, I can move it to be discussed later
together with Rule 2.1.