Devise macros to encapsulate (x & -x)

Devise macros to encapsulate (x & -x)

[Nicola Vetrini]

Hi all,

As discussed in this thread [1], which is about complying with MISRA C 
Rule 10.1,
a macro was introduced to encapsulate a well-known construct:

/*
  * Given an unsigned integer argument, expands to a mask where just the 
least
  * significant nonzero bit of the argument is set, or 0 if no bits are 
set.
  */
#define ISOLATE_LSB(x) ((x) & -(x))

This macro has a gained some calls in the subsequent patches in that 
thread, but concerns were raised around the fact that it would be better 
to devise a macro that evaluates its argument only once. A proposed 
solution is this (thanks to Jan Beulich):

#define ISOLATE_LSB(x) ({ \
      typeof(x) x_ = (x); \
      x_ & -x_; \
})

However, it can't be used in all call sites that the previous macro 
would have once that series is committed, as can be seen in [2]. 
Therefore, while the implementation looks ok,
a case has been made to have separate macros, of which the latter form 
is preferred.

The following points require some thought:

- where the single evaluation macro should be placed?
   One proposed location is xen/include/xen/bitops.h
- is the proposed form actually the best, or maybe it could be an inline 
function?

Then testing can happen and a subsequent version of those uncommitted 
patches introducing uses of either construct will be submitted, to 
modify all the call sites only once in the commit history.

Let me know what you think of this.
Regards,

[1] 
https://lore.kernel.org/xen-devel/8a1313b3ab5ba6dd556cf37409e3b703@bugseng.com/T/#mdeb510325e1acacb6477a88de8577e9e87351ba5
[2] https://gitlab.com/xen-project/people/bugseng/xen/-/jobs/5423693947

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

Re: Devise macros to encapsulate (x & -x)

[Andrew Cooper]

On 17/11/2023 10:17 am, Nicola Vetrini wrote:
> Hi all,
>
> As discussed in this thread [1], which is about complying with MISRA C
> Rule 10.1,
> a macro was introduced to encapsulate a well-known construct:
>
> /*
>  * Given an unsigned integer argument, expands to a mask where just
> the least
>  * significant nonzero bit of the argument is set, or 0 if no bits are
> set.
>  */
> #define ISOLATE_LSB(x) ((x) & -(x))
>
> This macro has a gained some calls in the subsequent patches in that
> thread, but concerns were raised around the fact that it would be
> better to devise a macro that evaluates its argument only once. A
> proposed solution is this (thanks to Jan Beulich):
>
> #define ISOLATE_LSB(x) ({ \
>      typeof(x) x_ = (x); \
>      x_ & -x_; \
> })

Of course this was going to explode.

This isn't even the first time an unwise attempt to do single-evaluation
has needed to be reverted because it doesn't work with Integer Constant
Expressions.

Switch it back to the first form.  It's obviously a macro to begin with,
and not likely to be used in cases that have side effects.

~Andrew

Re: Devise macros to encapsulate (x & -x)

[Nicola Vetrini]

On 2023-11-17 12:04, Andrew Cooper wrote:
> On 17/11/2023 10:17 am, Nicola Vetrini wrote:
>> Hi all,
>> 
>> As discussed in this thread [1], which is about complying with MISRA C
>> Rule 10.1,
>> a macro was introduced to encapsulate a well-known construct:
>> 
>> /*
>>  * Given an unsigned integer argument, expands to a mask where just
>> the least
>>  * significant nonzero bit of the argument is set, or 0 if no bits are
>> set.
>>  */
>> #define ISOLATE_LSB(x) ((x) & -(x))
>> 
>> This macro has a gained some calls in the subsequent patches in that
>> thread, but concerns were raised around the fact that it would be
>> better to devise a macro that evaluates its argument only once. A
>> proposed solution is this (thanks to Jan Beulich):
>> 
>> #define ISOLATE_LSB(x) ({ \
>>      typeof(x) x_ = (x); \
>>      x_ & -x_; \
>> })
> 
> Of course this was going to explode.
> 
> This isn't even the first time an unwise attempt to do 
> single-evaluation
> has needed to be reverted because it doesn't work with Integer Constant
> Expressions.
> 
> Switch it back to the first form.  It's obviously a macro to begin 
> with,
> and not likely to be used in cases that have side effects.
> 
> ~Andrew

