Re: [XEN PATCH] xen: Add SAF deviations for MISRA C:2012 Rule 7.1

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 3 Oct 2023, Andrew Cooper wrote:
> On 03/10/2023 6:14 pm, Luca Fancellu wrote:
> >
> >> On 3 Oct 2023, at 17:17, andrew.cooper3@xxxxxxxxxx wrote:
> >>
> >> On 03/10/2023 4:37 pm, Nicola Vetrini wrote:
> >>> As specified in rules.rst, these constants can be used
> >>> in the code.
> >>> Their deviation is now accomplished by using a SAF comment,
> >>> rather than an ECLAIR configuration.
> >>>
> >>> Signed-off-by: Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>
> >>> ---
> >>> automation/eclair_analysis/ECLAIR/deviations.ecl | 6 ------
> >>> docs/misra/safe.json                             | 8 ++++++++
> >>> xen/arch/x86/hvm/svm/emulate.c                   | 6 +++---
> >>> xen/arch/x86/hvm/svm/svm.h                       | 9 +++++++++
> >>> xen/common/inflate.c                             | 4 ++--
> >>> 5 files changed, 22 insertions(+), 11 deletions(-)
> >>>
> >>> diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl 
> >>> b/automation/eclair_analysis/ECLAIR/deviations.ecl
> >>> index d8170106b449..fbb806a75d73 100644
> >>> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
> >>> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
> >>> @@ -132,12 +132,6 @@ safe."
> >>> # Series 7.
> >>> #
> >>>
> >>> --doc_begin="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."
> >>> --config=MC3R1.R7.1,literals={safe, 
> >>> "^0(007|37|070|213|236|300|321|330|331|332|333|334|335|337|371)$"}
> >>> --doc_end
> >>> -
> >>> -doc_begin="Violations in files that maintainers have asked to not modify 
> >>> in the
> >>> context of R7.2."
> >>> -file_tag+={adopted_r7_2,"^xen/include/xen/libfdt/.*$"}
> >>> diff --git a/docs/misra/safe.json b/docs/misra/safe.json
> >>> index 39c5c056c7d4..7ea47344ffcc 100644
> >>> --- a/docs/misra/safe.json
> >>> +++ b/docs/misra/safe.json
> >>> @@ -20,6 +20,14 @@
> >>>         },
> >>>         {
> >>>             "id": "SAF-2-safe",
> >>> +            "analyser": {
> >>> +                "eclair": "MC3R1.R7.1"
> >>> +            },
> >>> +            "name": "Rule 7.1: constants defined in specifications, 
> >>> manuals, and algorithm descriptions",
> >>> +            "text": "It is safe to use certain octal constants the way 
> >>> they are defined in specifications, manuals, and algorithm descriptions."
> >>> +        },
> >>> +        {
> >>> +            "id": "SAF-3-safe",
> >>>             "analyser": {},
> >>>             "name": "Sentinel",
> >>>             "text": "Next ID to be used"
> >>> diff --git a/xen/arch/x86/hvm/svm/emulate.c 
> >>> b/xen/arch/x86/hvm/svm/emulate.c
> >>> index aa2c61c433b3..c5e3341c6316 100644
> >>> --- a/xen/arch/x86/hvm/svm/emulate.c
> >>> +++ b/xen/arch/x86/hvm/svm/emulate.c
> >>> @@ -90,9 +90,9 @@ unsigned int svm_get_insn_len(struct vcpu *v, unsigned 
> >>> int instr_enc)
> >>>         if ( !instr_modrm )
> >>>             return emul_len;
> >>>
> >>> -        if ( modrm_mod       == MASK_EXTR(instr_modrm, 0300) &&
> >>> -             (modrm_reg & 7) == MASK_EXTR(instr_modrm, 0070) &&
> >>> -             (modrm_rm  & 7) == MASK_EXTR(instr_modrm, 0007) )
> >>> +        if ( modrm_mod       == MASK_EXTR(instr_modrm, 0300) && /* 
> >>> SAF-2-safe */
> >>> +             (modrm_reg & 7) == MASK_EXTR(instr_modrm, 0070) && /* 
> >>> SAF-2-safe */
> >>> +             (modrm_rm  & 7) == MASK_EXTR(instr_modrm, 0007) )  /* 
> >>> SAF-2-safe */
> >>>             return emul_len;
> >>>     }
> > Hi Andrew,
> >
> >> This is line noise, and later examples are even worse.
> >>
> >> What does SAF mean?  It's presumably not the Scalable Agile Framework.
> > Please have a look on docs/misra/documenting-violations.rst, you will find 
> > all the
> > info about it.
> 
> Thankyou for proving my point perfectly.
> 
> The comment in the source code needs to be *far* clearer than it
> currently is.
> 
> Even s/SAF/ANALYSIS/ would be an improvement, because it makes the
> comment very clear that it's about code analysis.  An unknown initialism
> like SAF does not convey enough meaning to be useful.

Hi Andrew,

I am OK with a rename of the "SAF" tag.

The number of instances is still small enough that a rename can be done
at this point in time. Given that the SAF framework was reviewed by
multiple people, and we already have a few SAF tags in the code base and
even more in my for-4.19 branch, I suggest to start a separate thread on
the topic.

A new thread with a clear subject like "rename SAF to BLAH" and CCing
all the maintainers in the MISRA C working group to make sure everyone
is aware.

Ideally the email should also have a couple of good suggestions for new
tags. I don't have a strong opinion on the name. ANALYSIS is not great
because the tag is meant to say that the line below is safe even if some
MISRA C scanners might find an issue with it. SAF is meant to remind us
of "SAFE". So I would prefer to add the letter "E" and call it "SAFE".

If we can come up with 3-5 options then we can have a doodle poll or
something.

Cheers,

Stefano