[aarch64] assert(is_power_of_2(x)) failed: x must be a power of 2: 0xffffffff80000000

[Stefan Karlsson]

The situation is a bit awkward.

The old implementation was inconsistent because it returned:
  true for is_power_of_2((intptr_t)0x8000000000000000ULL)
  false for is_power_of_2((int)0x80000000))

The new implementation returns true for both (unfortunately with UB).

An alternative implementation would be to return false for negative 
values. This would be easy to implement and get rid of the UB. However, 
this could start to break code with negative intptr_ts.

It think it would be good if we are consistent with the return values 
for negative values of ints and intptr_ts.

If we convert the parameter to uint64_t, either by casting it inside the 
function or using uint64_t as the parameter type, then 
is_power_of_2((int)0x80000000)) will return false just like the original 
implementation.

An alternative would be to forbid negative values, just like exact_log2 
does. This requires changes to code calling is_power_of_2.

StefanK

On 2020-02-25 20:01, Doerr, Martin wrote:
> Hi all,
>
>> These overflow when x is the largest negative integer. This is
>> Undefined Behaviour, so we have to fix it somehow.
> That's right.
>
>> We'd do much better
>> converting to an unsigned type and then performing the calculation on
>> that. Maybe we should provide just one definition of this function,
>> casting its argument to uint64_t.
> I agree. We should never use is_power_of_2 or exact_log2 with signed types.
>
> Best regards,
> Martin
>
>
>> -----Original Message-----
>> From: hotspot-compiler-dev <hotspot-compiler-dev-
>> bounces at openjdk.java.net> On Behalf Of Andrew Haley
>> Sent: Dienstag, 25. Februar 2020 10:16
>> To: Schmidt, Lutz <lutz.schmidt at sap.com>; 'hotspot-compiler-
>> dev at openjdk.java.net' <hotspot-compiler-dev at openjdk.java.net>; Stefan
>> Karlsson <stefan.karlsson at oracle.com>
>> Subject: Re: [aarch64] assert(is_power_of_2(x)) failed: x must be a power of
>> 2: 0xffffffff80000000
>>
>> On 2/24/20 4:27 PM, Schmidt, Lutz wrote:
>>      >     > A possible fix would include:
>>>      >  - make is_power_of_2() and exact_log_2() sign-aware.
>>>      >  - call is_power_of_2() and exact_log_2() with arguments of proper
>>>      > type (juint in this case).
>>>
>>>      Or fix exact_log2(jint) by specializing it appropriately.
>>>
>>> Yes, by treating the jint argument as unsigned.
>> OK, I'll look at that. However, studying the code, we have at least
>> one more problem, and it's a dangerous one.
>>
>> template <typename T>
>> bool is_power_of_2_t(T x) {
>>    return (x != T(0)) && ((x & (x - 1)) == T(0));
>> }
>>
>> // long version of is_power_of_2
>> inline bool is_power_of_2_long(jlong x) {
>>    return ((x != NoLongBits) && (mask_long_bits(x, x - 1) == NoLongBits));
>> }
>>
>> These overflow when x is the largest negative integer. This is
>> Undefined Behaviour, so we have to fix it somehow. We'd do much better
>> converting to an unsigned type and then performing the calculation on
>> that. Maybe we should provide just one definition of this function,
>> casting its argument to uint64_t.
>>
>> UB problems keep occurring. It'd be good to test HotSpot changes with
>> -fsanitize=undefined, but we still have so much UB in HotSpot that
>> it's impractical to do so because there's so much noise.
>>
>> --
>> Andrew Haley  (he/him)
>> Java Platform Lead Engineer
>> Red Hat UK Ltd. <https://urldefense.com/v3/__https://www.redhat.com__;!!GqivPVa7Brio!I1bMJtH4BDSpPU_g7rFVb_-ninf-Gc2e1mvcyfvsO6vTutgkONhT-_LSYJ0O8Ckw9Y-j$ >
>> https://urldefense.com/v3/__https://keybase.io/andrewhaley__;!!GqivPVa7Brio!I1bMJtH4BDSpPU_g7rFVb_-ninf-Gc2e1mvcyfvsO6vTutgkONhT-_LSYJ0O8Nd_YYxx$
>> EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671