SBThread::StepInstruction shouldn't discard other plans #97493

jimingham · 2024-07-03T00:16:26Z

This was just a typo, none of the external execution control functions should discard other plans. In particular, it means if you stop in a hand-called function and step an instruction, the function call thread plan gets unshipped, popping all the function call frames.

I also added a test that asserts the correct behavior. I tested all the stepping operations even though only StepInstruction was wrong.

This was just a typo, none of the external execution control functions should discard other plans. In particular, it means if you stop in a hand-called function and step an instruction, the function call thread plan gets unshipped, popping all the function call frames. I also added a test that asserts the correct behavior. I tested all the stepping operations even though only StepInstruction was wrong.

llvmbot · 2024-07-03T00:16:59Z

@llvm/pr-subscribers-lldb

Author: None (jimingham)

Changes

This was just a typo, none of the external execution control functions should discard other plans. In particular, it means if you stop in a hand-called function and step an instruction, the function call thread plan gets unshipped, popping all the function call frames.

I also added a test that asserts the correct behavior. I tested all the stepping operations even though only StepInstruction was wrong.

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

3 Files Affected:

(modified) lldb/source/API/SBThread.cpp (+1-1)
(modified) lldb/test/API/python_api/thread/TestThreadAPI.py (+28)
(modified) lldb/test/API/python_api/thread/main.cpp (+10)

diff --git a/lldb/source/API/SBThread.cpp b/lldb/source/API/SBThread.cpp
index ac3e2cd25daa9..53643362421d4 100644
--- a/lldb/source/API/SBThread.cpp
+++ b/lldb/source/API/SBThread.cpp
@@ -722,7 +722,7 @@ void SBThread::StepInstruction(bool step_over, SBError &error) {
   Thread *thread = exe_ctx.GetThreadPtr();
   Status new_plan_status;
   ThreadPlanSP new_plan_sp(thread->QueueThreadPlanForStepSingleInstruction(
-      step_over, true, true, new_plan_status));
+      step_over, false, true, new_plan_status));
 
   if (new_plan_status.Success())
     error = ResumeNewPlan(exe_ctx, new_plan_sp.get());
diff --git a/lldb/test/API/python_api/thread/TestThreadAPI.py b/lldb/test/API/python_api/thread/TestThreadAPI.py
index 0fe91c88c325e..05ae3c8d36cf0 100644
--- a/lldb/test/API/python_api/thread/TestThreadAPI.py
+++ b/lldb/test/API/python_api/thread/TestThreadAPI.py
@@ -52,6 +52,11 @@ def test_negative_indexing(self):
         self.build()
         self.validate_negative_indexing()
 
+    def test_StepInstruction(self):
+        """Test that StepInstruction preserves the plan stack."""
+        self.build()
+        self.step_instruction_in_called_function()
+
     def setUp(self):
         # Call super's setUp().
         TestBase.setUp(self)
@@ -303,3 +308,26 @@ def validate_negative_indexing(self):
         neg_range = range(thread.num_frames, 0, -1)
         for pos, neg in zip(pos_range, neg_range):
             self.assertEqual(thread.frame[pos].idx, thread.frame[-neg].idx)
+
+    def step_instruction_in_called_function(self):
+        main_file_spec = lldb.SBFileSpec("main.cpp")
+        target, process, thread, bkpt = lldbutil.run_to_source_breakpoint(self, "Set break point at this line", main_file_spec)
+        options = lldb.SBExpressionOptions()
+        options.SetIgnoreBreakpoints(False)
+
+        call_me_bkpt = target.BreakpointCreateBySourceRegex("Set a breakpoint in call_me",
+                                                            main_file_spec)
+        self.assertGreater(call_me_bkpt.GetNumLocations(), 0, "Got at least one location in call_me")
+        # Now run the expression, this will fail because we stopped at a breakpoint:
+        self.runCmd('expr -i 0 -- call_me(true)', check=False)
+        # Now we should be stopped in call_me:
+        self.assertEqual(thread.frames[0].name, "call_me(bool)", "Stopped in call_me(bool)")
+        # Now do a various API steps.  These should not cause the expression context to get unshipped:
+        thread.StepInstruction(False)
+        self.assertEqual(thread.frames[0].name, "call_me(bool)", "Still in call_me(bool) after StepInstruction")
+        thread.StepInstruction(True)
+        self.assertEqual(thread.frames[0].name, "call_me(bool)", "Still in call_me(bool) after NextInstruction")
+        thread.StepInto()
+        self.assertEqual(thread.frames[0].name, "call_me(bool)", "Still in call_me(bool) after StepInto")
+        thread.StepOver(False)
+        self.assertEqual(thread.frames[0].name, "call_me(bool)", "Still in call_me(bool) after StepOver")
diff --git a/lldb/test/API/python_api/thread/main.cpp b/lldb/test/API/python_api/thread/main.cpp
index dde740a1b6bf6..d4b0ad2372c3d 100644
--- a/lldb/test/API/python_api/thread/main.cpp
+++ b/lldb/test/API/python_api/thread/main.cpp
@@ -5,8 +5,18 @@
 char my_char = 'u';
 int my_int = 0;
 
