Re: [XEN PATCH for-4.19] xen/bitmap: amend MISRA C deviation for Rule 20.7

[Jan Beulich <jbeulich@xxxxxxxx>]

On 12.07.2024 23:26, Stefano Stabellini wrote:
> On Tue, 9 Jul 2024, Jan Beulich wrote:
>> On 09.07.2024 11:34, Nicola Vetrini wrote:
>>> --- a/xen/include/xen/bitmap.h
>>> +++ b/xen/include/xen/bitmap.h
>>> @@ -103,18 +103,16 @@ extern int bitmap_allocate_region(unsigned long 
>>> *bitmap, int pos, int order);
>>>  #define bitmap_switch(nbits, zero, small, large)                     \
>>>     unsigned int n__ = (nbits);                                       \
>>>     if (__builtin_constant_p(nbits) && !n__) {                        \
>>> -           /* SAF-7-safe Rule 20.7 non-parenthesized macro argument */ \
>>>             zero;                                                     \
>>>     } else if (__builtin_constant_p(nbits) && n__ <= BITS_PER_LONG) { \
>>> -           /* SAF-7-safe Rule 20.7 non-parenthesized macro argument */ \
>>>             small;                                                    \
>>>     } else {                                                          \
>>> -           /* SAF-7-safe Rule 20.7 non-parenthesized macro argument */ \
>>>             large;                                                    \
>>>     }
>>
>> An observation I made only while discussing this on the meeting is that by
>> going from this form to ...
>>
>>>  static inline void bitmap_zero(unsigned long *dst, unsigned int nbits)
>>>  {
>>> +   /* SAF-7-safe Rule 20.7 non-parenthesized macro argument */
>>>     bitmap_switch(nbits,,
>>>             *dst = 0UL,
>>>             memset(dst, 0, bitmap_bytes(nbits)));
>>
>> ... this form, you actually widen what the deviation covers to the entire
>> macro, which is too much. We don't want to deviate the rule for all of the
>> arguments, after all.
>>
>> However, it further occurred to me that the reason for needing the deviation
>> looks to merely be that in some cases (like the one above) we pass empty
>> macro arguments. That's getting in the way of parenthesizing the use sites.
>> We could avoid this, though, by adding e.g.
>>
>> #define nothing ((void)0)
>>
>> near the definition of bitmap_switch() and then using that in place of the
>> empty arguments. Provided of course this is the only obstacle to
>> parenthesization. At which point no deviation ought to be needed in the
>> first place.
> 
> 
> Roberto suggested in another email thread:
> 
>  
>> The problem comes from macro arguments that are expressions, in some cases,
>> and statements, in other cases, as it happens for bitmap_{switch,zero}.
>>
>> Possible solutions include:
>> - wrap the arguments that are statements in a do-while-false;
>> - add a ';' after the arguments that are statements.
>>
>> But what we recommend is to add a deviation for the cases where an argument,
>> after the expansion, is surrounded by the following tokens: '{' '}' ';'.
>> This will address all violations related to bitmap_{switch,zero} and requires
>> only a modification of the ECLAIR configuration which will look like this:
>>
>> -doc_begin="The expansion of an argument between tokens '{', '}' and ';' is 
>> safe."
>> -config=MC3R1.R20.7,expansion_context+={safe, 
>> "left_right(^[\\{;]$,^[;\\}]$)"}
>> -doc_end
>>
>> With this, all the remaining 71 violations in x86 code concerns msi.h, which 
>> we were
>> requested not to touch, and the 2 violations in arm code can be easily 
>> resolved
>> with a patch adding parentheses, for which a patch was already submitted by
>> Nicola and rejected by Jan.
> 
> 
> I think this is a good way forward because it is a simple deviation that
> makes sense to have, and makes sense as project wide deviation (it is
> not a deviation by name, e.g. deviating anything called
> "bitmap_switch").
> 
> I like Roberto's suggestion. Jan, are you OK with it?

See my reply there.

Jan