From 7a945b9ecf4f0317248787f4aa9a52f791a48d72 Mon Sep 17 00:00:00 2001 From: Codrut Stancu Date: Tue, 18 Oct 2022 22:06:05 +0200 Subject: [PATCH] Reconstruct invoke source position for macro node and inlined invokes. --- .../pointsto/AbstractAnalysisEngine.java | 18 +++-- .../pointsto/flow/MethodTypeFlowBuilder.java | 72 +++++++++++++++---- .../graal/pointsto/reports/ReportUtils.java | 26 ++++--- 3 files changed, 85 insertions(+), 31 deletions(-) diff --git a/substratevm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/AbstractAnalysisEngine.java b/substratevm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/AbstractAnalysisEngine.java index 03767f57fd57b..a1b85dfa6b263 100644 --- a/substratevm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/AbstractAnalysisEngine.java +++ b/substratevm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/AbstractAnalysisEngine.java @@ -318,21 +318,25 @@ public Replacements getReplacements() { * Provide a non-null position. Some flows like newInstance and invoke require a non-null * position, for others is just better. The constructed position is best-effort, i.e., it * contains at least the method, and a BCI only if the node provides it. - * + *

* This is necessary because {@link Node#getNodeSourcePosition()} doesn't always provide a * position, like for example for generated factory methods in FactoryThrowMethodHolder. */ public static BytecodePosition sourcePosition(ValueNode node) { BytecodePosition position = node.getNodeSourcePosition(); if (position == null) { - int bci = BytecodeFrame.UNKNOWN_BCI; - if (node instanceof DeoptBciSupplier) { - bci = ((DeoptBciSupplier) node).bci(); - } - ResolvedJavaMethod method = node.graph().method(); - position = new BytecodePosition(null, method, bci); + position = syntheticSourcePosition(node, node.graph().method()); } return position; } + /** Creates a synthetic position for the node in the given method. */ + public static BytecodePosition syntheticSourcePosition(ValueNode node, ResolvedJavaMethod method) { + int bci = BytecodeFrame.UNKNOWN_BCI; + if (node instanceof DeoptBciSupplier) { + bci = ((DeoptBciSupplier) node).bci(); + } + return new BytecodePosition(null, method, bci); + } + } diff --git a/substratevm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/flow/MethodTypeFlowBuilder.java b/substratevm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/flow/MethodTypeFlowBuilder.java index 61f361fa5f929..d9f1647704aa6 100644 --- a/substratevm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/flow/MethodTypeFlowBuilder.java +++ b/substratevm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/flow/MethodTypeFlowBuilder.java @@ -43,6 +43,7 @@ import org.graalvm.compiler.graph.Node.NodeIntrinsic; import org.graalvm.compiler.graph.NodeBitMap; import org.graalvm.compiler.graph.NodeInputList; +import org.graalvm.compiler.graph.NodeSourcePosition; import org.graalvm.compiler.nodes.AbstractEndNode; import org.graalvm.compiler.nodes.AbstractMergeNode; import org.graalvm.compiler.nodes.BeginNode; @@ -109,6 +110,7 @@ import org.graalvm.compiler.replacements.nodes.UnaryMathIntrinsicNode; import org.graalvm.compiler.virtual.phases.ea.PartialEscapePhase; import org.graalvm.compiler.word.WordCastNode; +import org.graalvm.nativeimage.AnnotationAccess; import com.oracle.graal.pointsto.AbstractAnalysisEngine; import com.oracle.graal.pointsto.PointsToAnalysis; @@ -140,7 +142,6 @@ import jdk.vm.ci.meta.JavaConstant; import jdk.vm.ci.meta.JavaKind; import jdk.vm.ci.meta.VMConstant; -import org.graalvm.nativeimage.AnnotationAccess; public class MethodTypeFlowBuilder { @@ -1176,7 +1177,7 @@ protected void node(FixedNode n) { invoke.stateAfter(), invoke.stateAfter().outerFrameState()); MethodCallTargetNode target = (MethodCallTargetNode) invoke.callTarget(); - processMethodInvocation(state, invoke.asFixedNode(), target.invokeKind(), (PointsToAnalysisMethod) target.targetMethod(), target.arguments()); + processMethodInvocation(state, invoke, target.invokeKind(), (PointsToAnalysisMethod) target.targetMethod(), target.arguments()); } } else if (n instanceof ObjectClone) { @@ -1304,17 +1305,67 @@ protected boolean delegateNodeProcessing(FixedNode n, TypeFlowsOfNodes state) { } protected void processMacroInvokable(TypeFlowsOfNodes state, MacroInvokable macro, boolean installResult) { - processMethodInvocation(state, macro.asNode(), macro.getInvokeKind(), (PointsToAnalysisMethod) macro.getTargetMethod(), macro.getArguments(), installResult); + ValueNode macroNode = macro.asNode(); + BytecodePosition invokePosition = getInvokePosition(macro, macroNode); + processMethodInvocation(state, macroNode, macro.getInvokeKind(), (PointsToAnalysisMethod) macro.getTargetMethod(), macro.getArguments(), installResult, invokePosition); } - protected void processMethodInvocation(TypeFlowsOfNodes state, ValueNode invoke, InvokeKind invokeKind, PointsToAnalysisMethod targetMethod, NodeInputList arguments) { - processMethodInvocation(state, invoke, invokeKind, targetMethod, arguments, true); + /* Reconstruct the macro node invoke position, avoiding cycles in the parsing backtrace. */ + private BytecodePosition getInvokePosition(MacroInvokable macro, ValueNode macroNode) { + BytecodePosition invokePosition = null; + NodeSourcePosition position = macroNode.getNodeSourcePosition(); + if (position != null) { + /* + * BytecodeParser.applyInvocationPlugin() gives the macro nodes a position in the target + * method. We pop it here because the invoke flow needs a position in the caller, i.e., + * the currently parsed method. + */ + assert position.getMethod().equals(macro.getTargetMethod()) : "Unexpected macro node source position: " + macro + " at " + position; + invokePosition = position.getCaller(); + } + if (invokePosition == null) { + invokePosition = AbstractAnalysisEngine.syntheticSourcePosition(macroNode, method); + } + return invokePosition; + } + + protected void processMethodInvocation(TypeFlowsOfNodes state, Invoke invoke, InvokeKind invokeKind, PointsToAnalysisMethod targetMethod, NodeInputList arguments) { + FixedNode invokeNode = invoke.asFixedNode(); + BytecodePosition invokePosition = getInvokePosition(invokeNode); + processMethodInvocation(state, invokeNode, invokeKind, targetMethod, arguments, true, invokePosition); + } + + /* Get a reasonable position for inlined invokes, avoiding cycles in the parsing backtrace. */ + private BytecodePosition getInvokePosition(FixedNode invokeNode) { + BytecodePosition invokePosition = invokeNode.getNodeSourcePosition(); + /* Get the outermost caller position for inlined invokes. */ + while (invokePosition != null && invokePosition.getCaller() != null) { + /* + * Invokes coming from recursive inlined methods can lead to cycles when reporting the + * parsing backtrace if we use the reported position directly. For inlined nodes Graal + * reports the original position (in the original method before inlining) and sets its + * caller to the inline location. So by using the outermost caller we simply use a + * version of the graph preprocessed by inline-before-analysis, whose shape may differ + * from the original source code. In some cases this will lead to stack traces with + * missing frames, but it is always correct. + */ + invokePosition = invokePosition.getCaller(); + } + + if (invokePosition == null) { + /* + * The invokePosition is used for all sorts of call stack printing (for error messages + * and diagnostics), so we must have a non-null BytecodePosition. + */ + invokePosition = AbstractAnalysisEngine.syntheticSourcePosition(invokeNode, method); + } + return invokePosition; } protected void processMethodInvocation(TypeFlowsOfNodes state, ValueNode invoke, InvokeKind invokeKind, PointsToAnalysisMethod targetMethod, NodeInputList arguments, - boolean installResult) { + boolean installResult, BytecodePosition invokeLocation) { // check if the call is allowed - bb.isCallAllowed(bb, method, targetMethod, AbstractAnalysisEngine.sourcePosition(invoke)); + bb.isCallAllowed(bb, method, targetMethod, invokeLocation); /* * Collect the parameters builders into an array so that we don't capture the `state` @@ -1366,11 +1417,6 @@ protected void processMethodInvocation(TypeFlowsOfNodes state, ValueNode invoke, } } - /* - * The invokeLocation is used for all sorts of call stack printing (for error messages - * and diagnostics), so we must have a non-null BytecodePosition. - */ - BytecodePosition invokeLocation = AbstractAnalysisEngine.sourcePosition(invoke); InvokeTypeFlow invokeFlow; switch (invokeKind) { case Static: @@ -1425,7 +1471,7 @@ protected void processMethodInvocation(TypeFlowsOfNodes state, ValueNode invoke, * filter to avoid loosing precision. */ TypeFlowBuilder filterBuilder = TypeFlowBuilder.create(bb, invoke, FilterTypeFlow.class, () -> { - FilterTypeFlow filterFlow = new FilterTypeFlow(AbstractAnalysisEngine.sourcePosition(invoke), (AnalysisType) stamp.type(), stamp.isExactType(), true, true); + FilterTypeFlow filterFlow = new FilterTypeFlow(invokeLocation, (AnalysisType) stamp.type(), stamp.isExactType(), true, true); flowsGraph.addMiscEntryFlow(filterFlow); return filterFlow; }); diff --git a/substratevm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/reports/ReportUtils.java b/substratevm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/reports/ReportUtils.java index 5a6dc666a5adc..243e056d162a0 100644 --- a/substratevm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/reports/ReportUtils.java +++ b/substratevm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/reports/ReportUtils.java @@ -233,23 +233,27 @@ public static String parsingContext(AnalysisMethod method, int bci, String inden /* Include target method first. */ msg.append(String.format("%n%sat %s", indent, method.asStackTraceElement(bci))); } - /* Then add the parsing context. */ - for (int i = 0; i < parsingContext.length; i++) { - StackTraceElement e = parsingContext[i]; - if (isStackTraceTruncationSentinel(e)) { - msg.append(String.format("%n%s", e.getClassName())); - assert i == parsingContext.length - 1; - } else { - msg.append(String.format("%n%sat %s", indent, e)); - } - } - msg.append(String.format("%n")); + formatParsingContext(parsingContext, indent, msg); } else { msg.append(String.format(" %n")); } return msg.toString(); } + public static void formatParsingContext(StackTraceElement[] parsingContext, String indent, StringBuilder msg) { + /* Then add the parsing context. */ + for (int i = 0; i < parsingContext.length; i++) { + StackTraceElement e = parsingContext[i]; + if (isStackTraceTruncationSentinel(e)) { + msg.append(String.format("%n%s", e.getClassName())); + assert i == parsingContext.length - 1; + } else { + msg.append(String.format("%n%sat %s", indent, e)); + } + } + msg.append(String.format("%n")); + } + private static final String stackTraceTruncationSentinel = "WARNING: Parsing context is truncated because its depth exceeds a reasonable limit for "; private static boolean isStackTraceTruncationSentinel(StackTraceElement element) {