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

Commit Message

Stefano Stabellini May 25, 2022, 12:35 a.m. UTC

From: Stefano Stabellini <[e-mail]>

Introduce a list of MISRA C rules that apply to the Xen hypervisor. The
list is in RST format.

Add a mention of the new list to CODING_STYLE.

Signed-off-by: Bertrand Marquis <[e-mail]>
Signed-off-by: Stefano Stabellini <[e-mail]>
---
 CODING_STYLE         |  6 ++++
 docs/misra/rules.rst | 65 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 71 insertions(+)
 create mode 100644 docs/misra/rules.rst

Comments

Julien Grall May 25, 2022, 7:39 a.m. UTC | #1

Hi Stefano,

On 25/05/2022 01:35, Stefano Stabellini wrote:
> From: Stefano Stabellini <[e-mail]>
> 
> Introduce a list of MISRA C rules that apply to the Xen hypervisor. The
> list is in RST format.
> 
> Add a mention of the new list to CODING_STYLE.
> 
> Signed-off-by: Bertrand Marquis <[e-mail]>
> Signed-off-by: Stefano Stabellini <[e-mail]>

NIT: I was under the impression that the first Signed-off-by is usually 
author. But the From doesn't match.

> ---
>   CODING_STYLE         |  6 ++++
>   docs/misra/rules.rst | 65 ++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 71 insertions(+)
>   create mode 100644 docs/misra/rules.rst
> 
> diff --git a/CODING_STYLE b/CODING_STYLE
> index 9f50d9cec4..1ef35ee8d0 100644
> --- 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.
> diff --git a/docs/misra/rules.rst b/docs/misra/rules.rst
> new file mode 100644
> index 0000000000..c0ee58ab25
> --- /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.
I was under the impression that we would still allow deviations on those 
rules in some cases. In particular...

> +
> +- 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


... this one. We are using (void) + a comment when the return is ignored 
on purpose. This is technically not-compliant with MISRA but the best we 
can do in some situation.

With your proposed wording, we would technically have to remove them (or 
not introduce new one). So I think we need to document that we are 
allowing deviations so long they are commented.

Cheers,

Jan Beulich May 25, 2022, 8:25 a.m. UTC | #2

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

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

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

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

Jan

> +- Rule: Rule 8.8
> +  - Severity:  Required
> +  - Summary:  The static storage class specifier shall be used in all declarations of objects and functions that have internal linkage
> +  - Link:  https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_08_08.c
> +- Rule: Rule 8.12
> +  - Severity:  Required
> +  - Summary:  Within an enumerator list the value of an implicitly-specified enumeration constant shall be unique
> +  - Link:  https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_08_12.c

Andrew Cooper May 25, 2022, 12:21 p.m. UTC | #3

On 25/05/2022 01:35, Stefano Stabellini wrote:
> From: Stefano Stabellini <[e-mail]>
>
> Introduce a list of MISRA C rules that apply to the Xen hypervisor. The
> list is in RST format.
>
> Add a mention of the new list to CODING_STYLE.
>
> Signed-off-by: Bertrand Marquis <[e-mail]>
> Signed-off-by: Stefano Stabellini <[e-mail]>

Some comments on syntax/layout, unrelated to the specific content.

You can check the rendered content with either `make -C docs
sphinx-html` locally, or by pointing readthedocs at your repo.  (e.g.
https://andrewcoop-xen.readthedocs.io/en/docs-devel/ is a very out of
date WIP branch of some in-development content.)

Whatever gets committed will be rendered at
https://xenbits.xen.org/docs/latest/ once the cronjob catches up.

> ---
>  CODING_STYLE         |  6 ++++
>  docs/misra/rules.rst | 65 ++++++++++++++++++++++++++++++++++++++++++++

At minimum there needs to be an addition to a toctree directive in on of
the existing index.rst's

But  this looks like it ought to be part of the hypervisor guide ?

>  2 files changed, 71 insertions(+)
>  create mode 100644 docs/misra/rules.rst
>
> diff --git a/CODING_STYLE b/CODING_STYLE
> index 9f50d9cec4..1ef35ee8d0 100644
> --- 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.

I think this would be clearer to follow as:

"The Xen Hypervisor follows some MISRA C coding rules.  See ... for
details."

because otherwise there is an implication that we follow all rules. 
Also, "Xen Project" might be the name of our legal entity name, but the
hypervisor's name is Xen, not "Xen Project".

> diff --git a/docs/misra/rules.rst b/docs/misra/rules.rst
> new file mode 100644
> index 0000000000..c0ee58ab25
> --- /dev/null
> +++ b/docs/misra/rules.rst
> @@ -0,0 +1,65 @@

All Sphinx content needs to be

.. SPDX-License-Identifier: CC-BY-4.0

so it specifically can be vendored/tailored by downstream entities.

> +=====================
> +MISRA C rules for Xen
> +=====================

And the prevailing style is without the === overline.

> +
> +**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.

.. note::

and then with the two paragraphs indented to be a part of the note block.

> +
> +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

This wants to be .. list-table::  See
docs/guest-guide/x86/hypercall-abi.rst for an example.

Also, the URL wants to use the ext-links plugin.  See
https://lore.kernel.org/xen-devel/[e-mail]/

~Andrew

Stefano Stabellini May 26, 2022, 1:02 a.m. UTC | #4

On Wed, 25 May 2022, Julien Grall wrote:
> Hi Stefano,
> 
> On 25/05/2022 01:35, Stefano Stabellini wrote:
> > From: Stefano Stabellini <[e-mail]>
> > 
> > Introduce a list of MISRA C rules that apply to the Xen hypervisor. The
> > list is in RST format.
> > 
> > Add a mention of the new list to CODING_STYLE.
> > 
> > Signed-off-by: Bertrand Marquis <[e-mail]>
> > Signed-off-by: Stefano Stabellini <[e-mail]>
> 
> NIT: I was under the impression that the first Signed-off-by is usually
> author. But the From doesn't match.
> 
> > ---
> >   CODING_STYLE         |  6 ++++
> >   docs/misra/rules.rst | 65 ++++++++++++++++++++++++++++++++++++++++++++
> >   2 files changed, 71 insertions(+)
> >   create mode 100644 docs/misra/rules.rst
> > 
> > diff --git a/CODING_STYLE b/CODING_STYLE
> > index 9f50d9cec4..1ef35ee8d0 100644
> > --- 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.
> > diff --git a/docs/misra/rules.rst b/docs/misra/rules.rst
> > new file mode 100644
> > index 0000000000..c0ee58ab25
> > --- /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.
> I was under the impression that we would still allow deviations on those rules
> in some cases. In particular...
> > +- 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
> 
> 
> ... this one. We are using (void) + a comment when the return is ignored on
> purpose. This is technically not-compliant with MISRA but the best we can do
> in some situation.
> 
> With your proposed wording, we would technically have to remove them (or not
> introduce new one). So I think we need to document that we are allowing
> deviations so long they are commented.

Absolutely yes. All of these rules can have deviations as long as they
make sense and they are commented. Note that we still have to work out
a good tagging system so that ECLAIR and cppcheck can recognize the
deviations automatically but for now saying that they need to be
commented is sufficient I think.

So I'll add the following on top of the file:

"""
It is possible that in specific circumstances it is best not to follow a
rule because it is not possible or because the alternative leads to
better code quality. Those cases are called "deviations". They are
permissible as long as they are documented with an in-code comment.
"""

Stefano Stabellini May 26, 2022, 1:12 a.m. UTC | #5

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.

