Skip to content

Commit 09ca29c

Browse files
committed
[lldb] Change lldb's breakpoint handling behavior, reland (llvm#126988)
lldb today has two rules: When a thread stops at a BreakpointSite, we set the thread's StopReason to be "breakpoint hit" (regardless if we've actually hit the breakpoint, or if we've merely stopped *at* the breakpoint instruction/point and haven't tripped it yet). And second, when resuming a process, any thread sitting at a BreakpointSite is silently stepped over the BreakpointSite -- because we've already flagged the breakpoint hit when we stopped there originally. In this patch, I change lldb to only set a thread's stop reason to breakpoint-hit when we've actually executed the instruction/triggered the breakpoint. When we resume, we only silently step past a BreakpointSite that we've registered as hit. We preserve this state across inferior function calls that the user may do while stopped, etc. Also, when a user adds a new breakpoint at $pc while stopped, or changes $pc to be the address of a BreakpointSite, we will silently step past that breakpoint when the process resumes. This is purely a UX call, I don't think there's any person who wants to set a breakpoint at $pc and then hit it immediately on resuming. One non-intuitive UX from this change, butt is necessary: If you're stopped at a BreakpointSite that has not yet executed, you `stepi`, you will hit the breakpoint and the pc will not yet advance. This thread has not completed its stepi, and the ThreadPlanStepInstruction is still on the stack. If you then `continue` the thread, lldb will now stop and say, "instruction step completed", one instruction past the BreakpointSite. You can continue a second time to resume execution. The bugs driving this change are all from lldb dropping the real stop reason for a thread and setting it to breakpoint-hit when that was not the case. Jim hit one where we have an aarch64 watchpoint that triggers one instruction before a BreakpointSite. On this arch we are notified of the watchpoint hit after the instruction has been unrolled -- we disable the watchpoint, instruction step, re-enable the watchpoint and collect the new value. But now we're on a BreakpointSite so the watchpoint-hit stop reason is lost. Another was reported by ZequanWu in https://discourse.llvm.org/t/lldb-unable-to-break-at-start/78282 we attach to/launch a process with the pc at a BreakpointSite and misbehave. Caroline Tice mentioned it is also a problem they've had with putting a breakpoint on _dl_debug_state. The change to each Process plugin that does execution control is that 1. If we've stopped at a BreakpointSite that has not been executed yet, we will call Thread::SetThreadStoppedAtUnexecutedBP(pc) to record that. When the thread resumes, if the pc is still at the same site, we will continue, hit the breakpoint, and stop again. 2. When we've actually hit a breakpoint (enabled for this thread or not), the Process plugin should call Thread::SetThreadHitBreakpointSite(). When we go to resume the thread, we will push a step-over-breakpoint ThreadPlan before resuming. The biggest set of changes is to StopInfoMachException where we translate a Mach Exception into a stop reason. The Mach exception codes differ in a few places depending on the target (unambiguously), and I didn't want to duplicate the new code for each target so I've tested what mach exceptions we get for each action on each target, and reorganized StopInfoMachException::CreateStopReasonWithMachException to document these possible values, and handle them without specializing based on the target arch. I first landed this patch in July 2024 via llvm#96260 but the CI bots and wider testing found a number of test case failures that needed to be updated, I reverted it. I've fixed all of those issues in separate PRs and this change should run cleanly on all the CI bots now. rdar://123942164 (cherry picked from commit b666ac3)
1 parent f72f839 commit 09ca29c

File tree

9 files changed

+263
-261
lines changed

9 files changed

+263
-261
lines changed

lldb/include/lldb/Target/Thread.h

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -129,6 +129,7 @@ class Thread : public std::enable_shared_from_this<Thread>,
129129
register_backup_sp; // You need to restore the registers, of course...
130130
uint32_t current_inlined_depth;
131131
lldb::addr_t current_inlined_pc;
132+
lldb::addr_t stopped_at_unexecuted_bp;
132133
};
133134

134135
/// Constructor
@@ -381,6 +382,26 @@ class Thread : public std::enable_shared_from_this<Thread>,
381382

382383
virtual void SetQueueLibdispatchQueueAddress(lldb::addr_t dispatch_queue_t) {}
383384

385+
/// When a thread stops at an enabled BreakpointSite that has not executed,
386+
/// the Process plugin should call SetThreadStoppedAtUnexecutedBP(pc).
387+
/// If that BreakpointSite was actually triggered (the instruction was
388+
/// executed, for a software breakpoint), regardless of whether the
389+
/// breakpoint is valid for this thread, SetThreadHitBreakpointSite()
390+
/// should be called to record that fact.
391+
///
392+
/// Depending on the structure of the Process plugin, it may be easiest
393+
/// to call SetThreadStoppedAtUnexecutedBP(pc) unconditionally when at
394+
/// a BreakpointSite, and later when it is known that it was triggered,
395+
/// SetThreadHitBreakpointSite() can be called. These two methods
396+
/// overwrite the same piece of state in the Thread, the last one
397+
/// called on a Thread wins.
398+
void SetThreadStoppedAtUnexecutedBP(lldb::addr_t pc) {
399+
m_stopped_at_unexecuted_bp = pc;
400+
}
401+
void SetThreadHitBreakpointSite() {
402+
m_stopped_at_unexecuted_bp = LLDB_INVALID_ADDRESS;
403+
}
404+
384405
/// Whether this Thread already has all the Queue information cached or not
385406
///
386407
/// A Thread may be associated with a libdispatch work Queue at a given
@@ -1367,6 +1388,9 @@ class Thread : public std::enable_shared_from_this<Thread>,
13671388
bool m_should_run_before_public_stop; // If this thread has "stop others"
13681389
// private work to do, then it will
13691390
// set this.
1391+
lldb::addr_t m_stopped_at_unexecuted_bp; // Set to the address of a breakpoint
1392+
// instruction that we have not yet
1393+
// hit, but will hit when we resume.
13701394
const uint32_t m_index_id; ///< A unique 1 based index assigned to each thread
13711395
/// for easy UI/command line access.
13721396
lldb::RegisterContextSP m_reg_context_sp; ///< The register context for this

0 commit comments

Comments
 (0)