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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 22 Aug 2023, Jan Beulich wrote:
> 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?

Yes, I'll do


> See below as to whether a complete list of excepted constructs is
> wanted.

[...]

> >>> +         - 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?

Yes, you are right. Basically the choice is whether we want a
project-wide deviation, something like a change in how we interpret the
rules, or a regular deviation for one macro or one function.

After reading your comments, I also think that all the macros above
should be covered by safe.json and in-code comments instead.



> > 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.

+1


> >>> @@ -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?

Yes you are right

 
> > 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.

I agree. I'll send out another patch to update rules.rst. And later as a
follow-up I'll see what I can do to update safe.json.