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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

Jan