Author: Jason Molenda Date: 2025-02-12T13:48:01-08:00 New Revision: cbb4e99f3613549c2168f52d6f348e3a7ee3cf55
URL: https://github.com/llvm/llvm-project/commit/cbb4e99f3613549c2168f52d6f348e3a7ee3cf55 DIFF: https://github.com/llvm/llvm-project/commit/cbb4e99f3613549c2168f52d6f348e3a7ee3cf55.diff LOG: [lldb] Update ThreadPlanStepOut to handle new breakpoint behavior (#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. Added: lldb/test/API/functionalities/thread/finish-from-empty-func/Makefile lldb/test/API/functionalities/thread/finish-from-empty-func/TestEmptyFuncThreadStepOut.py lldb/test/API/functionalities/thread/finish-from-empty-func/main.c Modified: lldb/source/Target/ThreadPlanStepOut.cpp Removed: ################################################################################ diff --git a/lldb/source/Target/ThreadPlanStepOut.cpp b/lldb/source/Target/ThreadPlanStepOut.cpp index c0ea53e4a8cbb..95e51408dbcdc 100644 --- a/lldb/source/Target/ThreadPlanStepOut.cpp +++ b/lldb/source/Target/ThreadPlanStepOut.cpp @@ -364,8 +364,11 @@ 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 && 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; +} _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