Stefano Stabellini May 26, 2022, 1:57 a.m. UTC | #6

On Wed, 25 May 2022, Andrew Cooper wrote:
> On 25/05/2022 01:35, Stefano Stabellini wrote:
> > From: Stefano Stabellini <[e-mail]>
> >
> > Introduce a list of MISRA C rules that apply to the Xen hypervisor. The
> > list is in RST format.
> >
> > Add a mention of the new list to CODING_STYLE.
> >
> > Signed-off-by: Bertrand Marquis <[e-mail]>
> > Signed-off-by: Stefano Stabellini <[e-mail]>
> 
> Some comments on syntax/layout, unrelated to the specific content.
> 
> You can check the rendered content with either `make -C docs
> sphinx-html` locally, or by pointing readthedocs at your repo.  (e.g.
> https://andrewcoop-xen.readthedocs.io/en/docs-devel/ is a very out of
> date WIP branch of some in-development content.)

Thanks I did that and I can see that the layout needs a lot of
improvements.


> Whatever gets committed will be rendered at
> https://xenbits.xen.org/docs/latest/ once the cronjob catches up.
> 
> > ---
> >  CODING_STYLE         |  6 ++++
> >  docs/misra/rules.rst | 65 ++++++++++++++++++++++++++++++++++++++++++++
> 
> At minimum there needs to be an addition to a toctree directive in on of
> the existing index.rst's
> 
> But  this looks like it ought to be part of the hypervisor guide ?

I would keep the actual MISRA files in their own top-level directory
under docs/ but we can certainly link to them from docs/index.rst or
from hypervisor guide. What about the following added to docs/index.rst:

MISRA C coding guidelines
-------------------------

MISRA C rules and directive to be used as coding guidelines when writing
Xen hypervisor code.

.. toctree::
   :maxdepth: 2

   misra/rules



> >  2 files changed, 71 insertions(+)
> >  create mode 100644 docs/misra/rules.rst
> >
> > diff --git a/CODING_STYLE b/CODING_STYLE
> > index 9f50d9cec4..1ef35ee8d0 100644
> > --- 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.
> 
> I think this would be clearer to follow as:
> 
> "The Xen Hypervisor follows some MISRA C coding rules.  See ... for
> details."
> 
> because otherwise there is an implication that we follow all rules. 
> Also, "Xen Project" might be the name of our legal entity name, but the
> hypervisor's name is Xen, not "Xen Project".

Yep, I can do that


> > diff --git a/docs/misra/rules.rst b/docs/misra/rules.rst
> > new file mode 100644
> > index 0000000000..c0ee58ab25
> > --- /dev/null
> > +++ b/docs/misra/rules.rst
> > @@ -0,0 +1,65 @@
> 
> All Sphinx content needs to be
> 
> .. SPDX-License-Identifier: CC-BY-4.0
> 
> so it specifically can be vendored/tailored by downstream entities.
> 
> > +=====================
> > +MISRA C rules for Xen
> > +=====================
> 
> And the prevailing style is without the === overline.
> 
> > +
> > +**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.
> 
> .. note::
> 
> and then with the two paragraphs indented to be a part of the note block.
> 
> > +
> > +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
> 
> This wants to be .. list-table::  See
> docs/guest-guide/x86/hypercall-abi.rst for an example.

Ah yes, thank you



> Also, the URL wants to use the ext-links plugin.  See
> https://lore.kernel.org/xen-devel/[e-mail]/

It looks like that patch didn't make it upstream? But I can just use the
following format and it works with make -C docs sphinx-html:


   * - `Dir 2.1 <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/D_02_01.c>`_
     - Required
     - All source files shall compile without any compilation errors
     -

the format is `description <link>`_

Jan Beulich May 26, 2022, 9:36 a.m. UTC | #7

On 26.05.2022 03:12, Stefano Stabellini wrote:
> On Wed, 25 May 2022, Jan Beulich wrote:
>> On 25.05.2022 02:35, Stefano Stabellini wrote:
>>> --- /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.

We need to determine where such comments are to go. I hope you don't
mean code comments on each and every instance of similar-kind
deviations.

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

Oh, I see. I didn't recognize the distinction. Maybe have a visual
separator between the two classes?

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

I'd be fine with that.

Jan

Jan Beulich May 26, 2022, 9:43 a.m. UTC | #8

On 26.05.2022 03:02, Stefano Stabellini wrote:
> On Wed, 25 May 2022, Julien Grall wrote:
>> On 25/05/2022 01:35, Stefano Stabellini wrote:
>>> +- 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
>>
>>
>> ... this one. We are using (void) + a comment when the return is ignored on
>> purpose. This is technically not-compliant with MISRA but the best we can do
>> in some situation.
>>
>> With your proposed wording, we would technically have to remove them (or not
>> introduce new one). So I think we need to document that we are allowing
>> deviations so long they are commented.
> 
> Absolutely yes. All of these rules can have deviations as long as they
> make sense and they are commented. Note that we still have to work out
> a good tagging system so that ECLAIR and cppcheck can recognize the
> deviations automatically but for now saying that they need to be
> commented is sufficient I think.
> 
> So I'll add the following on top of the file:
> 
> """
> It is possible that in specific circumstances it is best not to follow a
> rule because it is not possible or because the alternative leads to
> better code quality. Those cases are called "deviations". They are
> permissible as long as they are documented with an in-code comment.
> """

Hmm, so you really mean in-code comments. I don't think this will scale
well (see e.g. the DCE related intended deviation), and it also goes
against the "no special casing for every static analysis tool" concern
I did voice on the call.

Jan

Bertrand Marquis May 26, 2022, 9:54 a.m. UTC | #9

Hi Jan,

> On 26 May 2022, at 10:43, Jan Beulich <[e-mail]> wrote:
> 
> On 26.05.2022 03:02, Stefano Stabellini wrote:
>> On Wed, 25 May 2022, Julien Grall wrote:
>>> On 25/05/2022 01:35, Stefano Stabellini wrote:
>>>> +- 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
>>> 
>>> 
>>> ... this one. We are using (void) + a comment when the return is ignored on
>>> purpose. This is technically not-compliant with MISRA but the best we can do
>>> in some situation.
>>> 
>>> With your proposed wording, we would technically have to remove them (or not
>>> introduce new one). So I think we need to document that we are allowing
>>> deviations so long they are commented.
>> 
>> Absolutely yes. All of these rules can have deviations as long as they
>> make sense and they are commented. Note that we still have to work out
>> a good tagging system so that ECLAIR and cppcheck can recognize the
>> deviations automatically but for now saying that they need to be
>> commented is sufficient I think.
>> 
>> So I'll add the following on top of the file:
>> 
>> """
>> It is possible that in specific circumstances it is best not to follow a
>> rule because it is not possible or because the alternative leads to
>> better code quality. Those cases are called "deviations". They are
>> permissible as long as they are documented with an in-code comment.
>> """
> 
> Hmm, so you really mean in-code comments. I don't think this will scale
> well (see e.g. the DCE related intended deviation), and it also goes
> against the "no special casing for every static analysis tool" concern
> I did voice on the call.

On this subject the idea was more to define a “xen” way to document
deviations in the code and do it in a way so that we could easily substitute
the “flag” to adapt it for each analyser using tools or command line options.

Bertrand

> 
> Jan

Jan Beulich May 26, 2022, 10:15 a.m. UTC | #10

