From cd77e1035b303e0d764a2c9b4c41fc27b2f2b166 Mon Sep 17 00:00:00 2001 From: Tom Shull Date: Mon, 2 Oct 2023 17:03:48 +0200 Subject: [PATCH 1/4] DeoptProxyNode is not an ArrayLengthProvider. --- .../compiler/nodes/util/GraphUtil.java | 35 +++++++++++++------ .../svm/hosted/nodes/DeoptProxyNode.java | 21 +++++------ 2 files changed, 33 insertions(+), 23 deletions(-) diff --git a/compiler/src/jdk.internal.vm.compiler/src/org/graalvm/compiler/nodes/util/GraphUtil.java b/compiler/src/jdk.internal.vm.compiler/src/org/graalvm/compiler/nodes/util/GraphUtil.java index 58456f0cc0c6..c521c4c88286 100644 --- a/compiler/src/jdk.internal.vm.compiler/src/org/graalvm/compiler/nodes/util/GraphUtil.java +++ b/compiler/src/jdk.internal.vm.compiler/src/org/graalvm/compiler/nodes/util/GraphUtil.java @@ -742,38 +742,53 @@ public static ValueNode arrayLength(ValueNode value, FindLengthMode mode, Consta return arrayLength(value, mode, constantReflection, null); } + /** + * Filters out non-constant results when requested. + */ + private static ValueNode filterArrayLengthResult(ValueNode result, boolean allowOnlyConstantResult) { + return result == null || !allowOnlyConstantResult || result.isConstant() ? result : null; + } + private static ValueNode arrayLength(ValueNode value, FindLengthMode mode, ConstantReflectionProvider constantReflection, EconomicMap visitedPhiInputs) { Objects.requireNonNull(mode); EconomicMap visitedPhiInputMap = visitedPhiInputs; ValueNode current = value; StructuredGraph graph = value.graph(); + boolean allowOnlyConstantResult = false; do { CompilationAlarm.checkProgress(graph); /* * PiArrayNode implements ArrayLengthProvider and ValueProxy. We want to treat it as an * ArrayLengthProvider, therefore we check this case first. */ - if (current instanceof ArrayLengthProvider) { - return ((ArrayLengthProvider) current).findLength(mode, constantReflection); + if (current instanceof ArrayLengthProvider provider) { + return filterArrayLengthResult(provider.findLength(mode, constantReflection), allowOnlyConstantResult); - } else if (current instanceof ValuePhiNode) { + } else if (current instanceof ValuePhiNode phi) { if (visitedPhiInputMap == null) { visitedPhiInputMap = EconomicMap.create(); } - return phiArrayLength((ValuePhiNode) current, mode, constantReflection, visitedPhiInputMap); + return filterArrayLengthResult(phiArrayLength(phi, mode, constantReflection, visitedPhiInputMap), allowOnlyConstantResult); - } else if (current instanceof ValueProxyNode) { - ValueProxyNode proxy = (ValueProxyNode) current; + } else if (current instanceof ValueProxyNode proxy) { ValueNode length = arrayLength(proxy.getOriginalNode(), mode, constantReflection); if (mode == ArrayLengthProvider.FindLengthMode.CANONICALIZE_READ && length != null && !length.isConstant()) { length = new ValueProxyNode(length, proxy.proxyPoint()); } - return length; + return filterArrayLengthResult(length, allowOnlyConstantResult); - } else if (current instanceof ValueProxy) { - /* Written as a loop instead of a recursive call to reduce recursion depth. */ - current = ((ValueProxy) current).getOriginalNode(); + } else if (current instanceof LimitedValueProxy valueProxy) { + /* + * Note is it usually recommended to check for ValueProxy, not LimitedValueProxy. + * However, in this case we are intentionally unproxifying all LimitedValueProxies, + * as we want constant lengths to be found across DeoptProxyNodes. When the result + * is not a ValueProxy we limit the returned result to constant values. + */ + if (!(valueProxy instanceof ValueProxy)) { + allowOnlyConstantResult = true; + } + current = valueProxy.getOriginalNode(); } else { return null; } diff --git a/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/nodes/DeoptProxyNode.java b/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/nodes/DeoptProxyNode.java index 58076e389f40..3f802147a5e5 100644 --- a/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/nodes/DeoptProxyNode.java +++ b/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/nodes/DeoptProxyNode.java @@ -28,8 +28,6 @@ import org.graalvm.compiler.graph.Node; import org.graalvm.compiler.graph.Node.ValueNumberable; import org.graalvm.compiler.graph.NodeClass; -import org.graalvm.compiler.nodes.spi.Canonicalizable; -import org.graalvm.compiler.nodes.spi.CanonicalizerTool; import org.graalvm.compiler.nodeinfo.InputType; import org.graalvm.compiler.nodeinfo.NodeCycles; import org.graalvm.compiler.nodeinfo.NodeInfo; @@ -38,16 +36,14 @@ import org.graalvm.compiler.nodes.ParameterNode; import org.graalvm.compiler.nodes.ValueNode; import org.graalvm.compiler.nodes.calc.FloatingNode; -import org.graalvm.compiler.nodes.spi.ArrayLengthProvider; +import org.graalvm.compiler.nodes.spi.Canonicalizable; +import org.graalvm.compiler.nodes.spi.CanonicalizerTool; import org.graalvm.compiler.nodes.spi.LIRLowerable; import org.graalvm.compiler.nodes.spi.LimitedValueProxy; import org.graalvm.compiler.nodes.spi.NodeLIRBuilderTool; -import org.graalvm.compiler.nodes.util.GraphUtil; import com.oracle.svm.core.deopt.Deoptimizer; -import jdk.vm.ci.meta.ConstantReflectionProvider; - /** * Wraps locals and bytecode stack elements at deoptimization points. DeoptProxyNodes are inserted * in deoptimization target methods to avoid global value numbering and rescheduling of local @@ -56,9 +52,14 @@ * This is needed to ensure that the values, which are set by the {@link Deoptimizer} at the * deoptimization point, are really read from their locations (and not held in a temporary register, * etc.) + * + * Note the {@link #value} of the DeoptProxyNode may be another DeoptProxyNode (i.e., there may be a + * chain of DeoptProxyNodes leading to the original value). This is by design: linking to the + * preceding DeoptProxyNode allows a given DeoptEntry and its corresponding DeoptProxyNodes to be + * easily removed without causing correctness issues. */ @NodeInfo(cycles = NodeCycles.CYCLES_0, size = NodeSize.SIZE_0) -public final class DeoptProxyNode extends FloatingNode implements LimitedValueProxy, ValueNumberable, LIRLowerable, Canonicalizable, IterableNodeType, ArrayLengthProvider { +public final class DeoptProxyNode extends FloatingNode implements LimitedValueProxy, ValueNumberable, LIRLowerable, Canonicalizable, IterableNodeType { public static final NodeClass TYPE = NodeClass.create(DeoptProxyNode.class); /** @@ -127,10 +128,4 @@ public boolean hasProxyPoint() { public ValueNode getProxyPoint() { return proxyPoint; } - - @Override - public ValueNode findLength(FindLengthMode mode, ConstantReflectionProvider constantReflection) { - ValueNode length = GraphUtil.arrayLength(value, mode, constantReflection); - return length != null && length.isConstant() ? length : null; - } } From cb22258d876cd32623a4091c2c17a7714d8d39cb Mon Sep 17 00:00:00 2001 From: Tom Shull Date: Thu, 5 Oct 2023 12:16:21 +0200 Subject: [PATCH 2/4] Revert "[GR-43389] [darwin-amd64] Workaround for buggy ld64 versions" This reverts commit e30698b53f00a021fce8e7be17da960fcb4a262a. --- .../AMD64LoadMethodPointerConstantOp.java | 12 ++------ .../core/graal/code/SubstrateDataBuilder.java | 27 +++++++---------- .../oracle/svm/hosted/code/CompileQueue.java | 29 +++++-------------- .../code/amd64/AMD64HostedPatcherFeature.java | 1 - .../hosted/image/NativeImageCodeCache.java | 6 ++-- .../hosted/image/NativeImageHeapWriter.java | 16 ++-------- 6 files changed, 25 insertions(+), 66 deletions(-) diff --git a/substratevm/src/com.oracle.svm.core.graal.amd64/src/com/oracle/svm/core/graal/amd64/AMD64LoadMethodPointerConstantOp.java b/substratevm/src/com.oracle.svm.core.graal.amd64/src/com/oracle/svm/core/graal/amd64/AMD64LoadMethodPointerConstantOp.java index df38fd162b75..46ec57d7ab26 100644 --- a/substratevm/src/com.oracle.svm.core.graal.amd64/src/com/oracle/svm/core/graal/amd64/AMD64LoadMethodPointerConstantOp.java +++ b/substratevm/src/com.oracle.svm.core.graal.amd64/src/com/oracle/svm/core/graal/amd64/AMD64LoadMethodPointerConstantOp.java @@ -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; @@ -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 TYPE = LIRInstructionClass.create(AMD64LoadMethodPointerConstantOp.class); @@ -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.movq(resultReg, 0L, true); } @Override diff --git a/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/graal/code/SubstrateDataBuilder.java b/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/graal/code/SubstrateDataBuilder.java index 16df77749125..abccd9e73d3d 100644 --- a/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/graal/code/SubstrateDataBuilder.java +++ b/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/graal/code/SubstrateDataBuilder.java @@ -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; @@ -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)) { @@ -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)); } @@ -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 @@ -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); } diff --git a/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/code/CompileQueue.java b/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/code/CompileQueue.java index ed9bef9f3994..f59a982b974f 100644 --- a/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/code/CompileQueue.java +++ b/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/code/CompileQueue.java @@ -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; @@ -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; @@ -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 { @@ -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)); } } } diff --git a/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/code/amd64/AMD64HostedPatcherFeature.java b/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/code/amd64/AMD64HostedPatcherFeature.java index 8258907c0d20..6b341a19d53c 100644 --- a/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/code/amd64/AMD64HostedPatcherFeature.java +++ b/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/code/amd64/AMD64HostedPatcherFeature.java @@ -126,7 +126,6 @@ public void relocate(Reference ref, RelocatableBuffer relocs, int compStart) { 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); diff --git a/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/image/NativeImageCodeCache.java b/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/image/NativeImageCodeCache.java index 1eb568e22f6d..630cffa2bd4c 100644 --- a/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/image/NativeImageCodeCache.java +++ b/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/image/NativeImageCodeCache.java @@ -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()); } } @@ -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); } } @@ -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); }); } diff --git a/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/image/NativeImageHeapWriter.java b/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/image/NativeImageHeapWriter.java index acc0bbbc7cef..4e5df32b5424 100644 --- a/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/image/NativeImageHeapWriter.java +++ b/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/image/NativeImageHeapWriter.java @@ -37,7 +37,6 @@ import org.graalvm.compiler.debug.DebugContext; import org.graalvm.compiler.debug.Indent; import org.graalvm.nativeimage.ImageSingletons; -import org.graalvm.nativeimage.Platform; import org.graalvm.nativeimage.c.function.CFunctionPointer; import org.graalvm.nativeimage.c.function.RelocatedPointer; import org.graalvm.word.WordBase; @@ -169,16 +168,10 @@ private void write(RelocatableBuffer buffer, int index, JavaConstant con, Object private final boolean useHeapBase = NativeImageHeap.useHeapBase(); private final CompressEncoding compressEncoding = ImageSingletons.lookup(CompressEncoding.class); - void writeReference(RelocatableBuffer buffer, int index, Constant constant, Object reason) { + void writeReference(RelocatableBuffer buffer, int index, JavaConstant target, Object reason) { + assert !(heap.hMetaAccess.isInstanceOf(target, WordBase.class)) : "word values are not references"; mustBeReferenceAligned(index); - - if (constant instanceof JavaConstant target) { - assert !(heap.hMetaAccess.isInstanceOf(target, WordBase.class)) : "word values are not references"; - - if (target.isNull()) { - return; - } - + if (target.isNonNull()) { ObjectInfo targetInfo = heap.getConstantInfo(target); verifyTargetDidNotChange(target, reason, targetInfo); if (useHeapBase) { @@ -187,9 +180,6 @@ void writeReference(RelocatableBuffer buffer, int index, Constant constant, Obje } else { addDirectRelocationWithoutAddend(buffer, index, referenceSize(), target); } - } else { - assert Platform.includedIn(Platform.DARWIN_AMD64.class) : "[GR-43389] Workaround for ld64 bug that does not allow direct8 relocations in .text on amd64"; - buffer.addRelocationWithoutAddend(index, ObjectFile.RelocationKind.DIRECT_8, ((SubstrateMethodPointerConstant) constant).pointer()); } } From 613d0c6c0c3aa764439e42f795a31c62322fdc9b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sol=C3=A8ne=20Husseini?= Date: Thu, 5 Oct 2023 11:19:21 +0200 Subject: [PATCH 3/4] Update vararg handling to match JDK22 --- .../com/oracle/svm/core/foreign/AbiUtils.java | 19 ++++++++-- .../graal/snippets/CFunctionSnippets.java | 37 ------------------- .../svm/core/nodes/CFunctionCaptureNode.java | 18 ++++----- 3 files changed, 25 insertions(+), 49 deletions(-) diff --git a/substratevm/src/com.oracle.svm.core.foreign/src/com/oracle/svm/core/foreign/AbiUtils.java b/substratevm/src/com.oracle.svm.core.foreign/src/com/oracle/svm/core/foreign/AbiUtils.java index 41f141598e89..5f7508fce6d5 100644 --- a/substratevm/src/com.oracle.svm.core.foreign/src/com/oracle/svm/core/foreign/AbiUtils.java +++ b/substratevm/src/com.oracle.svm.core.foreign/src/com/oracle/svm/core/foreign/AbiUtils.java @@ -511,9 +511,22 @@ protected CallingSequence makeCallingSequence(MethodType type, FunctionDescripto @Override protected List generateAdaptations(NativeEntryPointInfo nep) { var adaptations = super.generateAdaptations(nep); - /* Drop the rax parametersAssignment */ - assert adaptations.get(adaptations.size() - 1) == null; - adaptations.set(adaptations.size() - 1, Adapter.drop()); + var assignments = nep.parametersAssignment(); + + if (assignments.length > 0) { + final int last = assignments.length - 1; + if (assignments[last].equals(X86_64Architecture.Regs.rax)) { + /* + * This branch is only taken when the function is variadic, that is when rax is + * passed as an additional pseudo-parameter, where it will contain the number of + * XMM registers passed as arguments. However, we need to remove the rax + * assignment since rax will already be assigned separately in + * SubstrateAMD64RegisterConfig.getCallingConvention and later used in + * SubstrateAMD64NodeLIRBuilder.visitInvokeArguments. + */ + adaptations.set(last, Adapter.drop()); + } + } return adaptations; } diff --git a/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/graal/snippets/CFunctionSnippets.java b/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/graal/snippets/CFunctionSnippets.java index e94411963ce5..0b8463b956ab 100644 --- a/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/graal/snippets/CFunctionSnippets.java +++ b/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/graal/snippets/CFunctionSnippets.java @@ -30,15 +30,12 @@ import org.graalvm.compiler.api.replacements.Snippet; import org.graalvm.compiler.api.replacements.Snippet.ConstantParameter; -import org.graalvm.compiler.core.common.spi.ForeignCallDescriptor; import org.graalvm.compiler.graph.Node; import org.graalvm.compiler.nodes.FixedNode; import org.graalvm.compiler.nodes.FixedWithNextNode; import org.graalvm.compiler.nodes.Invoke; import org.graalvm.compiler.nodes.InvokeNode; import org.graalvm.compiler.nodes.InvokeWithExceptionNode; -import org.graalvm.compiler.nodes.ValueNode; -import org.graalvm.compiler.nodes.extended.ForeignCallNode; import org.graalvm.compiler.nodes.extended.MembarNode; import org.graalvm.compiler.nodes.spi.LoweringTool; import org.graalvm.compiler.options.OptionValues; @@ -50,7 +47,6 @@ import org.graalvm.compiler.replacements.Snippets; import org.graalvm.nativeimage.Platforms; import org.graalvm.nativeimage.c.struct.SizeOf; -import org.graalvm.nativeimage.c.type.CIntPointer; import org.graalvm.nativeimage.impl.InternalPlatform; import org.graalvm.word.LocationIdentity; @@ -62,7 +58,6 @@ import com.oracle.svm.core.graal.nodes.VerificationMarkerNode; import com.oracle.svm.core.graal.stackvalue.LoweredStackValueNode; import com.oracle.svm.core.graal.stackvalue.StackValueNode.StackSlotIdentity; -import com.oracle.svm.core.nodes.CFunctionCaptureNode; import com.oracle.svm.core.nodes.CFunctionEpilogueNode; import com.oracle.svm.core.nodes.CFunctionPrologueDataNode; import com.oracle.svm.core.nodes.CFunctionPrologueNode; @@ -98,7 +93,6 @@ public final class CFunctionSnippets extends SubstrateTemplates implements Snippets { private final SnippetInfo prologue; - private final SnippetInfo capture; private final SnippetInfo epilogue; /** @@ -123,14 +117,6 @@ private static CPrologueData prologueSnippet(@ConstantParameter int newThreadSta return CFunctionPrologueDataNode.cFunctionPrologueData(anchor, newThreadStatus); } - @Node.NodeIntrinsic(value = ForeignCallNode.class) - public static native void callCaptureFunction(@Node.ConstantNodeParameter ForeignCallDescriptor descriptor, int states, CIntPointer captureBuffer); - - @Snippet - private static void captureSnippet(@ConstantParameter ForeignCallDescriptor captureFunction, int statesToCapture, CIntPointer captureBuffer) { - callCaptureFunction(captureFunction, statesToCapture, captureBuffer); - } - @Snippet private static void epilogueSnippet(@ConstantParameter int oldThreadStatus) { if (SubstrateOptions.MultiThreaded.getValue()) { @@ -157,11 +143,9 @@ private static void epilogueSnippet(@ConstantParameter int oldThreadStatus) { super(options, providers); this.prologue = snippet(providers, CFunctionSnippets.class, "prologueSnippet"); - this.capture = snippet(providers, CFunctionSnippets.class, "captureSnippet"); this.epilogue = snippet(providers, CFunctionSnippets.class, "epilogueSnippet"); lowerings.put(CFunctionPrologueNode.class, new CFunctionPrologueLowering()); - lowerings.put(CFunctionCaptureNode.class, new CFunctionCaptureLowering()); lowerings.put(CFunctionEpilogueNode.class, new CFunctionEpilogueLowering()); } @@ -193,27 +177,6 @@ public void lower(CFunctionPrologueNode node, LoweringTool tool) { } } - class CFunctionCaptureLowering implements NodeLoweringProvider { - @Override - public void lower(CFunctionCaptureNode node, LoweringTool tool) { - if (tool.getLoweringStage() != LoweringTool.StandardLoweringStage.LOW_TIER) { - return; - } - - ValueNode statesToCapture = node.getStatesToCapture(); - ForeignCallDescriptor captureFunction = node.getCaptureFunction(); - ValueNode buffer = node.getCaptureBuffer(); - Arguments args = new Arguments(capture, node.graph().getGuardsStage(), tool.getLoweringStage()); - args.addConst("captureFunction", captureFunction); - args.add("statesToCapture", statesToCapture); - args.add("captureBuffer", buffer); - - SnippetTemplate template = template(tool, node, args); - template.setMayRemoveLocation(true); - template.instantiate(tool.getMetaAccess(), node, SnippetTemplate.DEFAULT_REPLACER, args); - } - } - class CFunctionEpilogueLowering implements NodeLoweringProvider { @Override public void lower(CFunctionEpilogueNode node, LoweringTool tool) { diff --git a/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/nodes/CFunctionCaptureNode.java b/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/nodes/CFunctionCaptureNode.java index 43319828d00c..fc7b0b255cf3 100644 --- a/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/nodes/CFunctionCaptureNode.java +++ b/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/nodes/CFunctionCaptureNode.java @@ -37,8 +37,10 @@ import org.graalvm.compiler.nodeinfo.NodeInfo; import org.graalvm.compiler.nodes.FixedWithNextNode; import org.graalvm.compiler.nodes.ValueNode; +import org.graalvm.compiler.nodes.extended.ForeignCallNode; import org.graalvm.compiler.nodes.memory.SingleMemoryKill; import org.graalvm.compiler.nodes.spi.Lowerable; +import org.graalvm.compiler.nodes.spi.LoweringTool; import org.graalvm.word.LocationIdentity; /** @@ -76,15 +78,13 @@ public LocationIdentity getKilledLocationIdentity() { return LocationIdentity.any(); } - public ForeignCallDescriptor getCaptureFunction() { - return captureFunction; - } - - public ValueNode getStatesToCapture() { - return statesToCapture; - } + @Override + public void lower(LoweringTool tool) { + if (tool.getLoweringStage() != LoweringTool.StandardLoweringStage.LOW_TIER) { + return; + } - public ValueNode getCaptureBuffer() { - return captureBuffer; + final ForeignCallNode call = graph().add(new ForeignCallNode(captureFunction, statesToCapture, captureBuffer)); + graph().replaceFixedWithFixed(this, call); } } From 946090a618d0269fea79da165fe738096577e3d1 Mon Sep 17 00:00:00 2001 From: Tom Shull Date: Thu, 5 Oct 2023 12:20:52 +0200 Subject: [PATCH 4/4] use pc-relative instructions for loading SubstrateMethodPointerConstants --- .../AMD64LoadMethodPointerConstantOp.java | 2 +- .../aarch64/AArch64HostedPatcherFeature.java | 24 ++++++----------- .../code/amd64/AMD64HostedPatcherFeature.java | 16 +++-------- .../hosted/image/LIRNativeImageCodeCache.java | 27 ++++++++++++++----- .../hosted/image/NativeImageHeapWriter.java | 2 -- 5 files changed, 33 insertions(+), 38 deletions(-) diff --git a/substratevm/src/com.oracle.svm.core.graal.amd64/src/com/oracle/svm/core/graal/amd64/AMD64LoadMethodPointerConstantOp.java b/substratevm/src/com.oracle.svm.core.graal.amd64/src/com/oracle/svm/core/graal/amd64/AMD64LoadMethodPointerConstantOp.java index 46ec57d7ab26..855bff4b10a2 100644 --- a/substratevm/src/com.oracle.svm.core.graal.amd64/src/com/oracle/svm/core/graal/amd64/AMD64LoadMethodPointerConstantOp.java +++ b/substratevm/src/com.oracle.svm.core.graal.amd64/src/com/oracle/svm/core/graal/amd64/AMD64LoadMethodPointerConstantOp.java @@ -54,7 +54,7 @@ public final class AMD64LoadMethodPointerConstantOp extends AMD64LIRInstruction public void emitCode(CompilationResultBuilder crb, AMD64MacroAssembler masm) { Register resultReg = asRegister(result); crb.recordInlineDataInCode(constant); - masm.movq(resultReg, 0L, true); + masm.leaq(resultReg, masm.getPlaceholder(masm.position())); } @Override diff --git a/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/code/aarch64/AArch64HostedPatcherFeature.java b/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/code/aarch64/AArch64HostedPatcherFeature.java index f34cbe49fe6c..f926ba12002d 100755 --- a/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/code/aarch64/AArch64HostedPatcherFeature.java +++ b/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/code/aarch64/AArch64HostedPatcherFeature.java @@ -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}) @@ -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 = ".") @@ -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 diff --git a/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/code/amd64/AMD64HostedPatcherFeature.java b/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/code/amd64/AMD64HostedPatcherFeature.java index 6b341a19d53c..af515906bb07 100644 --- a/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/code/amd64/AMD64HostedPatcherFeature.java +++ b/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/code/amd64/AMD64HostedPatcherFeature.java @@ -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}) @@ -122,16 +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) { - 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"); } diff --git a/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/image/LIRNativeImageCodeCache.java b/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/image/LIRNativeImageCodeCache.java index 7bbf7e868edb..783637b1ae5e 100644 --- a/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/image/LIRNativeImageCodeCache.java +++ b/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/image/LIRNativeImageCodeCache.java @@ -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; @@ -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; @@ -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 patches = new HashMap<>(); ByteBuffer targetCode = null; for (CodeAnnotation codeAnnotation : compilation.getCodeAnnotations()) { @@ -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++; diff --git a/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/image/NativeImageHeapWriter.java b/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/image/NativeImageHeapWriter.java index 4e5df32b5424..2203bcb91551 100644 --- a/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/image/NativeImageHeapWriter.java +++ b/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/image/NativeImageHeapWriter.java @@ -54,7 +54,6 @@ import com.oracle.svm.core.hub.DynamicHub; import com.oracle.svm.core.image.ImageHeapLayoutInfo; import com.oracle.svm.core.meta.MethodPointer; -import com.oracle.svm.core.meta.SubstrateMethodPointerConstant; import com.oracle.svm.core.meta.SubstrateObjectConstant; import com.oracle.svm.hosted.config.HybridLayout; import com.oracle.svm.hosted.image.NativeImageHeap.ObjectInfo; @@ -65,7 +64,6 @@ import com.oracle.svm.hosted.meta.RelocatableConstant; import jdk.internal.misc.Unsafe; -import jdk.vm.ci.meta.Constant; import jdk.vm.ci.meta.JavaConstant; import jdk.vm.ci.meta.JavaKind;