Re: [RFC PATCH 2/4] xen/arm64: bitops: justify uninitialized variable inside a macro

[Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>]

On 17/07/23 17:46, Jan Beulich wrote:
> On 17.07.2023 17:28, Nicola Vetrini wrote:
> > On 17/07/23 15:59, Jan Beulich wrote:
> > > On 14.07.2023 16:20, Luca Fancellu wrote:
> > > > > On 14 Jul 2023, at 12:49, Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx> wrote:
> > > > >
> > > > > The macro 'testop' expands to a function that declares the local
> > > > > variable 'oldbit', which is written before being set, but is such a
> > > > > way that is not amenable to automatic checking.
> > > > >
> > > > > Therefore, a deviation comment, is introduced to document this situation.
> > > > >
> > > > > A similar reasoning applies to macro 'guest_testop'.
> > > > >
> > > > > Signed-off-by: Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>
> > > > > ---
> > > > > docs/misra/safe.json                     | 16 ++++++++++++++++
> > > > > xen/arch/arm/arm64/lib/bitops.c          |  3 +++
> > > > > xen/arch/arm/include/asm/guest_atomics.h |  3 +++
> > > > > 3 files changed, 22 insertions(+)
> > > > >
> > > > > diff --git a/docs/misra/safe.json b/docs/misra/safe.json
> > > > > index 244001f5be..4cf7cbf57b 100644
> > > > > --- a/docs/misra/safe.json
> > > > > +++ b/docs/misra/safe.json
> > > > > @@ -20,6 +20,22 @@
> > > > >           },
> > > > >           {
> > > > >               "id": "SAF-2-safe",
> > > > > +            "analyser": {
> > > > > +                "eclair": "MC3R1.R9.1"
> > > > > +            },
> > > > > +            "name": "Rule 9.1: initializer not needed",
> > > > > +            "text": "The following local variables are possibly subject to being 
> > > > > read before being written, but code inspection ensured that the control flow in the construct where 
> > > > > they appear ensures that no such event may happen."
> > > > > +        },
> > > > > +        {
> > > > > +            "id": "SAF-3-safe",
> > > > > +            "analyser": {
> > > > > +                "eclair": "MC3R1.R9.1"
> > > > > +            },
> > > > > +            "name": "Rule 9.1: initializer not needed",
> > > > > +            "text": "The following local variables are possibly subject to being 
> > > > > read before being written, but code inspection ensured that the control flow in the construct where 
> > > > > they appear ensures that no such event may happen."
> > > > > +        },
> > > > Since the rule and the justification are the same, you can declare only once 
> > > > and use the same tag on top of the offending lines, so /* SAF-2-safe MC3R1.R9.1 
> > > > */,
> > > +1
> > >
> > > I'm puzzled by the wording vs comment placement though: The comments
> > > are inserted ahead of the macro invocations, so there are no "following
> > > local variables". Plus does this imply the comment would suppress the
> > > checking on _all_ of them, rather than just the one that was confirmed
> > > to be safe? What if another new one was added, that actually introduces
> > > a problem?
> > >
> > > > also, I remember some maintainers not happy about the misra rule being put after the 
> > > > tag, now I don’t recall who
> > > Me, at least. The annotations should be tool-agnostic imo, or else the
> > > more tools we use, the longer these comments might get.
> > No problem for both: Given the earlier remarks by Julien on patch 1/4, I
> > think we can devise something along the lines of
> >
> > /* SAF-x-safe MISRA:C 2012 Rule 9.1 <Justification> */
> > int var;
> >
> > and then write a generic justification in docs/misra/safe.json, while
> > <Justification> contains a specific one (e.g., this loop does so and so
> > to ensure that no access to unset variables happens).
> But that still mentions the rule. What if a new MISRA:C 2025 appears,
> with different rule numbers? I don't mind the comment mentioning, in
> a sufficiently terse way, what problem is circumvented. But I don't
> think tools or specifications should be referenced here. That's the
> purpose of the referenced "database" entry.
>
> Jan
Ok

--
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)