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

[Xenia Ragiadakou <burzalodowa@gmail.com>]

On 7/29/22 10:22, Jan Beulich wrote:
> 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.
>>

With the example that Jan provided I just realized, if function 
arguments are not parenthesized, somebody could alter the rest of the 
arguments with which a function is called via an intermediate macro ... 
a rather far fetched example but still ...

>>> 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

-- 
Xenia