Re: [PATCH] docs/misra: add exceptions to rules

[Jan Beulich <jbeulich@xxxxxxxx>]

On 19.08.2023 03:24, Stefano Stabellini wrote:
> From: Stefano Stabellini <stefano.stabellini@xxxxxxx>
> 
> During the discussions that led to the acceptable of the Rules, we
> decided on a few exceptions that were not properly recorded in
> rules.rst. Other times, the exceptions were decided later when it came
> to enabling a rule in ECLAIR.

In a number of cases I'm unaware of such decisions. May be worth splitting
the patch into a controversial and an uncontroversial part, such that the
latter can go in while we discuss the former.

> --- a/docs/misra/rules.rst
> +++ b/docs/misra/rules.rst
> @@ -59,7 +59,8 @@ maintainers if you want to suggest a change.
>       - Required
>       - Precautions shall be taken in order to prevent the contents of a
>         header file being included more than once
> -     -
> +     - Files that are intended to be included more than once do not need to
> +       conform to the directive (e.g. autogenerated or empty header files)

Auto-generated isn't a reason for an exception here. The logic generating
the header can very well be adjusted. Same for empty headers - there's no
reason they couldn't gain guards. An exception is needed for headers which
we deliberately include more than once, in order to have a single central
place for attributes, enumerations, and alike.

> @@ -106,7 +107,23 @@ 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. while(0) { S; })

When (and why) was this decided? The example given looks exactly like what
Misra wants us to not have.

> +         - Switch with a controlling value incompatible with labeled
> +           statements

What does this mean?

> +         - Functions that are intended to be never referenced from C
> +           code, or are referenced in builds not under analysis (e.g.
> +           'do_trap_fiq' for the former and 'check_for_unexpected_msi'
> +           for the latter)

I agree with the "not referenced from C", but I don't see why the other
kind couldn't be properly addressed.

> +         - Unreachability caused by the following macros/functions is
> +           deliberate: BUG, assert_failed, ERROR_EXIT, ERROR_EXIT_DOM,
> +           PIN_FAIL, __builtin_unreachable, panic, do_unexpected_trap,
> +           machine_halt, machine_restart, machine_reboot,
> +           ASSERT_UNREACHABLE

Base infrastructure items like BUG() are imo fine to mention here. But
specific items shouldn't be; the more we mention here, the more we invite
the list to be grown. Note also how you mention items which no longer
exist (ERROR_EXIT{,_DOM}, PIN_FAIL).

> +         - asm-offsets.c, as they are not linked deliberately, because
> +           they are used to generate definitions for asm modules
> +         - pure declarations (i.e. declarations without
> +           initialization) are safe, as they are not executed

I don't think "pure declarations" is a term used in the spec. Let's better
call it the way it is called elsewhere - declarations without initializer
(where, as mentioned before, the term "unreachable code" is questionable
anyway).

> @@ -167,7 +184,7 @@ maintainers if you want to suggest a change.
>     * - `Rule 5.6 
> <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_05_06.c>`_
>       - Required
>       - A typedef name shall be a unique identifier
> -     -
> +     - BOOLEAN, UINT{8,32,64} and INT{8,32,64} are allowed

I think this permission needs to be limited as much as possible.

> @@ -183,7 +200,10 @@ maintainers if you want to suggest a change.
>     * - `Rule 7.1 
> <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_07_01.c>`_
>       - Required
>       - Octal constants shall not be used
> -     -
> +     - Usage of the following constants is safe, since they are given
> +       as-is in the inflate algorithm specification and there is
> +       therefore no risk of them being interpreted as decimal constants:
> +       ^0(007|37|070|213|236|300|321|330|331|332|333|334|335|337|371)$

This is a very odd set of exceptions, which by stating them here you then
grant to be exercised everywhere. Once again I don't think special cases
dealing with a single source or sub-component should be globally named.

> @@ -239,13 +259,16 @@ maintainers if you want to suggest a change.
>       - Required
>       - All declarations of an object or function shall use the same
>         names and type qualifiers
> -     -
> +     - The type ret_t is deliberately used and defined as int or long
> +       depending on the architecture

That's not depending on the architecture, but depending on the type of
guest to service. I'd also suggest "may", not "is".

Jan