On 26.05.2022 11:54, Bertrand Marquis wrote:
> Hi Jan,
> 
>> On 26 May 2022, at 10:43, Jan Beulich <[e-mail]> wrote:
>>
>> On 26.05.2022 03:02, Stefano Stabellini wrote:
>>> On Wed, 25 May 2022, Julien Grall wrote:
>>>> On 25/05/2022 01:35, Stefano Stabellini wrote:
>>>>> +- 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
>>>>
>>>>
>>>> ... this one. We are using (void) + a comment when the return is ignored on
>>>> purpose. This is technically not-compliant with MISRA but the best we can do
>>>> in some situation.
>>>>
>>>> With your proposed wording, we would technically have to remove them (or not
>>>> introduce new one). So I think we need to document that we are allowing
>>>> deviations so long they are commented.
>>>
>>> Absolutely yes. All of these rules can have deviations as long as they
>>> make sense and they are commented. Note that we still have to work out
>>> a good tagging system so that ECLAIR and cppcheck can recognize the
>>> deviations automatically but for now saying that they need to be
>>> commented is sufficient I think.
>>>
>>> So I'll add the following on top of the file:
>>>
>>> """
>>> It is possible that in specific circumstances it is best not to follow a
>>> rule because it is not possible or because the alternative leads to
>>> better code quality. Those cases are called "deviations". They are
>>> permissible as long as they are documented with an in-code comment.
>>> """
>>
>> Hmm, so you really mean in-code comments. I don't think this will scale
>> well (see e.g. the DCE related intended deviation), and it also goes
>> against the "no special casing for every static analysis tool" concern
>> I did voice on the call.
> 
> On this subject the idea was more to define a “xen” way to document
> deviations in the code and do it in a way so that we could easily substitute
> the “flag” to adapt it for each analyser using tools or command line options.

I think the basic scheme of something like this would want laying out
before doc changes like the one here actually go in, so that it's clear
what the action is if a new deviation needs adding for whatever reason
(and also allowing interested people to start contributing patches to
add respective annotations).

Jan

Bertrand Marquis May 26, 2022, 1:04 p.m. UTC | #11

Hi Jan,

> On 26 May 2022, at 11:15, Jan Beulich <[e-mail]> wrote:
> 
> On 26.05.2022 11:54, Bertrand Marquis wrote:
>> Hi Jan,
>> 
>>> On 26 May 2022, at 10:43, Jan Beulich <[e-mail]> wrote:
>>> 
>>> On 26.05.2022 03:02, Stefano Stabellini wrote:
>>>> On Wed, 25 May 2022, Julien Grall wrote:
>>>>> On 25/05/2022 01:35, Stefano Stabellini wrote:
>>>>>> +- 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
>>>>> 
>>>>> 
>>>>> ... this one. We are using (void) + a comment when the return is ignored on
>>>>> purpose. This is technically not-compliant with MISRA but the best we can do
>>>>> in some situation.
>>>>> 
>>>>> With your proposed wording, we would technically have to remove them (or not
>>>>> introduce new one). So I think we need to document that we are allowing
>>>>> deviations so long they are commented.
>>>> 
>>>> Absolutely yes. All of these rules can have deviations as long as they
>>>> make sense and they are commented. Note that we still have to work out
>>>> a good tagging system so that ECLAIR and cppcheck can recognize the
>>>> deviations automatically but for now saying that they need to be
>>>> commented is sufficient I think.
>>>> 
>>>> So I'll add the following on top of the file:
>>>> 
>>>> """
>>>> It is possible that in specific circumstances it is best not to follow a
>>>> rule because it is not possible or because the alternative leads to
>>>> better code quality. Those cases are called "deviations". They are
>>>> permissible as long as they are documented with an in-code comment.
>>>> """
>>> 
>>> Hmm, so you really mean in-code comments. I don't think this will scale
>>> well (see e.g. the DCE related intended deviation), and it also goes
>>> against the "no special casing for every static analysis tool" concern
>>> I did voice on the call.
>> 
>> On this subject the idea was more to define a “xen” way to document
>> deviations in the code and do it in a way so that we could easily substitute
>> the “flag” to adapt it for each analyser using tools or command line options.
> 
> I think the basic scheme of something like this would want laying out
> before doc changes like the one here actually go in, so that it's clear
> what the action is if a new deviation needs adding for whatever reason
> (and also allowing interested people to start contributing patches to
> add respective annotations).

We will work on that but if we wait for everything to be solved we will
never progress.
I have a task on my side (ie at arm) to work on that and Luca Fancellu
will start working on it next month.
Now I do not think that this should block this patch, agreeing on rules does
not mean will respect all of them in the short term so we can wait a bit as I
definitely think that how to document violations in the code and in general
will be a work package on its own and will require some discussion.

Bertrand

> 
> Jan

Stefano Stabellini May 26, 2022, 7:57 p.m. UTC | #12

On Thu, 26 May 2022, Bertrand Marquis wrote:
> > On 26 May 2022, at 11:15, Jan Beulich <[e-mail]> wrote:
> > On 26.05.2022 11:54, Bertrand Marquis wrote:
> >>> On 26 May 2022, at 10:43, Jan Beulich <[e-mail]> wrote:
> >>> On 26.05.2022 03:02, Stefano Stabellini wrote:
> >>>> On Wed, 25 May 2022, Julien Grall wrote:
> >>>>> On 25/05/2022 01:35, Stefano Stabellini wrote:
> >>>>>> +- 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
> >>>>> 
> >>>>> 
> >>>>> ... this one. We are using (void) + a comment when the return is ignored on
> >>>>> purpose. This is technically not-compliant with MISRA but the best we can do
> >>>>> in some situation.
> >>>>> 
> >>>>> With your proposed wording, we would technically have to remove them (or not
> >>>>> introduce new one). So I think we need to document that we are allowing
> >>>>> deviations so long they are commented.
> >>>> 
> >>>> Absolutely yes. All of these rules can have deviations as long as they
> >>>> make sense and they are commented. Note that we still have to work out
> >>>> a good tagging system so that ECLAIR and cppcheck can recognize the
> >>>> deviations automatically but for now saying that they need to be
> >>>> commented is sufficient I think.
> >>>> 
> >>>> So I'll add the following on top of the file:
> >>>> 
> >>>> """
> >>>> It is possible that in specific circumstances it is best not to follow a
> >>>> rule because it is not possible or because the alternative leads to
> >>>> better code quality. Those cases are called "deviations". They are
> >>>> permissible as long as they are documented with an in-code comment.
> >>>> """
> >>> 
> >>> Hmm, so you really mean in-code comments. I don't think this will scale
> >>> well (see e.g. the DCE related intended deviation), and it also goes
> >>> against the "no special casing for every static analysis tool" concern
> >>> I did voice on the call.
> >> 
> >> On this subject the idea was more to define a “xen” way to document
> >> deviations in the code and do it in a way so that we could easily substitute
> >> the “flag” to adapt it for each analyser using tools or command line options.
> > 
> > I think the basic scheme of something like this would want laying out
> > before doc changes like the one here actually go in, so that it's clear
> > what the action is if a new deviation needs adding for whatever reason
> > (and also allowing interested people to start contributing patches to
> > add respective annotations).
> 
> We will work on that but if we wait for everything to be solved we will
> never progress.
> I have a task on my side (ie at arm) to work on that and Luca Fancellu
> will start working on it next month.
> Now I do not think that this should block this patch, agreeing on rules does
> not mean will respect all of them in the short term so we can wait a bit as I
> definitely think that how to document violations in the code and in general
> will be a work package on its own and will require some discussion.

Right.

In general, we'll need to document these deviations and ideally they
would be documented as in-code comments because they are easier to keep
in sync with the code. But we won't be able to do that in all cases.

