Skip to content
This repository has been archived by the owner on May 29, 2024. It is now read-only.

Commit

Permalink
[GR-34646] Avoid unaligned volatile reads in c2v_readFieldValue.
Browse files Browse the repository at this point in the history 
PullRequest: labsjdk-ce-17/12
  • Loading branch information
@dougxc
dougxc committed Oct 22, 2021
2 parents b8b2daf + 7ea09ea commit 4f5b4a3
Show file tree
Hide file tree
Showing 6 changed files with 62 additions and 22 deletions.
45 changes: 35 additions & 10 deletions src/hotspot/share/jvmci/jvmciCompilerToVM.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1891,7 +1891,20 @@ C2V_VMENTRY_NULL(jobjectArray, getDeclaredMethods, (JNIEnv* env, jobject, jobjec
return JVMCIENV->get_jobjectArray(methods);
C2V_END

C2V_VMENTRY_NULL(jobject, readFieldValue, (JNIEnv* env, jobject, jobject object, jobject expected_type, long displacement, jboolean is_volatile, jobject kind_object))
// Enforces volatile semantics for a non-volatile read.
class VolatileRead : public StackObj {
public:
VolatileRead() {
// Ensures a possibly volatile read is not reordered with a prior
// volatile write.
OrderAccess::storeload();
}
~VolatileRead() {
OrderAccess::acquire();
}
};

C2V_VMENTRY_NULL(jobject, readFieldValue, (JNIEnv* env, jobject, jobject object, jobject expected_type, long displacement, jobject kind_object))
if (object == NULL || kind_object == NULL) {
JVMCI_THROW_0(NullPointerException);
}
Expand Down Expand Up @@ -1975,15 +1988,25 @@ C2V_VMENTRY_NULL(jobject, readFieldValue, (JNIEnv* env, jobject, jobject object,
}

jlong value = 0;

// Treat all reads as volatile for simplicity as this function can be used
// both for reading Java fields declared as volatile as well as for constant
// folding Unsafe.get* methods with volatile semantics. This is done by
// performing the volatile barrier operations around a call to an
// oopDesc::<kind>_field method. The oopDesc::<kind>_field_acquire method
// cannot be used since it does not support unaligned reads on all platforms
// (e.g., an unaligned ldar on AArch64 causes a SIGBUS).


switch (basic_type) {
case T_BOOLEAN: value = is_volatile ? obj->bool_field_acquire(displacement) : obj->bool_field(displacement); break;
case T_BYTE: value = is_volatile ? obj->byte_field_acquire(displacement) : obj->byte_field(displacement); break;
case T_SHORT: value = is_volatile ? obj->short_field_acquire(displacement) : obj->short_field(displacement); break;
case T_CHAR: value = is_volatile ? obj->char_field_acquire(displacement) : obj->char_field(displacement); break;
case T_BOOLEAN: { VolatileRead vr; value = obj->bool_field(displacement); } break;
case T_BYTE: { VolatileRead vr; value = obj->byte_field(displacement); } break;
case T_SHORT: { VolatileRead vr; value = obj->short_field(displacement);} break;
case T_CHAR: { VolatileRead vr; value = obj->char_field(displacement); } break;
case T_FLOAT:
case T_INT: value = is_volatile ? obj->int_field_acquire(displacement) : obj->int_field(displacement); break;
case T_INT: { VolatileRead vr; value = obj->int_field(displacement); } break;
case T_DOUBLE:
case T_LONG: value = is_volatile ? obj->long_field_acquire(displacement) : obj->long_field(displacement); break;
case T_LONG: { VolatileRead vr; value = obj->long_field(displacement); } break;

case T_OBJECT: {
if (displacement == java_lang_Class::component_mirror_offset() && java_lang_Class::is_instance(obj()) &&
Expand All @@ -1993,7 +2016,9 @@ C2V_VMENTRY_NULL(jobject, readFieldValue, (JNIEnv* env, jobject, jobject object,
return JVMCIENV->get_jobject(JVMCIENV->get_JavaConstant_NULL_POINTER());
}

oop value = is_volatile ? obj->obj_field_acquire(displacement) : obj->obj_field(displacement);
oop value;
{ VolatileRead vr; value = obj->obj_field(displacement); }

if (value == NULL) {
return JVMCIENV->get_jobject(JVMCIENV->get_JavaConstant_NULL_POINTER());
} else {
Expand Down Expand Up @@ -2782,8 +2807,8 @@ JNINativeMethod CompilerToVM::methods[] = {
{CC "boxPrimitive", CC "(" OBJECT ")" OBJECTCONSTANT, FN_PTR(boxPrimitive)},
{CC "getDeclaredConstructors", CC "(" HS_RESOLVED_KLASS ")[" RESOLVED_METHOD, FN_PTR(getDeclaredConstructors)},
{CC "getDeclaredMethods", CC "(" HS_RESOLVED_KLASS ")[" RESOLVED_METHOD, FN_PTR(getDeclaredMethods)},
{CC "readFieldValue", CC "(" HS_RESOLVED_KLASS HS_RESOLVED_KLASS "JZLjdk/vm/ci/meta/JavaKind;)" JAVACONSTANT, FN_PTR(readFieldValue)},
{CC "readFieldValue", CC "(" OBJECTCONSTANT HS_RESOLVED_KLASS "JZLjdk/vm/ci/meta/JavaKind;)" JAVACONSTANT, FN_PTR(readFieldValue)},
{CC "readFieldValue", CC "(" HS_RESOLVED_KLASS HS_RESOLVED_KLASS "JLjdk/vm/ci/meta/JavaKind;)" JAVACONSTANT, FN_PTR(readFieldValue)},
{CC "readFieldValue", CC "(" OBJECTCONSTANT HS_RESOLVED_KLASS "JLjdk/vm/ci/meta/JavaKind;)" JAVACONSTANT, FN_PTR(readFieldValue)},
{CC "isInstance", CC "(" HS_RESOLVED_KLASS OBJECTCONSTANT ")Z", FN_PTR(isInstance)},
{CC "isAssignableFrom", CC "(" HS_RESOLVED_KLASS HS_RESOLVED_KLASS ")Z", FN_PTR(isAssignableFrom)},
{CC "isTrustedForIntrinsics", CC "(" HS_RESOLVED_KLASS ")Z", FN_PTR(isTrustedForIntrinsics)},
Expand Down
4 changes: 2 additions & 2 deletions ...dk.internal.vm.ci/share/classes/jdk.vm.ci.hotspot/src/jdk/vm/ci/hotspot/CompilerToVM.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -786,14 +786,14 @@ HotSpotResolvedObjectTypeImpl getResolvedJavaType(long displacement, boolean com
* object is exptected to be a subtype of {@code expectedType} and extra sanity checking is
* performed on the offset and kind of the read being performed.
*/
native JavaConstant readFieldValue(HotSpotResolvedObjectTypeImpl object, HotSpotResolvedObjectTypeImpl expectedType, long offset, boolean isVolatile, JavaKind kind);
native JavaConstant readFieldValue(HotSpotResolvedObjectTypeImpl object, HotSpotResolvedObjectTypeImpl expectedType, long offset, JavaKind kind);

/**
* Reads the current value of an instance field. If {@code expectedType} is non-null, then the
* object is exptected to be a subtype of {@code expectedType} and extra sanity checking is
* performed on the offset and kind of the read being performed.
*/
native JavaConstant readFieldValue(HotSpotObjectConstantImpl object, HotSpotResolvedObjectTypeImpl expectedType, long offset, boolean isVolatile, JavaKind kind);
native JavaConstant readFieldValue(HotSpotObjectConstantImpl object, HotSpotResolvedObjectTypeImpl expectedType, long offset, JavaKind kind);

/**
* @see ResolvedJavaType#isInstance(JavaConstant)
Expand Down
4 changes: 2 additions & 2 deletions ...re/classes/jdk.vm.ci.hotspot/src/jdk/vm/ci/hotspot/HotSpotConstantReflectionProvider.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -166,11 +166,11 @@ public JavaConstant readFieldValue(ResolvedJavaField field, JavaConstant receive
if (hotspotField.isStatic()) {
HotSpotResolvedObjectTypeImpl holder = (HotSpotResolvedObjectTypeImpl) hotspotField.getDeclaringClass();
if (holder.isInitialized()) {
return runtime().compilerToVm.readFieldValue(holder, (HotSpotResolvedObjectTypeImpl) hotspotField.getDeclaringClass(), hotspotField.getOffset(), field.isVolatile(),
return runtime().compilerToVm.readFieldValue(holder, (HotSpotResolvedObjectTypeImpl) hotspotField.getDeclaringClass(), hotspotField.getOffset(),
hotspotField.getType().getJavaKind());
}
} else if (receiver instanceof HotSpotObjectConstantImpl) {
return ((HotSpotObjectConstantImpl) receiver).readFieldValue(hotspotField, field.isVolatile());
return ((HotSpotObjectConstantImpl) receiver).readFieldValue(hotspotField);
} else if (receiver == null) {
throw new NullPointerException("receiver is null");
}
Expand Down
8 changes: 4 additions & 4 deletions ...hare/classes/jdk.vm.ci.hotspot/src/jdk/vm/ci/hotspot/HotSpotMemoryAccessProviderImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,8 @@ public JavaConstant readPrimitiveConstant(JavaKind kind, Constant baseConstant,
throw new IllegalArgumentException(String.valueOf(bits));
}
}
JavaConstant result = runtime().compilerToVm.readFieldValue((HotSpotObjectConstantImpl) baseConstant, null, initialDisplacement, true, readKind);
HotSpotObjectConstantImpl baseObject = (HotSpotObjectConstantImpl) baseConstant;
JavaConstant result = runtime().compilerToVm.readFieldValue(baseObject, null, initialDisplacement, readKind);
if (result != null && kind != readKind) {
return JavaConstant.forPrimitive(kind, result.asLong());
}
Expand Down Expand Up @@ -108,7 +109,7 @@ public JavaConstant readPrimitiveConstant(JavaKind kind, Constant baseConstant,
@Override
public JavaConstant readObjectConstant(Constant base, long displacement) {
if (base instanceof HotSpotObjectConstantImpl) {
return runtime.getCompilerToVM().readFieldValue((HotSpotObjectConstantImpl) base, null, displacement, true, JavaKind.Object);
return runtime.getCompilerToVM().readFieldValue((HotSpotObjectConstantImpl) base, null, displacement, JavaKind.Object);
}
if (base instanceof HotSpotMetaspaceConstant) {
MetaspaceObject metaspaceObject = HotSpotMetaspaceConstantImpl.getMetaspaceObject(base);
Expand All @@ -130,7 +131,7 @@ public JavaConstant readObjectConstant(Constant base, long displacement) {
public JavaConstant readNarrowOopConstant(Constant base, long displacement) {
if (base instanceof HotSpotObjectConstantImpl) {
assert runtime.getConfig().useCompressedOops;
JavaConstant res = runtime.getCompilerToVM().readFieldValue((HotSpotObjectConstantImpl) base, null, displacement, true, JavaKind.Object);
JavaConstant res = runtime.getCompilerToVM().readFieldValue((HotSpotObjectConstantImpl) base, null, displacement, JavaKind.Object);
if (res != null) {
return JavaConstant.NULL_POINTER.equals(res) ? HotSpotCompressedNullConstant.COMPRESSED_NULL : ((HotSpotObjectConstant) res).compress();
}
Expand All @@ -147,7 +148,6 @@ private HotSpotResolvedObjectTypeImpl readKlass(Constant base, long displacement
}
}


@Override
public Constant readKlassPointerConstant(Constant base, long displacement) {
HotSpotResolvedObjectTypeImpl klass = readKlass(base, displacement, false);
Expand Down
8 changes: 4 additions & 4 deletions ...m.ci/share/classes/jdk.vm.ci.hotspot/src/jdk/vm/ci/hotspot/HotSpotObjectConstantImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,15 +72,15 @@ private boolean isFullyInitializedConstantCallSite() {
}
// read ConstantCallSite.isFrozen as a volatile field
HotSpotResolvedJavaField field = HotSpotMethodHandleAccessProvider.Internals.instance().constantCallSiteFrozenField;
boolean isFrozen = readFieldValue(field, true /* volatile */).asBoolean();
boolean isFrozen = readFieldValue(field).asBoolean();
// isFrozen true implies fully-initialized
return isFrozen;
}

private HotSpotObjectConstantImpl readTarget() {
// read CallSite.target as a volatile field
HotSpotResolvedJavaField field = HotSpotMethodHandleAccessProvider.Internals.instance().callSiteTargetField;
return (HotSpotObjectConstantImpl) readFieldValue(field, true /* volatile */);
return (HotSpotObjectConstantImpl) readFieldValue(field);
}

@Override
Expand Down Expand Up @@ -184,7 +184,7 @@ public String toString() {
return (compressed ? "NarrowOop" : getJavaKind().getJavaName()) + "[" + runtime().reflection.formatString(this) + "]";
}

public JavaConstant readFieldValue(HotSpotResolvedJavaField field, boolean isVolatile) {
public JavaConstant readFieldValue(HotSpotResolvedJavaField field) {
if (IS_IN_NATIVE_IMAGE && this instanceof DirectHotSpotObjectConstantImpl) {
// cannot read fields from objects due to lack of
// general reflection support in native image
Expand All @@ -193,7 +193,7 @@ public JavaConstant readFieldValue(HotSpotResolvedJavaField field, boolean isVol
if (field.isStatic()) {
return null;
}
return runtime().compilerToVm.readFieldValue(this, (HotSpotResolvedObjectTypeImpl) field.getDeclaringClass(), field.getOffset(), isVolatile, field.getType().getJavaKind());
return runtime().compilerToVm.readFieldValue(this, (HotSpotResolvedObjectTypeImpl) field.getDeclaringClass(), field.getOffset(), field.getType().getJavaKind());
}

public ResolvedJavaType asJavaType() {
Expand Down
15 changes: 15 additions & 0 deletions ...ler/jvmci/jdk.vm.ci.hotspot.test/src/jdk/vm/ci/hotspot/test/MemoryAccessProviderData.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,9 @@ public static Object[][] getPositivePrimitiveJavaKinds() {
for (KindData k : PRIMITIVE_KIND_DATA) {
result.add(new Object[] {k.kind, TEST_CONSTANT, k.instanceFieldOffset, k.instanceFieldValue, Math.max(8, k.kind.getBitCount())});
result.add(new Object[] {k.kind, TEST_CLASS_CONSTANT, k.staticFieldOffset, k.staticFieldValue, Math.max(8, k.kind.getBitCount())});
if (k.unalignedInstanceFieldValue != null) {
result.add(new Object[] {k.kind, TEST_CONSTANT, k.instanceFieldOffset - 1, k.unalignedInstanceFieldValue, Math.max(8, k.kind.getBitCount())});
}
}
return result.toArray(new Object[result.size()][]);
}
Expand Down Expand Up @@ -170,6 +173,7 @@ static class KindData {
final long staticFieldOffset;
final JavaConstant instanceFieldValue;
final JavaConstant staticFieldValue;
final JavaConstant unalignedInstanceFieldValue;
KindData(JavaKind kind, Object testObject) {
this.kind = kind;
try {
Expand All @@ -182,6 +186,17 @@ static class KindData {
staticFieldOffset = UNSAFE.staticFieldOffset(staticField);
instanceFieldValue = JavaConstant.forBoxedPrimitive(instanceField.get(testObject));
staticFieldValue = JavaConstant.forBoxedPrimitive(staticField.get(null));
if (kind == JavaKind.Long) {
unalignedInstanceFieldValue = JavaConstant.forLong(UNSAFE.getLongUnaligned(testObject, instanceFieldOffset - 1));
} else if (kind == JavaKind.Int) {
unalignedInstanceFieldValue = JavaConstant.forInt(UNSAFE.getIntUnaligned(testObject, instanceFieldOffset - 1));
} else if (kind == JavaKind.Char) {
unalignedInstanceFieldValue = JavaConstant.forChar(UNSAFE.getCharUnaligned(testObject, instanceFieldOffset - 1));
} else if (kind == JavaKind.Short) {
unalignedInstanceFieldValue = JavaConstant.forShort(UNSAFE.getShortUnaligned(testObject, instanceFieldOffset - 1));
} else {
unalignedInstanceFieldValue = null;
}
} catch (Exception e) {
throw new Error("TESTBUG for kind " + kind, e);
}
Expand Down

0 comments on commit 4f5b4a3

Please sign in to comment.