RFR: 8240615: is_power_of_2() has Undefined Behaviour and is inconsistent

[Stefan Karlsson]

Hi Andrew,

The implementation of is_power_of_2 looks good to me.

Since this changes the value of is_power_of_2(LONG_MIN), did you try to 
figure out if any of the existing code relies on this returning true? It 
might be prudent to run this patch through testing with an assert 
catching any places where LONG_MIN is used as the argument. I can run 
that on our side, but need help for full platform coverage.

You used std::numeric_limits<T>::min() in the test. There's a comment in 
powerOfTwo.hpp stating:

// Helper function to get the maximum positive value. Implemented here
// since using std::numeric_limits<T>::max() seems problematic on some
// platforms.

I don't know if this means that min() is problematic as well?

Thanks for fixing this.

StefanK

On 2020-03-05 19:06, Andrew Haley wrote:
> is_power_of_2() has always behaved oddly, and it still behaves oddly
> but in a different way since 8183574: Unify the is_power_of_2
> functions. And this change breaks TCK on AArch64 systems.
>
> Previously, is_power_of_2(long) returned true for LONG_MIN, the most
> -ve signed long, but returned false for INT_MIN, the most -ve signed
> int.
>
> Also, is_power_of_2(long) had Undefined Behaviour (UB) for LONG_MIN:
> it overflowed from the most -ve long to the most +ve long.
>
> Now, is_power_of_2() has Undefined Behaviour for both int and long
> arguments when applied to their respective most negative values. In
> testing it returns true for both, but as this is UB it is not
> guaranteed.
>
> previous definition:
>
> inline bool is_power_of_2(intptr_t x) {
>    return ((x != NoBits) && (mask_bits(x, x - 1) == NoBits));
> }
>
> since JDK-8240615:
>
> template <typename T>
> bool is_power_of_2(T x) {
>    return (x != T(0)) && ((x & (x - 1)) == T(0));
> }
>
> I do not believe that this behavioural change was deliberate.
>
> I suggest that we should change the definition of is_power_of_2() for
> all signed types such that it returns false for negative
> arguments. This makes the behaviour consistent and also avoids
> undefined behaviour.
>
> This is important because is_power_of_2 is sometimes used as a guard
> in ADfile patterns:
>
> instruct cmpI_branch_bit(cmpOpEqNe cmp, iRegIorL2I op1, immI op2, immI0 op3, label labl) %{
>    match(If cmp (CmpI (AndI op1 op2) op3));
>    predicate(is_power_of_2(n->in(2)->in(1)->in(2)->get_int()));
>    effect(USE labl);
>
>    ins_cost(BRANCH_COST);
>    format %{ "tb$cmp $op1, $op2, $labl" %}
>    ins_encode %{
>      Label* L = $labl$$label;
>      Assembler::Condition cond = (Assembler::Condition)$cmp$$cmpcode;
>      int bit = exact_log2($op2$$constant);
>     ...
>
> This has a runtime error since JDK-8240615 because the initial
> is_power_of_2 test succeeds, but the later exact_log2
> fails. exact_log2() takes an intptr_t, not an int. On 64-bit systems,
> intptr_t is a 64-bit signed type, so the first call to is_power_of_2
> succeeds, whereas exact_log2 fails because its arg is sign-extended
> to 64 bits.
>
> http://cr.openjdk.java.net/~aph/8240615-1/
>