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
