8240615: is_power_of_2() has Undefined Behaviour and is inconsistent

Reviewed-by: jrose, redestad
openjdk · Mar 10, 2020 · 661c073594e71c6aa36b345c22c8a2fe390a4726 · 661c073
1 parent 534331f
commit 661c073594e71c6aa36b345c22c8a2fe390a4726
Showing with 16 additions and 12 deletions.

+8 −8 src/hotspot/cpu/aarch64/aarch64.ad

+3 −3 src/hotspot/cpu/x86/x86_64.ad

+1 −1 src/hotspot/share/utilities/powerOfTwo.hpp

+4 −0 test/hotspot/gtest/utilities/test_powerOfTwo.cpp
diff --git a/src/hotspot/cpu/aarch64/aarch64.ad b/src/hotspot/cpu/aarch64/aarch64.ad
@@ -14936,15 +14936,15 @@ instruct cmpI_branch_sign(cmpOpLtGe cmp, iRegIorL2I op1, immI0 op2, label labl)
 
 instruct cmpL_branch_bit(cmpOpEqNe cmp, iRegL op1, immL op2, immL0 op3, label labl) %{
   match(If cmp (CmpL (AndL op1 op2) op3));
-  predicate(is_power_of_2(n->in(2)->in(1)->in(2)->get_long()));
+  predicate(is_power_of_2((julong)n->in(2)->in(1)->in(2)->get_long()));
   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);
+    int bit = exact_log2_long($op2$$constant);
     __ tbr(cond, $op1$$Register, bit, *L);
   %}
   ins_pipe(pipe_cmp_branch);
@@ -14953,15 +14953,15 @@ instruct cmpL_branch_bit(cmpOpEqNe cmp, iRegL op1, immL op2, immL0 op3, label la
 
 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()));
+  predicate(is_power_of_2((juint)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);
+    int bit = exact_log2((juint)$op2$$constant);
     __ tbr(cond, $op1$$Register, bit, *L);
   %}
   ins_pipe(pipe_cmp_branch);
@@ -15001,31 +15001,31 @@ instruct far_cmpI_branch_sign(cmpOpLtGe cmp, iRegIorL2I op1, immI0 op2, label la
 
 instruct far_cmpL_branch_bit(cmpOpEqNe cmp, iRegL op1, immL op2, immL0 op3, label labl) %{
   match(If cmp (CmpL (AndL op1 op2) op3));
-  predicate(is_power_of_2(n->in(2)->in(1)->in(2)->get_long()));
+  predicate(is_power_of_2((julong)n->in(2)->in(1)->in(2)->get_long()));
   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);
+    int bit = exact_log2_long($op2$$constant);
     __ tbr(cond, $op1$$Register, bit, *L, /*far*/true);
   %}
   ins_pipe(pipe_cmp_branch);
 %}
 
 instruct far_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()));
+  predicate(is_power_of_2((juint)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);
+    int bit = exact_log2((juint)$op2$$constant);
     __ tbr(cond, $op1$$Register, bit, *L, /*far*/true);
   %}
   ins_pipe(pipe_cmp_branch);

diff --git a/src/hotspot/cpu/x86/x86_64.ad b/src/hotspot/cpu/x86/x86_64.ad
@@ -3120,7 +3120,7 @@ operand immL32()
 
 operand immL_Pow2()
 %{
-  predicate(is_power_of_2(n->get_long()));
+  predicate(is_power_of_2((julong)n->get_long()));
   match(ConL);
 
   op_cost(15);
@@ -3130,7 +3130,7 @@ operand immL_Pow2()
 
 operand immL_NotPow2()
 %{
-  predicate(is_power_of_2(~n->get_long()));
+  predicate(is_power_of_2((julong)~n->get_long()));
   match(ConL);
 
   op_cost(15);
@@ -10029,7 +10029,7 @@ instruct btsL_mem_imm(memory dst, immL_Pow2 con, rFlagsReg cr)
   ins_cost(125);
   format %{ "btsq    $dst, log2($con)\t# long" %}
   ins_encode %{
-    __ btsq($dst$$Address, log2_long($con$$constant));
+    __ btsq($dst$$Address, log2_long((julong)$con$$constant));
   %}
   ins_pipe(ialu_mem_imm);
 %}

diff --git a/src/hotspot/share/utilities/powerOfTwo.hpp b/src/hotspot/share/utilities/powerOfTwo.hpp
@@ -36,7 +36,7 @@
 
 template <typename T>
 bool is_power_of_2(T x) {
-  return (x != T(0)) && ((x & (x - 1)) == T(0));
+  return (x > T(0)) && ((x & (x - 1)) == T(0));
 }
 
 // Log2 of a power of 2

diff --git a/test/hotspot/gtest/utilities/test_powerOfTwo.cpp b/test/hotspot/gtest/utilities/test_powerOfTwo.cpp
@@ -36,6 +36,10 @@ template <typename T> static void test_is_power_of_2() {
   EXPECT_FALSE(is_power_of_2(T(0)));
   EXPECT_FALSE(is_power_of_2(~T(0)));
 
+  if (IsSigned<T>::value) {
+    EXPECT_FALSE(is_power_of_2(std::numeric_limits<T>::min()));
+  }
+
   // Test true
   for (T i = max_pow2<T>(); i > 0; i = (i >> 1)) {
     EXPECT_TRUE(is_power_of_2(i)) << "value = " << T(i);