Actually no usages of either forms are yet committed, just the 
definition of the first form, so nothing needs to be reverted. I should 
have clarified that, sorry. If the other patches in the series go in 
unmodified (modulo renaming, but that's trivial), they would use the 
first form.

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

Re: Devise macros to encapsulate (x & -x)

[Jan Beulich]

On 17.11.2023 12:15, Nicola Vetrini wrote:
> On 2023-11-17 12:04, Andrew Cooper wrote:
>> On 17/11/2023 10:17 am, Nicola Vetrini wrote:
>>> Hi all,
>>>
>>> As discussed in this thread [1], which is about complying with MISRA C
>>> Rule 10.1,
>>> a macro was introduced to encapsulate a well-known construct:
>>>
>>> /*
>>>  * Given an unsigned integer argument, expands to a mask where just
>>> the least
>>>  * significant nonzero bit of the argument is set, or 0 if no bits are
>>> set.
>>>  */
>>> #define ISOLATE_LSB(x) ((x) & -(x))
>>>
>>> This macro has a gained some calls in the subsequent patches in that
>>> thread, but concerns were raised around the fact that it would be
>>> better to devise a macro that evaluates its argument only once. A
>>> proposed solution is this (thanks to Jan Beulich):
>>>
>>> #define ISOLATE_LSB(x) ({ \
>>>      typeof(x) x_ = (x); \
>>>      x_ & -x_; \
>>> })
>>
>> Of course this was going to explode.
>>
>> This isn't even the first time an unwise attempt to do 
>> single-evaluation
>> has needed to be reverted because it doesn't work with Integer Constant
>> Expressions.
>>
>> Switch it back to the first form.  It's obviously a macro to begin 
>> with,
>> and not likely to be used in cases that have side effects.

I guess Nicola's original mail was lacking some pieces. After the issue
with the statement expression form was pointed out, I never asked to
replace the existing (already committed, ...

> Actually no usages of either forms are yet committed, just the 
> definition of the first form, so nothing needs to be reverted.

... with actual uses in MASK_EXTR() and MASK_INSR()) macro. Instead I
was suggesting to have a _second_ macro for use wherever Integer Constant
Expressions aren't the limiting factor. E.g. isolate_lsb(), deliberately
lower-case to look more like a function (and thus communicating that its
argument indeed is going to be evaluated only once, as would be the case
if the whole thing was a function).

Jan

Re: Devise macros to encapsulate (x & -x)

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 1221 bytes --]

On Fri, 17 Nov 2023, Andrew Cooper wrote:
> On 17/11/2023 10:17 am, Nicola Vetrini wrote:
> > Hi all,
> >
> > As discussed in this thread [1], which is about complying with MISRA C
> > Rule 10.1,
> > a macro was introduced to encapsulate a well-known construct:
> >
> > /*
> >  * Given an unsigned integer argument, expands to a mask where just
> > the least
> >  * significant nonzero bit of the argument is set, or 0 if no bits are
> > set.
> >  */
> > #define ISOLATE_LSB(x) ((x) & -(x))
> >
> > This macro has a gained some calls in the subsequent patches in that
> > thread, but concerns were raised around the fact that it would be
> > better to devise a macro that evaluates its argument only once. A
> > proposed solution is this (thanks to Jan Beulich):
> >
> > #define ISOLATE_LSB(x) ({ \
> >      typeof(x) x_ = (x); \
> >      x_ & -x_; \
> > })
> 
> Of course this was going to explode.
> 
> This isn't even the first time an unwise attempt to do single-evaluation
> has needed to be reverted because it doesn't work with Integer Constant
> Expressions.
> 
> Switch it back to the first form.  It's obviously a macro to begin with,
> and not likely to be used in cases that have side effects.

+1

Re: Devise macros to encapsulate (x & -x)

[Julien Grall]

