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

[Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>]

On 14/07/23 16:32, Luca Fancellu wrote:
> > On 14 Jul 2023, at 15:20, Luca Fancellu <Luca.Fancellu@xxxxxxx> 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 
> > */,
> > also, I remember some maintainers not happy about the misra rule being put after the 
> > tag, now I don’t recall who
> Sorry, I see there was in a patch before a SAF-1-safe with the same 
> justification, so I suggest you use SAF-3-safe as tag and drop the new 
> justifications introduced here
I was a bit uncertain in choosing whether to use different IDs for 
bitops.c and guest_atomics.h, as they are very similar. Maybe a 
catch-all justification is too broad, but I have no strong opinion here.
As for the rule identifier after the tag, that's not a big problem (I 
added it mostly to emphasize why the deviation is there at a glance).
> > > +        {
> > > +            "id": "SAF-4-safe",
> > >             "analyser": {},
> > >             "name": "Sentinel",
> > >             "text": "Next ID to be used"
> > > diff --git a/xen/arch/arm/arm64/lib/bitops.c b/xen/arch/arm/arm64/lib/bitops.c
> > > index 20e3f3d6ce..e0728bb29d 100644
> > > --- a/xen/arch/arm/arm64/lib/bitops.c
> > > +++ b/xen/arch/arm/arm64/lib/bitops.c
> > > @@ -114,8 +114,11 @@ bitop(change_bit, eor)
> > > bitop(clear_bit, bic)
> > > bitop(set_bit, orr)
> > >
> > > +/* SAF-2-safe MC3R1.R9.1 */
> > > testop(test_and_change_bit, eor)
> > > +/* SAF-2-safe MC3R1.R9.1 */
> > > testop(test_and_clear_bit, bic)
> > > +/* SAF-2-safe MC3R1.R9.1 */
> > > testop(test_and_set_bit, orr)
> > >
> > > static always_inline bool int_clear_mask16(uint16_t mask, volatile uint16_t *p,
> > > diff --git a/xen/arch/arm/include/asm/guest_atomics.h 
> > > b/xen/arch/arm/include/asm/guest_atomics.h
> > > index a1745f8613..9d8f8ec3a3 100644
> > > --- a/xen/arch/arm/include/asm/guest_atomics.h
> > > +++ b/xen/arch/arm/include/asm/guest_atomics.h
> > > @@ -67,8 +67,11 @@ guest_bitop(change_bit)
> > > /* test_bit does not use load-store atomic operations */
> > > #define guest_test_bit(d, nr, p) ((void)(d), test_bit(nr, p))
> > >
> > > +/* SAF-3-safe MC3R1.R9.1 */
> > > guest_testop(test_and_set_bit)
> > > +/* SAF-3-safe MC3R1.R9.1 */
> > > guest_testop(test_and_clear_bit)
> > > +/* SAF-3-safe MC3R1.R9.1 */
> > > guest_testop(test_and_change_bit)
> > >
> > > #undef guest_testop
> > > --
> > > 2.34.1
--
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)