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

[Doerr, Martin]

Hi Stefan,

we could use 
STATIC_ASSERT(T(-1) > T(0));
to disallow usage with signed types, but that would require fixing all usages.

Casting int to unsinged long implies sign extend.
So I guess that log variants like log2_int are not correct, either.

Best regards,
Martin


> -----Original Message-----
> From: Stefan Karlsson <stefan.karlsson at oracle.com>
> Sent: Dienstag, 25. Februar 2020 22:42
> To: Doerr, Martin <martin.doerr at sap.com>; Andrew Haley
> <aph at redhat.com>; Schmidt, Lutz <lutz.schmidt at sap.com>; 'hotspot-
> compiler-dev at openjdk.java.net' <hotspot-compiler-
> dev at openjdk.java.net>
> Subject: Re: [aarch64] assert(is_power_of_2(x)) failed: x must be a power of
> 2: 0xffffffff80000000
> 
> 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__;!!GqivPVa
> 7Brio!I1bMJtH4BDSpPU_g7rFVb_-ninf-Gc2e1mvcyfvsO6vTutgkONhT-
> _LSYJ0O8Nd_YYxx$
> >> EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671