[lldb] Update ThreadPlanStepOut to handle new breakpoint behavior #126838
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
Member
|
@llvm/pr-subscribers-lldb Author: Jason Molenda (jasonmolenda) ChangesI 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 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"), 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 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 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 I added an API test that will catch this bug in the new thread breakpoint algorithm. Full diff: https://github.com/llvm/llvm-project/pull/126838.diff 4 Files Affected:
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;
+}
|
Collaborator
Author
|
This was the last of the issues that were hit (found via a test by @mstorsjo ) for this PR from July 2024 #96260 I've been iterating on that PR to address all of the issues, but have landed all of the fixes as separate PRs on llvm-project main, I've been using #105594 to track those fixes (and I developed them all there as I was working on it) I'll probably create a clean PR (which is basically just a rebasing of the July 2024 patch) once this has landed and all known issues have been addressed on llvm-project main. |
JDevlieghere
approved these changes
Feb 12, 2025
| 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() && |
Member
There was a problem hiding this comment.
Suggested change
| if (stop_info_sp.get() && | |
| if (stop_info_sp && |
jasonmolenda
deleted the
thread-step-out-work-correctly-with-new-breakpoint-behavior
branch
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/181/builds/13554 Here is the relevant piece of the build log for the reference |
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/163/builds/13395 Here is the relevant piece of the build log for the reference |
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/162/builds/16093 Here is the relevant piece of the build log for the reference |
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/60/builds/19470 Here is the relevant piece of the build log for the reference |
flovent
pushed a commit
to flovent/llvm-project
that referenced
this pull request
Feb 13, 2025
…vm#126838) 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.
joaosaffran
pushed a commit
to joaosaffran/llvm-project
that referenced
this pull request
Feb 14, 2025
…vm#126838) 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.
sivan-shani
pushed a commit
to sivan-shani/llvm-project
that referenced
this pull request
Feb 24, 2025
…vm#126838) 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.
igorkudrin
added a commit
to igorkudrin/llvm-project
that referenced
this pull request
Oct 3, 2025
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 llvm#126838 reverted.
igorkudrin
added a commit
to igorkudrin/llvm-project
that referenced
this pull request
Oct 5, 2025
…ress 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 llvm#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 forces `ThreadPlanStepOut::ShouldStop()` to check the stop condition when `m_step_out_further_plan_sp` completes.
igorkudrin
added a commit
to igorkudrin/llvm-project
that referenced
this pull request
Oct 5, 2025
…ress 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 llvm#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.
igorkudrin
added a commit
to igorkudrin/llvm-project
that referenced
this pull request
Oct 5, 2025
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 llvm#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.
igorkudrin
added a commit
that referenced
this pull request
Oct 8, 2025
…at (#161982) 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, if `done` was `false`, the method checked if a suitable frame had been reached. 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.
svkeerthy
pushed a commit
that referenced
this pull request
Oct 9, 2025
…at (#161982) 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, if `done` was `false`, the method checked if a suitable frame had been reached. 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.
clingfei
pushed a commit
to clingfei/llvm-project
that referenced
this pull request
Oct 10, 2025
…at (llvm#161982) 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 llvm#126838, if `done` was `false`, the method checked if a suitable frame had been reached. 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.
akadutta
pushed a commit
to akadutta/llvm-project
that referenced
this pull request
Oct 14, 2025
…at (llvm#161982) 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 llvm#126838, if `done` was `false`, the method checked if a suitable frame had been reached. 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.
igorkudrin
added a commit
that referenced
this pull request
Oct 22, 2025
The test did not work as intended when the empty function `done()` contained prologue/epilogue 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 is 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 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 ``` In this case, setting a breakpoint by function name means setting it several instructions before `ret`, which does not provoke the interaction between `ThreadPlanStepOut` and `ThreadPlanStepOverBreakpoint`.
Lukacma
pushed a commit
to Lukacma/llvm-project
that referenced
this pull request
Oct 29, 2025
The test did not work as intended when the empty function `done()` contained prologue/epilogue code, because a breakpoint was set before the last instruction of the function, which caused the test to pass even with the fix from llvm#126838 having been reverted. The test is 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 interaction between `ThreadPlanStepOut` and `ThreadPlanStepOverBreakpoint` that could lead to missing the stop location in the outer frame; the detailed explanation can be found in llvm#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 ``` In this case, setting a breakpoint by function name means setting it several instructions before `ret`, which does not provoke the interaction between `ThreadPlanStepOut` and `ThreadPlanStepOverBreakpoint`.
aokblast
pushed a commit
to aokblast/llvm-project
that referenced
this pull request
Oct 30, 2025
The test did not work as intended when the empty function `done()` contained prologue/epilogue code, because a breakpoint was set before the last instruction of the function, which caused the test to pass even with the fix from llvm#126838 having been reverted. The test is 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 interaction between `ThreadPlanStepOut` and `ThreadPlanStepOverBreakpoint` that could lead to missing the stop location in the outer frame; the detailed explanation can be found in llvm#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 ``` In this case, setting a breakpoint by function name means setting it several instructions before `ret`, which does not provoke the interaction between `ThreadPlanStepOut` and `ThreadPlanStepOverBreakpoint`.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 aThreadPlanStepOverBreakpoint 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::DoPlanExplainsStophas 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()==trueso 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.