Re: [XEN PATCH][for-4.19 v2 2/2] docs/misra: add deviations.rst to document additional deviations.

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 10 Oct 2023, Nicola Vetrini wrote:
> On 10/10/2023 03:19, Stefano Stabellini wrote:
> > +Henry
> > 
> > On Mon, 9 Oct 2023, Nicola Vetrini wrote:
> > > This file contains the deviation that are not marked by
> > > a deviation comment, as specified in
> > > docs/misra/documenting-violations.rst.
> > > 
> > > Suggested-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> > > Signed-off-by: Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>
> > 
> > This is great! Thank you so much!
> > 
> > I have a few questions below but even as-is it is way better than
> > nothing. I think we should add this for 4.18
> > 
> > 
> > > ---
> > >  docs/index.rst            |   1 +
> > >  docs/misra/deviations.rst | 240 ++++++++++++++++++++++++++++++++++++++
> > >  docs/misra/rules.rst      |   2 +-
> > >  3 files changed, 242 insertions(+), 1 deletion(-)
> > >  create mode 100644 docs/misra/deviations.rst
> > > 
> > > diff --git a/docs/index.rst b/docs/index.rst
> > > index 2c47cfa999f2..f3f779f89ce5 100644
> > > --- a/docs/index.rst
> > > +++ b/docs/index.rst
> > > @@ -63,6 +63,7 @@ Xen hypervisor code.
> > >     :maxdepth: 2
> > > 
> > >     misra/rules
> > > +   misra/deviations
> > > 
> > > 
> > >  Miscellanea
> > > diff --git a/docs/misra/deviations.rst b/docs/misra/deviations.rst
> > > new file mode 100644
> > > index 000000000000..19743e34ce03
> > > --- /dev/null
> > > +++ b/docs/misra/deviations.rst
> > > @@ -0,0 +1,240 @@
> > > +.. SPDX-License-Identifier: CC-BY-4.0
> > > +
> > > +MISRA C deviations for Xen
> > > +==========================
> > > +
> > > +The following is the list of MISRA C:2012 deviations for the Xen codebase
> > > that
> > > +are not covered by a `SAF-x-safe` or `SAF-x-false-positive-<tool>`
> > > comment, as
> > > +specified in docs/misra/documenting-violations.rst; the lack of
> > > +such comments is usually due to the excessive clutter they would bring to
> > > the
> > > +codebase or the impossibility to express such a deviation (e.g., if it's
> > > +composed of several conditions).
> > > +
> > > +Deviations related to MISRA C:2012 Directives:
> > > +----------------------------------------------
> > > +
> > > +.. list-table::
> > > +   :header-rows: 1
> > > +
> > > +   * - Directive identifier
> > > +     - Justification
> > > +     - Notes
> > > +
> > > +   * - D4.3
> > > +     - Accepted for the ARM64 codebase
> > > +     - Tagged as `disapplied` for ECLAIR on any other violation report.
> > 
> > This mean it has been applied for ARM64 but not x86, right?
> > 
> > 
> 
> Yes.
> 
> > > +   * - D4.3
> > > +     - The inline asm in 'xen/arch/arm/arm64/lib/bitops.c' is tightly
> > > coupled
> > > +       with the surronding C code that acts as a wrapper, so it has been
> > > decided
> > > +       not to add an additional encapsulation layer.
> > > +     - Tagged as `deliberate` for ECLAIR.
> > > +
> > > +Deviations related to MISRA C:2012 Rules:
> > > +-----------------------------------------
> > > +
> > > +.. list-table::
> > > +   :header-rows: 1
> > > +
> > > +   * - Rule identifier
> > > +     - Justification
> > > +     - Notes
> > > +
> > > +   * - R2.1
> > > +     - The compiler implementation guarantees that the unreachable code
> > > is
> > > +       removed. Constant expressions and unreachable branches of if and
> > > switch
> > > +       statements are expected.
> > > +     - Tagged as `safe` for ECLAIR.
> > > +
> > > +   * - R2.1
> > > +     - Some functions are intended not to be referenced.
> > > +     - Tagged as `deliberate` for ECLAIR.
> > 
> > What does it mean "some functions" in this case? Should we list which
> > functions?
> > 
> 
> Well, there are a lot, typically resulting from build configurations that do
> not
> use them, or because they are used only in asm code. I can mention these
> reasons in the
> document, to make it easier to understand.

Yes, I think we need to clarify further this point, because saying "Some
functions" doesn't help the reader understand:
- whether all functions can be not referenced
- which subset of functions can be not referenced

How to distinguish between? How do we know whether a certain patch is
violating the rule or not?

If there is a clear list of functions that can be not referenced, then
we should list them here. If there is a methodology we can use to
distinguish between them (e.g. functions called from asm only) then we
can write the methodology here. Either way it is fine as long as the
criteria to know if it is OK if a function is not referenced is clear.