[lldb][test] Fix TestEmptyFuncThreadStepOut.py

[igorkudrin]

The test did not work as intended when the empty function 'done()' contained prologue/epilog code, because a breakpoint was set before the last instruction of the function, which caused the test to pass even with the fix from #126838 having been reverted.

The test was intended to check a case when a breakpoint is set on a return instruction, which is the very last instruction of a function. When stepping out from this breakpoint, there is some interaction between ThreadPlanStepOut and ThreadPlanStepOverBreakpoint that could lead to missing the stop location in the outer frame; the detailed explanation can be found in #126838.

On Linux/AArch64, the source is compiled into:

> objdump -d main.o
0000000000000000 <done>:
   0:   d65f03c0        ret

So, when the command bt set -n done from the original test sets a breakpoint to the first instruction of done(), this instruction luckily also happens to be the last one.

On Linux/x86_64, it compiles into:

> objdump -d main.o
0000000000000000 <done>:
   0:   55                      push   %rbp
   1:   48 89 e5                mov    %rsp,%rbp
   4:   5d                      pop    %rbp
   5:   c3                      ret

And in this case, setting a breakpoint by a function name means setting it several instructions before ret, which does not provoke the interaction between ThreadPlanStepOut and ThreadPlanStepOverBreakpoint.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Igor Kudrin (igorkudrin)

Changes

The test did not work as intended when the empty function 'done()' contained epilog code, because a breakpoint was set on the first instruction of the epilog instead of on the last instruction of the function. This caused the test to pass even with the fix from #126838 reverted.

---

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

2 Files Affected:

• (modified) lldb/test/API/functionalities/thread/finish-from-empty-func/TestEmptyFuncThreadStepOut.py (+16-4)
• (modified) lldb/test/API/functionalities/thread/finish-from-empty-func/main.c (+1)

diff --git a/lldb/test/API/functionalities/thread/finish-from-empty-func/TestEmptyFuncThreadStepOut.py b/lldb/test/API/functionalities/thread/finish-from-empty-func/TestEmptyFuncThreadStepOut.py
index f5d3da530f4f5..be0d47d1d8fe6 100644
--- a/lldb/test/API/functionalities/thread/finish-from-empty-func/TestEmptyFuncThreadStepOut.py
+++ b/lldb/test/API/functionalities/thread/finish-from-empty-func/TestEmptyFuncThreadStepOut.py
@@ -15,10 +15,23 @@ class FinishFromEmptyFunctionTestCase(TestBase):
     def test_finish_from_empty_function(self):
         """Test that when stopped at a breakpoint in an empty function, finish leaves it correctly."""
         self.build()
-        exe = self.getBuildArtifact("a.out")
-        target, process, thread, _ = lldbutil.run_to_name_breakpoint(
-            self, "done", exe_name=exe
+        target, _, thread, _ = lldbutil.run_to_source_breakpoint(
+            self, "// Set breakpoint here", lldb.SBFileSpec("main.c")
         )
+        # Find the last instruction address of 'done()' and set a breakpoint there.
+        error = lldb.SBError()
+        ret_bp_addr = lldb.SBAddress()
+        while True:
+            thread.StepInstruction(False, error)
+            self.assertTrue(error.Success())
+            frame = thread.GetSelectedFrame()
+            if "done" in frame.GetFunctionName():
+                ret_bp_addr = frame.GetPCAddress()
+            elif ret_bp_addr.IsValid():
+                break
+        ret_bp = target.BreakpointCreateByAddress(ret_bp_addr.GetLoadAddress(target))
+        self.assertTrue(ret_bp.IsValid())
+        self.runCmd("cont")
         if self.TraceOn():
             self.runCmd("bt")
 
@@ -29,7 +42,6 @@ def test_finish_from_empty_function(self):
         )
         self.assertTrue(safety_bp.IsValid())
 
-        error = lldb.SBError()
         thread.StepOut(error)
         self.assertTrue(error.Success())
 
diff --git a/lldb/test/API/functionalities/thread/finish-from-empty-func/main.c b/lldb/test/API/functionalities/thread/finish-from-empty-func/main.c
index bc66a548a89df..b3f90db5e2562 100644
--- a/lldb/test/API/functionalities/thread/finish-from-empty-func/main.c
+++ b/lldb/test/API/functionalities/thread/finish-from-empty-func/main.c
@@ -2,6 +2,7 @@
 void done() {}
 int main() {
   puts("in main");
+  done(); // Set breakpoint here
   done();
   puts("leaving main");
   return 0;

[igorkudrin]

Ping

[DavidSpickett]

I never looked at the original fix, but the changes make sense given what you're trying to do.

I would have thought finish from the epilogue had the same issue as from the "body" of the function but you've apparently proven that not to be the case.

[igorkudrin]

I would have thought finish from the epilogue had the same issue as from the "body" of the function but you've apparently proven that not to be the case.

Perhaps I was not clear enough. As I understand it, the original test was supposed to demonstrate an issue with stepping out from the last instruction of a function after hitting a breakpoint at that location. Probably, prologue and epilogue code are not generated for empty functions on some targets, but it is on Linux/x64, for example.

[DavidSpickett]

Probably, prologue and epilogue code are not generated for empty functions on some targets, but it is on Linux/x64, for example.

Ok so if the function did not have an epilogue, the test did what it claimed to. If it did, the breakpoint got placed before the epilogue because that's the last "user visible" place for it.

I understand the fix now.

I was thinking maybe the original author thought that epilogues were included in the function's range in the debug info but were not (I assume they in fact are), but that's nothing to do with this fix.

[DavidSpickett]

because a breakpoint was set before the last instruction of the function

Expand this a bit. Like "Meaning, before the epilogue of the function."

[DavidSpickett]

LGTM

[igorkudrin]

because a breakpoint was set before the last instruction of the function

Expand this a bit. Like "Meaning, before the epilogue of the function."

I've expanded the description; hope it is clearer now. I don't think mentioning "epilogue" would improve it. Probably, my original description, which mentioned it, was too confusing.

Actually, we can set a breakpoint at the first instruction of the epilogue by rewriting the source like:

void done() {
  return; // Set breakpoint here
}

With br set -p "// Set breakpoint here", a breakpoint can be set at the start of the epilogue:

> objdump -dl main.o
0000000000000000 <done>:
done():
.../main.c:2
   0:   55                      push   %rbp
   1:   48 89 e5                mov    %rsp,%rbp
.../main.c:3
   4:   5d                      pop    %rbp
   5:   c3                      ret
   6:   66 2e 0f 1f 84 00 00    cs nopw 0x0(%rax,%rax,1)
   d:   00 00 00

But that would not help much, because it is still not the last instruction of the function and does not provoke interaction between thread plans.

[DavidSpickett]

So, when the command bt set -n done from the original test sets a breakpoint to the first instruction of done(), this instruction luckily also happens to be the last one.

Ok now it's blindingly obvious to me :)

(I'm sure your previous explanation wasn't bad, but not being familiar with the original issue it took me a while to understand)

[DavidSpickett]

LGTM