Re: [XEN PATCH 4/6] x86emul: address violations of MISRA C Rule 20.7

[Nicola Vetrini <nicola.vetrini@bugseng.com>]

On 2024-06-12 11:19, Jan Beulich wrote:
> On 11.06.2024 17:53, 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@bugseng.com>
>> ---
>> These local helpers could in principle be deviated, but the 
>> readability
>> and functionality are essentially unchanged by complying with the 
>> rule,
>> so I decided to modify the macro definition as the preferred option.
> 
> Well, yes, but ...
> 
>> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
>> @@ -2255,7 +2255,7 @@ x86_emulate(
>>          switch ( modrm_reg & 7 )
>>          {
>>  #define GRP2(name, ext) \
>> -        case ext: \
>> +        case (ext): \
>>              if ( ops->rmw && dst.type == OP_MEM ) \
>>                  state->rmw = rmw_##name; \
>>              else \
>> @@ -8611,7 +8611,7 @@ int x86_emul_rmw(
>>              unsigned long dummy;
>> 
>>  #define XADD(sz, cst, mod) \
>> -        case sz: \
>> +        case (sz): \
>>              asm ( "" \
>>                    COND_LOCK(xadd) " %"#mod"[reg], %[mem]; " \
>>                    _POST_EFLAGS("[efl]", "[msk]", "[tmp]") \
> 
> ... this is really nitpicky of the rule / tool. What halfway realistic
> ways do you see to actually misuse these macros? What follows the 
> "case"
> keyword is just an expression; no precedence related issues are 
> possible.
> 

I do share the view: no real danger is possible in sensible uses. Often 
MISRA rules are stricter than necessary to have a simple formulation, by 
avoiding too many special cases.

However, if a deviation is formulated, then it needs to be maintained, 
for no real readability benefit in this case, in my opinion. I can be 
convinced otherwise, of course.

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)