Skip to content

[GR-47886] Directly patch LoadMethod Constants. #7566

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

Merged
merged 2 commits into from
Oct 9, 2023
Merged

[GR-47886] Directly patch LoadMethod Constants. #7566

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
cb22258
Revert "[GR-43389] [darwin-amd64] Workaround for buggy ld64 versions"
teshull Oct 5, 2023
946090a
use pc-relative instructions for loading SubstrateMethodPointerConstants
teshull Oct 5, 2023
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
12 changes: 2 additions & 10 deletions ...ore.graal.amd64/src/com/oracle/svm/core/graal/amd64/AMD64LoadMethodPointerConstantOp.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,6 @@
import static org.graalvm.compiler.lir.LIRInstruction.OperandFlag.HINT;
import static org.graalvm.compiler.lir.LIRInstruction.OperandFlag.REG;

import com.oracle.svm.core.FrameAccess;
import org.graalvm.compiler.asm.amd64.AMD64Address;
import org.graalvm.compiler.asm.amd64.AMD64MacroAssembler;
import org.graalvm.compiler.lir.LIRInstructionClass;
import org.graalvm.compiler.lir.StandardOp;
Expand All @@ -40,7 +38,6 @@

import jdk.vm.ci.code.Register;
import jdk.vm.ci.meta.AllocatableValue;
import org.graalvm.nativeimage.Platform;