On 18/11/2023 02:46, Stefano Stabellini wrote:
> On Fri, 17 Nov 2023, Andrew Cooper wrote:
>> On 17/11/2023 10:17 am, Nicola Vetrini wrote:
>>> Hi all,
>>>
>>> As discussed in this thread [1], which is about complying with MISRA C
>>> Rule 10.1,
>>> a macro was introduced to encapsulate a well-known construct:
>>>
>>> /*
>>>   * Given an unsigned integer argument, expands to a mask where just
>>> the least
>>>   * significant nonzero bit of the argument is set, or 0 if no bits are
>>> set.
>>>   */
>>> #define ISOLATE_LSB(x) ((x) & -(x))
>>>
>>> This macro has a gained some calls in the subsequent patches in that
>>> thread, but concerns were raised around the fact that it would be
>>> better to devise a macro that evaluates its argument only once. A
>>> proposed solution is this (thanks to Jan Beulich):
>>>
>>> #define ISOLATE_LSB(x) ({ \
>>>       typeof(x) x_ = (x); \
>>>       x_ & -x_; \
>>> })
>>
>> Of course this was going to explode.
>>
>> This isn't even the first time an unwise attempt to do single-evaluation
>> has needed to be reverted because it doesn't work with Integer Constant
>> Expressions.
>>
>> Switch it back to the first form.  It's obviously a macro to begin with,
>> and not likely to be used in cases that have side effects.
> 
> +1

FWIW +1. I don't much like the idea to have two different versions of 
the helper if there is no real need for it.

Cheers,

-- 
Julien Grall

Re: Devise macros to encapsulate (x & -x)

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 1766 bytes --]

On Mon, 20 Nov 2023, Julien Grall wrote:
> On 18/11/2023 02:46, Stefano Stabellini wrote:
> > On Fri, 17 Nov 2023, Andrew Cooper wrote:
> > > On 17/11/2023 10:17 am, Nicola Vetrini wrote:
> > > > Hi all,
> > > > 
> > > > As discussed in this thread [1], which is about complying with MISRA C
> > > > Rule 10.1,
> > > > a macro was introduced to encapsulate a well-known construct:
> > > > 
> > > > /*
> > > >   * Given an unsigned integer argument, expands to a mask where just
> > > > the least
> > > >   * significant nonzero bit of the argument is set, or 0 if no bits are
> > > > set.
> > > >   */
> > > > #define ISOLATE_LSB(x) ((x) & -(x))
> > > > 
> > > > This macro has a gained some calls in the subsequent patches in that
> > > > thread, but concerns were raised around the fact that it would be
> > > > better to devise a macro that evaluates its argument only once. A
> > > > proposed solution is this (thanks to Jan Beulich):
> > > > 
> > > > #define ISOLATE_LSB(x) ({ \
> > > >       typeof(x) x_ = (x); \
> > > >       x_ & -x_; \
> > > > })
> > > 
> > > Of course this was going to explode.
> > > 
> > > This isn't even the first time an unwise attempt to do single-evaluation
> > > has needed to be reverted because it doesn't work with Integer Constant
> > > Expressions.
> > > 
> > > Switch it back to the first form.  It's obviously a macro to begin with,
> > > and not likely to be used in cases that have side effects.
> > 
> > +1
> 
> FWIW +1. I don't much like the idea to have two different versions of the
> helper if there is no real need for it.

Jan, would you be willing to accept that other maintainers have a
preference for having a single MACRO even if suboptimal?

If so, can we go ahead and commit the original patches?

Re: Devise macros to encapsulate (x & -x)

[Jan Beulich]

On 22.11.2023 02:43, Stefano Stabellini wrote:
> On Mon, 20 Nov 2023, Julien Grall wrote:
>> On 18/11/2023 02:46, Stefano Stabellini wrote:
>>> On Fri, 17 Nov 2023, Andrew Cooper wrote:
>>>> On 17/11/2023 10:17 am, Nicola Vetrini wrote:
>>>>> Hi all,
>>>>>
>>>>> As discussed in this thread [1], which is about complying with MISRA C
>>>>> Rule 10.1,
>>>>> a macro was introduced to encapsulate a well-known construct:
>>>>>
>>>>> /*
>>>>>   * Given an unsigned integer argument, expands to a mask where just
>>>>> the least
>>>>>   * significant nonzero bit of the argument is set, or 0 if no bits are
>>>>> set.
>>>>>   */
>>>>> #define ISOLATE_LSB(x) ((x) & -(x))
>>>>>
>>>>> This macro has a gained some calls in the subsequent patches in that
>>>>> thread, but concerns were raised around the fact that it would be
>>>>> better to devise a macro that evaluates its argument only once. A
>>>>> proposed solution is this (thanks to Jan Beulich):
>>>>>
>>>>> #define ISOLATE_LSB(x) ({ \
>>>>>       typeof(x) x_ = (x); \
>>>>>       x_ & -x_; \
>>>>> })
>>>>
>>>> Of course this was going to explode.
>>>>
>>>> This isn't even the first time an unwise attempt to do single-evaluation
>>>> has needed to be reverted because it doesn't work with Integer Constant
>>>> Expressions.
>>>>
>>>> Switch it back to the first form.  It's obviously a macro to begin with,
>>>> and not likely to be used in cases that have side effects.
>>>
>>> +1
>>
>> FWIW +1. I don't much like the idea to have two different versions of the
>> helper if there is no real need for it.
> 
> Jan, would you be willing to accept that other maintainers have a
> preference for having a single MACRO even if suboptimal?