We'll also need a special TAG to mark the deviation. Nobody wants
multiple tagging systems for different tools (ECLAIR, cppcheck,
Coverity, etc.) We'll come up with one tagging system and introduce
conversion scripts as needed. Roberto offered to help on the call to
come up with a generic tagging system.

In some cases in-code comments for every deviation would be too verbose.
We'll want to handle it in another way. It could be a document
somewhere else, or simply disabling the Rules check in ECLAIR/cppcheck
(but that partially defeats the purpose.) We'll have to see. I think
it is going to be on a case by case basis.


In short, I don't think we have all the info and expertise to come up
with a good deviation system right now. We need to make more progress
and analize a few specific examples before we can do that. But to gain
that expertise we need to agree on a set of rules we want to follow
first, which is this patch series.


So, I think this is the best way we can start the process. We can
clarify further with the comment on top of this file, and we could even
remove the specific part about the "in-code comment" with an open-ended
statement until we come up with a clear deviation strategy. For
instance:

"""
It is possible that in specific circumstances it is best not to follow a
rule because it is not possible or because the alternative leads to
better code quality. Those cases are called "deviations". They are
permissible as long as they are documented.

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

Stefano Stabellini May 26, 2022, 7:57 p.m. UTC | #13

On Thu, 26 May 2022, Jan Beulich wrote:
> On 26.05.2022 03:12, Stefano Stabellini wrote:
> > On Wed, 25 May 2022, Jan Beulich wrote:
> >> On 25.05.2022 02:35, Stefano Stabellini wrote:
> >>> --- /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.
> 
> We need to determine where such comments are to go. I hope you don't
> mean code comments on each and every instance of similar-kind
> deviations.

I'll reply to this in the other thread.


> > 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".
> 
> Oh, I see. I didn't recognize the distinction. Maybe have a visual
> separator between the two classes?

I'll try but the layout changed significantly to become "proper RST"
following Andrew's comments. I'll see if I can come up with something.
If not, I could create two tables. First Dir, then 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)
> 
> I'd be fine with that.

Cool, I'll add it in.

Jan Beulich May 27, 2022, 6:56 a.m. UTC | #14

On 26.05.2022 21:57, Stefano Stabellini wrote:
> On Thu, 26 May 2022, Bertrand Marquis wrote:
>>> On 26 May 2022, at 11:15, Jan Beulich <[e-mail]> wrote:
>>> On 26.05.2022 11:54, Bertrand Marquis wrote:
>>>>> On 26 May 2022, at 10:43, Jan Beulich <[e-mail]> wrote:
>>>>> On 26.05.2022 03:02, Stefano Stabellini wrote:
>>>>>> On Wed, 25 May 2022, Julien Grall wrote:
>>>>>>> On 25/05/2022 01:35, Stefano Stabellini wrote:
>>>>>>>> +- 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
>>>>>>>
>>>>>>>
>>>>>>> ... this one. We are using (void) + a comment when the return is ignored on
>>>>>>> purpose. This is technically not-compliant with MISRA but the best we can do
>>>>>>> in some situation.
>>>>>>>
>>>>>>> With your proposed wording, we would technically have to remove them (or not
>>>>>>> introduce new one). So I think we need to document that we are allowing
>>>>>>> deviations so long they are commented.
>>>>>>
>>>>>> Absolutely yes. All of these rules can have deviations as long as they
>>>>>> make sense and they are commented. Note that we still have to work out
>>>>>> a good tagging system so that ECLAIR and cppcheck can recognize the
>>>>>> deviations automatically but for now saying that they need to be
>>>>>> commented is sufficient I think.
>>>>>>
>>>>>> So I'll add the following on top of the file:
>>>>>>
>>>>>> """
>>>>>> It is possible that in specific circumstances it is best not to follow a
>>>>>> rule because it is not possible or because the alternative leads to
>>>>>> better code quality. Those cases are called "deviations". They are
>>>>>> permissible as long as they are documented with an in-code comment.
>>>>>> """
>>>>>
>>>>> Hmm, so you really mean in-code comments. I don't think this will scale
>>>>> well (see e.g. the DCE related intended deviation), and it also goes
>>>>> against the "no special casing for every static analysis tool" concern
>>>>> I did voice on the call.
>>>>
>>>> On this subject the idea was more to define a “xen” way to document
>>>> deviations in the code and do it in a way so that we could easily substitute
>>>> the “flag” to adapt it for each analyser using tools or command line options.
>>>
>>> I think the basic scheme of something like this would want laying out
>>> before doc changes like the one here actually go in, so that it's clear
>>> what the action is if a new deviation needs adding for whatever reason
>>> (and also allowing interested people to start contributing patches to
>>> add respective annotations).
>>
>> We will work on that but if we wait for everything to be solved we will
>> never progress.
>> I have a task on my side (ie at arm) to work on that and Luca Fancellu
>> will start working on it next month.
>> Now I do not think that this should block this patch, agreeing on rules does
>> not mean will respect all of them in the short term so we can wait a bit as I
>> definitely think that how to document violations in the code and in general
>> will be a work package on its own and will require some discussion.
> 
> Right.
> 
> In general, we'll need to document these deviations and ideally they
> would be documented as in-code comments because they are easier to keep
> in sync with the code. But we won't be able to do that in all cases.
> 
> We'll also need a special TAG to mark the deviation. Nobody wants
> multiple tagging systems for different tools (ECLAIR, cppcheck,
> Coverity, etc.) We'll come up with one tagging system and introduce
> conversion scripts as needed. Roberto offered to help on the call to
> come up with a generic tagging system.
> 
> In some cases in-code comments for every deviation would be too verbose.
> We'll want to handle it in another way. It could be a document
> somewhere else, or simply disabling the Rules check in ECLAIR/cppcheck
> (but that partially defeats the purpose.) We'll have to see. I think
> it is going to be on a case by case basis.
> 
> 
> In short, I don't think we have all the info and expertise to come up
> with a good deviation system right now. We need to make more progress
> and analize a few specific examples before we can do that. But to gain
> that expertise we need to agree on a set of rules we want to follow
> first, which is this patch series.
> 
> 
> So, I think this is the best way we can start the process. We can
> clarify further with the comment on top of this file, and we could even
> remove the specific part about the "in-code comment" with an open-ended
> statement until we come up with a clear deviation strategy. For
> instance:
> 
> """
> It is possible that in specific circumstances it is best not to follow a
> rule because it is not possible or because the alternative leads to
> better code quality. Those cases are called "deviations". They are
> permissible as long as they are documented.
> 
> 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.
> """

This is better, yes, yet I'm still concerned of "existing codebase":
Without it being clear how to deal with deviations, what would we do
with new additions of deviations? We need to be able to say something
concrete in review comments, and prior to getting any review comments
people should at least stand a chance of being able to figure out
what's expected of them.

Jan

Stefano Stabellini May 27, 2022, 11:16 p.m. UTC | #15

On Fri, 27 May 2022, Jan Beulich wrote:
> On 26.05.2022 21:57, Stefano Stabellini wrote:
> > On Thu, 26 May 2022, Bertrand Marquis wrote:
> >>> On 26 May 2022, at 11:15, Jan Beulich <[e-mail]> wrote:
> >>> On 26.05.2022 11:54, Bertrand Marquis wrote:
> >>>>> On 26 May 2022, at 10:43, Jan Beulich <[e-mail]> wrote:
> >>>>> On 26.05.2022 03:02, Stefano Stabellini wrote:
> >>>>>> On Wed, 25 May 2022, Julien Grall wrote:
> >>>>>>> On 25/05/2022 01:35, Stefano Stabellini wrote:
> >>>>>>>> +- 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
> >>>>>>>
> >>>>>>>
> >>>>>>> ... this one. We are using (void) + a comment when the return is ignored on
> >>>>>>> purpose. This is technically not-compliant with MISRA but the best we can do
> >>>>>>> in some situation.
> >>>>>>>
> >>>>>>> With your proposed wording, we would technically have to remove them (or not
> >>>>>>> introduce new one). So I think we need to document that we are allowing
> >>>>>>> deviations so long they are commented.
> >>>>>>
> >>>>>> Absolutely yes. All of these rules can have deviations as long as they
> >>>>>> make sense and they are commented. Note that we still have to work out
> >>>>>> a good tagging system so that ECLAIR and cppcheck can recognize the
> >>>>>> deviations automatically but for now saying that they need to be
> >>>>>> commented is sufficient I think.
> >>>>>>
> >>>>>> So I'll add the following on top of the file:
> >>>>>>
> >>>>>> """
> >>>>>> It is possible that in specific circumstances it is best not to follow a
> >>>>>> rule because it is not possible or because the alternative leads to
> >>>>>> better code quality. Those cases are called "deviations". They are
> >>>>>> permissible as long as they are documented with an in-code comment.
> >>>>>> """
> >>>>>
> >>>>> Hmm, so you really mean in-code comments. I don't think this will scale
> >>>>> well (see e.g. the DCE related intended deviation), and it also goes
> >>>>> against the "no special casing for every static analysis tool" concern
> >>>>> I did voice on the call.
> >>>>
> >>>> On this subject the idea was more to define a “xen” way to document
> >>>> deviations in the code and do it in a way so that we could easily substitute
> >>>> the “flag” to adapt it for each analyser using tools or command line options.
> >>>
> >>> I think the basic scheme of something like this would want laying out
> >>> before doc changes like the one here actually go in, so that it's clear
> >>> what the action is if a new deviation needs adding for whatever reason
> >>> (and also allowing interested people to start contributing patches to
> >>> add respective annotations).
> >>
> >> We will work on that but if we wait for everything to be solved we will
> >> never progress.
> >> I have a task on my side (ie at arm) to work on that and Luca Fancellu
> >> will start working on it next month.
> >> Now I do not think that this should block this patch, agreeing on rules does
> >> not mean will respect all of them in the short term so we can wait a bit as I
> >> definitely think that how to document violations in the code and in general
> >> will be a work package on its own and will require some discussion.
> > 
> > Right.
> > 
> > In general, we'll need to document these deviations and ideally they
> > would be documented as in-code comments because they are easier to keep
> > in sync with the code. But we won't be able to do that in all cases.
> > 
> > We'll also need a special TAG to mark the deviation. Nobody wants
> > multiple tagging systems for different tools (ECLAIR, cppcheck,
> > Coverity, etc.) We'll come up with one tagging system and introduce
> > conversion scripts as needed. Roberto offered to help on the call to
> > come up with a generic tagging system.
> > 
> > In some cases in-code comments for every deviation would be too verbose.
> > We'll want to handle it in another way. It could be a document
> > somewhere else, or simply disabling the Rules check in ECLAIR/cppcheck
> > (but that partially defeats the purpose.) We'll have to see. I think
> > it is going to be on a case by case basis.
> > 
> > 
> > In short, I don't think we have all the info and expertise to come up
> > with a good deviation system right now. We need to make more progress
> > and analize a few specific examples before we can do that. But to gain
> > that expertise we need to agree on a set of rules we want to follow
> > first, which is this patch series.
> > 
> > 
> > So, I think this is the best way we can start the process. We can
> > clarify further with the comment on top of this file, and we could even
> > remove the specific part about the "in-code comment" with an open-ended
> > statement until we come up with a clear deviation strategy. For
> > instance:
> > 
> > """
> > It is possible that in specific circumstances it is best not to follow a
> > rule because it is not possible or because the alternative leads to
> > better code quality. Those cases are called "deviations". They are
> > permissible as long as they are documented.
> > 
> > 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.
> > """
> 
> This is better, yes, yet I'm still concerned of "existing codebase":
> Without it being clear how to deal with deviations, what would we do
> with new additions of deviations? We need to be able to say something
> concrete in review comments, and prior to getting any review comments
> people should at least stand a chance of being able to figure out
> what's expected of them.


I think you are right that it would be nice to provide a guideline for
new patches. Even a simple one. For new patches, if it is not an in-code
comment it could be part of the commit message. (Also it is unlikely
that a new patch would introduce very many new deviations.)

What about the following:

"""
It is possible that in specific circumstances it is best not to follow a
rule because it is not possible or because the alternative leads to
better code quality. Those cases are called "deviations". They are
permissible as long as they are documented, either as an in-code comment
or as part of the commit message. Other documentation mechanisms are
work-in-progress.

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 are work-in-progress.
"""

The goal is to provide a basic frame of reference for new patches, while
also saying that we are still working on the documentation system.

Jan Beulich May 30, 2022, 8:37 a.m. UTC | #16

On 28.05.2022 01:16, Stefano Stabellini wrote:
> On Fri, 27 May 2022, Jan Beulich wrote:
>> On 26.05.2022 21:57, Stefano Stabellini wrote:
>>> On Thu, 26 May 2022, Bertrand Marquis wrote:
>>>>> On 26 May 2022, at 11:15, Jan Beulich <[e-mail]> wrote:
>>>>> On 26.05.2022 11:54, Bertrand Marquis wrote:
>>>>>>> On 26 May 2022, at 10:43, Jan Beulich <[e-mail]> wrote:
>>>>>>> On 26.05.2022 03:02, Stefano Stabellini wrote:
>>>>>>>> On Wed, 25 May 2022, Julien Grall wrote:
>>>>>>>>> On 25/05/2022 01:35, Stefano Stabellini wrote:
>>>>>>>>>> +- 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
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> ... this one. We are using (void) + a comment when the return is ignored on
>>>>>>>>> purpose. This is technically not-compliant with MISRA but the best we can do
>>>>>>>>> in some situation.
>>>>>>>>>
>>>>>>>>> With your proposed wording, we would technically have to remove them (or not
>>>>>>>>> introduce new one). So I think we need to document that we are allowing
>>>>>>>>> deviations so long they are commented.
>>>>>>>>
>>>>>>>> Absolutely yes. All of these rules can have deviations as long as they
>>>>>>>> make sense and they are commented. Note that we still have to work out
>>>>>>>> a good tagging system so that ECLAIR and cppcheck can recognize the
>>>>>>>> deviations automatically but for now saying that they need to be
>>>>>>>> commented is sufficient I think.
>>>>>>>>
>>>>>>>> So I'll add the following on top of the file:
>>>>>>>>
>>>>>>>> """
>>>>>>>> It is possible that in specific circumstances it is best not to follow a
>>>>>>>> rule because it is not possible or because the alternative leads to
>>>>>>>> better code quality. Those cases are called "deviations". They are
>>>>>>>> permissible as long as they are documented with an in-code comment.
>>>>>>>> """
>>>>>>>
>>>>>>> Hmm, so you really mean in-code comments. I don't think this will scale
>>>>>>> well (see e.g. the DCE related intended deviation), and it also goes
>>>>>>> against the "no special casing for every static analysis tool" concern
>>>>>>> I did voice on the call.
>>>>>>
>>>>>> On this subject the idea was more to define a “xen” way to document
>>>>>> deviations in the code and do it in a way so that we could easily substitute
>>>>>> the “flag” to adapt it for each analyser using tools or command line options.
>>>>>
>>>>> I think the basic scheme of something like this would want laying out
>>>>> before doc changes like the one here actually go in, so that it's clear
>>>>> what the action is if a new deviation needs adding for whatever reason
>>>>> (and also allowing interested people to start contributing patches to
>>>>> add respective annotations).
>>>>
>>>> We will work on that but if we wait for everything to be solved we will
>>>> never progress.
>>>> I have a task on my side (ie at arm) to work on that and Luca Fancellu
>>>> will start working on it next month.
>>>> Now I do not think that this should block this patch, agreeing on rules does
>>>> not mean will respect all of them in the short term so we can wait a bit as I
>>>> definitely think that how to document violations in the code and in general
>>>> will be a work package on its own and will require some discussion.
>>>
>>> Right.
>>>
>>> In general, we'll need to document these deviations and ideally they
>>> would be documented as in-code comments because they are easier to keep
>>> in sync with the code. But we won't be able to do that in all cases.
>>>
>>> We'll also need a special TAG to mark the deviation. Nobody wants
>>> multiple tagging systems for different tools (ECLAIR, cppcheck,
>>> Coverity, etc.) We'll come up with one tagging system and introduce
>>> conversion scripts as needed. Roberto offered to help on the call to
>>> come up with a generic tagging system.
>>>
>>> In some cases in-code comments for every deviation would be too verbose.
>>> We'll want to handle it in another way. It could be a document
>>> somewhere else, or simply disabling the Rules check in ECLAIR/cppcheck
>>> (but that partially defeats the purpose.) We'll have to see. I think
>>> it is going to be on a case by case basis.
>>>
>>>
>>> In short, I don't think we have all the info and expertise to come up
>>> with a good deviation system right now. We need to make more progress
>>> and analize a few specific examples before we can do that. But to gain
>>> that expertise we need to agree on a set of rules we want to follow
>>> first, which is this patch series.
>>>
>>>
>>> So, I think this is the best way we can start the process. We can
>>> clarify further with the comment on top of this file, and we could even
>>> remove the specific part about the "in-code comment" with an open-ended
>>> statement until we come up with a clear deviation strategy. For
>>> instance:
>>>
>>> """
>>> It is possible that in specific circumstances it is best not to follow a
>>> rule because it is not possible or because the alternative leads to
>>> better code quality. Those cases are called "deviations". They are
>>> permissible as long as they are documented.
>>>
>>> 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.
>>> """
>>
>> This is better, yes, yet I'm still concerned of "existing codebase":
>> Without it being clear how to deal with deviations, what would we do
>> with new additions of deviations? We need to be able to say something
>> concrete in review comments, and prior to getting any review comments
>> people should at least stand a chance of being able to figure out
>> what's expected of them.
> 
> 
> I think you are right that it would be nice to provide a guideline for
> new patches. Even a simple one. For new patches, if it is not an in-code
> comment it could be part of the commit message. (Also it is unlikely
> that a new patch would introduce very many new deviations.)
> 
> What about the following:
> 
> """
> It is possible that in specific circumstances it is best not to follow a
> rule because it is not possible or because the alternative leads to
> better code quality. Those cases are called "deviations". They are
> permissible as long as they are documented, either as an in-code comment
> or as part of the commit message. Other documentation mechanisms are
> work-in-progress.
> 
> 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 are work-in-progress.
> """
> 
> The goal is to provide a basic frame of reference for new patches, while
> also saying that we are still working on the documentation system.

Fine with me for the time being.

Jan

Julien Grall May 30, 2022, 9:12 a.m. UTC | #17

Hi Stefano,

On 28/05/2022 00:16, Stefano Stabellini wrote:
> """
> It is possible that in specific circumstances it is best not to follow a
> rule because it is not possible or because the alternative leads to
> better code quality. Those cases are called "deviations". They are
> permissible as long as they are documented, either as an in-code comment
> or as part of the commit message. Other documentation mechanisms are

