1. Patchew
  2. Xen
  3. View series

[PATCH] docs/misra: add rule 2.1 exceptions

Stefano Stabellini posted 1 patch 8 months, 1 week ago
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/20230823002458.2738365-1-sstabellini@kernel.org
There is a newer version of this series
docs/misra/rules.rst | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
[PATCH] docs/misra: add rule 2.1 exceptions
Posted by Stefano Stabellini 8 months, 1 week ago 
From: Stefano Stabellini <stefano.stabellini@amd.com>

During the discussions that led to the acceptable of Rule 2.1, we
decided on a few exceptions that were not properly recorded in
rules.rst. Add them now.

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

diff --git a/docs/misra/rules.rst b/docs/misra/rules.rst
index b6d87e076b..8f38227994 100644
--- a/docs/misra/rules.rst
+++ b/docs/misra/rules.rst
@@ -107,7 +107,18 @@ maintainers if you want to suggest a change.
    * - `Rule 2.1 <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_02_01_1.c>`_
      - Required
      - A project shall not contain unreachable code
-     -
+     - The following are allowed:
+         - Invariantly constant conditions, e.g. if(IS_ENABLED(CONFIG_HVM)) { S; }
+         - Switch with a controlling value statically determined not to
+           match one or more case statements
+         - Functions that are intended to be never referenced from C
+           code (e.g. 'do_trap_fiq')
+         - Unreachability caused by the certain macros/functions is
+           deliberate, e.g. BUG, assert_failed, panic, etc.
+         - asm-offsets.c, as they are not linked deliberately, because
+           they are used to generate definitions for asm modules
+         - declarations without initializer are safe, as they are not
+           executed
 
    * - `Rule 2.6 <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_02_06.c>`_
      - Advisory
-- 
2.25.1
Re: [PATCH] docs/misra: add rule 2.1 exceptions
Posted by Julien Grall 8 months, 1 week ago 
Hi Stefano,

On 23/08/2023 01:24, Stefano Stabellini wrote:
> From: Stefano Stabellini <stefano.stabellini@amd.com>
> 
> During the discussions that led to the acceptable of Rule 2.1, we
> decided on a few exceptions that were not properly recorded in
> rules.rst. Add them now.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
> ---
>   docs/misra/rules.rst | 13 ++++++++++++-
>   1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/docs/misra/rules.rst b/docs/misra/rules.rst
> index b6d87e076b..8f38227994 100644
> --- a/docs/misra/rules.rst
> +++ b/docs/misra/rules.rst
> @@ -107,7 +107,18 @@ maintainers if you want to suggest a change.
>      * - `Rule 2.1 <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_02_01_1.c>`_
>        - Required
>        - A project shall not contain unreachable code
> -     -
> +     - The following are allowed:
> +         - Invariantly constant conditions, e.g. if(IS_ENABLED(CONFIG_HVM)) { S; }
> +         - Switch with a controlling value statically determined not to
> +           match one or more case statements
> +         - Functions that are intended to be never referenced from C
> +           code (e.g. 'do_trap_fiq')
> +         - Unreachability caused by the certain macros/functions is
> +           deliberate, e.g. BUG, assert_failed, panic, etc.

I find the wording quite ambiguous. How will an assessor be able to know 
this is deliberate? I think it would be better if this is based on a 
keyword in the code such as a function that has the attribute noreturn 
and/or there is an unreachable() statement.

Cheers,

-- 
Julien Grall
Re: [PATCH] docs/misra: add rule 2.1 exceptions
Posted by Stefano Stabellini 8 months, 1 week ago 
On Wed, 23 Aug 2023, Julien Grall wrote:
> Hi Stefano,
> 
> On 23/08/2023 01:24, Stefano Stabellini wrote:
> > From: Stefano Stabellini <stefano.stabellini@amd.com>
> > 
> > During the discussions that led to the acceptable of Rule 2.1, we
> > decided on a few exceptions that were not properly recorded in
> > rules.rst. Add them now.
> > 
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
> > ---
> >   docs/misra/rules.rst | 13 ++++++++++++-
> >   1 file changed, 12 insertions(+), 1 deletion(-)
> > 
> > diff --git a/docs/misra/rules.rst b/docs/misra/rules.rst
> > index b6d87e076b..8f38227994 100644
> > --- a/docs/misra/rules.rst
> > +++ b/docs/misra/rules.rst
> > @@ -107,7 +107,18 @@ maintainers if you want to suggest a change.
> >      * - `Rule 2.1
> > <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_02_01_1.c>`_
> >        - Required
> >        - A project shall not contain unreachable code
> > -     -
> > +     - The following are allowed:
> > +         - Invariantly constant conditions, e.g. if(IS_ENABLED(CONFIG_HVM))
> > { S; }
> > +         - Switch with a controlling value statically determined not to
> > +           match one or more case statements
> > +         - Functions that are intended to be never referenced from C
> > +           code (e.g. 'do_trap_fiq')
> > +         - Unreachability caused by the certain macros/functions is
> > +           deliberate, e.g. BUG, assert_failed, panic, etc.
> 
> I find the wording quite ambiguous. How will an assessor be able to know this
> is deliberate? I think it would be better if this is based on a keyword in the
> code such as a function that has the attribute noreturn and/or there is an
> unreachable() statement.

