| Message ID | 546cf30aa43d6d0687a9a6c6d23b11128e5783e8.1697815135.git.nicola.vetrini@bugseng.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | address violations of MISRA C:2012 Rule 10.1 | expand |
Hi Nicola, On 20/10/2023 16:28, Nicola Vetrini wrote: > diff --git a/xen/include/xen/macros.h b/xen/include/xen/macros.h > index d0caae7db298..49f3ebf848e9 100644 > --- 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)) > + > +#define MASK_EXTR(v, m) (((v) & (m)) / LOWEST_BIT(m)) > +#define MASK_INSR(v, m) (((v) * LOWEST_BIT(m)) & (m)) > > #define count_args_(dot, a1, a2, a3, a4, a5, a6, a7, a8, x, ...) x > #define count_args(args...) \ > diff --git a/xen/include/xen/types.h b/xen/include/xen/types.h > index aea259db1ef2..23cad71c8a47 100644 > --- a/xen/include/xen/types.h > +++ b/xen/include/xen/types.h I don't understand how the changes in this file are related to the commit. Did you intend to create a separate commit? The rest LGTM. > @@ -31,9 +31,9 @@ typedef __PTRDIFF_TYPE__ ptrdiff_t; > #define NULL ((void*)0) > #endif > > -#define INT8_MIN (-127-1) > -#define INT16_MIN (-32767-1) > -#define INT32_MIN (-2147483647-1) > +#define INT8_MIN (-127 - 1) > +#define INT16_MIN (-32767 - 1) > +#define INT32_MIN (-2147483647 - 1) > > #define INT8_MAX (127) > #define INT16_MAX (32767) > @@ -43,10 +43,10 @@ typedef __PTRDIFF_TYPE__ ptrdiff_t; > #define UINT16_MAX (65535) > #define UINT32_MAX (4294967295U) > > -#define INT_MAX ((int)(~0U>>1)) > +#define INT_MAX ((int)(~0U >> 1)) > #define INT_MIN (-INT_MAX - 1) > #define UINT_MAX (~0U) > -#define LONG_MAX ((long)(~0UL>>1)) > +#define LONG_MAX ((long)(~0UL >> 1)) > #define LONG_MIN (-LONG_MAX - 1) > #define ULONG_MAX (~0UL) > Cheers,
On 20/10/2023 19:03, Julien Grall wrote: > Hi Nicola, > > On 20/10/2023 16:28, Nicola Vetrini wrote: >> diff --git a/xen/include/xen/macros.h b/xen/include/xen/macros.h >> index d0caae7db298..49f3ebf848e9 100644 >> --- 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)) >> + >> +#define MASK_EXTR(v, m) (((v) & (m)) / LOWEST_BIT(m)) >> +#define MASK_INSR(v, m) (((v) * LOWEST_BIT(m)) & (m)) >> #define count_args_(dot, a1, a2, a3, a4, a5, a6, a7, a8, x, ...) x >> #define count_args(args...) \ >> diff --git a/xen/include/xen/types.h b/xen/include/xen/types.h >> index aea259db1ef2..23cad71c8a47 100644 >> --- a/xen/include/xen/types.h >> +++ b/xen/include/xen/types.h > > I don't understand how the changes in this file are related to the > commit. Did you intend to create a separate commit? > > The rest LGTM. > Oh, I made a mistake. This hunk should have been part of patch 7 as a cleanup. I can resubmit with this removed, or it could be ignored altogether. >> @@ -31,9 +31,9 @@ typedef __PTRDIFF_TYPE__ ptrdiff_t; >> #define NULL ((void*)0) >> #endif >> -#define INT8_MIN (-127-1) >> -#define INT16_MIN (-32767-1) >> -#define INT32_MIN (-2147483647-1) >> +#define INT8_MIN (-127 - 1) >> +#define INT16_MIN (-32767 - 1) >> +#define INT32_MIN (-2147483647 - 1) >> #define INT8_MAX (127) >> #define INT16_MAX (32767) >> @@ -43,10 +43,10 @@ typedef __PTRDIFF_TYPE__ ptrdiff_t; >> #define UINT16_MAX (65535) >> #define UINT32_MAX (4294967295U) >> -#define INT_MAX ((int)(~0U>>1)) >> +#define INT_MAX ((int)(~0U >> 1)) >> #define INT_MIN (-INT_MAX - 1) >> #define UINT_MAX (~0U) >> -#define LONG_MAX ((long)(~0UL>>1)) >> +#define LONG_MAX ((long)(~0UL >> 1)) >> #define LONG_MIN (-LONG_MAX - 1) >> #define ULONG_MAX (~0UL) >> > > Cheers,
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
On 23/10/2023 09:48, Jan Beulich wrote: > 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. > Sure, will do. >> --- 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).) > Makes sense, I think I'll use an plain english explanation to avoid any confusion about notation. >> --- 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 I didn't come up with much better names, so I left it as is. Here's a few ideas: - LOWEST_SET - MASK_LOWEST_SET - MASK_LOWEST_BIT - MASK_FFS_0 - LOWEST_ONE and also yours, included here for convenience, even though you felt it was too long: - ISOLATE_LOW_BIT() All maintainers from THE REST are CC-ed, so we can see if anyone has any suggestion.
On 23/10/2023 11:19, Nicola Vetrini wrote: > On 23/10/2023 09:48, Jan Beulich wrote: >> 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. >> > > Sure, will do. > >>> --- 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).) >> > > Makes sense, I think I'll use an plain english explanation to avoid > any confusion > about notation. > >>> --- 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 > > I didn't come up with much better names, so I left it as is. Here's a > few ideas: > > - LOWEST_SET > - MASK_LOWEST_SET > - MASK_LOWEST_BIT > - MASK_FFS_0 > - LOWEST_ONE > > and also yours, included here for convenience, even though you felt it > was too long: > > - ISOLATE_LOW_BIT() > > All maintainers from THE REST are CC-ed, so we can see if anyone has > any suggestion. There's also LOWEST_BIT_MASK as another possible name.
On 23.10.2023 15:19, Nicola Vetrini wrote: > On 23/10/2023 11:19, Nicola Vetrini wrote: >> On 23/10/2023 09:48, Jan Beulich wrote: >>> 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. >>> >> >> Sure, will do. >> >>>> --- 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).) >>> >> >> Makes sense, I think I'll use an plain english explanation to avoid >> any confusion >> about notation. >> >>>> --- 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 >> >> I didn't come up with much better names, so I left it as is. Here's a >> few ideas: >> >> - LOWEST_SET >> - MASK_LOWEST_SET >> - MASK_LOWEST_BIT >> - MASK_FFS_0 >> - LOWEST_ONE >> >> and also yours, included here for convenience, even though you felt it >> was too long: >> >> - ISOLATE_LOW_BIT() >> >> All maintainers from THE REST are CC-ed, so we can see if anyone has >> any suggestion. > > There's also LOWEST_BIT_MASK as another possible name. While naming-wise okay to me, it has the same "too long" issue as ISOLATE_LOW_BIT(). Considering x86, in the BMI ISA extension, has an insn doing exactly that (BLSI), taking inspiration from its mnemonic may also be an option. Jan
On Mon, 23 Oct 2023, Jan Beulich wrote: > On 23.10.2023 15:19, Nicola Vetrini wrote: > > On 23/10/2023 11:19, Nicola Vetrini wrote: > >> On 23/10/2023 09:48, Jan Beulich wrote: > >>> 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. > >>> > >> > >> Sure, will do. > >> > >>>> --- 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).) > >>> > >> > >> Makes sense, I think I'll use an plain english explanation to avoid > >> any confusion > >> about notation. > >> > >>>> --- 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 > >> > >> I didn't come up with much better names, so I left it as is. Here's a > >> few ideas: > >> > >> - LOWEST_SET > >> - MASK_LOWEST_SET > >> - MASK_LOWEST_BIT > >> - MASK_FFS_0 > >> - LOWEST_ONE > >> > >> and also yours, included here for convenience, even though you felt it > >> was too long: > >> > >> - ISOLATE_LOW_BIT() > >> > >> All maintainers from THE REST are CC-ed, so we can see if anyone has > >> any suggestion. > > > > There's also LOWEST_BIT_MASK as another possible name. > > While naming-wise okay to me, it has the same "too long" issue as > ISOLATE_LOW_BIT(). Considering x86, in the BMI ISA extension, has an > insn doing exactly that (BLSI), taking inspiration from its mnemonic > may also be an option. I don't mean to make things difficult but I prefer LOWEST_BIT or MASK_LOWEST_BIT. It is clear at first glance. BLSI is not as clear, unless you work on the specific ISA with BLSI. In general, I do appreciate shorter names, but if one option is much clearer than the other, I prefer clarity over shortness. Maybe we need a third opinion here to make progress a bit faster. Julien?
On 23.10.2023 22:44, Stefano Stabellini wrote: > On Mon, 23 Oct 2023, Jan Beulich wrote: >> On 23.10.2023 15:19, Nicola Vetrini wrote: >>> On 23/10/2023 11:19, Nicola Vetrini wrote: >>>> On 23/10/2023 09:48, Jan Beulich wrote: >>>>> 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. >>>>> >>>> >>>> Sure, will do. >>>> >>>>>> --- 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).) >>>>> >>>> >>>> Makes sense, I think I'll use an plain english explanation to avoid >>>> any confusion >>>> about notation. >>>> >>>>>> --- 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 >>>> >>>> I didn't come up with much better names, so I left it as is. Here's a >>>> few ideas: >>>> >>>> - LOWEST_SET >>>> - MASK_LOWEST_SET >>>> - MASK_LOWEST_BIT >>>> - MASK_FFS_0 >>>> - LOWEST_ONE >>>> >>>> and also yours, included here for convenience, even though you felt it >>>> was too long: >>>> >>>> - ISOLATE_LOW_BIT() >>>> >>>> All maintainers from THE REST are CC-ed, so we can see if anyone has >>>> any suggestion. >>> >>> There's also LOWEST_BIT_MASK as another possible name. >> >> While naming-wise okay to me, it has the same "too long" issue as >> ISOLATE_LOW_BIT(). Considering x86, in the BMI ISA extension, has an >> insn doing exactly that (BLSI), taking inspiration from its mnemonic >> may also be an option. > > I don't mean to make things difficult but I prefer LOWEST_BIT or > MASK_LOWEST_BIT. It is clear at first glance. BLSI is not as clear, > unless you work on the specific ISA with BLSI. > > In general, I do appreciate shorter names, but if one option is much > clearer than the other, I prefer clarity over shortness. That's fine with me, but note that neither LOWEST_BIT nor MASK_LOWEST_BIT really provide the aimed at clarity. The shortest that I could think of that would be derived from that would be LOWEST_SET_BIT_MASK() (albeit even that leaves a bit of ambiguity, thinking about it for a little longer). The main point I'm trying to make that _if_ we need a wrapper macro for this in the first place (note the other thread about macros still requiring deviation comments at all use sites for Eclair), its name needs to somehow express the precise operation it does (or, like would be the case for BLSI, make people not recognizing the name go look rather than guess). Jan
On 24/10/2023 08:14, Jan Beulich wrote: > On 23.10.2023 22:44, Stefano Stabellini wrote: >> On Mon, 23 Oct 2023, Jan Beulich wrote: >>> On 23.10.2023 15:19, Nicola Vetrini wrote: >>>> On 23/10/2023 11:19, Nicola Vetrini wrote: >>>>> On 23/10/2023 09:48, Jan Beulich wrote: >>>>>> 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. >>>>>> >>>>> >>>>> Sure, will do. >>>>> >>>>>>> --- 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).) >>>>>> >>>>> >>>>> Makes sense, I think I'll use an plain english explanation to avoid >>>>> any confusion >>>>> about notation. >>>>> >>>>>>> --- 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 >>>>> >>>>> I didn't come up with much better names, so I left it as is. Here's >>>>> a >>>>> few ideas: >>>>> >>>>> - LOWEST_SET >>>>> - MASK_LOWEST_SET >>>>> - MASK_LOWEST_BIT >>>>> - MASK_FFS_0 >>>>> - LOWEST_ONE >>>>> >>>>> and also yours, included here for convenience, even though you felt >>>>> it >>>>> was too long: >>>>> >>>>> - ISOLATE_LOW_BIT() >>>>> >>>>> All maintainers from THE REST are CC-ed, so we can see if anyone >>>>> has >>>>> any suggestion. >>>> >>>> There's also LOWEST_BIT_MASK as another possible name. >>> >>> While naming-wise okay to me, it has the same "too long" issue as >>> ISOLATE_LOW_BIT(). Considering x86, in the BMI ISA extension, has an >>> insn doing exactly that (BLSI), taking inspiration from its mnemonic >>> may also be an option. >> >> I don't mean to make things difficult but I prefer LOWEST_BIT or >> MASK_LOWEST_BIT. It is clear at first glance. BLSI is not as clear, >> unless you work on the specific ISA with BLSI. >> >> In general, I do appreciate shorter names, but if one option is much >> clearer than the other, I prefer clarity over shortness. > > That's fine with me, but note that neither LOWEST_BIT nor > MASK_LOWEST_BIT > really provide the aimed at clarity. The shortest that I could think of > that would be derived from that would be LOWEST_SET_BIT_MASK() (albeit > even that leaves a bit of ambiguity, thinking about it for a little > longer). The main point I'm trying to make that _if_ we need a wrapper > macro for this in the first place (note the other thread about macros > still requiring deviation comments at all use sites for Eclair), its > name > needs to somehow express the precise operation it does (or, like would > be > the case for BLSI, make people not recognizing the name go look rather > than guess). > > Jan Ok, I'll send a revised version using MASK_LOWEST_BIT, taking into account also the other comments about the explanation on the macro definition (which some IDEs even show when hovering on its usage, which could partially address the latter concern).
On 25.10.2023 16:50, Nicola Vetrini wrote: > Ok, I'll send a revised version using MASK_LOWEST_BIT, taking into > account also the > other comments about the explanation on the macro definition > (which some IDEs even show when hovering on its usage, which could > partially address > the latter concern). You're of course free to do so, but since - as indicated before - MASK_LOWEST_BIT() imo isn't a better name than LOWEST_BIT(), I'll continue to object. Jan
On Wed, 25 Oct 2023, Jan Beulich wrote: > On 25.10.2023 16:50, Nicola Vetrini wrote: > > Ok, I'll send a revised version using MASK_LOWEST_BIT, taking into > > account also the > > other comments about the explanation on the macro definition > > (which some IDEs even show when hovering on its usage, which could > > partially address > > the latter concern). > > You're of course free to do so, but since - as indicated before - > MASK_LOWEST_BIT() imo isn't a better name than LOWEST_BIT(), I'll > continue to object. Jan if you are OK with that I'll ask Julien to break the tie and pick the name to use. Julien can you please help us move forward?
On 26.10.2023 00:38, Stefano Stabellini wrote: > On Wed, 25 Oct 2023, Jan Beulich wrote: >> On 25.10.2023 16:50, Nicola Vetrini wrote: >>> Ok, I'll send a revised version using MASK_LOWEST_BIT, taking into >>> account also the >>> other comments about the explanation on the macro definition >>> (which some IDEs even show when hovering on its usage, which could >>> partially address >>> the latter concern). >> >> You're of course free to do so, but since - as indicated before - >> MASK_LOWEST_BIT() imo isn't a better name than LOWEST_BIT(), I'll >> continue to object. > > Jan if you are OK with that I'll ask Julien to break the tie and pick > the name to use. Julien can you please help us move forward? Hmm, I'm having trouble seeing us at the point of breaking ties yet. First we need naming suggestions which actually unambiguously describe what's being done by the macro. I gave one suggestion which I think fulfills this property, but is a little too long for my taste. I gave another suggestion with a far-off but shorter name, which I can appreciate isn't liked. I've not seen other suggestions fulfilling this base criteria. Jan
On 26/10/2023 08:52, Jan Beulich wrote: > On 26.10.2023 00:38, Stefano Stabellini wrote: >> On Wed, 25 Oct 2023, Jan Beulich wrote: >>> On 25.10.2023 16:50, Nicola Vetrini wrote: >>>> Ok, I'll send a revised version using MASK_LOWEST_BIT, taking into >>>> account also the >>>> other comments about the explanation on the macro definition >>>> (which some IDEs even show when hovering on its usage, which could >>>> partially address >>>> the latter concern). >>> >>> You're of course free to do so, but since - as indicated before - >>> MASK_LOWEST_BIT() imo isn't a better name than LOWEST_BIT(), I'll >>> continue to object. >> >> Jan if you are OK with that I'll ask Julien to break the tie and pick >> the name to use. Julien can you please help us move forward? > > Hmm, I'm having trouble seeing us at the point of breaking ties yet. > First we need naming suggestions which actually unambiguously > describe what's being done by the macro. I gave one suggestion which > I think fulfills this property, but is a little too long for my > taste. I gave another suggestion with a far-off but shorter name, > which I can appreciate isn't liked. I've not seen other suggestions > fulfilling this base criteria. > > Jan Any name is fine with me. ISOLATE_LOW_BIT may be longish, but the macro would be used in just a few places for a specific reason, so the loss in readability is probably not that high.
Hi, On 26/10/2023 11:32, Nicola Vetrini wrote: > On 26/10/2023 08:52, Jan Beulich wrote: >> On 26.10.2023 00:38, Stefano Stabellini wrote: >>> On Wed, 25 Oct 2023, Jan Beulich wrote: >>>> On 25.10.2023 16:50, Nicola Vetrini wrote: >>>>> Ok, I'll send a revised version using MASK_LOWEST_BIT, taking into >>>>> account also the >>>>> other comments about the explanation on the macro definition >>>>> (which some IDEs even show when hovering on its usage, which could >>>>> partially address >>>>> the latter concern). >>>> >>>> You're of course free to do so, but since - as indicated before - >>>> MASK_LOWEST_BIT() imo isn't a better name than LOWEST_BIT(), I'll >>>> continue to object. >>> >>> Jan if you are OK with that I'll ask Julien to break the tie and pick >>> the name to use. Julien can you please help us move forward? >> >> Hmm, I'm having trouble seeing us at the point of breaking ties yet. >> First we need naming suggestions which actually unambiguously >> describe what's being done by the macro. I gave one suggestion which >> I think fulfills this property, but is a little too long for my >> taste. I gave another suggestion with a far-off but shorter name, >> which I can appreciate isn't liked. I've not seen other suggestions >> fulfilling this base criteria. >> >> Jan > > Any name is fine with me. ISOLATE_LOW_BIT may be longish, but the macro > would be used > in just a few places for a specific reason, so the loss in readability > is probably not > that high. +1. It doesn't seem we will be able to find a name that 100% fit all the criteria. So of all the choice, my preference would be ISOLATE_LOW_BIT(). Cheers,
On Thu, 26 Oct 2023, Julien Grall wrote: > On 26/10/2023 11:32, Nicola Vetrini wrote: > > On 26/10/2023 08:52, Jan Beulich wrote: > > > On 26.10.2023 00:38, Stefano Stabellini wrote: > > > > On Wed, 25 Oct 2023, Jan Beulich wrote: > > > > > On 25.10.2023 16:50, Nicola Vetrini wrote: > > > > > > Ok, I'll send a revised version using MASK_LOWEST_BIT, taking into > > > > > > account also the > > > > > > other comments about the explanation on the macro definition > > > > > > (which some IDEs even show when hovering on its usage, which could > > > > > > partially address > > > > > > the latter concern). > > > > > > > > > > You're of course free to do so, but since - as indicated before - > > > > > MASK_LOWEST_BIT() imo isn't a better name than LOWEST_BIT(), I'll > > > > > continue to object. > > > > > > > > Jan if you are OK with that I'll ask Julien to break the tie and pick > > > > the name to use. Julien can you please help us move forward? > > > > > > Hmm, I'm having trouble seeing us at the point of breaking ties yet. > > > First we need naming suggestions which actually unambiguously > > > describe what's being done by the macro. I gave one suggestion which > > > I think fulfills this property, but is a little too long for my > > > taste. I gave another suggestion with a far-off but shorter name, > > > which I can appreciate isn't liked. I've not seen other suggestions > > > fulfilling this base criteria. > > > > > > Jan > > > > Any name is fine with me. ISOLATE_LOW_BIT may be longish, but the macro > > would be used > > in just a few places for a specific reason, so the loss in readability is > > probably not > > that high. > > +1. It doesn't seem we will be able to find a name that 100% fit all the > criteria. So of all the choice, my preference would be ISOLATE_LOW_BIT(). It is clear enough, so +1
diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl b/automation/eclair_analysis/ECLAIR/deviations.ecl index fa56e5c00a27..9390f6d839ff 100644 --- 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 + # # Series 13 # diff --git a/docs/misra/deviations.rst b/docs/misra/deviations.rst index 8511a189253b..c3c65f4a9454 100644 --- 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. + * - R13.5 - All developers and reviewers can be safely assumed to be well aware of the short-circuit evaluation strategy for logical operators. diff --git a/xen/include/xen/macros.h b/xen/include/xen/macros.h index d0caae7db298..49f3ebf848e9 100644 --- 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)) + +#define MASK_EXTR(v, m) (((v) & (m)) / LOWEST_BIT(m)) +#define MASK_INSR(v, m) (((v) * LOWEST_BIT(m)) & (m)) #define count_args_(dot, a1, a2, a3, a4, a5, a6, a7, a8, x, ...) x #define count_args(args...) \ diff --git a/xen/include/xen/types.h b/xen/include/xen/types.h index aea259db1ef2..23cad71c8a47 100644 --- a/xen/include/xen/types.h +++ b/xen/include/xen/types.h @@ -31,9 +31,9 @@ typedef __PTRDIFF_TYPE__ ptrdiff_t; #define NULL ((void*)0) #endif -#define INT8_MIN (-127-1) -#define INT16_MIN (-32767-1) -#define INT32_MIN (-2147483647-1) +#define INT8_MIN (-127 - 1) +#define INT16_MIN (-32767 - 1) +#define INT32_MIN (-2147483647 - 1) #define INT8_MAX (127) #define INT16_MAX (32767) @@ -43,10 +43,10 @@ typedef __PTRDIFF_TYPE__ ptrdiff_t; #define UINT16_MAX (65535) #define UINT32_MAX (4294967295U) -#define INT_MAX ((int)(~0U>>1)) +#define INT_MAX ((int)(~0U >> 1)) #define INT_MIN (-INT_MAX - 1) #define UINT_MAX (~0U) -#define LONG_MAX ((long)(~0UL>>1)) +#define LONG_MAX ((long)(~0UL >> 1)) #define LONG_MIN (-LONG_MAX - 1) #define ULONG_MAX (~0UL)
The purpose of this macro is to encapsulate the well-known expression 'x & -x', that in 2's complement architectures on unsigned integers will give 2^ffs(x), where ffs(x) is the position of the lowest set bit in x. A deviation for ECLAIR is also introduced. Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> --- Changes in v2: - rename to LOWEST_BIT Changes in v3: - entry for deviations.rst - comment on the macro defn --- automation/eclair_analysis/ECLAIR/deviations.ecl | 6 ++++++ docs/misra/deviations.rst | 7 +++++++ xen/include/xen/macros.h | 7 +++++-- xen/include/xen/types.h | 10 +++++----- 4 files changed, 23 insertions(+), 7 deletions(-)