I would drop the "as part of the commit message" because it is a lot 
more difficult to associate the deviation with a rationale (the code may 
have been moved and you would need to go through the history).

Other than that, the text looks fine.

Cheers,

Jan Beulich May 30, 2022, 9:16 a.m. UTC | #18

On 30.05.2022 11:12, Julien Grall wrote:
> On 28/05/2022 00:16, Stefano Stabellini wrote:
>> """
>> It is possible that in specific circumstances it is best not to follow a
>> rule because it is not possible or because the alternative leads to
>> better code quality. Those cases are called "deviations". They are
>> permissible as long as they are documented, either as an in-code comment
>> or as part of the commit message. Other documentation mechanisms are
> 
> I would drop the "as part of the commit message" because it is a lot 
> more difficult to associate the deviation with a rationale (the code may 
> have been moved and you would need to go through the history).

But this was added in response to me pointing out that code comments
aren't standardized yet as to their format. The alternative, as said
before, would be to come up with a scheme first, before starting to
mandate playing by certain of the rules (and hence requiring deviations
to be documented).

Jan

Julien Grall May 30, 2022, 9:27 a.m. UTC | #19

Hi,

On 30/05/2022 10:16, Jan Beulich wrote:
> On 30.05.2022 11:12, Julien Grall wrote:
>> On 28/05/2022 00:16, Stefano Stabellini wrote:
>>> """
>>> It is possible that in specific circumstances it is best not to follow a
>>> rule because it is not possible or because the alternative leads to
>>> better code quality. Those cases are called "deviations". They are
>>> permissible as long as they are documented, either as an in-code comment
>>> or as part of the commit message. Other documentation mechanisms are
>>
>> I would drop the "as part of the commit message" because it is a lot
>> more difficult to associate the deviation with a rationale (the code may
>> have been moved and you would need to go through the history).
> 
> But this was added in response to me pointing out that code comments
> aren't standardized yet as to their format. The alternative, as said
> before, would be to come up with a scheme first, before starting to
> mandate playing by certain of the rules (and hence requiring deviations
> to be documented).

