[Lldb-commits] [lldb] [lldb] Update ThreadPlanStepOut to handle new breakpoint behavior (PR #126838)
Jason Molenda via lldb-commits
lldb-commits at lists.llvm.org
Wed Feb 12 13:47:58 PST 2025
https://github.com/jasonmolenda updated https://github.com/llvm/llvm-project/pull/126838
>From 4bc364c7217ad43b886ece04f4ee00e5b0a03ce3 Mon Sep 17 00:00:00 2001
From: Jason Molenda <jmolenda at apple.com>
Date: Tue, 11 Feb 2025 18:01:33 -0800
Subject: [PATCH 1/2] [lldb] Update ThreadPlanStepOut to handle new breakpoint
behavior
I will be changing breakpoint hitting behavior soon, where currently
lldb reports a breakpoint as being hit when a thread is *at* a
BreakpointSite, but possibly has not executed the breakpoint
instruction and trapped yet, to having lldb only report a breakpoint
hit when the breakpoint instruction has actually been executed.
One corner case bug with this change is that when you are stopped
at a breakpoint (that has been hit) on the last instruction of a
function, and you do `finish`, a ThreadPlanStepOut is pushed to the
thread's plan stack to put a breakpoint on the return address and
resume execution. And when the thread is asked to resume, it sees
that it is at a BreakpointSite that has been hit, and pushes a
ThreadPlanStepOverBreakpoint on the thread. The StepOverBreakpoint
plan sees that the thread's state is eStateRunning (not eStateStepping),
so it marks itself as "auto continue" -- so once the breakpoint has
been stepped over, we will execution on the thread.
With current lldb stepping behavior ("a thread *at* a BreakpointSite
is said to have stopped with a breakpoint-hit stop reason, even if
the breakpoint hasn't been executed yet"),
`ThreadPlanStepOverBreakpoint::DoPlanExplainsStop` has a special
bit of code which detects when the thread stops with a
eStopReasonBreakpoint. It first checks if the pc is the same as
when we started -- did our "step instruction" not actually step?
-- says the stop reason is explained. Otherwise it sets auto-continue
to false (because we've hit an *unexpected* breakpoint, and we have
advanced past our original pc, and returns false - the stop reason
is not explained.
So we do the "finish", lldb instruction steps, we stop *at* the
return-address breakpoint and lldb sets the thread's stop reason
to breakpoint-hit. ThreadPlanStepOverBreakpoint sees an
eStopReasonBreakpoint, sets its auto-continue to false, and says
we stopped for osme reason other than this plan. (and it will also
report `IsPlanStale()==true` so it will remove itself) Meanwhile
the ThreadPlanStepOut sees that it has stopped in the StackID it
wanted to run to, and return success.
This all changes when stopping at a breakpoint site doesn't report
breakpoint-hit until we actually execute the instruction. Now the
ThraedPlanStepOverBreakpoint looks at the thread's stop reason,
it's eStopReasonTrace (we've instruction stepped), and so it leaves
its auto-continue to `true`. ThreadPlanStepOut sees that it has
reached its goal StackID, removes its breakpoint, and says it is done.
Thread::ShouldStop thinks the auto-continue == yes vote from
ThreadPlanStepOverBreakpoint wins, and we lose control of the process.
This patch changes ThreadPlanStepOut to require that *both* (1)
we are at the StackID of the caller function, where we wanted to
end up, and (2) we have actually hit the breakpoint that we inserted.
This in effect means that now lldb instruction-steps over the breakpoint
in the callee function, stops at the return address of the caller
function. StepOverBreakpoint has completed. StepOut is still running,
and we continue the thread again. We immediatley hit the breakpoint
(that we're sitting at), and now ThreadPlanStepOut marks itself as
completed, and we return control to the user.
Jim suggests that ThreadPlanStepOverBreakpoint is a bit unusual
because it's not something pushed on the stack by a higher-order
thread plan that "owns" it, it is inserted by the Thread as it is
about to resume, if we're at a BreakpointSite. It has no connection
to the thread plans above it, but tries to set the auto-continue
mode based on the state of the thread when it is inserted (and tries
to detect an unexpected breakpoint and unset that auto-continue it
previously decided on, because it now realizes it should not influence
execution control any more). Instead maybe the
ThreadPlanStepOverBreakpoint should be inserted as a child plan of
whatever the lowest plan is on the stack at the point it is added.
I added an API test that will catch this bug in the new thread
breakpoint algorithm.
---
lldb/source/Target/ThreadPlanStepOut.cpp | 8 +++-
.../thread/finish-from-empty-func/Makefile | 3 ++
.../TestEmptyFuncThreadStepOut.py | 43 +++++++++++++++++++
.../thread/finish-from-empty-func/main.c | 8 ++++
4 files changed, 60 insertions(+), 2 deletions(-)
create mode 100644 lldb/test/API/functionalities/thread/finish-from-empty-func/Makefile
create mode 100644 lldb/test/API/functionalities/thread/finish-from-empty-func/TestEmptyFuncThreadStepOut.py
create mode 100644 lldb/test/API/functionalities/thread/finish-from-empty-func/main.c
diff --git a/lldb/source/Target/ThreadPlanStepOut.cpp b/lldb/source/Target/ThreadPlanStepOut.cpp
index c0ea53e4a8cbb..2376b52cfc03a 100644
--- a/lldb/source/Target/ThreadPlanStepOut.cpp
+++ b/lldb/source/Target/ThreadPlanStepOut.cpp
@@ -364,8 +364,12 @@ bool ThreadPlanStepOut::ShouldStop(Event *event_ptr) {
}
if (!done) {
- StackID frame_zero_id = GetThread().GetStackFrameAtIndex(0)->GetStackID();
- done = !(frame_zero_id < m_step_out_to_id);
+ StopInfoSP stop_info_sp = GetPrivateStopInfo();
+ if (stop_info_sp.get() &&
+ stop_info_sp->GetStopReason() == eStopReasonBreakpoint) {
+ StackID frame_zero_id = GetThread().GetStackFrameAtIndex(0)->GetStackID();
+ done = !(frame_zero_id < m_step_out_to_id);
+ }
}
// The normal step out computations think we are done, so all we need to do
diff --git a/lldb/test/API/functionalities/thread/finish-from-empty-func/Makefile b/lldb/test/API/functionalities/thread/finish-from-empty-func/Makefile
new file mode 100644
index 0000000000000..10495940055b6
--- /dev/null
+++ b/lldb/test/API/functionalities/thread/finish-from-empty-func/Makefile
@@ -0,0 +1,3 @@
+C_SOURCES := main.c
+
+include Makefile.rules
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
new file mode 100644
index 0000000000000..bf57070e336e7
--- /dev/null
+++ b/lldb/test/API/functionalities/thread/finish-from-empty-func/TestEmptyFuncThreadStepOut.py
@@ -0,0 +1,43 @@
+"""
+Test finish out of an empty function (may be one-instruction long)
+"""
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class FinishFromEmptyFunctionTestCase(TestBase):
+ NO_DEBUG_INFO_TESTCASE = True
+
+ 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
+ )
+ if self.TraceOn():
+ self.runCmd("bt")
+
+ correct_stepped_out_line = line_number("main.c", "leaving main")
+ return_statement_line = line_number("main.c", "return 0")
+ 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())
+
+ if self.TraceOn():
+ self.runCmd("bt")
+
+ 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/finish-from-empty-func/main.c b/lldb/test/API/functionalities/thread/finish-from-empty-func/main.c
new file mode 100644
index 0000000000000..bc66a548a89df
--- /dev/null
+++ b/lldb/test/API/functionalities/thread/finish-from-empty-func/main.c
@@ -0,0 +1,8 @@
+#include <stdio.h>
+void done() {}
+int main() {
+ puts("in main");
+ done();
+ puts("leaving main");
+ return 0;
+}
>From 515d25784625702c711d2eb0d42c4c2ba75e9d7a Mon Sep 17 00:00:00 2001
From: Jason Molenda <jmolenda at apple.com>
Date: Wed, 12 Feb 2025 13:47:41 -0800
Subject: [PATCH 2/2] nit
---
lldb/source/Target/ThreadPlanStepOut.cpp | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/lldb/source/Target/ThreadPlanStepOut.cpp b/lldb/source/Target/ThreadPlanStepOut.cpp
index 2376b52cfc03a..95e51408dbcdc 100644
--- a/lldb/source/Target/ThreadPlanStepOut.cpp
+++ b/lldb/source/Target/ThreadPlanStepOut.cpp
@@ -365,8 +365,7 @@ bool ThreadPlanStepOut::ShouldStop(Event *event_ptr) {
if (!done) {
StopInfoSP stop_info_sp = GetPrivateStopInfo();
- if (stop_info_sp.get() &&
- stop_info_sp->GetStopReason() == eStopReasonBreakpoint) {
+ if (stop_info && stop_info_sp->GetStopReason() == eStopReasonBreakpoint) {
StackID frame_zero_id = GetThread().GetStackFrameAtIndex(0)->GetStackID();
done = !(frame_zero_id < m_step_out_to_id);
}
More information about the lldb-commits
mailing list