Do not step over roots and use the next BCI depending on the suspend anchor. (GR-67909)

entlicher · entlicher · commit be3ccbbe872b · 2025-07-25T10:28:16.000+02:00
diff --git a/espresso/src/com.oracle.truffle.espresso.jdwp/src/com/oracle/truffle/espresso/jdwp/api/JDWPContext.java b/espresso/src/com.oracle.truffle.espresso.jdwp/src/com/oracle/truffle/espresso/jdwp/api/JDWPContext.java
@@ -382,11 +382,12 @@ public interface JDWPContext {
     /**
      * Returns the bci of the next bytecode instruction within the current frame
      *
-     * @param callerRoot the root node of the caller frame
+     * @param method the current method
+     * @param rawNode the current node
      * @param frame the frame to read the current bci from
      * @return the bci of the next instruction
      */
-    int getNextBCI(RootNode callerRoot, Frame frame);
+    int getNextBCI(MethodRef method, Node rawNode, Frame frame);
 
     /**
      * Returns the current BCI or -1 if the BCI cannot be read.
@@ -395,7 +396,7 @@ public interface JDWPContext {
      * @param frame the frame to read the bci from
      * @return the BCI or -1
      */
-    long readBCIFromFrame(RootNode root, Frame frame);
+    int readBCIFromFrame(RootNode root, Frame frame);
 
     /**
      * Returns a {@link CallFrame} representation of the location of
@@ -461,7 +462,7 @@ public interface JDWPContext {
      * @param frame the current frame
      * @return the current bci
      */
-    long getBCI(Node rawNode, Frame frame);
+    int getBCI(Node rawNode, Frame frame);
 
     /**
      * Returns the instrumentable delegate node for the language root node or <code>rootNode</code>
diff --git a/espresso/src/com.oracle.truffle.espresso.jdwp/src/com/oracle/truffle/espresso/jdwp/api/MethodRef.java b/espresso/src/com.oracle.truffle.espresso.jdwp/src/com/oracle/truffle/espresso/jdwp/api/MethodRef.java
@@ -231,7 +231,7 @@ public interface MethodRef {
      *
      * @return last bci
      */
-    long getLastBCI();
+    int getLastBCI();
 
     /**
      * Determines if the method is a constructor.
diff --git a/espresso/src/com.oracle.truffle.espresso.jdwp/src/com/oracle/truffle/espresso/jdwp/api/VMEventListenerImpl.java b/espresso/src/com.oracle.truffle.espresso.jdwp/src/com/oracle/truffle/espresso/jdwp/api/VMEventListenerImpl.java
@@ -38,7 +38,6 @@
 import com.oracle.truffle.api.interop.UnsupportedMessageException;
 import com.oracle.truffle.espresso.jdwp.impl.BreakpointInfo;
 import com.oracle.truffle.espresso.jdwp.impl.ClassPrepareRequest;
-import com.oracle.truffle.espresso.jdwp.impl.DebuggerCommand;
 import com.oracle.truffle.espresso.jdwp.impl.DebuggerController;
 import com.oracle.truffle.espresso.jdwp.impl.FieldBreakpointEvent;
 import com.oracle.truffle.espresso.jdwp.impl.FieldBreakpointInfo;
@@ -532,13 +531,6 @@ public void stepCompleted(SteppingInfo info, CallFrame currentFrame) {
             stream.writeLong(currentFrame.getClassId());
             stream.writeLong(currentFrame.getMethodId());
             long codeIndex = currentFrame.getCodeIndex();
-            if (info.getStepKind() == DebuggerCommand.Kind.STEP_OUT) {
-                // Step out in Truffle is implemented on the callee exit, where the event's top
-                // stack frame is set to the caller frame. Hence, to avoid sending the code index
-                // from the caller location, we must fetch the next bci from the frame to pass the
-                // correct location.
-                codeIndex = context.getNextBCI(currentFrame.getRootNode(), currentFrame.getFrame());
-            }
             stream.writeLong(codeIndex);
             debuggerController.fine(() -> "Sending step completed event");
 
diff --git a/espresso/src/com.oracle.truffle.espresso.jdwp/src/com/oracle/truffle/espresso/jdwp/impl/DebuggerController.java b/espresso/src/com.oracle.truffle.espresso.jdwp/src/com/oracle/truffle/espresso/jdwp/impl/DebuggerController.java
@@ -127,7 +127,7 @@ public void initialize(Debugger debug, JDWPOptions jdwpOptions, JDWPContext jdwp
         ids.injectLogger(jdwpLogger);
 
         // set up the debug session object early to make sure instrumentable nodes are materialized
-        debuggerSession = debug.startSession(new SuspendedCallbackImpl(), SourceElement.ROOT, SourceElement.STATEMENT);
+        debuggerSession = debug.startSession(new SuspendedCallbackImpl(), SourceElement.STATEMENT);
         debuggerSession.setSteppingFilter(SuspensionFilter.newBuilder().ignoreLanguageContextInitialization(true).build());
 
         init(jdwpContext);
@@ -957,8 +957,8 @@ public void onSuspend(SuspendedEvent event) {
                     return;
                 }
             }
-            boolean isStepOut = steppingInfo != null && event.isStep() && steppingInfo.getStepKind() == DebuggerCommand.Kind.STEP_OUT;
-            CallFrame[] callFrames = createCallFrames(ids.getIdAsLong(currentThread), event.getStackFrames(), -1, isStepOut);
+            boolean isAfter = event.getSuspendAnchor() == SuspendAnchor.AFTER;
+            CallFrame[] callFrames = createCallFrames(ids.getIdAsLong(currentThread), event.getStackFrames(), -1, isAfter);
             RootNode callerRootNode = callFrames.length > 1 ? callFrames[1].getRootNode() : null;
 
             SuspendedInfo suspendedInfo = new SuspendedInfo(context, event, callFrames, currentThread, callerRootNode);
@@ -1179,7 +1179,7 @@ private void continueStepping(SuspendedEvent event, SteppingInfo steppingInfo) {
             }
         }
 
-        private CallFrame[] createCallFrames(long threadId, Iterable<DebugStackFrame> stackFrames, int frameLimit, boolean isStepOut) {
+        private CallFrame[] createCallFrames(long threadId, Iterable<DebugStackFrame> stackFrames, int frameLimit, boolean isAfter) {
             LinkedList<CallFrame> list = new LinkedList<>();
             int frameCount = 0;
             for (DebugStackFrame frame : stackFrames) {
@@ -1206,15 +1206,16 @@ private CallFrame[] createCallFrames(long threadId, Iterable<DebugStackFrame> st
 
                 Frame rawFrame = frame.getRawFrame(context.getLanguageClass(), FrameInstance.FrameAccess.READ_WRITE);
                 MethodVersionRef methodVersion = context.getMethodFromRootNode(root);
-                KlassRef klass = methodVersion.getMethod().getDeclaringKlassRef();
+                MethodRef method = methodVersion.getMethod();
+                KlassRef klass = method.getDeclaringKlassRef();
 
                 klassId = ids.getIdAsLong(klass);
-                methodId = ids.getIdAsLong(methodVersion.getMethod());
+                methodId = ids.getIdAsLong(method);
                 typeTag = TypeTag.getKind(klass);
-                if (isStepOut) {
-                    // Truffle reports step out at the callers entry to the method, so we must fetch
+                if (isAfter && frameCount == 0) {
+                    // Truffle reports anchor after this instruction, so we must fetch
                     // the BCI that follows to get the expected location within the frame.
-                    codeIndex = context.getNextBCI(root, rawFrame);
+                    codeIndex = context.getNextBCI(method, rawNode, rawFrame);
                 } else {
                     codeIndex = context.getBCI(rawNode, rawFrame);
                 }
diff --git a/espresso/src/com.oracle.truffle.espresso/src/com/oracle/truffle/espresso/impl/Method.java b/espresso/src/com.oracle.truffle.espresso/src/com/oracle/truffle/espresso/impl/Method.java
@@ -1390,7 +1390,7 @@ public int getLastLine() {
     }
 
     @Override
-    public long getLastBCI() {
+    public int getLastBCI() {
         int bci = 0;
         BytecodeStream bs = new BytecodeStream(getOriginalCode());
         int end = bs.endBCI();
diff --git a/espresso/src/com.oracle.truffle.espresso/src/com/oracle/truffle/espresso/runtime/JDWPContextImpl.java b/espresso/src/com.oracle.truffle.espresso/src/com/oracle/truffle/espresso/runtime/JDWPContextImpl.java
@@ -654,19 +654,21 @@ public int getCatchLocation(MethodRef method, Object guestException, int bci) {
     }
 
     @Override
-    public int getNextBCI(RootNode callerRoot, Frame frame) {
-        if (callerRoot instanceof EspressoRootNode espressoRootNode) {
-            int bci = (int) readBCIFromFrame(callerRoot, frame);
-            if (bci >= 0) {
-                BytecodeStream bs = new BytecodeStream(espressoRootNode.getMethodVersion().getOriginalCode());
-                return bs.nextBCI(bci);
+    public int getNextBCI(MethodRef method, Node rawNode, Frame frame) {
+        int bci = getBCI(rawNode, frame);
+        if (bci >= 0) {
+            BytecodeStream bs = new BytecodeStream(method.getOriginalCode());
+            int nextBci = bs.nextBCI(bci);
+            if (nextBci <= bs.endBCI()) {
+                // Use the next only if it's in bounds.
+                bci = nextBci;
             }
         }
-        return -1;
+        return bci;
     }
 
     @Override
-    public long readBCIFromFrame(RootNode root, Frame frame) {
+    public int readBCIFromFrame(RootNode root, Frame frame) {
         if (root instanceof EspressoRootNode rootNode && frame != null) {
             return rootNode.readBCI(frame);
         }
@@ -792,7 +794,7 @@ private BciProvider getBciProviderNode(Node node) {
         return null;
     }
 
-    public long getBCI(Node rawNode, Frame frame) {
+    public int getBCI(Node rawNode, Frame frame) {
         BciProvider bciProvider = getBciProviderNode(rawNode);
         if (bciProvider == null) {
             return -1;
diff --git a/tools/src/com.oracle.truffle.tools.chromeinspector.test/src/com/oracle/truffle/tools/chromeinspector/test/SLInspectDebugTest.java b/tools/src/com.oracle.truffle.tools.chromeinspector.test/src/com/oracle/truffle/tools/chromeinspector/test/SLInspectDebugTest.java
@@ -1322,7 +1322,7 @@ public void testRestartFrame() throws Exception {
                                                                  "{\"name\":\"global\",\"type\":\"global\",\"object\":{\"description\":\"global\",\"type\":\"object\",\"objectId\":\"10\"}}]," +
                                                  "\"this\":null," +
                                                  "\"functionLocation\":{\"scriptId\":\"1\",\"columnNumber\":9,\"lineNumber\":4}," +
-                                                 "\"location\":{\"scriptId\":\"1\",\"columnNumber\":28,\"lineNumber\":8}," +
+                                                 "\"location\":{\"scriptId\":\"1\",\"columnNumber\":12,\"lineNumber\":8}," +
                                                  "\"url\":\"" + srcURL + "\"}," +
                                                 "{\"callFrameId\":\"1\",\"functionName\":\"factorial\"," +
                                                  "\"scopeChain\":[{\"name\":\"factorial\",\"type\":\"local\",\"object\":{\"description\":\"factorial\",\"type\":\"object\",\"objectId\":\"11\"}}," +
@@ -1347,7 +1347,7 @@ public void testRestartFrame() throws Exception {
                                                                  "{\"name\":\"global\",\"type\":\"global\",\"object\":{\"description\":\"global\",\"type\":\"object\",\"objectId\":\"16\"}}]," +
                                                  "\"this\":null," +
                                                  "\"functionLocation\":{\"scriptId\":\"1\",\"columnNumber\":9,\"lineNumber\":0}," +
-                                                 "\"location\":{\"scriptId\":\"1\",\"columnNumber\":14,\"lineNumber\":2}," +
+                                                 "\"location\":{\"scriptId\":\"1\",\"columnNumber\":2,\"lineNumber\":2}," +
                                                  "\"url\":\"" + srcURL + "\"}]}," +
                         "\"id\":5}\n"));
         tester.sendMessage("{\"id\":6,\"method\":\"Debugger.resume\"}");
diff --git a/truffle/src/com.oracle.truffle.api.debug.test/src/com/oracle/truffle/api/debug/test/ReenterStackFrameTest.java b/truffle/src/com.oracle.truffle.api.debug.test/src/com/oracle/truffle/api/debug/test/ReenterStackFrameTest.java
@@ -42,6 +42,7 @@
 
 import com.oracle.truffle.api.debug.DebugStackFrame;
 import com.oracle.truffle.api.debug.DebuggerSession;
+import com.oracle.truffle.api.debug.SuspendAnchor;
 import com.oracle.truffle.api.debug.SuspendedEvent;
 import java.util.Iterator;
 import org.graalvm.polyglot.Source;
@@ -88,6 +89,8 @@ public void testReenterCurrent() throws Throwable {
             expectSuspended((SuspendedEvent event) -> {
                 DebugStackFrame currentFrame = event.getTopStackFrame();
                 try {
+                    // Assert that we're before the CALL again after unwind
+                    Assert.assertEquals(SuspendAnchor.BEFORE, event.getSuspendAnchor());
                     Assert.assertEquals("CALL(a)", currentFrame.getSourceSection().getCharacters());
                     Iterator<DebugStackFrame> frames = event.getStackFrames().iterator();
                     Assert.assertEquals("", frames.next().getName());
diff --git a/truffle/src/com.oracle.truffle.api.debug/src/com/oracle/truffle/api/debug/DebuggerSession.java b/truffle/src/com.oracle.truffle.api.debug/src/com/oracle/truffle/api/debug/DebuggerSession.java
@@ -1140,7 +1140,7 @@ private void notifyUnwindCallback(MaterializedFrame frame, InsertableNode insert
         // Fake the caller context
         Caller caller = findCurrentCaller(this, includeInternal);
         SuspendedContext context = SuspendedContext.create(caller.node, ((SteppingStrategy.Unwind) s).unwind);
-        doSuspend(context, SuspendAnchor.AFTER, caller.frame, insertableNode, false, true);
+        doSuspend(context, SuspendAnchor.BEFORE, caller.frame, insertableNode, false, true);
     }
 
     static Caller findCurrentCaller(DebuggerSession session, boolean includeInternal) {

Original file line number	Diff line number	Diff line change
`@@ -231,7 +231,7 @@ public interface MethodRef {`
`231`	`231`	`*`
`232`	`232`	`* @return last bci`
`233`	`233`	`*/`
`234`		`- long getLastBCI();`
	`234`	`+ int getLastBCI();`
`235`	`235`
`236`	`236`	`/**`
`237`	`237`	`* Determines if the method is a constructor.`
Original file line number	Diff line number	Diff line change
`@@ -1390,7 +1390,7 @@ public int getLastLine() {`
`1390`	`1390`	`}`
`1391`	`1391`
`1392`	`1392`	`@Override`
`1393`		`- public long getLastBCI() {`
	`1393`	`+ public int getLastBCI() {`
`1394`	`1394`	`int bci = 0;`
`1395`	`1395`	`BytecodeStream bs = new BytecodeStream(getOriginalCode());`
`1396`	`1396`	`int end = bs.endBCI();`
Original file line number	Diff line number	Diff line change
`@@ -1140,7 +1140,7 @@ private void notifyUnwindCallback(MaterializedFrame frame, InsertableNode insert`
`1140`	`1140`	`// Fake the caller context`
`1141`	`1141`	`Caller caller = findCurrentCaller(this, includeInternal);`
`1142`	`1142`	`SuspendedContext context = SuspendedContext.create(caller.node, ((SteppingStrategy.Unwind) s).unwind);`
`1143`		`- doSuspend(context, SuspendAnchor.AFTER, caller.frame, insertableNode, false, true);`
	`1143`	`+ doSuspend(context, SuspendAnchor.BEFORE, caller.frame, insertableNode, false, true);`
`1144`	`1144`	`}`
`1145`	`1145`
`1146`	`1146`	`static Caller findCurrentCaller(DebuggerSession session, boolean includeInternal) {`