diff mbox series

docs/misra: add rule 2.1 exceptions

Message ID 20230823002458.2738365-1-sstabellini@kernel.org (mailing list archive)
State Superseded
Headers show
Series docs/misra: add rule 2.1 exceptions | expand

Commit Message

Stefano Stabellini Aug. 23, 2023, 12:24 a.m. UTC
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(-)

Comments

Jan Beulich Aug. 23, 2023, 6:55 a.m. UTC | #1
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
Julien Grall Aug. 23, 2023, 9:01 a.m. UTC | #2
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,
Stefano Stabellini Aug. 23, 2023, 10:19 p.m. UTC | #3
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.
Stefano Stabellini Aug. 23, 2023, 10:19 p.m. UTC | #4
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.
diff mbox series

Patch

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