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

[Stefan Karlsson]

I ran with this patch:
diff --git a/src/hotspot/share/utilities/powerOfTwo.hpp 
b/src/hotspot/share/utilities/powerOfTwo.hpp
--- a/src/hotspot/share/utilities/powerOfTwo.hpp
+++ b/src/hotspot/share/utilities/powerOfTwo.hpp
@@ -36,6 +36,7 @@

  template <typename T>
  bool is_power_of_2(T x) {
+  assert(sizeof(x) != 8 || !IsSigned<T>::value || x != 
std::numeric_limits<T>::min(), "Caught min value");
    return (x > T(0)) && ((x & (x - 1)) == T(0));
  }

and it asserts in DaCapo in immL_Pow2 below:

0x00007f33727e26ea in State::_sub_Op_ConL (this=0x7f3340842f60, 
n=0x7f336cbb04b8) at src/hotspot/cpu/x86/x86_64.ad:3123

operand immL_Pow2()
%{
   predicate(is_power_of_2(n->get_long()));
   match(ConL);

   op_cost(15);
   format %{ %}
   interface(CONST_INTER);
%}

operand immL_NotPow2()
%{
   predicate(is_power_of_2(~n->get_long()));
   match(ConL);

   op_cost(15);
   format %{ %}
   interface(CONST_INTER);
%}

The predicate for Pow2 and NotPow2 will both be false because of this 
change. Is that a problem?

StefanK

On 2020-03-05 19:23, Stefan Karlsson wrote:
> 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/
>>
>