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

[Doerr, Martin]

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://www.redhat.com>
> https://keybase.io/andrewhaley
> EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671