+void
+call_me(bool should_spin) {
+    int counter = 0;
+    if (should_spin) {
+        while (1)
+            counter++;  // Set a breakpoint in call_me
+     }
+}
+
 int main (int argc, char const *argv[])
 {
+    call_me(false);
     for (int i = 0; i < 3; ++i) {
         printf("my_char='%c'\n", my_char);
         ++my_char;

github-actions · 2024-07-03T00:20:05Z

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff 57555c6a0a96790bf1408b056405abe07899ead4 3fba16eaee25b1d63907640b79019309e9c019a7 -- lldb/source/API/SBThread.cpp lldb/test/API/python_api/thread/main.cpp

View the diff from clang-format here.

diff --git a/lldb/test/API/python_api/thread/main.cpp b/lldb/test/API/python_api/thread/main.cpp
index d4b0ad2372..f36838a6a7 100644
--- a/lldb/test/API/python_api/thread/main.cpp
+++ b/lldb/test/API/python_api/thread/main.cpp
@@ -5,21 +5,20 @@
 char my_char = 'u';
 int my_int = 0;
 
-void
-call_me(bool should_spin) {
-    int counter = 0;
-    if (should_spin) {
-        while (1)
-            counter++;  // Set a breakpoint in call_me
-     }
+void call_me(bool should_spin) {
+  int counter = 0;
+  if (should_spin) {
+    while (1)
+      counter++; // Set a breakpoint in call_me
+  }
 }
 
 int main (int argc, char const *argv[])
 {
-    call_me(false);
-    for (int i = 0; i < 3; ++i) {
-        printf("my_char='%c'\n", my_char);
-        ++my_char;
+  call_me(false);
+  for (int i = 0; i < 3; ++i) {
+    printf("my_char='%c'\n", my_char);
+    ++my_char;
     }
 
     printf("after the loop: my_char='%c'\n", my_char); // 'my_char' should print out as 'x'.

github-actions · 2024-07-03T00:20:05Z

⚠️ Python code formatter, darker found issues in your code. ⚠️

You can test this locally with the following command:

darker --check --diff -r 57555c6a0a96790bf1408b056405abe07899ead4...3fba16eaee25b1d63907640b79019309e9c019a7 lldb/test/API/python_api/thread/TestThreadAPI.py

View the diff from darker here.

--- TestThreadAPI.py	2024-07-02 23:42:00.000000 +0000
+++ TestThreadAPI.py	2024-07-03 00:19:47.133473 +0000
@@ -309,25 +309,48 @@
         for pos, neg in zip(pos_range, neg_range):
             self.assertEqual(thread.frame[pos].idx, thread.frame[-neg].idx)
 
     def step_instruction_in_called_function(self):
         main_file_spec = lldb.SBFileSpec("main.cpp")
-        target, process, thread, bkpt = lldbutil.run_to_source_breakpoint(self, "Set break point at this line", main_file_spec)
+        target, process, thread, bkpt = lldbutil.run_to_source_breakpoint(
+            self, "Set break point at this line", main_file_spec
+        )
         options = lldb.SBExpressionOptions()
         options.SetIgnoreBreakpoints(False)
 
-        call_me_bkpt = target.BreakpointCreateBySourceRegex("Set a breakpoint in call_me",
-                                                            main_file_spec)
-        self.assertGreater(call_me_bkpt.GetNumLocations(), 0, "Got at least one location in call_me")
+        call_me_bkpt = target.BreakpointCreateBySourceRegex(
+            "Set a breakpoint in call_me", main_file_spec
+        )
+        self.assertGreater(
+            call_me_bkpt.GetNumLocations(), 0, "Got at least one location in call_me"
+        )
         # Now run the expression, this will fail because we stopped at a breakpoint:
-        self.runCmd('expr -i 0 -- call_me(true)', check=False)
+        self.runCmd("expr -i 0 -- call_me(true)", check=False)
         # Now we should be stopped in call_me:
-        self.assertEqual(thread.frames[0].name, "call_me(bool)", "Stopped in call_me(bool)")
+        self.assertEqual(
+            thread.frames[0].name, "call_me(bool)", "Stopped in call_me(bool)"
+        )
         # Now do a various API steps.  These should not cause the expression context to get unshipped:
         thread.StepInstruction(False)
-        self.assertEqual(thread.frames[0].name, "call_me(bool)", "Still in call_me(bool) after StepInstruction")
+        self.assertEqual(
+            thread.frames[0].name,
+            "call_me(bool)",
+            "Still in call_me(bool) after StepInstruction",
+        )
         thread.StepInstruction(True)
-        self.assertEqual(thread.frames[0].name, "call_me(bool)", "Still in call_me(bool) after NextInstruction")
+        self.assertEqual(
+            thread.frames[0].name,
+            "call_me(bool)",
+            "Still in call_me(bool) after NextInstruction",
+        )
         thread.StepInto()
-        self.assertEqual(thread.frames[0].name, "call_me(bool)", "Still in call_me(bool) after StepInto")
+        self.assertEqual(
+            thread.frames[0].name,
+            "call_me(bool)",
+            "Still in call_me(bool) after StepInto",
+        )
         thread.StepOver(False)
-        self.assertEqual(thread.frames[0].name, "call_me(bool)", "Still in call_me(bool) after StepOver")
+        self.assertEqual(
+            thread.frames[0].name,
+            "call_me(bool)",
+            "Still in call_me(bool) after StepOver",
+        )

Michael137

LGTM, thanks for fixing this

TestThreadAPI.py test_StepInstruction started failing after #97493 Following assertion fails but I am not sure if test will pass after changing the test. AssertionError: 'void __cdecl call_me(bool)' != 'call_me(bool)' I have marked it as xfail I ll run it on a Windows machine to find an appropriate fix. https://lab.llvm.org/buildbot/#/builders/141/builds/476

This was just a typo, none of the external execution control functions should discard other plans. In particular, it means if you stop in a hand-called function and step an instruction, the function call thread plan gets unshipped, popping all the function call frames. I also added a test that asserts the correct behavior. I tested all the stepping operations even though only StepInstruction was wrong.

TestThreadAPI.py test_StepInstruction started failing after llvm#97493 Following assertion fails but I am not sure if test will pass after changing the test. AssertionError: 'void __cdecl call_me(bool)' != 'call_me(bool)' I have marked it as xfail I ll run it on a Windows machine to find an appropriate fix. https://lab.llvm.org/buildbot/#/builders/141/builds/476

jimingham requested a review from JDevlieghere as a code owner July 3, 2024 00:16

llvmbot added the lldb label Jul 3, 2024

jimingham requested a review from Michael137 July 3, 2024 00:17

Michael137 approved these changes Jul 3, 2024

View reviewed changes

Make it uglier

55c7e74

jimingham merged commit 845dee3 into llvm:main Jul 3, 2024
3 of 5 checks passed

jimingham deleted the sbthread-step-instruction branch July 3, 2024 17:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

SBThread::StepInstruction shouldn't discard other plans #97493

SBThread::StepInstruction shouldn't discard other plans #97493

Uh oh!

jimingham commented Jul 3, 2024

Uh oh!

llvmbot commented Jul 3, 2024

Uh oh!

github-actions bot commented Jul 3, 2024

Uh oh!

github-actions bot commented Jul 3, 2024

Uh oh!

Michael137 left a comment

Uh oh!

Uh oh!

Uh oh!

SBThread::StepInstruction shouldn't discard other plans #97493

SBThread::StepInstruction shouldn't discard other plans #97493

Uh oh!

Conversation

jimingham commented Jul 3, 2024

Uh oh!

llvmbot commented Jul 3, 2024

Uh oh!

github-actions bot commented Jul 3, 2024

Uh oh!

github-actions bot commented Jul 3, 2024

Uh oh!

Michael137 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!