[PATCH 0/2] introduce docs/misra/rules.rst

[PATCH 0/2] introduce docs/misra/rules.rst

[Stefano Stabellini]

Hi all,

This patch series is a follow-up from the MISRA C meeting last Thursday,
when we went through the list of MISRA C rules on the spreadsheet and
agreed to accept into the Xen coding style the first ones, starting from
Dir 2.1 up until Rule 5.1 (except for Rule 2.2) in descending popularity
order.

This is the full list of accepted rules so far:

Dir 2.1
Dir 4.7
Dir 4.10
Dir 4.14
Rule 1.3
Rule 3.2
Rule 5.1
Rule 6.2
Rule 8.1
Rule 8.4
Rule 8.5
Rule 8.6
Rule 8.8
Rule 8.12

This short patch series add them as a new document under docs/misra as a
list in rst format. The file can be used as input to cppcheck using a
small python script from Bertrand (who will send it to the xen-devel
separately.)

Cheers,

Stefano

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

[Stefano Stabellini]

From: Stefano Stabellini <stefano.stabellini@xilinx.com>

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 <bertrand.marquis@arm.com>
Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
---
 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.
+
+- 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
-- 
2.25.1

[PATCH 2/2] docs/misra: add Rule 5.1

[Stefano Stabellini]

From: Stefano Stabellini <stefano.stabellini@xilinx.com>

Add Rule 5.1, with the additional note that the character limit for Xen
is 63 characters.

The max length identifiers found by ECLAIR are:

__mitigate_spectre_bhb_clear_insn_start
domain_pause_by_systemcontroller_nosync

Both of them are 40 characters long. A limit of 63 characters work for
the existing code.

Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
---
 docs/misra/rules.rst | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/docs/misra/rules.rst b/docs/misra/rules.rst
index c0ee58ab25..6052fc6309 100644
--- a/docs/misra/rules.rst
+++ b/docs/misra/rules.rst
@@ -35,6 +35,11 @@ hypervisor.
   - 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 5.1
+  - Severity:  Required
+  - Summary:  External identifiers shall be distinct
+  - Link:  https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_05_01_2.c
+  - Note: the Xen characters limit for identifiers is 63
 - Rule: Rule 6.2
   - Severity:  Required
   - Summary:  Single-bit named bit fields shall not be of a signed type
-- 
2.25.1

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

[Julien Grall]

Hi Stefano,

On 25/05/2022 01:35, Stefano Stabellini wrote:
> From: Stefano Stabellini <stefano.stabellini@xilinx.com>
> 
> 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 <bertrand.marquis@arm.com>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>

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,

-- 
Julien Grall

Re: [PATCH 2/2] docs/misra: add Rule 5.1

[Julien Grall]

Hi,

On 25/05/2022 01:35, Stefano Stabellini wrote:
> From: Stefano Stabellini <stefano.stabellini@xilinx.com>
> 
> Add Rule 5.1, with the additional note that the character limit for Xen
> is 63 characters.

63 is a bit an odd numbers. Why not 64?

> 
> The max length identifiers found by ECLAIR are:
> 
> __mitigate_spectre_bhb_clear_insn_start
> domain_pause_by_systemcontroller_nosync
> 
> Both of them are 40 characters long. A limit of 63 characters work for
> the existing code.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>

Regardless what I wrote above:

Acked-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall

Re: [PATCH 2/2] docs/misra: add Rule 5.1

[Jan Beulich]

On 25.05.2022 02:35, Stefano Stabellini wrote:
> From: Stefano Stabellini <stefano.stabellini@xilinx.com>
> 
> Add Rule 5.1, with the additional note that the character limit for Xen
> is 63 characters.
> 
> The max length identifiers found by ECLAIR are:
> 
> __mitigate_spectre_bhb_clear_insn_start
> domain_pause_by_systemcontroller_nosync
> 
> Both of them are 40 characters long. A limit of 63 characters work for
> the existing code.

I have to admit that it hasn't become clear to me why we want to
permit (if not to say encourage) the use of such long identifiers.
If 40 is the longest we've got, why not limit it to 40 for now
with a goal of further reducing? A 40-char symbol plus some
indentation will already pose problems with 80-char line length.

