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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 09.07.2024 12:15, Nicola Vetrini wrote:
> On 2024-07-09 11:40, Jan Beulich wrote:
>> On 09.07.2024 11:34, Nicola Vetrini wrote:
>>> As noticed in the gitlab analyses, deviating bitmap_switch
>>> for Rule 20.7 in this way does not work for ECLAIR.
>>>
>>> Instead, the deviation should be put in the macro invocation.
>>
>> Why is this? I ask in particular because ...
>>
>>> --- 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;                                                    \
>>>     }
>>>
>>>  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)));
>>> @@ -139,6 +137,7 @@ static inline void bitmap_fill(unsigned long *dst, 
>>> unsigned int nbits)
>>>  static inline void bitmap_copy(unsigned long *dst, const unsigned 
>>> long *src,
>>>                     unsigned int nbits)
>>>  {
>>> +        /* SAF-7-safe Rule 20.7 non-parenthesized macro argument */
>>>     bitmap_switch(nbits,,
>>>             *dst = *src,
>>>             memcpy(dst, src, bitmap_bytes(nbits)));
>>> @@ -147,6 +146,7 @@ static inline void bitmap_copy(unsigned long *dst, 
>>> const unsigned long *src,
>>>  static inline void bitmap_and(unsigned long *dst, const unsigned long 
>>> *src1,
>>>                     const unsigned long *src2, unsigned int nbits)
>>>  {
>>> +   /* SAF-7-safe Rule 20.7 non-parenthesized macro argument */
>>>     bitmap_switch(nbits,,
>>>             *dst = *src1 & *src2,
>>>             __bitmap_and(dst, src1, src2, nbits));
>>> @@ -155,6 +155,7 @@ static inline void bitmap_and(unsigned long *dst, 
>>> const unsigned long *src1,
>>>  static inline void bitmap_or(unsigned long *dst, const unsigned long 
>>> *src1,
>>>                     const unsigned long *src2, unsigned int nbits)
>>>  {
>>> +   /* SAF-7-safe Rule 20.7 non-parenthesized macro argument */
>>>     bitmap_switch(nbits,,
>>>             *dst = *src1 | *src2,
>>>             __bitmap_or(dst, src1, src2, nbits));
>>> @@ -163,6 +164,7 @@ static inline void bitmap_or(unsigned long *dst, 
>>> const unsigned long *src1,
>>>  static inline void bitmap_xor(unsigned long *dst, const unsigned long 
>>> *src1,
>>>                     const unsigned long *src2, unsigned int nbits)
>>>  {
>>> +   /* SAF-7-safe Rule 20.7 non-parenthesized macro argument */
>>>     bitmap_switch(nbits,,
>>>             *dst = *src1 ^ *src2,
>>>             __bitmap_xor(dst, src1, src2, nbits));
>>> @@ -171,6 +173,7 @@ static inline void bitmap_xor(unsigned long *dst, 
>>> const unsigned long *src1,
>>>  static inline void bitmap_andnot(unsigned long *dst, const unsigned 
>>> long *src1,
>>>                     const unsigned long *src2, unsigned int nbits)
>>>  {
>>> +   /* SAF-7-safe Rule 20.7 non-parenthesized macro argument */
>>>     bitmap_switch(nbits,,
>>>             *dst = *src1 & ~*src2,
>>>             __bitmap_andnot(dst, src1, src2, nbits));
>>> @@ -179,6 +182,7 @@ static inline void bitmap_andnot(unsigned long 
>>> *dst, const unsigned long *src1,
>>>  static inline void bitmap_complement(unsigned long *dst, const 
>>> unsigned long *src,
>>>                     unsigned int nbits)
>>>  {
>>> +   /* SAF-7-safe Rule 20.7 non-parenthesized macro argument */
>>>     bitmap_switch(nbits,,
>>>             *dst = ~*src & BITMAP_LAST_WORD_MASK(nbits),
>>>             __bitmap_complement(dst, src, nbits));
>>> @@ -187,6 +191,7 @@ static inline void bitmap_complement(unsigned long 
>>> *dst, const unsigned long *sr
>>>  static inline int bitmap_equal(const unsigned long *src1,
>>>                     const unsigned long *src2, unsigned int nbits)
>>>  {
>>> +   /* SAF-7-safe Rule 20.7 non-parenthesized macro argument */
>>>     bitmap_switch(nbits,
>>>             return -1,
>>>             return !((*src1 ^ *src2) & BITMAP_LAST_WORD_MASK(nbits)),
>>> @@ -196,6 +201,7 @@ static inline int bitmap_equal(const unsigned long 
>>> *src1,
>>>  static inline int bitmap_intersects(const unsigned long *src1,
>>>                     const unsigned long *src2, unsigned int nbits)
>>>  {
>>> +   /* SAF-7-safe Rule 20.7 non-parenthesized macro argument */
>>>     bitmap_switch(nbits,
>>>             return -1,
>>>             return ((*src1 & *src2) & BITMAP_LAST_WORD_MASK(nbits)) != 0,
>>> @@ -205,6 +211,7 @@ static inline int bitmap_intersects(const unsigned 
>>> long *src1,
>>>  static inline int bitmap_subset(const unsigned long *src1,
>>>                     const unsigned long *src2, unsigned int nbits)
>>>  {
>>> +   /* SAF-7-safe Rule 20.7 non-parenthesized macro argument */
>>>     bitmap_switch(nbits,
>>>             return -1,
>>>             return !((*src1 & ~*src2) & BITMAP_LAST_WORD_MASK(nbits)),
>>> @@ -213,6 +220,7 @@ static inline int bitmap_subset(const unsigned 
>>> long *src1,
>>>
>>>  static inline int bitmap_empty(const unsigned long *src, unsigned int 
>>> nbits)
>>>  {
>>> +   /* SAF-7-safe Rule 20.7 non-parenthesized macro argument */
>>>     bitmap_switch(nbits,
>>>             return -1,
>>>             return !(*src & BITMAP_LAST_WORD_MASK(nbits)),
>>> @@ -221,6 +229,7 @@ static inline int bitmap_empty(const unsigned long 
>>> *src, unsigned int nbits)
>>>
>>>  static inline int bitmap_full(const unsigned long *src, unsigned int 
>>> nbits)
>>>  {
>>> +   /* SAF-7-safe Rule 20.7 non-parenthesized macro argument */
>>>     bitmap_switch(nbits,
>>>             return -1,
>>>             return !(~*src & BITMAP_LAST_WORD_MASK(nbits)),
>>
>> ... having the same comment on every invocation is naturally quite a 
>> bit
>> less desirable. So far I was under the impression that macro-specific
>> deviations can be dealt with by marking the macro definition 
>> accordingly.
> 
> Not with a comment-based one, but one based on ECL.
> As stated under the cut:
> An alternative approach would be to use an ecl configuration, but that
> would be tool-specific.

I read that, yes, but it's orthogonal to the point I made.

> Stefano had a preference for a tool-agnostic SAF comment, so that's what 
> I used.

I second this; I wonder though if e.g. for cppcheck the new placement
would have any effect.

>> I've been assuming this is a general pattern. If it isn't, would you
>> please first clarify what Eclair's specific requirements are for a SAF
>> marker to take effect when involving a macro?
> 
> it should be put directly above macro invocations.

That's then contrary to what was communicated before. Stefano, can we
please put this on the agenda of the call later in the day?

> ECLAIR has a 
> tool-specific comment-based deviation that essentially deviates a range 
> of lines, but that is not supported by the SAF framework, so I avoided 
> that.
> 
> Is it safe to say that the uses of bitmap_switch will likely not change 
> much over time?

Pretty much so, yes.

Jan