I don't think this is necessary short term. It is easy to rework a 
comment after the fact. It is a lot more difficult to go through the 
history and find the rationale.

Documenting the deviation in the commit message also makes a lot more 
difficult to triage issues reported by scanner. With a comment you could 
just read it from the same "page" (scanner will usually provide context).

So overall, I think allowing to document deviations in a commit message 
is a pretty bad move (the more if it is a short term solution).

Cheers,

Jan Beulich May 30, 2022, 9:33 a.m. UTC | #20

On 30.05.2022 11:27, Julien Grall wrote:
> Hi,
> 
> On 30/05/2022 10:16, Jan Beulich wrote:
>> On 30.05.2022 11:12, Julien Grall wrote:
>>> On 28/05/2022 00:16, Stefano Stabellini wrote:
>>>> """
>>>> It is possible that in specific circumstances it is best not to follow a
>>>> rule because it is not possible or because the alternative leads to
>>>> better code quality. Those cases are called "deviations". They are
>>>> permissible as long as they are documented, either as an in-code comment
>>>> or as part of the commit message. Other documentation mechanisms are
>>>
>>> I would drop the "as part of the commit message" because it is a lot
>>> more difficult to associate the deviation with a rationale (the code may
>>> have been moved and you would need to go through the history).
>>
>> But this was added in response to me pointing out that code comments
>> aren't standardized yet as to their format. The alternative, as said
>> before, would be to come up with a scheme first, before starting to
>> mandate playing by certain of the rules (and hence requiring deviations
>> to be documented).
> 
> I don't think this is necessary short term. It is easy to rework a 
> comment after the fact. It is a lot more difficult to go through the 
> history and find the rationale.

We all know what "short term" may mean - we may remain in this mode of
operation for an extended period of time. It'll potentially be quite a
bit of churn to subsequently adjust all such comments which would
have accumulated, and - for not being standardized - can't easily be
grep-ed for. By documenting things in the commit message the state of
the code base doesn't change, and we'll continue to rely on scanners
to locate sets of candidates for adjustment or deviation commentary.

Jan

Julien Grall May 30, 2022, 9:41 a.m. UTC | #21

On 30/05/2022 10:33, Jan Beulich wrote:
> On 30.05.2022 11:27, Julien Grall wrote:
>> Hi,
>>
>> On 30/05/2022 10:16, Jan Beulich wrote:
>>> On 30.05.2022 11:12, Julien Grall wrote:
>>>> On 28/05/2022 00:16, Stefano Stabellini wrote:
>>>>> """
>>>>> It is possible that in specific circumstances it is best not to follow a
>>>>> rule because it is not possible or because the alternative leads to
>>>>> better code quality. Those cases are called "deviations". They are
>>>>> permissible as long as they are documented, either as an in-code comment
>>>>> or as part of the commit message. Other documentation mechanisms are
>>>>
>>>> I would drop the "as part of the commit message" because it is a lot
>>>> more difficult to associate the deviation with a rationale (the code may
>>>> have been moved and you would need to go through the history).
>>>
>>> But this was added in response to me pointing out that code comments
>>> aren't standardized yet as to their format. The alternative, as said
>>> before, would be to come up with a scheme first, before starting to
>>> mandate playing by certain of the rules (and hence requiring deviations
>>> to be documented).
>>
>> I don't think this is necessary short term. It is easy to rework a
>> comment after the fact. It is a lot more difficult to go through the
>> history and find the rationale.
> 
> We all know what "short term" may mean - we may remain in this mode of
> operation for an extended period of time. It'll potentially be quite a
> bit of churn to subsequently adjust all such comments which would
> have accumulated, and - for not being standardized - can't easily be
> grep-ed for.