Otoh, as said on the call, I think the public headers want
mentioning explicitly here in some way. Part of them (most or all
of what's under io/) aren't used when building Xen, so won't be
seen by Eclair (aiui). Yet they are a formal part of the code
base, and e.g. ring.h has some pretty long names (albeit still
below 40 chars as it looks). So once we're able to go down to e.g.
32 for the bulk of the code base, public headers should imo still
be explicitly allowed to use longer identifiers.

Jan

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

[Jan Beulich]

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

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

[Andrew Cooper]

On 25/05/2022 01:35, Stefano Stabellini wrote:
> From: Stefano Stabellini <stefano.stabellini@xilinx.com>
>
> 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 <bertrand.marquis@arm.com>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>

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/20191003205623.20839-4-andrew.cooper3@citrix.com/

~Andrew

Re: [PATCH 2/2] docs/misra: add Rule 5.1

[Andrew Cooper]

On 25/05/2022 08:40, Julien Grall wrote:
> Hi,
>
> On 25/05/2022 01:35, Stefano Stabellini wrote:
>> From: Stefano Stabellini <stefano.stabellini@xilinx.com>
>>
>> Add Rule 5.1, with the additional note that the character limit for Xen
>> is 63 characters.
>
> 63 is a bit an odd numbers. Why not 64?

Because of the NUL terminator in a power-of-2 buffer in the compiler. 
That's why all of these spec limits exist in the first place.

~Andrew

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

[Stefano Stabellini]

On Wed, 25 May 2022, Julien Grall wrote:
> Hi Stefano,
> 
> On 25/05/2022 01:35, Stefano Stabellini wrote:
> > From: Stefano Stabellini <stefano.stabellini@xilinx.com>
> > 
> > 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 <bertrand.marquis@arm.com>
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> 
> 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.
"""

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

[Stefano Stabellini]

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.

Re: [PATCH 2/2] docs/misra: add Rule 5.1

[Stefano Stabellini]

On Wed, 25 May 2022, Jan Beulich wrote:
> On 25.05.2022 02:35, Stefano Stabellini wrote:
> > From: Stefano Stabellini <stefano.stabellini@xilinx.com>
> > 
> > Add Rule 5.1, with the additional note that the character limit for Xen
> > is 63 characters.
> > 
> > The max length identifiers found by ECLAIR are:
> > 
> > __mitigate_spectre_bhb_clear_insn_start
> > domain_pause_by_systemcontroller_nosync
> > 
> > Both of them are 40 characters long. A limit of 63 characters work for
> > the existing code.
> 
> I have to admit that it hasn't become clear to me why we want to
> permit (if not to say encourage) the use of such long identifiers.
> If 40 is the longest we've got, why not limit it to 40 for now
> with a goal of further reducing? A 40-char symbol plus some
> indentation will already pose problems with 80-char line length.
 
We can go lower than 63 if we want to. I chose the closest power-of-two
length -1 for the NUL terminator. But it doesn't have to be a
power-of-two. So we could go with "41" if we want to.

Anyone in favor of that? I am happy with any number between 41 and 63.


> Otoh, as said on the call, I think the public headers want
> mentioning explicitly here in some way. Part of them (most or all
> of what's under io/) aren't used when building Xen, so won't be
> seen by Eclair (aiui). Yet they are a formal part of the code
> base, and e.g. ring.h has some pretty long names (albeit still
> below 40 chars as it looks). So once we're able to go down to e.g.
> 32 for the bulk of the code base, public headers should imo still
> be explicitly allowed to use longer identifiers.

Actually I thought about writing something for the public header but I
wasn't sure what to write. What about:

- Note: the Xen characters limit for identifiers is 41. Public headers
  (xen/include/public/) are allowed to retain longer identifiers for
  backward compatibility.

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

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 4537 bytes --]

On Wed, 25 May 2022, Andrew Cooper wrote:
> On 25/05/2022 01:35, Stefano Stabellini wrote:
> > From: Stefano Stabellini <stefano.stabellini@xilinx.com>
> >
> > 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 <bertrand.marquis@arm.com>
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> 
> 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/20191003205623.20839-4-andrew.cooper3@citrix.com/

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>`_

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

[Jan Beulich]

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

Re: [PATCH 2/2] docs/misra: add Rule 5.1

[Jan Beulich]

On 26.05.2022 03:18, Stefano Stabellini wrote:
> On Wed, 25 May 2022, Jan Beulich wrote:
>> On 25.05.2022 02:35, Stefano Stabellini wrote:
>>> From: Stefano Stabellini <stefano.stabellini@xilinx.com>
>>>
>>> Add Rule 5.1, with the additional note that the character limit for Xen
>>> is 63 characters.
>>>
>>> The max length identifiers found by ECLAIR are:
>>>
>>> __mitigate_spectre_bhb_clear_insn_start
>>> domain_pause_by_systemcontroller_nosync
>>>
>>> Both of them are 40 characters long. A limit of 63 characters work for
>>> the existing code.
>>
>> I have to admit that it hasn't become clear to me why we want to
>> permit (if not to say encourage) the use of such long identifiers.
>> If 40 is the longest we've got, why not limit it to 40 for now
>> with a goal of further reducing? A 40-char symbol plus some
>> indentation will already pose problems with 80-char line length.
>  
> We can go lower than 63 if we want to. I chose the closest power-of-two
> length -1 for the NUL terminator. But it doesn't have to be a
> power-of-two. So we could go with "41" if we want to.
> 
> Anyone in favor of that? I am happy with any number between 41 and 63.

Why 41, not 40? 41 seems yet more arbitrary to me than e.g. 40.

>> Otoh, as said on the call, I think the public headers want
>> mentioning explicitly here in some way. Part of them (most or all
>> of what's under io/) aren't used when building Xen, so won't be
>> seen by Eclair (aiui). Yet they are a formal part of the code
>> base, and e.g. ring.h has some pretty long names (albeit still
>> below 40 chars as it looks). So once we're able to go down to e.g.
>> 32 for the bulk of the code base, public headers should imo still
>> be explicitly allowed to use longer identifiers.
> 
> Actually I thought about writing something for the public header but I
> wasn't sure what to write. What about:
> 
> - Note: the Xen characters limit for identifiers is 41. Public headers
>   (xen/include/public/) are allowed to retain longer identifiers for
>   backward compatibility.

Fine with me, except I wonder in how far going forward we actually
need to play by that limit there. Proper name-spacing is particularly
important in the public headers, so may warrant a higher limit for
certain (unusual?) circumstances.

Jan

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

[Jan Beulich]

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

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

[Bertrand Marquis]

Hi Jan,

> On 26 May 2022, at 10:43, Jan Beulich <jbeulich@suse.com> 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

Re: [PATCH 2/2] docs/misra: add Rule 5.1

[Bertrand Marquis]

> On 26 May 2022, at 10:40, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 26.05.2022 03:18, Stefano Stabellini wrote:
>> On Wed, 25 May 2022, Jan Beulich wrote:
>>> On 25.05.2022 02:35, Stefano Stabellini wrote:
>>>> From: Stefano Stabellini <stefano.stabellini@xilinx.com>
>>>> 
>>>> Add Rule 5.1, with the additional note that the character limit for Xen
>>>> is 63 characters.
>>>> 
>>>> The max length identifiers found by ECLAIR are:
>>>> 
>>>> __mitigate_spectre_bhb_clear_insn_start
>>>> domain_pause_by_systemcontroller_nosync
>>>> 
>>>> Both of them are 40 characters long. A limit of 63 characters work for
>>>> the existing code.
>>> 
>>> I have to admit that it hasn't become clear to me why we want to
>>> permit (if not to say encourage) the use of such long identifiers.
>>> If 40 is the longest we've got, why not limit it to 40 for now
>>> with a goal of further reducing? A 40-char symbol plus some
>>> indentation will already pose problems with 80-char line length.
>> 
>> We can go lower than 63 if we want to. I chose the closest power-of-two
>> length -1 for the NUL terminator. But it doesn't have to be a
>> power-of-two. So we could go with "41" if we want to.
>> 
>> Anyone in favor of that? I am happy with any number between 41 and 63.
> 
> Why 41, not 40? 41 seems yet more arbitrary to me than e.g. 40.
> 
>>> Otoh, as said on the call, I think the public headers want
>>> mentioning explicitly here in some way. Part of them (most or all
>>> of what's under io/) aren't used when building Xen, so won't be
>>> seen by Eclair (aiui). Yet they are a formal part of the code
>>> base, and e.g. ring.h has some pretty long names (albeit still
>>> below 40 chars as it looks). So once we're able to go down to e.g.
>>> 32 for the bulk of the code base, public headers should imo still
>>> be explicitly allowed to use longer identifiers.
>> 
>> Actually I thought about writing something for the public header but I
>> wasn't sure what to write. What about:
>> 
>> - Note: the Xen characters limit for identifiers is 41. Public headers
>> (xen/include/public/) are allowed to retain longer identifiers for
>> backward compatibility.
> 
> Fine with me, except I wonder in how far going forward we actually
> need to play by that limit there. Proper name-spacing is particularly
> important in the public headers, so may warrant a higher limit for
> certain (unusual?) circumstances.

I think we can have a “global” deviation for public headers with an higher
limit but there should still be a limit.

Bertrand

> 
> Jan

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

[Jan Beulich]

On 26.05.2022 11:54, Bertrand Marquis wrote:
> Hi Jan,
> 
>> On 26 May 2022, at 10:43, Jan Beulich <jbeulich@suse.com> 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

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

[Bertrand Marquis]

Hi Jan,

> On 26 May 2022, at 11:15, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 26.05.2022 11:54, Bertrand Marquis wrote:
>> Hi Jan,
>> 
>>> On 26 May 2022, at 10:43, Jan Beulich <jbeulich@suse.com> 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

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

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 5185 bytes --]

On Thu, 26 May 2022, Bertrand Marquis wrote:
> > On 26 May 2022, at 11:15, Jan Beulich <jbeulich@suse.com> wrote:
> > On 26.05.2022 11:54, Bertrand Marquis wrote:
> >>> On 26 May 2022, at 10:43, Jan Beulich <jbeulich@suse.com> 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.
"""

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

[Stefano Stabellini]

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.

Re: [PATCH 2/2] docs/misra: add Rule 5.1

[Stefano Stabellini]

On Thu, 26 May 2022, Jan Beulich wrote:
> On 26.05.2022 03:18, Stefano Stabellini wrote:
> > On Wed, 25 May 2022, Jan Beulich wrote:
> >> On 25.05.2022 02:35, Stefano Stabellini wrote:
> >>> From: Stefano Stabellini <stefano.stabellini@xilinx.com>
> >>>
> >>> Add Rule 5.1, with the additional note that the character limit for Xen
> >>> is 63 characters.
> >>>
> >>> The max length identifiers found by ECLAIR are:
> >>>
> >>> __mitigate_spectre_bhb_clear_insn_start
> >>> domain_pause_by_systemcontroller_nosync
> >>>
> >>> Both of them are 40 characters long. A limit of 63 characters work for
> >>> the existing code.
> >>
> >> I have to admit that it hasn't become clear to me why we want to
> >> permit (if not to say encourage) the use of such long identifiers.
> >> If 40 is the longest we've got, why not limit it to 40 for now
> >> with a goal of further reducing? A 40-char symbol plus some
> >> indentation will already pose problems with 80-char line length.
> >  
> > We can go lower than 63 if we want to. I chose the closest power-of-two
> > length -1 for the NUL terminator. But it doesn't have to be a
> > power-of-two. So we could go with "41" if we want to.
> > 
> > Anyone in favor of that? I am happy with any number between 41 and 63.
> 
> Why 41, not 40? 41 seems yet more arbitrary to me than e.g. 40.

I was adding +1 to account for the NUL terminator, but actually I
already added it before (__mitigate_spectre_bhb_clear_insn_start and
domain_pause_by_systemcontroller_nosync are 39 characters).

So yes we can go with 40.


> >> Otoh, as said on the call, I think the public headers want
> >> mentioning explicitly here in some way. Part of them (most or all
> >> of what's under io/) aren't used when building Xen, so won't be
> >> seen by Eclair (aiui). Yet they are a formal part of the code
> >> base, and e.g. ring.h has some pretty long names (albeit still
> >> below 40 chars as it looks). So once we're able to go down to e.g.
> >> 32 for the bulk of the code base, public headers should imo still
> >> be explicitly allowed to use longer identifiers.
> > 
> > Actually I thought about writing something for the public header but I
> > wasn't sure what to write. What about:
> > 
> > - Note: the Xen characters limit for identifiers is 41. Public headers
> >   (xen/include/public/) are allowed to retain longer identifiers for
> >   backward compatibility.
> 
> Fine with me, except I wonder in how far going forward we actually
> need to play by that limit there. Proper name-spacing is particularly
> important in the public headers, so may warrant a higher limit for
> certain (unusual?) circumstances.

Keep in mind that the rules are general guidelines and good defaults.
There can be always special cases, and that is especially true for
"unusual circumstances".

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

[Jan Beulich]

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 <jbeulich@suse.com> wrote:
>>> On 26.05.2022 11:54, Bertrand Marquis wrote:
>>>>> On 26 May 2022, at 10:43, Jan Beulich <jbeulich@suse.com> 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

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

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 7028 bytes --]

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 <jbeulich@suse.com> wrote:
> >>> On 26.05.2022 11:54, Bertrand Marquis wrote:
> >>>>> On 26 May 2022, at 10:43, Jan Beulich <jbeulich@suse.com> 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.

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

[Jan Beulich]

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 <jbeulich@suse.com> wrote:
>>>>> On 26.05.2022 11:54, Bertrand Marquis wrote:
>>>>>>> On 26 May 2022, at 10:43, Jan Beulich <jbeulich@suse.com> 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

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

[Julien Grall]

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,

-- 
Julien Grall

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

[Jan Beulich]

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

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

[Julien Grall]

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,

-- 
Julien Grall

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

[Jan Beulich]

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

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

[Julien Grall]

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,

-- 
Julien Grall

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

[Jan Beulich]

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

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

[George Dunlap]

[-- Attachment #1.1: Type: text/plain, Size: 3447 bytes --]



> On 30 May 2022, at 10:55, Jan Beulich <jbeulich@suse.com <mailto:jbeulich@suse.com>> 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

[-- Attachment #1.2: Type: text/html, Size: 7603 bytes --]

[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Bertrand Marquis]

Hi,

> On 30 May 2022, at 11:21, George Dunlap <george.dunlap@citrix.com> wrote:
> 
> 
> 
>> On 30 May 2022, at 10:55, Jan Beulich <jbeulich@suse.com> 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

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

[Julien Grall]

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,

-- 
Julien Grall

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

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 1578 bytes --]

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.