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