I can live with that, even if I'm surprised by this perspective that others
take. How can we, in reviews, tell people to make sure arguments are
evaluated only once, when then we deliberately do otherwise in a case like
the one here? The criteria of "not likely to be used in cases that have
side effects" is an extremely fuzzy and sufficiently weak one, imo. I for
one am even worried about the uses in MASK_EXTR() / MASK_INSR(), and would
have considered introducing single-evaluation forms there as well.

> If so, can we go ahead and commit the original patches?

Well, the renaming needs to be done there anyway.

Jan

Re: Devise macros to encapsulate (x & -x)

[Nicola Vetrini]

>> 
>> Jan, would you be willing to accept that other maintainers have a
>> preference for having a single MACRO even if suboptimal?
> 
> I can live with that, even if I'm surprised by this perspective that 
> others
> take. How can we, in reviews, tell people to make sure arguments are
> evaluated only once, when then we deliberately do otherwise in a case 
> like
> the one here? The criteria of "not likely to be used in cases that have
> side effects" is an extremely fuzzy and sufficiently weak one, imo. I 
> for
> one am even worried about the uses in MASK_EXTR() / MASK_INSR(), and 
> would
> have considered introducing single-evaluation forms there as well.
> 
>> If so, can we go ahead and commit the original patches?
> 
> Well, the renaming needs to be done there anyway.
> 

I can do the renaming if you don't feel particularly safe doing it on 
commit. I already modified my local version to do experiments with 
single evaluation anyway.

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

Re: Devise macros to encapsulate (x & -x)

[Stefano Stabellini]

On Wed, 22 Nov 2023, Nicola Vetrini wrote:
> > > 
> > > Jan, would you be willing to accept that other maintainers have a
> > > preference for having a single MACRO even if suboptimal?
> > 
> > I can live with that, even if I'm surprised by this perspective that others
> > take. How can we, in reviews, tell people to make sure arguments are
> > evaluated only once, when then we deliberately do otherwise in a case like
> > the one here? The criteria of "not likely to be used in cases that have
> > side effects" is an extremely fuzzy and sufficiently weak one, imo. I for
> > one am even worried about the uses in MASK_EXTR() / MASK_INSR(), and would
> > have considered introducing single-evaluation forms there as well.
> > 
> > > If so, can we go ahead and commit the original patches?
> > 
> > Well, the renaming needs to be done there anyway.
> > 
> 
> I can do the renaming if you don't feel particularly safe doing it on commit

Please resend

Re: Devise macros to encapsulate (x & -x)

[Jan Beulich]

On 17.11.2023 11:17, Nicola Vetrini wrote:
> Hi all,
> 
> As discussed in this thread [1], which is about complying with MISRA C 
> Rule 10.1,
> a macro was introduced to encapsulate a well-known construct:
> 
> /*
>   * Given an unsigned integer argument, expands to a mask where just the 
> least
>   * significant nonzero bit of the argument is set, or 0 if no bits are 
> set.
>   */
> #define ISOLATE_LSB(x) ((x) & -(x))
> 
> This macro has a gained some calls in the subsequent patches in that 
> thread, but concerns were raised around the fact that it would be better 
> to devise a macro that evaluates its argument only once. A proposed 
> solution is this (thanks to Jan Beulich):
> 
> #define ISOLATE_LSB(x) ({ \
>       typeof(x) x_ = (x); \
>       x_ & -x_; \
> })
> 
> However, it can't be used in all call sites that the previous macro 
> would have once that series is committed, as can be seen in [2]. 
> Therefore, while the implementation looks ok,
> a case has been made to have separate macros, of which the latter form 
> is preferred.
> 
> The following points require some thought:
> 
> - where the single evaluation macro should be placed?
>    One proposed location is xen/include/xen/bitops.h

