llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-lldb Author: alexey-gusarov <details> <summary>Changes</summary> Fixes part (2) of #<!-- -->215189. **The issue blames `ThreadPlanStepOverBreakpoint::DoPlanExplainsStop()`; the `lldb step` log says otherwise.** The plans below are not skipped -- they are asked, they vote to stop, and the votes are then discarded: ``` Plan Step over breakpoint trap auto-continue: true. Popping plan: "Step over breakpoint trap" Plan Run to address plan should stop: 1. <- voted to stop Completed run to address plan. <- and completed Plan Script based Thread Plan should stop: 1. vvvvvvvv Thread::ShouldStop End (returning 0) vvvvvvvv ``` `Thread::ShouldStop()` walks the stack from the top; a finished plan whose `ShouldAutoContinue()` is true sets `override_stop`, and after the walk `if (override_stop) should_stop = false;` discards everything below. Asking those plans is not free, because the asking **consumes** them: a plan that answers `MischiefManaged()` is popped there and tears its state down on the way out -- `ThreadPlanRunToAddress::MischiefManaged()` deletes the breakpoint it was running to. With its vote discarded and its breakpoint gone, nothing is left to stop the thread and the process runs away. So don't ask. The plan that reaches this is the one nothing on the stack owns: `Thread::SetupToStepOverBreakpointIfNeeded()`, called from `ThreadList::WillResume()`, interposes a `ThreadPlanStepOverBreakpoint` just before a resume and gives it auto-continue exactly when it was interposed for a plan that wanted to run rather than step. The stop being processed is the end of the single instruction it stepped on that plan's behalf, not an event any plan below asked to see. Pop it and resume: the plans below keep their state, and a `BreakpointSite` the thread is sitting on but has not executed is still installed, so it is hit for real on that resume. **It has nothing to do with scripted plans.** With no Python at all: ```python # thread stopped on an enabled breakpoint site thread.RunToAddress(next_instruction_address) # process runs away ``` **Why not "let the completed plan's stop stand" instead.** That shape was built and measured too, and it is wrong in a way no in-tree test covered: with a *user* breakpoint at the address run to, it stops the thread there with the user's breakpoint never reported (`hit_count = 0`, stop reason plan-complete), where `main` today reports it as a hit. This version keeps `main`'s behaviour, and the case is now a test. ### One consequence, stated At the stop where this fires the plans below are no longer asked `ShouldStop()`/`MischiefManaged()`, so a scripted thread plan under the interposed one sees one fewer callback per breakpoint it resumes off. That callback was for a manufactured internal stop the plan never asked to see, and its answer was discarded anyway. `ThreadPlanSingleThreadTimeout` is the one in-tree plan whose deadline depends on being polled: its `MischiefManaged()` returns true so that the poll pops it, and the next resume re-arms it with a fresh timeout. None is armed across the interposed plan's single step -- one is only created while the current plan answers `SupportsResumeOthers()`, and `ThreadPlanStepOverBreakpoint` answers false there for exactly that reason. One armed by an earlier resume and still alive below would keep its deadline instead of being re-armed at this stop: a bounded effect on when `target.process.thread.single-thread-plan-timeout` fires, not on where the thread stops. ### Testing A new `lldb/test/API/functionalities/thread_plan/step_over_breakpoint_site/` with six methods: * the defect, via `SBThread::RunToAddress()`; * the same plan queued by a scripted thread plan -- the shape the issue reports; * that the stepped-over breakpoint is still hit on a later pass; * the same plan with the pc one instruction off the site, as a control; * a target two instructions away, so that "stopped at the address it was given" cannot be satisfied by single-stepping; * a user breakpoint at the address run to, which must still report a hit. The last three pass before this change as well: they are there to pin what a narrower fix would have lost. The first three fail before it, on Linux and on Windows alike. Whole `check-lldb`, compared per test id against `main`: | | ids before | ids after | verdicts changed | |---|---|---|---| | Linux x86_64 (`lldb-api`, `lldb-shell`, `lldb-unit`) | 34465 | 34466 | **0** | | Windows x86_64 (`lldb-api`, `lldb-shell`) | 1849 | 1850 | **0** | 0 failed and 0 unresolved in every arm on both platforms; the single added id is the new test package. The new tests were also run against the unpatched build on both platforms and fail there. Not covered: macOS (no host available), and out-of-tree thread plans that implement `ShouldAutoContinue()` -- in tree there is exactly one, and the new branch is additionally guarded on that plan's kind. The sibling PR for part (1) of the same issue is #<!-- -->215521; they are independent (each moves only its own reproducer) but are best landed together. --- Full diff: https://github.com/llvm/llvm-project/pull/215522.diff 5 Files Affected: - (modified) lldb/source/Target/Thread.cpp (+33-1) - (added) lldb/test/API/functionalities/thread_plan/step_over_breakpoint_site/Makefile (+3) - (added) lldb/test/API/functionalities/thread_plan/step_over_breakpoint_site/TestStepOverBreakpointSite.py (+142) - (added) lldb/test/API/functionalities/thread_plan/step_over_breakpoint_site/main.c (+10) - (added) lldb/test/API/functionalities/thread_plan/step_over_breakpoint_site/run_to_address_plan.py (+21) ``````````diff diff --git a/lldb/source/Target/Thread.cpp b/lldb/source/Target/Thread.cpp index 6aaec4686cde9..3d77a9ae99327 100644 --- a/lldb/source/Target/Thread.cpp +++ b/lldb/source/Target/Thread.cpp @@ -940,12 +940,44 @@ bool Thread::ShouldStop(Event *event_ptr) { if (should_stop) current_plan->WillStop(); - if (current_plan->ShouldAutoContinue(event_ptr)) { + const bool auto_continue = + current_plan->ShouldAutoContinue(event_ptr); + if (auto_continue) { override_stop = true; LLDB_LOGF(log, "Plan %s auto-continue: true.", current_plan->GetName()); } + // A plan that auto-continues has already settled that this stop will + // not be reported: override_stop discards whatever the plans below + // answer. Asking them anyway is not free, because the asking + // consumes them -- a plan that answers MischiefManaged() is popped + // here, and it tears its state down on the way out, the way + // ThreadPlanRunToAddress deletes the breakpoint it was running to. + // With its vote discarded and its breakpoint gone, nothing is left + // to stop the thread and the process runs away. + // + // The plan that does this is the one nothing on the stack owns: + // SetupToStepOverBreakpointIfNeeded() interposes a + // ThreadPlanStepOverBreakpoint just before the resume, and gives it + // auto-continue exactly when it was interposed for a plan that + // wanted to run rather than step. This stop is the end of the one + // instruction it stepped on that plan's behalf, not an event any + // plan below asked to see. So pop it and resume instead: the plans + // below keep their state, and a BreakpointSite the thread is sitting + // on but has not executed is still installed, so it is hit for real + // on that resume -- which is also what keeps a user breakpoint there + // reported as a hit. + if (auto_continue && + current_plan->GetKind() == ThreadPlan::eKindStepOverBreakpoint) { + LLDB_LOGF(log, + "Plan %s auto-continues; not asking the plans below it " + "about a stop that will not be reported.", + current_plan->GetName()); + PopPlan(); + break; + } + // If a Controlling Plan wants to stop, we let it. Otherwise, see if // the plan's parent wants to stop. diff --git a/lldb/test/API/functionalities/thread_plan/step_over_breakpoint_site/Makefile b/lldb/test/API/functionalities/thread_plan/step_over_breakpoint_site/Makefile new file mode 100644 index 0000000000000..10495940055b6 --- /dev/null +++ b/lldb/test/API/functionalities/thread_plan/step_over_breakpoint_site/Makefile @@ -0,0 +1,3 @@ +C_SOURCES := main.c + +include Makefile.rules diff --git a/lldb/test/API/functionalities/thread_plan/step_over_breakpoint_site/TestStepOverBreakpointSite.py b/lldb/test/API/functionalities/thread_plan/step_over_breakpoint_site/TestStepOverBreakpointSite.py new file mode 100644 index 0000000000000..34840c276f12c --- /dev/null +++ b/lldb/test/API/functionalities/thread_plan/step_over_breakpoint_site/TestStepOverBreakpointSite.py @@ -0,0 +1,142 @@ +""" +Test that a thread plan queued while the thread sits on a breakpoint site keeps +control of the process. + +Resuming from a stop on an enabled breakpoint site makes lldb push a +ThreadPlanStepOverBreakpoint to single-step off the site first. That plan +auto-continues, and the stop it produces must not consume the plans below it: +they keep their state, and the site the single step lands on is hit for real on +the resume. +""" + +import lldb +import lldbsuite.test.lldbutil as lldbutil +from lldbsuite.test.decorators import * +from lldbsuite.test.lldbtest import * + + +class StepOverBreakpointSiteTestCase(TestBase): + NO_DEBUG_INFO_TESTCASE = True + + def test_run_to_address_from_a_breakpoint_site(self): + """The pc is on an enabled breakpoint site when the plan is queued.""" + thread, target, process = self.setup_target() + self.expect_run_to_next_instruction(target, process, thread) + + def test_run_to_address_off_a_breakpoint_site(self): + """The control: the same plan, with the pc one instruction past the + site, which needs no step-over plan at all.""" + thread, target, process = self.setup_target() + thread.StepInstruction(False) + self.assertState(process.GetState(), lldb.eStateStopped) + self.expect_run_to_next_instruction(target, process, thread) + + def test_user_breakpoint_at_the_target_is_still_reported(self): + """A user breakpoint at the address the plan runs to must still be + reported as a hit: the thread steps off the site it was stopped on and + the site it lands on is hit on the resume, not swallowed.""" + thread, target, process = self.setup_target() + next_pc = self.next_pc(target, thread) + user_bp = target.BreakpointCreateByAddress(next_pc) + self.assertTrue(user_bp.GetNumLocations() > 0, VALID_BREAKPOINT) + + thread.RunToAddress(next_pc) + + self.assertState(process.GetState(), lldb.eStateStopped) + self.assertEqual(thread.GetFrameAtIndex(0).GetPC(), next_pc) + self.assertEqual( + user_bp.GetHitCount(), + 1, + "the user breakpoint at the address run to was not reported as hit", + ) + self.assertStopReason(thread.GetStopReason(), lldb.eStopReasonBreakpoint) + + def test_run_to_address_two_instructions_from_a_breakpoint_site(self): + """A target further than one instruction away must be reached, not + merely stepped towards: the single step off the site lands short of it, + and the thread has to carry on to the address it was given.""" + thread, target, process = self.setup_target() + pc = thread.GetFrameAtIndex(0).GetPCAddress() + instructions = target.ReadInstructions(pc, 3) + self.assertEqual(instructions.GetSize(), 3) + target_pc = ( + instructions.GetInstructionAtIndex(2).GetAddress().GetLoadAddress(target) + ) + + thread.RunToAddress(target_pc) + + self.assertState(process.GetState(), lldb.eStateStopped) + self.assertEqual( + thread.GetFrameAtIndex(0).GetPC(), + target_pc, + "the thread stopped short of the address the plan was given", + ) + + def test_the_stepped_over_breakpoint_is_hit_again(self): + """The site the thread was parked on must be re-enabled after the plan + that stepped off it is popped, so a later pass hits it again.""" + thread, target, process = self.setup_target() + breakpoint = target.GetBreakpointAtIndex(0) + self.assertEqual(breakpoint.GetHitCount(), 1) + + self.expect_run_to_next_instruction(target, process, thread) + process.Continue() + + self.assertState(process.GetState(), lldb.eStateStopped) + self.assertEqual( + breakpoint.GetHitCount(), + 2, + "the breakpoint that was stepped over was not hit on the next pass", + ) + + def test_scripted_plan_queueing_the_run_to_address(self): + """The shape the issue reports: the run-to-address plan is queued by a + scripted thread plan rather than by the API directly.""" + thread, target, process = self.setup_target() + self.runCmd("command script import run_to_address_plan.py") + next_pc = self.next_pc(target, thread) + + args = lldb.SBStructuredData() + args.SetFromJSON('{"addr":%d}' % next_pc) + err = thread.StepUsingScriptedThreadPlan( + "run_to_address_plan.RunToAddress", args, True + ) + self.assertSuccess(err) + + self.assertState( + process.GetState(), + lldb.eStateStopped, + "the process ran away instead of stopping at the queued address", + ) + self.assertEqual(thread.GetFrameAtIndex(0).GetPC(), next_pc) + + def next_pc(self, target, thread): + pc = thread.GetFrameAtIndex(0).GetPCAddress() + instructions = target.ReadInstructions(pc, 2) + self.assertEqual( + instructions.GetSize(), 2, "could not read two instructions at the pc" + ) + return instructions.GetInstructionAtIndex(1).GetAddress().GetLoadAddress(target) + + def expect_run_to_next_instruction(self, target, process, thread): + next_pc = self.next_pc(target, thread) + + thread.RunToAddress(next_pc) + + self.assertState( + process.GetState(), + lldb.eStateStopped, + "the process was not stopped at the address the plan was given", + ) + self.assertEqual( + thread.GetFrameAtIndex(0).GetPC(), + next_pc, + "the thread did not stop at the address the plan was given", + ) + + def setup_target(self): + self.build() + (target, process, thread, bkpt) = lldbutil.run_to_source_breakpoint( + self, "Set a breakpoint here", lldb.SBFileSpec("main.c") + ) + return thread, target, process diff --git a/lldb/test/API/functionalities/thread_plan/step_over_breakpoint_site/main.c b/lldb/test/API/functionalities/thread_plan/step_over_breakpoint_site/main.c new file mode 100644 index 0000000000000..0390a53881454 --- /dev/null +++ b/lldb/test/API/functionalities/thread_plan/step_over_breakpoint_site/main.c @@ -0,0 +1,10 @@ +static int f(int x) { + int y = x + 1; // Set a breakpoint here. + return y; +} + +int main() { + int a = f(1); + a += f(2); + return a; +} diff --git a/lldb/test/API/functionalities/thread_plan/step_over_breakpoint_site/run_to_address_plan.py b/lldb/test/API/functionalities/thread_plan/step_over_breakpoint_site/run_to_address_plan.py new file mode 100644 index 0000000000000..6220dd04b513b --- /dev/null +++ b/lldb/test/API/functionalities/thread_plan/step_over_breakpoint_site/run_to_address_plan.py @@ -0,0 +1,21 @@ +import lldb + + +class RunToAddress: + """The shape the issue reports: a scripted plan whose only job is to queue a + run-to-address sub-plan for an address it is handed.""" + + def __init__(self, thread_plan, args_data): + self.thread_plan = thread_plan + target = thread_plan.GetThread().GetProcess().GetTarget() + addr = args_data.GetValueForKey("addr").GetUnsignedIntegerValue() + self.sub_plan = thread_plan.QueueThreadPlanForRunToAddress( + lldb.SBAddress(addr, target), lldb.SBError() + ) + + def explains_stop(self, event): + return False + + def should_stop(self, event): + self.thread_plan.SetPlanComplete(True) + return True `````````` </details> https://github.com/llvm/llvm-project/pull/215522 _______________________________________________ lldb-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
