Re: [XEN PATCH] automation/eclair: add deviations for MISRA C:2012 Rule 16.3

[Julien Grall <julien@xxxxxxx>]

Hi Stefano,

On 08/12/2023 00:30, Stefano Stabellini wrote:
> On Thu, 7 Dec 2023, Julien Grall wrote:
> > Hi Federico,
> >
> > On 07/12/2023 09:08, Federico Serafini wrote:
> > > MISRA C:2012 Rule 16.3 states that an unconditional break statement
> > > shall terminate every switch-clause.
> > >
> > > Update ECLAIR configuration to take into account:
> > > - continue, goto, return statements;
> > > - functions and macros that do not give the control back;
> > > - fallthrough comments and pseudo-keywords.
> > >
> > > Update docs/misra/deviations.rst accordingly.
> > >
> > > Signed-off-by: Federico Serafini <federico.serafini@xxxxxxxxxxx>
> > > ---
> > >    .../eclair_analysis/ECLAIR/deviations.ecl     | 18 ++++++++++++++
> > >    docs/misra/deviations.rst                     | 24 +++++++++++++++++++
> > >    2 files changed, 42 insertions(+)
> > It would be good that this is depending on to be accepted:
> >
> > https://lore.kernel.org/alpine.DEB.2.22.394.2312051859440.110490@ubuntu-linux-20-04-desktop.
> >
> > > diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl
> > > b/automation/eclair_analysis/ECLAIR/deviations.ecl
> > > index b0c79741b5..df0b58a010 100644
> > > --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
> > > +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
> > > @@ -321,6 +321,24 @@ statements are deliberate"
> > >    -config=MC3R1.R14.3,statements={deliberate ,
> > > "wrapped(any(),node(if_stmt))" }
> > >    -doc_end
> > >    +#
> > > +# Series 16.
> > > +#
> > > +
> > > +-doc_begin="Switch clauses ending with continue, goto, return statements
> > > are safe."
> > > +-config=MC3R1.R16.3,terminals+={safe,
> > > "node(continue_stmt||goto_stmt||return_stmt)"}
> > > +-doc_end
> > > +
> > > +-doc_begin="Switch clauses not ending with the break statement are safe if
> > > a function/macro that does not give the control back is present."
> > > +-config=MC3R1.R16.3,terminals+={safe,
> > > "call(decl(name(__builtin_unreachable||do_unexpected_trap||fatal_trap||machine_halt||machine_restart||maybe_reboot||panic)))"}
> > > +-config=MC3R1.R16.3,terminals+={safe,"macro(name(BUG||BUG_ON))"}
> > > +-doc_end
> > > +
> > > +-doc_begin="Switch clauses not ending with the break statement are safe if
> > > an explicit comment or pseudo-keyword indicating the fallthrough intention
> > > is present."
> > > +-config=MC3R1.R16.3,reports+={safe,
> > > "any_area(any_loc(any_exp(text(^(?s).*([fF]all[- ]?[tT]hrough|FALL[-
> > > ]?THROUGH).*$,0..1))))"}
> > > +-config=MC3R1.R16.3,reports+={safe, "any_area(text(^(?s).*([fF]all[-
> > > ]?[tT]hrough|FALL[- ]?THROUGH).*$,0..1))"}
> > This is not trivial to read. Can you document the exhaustive list of keywords
> > you are actually trying to deviate on? Also, did you consider to harmonize to
> > only a few?
> >
> > > +-doc_end
> > > +
> > >    #
> > >    # Series 20.
> > >    #
> > > diff --git a/docs/misra/deviations.rst b/docs/misra/deviations.rst
> > > index 6e7c4f25b8..fecd2bae8e 100644
> > > --- a/docs/misra/deviations.rst
> > > +++ b/docs/misra/deviations.rst
> > > @@ -270,6 +270,30 @@ Deviations related to MISRA C:2012 Rules:
> > >           statements are deliberate.
> > >         - Project-wide deviation; tagged as `disapplied` for ECLAIR.
> > >    +   * - R16.3
> > > +     - Switch clauses ending with continue, goto, return statements are
> > > safe.
> > > +     - Tagged as `safe` for ECLAIR.
> > > +
> > > +   * - R16.3
> > > +     - Switch clauses not ending with the break statement are safe if a
> > > +       function/macro that does not give the control back is present.
> > > +     - Tagged as `safe` for ECLAIR, such functions/macros are:
> > > +        - __builtin_unreachable
> > > +        - do_unexpected_trap
> > > +        - fatal_trap
> > > +        - machine_halt
> > > +        - machine_restart
> > > +        - maybe_reboot
> > > +        - panic
> > > +        - BUG
> > To me, it seems to be odd to deviate R16.3 by function. Some of those have an
> > attribute noreboot. Can this be used?
> Just to clarify, I think Julien meant "noreturn" which is defined as
> __attribute__((__noreturn__))
Whoops yes.

> I think we need to deviate by function, rather than using SAF, because
> we would have to use SAF in every switch rather than at the declaration
> of panic and friends. But if we could use noreturn, that would be
> awesome.
In general, I really dislike deviate by name because this is assuming 
that the functions name are uniq across all the architecture. For 
instance, fatal_trap() is x86 specific whilst do_unexpected_trap() is 
arm specific.
In this case, the name of the function clearly indicates that they are 
never expected to return. So we are ok.
But I would strongly prefer if we rely on property attribute rather than 
names whenever it is possible.
Cheers,

--
Julien Grall