You are right that it is not precise enough. I am thinking of changing
this to (also following Jan's suggestion):

Deliberate unreachability caused by certain macros/functions, e.g. BUG,
assert_failed, panic, etc. See safe.json.

In particular the important part is "See safe.json". Right now there is
nothing in safe.json about it, but it is should be the right place to
maintain the list of deviations and tags. Then we can add the tag at the
declaration site of BUG, panic, etc. Nicola is also checking if this
approach works with ECLAIR.
Re: [PATCH] docs/misra: add rule 2.1 exceptions
Posted by Jan Beulich 8 months, 1 week ago 
On 23.08.2023 02:24, Stefano Stabellini wrote:
> From: Stefano Stabellini <stefano.stabellini@amd.com>
> 
> During the discussions that led to the acceptable of Rule 2.1, we

Nit (as before): "acceptance"?

> decided on a few exceptions that were not properly recorded in
> rules.rst. Add them now.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
> ---
>  docs/misra/rules.rst | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/docs/misra/rules.rst b/docs/misra/rules.rst
> index b6d87e076b..8f38227994 100644
> --- a/docs/misra/rules.rst
> +++ b/docs/misra/rules.rst
> @@ -107,7 +107,18 @@ maintainers if you want to suggest a change.
>     * - `Rule 2.1 <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_02_01_1.c>`_
>       - Required
>       - A project shall not contain unreachable code
> -     -
> +     - The following are allowed:
> +         - Invariantly constant conditions, e.g. if(IS_ENABLED(CONFIG_HVM)) { S; }
> +         - Switch with a controlling value statically determined not to
> +           match one or more case statements
> +         - Functions that are intended to be never referenced from C
> +           code (e.g. 'do_trap_fiq')

Maybe better put it the other way around, "only referenced from assembly
code"? This is because "never referenced from C" also includes truly
unreferenced functions/data.

> +         - Unreachability caused by the certain macros/functions is
> +           deliberate, e.g. BUG, assert_failed, panic, etc.

I think the "the" here wants dropping, and even then the result doesn't
read very well imo. How about "Deliberate unreachability caused by
certain macros/functions, e.g. BUG, assert_failed, panic, etc"?

> +         - asm-offsets.c, as they are not linked deliberately, because
> +           they are used to generate definitions for asm modules
> +         - declarations without initializer are safe, as they are not
> +           executed

Provided additionally this sub-sub-bullet-list then also translates
correctly to the various derived formats, then:
Acked-by: Jan Beulich <jbeulich@suse.com>

Jan
Re: [PATCH] docs/misra: add rule 2.1 exceptions
Posted by Stefano Stabellini 8 months, 1 week ago 
On Wed, 23 Aug 2023, Jan Beulich wrote:
> On 23.08.2023 02:24, Stefano Stabellini wrote:
> > From: Stefano Stabellini <stefano.stabellini@amd.com>
> > 
> > During the discussions that led to the acceptable of Rule 2.1, we
> 
> Nit (as before): "acceptance"?
> 
> > decided on a few exceptions that were not properly recorded in
> > rules.rst. Add them now.
> > 
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
> > ---
> >  docs/misra/rules.rst | 13 ++++++++++++-
> >  1 file changed, 12 insertions(+), 1 deletion(-)
> > 
> > diff --git a/docs/misra/rules.rst b/docs/misra/rules.rst
> > index b6d87e076b..8f38227994 100644
> > --- a/docs/misra/rules.rst
> > +++ b/docs/misra/rules.rst
> > @@ -107,7 +107,18 @@ maintainers if you want to suggest a change.
> >     * - `Rule 2.1 <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_02_01_1.c>`_
> >       - Required
> >       - A project shall not contain unreachable code
> > -     -
> > +     - The following are allowed:
> > +         - Invariantly constant conditions, e.g. if(IS_ENABLED(CONFIG_HVM)) { S; }
> > +         - Switch with a controlling value statically determined not to
> > +           match one or more case statements
> > +         - Functions that are intended to be never referenced from C
> > +           code (e.g. 'do_trap_fiq')
> 
> Maybe better put it the other way around, "only referenced from assembly
> code"? This is because "never referenced from C" also includes truly
> unreferenced functions/data.
> 
> > +         - Unreachability caused by the certain macros/functions is
> > +           deliberate, e.g. BUG, assert_failed, panic, etc.
> 
> I think the "the" here wants dropping, and even then the result doesn't
> read very well imo. How about "Deliberate unreachability caused by
> certain macros/functions, e.g. BUG, assert_failed, panic, etc"?
> 
> > +         - asm-offsets.c, as they are not linked deliberately, because
> > +           they are used to generate definitions for asm modules
> > +         - declarations without initializer are safe, as they are not
> > +           executed
> 
> Provided additionally this sub-sub-bullet-list then also translates
> correctly to the various derived formats, then:
> Acked-by: Jan Beulich <jbeulich@suse.com>

Yes I checked. Also thanks for the good suggestions, I'll make the
changes and resend.

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.