Or next to the existing one in macros.h. I can see pros and cons for either.

> - is the proposed form actually the best, or maybe it could be an inline 
> function?

How would you make such a function type generic?

Jan

Re: Devise macros to encapsulate (x & -x)

[Nicola Vetrini]

On 2023-11-17 12:39, Jan Beulich wrote:
> On 17.11.2023 11:17, Nicola Vetrini wrote:
>> Hi all,
>> 
>> As discussed in this thread [1], which is about complying with MISRA C
>> Rule 10.1,
>> a macro was introduced to encapsulate a well-known construct:
>> 
>> /*
>>   * Given an unsigned integer argument, expands to a mask where just 
>> the
>> least
>>   * significant nonzero bit of the argument is set, or 0 if no bits 
>> are
>> set.
>>   */
>> #define ISOLATE_LSB(x) ((x) & -(x))
>> 
>> This macro has a gained some calls in the subsequent patches in that
>> thread, but concerns were raised around the fact that it would be 
>> better
>> to devise a macro that evaluates its argument only once. A proposed
>> solution is this (thanks to Jan Beulich):
>> 
>> #define ISOLATE_LSB(x) ({ \
>>       typeof(x) x_ = (x); \
>>       x_ & -x_; \
>> })
>> 
>> However, it can't be used in all call sites that the previous macro
>> would have once that series is committed, as can be seen in [2].
>> Therefore, while the implementation looks ok,
>> a case has been made to have separate macros, of which the latter form
>> is preferred.
>> 
>> The following points require some thought:
>> 
>> - where the single evaluation macro should be placed?
>>    One proposed location is xen/include/xen/bitops.h
> 
> Or next to the existing one in macros.h. I can see pros and cons for 
> either.
> 
>> - is the proposed form actually the best, or maybe it could be an 
>> inline
>> function?
> 
> How would you make such a function type generic?
> 

Ah, yes indeed this wouldn't work.

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

Re: Devise macros to encapsulate (x & -x)

[Nicola Vetrini]

On 2023-11-17 11:17, Nicola Vetrini wrote:
> Hi all,
> 
> As discussed in this thread [1], which is about complying with MISRA C 
> Rule 10.1,
> a macro was introduced to encapsulate a well-known construct:
> 
> /*
>  * Given an unsigned integer argument, expands to a mask where just the 
> least
>  * significant nonzero bit of the argument is set, or 0 if no bits are 
> set.
>  */
> #define ISOLATE_LSB(x) ((x) & -(x))
> 
> This macro has a gained some calls in the subsequent patches in that 
> thread, but concerns were raised around the fact that it would be 
> better to devise a macro that evaluates its argument only once. A 
> proposed solution is this (thanks to Jan Beulich):
> 
> #define ISOLATE_LSB(x) ({ \
>      typeof(x) x_ = (x); \
>      x_ & -x_; \
> })
> 
> However, it can't be used in all call sites that the previous macro 
> would have once that series is committed, as can be seen in [2]. 
> Therefore, while the implementation looks ok,
> a case has been made to have separate macros, of which the latter form 
> is preferred.
> 
> The following points require some thought:
> 
> - where the single evaluation macro should be placed?
>   One proposed location is xen/include/xen/bitops.h
> - is the proposed form actually the best, or maybe it could be an 
> inline function?
> 
> Then testing can happen and a subsequent version of those uncommitted 
> patches introducing uses of either construct will be submitted, to 
> modify all the call sites only once in the commit history.
> 
> Let me know what you think of this.
> Regards,
> 
> [1] 
> https://lore.kernel.org/xen-devel/8a1313b3ab5ba6dd556cf37409e3b703@bugseng.com/T/#mdeb510325e1acacb6477a88de8577e9e87351ba5
> [2] https://gitlab.com/xen-project/people/bugseng/xen/-/jobs/5423693947

I did a few tests; the only places where the first form can't be 
replaced are:

- inside BUILD_BUG_ON (two instances in x86/x86_64/mm.c)
- the definition of MASK_EXTR/MASK_INSR
- the definition of PDX_GROUP_COUNT

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)