[lldb] Fix stepping out if the return address is not allowed to stop at

[igorkudrin]

When a thread reaches a breakpoint at the return address set by ThreadPlanStepOut, ThreadPlanStepOut::ShouldStop() calls ThreadPlanShouldStopHere::InvokeShouldStopHereCallback(), and if it returns false, ThreadPlanShouldStopHere::QueueStepOutFromHerePlan() is called to queue a new plan to skip the corresponding range. Once the new plan finishes, ThreadPlanStepOut::ShouldStop() should recheck the stop condition; however, there is no code path in the method that sets done to true. Before #126838, the method checked if it had reached a suitable frame. After the patch, the check is only performed at a breakpoint; thus, the execution continues.

This patch causes ThreadPlanStepOut::ShouldStop() to recheck the stop condition when m_step_out_further_plan_sp completes.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Igor Kudrin (igorkudrin)

Changes

When a thread reaches a breakpoint at the return address set by ThreadPlanStepOut, ThreadPlanStepOut::ShouldStop() calls ThreadPlanShouldStopHere::InvokeShouldStopHereCallback(), and if it returns false, ThreadPlanShouldStopHere::QueueStepOutFromHerePlan() is called to queue a new plan to skip the corresponding range. Once the new plan finishes, ThreadPlanStepOut::ShouldStop() should recheck the stop condition; however, there is no branch that sets done to true. Before #126838, the method checked if it had reached a suitable frame. After the patch, the check is only performed at a breakpoint; thus, the execution continues.

This patch causes ThreadPlanStepOut::ShouldStop() to recheck the stop condition when m_step_out_further_plan_sp completes.

---

Full diff: https://github.com/llvm/llvm-project/pull/161982.diff

4 Files Affected:

• (modified) lldb/source/Target/ThreadPlanStepOut.cpp (+3-2)
• (added) lldb/test/API/functionalities/thread/step_out_line0/Makefile (+3)
• (added) lldb/test/API/functionalities/thread/step_out_line0/TestThreadStepOutLine0.py (+35)
• (added) lldb/test/API/functionalities/thread/step_out_line0/main.c (+11)

diff --git a/lldb/source/Target/ThreadPlanStepOut.cpp b/lldb/source/Target/ThreadPlanStepOut.cpp
index 0628451a5abf9..d49a01bdbcef7 100644
--- a/lldb/source/Target/ThreadPlanStepOut.cpp
+++ b/lldb/source/Target/ThreadPlanStepOut.cpp
@@ -397,9 +397,10 @@ bool ThreadPlanStepOut::ShouldStop(Event *event_ptr) {
     else
       return m_step_through_inline_plan_sp->ShouldStop(event_ptr);
   } else if (m_step_out_further_plan_sp) {
-    if (m_step_out_further_plan_sp->MischiefManaged())
+    if (m_step_out_further_plan_sp->MischiefManaged()) {
       m_step_out_further_plan_sp.reset();
-    else
+      done = true;
+    } else
       return m_step_out_further_plan_sp->ShouldStop(event_ptr);
   }
 
diff --git a/lldb/test/API/functionalities/thread/step_out_line0/Makefile b/lldb/test/API/functionalities/thread/step_out_line0/Makefile
new file mode 100644
index 0000000000000..10495940055b6
--- /dev/null
+++ b/lldb/test/API/functionalities/thread/step_out_line0/Makefile
@@ -0,0 +1,3 @@
+C_SOURCES := main.c
+
+include Makefile.rules
diff --git a/lldb/test/API/functionalities/thread/step_out_line0/TestThreadStepOutLine0.py b/lldb/test/API/functionalities/thread/step_out_line0/TestThreadStepOutLine0.py
new file mode 100644
index 0000000000000..2707ca852f662
--- /dev/null
+++ b/lldb/test/API/functionalities/thread/step_out_line0/TestThreadStepOutLine0.py
@@ -0,0 +1,35 @@
+"""
+Test stepping out of a function when the return location is an unsuitable
+stopping point.
+"""
+
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class ThreadStepOutLine0TestCase(TestBase):
+    def test(self):
+        self.build()
+        target, process, thread, _ = lldbutil.run_to_source_breakpoint(
+            self, "// Set breakpoint here", lldb.SBFileSpec("main.c")
+        )
+        correct_stepped_out_line = line_number("main.c", "// Should stop here")
+        return_statement_line = line_number("main.c", "// Ran too far")
+        safety_bp = target.BreakpointCreateByLocation(
+            lldb.SBFileSpec("main.c"), return_statement_line
+        )
+        self.assertTrue(safety_bp.IsValid())
+
+        error = lldb.SBError()
+        thread.StepOut(error)
+        self.assertTrue(error.Success())
+
+        frame = thread.GetSelectedFrame()
+        self.assertEqual(
+            frame.line_entry.GetLine(),
+            correct_stepped_out_line,
+            "Step-out lost control of execution, ran too far",
+        )
diff --git a/lldb/test/API/functionalities/thread/step_out_line0/main.c b/lldb/test/API/functionalities/thread/step_out_line0/main.c
new file mode 100644
index 0000000000000..ad0c1c05d6952
--- /dev/null
+++ b/lldb/test/API/functionalities/thread/step_out_line0/main.c
@@ -0,0 +1,11 @@
+int step_out_of_here(int a) {
+  return a + 5; // Set breakpoint here
+}
+
+int main() {
+#line 0
+  int v = step_out_of_here(3) + 7;
+#line 9
+  v += 11;  // Should stop here
+  return v; // Ran too far
+}

[jimingham]

We had to fix this in another place just recently, this is the right thing to do. Thanks for the test!