Re: [XEN PATCH 10/10] xen/keyhandler: address violations of MISRA C Rule 20.7

[Jan Beulich <jbeulich@xxxxxxxx>]

On 02.03.2024 02:37, Stefano Stabellini wrote:
> On Fri, 1 Mar 2024, Jan Beulich wrote:
>> On 29.02.2024 23:57, Stefano Stabellini wrote:
>>> On Thu, 29 Feb 2024, Nicola Vetrini wrote:
>>>> MISRA C Rule 20.7 states: "Expressions resulting from the expansion
>>>> of macro parameters shall be enclosed in parentheses". Therefore, some
>>>> macro definitions should gain additional parentheses to ensure that all
>>>> current and future users will be safe with respect to expansions that
>>>> can possibly alter the semantics of the passed-in macro parameter.
>>>>
>>>> No functional change.
>>>>
>>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>
>>>
>>> Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
>>
>> You did see the discussion on earlier patches, though? I don't think
>> any of the parentheses here are needed or wanted.
> 
> We need to align on this. Currently if we go by what's written in
> docs/misra/deviations.rst, then rhs should have parentheses.

Quoting the actual patch again:

--- a/xen/common/keyhandler.c
+++ b/xen/common/keyhandler.c
@@ -42,10 +42,10 @@ static struct keyhandler {
 } key_table[128] __read_mostly =
 {
 #define KEYHANDLER(k, f, desc, diag)            \
-    [k] = { { .fn = (f) }, desc, 0, diag }
+    [k] = { { .fn = (f) }, (desc), 0, (diag) }
 
 #define IRQ_KEYHANDLER(k, f, desc, diag)        \
-    [k] = { { .irq_fn = (f) }, desc, 1, diag }
+    [k] = { { .irq_fn = (f) }, (desc), 1, (diag) }

What rhs are you talking about in light of this change? The only rhs I
can spot here is already parenthesized.

> Can we safely claim that rhs parentheses are never needed? If so, then
> great, let's add it to deviations.rst and skip them here and other
> places in this patch series (e.g. patch #8). When I say "never" I am
> taking for granted that the caller is not doing something completely
> unacceptably broken such as: 
> 
>      WRITE_SYSREG64(var +, TTBR0_EL1)

I'm afraid I can't associate this with the patch here either. Instead in
the context here a (respective) construct as you mention above would simply
fail to build.

Jan

> If we cannot generically claim that rhs parentheses are never needed,
> then I don't think we should make any exceptions. We should add them here
> and everywhere else. It should be easy to write a macro or review a
> patch with a macro from someone else, and making special exception makes
> it more difficult for everyone.