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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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?