Well... Scanner will likely point out the issues we deviate from. So you 
we have an easy way to know where the comments need to be adjusted.

> By documenting things in the commit message the state of
> the code base doesn't change, and we'll continue to rely on scanners
> to locate sets of candidates for adjustment or deviation commentary.

The part I am missing how documenting the deviations in the commit 
message help... Can you clarify it?

Cheers,

Jan Beulich May 30, 2022, 9:55 a.m. UTC | #22

On 30.05.2022 11:41, Julien Grall wrote:
> 
> 
> On 30/05/2022 10:33, Jan Beulich wrote:
>> On 30.05.2022 11:27, Julien Grall wrote:
>>> Hi,
>>>
>>> On 30/05/2022 10:16, Jan Beulich wrote:
>>>> On 30.05.2022 11:12, Julien Grall wrote:
>>>>> On 28/05/2022 00:16, Stefano Stabellini wrote:
>>>>>> """
>>>>>> It is possible that in specific circumstances it is best not to follow a
>>>>>> rule because it is not possible or because the alternative leads to
>>>>>> better code quality. Those cases are called "deviations". They are
>>>>>> permissible as long as they are documented, either as an in-code comment
>>>>>> or as part of the commit message. Other documentation mechanisms are
>>>>>
>>>>> I would drop the "as part of the commit message" because it is a lot
>>>>> more difficult to associate the deviation with a rationale (the code may
>>>>> have been moved and you would need to go through the history).
>>>>
>>>> But this was added in response to me pointing out that code comments
>>>> aren't standardized yet as to their format. The alternative, as said
>>>> before, would be to come up with a scheme first, before starting to
>>>> mandate playing by certain of the rules (and hence requiring deviations
>>>> to be documented).
>>>
>>> I don't think this is necessary short term. It is easy to rework a
>>> comment after the fact. It is a lot more difficult to go through the
>>> history and find the rationale.
>>
>> We all know what "short term" may mean - we may remain in this mode of
>> operation for an extended period of time. It'll potentially be quite a
>> bit of churn to subsequently adjust all such comments which would
>> have accumulated, and - for not being standardized - can't easily be
>> grep-ed for.
> 
> Well... Scanner will likely point out the issues we deviate from. So you 
> we have an easy way to know where the comments need to be adjusted.
> 
>> By documenting things in the commit message the state of
>> the code base doesn't change, and we'll continue to rely on scanners
>> to locate sets of candidates for adjustment or deviation commentary.
> 
> The part I am missing how documenting the deviations in the commit 
> message help... Can you clarify it?

I understood Stefano for this to merely be for the purpose of justifying
the deviation (preempting review comments).

Jan

George Dunlap May 30, 2022, 10:21 a.m. UTC | #23

> On 30 May 2022, at 10:55, Jan Beulich <[e-mail] <mailto:[e-mail]>> wrote:
> 
> On 30.05.2022 11:41, Julien Grall wrote:
>> 
>> 
>> On 30/05/2022 10:33, Jan Beulich wrote:
>>> On 30.05.2022 11:27, Julien Grall wrote:
>>>> Hi,
>>>> 
>>>> On 30/05/2022 10:16, Jan Beulich wrote:
>>>>> On 30.05.2022 11:12, Julien Grall wrote:
>>>>>> On 28/05/2022 00:16, Stefano Stabellini wrote:
>>>>>>> """
>>>>>>> It is possible that in specific circumstances it is best not to follow a
>>>>>>> rule because it is not possible or because the alternative leads to
>>>>>>> better code quality. Those cases are called "deviations". They are
>>>>>>> permissible as long as they are documented, either as an in-code comment
>>>>>>> or as part of the commit message. Other documentation mechanisms are
>>>>>> 
>>>>>> I would drop the "as part of the commit message" because it is a lot
>>>>>> more difficult to associate the deviation with a rationale (the code may
>>>>>> have been moved and you would need to go through the history).
>>>>> 
>>>>> But this was added in response to me pointing out that code comments
>>>>> aren't standardized yet as to their format. The alternative, as said
>>>>> before, would be to come up with a scheme first, before starting to
>>>>> mandate playing by certain of the rules (and hence requiring deviations
>>>>> to be documented).
>>>> 
>>>> I don't think this is necessary short term. It is easy to rework a
>>>> comment after the fact. It is a lot more difficult to go through the
>>>> history and find the rationale.
>>> 
>>> We all know what "short term" may mean - we may remain in this mode of
>>> operation for an extended period of time. It'll potentially be quite a
>>> bit of churn to subsequently adjust all such comments which would
>>> have accumulated, and - for not being standardized - can't easily be
>>> grep-ed for.
>> 
>> Well... Scanner will likely point out the issues we deviate from. So you
>> we have an easy way to know where the comments need to be adjusted.
>> 
>>> By documenting things in the commit message the state of
>>> the code base doesn't change, and we'll continue to rely on scanners
>>> to locate sets of candidates for adjustment or deviation commentary.
>> 
>> The part I am missing how documenting the deviations in the commit
>> message help... Can you clarify it?
> 
> I understood Stefano for this to merely be for the purpose of justifying
> the deviation (preempting review comments).

Right, so at a very minimum, if we say “This is a rule now”, and a submitter wants a deviation from that rule, then the reviewer needs to know the justification for the deviation.  The commit message is the obvious place for that.

