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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 02.08.2022 19:32, Julien Grall wrote:
> Hi Jan,
> 
> On 29/07/2022 08: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.
>>>
>>>> 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);
>> }
> 
> Interesting. I would have expected the pre-processor to first expand M 
> and then consider wr() is called with 2 parameters.
> 
>>
>> While this would result in an unused variable warning,
> 
> FWIW, in our case, the compiler is going to throw an error.
> 
>> it's surely
>> misleading (and less certain to be noticed during review) - which
> My expectation is we would notice that M is missing the parentheses. If 
> it is really wanted, the name of the macro should be obvious.
> 
>> is what Misra wants to avoid. Let's see what Julien thinks.
> I am struggling to see how this is different from:
> 
> #define wr(v) printf("%u\n", v)
> 
> If I am not mistaken, you have been arguing against adding the 
> parentheses here.

Yes - not the least because we actually use such in our code (at
the very least in hvmloader, see PRIllx_arg()).

> So, AFAIU, this means we will need to rely on the 
> compiler to notice the extra parameters.
> 
> Anyway, I am not against adding the parentheses in your example. 
> However, I think we should be consistent how we use them.

Indeed I, too, am all for consistency.

Jan