8295486: Inconsistent constant field values observed during compilation

TobiHartmann · TobiHartmann · commit cae577a7102e · 2023-02-01T11:15:35.000Z
Reviewed-by: chagedorn, kvn, jbhateja, vlivanov
diff --git a/src/hotspot/share/ci/ciArray.cpp b/src/hotspot/share/ci/ciArray.cpp
@@ -63,9 +63,7 @@ ciConstant ciArray::element_value_impl(BasicType elembt,
       assert(ary->is_objArray(), "");
       objArrayOop objary = (objArrayOop) ary;
       oop elem = objary->obj_at(index);
-      ciEnv* env = CURRENT_ENV;
-      ciObject* box = env->get_object(elem);
-      return ciConstant(T_OBJECT, box);
+      return ciConstant(elembt, CURRENT_ENV->get_object(elem));
     }
   default:
     break;
@@ -94,9 +92,15 @@ ciConstant ciArray::element_value_impl(BasicType elembt,
 // Returns T_ILLEGAL if there is no element at the given index.
 ciConstant ciArray::element_value(int index) {
   BasicType elembt = element_basic_type();
+  ciConstant value = check_constant_value_cache(index, elembt);
+  if (value.is_valid()) {
+    return value;
+  }
   GUARDED_VM_ENTRY(
-    return element_value_impl(elembt, get_arrayOop(), index);
+    value = element_value_impl(elembt, get_arrayOop(), index);
   )
+  add_to_constant_value_cache(index, value);
+  return value;
 }
 
 // ------------------------------------------------------------------
diff --git a/src/hotspot/share/ci/ciConstant.cpp b/src/hotspot/share/ci/ciConstant.cpp
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1999, 2019, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1999, 2023, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -32,6 +32,35 @@
 //
 // This class represents a constant value.
 
+// ------------------------------------------------------------------
+// ciConstant::is_null_or_zero
+bool ciConstant::is_null_or_zero() const {
+  if (!is_java_primitive(basic_type())) {
+    return as_object()->is_null_object();
+  } else if (type2size[basic_type()] == 1) {
+    // treat float bits as int, to avoid comparison with -0 and NaN
+    return (_value._int == 0);
+  } else if (type2size[basic_type()] == 2) {
+    // treat double bits as long, to avoid comparison with -0 and NaN
+    return (_value._long == 0);
+  } else {
+    return false;
+  }
+}
+
+// ------------------------------------------------------------------
+// ciConstant::is_loaded
+bool ciConstant::is_loaded() const {
+  if (is_valid()) {
+    if (is_reference_type(basic_type())) {
+      return as_object()->is_loaded();
+    } else {
+      return true;
+    }
+  }
+  return false;
+}
+
 // ------------------------------------------------------------------
 // ciConstant::print
 void ciConstant::print() {
diff --git a/src/hotspot/share/ci/ciConstant.hpp b/src/hotspot/share/ci/ciConstant.hpp
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1999, 2019, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1999, 2023, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -26,7 +26,7 @@
 #define SHARE_CI_CICONSTANT_HPP
 
 #include "ci/ciClassList.hpp"
-#include "ci/ciNullObject.hpp"
+#include "utilities/globalDefinitions.hpp"
 
 // ciConstant
 //
@@ -110,34 +110,14 @@ class ciConstant {
     return _value._object;
   }
 
-  bool      is_null_or_zero() const {
-    if (!is_java_primitive(basic_type())) {
-      return as_object()->is_null_object();
-    } else if (type2size[basic_type()] == 1) {
-      // treat float bits as int, to avoid comparison with -0 and NaN
-      return (_value._int == 0);
-    } else if (type2size[basic_type()] == 2) {
-      // treat double bits as long, to avoid comparison with -0 and NaN
-      return (_value._long == 0);
-    } else {
-      return false;
-    }
-  }
+  bool is_null_or_zero() const;
 
   bool is_valid() const {
     return basic_type() != T_ILLEGAL;
   }
 
-  bool is_loaded() const {
-    if (is_valid()) {
-      if (is_reference_type(basic_type())) {
-        return as_object()->is_loaded();
-      } else {
-        return true;
-      }
-    }
-    return false;
-  }
+  bool is_loaded() const;
+
   // Debugging output
   void print();
 };
diff --git a/src/hotspot/share/ci/ciField.cpp b/src/hotspot/share/ci/ciField.cpp
@@ -311,8 +311,7 @@ ciConstant ciField::constant_value() {
   }
   if (_constant_value.basic_type() == T_ILLEGAL) {
     // Static fields are placed in mirror objects.
-    VM_ENTRY_MARK;
-    ciInstance* mirror = CURRENT_ENV->get_instance(_holder->get_Klass()->java_mirror());
+    ciInstance* mirror = _holder->java_mirror();
     _constant_value = mirror->field_value_impl(type()->basic_type(), offset());
   }
   if (FoldStableValues && is_stable() && _constant_value.is_null_or_zero()) {
diff --git a/src/hotspot/share/ci/ciInstance.cpp b/src/hotspot/share/ci/ciInstance.cpp
@@ -28,6 +28,7 @@
 #include "ci/ciField.hpp"
 #include "ci/ciInstance.hpp"
 #include "ci/ciInstanceKlass.hpp"
+#include "ci/ciNullObject.hpp"
 #include "ci/ciUtilities.inline.hpp"
 #include "classfile/vmClasses.hpp"
 #include "oops/oop.inline.hpp"
@@ -59,17 +60,22 @@ ciType* ciInstance::java_mirror_type() {
 // ------------------------------------------------------------------
 // ciInstance::field_value_impl
 ciConstant ciInstance::field_value_impl(BasicType field_btype, int offset) {
+  ciConstant value = check_constant_value_cache(offset, field_btype);
+  if (value.is_valid()) {
+    return value;
+  }
+  VM_ENTRY_MARK;
   oop obj = get_oop();
   assert(obj != nullptr, "bad oop");
   switch(field_btype) {
-    case T_BYTE:    return ciConstant(field_btype, obj->byte_field(offset));
-    case T_CHAR:    return ciConstant(field_btype, obj->char_field(offset));
-    case T_SHORT:   return ciConstant(field_btype, obj->short_field(offset));
-    case T_BOOLEAN: return ciConstant(field_btype, obj->bool_field(offset));
-    case T_INT:     return ciConstant(field_btype, obj->int_field(offset));
-    case T_FLOAT:   return ciConstant(obj->float_field(offset));
-    case T_DOUBLE:  return ciConstant(obj->double_field(offset));
-    case T_LONG:    return ciConstant(obj->long_field(offset));
+    case T_BYTE:    value = ciConstant(field_btype, obj->byte_field(offset)); break;
+    case T_CHAR:    value = ciConstant(field_btype, obj->char_field(offset)); break;
+    case T_SHORT:   value = ciConstant(field_btype, obj->short_field(offset)); break;
+    case T_BOOLEAN: value = ciConstant(field_btype, obj->bool_field(offset)); break;
+    case T_INT:     value = ciConstant(field_btype, obj->int_field(offset)); break;
+    case T_FLOAT:   value = ciConstant(obj->float_field(offset)); break;
+    case T_DOUBLE:  value = ciConstant(obj->double_field(offset)); break;
+    case T_LONG:    value = ciConstant(obj->long_field(offset)); break;
     case T_OBJECT:  // fall through
     case T_ARRAY: {
       oop o = obj->obj_field(offset);
@@ -82,15 +88,17 @@ ciConstant ciInstance::field_value_impl(BasicType field_btype, int offset) {
       // information about the object's class (which is exact) or length.
 
       if (o == nullptr) {
-        return ciConstant(field_btype, ciNullObject::make());
+        value = ciConstant(field_btype, ciNullObject::make());
       } else {
-        return ciConstant(field_btype, CURRENT_ENV->get_object(o));
+        value = ciConstant(field_btype, CURRENT_ENV->get_object(o));
       }
+      break;
     }
     default:
       fatal("no field value: %s", type2name(field_btype));
-      return ciConstant();
   }
+  add_to_constant_value_cache(offset, value);
+  return value;
 }
 
 // ------------------------------------------------------------------
@@ -101,8 +109,7 @@ ciConstant ciInstance::field_value(ciField* field) {
   assert(is_loaded(), "invalid access - must be loaded");
   assert(field->holder()->is_loaded(), "invalid access - holder must be loaded");
   assert(field->is_static() || klass()->is_subclass_of(field->holder()), "invalid access - must be subclass");
-
-  GUARDED_VM_ENTRY(return field_value_impl(field->type()->basic_type(), field->offset());)
+  return field_value_impl(field->type()->basic_type(), field->offset());
 }
 
 // ------------------------------------------------------------------
diff --git a/src/hotspot/share/ci/ciObject.cpp b/src/hotspot/share/ci/ciObject.cpp
@@ -168,6 +168,38 @@ jobject ciObject::constant_encoding() {
   return handle();
 }
 
+// ------------------------------------------------------------------
+// ciObject::check_constant_value_cache()
+//
+// Cache constant value lookups to ensure that consistent values are observed
+// during compilation because fields may be (re-)initialized concurrently.
+ciConstant ciObject::check_constant_value_cache(int off, BasicType bt) {
+  if (_constant_values != nullptr) {
+    for (int i = 0; i < _constant_values->length(); ++i) {
+      ConstantValue cached_val = _constant_values->at(i);
+      if (cached_val.off() == off) {
+        assert(cached_val.value().basic_type() == bt, "unexpected type");
+        return cached_val.value();
+      }
+    }
+  }
+  return ciConstant();
+}
+
+// ------------------------------------------------------------------
+// ciObject::add_to_constant_value_cache()
+//
+// Add a constant value to the cache.
+void ciObject::add_to_constant_value_cache(int off, ciConstant val) {
+  assert(val.is_valid(), "value must be valid");
+  assert(!check_constant_value_cache(off, val.basic_type()).is_valid(), "duplicate");
+  if (_constant_values == nullptr) {
+    Arena* arena = CURRENT_ENV->arena();
+    _constant_values = new (arena) GrowableArray<ConstantValue>(arena, 1, 0, ConstantValue());
+  }
+  _constant_values->append(ConstantValue(off, val));
+}
+
 // ------------------------------------------------------------------
 // ciObject::should_be_constant()
 bool ciObject::should_be_constant() {
diff --git a/src/hotspot/share/ci/ciObject.hpp b/src/hotspot/share/ci/ciObject.hpp
@@ -27,7 +27,9 @@
 
 #include "ci/ciBaseObject.hpp"
 #include "ci/ciClassList.hpp"
+#include "ci/ciConstant.hpp"
 #include "runtime/handles.hpp"
+#include "utilities/growableArray.hpp"
 
 // ciObject
 //
@@ -57,6 +59,22 @@ class ciObject : public ciBaseObject {
   jobject  _handle;
   ciKlass* _klass;
 
+  // Cache constant value lookups to ensure that consistent values are observed during compilation.
+  class ConstantValue {
+    private:
+      ciConstant _value;
+      int _off;
+
+    public:
+      ConstantValue() : _value(ciConstant()), _off(0) { }
+      ConstantValue(int off, ciConstant value) : _value(value), _off(off) { }
+
+      int off() const { return _off; }
+      ciConstant value() const { return _value; }
+  };
+
+  GrowableArray<ConstantValue>* _constant_values = nullptr;
+
 protected:
   ciObject();
   ciObject(oop o);
@@ -94,6 +112,10 @@ class ciObject : public ciBaseObject {
   // be registered with the oopRecorder.
   jobject constant_encoding();
 
+  // Access to the constant value cache
+  ciConstant check_constant_value_cache(int off, BasicType bt);
+  void add_to_constant_value_cache(int off, ciConstant val);
+
   virtual bool is_object() const            { return true; }
 
   // What kind of ciObject is this?
diff --git a/src/hotspot/share/opto/phaseX.cpp b/src/hotspot/share/opto/phaseX.cpp
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1997, 2022, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1997, 2023, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -1759,19 +1759,15 @@ PhaseCCP::~PhaseCCP() {
 
 
 #ifdef ASSERT
-static bool ccp_type_widens(const Type* t, const Type* t0) {
-  assert(t->meet(t0) == t->remove_speculative(), "Not monotonic");
-  switch (t->base() == t0->base() ? t->base() : Type::Top) {
-  case Type::Int:
-    assert(t0->isa_int()->_widen <= t->isa_int()->_widen, "widen increases");
-    break;
-  case Type::Long:
-    assert(t0->isa_long()->_widen <= t->isa_long()->_widen, "widen increases");
-    break;
-  default:
-    break;
+void PhaseCCP::verify_type(Node* n, const Type* tnew, const Type* told) {
+  if (tnew->meet(told) != tnew->remove_speculative()) {
+    n->dump(1);
+    tty->print("told = "); told->dump(); tty->cr();
+    tty->print("tnew = "); tnew->dump(); tty->cr();
+    fatal("Not monotonic");
   }
-  return true;
+  assert(!told->isa_int() || !tnew->isa_int() || told->is_int()->_widen <= tnew->is_int()->_widen, "widen increases");
+  assert(!told->isa_long() || !tnew->isa_long() || told->is_long()->_widen <= tnew->is_long()->_widen, "widen increases");
 }
 #endif //ASSERT
 
@@ -1806,7 +1802,7 @@ void PhaseCCP::analyze() {
     }
     const Type* new_type = n->Value(this);
     if (new_type != type(n)) {
-      assert(ccp_type_widens(new_type, type(n)), "ccp type must widen");
+      DEBUG_ONLY(verify_type(n, new_type, type(n));)
       dump_type_and_node(n, new_type);
       set_type(n, new_type);
       push_child_nodes_to_worklist(worklist, n);
@@ -1834,12 +1830,13 @@ void PhaseCCP::verify_analyze(Unique_Node_List& worklist_verify) {
           continue; // ignore integer widen
         }
       }
-      if (n->is_Load()) {
+      if (n->is_Load() && !told->singleton()) {
         // MemNode::can_see_stored_value looks up through many memory nodes,
         // which means we would need to notify modifications from far up in
         // the inputs all the way down to the LoadNode. We don't do that.
         continue;
       }
+      verify_type(n, tnew, told);
       tty->cr();
       tty->print_cr("Missed optimization (PhaseCCP):");
       n->dump_bfs(1, 0, "");
diff --git a/src/hotspot/share/opto/phaseX.hpp b/src/hotspot/share/opto/phaseX.hpp
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1997, 2021, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1997, 2023, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -605,6 +605,7 @@ class PhaseCCP : public PhaseIterGVN {
   // Worklist algorithm identifies constants
   void analyze();
 #ifdef ASSERT
+  void verify_type(Node* n, const Type* tnew, const Type* told);
   // For every node n on verify list, check if type(n) == n->Value()
   void verify_analyze(Unique_Node_List& worklist_verify);
 #endif
diff --git a/src/hotspot/share/opto/type.cpp b/src/hotspot/share/opto/type.cpp
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1997, 2022, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1997, 2023, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -3370,7 +3370,6 @@ TypeOopPtr::TypeOopPtr(TYPES t, PTR ptr, ciKlass* k, const InterfaceSet& interfa
                              _offset != arrayOopDesc::length_offset_in_bytes());
     } else if (klass()->is_instance_klass()) {
       ciInstanceKlass* ik = klass()->as_instance_klass();
-      ciField* field = NULL;
       if (this->isa_klassptr()) {
         // Perm objects don't use compressed references
       } else if (_offset == OffsetBot || _offset == OffsetTop) {
@@ -3402,7 +3401,7 @@ TypeOopPtr::TypeOopPtr(TYPES t, PTR ptr, ciKlass* k, const InterfaceSet& interfa
           }
         } else {
           // Instance fields which contains a compressed oop references.
-          field = ik->get_field_by_offset(_offset, false);
+          ciField* field = ik->get_field_by_offset(_offset, false);
           if (field != NULL) {
             BasicType basic_elem_type = field->layout_type();
             _is_ptr_to_narrowoop = UseCompressedOops && ::is_reference_type(basic_elem_type);
diff --git a/test/hotspot/jtreg/compiler/stable/TestUnstableStable.java b/test/hotspot/jtreg/compiler/stable/TestUnstableStable.java

Original file line number	Diff line number	Diff line change
`@@ -311,8 +311,7 @@ ciConstant ciField::constant_value() {`
`311`	`311`	`}`
`312`	`312`	`if (_constant_value.basic_type() == T_ILLEGAL) {`
`313`	`313`	`// Static fields are placed in mirror objects.`
`314`		`- VM_ENTRY_MARK;`
`315`		`- ciInstance* mirror = CURRENT_ENV->get_instance(_holder->get_Klass()->java_mirror());`
	`314`	`+ ciInstance* mirror = _holder->java_mirror();`
`316`	`315`	`_constant_value = mirror->field_value_impl(type()->basic_type(), offset());`
`317`	`316`	`}`
`318`	`317`	`if (FoldStableValues && is_stable() && _constant_value.is_null_or_zero()) {`