Re: [PATCH] xen/arm64: sysreg.h: Fix MISRA C 2012 Rule 20.7 violation

[Jan Beulich <jbeulich@xxxxxxxx>]

On 29.07.2022 09:01, Xenia Ragiadakou wrote:
> On 7/29/22 09:16, Jan Beulich wrote:
>> On 29.07.2022 07:23, Xenia Ragiadakou wrote:
>>> On 7/29/22 01:56, Stefano Stabellini wrote:
>>>> On Thu, 28 Jul 2022, Julien Grall wrote:
>>>>> On 28/07/2022 15:20, Jan Beulich wrote:
>>>>>> On 28.07.2022 15:56, Julien Grall wrote:
>>>>>>> On 28/07/2022 14:49, Xenia Ragiadakou wrote:
>>>>>>>> --- a/xen/arch/arm/include/asm/arm64/sysregs.h
>>>>>>>> +++ b/xen/arch/arm/include/asm/arm64/sysregs.h
>>>>>>>> @@ -461,7 +461,7 @@
>>>>>>>>      /* Access to system registers */
>>>>>>>>         #define WRITE_SYSREG64(v, name) do {                    \
>>>>>>>> -    uint64_t _r = v;                                    \
>>>>>>>> +    uint64_t _r = (v);                                              \
>>>>>>>
>>>>>>> I am failing to see why the parentheses are necessary here. Could you
>>>>>>> give an example where the lack of them would end up to different code?
>>>>>>
>>>>>> I think it is merely good practice to parenthesize the right sides of =.
>>>>>> Indeed with assignment operators having second to lowest precedence, and
>>>>>> with comma (the lowest precedence one) requiring parenthesization at the
>>>>>> macro invocation site, there should be no real need for parentheses here.
>>>>>
>>>>> I am not really happy with adding those parentheses because they are
>>>>> pointless. But if there are a consensus to use it, then the commit message
>>>>> should be updated to clarify this is just here to please MISRA (to me 
>>>>> "need"
>>>>> implies it would be bug).
>>>>
>>>> Let me premise that I don't know if this counts as a MISRA violation or
>>>> not. (Also I haven't checked if cppcheck/eclair report it as violation.)
>>>>
>>>> But I think the reason for making the change would be to follow our
>>>> coding style / coding practices. It makes the code simpler to figure out
>>>> that it is correct, to review and maintain if we always add the
>>>> parenthesis even in cases like this one where they are not strictly
>>>> necessary. We are going to save our future selves some mental cycles.
>>>>
>>>> So the explanation on the commit message could be along those lines.
>>>
>>> First, the rule 20.7 states "Expressions resulting from the expansion of
>>> macro parameters shall
>>>    be enclosed in parentheses". So, here it is a clear violation of the
>>> rule because the right side of the assignment operator is an expression.
>>>
>>> Second, as I stated in a previous email, if v is not enclosed in
>>> parentheses, I could write the story of my life in there and compile it
>>> :) So, it would be a bug.
>>>
>>> So, I recommend the title and the explanation i.e
>>> "xen/arm64: sysreg.h: Fix MISRA C 2012 Rule 20.7 violation
>>>
>>> The macro parameter 'v' is used as an expression and needs to be enclosed in
>>>    parentheses."
>>> to remain as is because they are accurate.
>>
>> I'm afraid you're following the MISRA wording too much to the latter.
>> Earlier on you agreed that when macro parameters are used as function
>> arguments, the parentheses can be omitted. Yet by what you say above
>> those are also expressions. 
> 
> Yes, those are also expressions (that's why I added parentheses 
> initially) and I agreed with you that the parentheses there may not be 
> necessary because I could not think of an example that will produce 
> different behaviors with and without the parentheses. This will need a 
> formal deviation I imagine or maybe a MISRA C expert could provide a 
> justification regarding why parentheses are needed around function 
> arguments that we may have not think of.
> 
>> As indicated before - I think parentheses
>> are wanted here, but it's strictly "wanted", and hence the title
>> better wouldn't say "fix" (but e.g. "improve") and the description
>> also should be "softened".
>>
> 
> Regarding the latter, are you saying that the parentheses are not needed?
> In my opinion they are needed to prevent the bug described in the 
> previous email i.e passing multiple statements to the macro.

Any such use would be rejected during review, I'm sure.

However I think there's another case which might indeed make this
more than just a "want" (and then responses further down are to be
viewed only in the context of earlier discussion):

#define wr(v) ({ \
        unsigned r_ = v; \
        asm("" :: "r" (r_)); \
})

#define M x, y

void test(unsigned x) {
        wr(M);
}

While this would result in an unused variable warning, it's surely
misleading (and less certain to be noticed during review) - which
is what Misra wants to avoid. Let's see what Julien thinks.

> By whom are they wanted? I 'm afraid I cannot understand.

By us as a community: This can be viewed as one of many agreements we
have on coding style. (As such it may want to be written down somewhere.)

> Also, are you proposing to change the title into "Improve MISRA C 2012 
> Rule 20.7 violation" ?

Obviously not. I was thinking of "improve to avoid ...".

Jan