stan/jdk-24
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

8295486: Inconsistent constant field values observed during compilation

Browse Source 
Reviewed-by: chagedorn, kvn, jbhateja, vlivanov
This commit is contained in:
Tobias Hartmann 2023-02-01 11:15:35 +00:00
parent 969f6a37e4
commit cae577a710
11 changed files with 299 additions and 64 deletions

12
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;
}
// ------------------------------------------------------------------

31
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() {

30
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 @@ public:
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();
};

3
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()) {

33
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());
}
// ------------------------------------------------------------------

32
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() {

22
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 @@ private:
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 @@ public:
// 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?

27
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, "");

3
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

5
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);

165
test/hotspot/jtreg/compiler/stable/TestUnstableStable.java Normal file

@ -0,0 +1,165 @@
/*
* Copyright (c) 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
* under the terms of the GNU General Public License version 2 only, as
* published by the Free Software Foundation.
*
* This code is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
* version 2 for more details (a copy is included in the LICENSE file that
* accompanied this code).
*
* You should have received a copy of the GNU General Public License version
* 2 along with this work; if not, write to the Free Software Foundation,
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
*
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
* or visit www.oracle.com if you need additional information or have any
* questions.
*/
/*
* @test
* @bug 8295486
* @summary Verify that constant folding of field loads observes consistent values during compilation.
* @library /test/lib /
* @modules java.base/jdk.internal.vm.annotation
* java.base/jdk.internal.misc
* @build jdk.test.whitebox.WhiteBox
* @run driver jdk.test.lib.helpers.ClassFileInstaller jdk.test.whitebox.WhiteBox
* @run main/bootclasspath/othervm -Xbootclasspath/a:. -XX:+UnlockDiagnosticVMOptions -XX:+WhiteBoxAPI
* -XX:-TieredCompilation -Xbatch -XX:PerMethodRecompilationCutoff=-1
* -XX:CompileCommand=compileonly,compiler.stable.TestUnstableStable::test*
* compiler.stable.TestUnstableStable
*/
package compiler.stable;
import compiler.whitebox.CompilerWhiteBoxTest;
import java.lang.reflect.Method;
import jdk.internal.misc.Unsafe;
import jdk.internal.vm.annotation.Stable;
import jdk.test.whitebox.WhiteBox;
public class TestUnstableStable {
static final Unsafe U = Unsafe.getUnsafe();
static final WhiteBox WHITE_BOX = WhiteBox.getWhiteBox();
static final TestUnstableStable HOLDER = new TestUnstableStable();
@Stable Integer stableField = null;
static @Stable Integer staticStableField = null;
static @Stable Integer[] stableArray0 = new Integer[1];
static @Stable Integer[][] stableArray1 = new Integer[1][1];
static final Integer finalField = 43;
static final long FIELD_OFFSET;
static {
try {
FIELD_OFFSET = U.staticFieldOffset(TestUnstableStable.class.getDeclaredField("finalField"));
} catch (NoSuchFieldException e) {
throw new RuntimeException("Field not found", e);
}
}
static class Writer implements Runnable {
public void run() {
while (true) {
HOLDER.stableField = null;
HOLDER.stableField = 42;
HOLDER.stableField = 43;
staticStableField = null;
staticStableField = 42;
staticStableField = 43;
stableArray0[0] = null;
stableArray0[0] = 42;
stableArray0[0] = 43;
stableArray1[0] = null;
Integer[] tmp1 = {null};
stableArray1[0] = tmp1;
Integer[] tmp2 = {42};
stableArray1[0] = tmp2;
Integer[] tmp3 = {43};
stableArray1[0] = tmp3;
stableArray1[0][0] = null;
stableArray1[0][0] = 42;
stableArray1[0][0] = 43;
U.putObject(TestUnstableStable.class, FIELD_OFFSET, null);
U.putObject(TestUnstableStable.class, FIELD_OFFSET, 42);
U.putObject(TestUnstableStable.class, FIELD_OFFSET, 43);
}
}
}
static Object testNonStatic() {
// Trigger PhaseCCP and LoadNode::Value -> Type::make_constant_from_field
// which may observe different values of the stable field when invoked twice.
Integer val = HOLDER.stableField;
if (val == null) {
val = null;
}
return val;
}
static Object testStatic() {
Integer val = staticStableField;
if (val == null) {
val = null;
}
return val;
}
static Object testArray0() {
Integer val = stableArray0[0];
if (val == null) {
val = null;
}
return val;
}
static Object testArray1() {
Integer[] val = stableArray1[0];
if (val == null) {
val = null;
} else {
return val[0];
}
return val;
}
static Object testFinal() {
Integer val = finalField;
if (val == null) {
val = null;
}
return val;
}
public static void main(String[] args) throws Exception {
Thread t = new Thread(new Writer());
t.start();
Method testNonStatic = TestUnstableStable.class.getDeclaredMethod("testNonStatic");
Method testStatic = TestUnstableStable.class.getDeclaredMethod("testStatic");
Method testArray0 = TestUnstableStable.class.getDeclaredMethod("testArray0");
Method testArray1 = TestUnstableStable.class.getDeclaredMethod("testArray1");
Method testFinal = TestUnstableStable.class.getDeclaredMethod("testFinal");
testFinal();
for (int i = 0; i < 1000; ++i) {
WHITE_BOX.deoptimizeMethod(testNonStatic, false);
WHITE_BOX.enqueueMethodForCompilation(testNonStatic, CompilerWhiteBoxTest.COMP_LEVEL_FULL_OPTIMIZATION);
WHITE_BOX.deoptimizeMethod(testStatic, false);
WHITE_BOX.enqueueMethodForCompilation(testStatic, CompilerWhiteBoxTest.COMP_LEVEL_FULL_OPTIMIZATION);
WHITE_BOX.deoptimizeMethod(testArray0, false);
WHITE_BOX.enqueueMethodForCompilation(testArray0, CompilerWhiteBoxTest.COMP_LEVEL_FULL_OPTIMIZATION);
WHITE_BOX.deoptimizeMethod(testArray1, false);
WHITE_BOX.enqueueMethodForCompilation(testArray1, CompilerWhiteBoxTest.COMP_LEVEL_FULL_OPTIMIZATION);
WHITE_BOX.deoptimizeMethod(testFinal, false);
WHITE_BOX.enqueueMethodForCompilation(testFinal, CompilerWhiteBoxTest.COMP_LEVEL_FULL_OPTIMIZATION);
}
}
}