Obviously something *else* we might want is a more convenient way to keep that rationale for the future, when we start to officially document deviations.  Given that the scanner will point out all the places where deviations happen, I don’t think an unstructured comment with an informal summary of the justification would be a problem — it seems like it would be a lot easier, when we start to officially document deviations, to transform comments in the existing codebase, than to search through the mailing lists and/or git commit history to find the rationale (or try to work out unaided what the intent was).  But I don’t have strong opinions on the matter.

 -George

Bertrand Marquis May 30, 2022, 1:35 p.m. UTC | #24

Hi,

> On 30 May 2022, at 11:21, George Dunlap <[e-mail]> wrote:
> 
> 
> 
>> On 30 May 2022, at 10:55, Jan Beulich <[e-mail]> wrote:
>> 
>> On 30.05.2022 11:41, Julien Grall wrote:
>>> 
>>> 
>>> On 30/05/2022 10:33, Jan Beulich wrote:
>>>> On 30.05.2022 11:27, Julien Grall wrote:
>>>>> Hi,
>>>>> 
>>>>> On 30/05/2022 10:16, Jan Beulich wrote:
>>>>>> On 30.05.2022 11:12, Julien Grall wrote:
>>>>>>> On 28/05/2022 00:16, Stefano Stabellini wrote:
>>>>>>>> """
>>>>>>>> It is possible that in specific circumstances it is best not to follow a
>>>>>>>> rule because it is not possible or because the alternative leads to
>>>>>>>> better code quality. Those cases are called "deviations". They are
>>>>>>>> permissible as long as they are documented, either as an in-code comment
>>>>>>>> or as part of the commit message. Other documentation mechanisms are
>>>>>>> 
>>>>>>> I would drop the "as part of the commit message" because it is a lot
>>>>>>> more difficult to associate the deviation with a rationale (the code may
>>>>>>> have been moved and you would need to go through the history).
>>>>>> 
>>>>>> But this was added in response to me pointing out that code comments
>>>>>> aren't standardized yet as to their format. The alternative, as said
>>>>>> before, would be to come up with a scheme first, before starting to
>>>>>> mandate playing by certain of the rules (and hence requiring deviations
>>>>>> to be documented).
>>>>> 
>>>>> I don't think this is necessary short term. It is easy to rework a
>>>>> comment after the fact. It is a lot more difficult to go through the
>>>>> history and find the rationale.
>>>> 
>>>> We all know what "short term" may mean - we may remain in this mode of
>>>> operation for an extended period of time. It'll potentially be quite a
>>>> bit of churn to subsequently adjust all such comments which would
>>>> have accumulated, and - for not being standardized - can't easily be
>>>> grep-ed for.
>>> 
>>> Well... Scanner will likely point out the issues we deviate from. So you 
>>> we have an easy way to know where the comments need to be adjusted.
>>> 
>>>> By documenting things in the commit message the state of
>>>> the code base doesn't change, and we'll continue to rely on scanners
>>>> to locate sets of candidates for adjustment or deviation commentary.
>>> 
>>> The part I am missing how documenting the deviations in the commit 
>>> message help... Can you clarify it?
>> 
>> I understood Stefano for this to merely be for the purpose of justifying
>> the deviation (preempting review comments).
> 
> Right, so at a very minimum, if we say “This is a rule now”, and a submitter wants a deviation from that rule, then the reviewer needs to know the justification for the deviation.  The commit message is the obvious place for that.

Agree

> 
> Obviously something *else* we might want is a more convenient way to keep that rationale for the future, when we start to officially document deviations.  Given that the scanner will point out all the places where deviations happen, I don’t think an unstructured comment with an informal summary of the justification would be a problem — it seems like it would be a lot easier, when we start to officially document deviations, to transform comments in the existing codebase, than to search through the mailing lists and/or git commit history to find the rationale (or try to work out unaided what the intent was).  But I don’t have strong opinions on the matter.

Maybe agreeing on a simple tag to start that can later be improved (Luca Fancellu on my side will start working on that with the FuSa SIG and Eclair next month).

So I would suggest:

/**
 * MISRA_DEV: Rule ID
 * xxxxx justification
 *
 */

Whenever we will have defined the final way, we will replace those entries with the new system.

Would that be an agreeable solution ?

Regards
Bertrand


> 
>  -George

Julien Grall May 31, 2022, 9:41 a.m. UTC | #25

Hi,

On 30/05/2022 14:35, Bertrand Marquis wrote:
>> Obviously something *else* we might want is a more convenient way to keep that rationale for the future, when we start to officially document deviations.  Given that the scanner will point out all the places where deviations happen, I don’t think an unstructured comment with an informal summary of the justification would be a problem — it seems like it would be a lot easier, when we start to officially document deviations, to transform comments in the existing codebase, than to search through the mailing lists and/or git commit history to find the rationale (or try to work out unaided what the intent was).  But I don’t have strong opinions on the matter.
> 
> Maybe agreeing on a simple tag to start that can later be improved (Luca Fancellu on my side will start working on that with the FuSa SIG and Eclair next month).
> 
> So I would suggest:
> 
> /**
>   * MISRA_DEV: Rule ID
>   * xxxxx justification
>   *
>   */
> 
> Whenever we will have defined the final way, we will replace those entries with the new system.
> 
> Would that be an agreeable solution ?

I am fine with that. With one NIT thought, in Xen comments the first 
line of multi-line comment is "/*" rather than "/**".

Cheers,

Stefano Stabellini June 1, 2022, 1:25 a.m. UTC | #26

On Tue, 31 May 2022, Julien Grall wrote:
> Hi,
> 
> On 30/05/2022 14:35, Bertrand Marquis wrote:
> > > Obviously something *else* we might want is a more convenient way to keep
> > > that rationale for the future, when we start to officially document
> > > deviations.  Given that the scanner will point out all the places where
> > > deviations happen, I don’t think an unstructured comment with an informal
> > > summary of the justification would be a problem — it seems like it would
> > > be a lot easier, when we start to officially document deviations, to
> > > transform comments in the existing codebase, than to search through the
> > > mailing lists and/or git commit history to find the rationale (or try to
> > > work out unaided what the intent was).  But I don’t have strong opinions
> > > on the matter.
> > 
> > Maybe agreeing on a simple tag to start that can later be improved (Luca
> > Fancellu on my side will start working on that with the FuSa SIG and Eclair
> > next month).
> > 
> > So I would suggest:
> > 
> > /**
> >   * MISRA_DEV: Rule ID
> >   * xxxxx justification
> >   *
> >   */
> > 
> > Whenever we will have defined the final way, we will replace those entries
> > with the new system.
> > 
> > Would that be an agreeable solution ?
> 
> I am fine with that. With one NIT thought, in Xen comments the first line of
> multi-line comment is "/*" rather than "/**".

I went with this (added it on top of the file.) As George wrote, I don't
have a strong opinion as at this stage we just need to get the ball
rolling and all options are OK.

Patch

diff --git a/CODING_STYLE b/CODING_STYLE
index 9f50d9cec4..1ef35ee8d0 100644
--- 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.
diff --git a/docs/misra/rules.rst b/docs/misra/rules.rst
new file mode 100644
index 0000000000..c0ee58ab25
--- /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
+- 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
+- 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
+- Rule: Rule 8.8
+  - Severity:  Required
+  - Summary:  The static storage class specifier shall be used in all declarations of objects and functions that have internal linkage
+  - Link:  https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_08_08.c
+- Rule: Rule 8.12
+  - Severity:  Required
+  - Summary:  Within an enumerator list the value of an implicitly-specified enumeration constant shall be unique
+  - Link:  https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_08_12.c