On 27.08.2024 13:50, Andrew Cooper wrote:
> On 26/08/2024 12:55 pm, Jan Beulich wrote:
>> On 23.08.2024 01:06, Andrew Cooper wrote:
>>> --- a/xen/include/xen/bitops.h
>>> +++ b/xen/include/xen/bitops.h
>>> @@ -302,6 +302,14 @@ static always_inline __pure unsigned int
>>> hweightl(unsigned long x)
>>> #endif
>>> }
>>>
>>> +static always_inline __pure unsigned int hweight64(uint64_t x)
>>> +{
>>> + if ( BITS_PER_LONG == 64 )
>>> + return hweightl(x);
>>> + else
>>> + return hweightl(x >> 32) + hweightl(x);
>> This assume BITS_PER_LONG == 32, which of course is true right now, but
>> doesn't need to be in general. Better add an explicit cast to uint32_t
>> (or masking by 0xffffffffU)?
>
> This is part of the point of putting in the self-tests. They're
> intended to catch things like this in new build environments.
I don't think I saw any testcase where the result would be wrong if
this split didn't truncate x to the low 32 bits on the rhs of the +.
> Although, I think we've got enough cases which will #error on
> BITS_PER_LONG not being 32 or 64.
My take on this is: Such checks (#error or whatever else precaution)
should like in every single place where violating the assumptions
made would matter. Or else - how do you locate all the places that
need changing?
> Again, this is modelled after f[fl]s64() which have the same
> expectations about the BITS_PER_LONG != 64 case.
Both of them are fine afaict. fls64() has an explicit intermediate
variable of type uint32_t, and ffs64() has a (uint32_t)x as part
of the conditional expression achieving the intended effect.
Anyway, why not use hweight32() instead of hweightl() here? That'll
make things very explicit.
Jan