public final class AMD64LoadMethodPointerConstantOp extends AMD64LIRInstruction implements StandardOp.LoadConstantOp {
public static final LIRInstructionClass<AMD64LoadMethodPointerConstantOp> TYPE = LIRInstructionClass.create(AMD64LoadMethodPointerConstantOp.class);
Expand All @@ -56,13 +53,8 @@ public final class AMD64LoadMethodPointerConstantOp extends AMD64LIRInstruction
@Override
public void emitCode(CompilationResultBuilder crb, AMD64MacroAssembler masm) {
Register resultReg = asRegister(result);
if (!Platform.includedIn(Platform.DARWIN_AMD64.class)) {
crb.recordInlineDataInCode(constant);
masm.movq(resultReg, 0L, true);
} else {
/* [GR-43389] ld64 bug does not allow direct8 relocations in .text on darwin */
masm.movq(resultReg, (AMD64Address) crb.recordDataReferenceInCode(constant, FrameAccess.wordSize()));
}
crb.recordInlineDataInCode(constant);
masm.leaq(resultReg, masm.getPlaceholder(masm.position()));
}

@Override
Expand Down
27 changes: 10 additions & 17 deletions ...tevm/src/com.oracle.svm.core/src/com/oracle/svm/core/graal/code/SubstrateDataBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@

import java.nio.ByteBuffer;

import com.oracle.svm.core.meta.SubstrateMethodPointerConstant;
import org.graalvm.compiler.code.DataSection.Data;
import org.graalvm.compiler.code.DataSection.Patches;
import org.graalvm.compiler.core.common.type.CompressibleConstant;
Expand All @@ -51,11 +50,8 @@ public class SubstrateDataBuilder extends DataBuilder {
@Override
public Data createDataItem(Constant constant) {
int size;
if (constant instanceof SubstrateMethodPointerConstant methodPointerConstant) {
size = FrameAccess.wordSize();
return new ObjectData(size, size, methodPointerConstant);
} else if (constant instanceof VMConstant vmConstant) {
assert constant instanceof CompressibleConstant && constant instanceof TypedConstant : constant;
if (constant instanceof VMConstant vmConstant) {
assert constant instanceof JavaConstant && constant instanceof CompressibleConstant && constant instanceof TypedConstant : constant;
return new ObjectData(vmConstant);
} else if (JavaConstant.isNull(constant)) {
if (SubstrateObjectConstant.isCompressed((JavaConstant) constant)) {
Expand All @@ -64,8 +60,9 @@ public Data createDataItem(Constant constant) {
size = FrameAccess.uncompressedReferenceSize();
}
return createZeroData(size, size);
} else if (constant instanceof SerializableConstant serializableConstant) {
return createSerializableData(serializableConstant);
} else if (constant instanceof SerializableConstant) {
SerializableConstant s = (SerializableConstant) constant;
return createSerializableData(s);
} else {
throw new JVMCIError(String.valueOf(constant));
}
Expand All @@ -74,19 +71,15 @@ public Data createDataItem(Constant constant) {
public static class ObjectData extends Data {
private final VMConstant constant;

protected ObjectData(int alignment, int size, VMConstant constant) {
super(alignment, size);
this.constant = constant;
}

protected ObjectData(VMConstant constant) {
this(ConfigurationValues.getObjectLayout().getReferenceSize(), ConfigurationValues.getObjectLayout().getReferenceSize(), constant);
super(ConfigurationValues.getObjectLayout().getReferenceSize(), ConfigurationValues.getObjectLayout().getReferenceSize());
assert ((CompressibleConstant) constant).isCompressed() == ReferenceAccess.singleton()
.haveCompressedReferences() : "Constant object references in compiled code must be compressed (base-relative)";
this.constant = constant;
}

public VMConstant getConstant() {
return constant;
public JavaConstant getConstant() {
return (JavaConstant) constant;
}

@Override
Expand All @@ -99,7 +92,7 @@ public static void emit(ByteBuffer buffer, Patches patches, int size, VMConstant
if (size == Integer.BYTES) {
buffer.putInt(0);
} else if (size == Long.BYTES) {
buffer.putLong(0);
buffer.putLong(0L);
} else {
shouldNotReachHere("Unsupported object constant reference size: " + size);
}
Expand Down
29 changes: 7 additions & 22 deletions substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/code/CompileQueue.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@
import java.util.concurrent.ConcurrentMap;
import java.util.concurrent.ForkJoinPool;

import com.oracle.svm.core.graal.code.SubstrateDataBuilder;
import org.graalvm.collections.EconomicMap;
import org.graalvm.compiler.api.replacements.Fold;
import org.graalvm.compiler.api.replacements.SnippetReflectionProvider;
Expand Down Expand Up @@ -123,6 +122,7 @@
import com.oracle.svm.core.graal.phases.OptimizeExceptionPathsPhase;
import com.oracle.svm.core.heap.RestrictHeapAccess;
import com.oracle.svm.core.heap.RestrictHeapAccessCallees;
import com.oracle.svm.core.meta.MethodPointer;
import com.oracle.svm.core.meta.SubstrateMethodPointerConstant;
import com.oracle.svm.core.util.InterruptImageBuilding;
import com.oracle.svm.core.util.VMError;
Expand Down Expand Up @@ -151,7 +151,6 @@
import jdk.vm.ci.meta.MetaAccessProvider;
import jdk.vm.ci.meta.ResolvedJavaMethod;
import jdk.vm.ci.meta.VMConstant;
import org.graalvm.nativeimage.Platform;

public class CompileQueue {

Expand Down Expand Up @@ -1369,29 +1368,15 @@ protected void removeDeoptTargetOptimizations(LIRSuites lirSuites) {
DeoptimizationUtils.removeDeoptTargetOptimizations(lirSuites);
}

private void ensureCompiledForMethodPointerConstant(HostedMethod method, CompileReason reason, SubstrateMethodPointerConstant methodPointerConstant) {
HostedMethod referencedMethod = (HostedMethod) methodPointerConstant.pointer().getMethod();
ensureCompiled(referencedMethod, new MethodPointerConstantReason(method, referencedMethod, reason));
}

protected final void ensureCompiledForMethodPointerConstants(HostedMethod method, CompileReason reason, CompilationResult result) {
for (DataPatch dataPatch : result.getDataPatches()) {
Reference reference = dataPatch.reference;
if (reference instanceof ConstantReference constantReference) {
VMConstant vmConstant = constantReference.getConstant();
if (vmConstant instanceof SubstrateMethodPointerConstant methodPointerConstant) {
ensureCompiledForMethodPointerConstant(method, reason, methodPointerConstant);
}
}
}

for (DataSection.Data data : result.getDataSection()) {
if (data instanceof SubstrateDataBuilder.ObjectData objectData) {
VMConstant vmConstant = objectData.getConstant();
if (vmConstant instanceof SubstrateMethodPointerConstant methodPointerConstant) {
/* [GR-43389] Only reachable with ld64 workaround on */
VMError.guarantee(Platform.includedIn(Platform.DARWIN_AMD64.class));
ensureCompiledForMethodPointerConstant(method, reason, methodPointerConstant);
if (reference instanceof ConstantReference) {
VMConstant constant = ((ConstantReference) reference).getConstant();
if (constant instanceof SubstrateMethodPointerConstant) {
MethodPointer pointer = ((SubstrateMethodPointerConstant) constant).pointer();
HostedMethod referencedMethod = (HostedMethod) pointer.getMethod();
ensureCompiled(referencedMethod, new MethodPointerConstantReason(method, referencedMethod, reason));
}
}
}
Expand Down
24 changes: 8 additions & 16 deletions ...oracle.svm.hosted/src/com/oracle/svm/hosted/code/aarch64/AArch64HostedPatcherFeature.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,17 +41,14 @@
import com.oracle.svm.core.feature.InternalFeature;
import com.oracle.svm.core.graal.code.CGlobalDataReference;
import com.oracle.svm.core.graal.code.PatchConsumerFactory;
import com.oracle.svm.core.meta.MethodPointer;
import com.oracle.svm.core.meta.SubstrateMethodPointerConstant;
import com.oracle.svm.core.util.VMError;
import com.oracle.svm.hosted.code.HostedPatcher;
import com.oracle.svm.hosted.image.RelocatableBuffer;
import com.oracle.svm.hosted.meta.HostedMethod;

import jdk.vm.ci.code.site.ConstantReference;
import jdk.vm.ci.code.site.DataSectionReference;
import jdk.vm.ci.code.site.Reference;
import jdk.vm.ci.meta.VMConstant;

@AutomaticallyRegisteredFeature
@Platforms({Platform.AARCH64.class})
Expand Down Expand Up @@ -172,22 +169,17 @@ class AdrpAddMacroInstructionHostedPatcher extends CompilationResult.CodeAnnotat

@Override
public void relocate(Reference ref, RelocatableBuffer relocs, int compStart) {
Object relocVal = ref;
if (ref instanceof ConstantReference) {
VMConstant constant = ((ConstantReference) ref).getConstant();
if (constant instanceof SubstrateMethodPointerConstant) {
MethodPointer pointer = ((SubstrateMethodPointerConstant) constant).pointer();
HostedMethod hMethod = (HostedMethod) pointer.getMethod();
VMError.guarantee(hMethod.isCompiled(), "Method %s is not compiled although there is a method pointer constant created for it.", hMethod);
relocVal = pointer;
}
if (ref instanceof ConstantReference constantRef) {
VMError.guarantee(!(constantRef.getConstant() instanceof SubstrateMethodPointerConstant), "SubstrateMethodPointerConstants should not be relocated %s", constantRef);
} else {
VMError.guarantee(ref instanceof DataSectionReference || ref instanceof CGlobalDataReference, "Unexpected reference: %s", ref);
}

int siteOffset = compStart + macroInstruction.instructionPosition;
relocs.addRelocationWithoutAddend(siteOffset, RelocationKind.AARCH64_R_AARCH64_ADR_PREL_PG_HI21, relocVal);
relocs.addRelocationWithoutAddend(siteOffset, RelocationKind.AARCH64_R_AARCH64_ADR_PREL_PG_HI21, ref);

siteOffset += 4;
relocs.addRelocationWithoutAddend(siteOffset, RelocationKind.AARCH64_R_AARCH64_ADD_ABS_LO12_NC, relocVal);
relocs.addRelocationWithoutAddend(siteOffset, RelocationKind.AARCH64_R_AARCH64_ADD_ABS_LO12_NC, ref);
}

@Uninterruptible(reason = ".")
Expand Down Expand Up @@ -221,8 +213,8 @@ public void relocate(Reference ref, RelocatableBuffer relocs, int compStart) {
*/
int siteOffset = compStart + annotation.instructionPosition;
if (ref instanceof DataSectionReference || ref instanceof CGlobalDataReference || ref instanceof ConstantReference) {
if (ref instanceof ConstantReference) {
assert !(((ConstantReference) ref).getConstant() instanceof SubstrateMethodPointerConstant);
if (ref instanceof ConstantReference constantRef) {
VMError.guarantee(!(constantRef.getConstant() instanceof SubstrateMethodPointerConstant), "SubstrateMethodPointerConstants should not be relocated %s", constantRef);
}
/*
* calculating the last mov index. This is necessary ensure the proper overflow checks
Expand Down
17 changes: 3 additions & 14 deletions ...com.oracle.svm.hosted/src/com/oracle/svm/hosted/code/amd64/AMD64HostedPatcherFeature.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,19 +40,16 @@
import com.oracle.svm.core.feature.InternalFeature;
import com.oracle.svm.core.graal.code.CGlobalDataReference;
import com.oracle.svm.core.graal.code.PatchConsumerFactory;
import com.oracle.svm.core.meta.MethodPointer;
import com.oracle.svm.core.meta.SubstrateMethodPointerConstant;
import com.oracle.svm.core.util.VMError;
import com.oracle.svm.hosted.code.HostedImageHeapConstantPatch;
import com.oracle.svm.hosted.code.HostedPatcher;
import com.oracle.svm.hosted.image.RelocatableBuffer;
import com.oracle.svm.hosted.meta.HostedMethod;

import jdk.vm.ci.code.site.ConstantReference;
import jdk.vm.ci.code.site.DataSectionReference;
import jdk.vm.ci.code.site.Reference;
import jdk.vm.ci.meta.JavaConstant;
import jdk.vm.ci.meta.VMConstant;

@AutomaticallyRegisteredFeature
@Platforms({Platform.AMD64.class})
Expand Down Expand Up @@ -122,17 +119,9 @@ public void relocate(Reference ref, RelocatableBuffer relocs, int compStart) {
*/
long addend = (annotation.nextInstructionPosition - annotation.operandPosition);
relocs.addRelocationWithAddend((int) siteOffset, ObjectFile.RelocationKind.getPCRelative(annotation.operandSize), addend, ref);
} else if (ref instanceof ConstantReference) {
VMConstant constant = ((ConstantReference) ref).getConstant();
Object relocVal = ref;
if (constant instanceof SubstrateMethodPointerConstant) {
VMError.guarantee(!Platform.includedIn(Platform.DARWIN_AMD64.class), "[GR-43389] method pointer relocations should not be inlined.");
MethodPointer pointer = ((SubstrateMethodPointerConstant) constant).pointer();
HostedMethod hMethod = (HostedMethod) pointer.getMethod();
VMError.guarantee(hMethod.isCompiled(), "Method %s is not compiled although there is a method pointer constant created for it.", hMethod);
relocVal = pointer;
}
relocs.addRelocationWithoutAddend((int) siteOffset, ObjectFile.RelocationKind.getDirect(annotation.operandSize), relocVal);
} else if (ref instanceof ConstantReference constantRef) {
VMError.guarantee(!(constantRef.getConstant() instanceof SubstrateMethodPointerConstant), "SubstrateMethodPointerConstants should not be relocated %s", constantRef);
relocs.addRelocationWithoutAddend((int) siteOffset, ObjectFile.RelocationKind.getDirect(annotation.operandSize), ref);
} else {
throw VMError.shouldNotReachHere("Unknown type of reference in code");
}
Expand Down
27 changes: 21 additions & 6 deletions ...vm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/image/LIRNativeImageCodeCache.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@
import com.oracle.objectfile.ObjectFile;
import com.oracle.svm.core.SubstrateOptions;
import com.oracle.svm.core.config.ConfigurationValues;
import com.oracle.svm.core.meta.SubstrateMethodPointerConstant;
import com.oracle.svm.core.util.VMError;
import com.oracle.svm.hosted.code.HostedDirectCallTrampolineSupport;
import com.oracle.svm.hosted.code.HostedImageHeapConstantPatch;
Expand All @@ -54,6 +55,7 @@

import jdk.vm.ci.code.TargetDescription;
import jdk.vm.ci.code.site.Call;
import jdk.vm.ci.code.site.ConstantReference;
import jdk.vm.ci.code.site.DataPatch;
import jdk.vm.ci.code.site.Infopoint;
import jdk.vm.ci.code.site.Reference;
Expand Down Expand Up @@ -343,7 +345,7 @@ public void patchMethods(DebugContext debug, RelocatableBuffer relocs, ObjectFil
// the codecache-relative offset of the compilation
int compStart = method.getCodeAddressOffset();

// Build an index of PatchingAnnoations
// Build an index of PatchingAnnotations
Map<Integer, HostedPatcher> patches = new HashMap<>();
ByteBuffer targetCode = null;
for (CodeAnnotation codeAnnotation : compilation.getCodeAnnotations()) {
Expand Down Expand Up @@ -395,12 +397,25 @@ public void patchMethods(DebugContext debug, RelocatableBuffer relocs, ObjectFil
}
}
for (DataPatch dataPatch : compilation.getDataPatches()) {
assert dataPatch.note == null : "Unexpected note: " + dataPatch.note;
Reference ref = dataPatch.reference;
/*
* Constants are allocated offsets in a separate space, which can be emitted as
* read-only (.rodata) section.
*/
patches.get(dataPatch.pcOffset).relocate(ref, relocs, compStart);
var patcher = patches.get(dataPatch.pcOffset);
if (ref instanceof ConstantReference constant && constant.getConstant() instanceof SubstrateMethodPointerConstant methodPtrConstant) {
/*
* We directly patch SubstrateMethodPointerConstants.
*/
HostedMethod hMethod = (HostedMethod) methodPtrConstant.pointer().getMethod();
VMError.guarantee(hMethod.isCompiled(), "Method %s is not compiled although there is a method pointer constant created for it.", hMethod);
int targetOffset = hMethod.getCodeAddressOffset();
int pcDisplacement = targetOffset - (compStart + dataPatch.pcOffset);
patcher.patch(compStart, pcDisplacement, compilation.getTargetCode());
} else {
/*
* Constants are allocated offsets in a separate space, which can be emitted as
* read-only (.rodata) section.
*/
patcher.relocate(ref, relocs, compStart);
}
boolean noPriorMatch = patchedOffsets.add(dataPatch.pcOffset);
VMError.guarantee(noPriorMatch, "Patching same offset twice.");
patchesHandled++;
Expand Down
6 changes: 3 additions & 3 deletions ...atevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/image/NativeImageCodeCache.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,7 @@ public void layoutConstants() {
CompilationResult compilation = pair.getRight();
for (DataSection.Data data : compilation.getDataSection()) {
if (data instanceof SubstrateDataBuilder.ObjectData) {
VMConstant constant = ((SubstrateDataBuilder.ObjectData) data).getConstant();
JavaConstant constant = ((SubstrateDataBuilder.ObjectData) data).getConstant();
constantReasons.put(constant, compilation.getName());
}
}
Expand All @@ -217,7 +217,7 @@ public void layoutConstants() {
public void addConstantsToHeap() {
for (DataSection.Data data : dataSection) {
if (data instanceof SubstrateDataBuilder.ObjectData) {
VMConstant constant = ((SubstrateDataBuilder.ObjectData) data).getConstant();
JavaConstant constant = ((SubstrateDataBuilder.ObjectData) data).getConstant();
addConstantToHeap(constant, NativeImageHeap.HeapInclusionReason.DataSection);
}
}
Expand Down Expand Up @@ -599,7 +599,7 @@ protected boolean verifyMethods(HostedUniverse hUniverse, ForkJoinPool threadPoo
public void writeConstants(NativeImageHeapWriter writer, RelocatableBuffer buffer) {
ByteBuffer bb = buffer.getByteBuffer();
dataSection.buildDataSection(bb, (position, constant) -> {
writer.writeReference(buffer, position, constant, "VMConstant: " + constant);
writer.writeReference(buffer, position, (JavaConstant) constant, "VMConstant: " + constant);
});
}

Expand Down
Loading