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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 22.08.2023 03:40, Stefano Stabellini wrote:
> On Mon, 21 Aug 2023, Jan Beulich wrote:
>> On 19.08.2023 03:24, Stefano Stabellini wrote:
>>> @@ -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.
> 
> This covers things like:
> 
> if (IS_ENABLED(CONFIG_HVM))
> 
> which could resolve in if (0) in certain configurations. I think we gave
> feedback to Roberto that we wanted to keep this type of things as is.

Ah, I see. But then perhaps mention that rather than plain 0 here? See
below as to whether a complete list of excepted constructs is wanted.

>>> +         - Switch with a controlling value incompatible with labeled
>>> +           statements
>>
>> What does this mean?
> 
> I am not certain about this one actually. It could be when we have:
> 
> switch (var) {
>   case 1:
>       something();
>       break;
>   case 2:
>       something();
>       break;
> }
> 
> and var could be 3 in theory?

That would be a unhandled value, but no unreachable code.

>>> +         - 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).
> 
> The question is whether we want this list to be exhaustive (so we want
> to mention everything for which we make an exception) or only an example
> (in which case just BUG is fine.)
> 
> Let's say we only mention BUG. Where should we keep the exhaustive list?
> Is it OK if it only lives as an ECLAIR config file under
> automation/eclair_analysis? There is another very similar question
> below.

First and foremost we need to have a single place where everything is
recorded, or where at least a pointer exists to where further details
are. As to this being the Eclair config file: Shouldn't any such
constructs rather be listed in the deviations file, such that e.g.
cppcheck can also benefit?

> BTW I think both options are OK.
> 
> If we only mention BUG, we are basically saying that as a general rule
> only BUG is an exception. Then we have a longer more detailed list for
> ECLAIR because in practice things are always complicated.
> 
> On the other hand if we have the full list here, then the documentation
> is more precise, but it looks a bit "strange" to see such a detailed
> list in this document and also we need to make sure to keep the list
> up-to-date.

Thing is: This list shouldn't grow very long anyway, and also better
would grow / change much over time.

>>> @@ -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.
> 
> Maybe we should take this out completely given that they should be
> limited to efi and acpi code that is excepted anyway

I would favor that, yes.

>>> @@ -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.
> 
> Actually I agree with you there. The problem is where to put them.

safe.json?

> Right now we have docs/misra/rules.rst with the list of accepted rules
> and their special interpretations by Xen Project. We also have the
> ECLAIR configuration under automation/eclair_analysis with a bunch of
> ECLAIR specific config files.
> 
> Is it OK if the constants above only live under
> automation/eclair_analysis and nowhere else? Or should we have another
> rst document under docs/misra for special cases dealing with a single
> source? 

As per above, I think putting anything in Eclair's config file should
only ever be a last resort. Wherever possible we should try to put stuff
in files which aren't tool specific. Where necessary special tool
settings can then be (machine-)derived from there.

Jan