| Message ID | 794724e70c4e45224916576af01070e8aebe9aab.1720517398.git.nicola.vetrini@bugseng.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [XEN,for-4.19] xen/bitmap: amend MISRA C deviation for Rule 20.7 | expand |
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. 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? Jan
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. Stefano had a preference for a tool-agnostic SAF comment, so that's what I used. > 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. 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? Thanks,
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
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. Jan
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?
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
diff --git a/xen/include/xen/bitmap.h b/xen/include/xen/bitmap.h index 6ee39aa35ac6..f8ef85541a84 100644 --- 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)),
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. No functional change. Fixes: 0dca0f2b9a7e ("automation/eclair: address violations of MISRA C Rule 20.7") Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> --- An alternative approach would be to use an ecl configuration, but that would be tool-specific. As this is purely an improvement to the CI, I'd like to ask for a release ack. --- xen/include/xen/bitmap.h | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-)