On 20.10.2023 17:28, Nicola Vetrini wrote: > --- a/automation/eclair_analysis/ECLAIR/deviations.ecl > +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl > @@ -246,6 +246,12 @@ constant expressions are required.\"" > "any()"} > -doc_end > > +-doc_begin="The macro LOWEST_BIT encapsulates a well-known pattern to obtain > the value > +2^ffs(x) for unsigned integers on two's complement architectures > +(all the architectures supported by Xen satisfy this requirement)." > +-config=MC3R1.R10.1,reports+={safe, > "any_area(any_loc(any_exp(macro(^LOWEST_BIT$))))"} > +-doc_end
This being deviated this way (rather than by SAF-* comments) wants justifying in the description. You did reply to my respective comment on v2, but such then (imo) needs propagating into the actual patch as well. > --- a/docs/misra/deviations.rst > +++ b/docs/misra/deviations.rst > @@ -192,6 +192,13 @@ Deviations related to MISRA C:2012 Rules: > See automation/eclair_analysis/deviations.ecl for the full > explanation. > - Tagged as `safe` for ECLAIR. > > + * - R10.1 > + - The well-known pattern (x & -x) applied to unsigned integer values on > 2's > + complement architectures (i.e., all architectures supported by Xen), > used > + to obtain the value 2^ffs(x), where ffs(x) is the position of the > first > + bit set. If no bits are set, zero is returned. > + - Tagged as `safe` for ECLAIR. In such an explanation there shall not be any ambiguity. Here I see an issue with ffs() returning 1-based bit position numbers, which isn't in line with the use in 2^ffs(x). (Arguably use of ^ itself is also problematic, as that's XOR in C, not POW. I'd suggest 2^^ffs(x) or 2**ffs(x).) > --- a/xen/include/xen/macros.h > +++ b/xen/include/xen/macros.h > @@ -8,8 +8,11 @@ > #define DIV_ROUND(n, d) (((n) + (d) / 2) / (d)) > #define DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d)) > > -#define MASK_EXTR(v, m) (((v) & (m)) / ((m) & -(m))) > -#define MASK_INSR(v, m) (((v) * ((m) & -(m))) & (m)) > +/* Returns the 2^ffs(x) or 0, where ffs(x) is the index of the lowest set > bit */ > +#define LOWEST_BIT(x) ((x) & -(x)) I'm afraid my concern regarding this new macro's name (voiced on v2) hasn't been addressed (neither verbally nor by finding a more suitable name). Jan