Skip to content

8275645: [JVMCI] avoid unaligned volatile reads on AArch64 #6044

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
dougxc wants to merge 2 commits into from
Closed

8275645: [JVMCI] avoid unaligned volatile reads on AArch64 #6044

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f71836c
expanded MemoryAccessProviderTest to test unaligned reads
dougxc Oct 20, 2021
2accdf7
avoid unaligned volatile reads in c2v_readFieldValue
dougxc Oct 20, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 35 additions & 10 deletions src/hotspot/share/jvmci/jvmciCompilerToVM.cpp
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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