[Lldb-commits] [lldb] [lldb] Change lldb's breakpoint handling behavior (PR #96260)
Jason Molenda via lldb-commits
lldb-commits at lists.llvm.org
Fri Jul 19 14:29:31 PDT 2024
https://github.com/jasonmolenda updated https://github.com/llvm/llvm-project/pull/96260
>From 9b541e6a035635e26c6a24eca022de8552fa4c17 Mon Sep 17 00:00:00 2001
From: Jason Molenda <jmolenda at apple.com>
Date: Thu, 20 Jun 2024 17:53:17 -0700
Subject: [PATCH 1/8] [lldb] Change lldb's breakpoint handling behavior
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, but I'm convinced it 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
thread plan 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. I discussed this with Jim, and trying
to paper over this behavior will lead to more complicated scenarios
behaving non-intuitively. And mostly it's the testsuite that was
trying to instruction step past a breakpoint and getting thrown off
-- and I changed those tests to expect the new behavior.
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 (whether we hit it or not),
we call Thread::SetThreadStoppedAtBreakpointSite(pc) to record the
state at the point when the thread stopped. (so we can detect
newly-added breakpoints, or when the pc is changed to an instruction
that is a BreakpointSite)
2. When we have actually hit a breakpoint, and it is enabled for
this thread, we call Thread::SetThreadHitBreakpointAtAddr(pc) so
we know that it should be silently stepped past when we resume
execution.
When resuming, we silently step over a breakpoint if we've hit it,
or if it is newly added (or the pc was changed to an existing
BreakpointSite).
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.
rdar://123942164
---
lldb/include/lldb/Target/Thread.h | 29 ++
.../Process/Utility/StopInfoMachException.cpp | 296 +++++++-----------
.../Process/Windows/Common/ProcessWindows.cpp | 16 +-
.../Process/gdb-remote/ProcessGDBRemote.cpp | 118 +++----
.../Process/scripted/ScriptedThread.cpp | 9 +
lldb/source/Target/Thread.cpp | 17 +-
.../TestConsecutiveBreakpoints.py | 26 +-
.../TestStepOverBreakpoint.py | 6 +-
8 files changed, 235 insertions(+), 282 deletions(-)
diff --git a/lldb/include/lldb/Target/Thread.h b/lldb/include/lldb/Target/Thread.h
index c17bddf4d98b8..1e1aead896018 100644
--- a/lldb/include/lldb/Target/Thread.h
+++ b/lldb/include/lldb/Target/Thread.h
@@ -128,6 +128,11 @@ class Thread : public std::enable_shared_from_this<Thread>,
register_backup_sp; // You need to restore the registers, of course...
uint32_t current_inlined_depth;
lldb::addr_t current_inlined_pc;
+ lldb::addr_t
+ hit_bp_at_addr; // Set to the address of a breakpoint that we have hit.
+ lldb::addr_t bpsite_at_stop_pc; // Set to the address of a breakpoint
+ // instruction that we have not yet hit, but
+ // will hit when we resume.
};
/// Constructor
@@ -377,6 +382,19 @@ class Thread : public std::enable_shared_from_this<Thread>,
virtual void SetQueueLibdispatchQueueAddress(lldb::addr_t dispatch_queue_t) {}
+ /// When a thread has executed/trapped a breakpoint, set the address of that
+ /// breakpoint so we know it has been hit already, and should be silently
+ /// stepped past on resume.
+ void SetThreadHitBreakpointAtAddr(lldb::addr_t pc) { m_hit_bp_at_addr = pc; }
+
+ /// When a thread stops at a breakpoint instruction/address, but has not yet
+ /// executed/triggered it, record that so we can detect when a user adds a
+ /// breakpoint (or changes a thread to a breakpoint site) and we need to
+ /// silently step past that when resuming.
+ void SetThreadStoppedAtBreakpointSite(lldb::addr_t pc) {
+ m_bpsite_at_stop_pc = pc;
+ }
+
/// Whether this Thread already has all the Queue information cached or not
///
/// A Thread may be associated with a libdispatch work Queue at a given
@@ -1311,6 +1329,17 @@ class Thread : public std::enable_shared_from_this<Thread>,
bool m_should_run_before_public_stop; // If this thread has "stop others"
// private work to do, then it will
// set this.
+ lldb::addr_t m_hit_bp_at_addr; // If this thread originally stopped at a
+ // breakpoint instruction, AND HIT IT,
+ // record the address of that breakpoint.
+ // LLDB_INVALID_ADDRESS if this thread did
+ // not stop at a breakpoint insn, or did not
+ // hit it yet.
+ lldb::addr_t m_bpsite_at_stop_pc; // If this thread originally stopped at a
+ // breakpoint site, record the address of
+ // that breakpoint site.
+ // LLDB_INVALID_ADDRESS if this thread did
+ // not stop at a breakpoint site.
const uint32_t m_index_id; ///< A unique 1 based index assigned to each thread
/// for easy UI/command line access.
lldb::RegisterContextSP m_reg_context_sp; ///< The register context for this
diff --git a/lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp b/lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp
index 25cee369d7ee3..8087ef1accde8 100644
--- a/lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp
+++ b/lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp
@@ -488,38 +488,6 @@ const char *StopInfoMachException::GetDescription() {
return m_description.c_str();
}
-static StopInfoSP GetStopInfoForHardwareBP(Thread &thread, Target *target,
- uint32_t exc_data_count,
- uint64_t exc_sub_code,
- uint64_t exc_sub_sub_code) {
- // Try hardware watchpoint.
- if (target) {
- // The exc_sub_code indicates the data break address.
- WatchpointResourceSP wp_rsrc_sp =
- target->GetProcessSP()->GetWatchpointResourceList().FindByAddress(
- (addr_t)exc_sub_code);
- if (wp_rsrc_sp && wp_rsrc_sp->GetNumberOfConstituents() > 0) {
- return StopInfo::CreateStopReasonWithWatchpointID(
- thread, wp_rsrc_sp->GetConstituentAtIndex(0)->GetID());
- }
- }
-
- // Try hardware breakpoint.
- ProcessSP process_sp(thread.GetProcess());
- if (process_sp) {
- // The exc_sub_code indicates the data break address.
- lldb::BreakpointSiteSP bp_sp =
- process_sp->GetBreakpointSiteList().FindByAddress(
- (lldb::addr_t)exc_sub_code);
- if (bp_sp && bp_sp->IsEnabled()) {
- return StopInfo::CreateStopReasonWithBreakpointSiteID(thread,
- bp_sp->GetID());
- }
- }
-
- return nullptr;
-}
-
#if defined(__APPLE__)
const char *
StopInfoMachException::MachException::Name(exception_type_t exc_type) {
@@ -633,171 +601,143 @@ StopInfoSP StopInfoMachException::CreateStopReasonWithMachException(
}
break;
+ // [exc_type, exc_code, exc_sub_code, exc_sub_sub_code]
+ //
+ // Instruction step:
+ // [6, 1, 0]
+ // Intel KDP [6, 3, ??]
+ // armv7 [6, 0x102, <stop-pc>] Same as software breakpoint!
+ //
+ // Software breakpoint:
+ // x86 [6, 2, 0]
+ // Intel KDP [6, 2, <bp-addr + 1>]
+ // arm64 [6, 1, <bp-addr>]
+ // armv7 [6, 0x102, <bp-addr>] Same as instruction step!
+ //
+ // Hardware breakpoint:
+ // x86 [6, 1, <bp-addr>, 0]
+ // x86/Rosetta not implemented, see software breakpoint
+ // arm64 [6, 1, <bp-addr>]
+ // armv7 not implemented, see software breakpoint
+ //
+ // Hardware watchpoint:
+ // x86 [6, 1, <accessed-addr>, 0] (both Intel hw and Rosetta)
+ // arm64 [6, 0x102, <accessed-addr>, 0]
+ // armv7 [6, 0x102, <accessed-addr>, 0]
+ //
+ // arm64 BRK instruction (imm arg not reflected in the ME)
+ // [ 6, 1, <addr-of-BRK-insn>]
+ //
+ // In order of codes mach exceptions:
+ // [6, 1, 0] - instruction step
+ // [6, 1, <bp-addr>] - hardware breakpoint or watchpoint
+ //
+ // [6, 2, 0] - software breakpoint
+ // [6, 2, <bp-addr + 1>] - software breakpoint
+ //
+ // [6, 3] - instruction step
+ //
+ // [6, 0x102, <stop-pc>] armv7 instruction step
+ // [6, 0x102, <bp-addr>] armv7 software breakpoint
+ // [6, 0x102, <accessed-addr>, 0] arm64/armv7 watchpoint
case 6: // EXC_BREAKPOINT
{
- bool is_actual_breakpoint = false;
- bool is_trace_if_actual_breakpoint_missing = false;
- switch (cpu) {
- case llvm::Triple::x86:
- case llvm::Triple::x86_64:
- if (exc_code == 1) // EXC_I386_SGL
- {
- if (!exc_sub_code) {
- // This looks like a plain trap.
- // Have to check if there is a breakpoint here as well. When you
- // single-step onto a trap, the single step stops you not to trap.
- // Since we also do that check below, let's just use that logic.
- is_actual_breakpoint = true;
- is_trace_if_actual_breakpoint_missing = true;
- } else {
- if (StopInfoSP stop_info =
- GetStopInfoForHardwareBP(thread, target, exc_data_count,
- exc_sub_code, exc_sub_sub_code))
- return stop_info;
- }
- } else if (exc_code == 2 || // EXC_I386_BPT
- exc_code == 3) // EXC_I386_BPTFLT
- {
- // KDP returns EXC_I386_BPTFLT for trace breakpoints
- if (exc_code == 3)
- is_trace_if_actual_breakpoint_missing = true;
-
- is_actual_breakpoint = true;
- if (!pc_already_adjusted)
- pc_decrement = 1;
- }
- break;
+ bool stopped_by_hitting_breakpoint = false;
+ bool stopped_by_completing_stepi = false;
+ bool stopped_watchpoint = false;
+ std::optional<addr_t> value;
+
+ // exc_code 1
+ if (exc_code == 1 && exc_sub_code == 0)
+ stopped_by_completing_stepi = true;
+ if (exc_code == 1 && exc_sub_code != 0) {
+ stopped_by_hitting_breakpoint = true;
+ stopped_watchpoint = true;
+ value = exc_sub_code;
+ }
- case llvm::Triple::arm:
- case llvm::Triple::thumb:
- if (exc_code == 0x102) // EXC_ARM_DA_DEBUG
- {
- // LWP_TODO: We need to find the WatchpointResource that matches
- // the address, and evaluate its Watchpoints.
-
- // It's a watchpoint, then, if the exc_sub_code indicates a
- // known/enabled data break address from our watchpoint list.
- lldb::WatchpointSP wp_sp;
- if (target)
- wp_sp = target->GetWatchpointList().FindByAddress(
- (lldb::addr_t)exc_sub_code);
- if (wp_sp && wp_sp->IsEnabled()) {
- return StopInfo::CreateStopReasonWithWatchpointID(thread,
- wp_sp->GetID());
- } else {
- is_actual_breakpoint = true;
- is_trace_if_actual_breakpoint_missing = true;
- }
- } else if (exc_code == 1) // EXC_ARM_BREAKPOINT
- {
- is_actual_breakpoint = true;
- is_trace_if_actual_breakpoint_missing = true;
- } else if (exc_code == 0) // FIXME not EXC_ARM_BREAKPOINT but a kernel
- // is currently returning this so accept it
- // as indicating a breakpoint until the
- // kernel is fixed
- {
- is_actual_breakpoint = true;
- is_trace_if_actual_breakpoint_missing = true;
- }
- break;
+ // exc_code 2
+ if (exc_code == 2 && exc_sub_code == 0)
+ stopped_by_hitting_breakpoint = true;
+ if (exc_code == 2 && exc_sub_code != 0) {
+ stopped_by_hitting_breakpoint = true;
+ // Intel KDP software breakpoint
+ if (!pc_already_adjusted)
+ pc_decrement = 1;
+ }
- case llvm::Triple::aarch64_32:
- case llvm::Triple::aarch64: {
- // xnu describes three things with type EXC_BREAKPOINT:
- //
- // exc_code 0x102 [EXC_ARM_DA_DEBUG], exc_sub_code addr-of-insn
- // Watchpoint access. exc_sub_code is the address of the
- // instruction which trigged the watchpoint trap.
- // debugserver may add the watchpoint number that was triggered
- // in exc_sub_sub_code.
- //
- // exc_code 1 [EXC_ARM_BREAKPOINT], exc_sub_code 0
- // Instruction step has completed.
- //
- // exc_code 1 [EXC_ARM_BREAKPOINT], exc_sub_code address-of-instruction
- // Software breakpoint instruction executed.
-
- if (exc_code == 1 && exc_sub_code == 0) // EXC_ARM_BREAKPOINT
- {
- // This is hit when we single instruction step aka MDSCR_EL1 SS bit 0
- // is set
- is_actual_breakpoint = true;
- is_trace_if_actual_breakpoint_missing = true;
- if (thread.GetTemporaryResumeState() != eStateStepping)
- not_stepping_but_got_singlestep_exception = true;
- }
- if (exc_code == 0x102) // EXC_ARM_DA_DEBUG
- {
- // LWP_TODO: We need to find the WatchpointResource that matches
- // the address, and evaluate its Watchpoints.
-
- // It's a watchpoint, then, if the exc_sub_code indicates a
- // known/enabled data break address from our watchpoint list.
- lldb::WatchpointSP wp_sp;
- if (target)
- wp_sp = target->GetWatchpointList().FindByAddress(
- (lldb::addr_t)exc_sub_code);
- if (wp_sp && wp_sp->IsEnabled()) {
- return StopInfo::CreateStopReasonWithWatchpointID(thread,
- wp_sp->GetID());
- }
- // EXC_ARM_DA_DEBUG seems to be reused for EXC_BREAKPOINT as well as
- // EXC_BAD_ACCESS
- if (thread.GetTemporaryResumeState() == eStateStepping)
- return StopInfo::CreateStopReasonToTrace(thread);
+ // exc_code 3
+ if (exc_code == 3)
+ stopped_by_completing_stepi = true;
+
+ // exc_code 0x102
+ if (exc_code == 0x102 && exc_sub_code != 0) {
+ if (cpu == llvm::Triple::arm || cpu == llvm::Triple::thumb) {
+ stopped_by_hitting_breakpoint = true;
+ stopped_by_completing_stepi = true;
}
- // It looks like exc_sub_code has the 4 bytes of the instruction that
- // triggered the exception, i.e. our breakpoint opcode
- is_actual_breakpoint = exc_code == 1;
- break;
+ stopped_watchpoint = true;
+ value = exc_sub_code;
}
- default:
- break;
- }
+ // Go through the reasons why we stopped, starting
+ // with the easiest to detect unambiguously. We
+ // may have multiple possible reasons set.
- if (is_actual_breakpoint) {
+ if (stopped_by_hitting_breakpoint) {
+ ProcessSP process_sp(thread.GetProcess());
RegisterContextSP reg_ctx_sp(thread.GetRegisterContext());
- addr_t pc = reg_ctx_sp->GetPC() - pc_decrement;
-
- ProcessSP process_sp(thread.CalculateProcess());
-
- lldb::BreakpointSiteSP bp_site_sp;
- if (process_sp)
+ BreakpointSiteSP bp_site_sp;
+ addr_t pc = LLDB_INVALID_ADDRESS;
+ if (reg_ctx_sp)
+ pc = reg_ctx_sp->GetPC() - pc_decrement;
+ else if (value)
+ pc = *value;
+
+ if (value)
+ bp_site_sp = process_sp->GetBreakpointSiteList().FindByAddress(*value);
+ if (!bp_site_sp && reg_ctx_sp) {
bp_site_sp = process_sp->GetBreakpointSiteList().FindByAddress(pc);
+ }
if (bp_site_sp && bp_site_sp->IsEnabled()) {
- // Update the PC if we were asked to do so, but only do so if we find
- // a breakpoint that we know about cause this could be a trap
- // instruction in the code
- if (pc_decrement > 0 && adjust_pc_if_needed)
- reg_ctx_sp->SetPC(pc);
-
- // If the breakpoint is for this thread, then we'll report the hit,
- // but if it is for another thread, we can just report no reason. We
- // don't need to worry about stepping over the breakpoint here, that
- // will be taken care of when the thread resumes and notices that
- // there's a breakpoint under the pc. If we have an operating system
- // plug-in, we might have set a thread specific breakpoint using the
- // operating system thread ID, so we can't make any assumptions about
- // the thread ID so we must always report the breakpoint regardless
- // of the thread.
+ // If we have an operating system plug-in, we might have set a thread
+ // specific breakpoint using the operating system thread ID, so we
+ // can't make any assumptions about the thread ID so we must always
+ // report the breakpoint regardless of the thread.
if (bp_site_sp->ValidForThisThread(thread) ||
- thread.GetProcess()->GetOperatingSystem() != nullptr)
+ thread.GetProcess()->GetOperatingSystem() != nullptr) {
+ // Update the PC if we were asked to do so, but only do so if we find
+ // a breakpoint that we know about cause this could be a trap
+ // instruction in the code
+ if (pc_decrement > 0 && adjust_pc_if_needed && reg_ctx_sp)
+ reg_ctx_sp->SetPC(pc);
+ thread.SetThreadHitBreakpointAtAddr(pc);
return StopInfo::CreateStopReasonWithBreakpointSiteID(
thread, bp_site_sp->GetID());
- else if (is_trace_if_actual_breakpoint_missing)
- return StopInfo::CreateStopReasonToTrace(thread);
- else
+ } else {
+ thread.SetThreadHitBreakpointAtAddr(pc);
return StopInfoSP();
+ }
}
+ }
- // Don't call this a trace if we weren't single stepping this thread.
- if (is_trace_if_actual_breakpoint_missing &&
- thread.GetTemporaryResumeState() == eStateStepping) {
- return StopInfo::CreateStopReasonToTrace(thread);
+ if (stopped_watchpoint && value) {
+ WatchpointResourceSP wp_rsrc_sp =
+ target->GetProcessSP()->GetWatchpointResourceList().FindByAddress(
+ *value);
+ if (wp_rsrc_sp && wp_rsrc_sp->GetNumberOfConstituents() > 0) {
+ return StopInfo::CreateStopReasonWithWatchpointID(
+ thread, wp_rsrc_sp->GetConstituentAtIndex(0)->GetID());
}
}
+
+ if (stopped_by_completing_stepi) {
+ if (thread.GetTemporaryResumeState() != eStateStepping)
+ not_stepping_but_got_singlestep_exception = true;
+ else
+ return StopInfo::CreateStopReasonToTrace(thread);
+ }
+
} break;
case 7: // EXC_SYSCALL
diff --git a/lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp b/lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp
index f383b3d40a4f3..c4005a1f51150 100644
--- a/lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp
+++ b/lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp
@@ -382,19 +382,8 @@ void ProcessWindows::RefreshStateAfterStop() {
RegisterContextSP register_context = stop_thread->GetRegisterContext();
const uint64_t pc = register_context->GetPC();
BreakpointSiteSP site(GetBreakpointSiteList().FindByAddress(pc));
- if (site && site->ValidForThisThread(*stop_thread)) {
- LLDB_LOG(log,
- "Single-stepped onto a breakpoint in process {0} at "
- "address {1:x} with breakpoint site {2}",
- m_session_data->m_debugger->GetProcess().GetProcessId(), pc,
- site->GetID());
- stop_info = StopInfo::CreateStopReasonWithBreakpointSiteID(*stop_thread,
- site->GetID());
- stop_thread->SetStopInfo(stop_info);
-
- return;
- }
-
+ if (site)
+ stop_thread->SetThreadStoppedAtBreakpointSite(pc);
auto *reg_ctx = static_cast<RegisterContextWindows *>(
stop_thread->GetRegisterContext().get());
uint32_t slot_id = reg_ctx->GetTriggeredHardwareBreakpointSlotId();
@@ -462,6 +451,7 @@ void ProcessWindows::RefreshStateAfterStop() {
stop_info = StopInfo::CreateStopReasonWithBreakpointSiteID(
*stop_thread, site->GetID());
register_context->SetPC(pc);
+ stop_thread->SetThreadHitBreakpointAtAddr(pc);
} else {
LLDB_LOG(log,
"Breakpoint site {0} is not valid for this thread, "
diff --git a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
index a5a731981299f..bad42d44503fd 100644
--- a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -1598,29 +1598,8 @@ bool ProcessGDBRemote::CalculateThreadStopInfo(ThreadGDBRemote *thread) {
// that have stop reasons, and if there is no entry for a thread, then it
// has no stop reason.
thread->GetRegisterContext()->InvalidateIfNeeded(true);
- if (!GetThreadStopInfoFromJSON(thread, m_jstopinfo_sp)) {
- // If a thread is stopped at a breakpoint site, set that as the stop
- // reason even if it hasn't executed the breakpoint instruction yet.
- // We will silently step over the breakpoint when we resume execution
- // and miss the fact that this thread hit the breakpoint.
- const size_t num_thread_ids = m_thread_ids.size();
- for (size_t i = 0; i < num_thread_ids; i++) {
- if (m_thread_ids[i] == thread->GetID() && m_thread_pcs.size() > i) {
- addr_t pc = m_thread_pcs[i];
- lldb::BreakpointSiteSP bp_site_sp =
- thread->GetProcess()->GetBreakpointSiteList().FindByAddress(pc);
- if (bp_site_sp) {
- if (bp_site_sp->ValidForThisThread(*thread)) {
- thread->SetStopInfo(
- StopInfo::CreateStopReasonWithBreakpointSiteID(
- *thread, bp_site_sp->GetID()));
- return true;
- }
- }
- }
- }
+ if (!GetThreadStopInfoFromJSON(thread, m_jstopinfo_sp))
thread->SetStopInfo(StopInfoSP());
- }
return true;
}
@@ -1729,6 +1708,12 @@ ThreadSP ProcessGDBRemote::SetThreadStopInfo(
if (ThreadSP memory_thread_sp = m_thread_list.GetBackingThread(thread_sp))
thread_sp = memory_thread_sp;
+ addr_t pc = thread_sp->GetRegisterContext()->GetPC();
+ BreakpointSiteSP bp_site_sp =
+ thread_sp->GetProcess()->GetBreakpointSiteList().FindByAddress(pc);
+ if (bp_site_sp)
+ thread_sp->SetThreadStoppedAtBreakpointSite(pc);
+
if (exc_type != 0) {
const size_t exc_data_size = exc_data.size();
@@ -1745,23 +1730,11 @@ ThreadSP ProcessGDBRemote::SetThreadStopInfo(
// to no reason.
if (!reason.empty() && reason != "none") {
if (reason == "trace") {
- addr_t pc = thread_sp->GetRegisterContext()->GetPC();
- lldb::BreakpointSiteSP bp_site_sp =
- thread_sp->GetProcess()->GetBreakpointSiteList().FindByAddress(
- pc);
-
- // If the current pc is a breakpoint site then the StopInfo should be
- // set to Breakpoint Otherwise, it will be set to Trace.
- if (bp_site_sp && bp_site_sp->ValidForThisThread(*thread_sp)) {
- thread_sp->SetStopInfo(
- StopInfo::CreateStopReasonWithBreakpointSiteID(
- *thread_sp, bp_site_sp->GetID()));
- } else
- thread_sp->SetStopInfo(
- StopInfo::CreateStopReasonToTrace(*thread_sp));
+ thread_sp->SetStopInfo(StopInfo::CreateStopReasonToTrace(*thread_sp));
handled = true;
} else if (reason == "breakpoint") {
addr_t pc = thread_sp->GetRegisterContext()->GetPC();
+ thread_sp->SetThreadHitBreakpointAtAddr(pc);
lldb::BreakpointSiteSP bp_site_sp =
thread_sp->GetProcess()->GetBreakpointSiteList().FindByAddress(
pc);
@@ -1776,6 +1749,7 @@ ThreadSP ProcessGDBRemote::SetThreadStopInfo(
thread_sp->SetStopInfo(
StopInfo::CreateStopReasonWithBreakpointSiteID(
*thread_sp, bp_site_sp->GetID()));
+ thread_sp->SetThreadHitBreakpointAtAddr(pc);
} else {
StopInfoSP invalid_stop_info_sp;
thread_sp->SetStopInfo(invalid_stop_info_sp);
@@ -1880,60 +1854,40 @@ ThreadSP ProcessGDBRemote::SetThreadStopInfo(
StopInfo::CreateStopReasonVForkDone(*thread_sp));
handled = true;
}
- } else if (!signo) {
- addr_t pc = thread_sp->GetRegisterContext()->GetPC();
- lldb::BreakpointSiteSP bp_site_sp =
- thread_sp->GetProcess()->GetBreakpointSiteList().FindByAddress(pc);
-
- // If a thread is stopped at a breakpoint site, set that as the stop
- // reason even if it hasn't executed the breakpoint instruction yet.
- // We will silently step over the breakpoint when we resume execution
- // and miss the fact that this thread hit the breakpoint.
- if (bp_site_sp && bp_site_sp->ValidForThisThread(*thread_sp)) {
- thread_sp->SetStopInfo(StopInfo::CreateStopReasonWithBreakpointSiteID(
- *thread_sp, bp_site_sp->GetID()));
- handled = true;
- }
}
if (!handled && signo && !did_exec) {
if (signo == SIGTRAP) {
// Currently we are going to assume SIGTRAP means we are either
// hitting a breakpoint or hardware single stepping.
- handled = true;
- addr_t pc =
- thread_sp->GetRegisterContext()->GetPC() + m_breakpoint_pc_offset;
- lldb::BreakpointSiteSP bp_site_sp =
- thread_sp->GetProcess()->GetBreakpointSiteList().FindByAddress(
- pc);
- if (bp_site_sp) {
- // If the breakpoint is for this thread, then we'll report the hit,
- // but if it is for another thread, we can just report no reason.
- // We don't need to worry about stepping over the breakpoint here,
- // that will be taken care of when the thread resumes and notices
- // that there's a breakpoint under the pc.
- if (bp_site_sp->ValidForThisThread(*thread_sp)) {
- if (m_breakpoint_pc_offset != 0)
- thread_sp->GetRegisterContext()->SetPC(pc);
- thread_sp->SetStopInfo(
- StopInfo::CreateStopReasonWithBreakpointSiteID(
- *thread_sp, bp_site_sp->GetID()));
- } else {
- StopInfoSP invalid_stop_info_sp;
- thread_sp->SetStopInfo(invalid_stop_info_sp);
- }
+ // If we were stepping then assume the stop was the result of the
+ // trace.
+ if (thread_sp->GetTemporaryResumeState() == eStateStepping) {
+ thread_sp->SetStopInfo(
+ StopInfo::CreateStopReasonToTrace(*thread_sp));
+ handled = true;
} else {
- // If we were stepping then assume the stop was the result of the
- // trace. If we were not stepping then report the SIGTRAP.
- // FIXME: We are still missing the case where we single step over a
- // trap instruction.
- if (thread_sp->GetTemporaryResumeState() == eStateStepping)
- thread_sp->SetStopInfo(
- StopInfo::CreateStopReasonToTrace(*thread_sp));
- else
- thread_sp->SetStopInfo(StopInfo::CreateStopReasonWithSignal(
- *thread_sp, signo, description.c_str()));
+ addr_t pc = thread_sp->GetRegisterContext()->GetPC() +
+ m_breakpoint_pc_offset;
+ lldb::BreakpointSiteSP bp_site_sp =
+ thread_sp->GetProcess()->GetBreakpointSiteList().FindByAddress(
+ pc);
+
+ if (bp_site_sp) {
+ // If the breakpoint is for this thread, then we'll report the
+ // hit, but if it is for another thread, we can just report no
+ // reason.
+ if (bp_site_sp->ValidForThisThread(*thread_sp)) {
+ if (m_breakpoint_pc_offset != 0)
+ thread_sp->GetRegisterContext()->SetPC(pc);
+ thread_sp->SetStopInfo(
+ StopInfo::CreateStopReasonWithBreakpointSiteID(
+ *thread_sp, bp_site_sp->GetID()));
+ thread_sp->SetThreadHitBreakpointAtAddr(pc);
+ handled = true;
+ }
+ }
}
}
if (!handled)
diff --git a/lldb/source/Plugins/Process/scripted/ScriptedThread.cpp b/lldb/source/Plugins/Process/scripted/ScriptedThread.cpp
index 88a4ca3b0389f..8e60e5ed827f6 100644
--- a/lldb/source/Plugins/Process/scripted/ScriptedThread.cpp
+++ b/lldb/source/Plugins/Process/scripted/ScriptedThread.cpp
@@ -254,6 +254,8 @@ bool ScriptedThread::CalculateStopInfo() {
LLDB_INVALID_BREAK_ID);
stop_info_sp =
StopInfo::CreateStopReasonWithBreakpointSiteID(*this, break_id);
+ if (RegisterContextSP reg_ctx_sp = GetRegisterContext())
+ SetThreadHitBreakpointAtAddr(reg_ctx_sp->GetPC());
} break;
case lldb::eStopReasonSignal: {
uint32_t signal;
@@ -319,6 +321,13 @@ bool ScriptedThread::CalculateStopInfo() {
.str(),
error, LLDBLog::Thread);
}
+ if (RegisterContextSP reg_ctx_sp = GetRegisterContext()) {
+ addr_t pc = reg_ctx_sp->GetPC();
+ if (BreakpointSiteSP bp_site_sp =
+ GetProcess()->GetBreakpointSiteList().FindByAddress(pc)) {
+ SetThreadStoppedAtBreakpointSite(pc);
+ }
+ }
if (!stop_info_sp)
return false;
diff --git a/lldb/source/Target/Thread.cpp b/lldb/source/Target/Thread.cpp
index e75f5a356cec2..4f9d958648e27 100644
--- a/lldb/source/Target/Thread.cpp
+++ b/lldb/source/Target/Thread.cpp
@@ -217,6 +217,8 @@ Thread::Thread(Process &process, lldb::tid_t tid, bool use_invalid_index_id)
m_process_wp(process.shared_from_this()), m_stop_info_sp(),
m_stop_info_stop_id(0), m_stop_info_override_stop_id(0),
m_should_run_before_public_stop(false),
+ m_hit_bp_at_addr(LLDB_INVALID_ADDRESS),
+ m_bpsite_at_stop_pc(LLDB_INVALID_ADDRESS),
m_index_id(use_invalid_index_id ? LLDB_INVALID_INDEX32
: process.GetNextThreadIndexID(tid)),
m_reg_context_sp(), m_state(eStateUnloaded), m_state_mutex(),
@@ -519,6 +521,8 @@ bool Thread::CheckpointThreadState(ThreadStateCheckpoint &saved_state) {
saved_state.current_inlined_depth = GetCurrentInlinedDepth();
saved_state.m_completed_plan_checkpoint =
GetPlans().CheckpointCompletedPlans();
+ saved_state.hit_bp_at_addr = m_hit_bp_at_addr;
+ saved_state.bpsite_at_stop_pc = m_bpsite_at_stop_pc;
return true;
}
@@ -554,6 +558,8 @@ void Thread::RestoreThreadStateFromCheckpoint(
saved_state.current_inlined_depth);
GetPlans().RestoreCompletedPlanCheckpoint(
saved_state.m_completed_plan_checkpoint);
+ m_hit_bp_at_addr = saved_state.hit_bp_at_addr;
+ m_bpsite_at_stop_pc = saved_state.bpsite_at_stop_pc;
}
StateType Thread::GetState() const {
@@ -622,7 +628,12 @@ void Thread::SetupForResume() {
const addr_t thread_pc = reg_ctx_sp->GetPC();
BreakpointSiteSP bp_site_sp =
GetProcess()->GetBreakpointSiteList().FindByAddress(thread_pc);
- if (bp_site_sp) {
+ // Only step over a breakpoint if the thread stopped by hitting the
+ // BreakpointSite at this pc value, or if we're at a breakpoint site
+ // that was added at this pc while stopped/we jumped to a breakpoint
+ // site.
+ if (bp_site_sp &&
+ (thread_pc == m_hit_bp_at_addr || thread_pc != m_bpsite_at_stop_pc)) {
// Note, don't assume there's a ThreadPlanStepOverBreakpoint, the
// target may not require anything special to step over a breakpoint.
@@ -711,6 +722,8 @@ bool Thread::ShouldResume(StateType resume_state) {
if (need_to_resume) {
ClearStackFrames();
+ m_hit_bp_at_addr = LLDB_INVALID_ADDRESS;
+ m_bpsite_at_stop_pc = LLDB_INVALID_ADDRESS;
// Let Thread subclasses do any special work they need to prior to resuming
WillResume(resume_state);
}
@@ -1894,6 +1907,8 @@ Unwind &Thread::GetUnwinder() {
void Thread::Flush() {
ClearStackFrames();
m_reg_context_sp.reset();
+ m_hit_bp_at_addr = LLDB_INVALID_ADDRESS;
+ m_bpsite_at_stop_pc = LLDB_INVALID_ADDRESS;
}
bool Thread::IsStillAtLastBreakpointHit() {
diff --git a/lldb/test/API/functionalities/breakpoint/consecutive_breakpoints/TestConsecutiveBreakpoints.py b/lldb/test/API/functionalities/breakpoint/consecutive_breakpoints/TestConsecutiveBreakpoints.py
index 73de4a294388b..ecea28c6e1f6d 100644
--- a/lldb/test/API/functionalities/breakpoint/consecutive_breakpoints/TestConsecutiveBreakpoints.py
+++ b/lldb/test/API/functionalities/breakpoint/consecutive_breakpoints/TestConsecutiveBreakpoints.py
@@ -2,7 +2,6 @@
Test that we handle breakpoints on consecutive instructions correctly.
"""
-
import lldb
from lldbsuite.test.decorators import *
from lldbsuite.test.lldbtest import *
@@ -64,20 +63,30 @@ def test_single_step(self):
"""Test that single step stops at the second breakpoint."""
self.prepare_test()
+ # Instruction step to the next instruction
+ # We haven't executed breakpoint2 yet, we're sitting at it now.
+ step_over = False
+ self.thread.StepInstruction(step_over)
+
step_over = False
self.thread.StepInstruction(step_over)
+ # We've now hit the breakpoint and this StepInstruction has
+ # been interrupted, it is still sitting on the thread plan stack.
+
self.assertState(self.process.GetState(), lldb.eStateStopped)
self.assertEqual(
self.thread.GetFrameAtIndex(0).GetPCAddress().GetLoadAddress(self.target),
self.bkpt_address.GetLoadAddress(self.target),
)
- self.thread = lldbutil.get_one_thread_stopped_at_breakpoint(
- self.process, self.breakpoint2
- )
- self.assertIsNotNone(
- self.thread, "Expected one thread to be stopped at breakpoint 2"
- )
+
+ # One more instruction to complete the Step that was interrupted
+ # earlier.
+ self.thread.StepInstruction(step_over)
+ strm = lldb.SBStream()
+ self.thread.GetDescription(strm)
+ self.assertIn("instruction step into", strm.GetData())
+ self.assertIsNotNone(self.thread, "Expected to see that step-in had completed")
self.finish_test()
@@ -106,4 +115,7 @@ def test_single_step_thread_specific(self):
"Stop reason should be 'plan complete'",
)
+ # Hit our second breakpoint
+ self.process.Continue()
+
self.finish_test()
diff --git a/lldb/test/API/functionalities/breakpoint/step_over_breakpoint/TestStepOverBreakpoint.py b/lldb/test/API/functionalities/breakpoint/step_over_breakpoint/TestStepOverBreakpoint.py
index 3a7440a31677a..1315288b35c55 100644
--- a/lldb/test/API/functionalities/breakpoint/step_over_breakpoint/TestStepOverBreakpoint.py
+++ b/lldb/test/API/functionalities/breakpoint/step_over_breakpoint/TestStepOverBreakpoint.py
@@ -5,7 +5,6 @@
and eStopReasonPlanComplete when breakpoint's condition fails.
"""
-
import lldb
from lldbsuite.test.decorators import *
from lldbsuite.test.lldbtest import *
@@ -90,6 +89,11 @@ def test_step_instruction(self):
self.assertGreaterEqual(step_count, steps_expected)
break
+ # We did a `stepi` when we hit our last breakpoint, and the stepi was not
+ # completed yet, so when we resume it will complete (running process.Continue()
+ # would have the same result - we step one instruction and stop again when
+ # our interrupted stepi completes).
+ self.thread.StepInstruction(True)
# Run the process until termination
self.process.Continue()
self.assertState(self.process.GetState(), lldb.eStateExited)
>From 2ac364c122bc9bdbc988a6ac11e24d46b9aa452e Mon Sep 17 00:00:00 2001
From: Jason Molenda <jason at molenda.com>
Date: Thu, 20 Jun 2024 20:49:27 -0700
Subject: [PATCH 2/8] Remove call to SetThreadHitBreakpointAtAddr in
ProcessWindows.
There's a comment,
```
The current PC is AFTER the BP opcode, on all architectures
```
and code to decrement the pc value before looking for a BreakpointSite,
if this is how Windows really behaves with breakpoints then we don't
need to silently step past the breakpoint, windows has already done
that.
I was originally fixing a mistake where I called
Thread::SetThreadHitBreakpointAtAddr(pc) only if the breakpoint was
valid for this thread. But we need to mark the breakpoint that way
regardless of whether it is valid for this thread or not -- for a
bp not on this thread, we still need to silently step past it.
But as I was making that change, I suspect none of this is necessary
on Windows, so I'm removing it.
---
lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp | 1 -
1 file changed, 1 deletion(-)
diff --git a/lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp b/lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp
index c4005a1f51150..051901d64ab44 100644
--- a/lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp
+++ b/lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp
@@ -451,7 +451,6 @@ void ProcessWindows::RefreshStateAfterStop() {
stop_info = StopInfo::CreateStopReasonWithBreakpointSiteID(
*stop_thread, site->GetID());
register_context->SetPC(pc);
- stop_thread->SetThreadHitBreakpointAtAddr(pc);
} else {
LLDB_LOG(log,
"Breakpoint site {0} is not valid for this thread, "
>From 789c786d4b2bf0b2a3f005a19e6d4413096dc028 Mon Sep 17 00:00:00 2001
From: Jason Molenda <jason at molenda.com>
Date: Fri, 21 Jun 2024 14:11:40 -0700
Subject: [PATCH 3/8] Correctly set that the thread hit a breakpoint in
ProcessWindows plugin. I misread this method's behavior initially, when I
saw that the stop pc is past the breakpoint instruction. But we're not ready
to resume the process now - we need to set the pc back to the start of the
breakpoint, then do lldb's normal "disable breakpoint, instruction step,
re-insert breakpoint" behavior just like all other targets. Re-adding the
call to register the thread as having hit the breakpoint after testing help
from Aleksandr Korepanov.
---
lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp | 1 +
1 file changed, 1 insertion(+)
diff --git a/lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp b/lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp
index 051901d64ab44..231b22f5f1897 100644
--- a/lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp
+++ b/lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp
@@ -442,6 +442,7 @@ void ProcessWindows::RefreshStateAfterStop() {
m_session_data->m_debugger->GetProcess().GetProcessId(), pc,
site->GetID());
+ stop_thread->SetThreadHitBreakpointAtAddr(pc);
if (site->ValidForThisThread(*stop_thread)) {
LLDB_LOG(log,
"Breakpoint site {0} is valid for this thread ({1:x}), "
>From 8f650136692f2c8b9b376560f0b0965d16748208 Mon Sep 17 00:00:00 2001
From: Jason Molenda <jason at molenda.com>
Date: Mon, 1 Jul 2024 20:43:52 -0700
Subject: [PATCH 4/8] Change to a single setting in the Thread object, to track
if we're at a BreakpointSite that we haven't hit yet, when we stop.
This adds two methods that the Process plugins need
to call.
When a thread has stopped at an enabled BreakpointSite, but has not
yet hit the breakpoint, it will call
Thread::SetThreadStoppedAtUnexecutedBP(pc).
When a thread has hit a breakpoint, it will call
Thread::SetThreadHitBreakpointSite().
These are mutually exclusive, and the last one called wins. So a
Process plugin may first detect that it is at an enabled BreakpointSite,
call SetThreadStoppedAtUnexecutedBP(pc), and later realize breakpoint
was actually hit, and update the state by calling
SetThreadHitBreakpointSite().
When resuming a thread, if SetThreadStoppedAtUnexecutedBP(pc) was
called and the thread's pc is still the same as that value when
the thread stopped, we will execute the Breakpoint and stop again
with a breakpoint-hit stop-reason. Otherwise we will silently
step past the breakpoint.
I've tested this update to the patch on macOS, I need to test on
aarch64 ubuntu still, and ask Aleksandr if it's possible to
check on Windows before we try to merge the PR.
---
lldb/include/lldb/Target/Thread.h | 58 ++++++++++---------
.../Process/Utility/StopInfoMachException.cpp | 21 +++++--
.../Process/Windows/Common/ProcessWindows.cpp | 14 +++--
.../Process/gdb-remote/ProcessGDBRemote.cpp | 13 ++---
.../Process/scripted/ScriptedThread.cpp | 19 +++---
lldb/source/Target/StopInfo.cpp | 2 +
lldb/source/Target/Thread.cpp | 30 +++++-----
7 files changed, 86 insertions(+), 71 deletions(-)
diff --git a/lldb/include/lldb/Target/Thread.h b/lldb/include/lldb/Target/Thread.h
index 1e1aead896018..2f3eaba935664 100644
--- a/lldb/include/lldb/Target/Thread.h
+++ b/lldb/include/lldb/Target/Thread.h
@@ -128,11 +128,9 @@ class Thread : public std::enable_shared_from_this<Thread>,
register_backup_sp; // You need to restore the registers, of course...
uint32_t current_inlined_depth;
lldb::addr_t current_inlined_pc;
- lldb::addr_t
- hit_bp_at_addr; // Set to the address of a breakpoint that we have hit.
- lldb::addr_t bpsite_at_stop_pc; // Set to the address of a breakpoint
- // instruction that we have not yet hit, but
- // will hit when we resume.
+ lldb::addr_t stopped_at_unexecuted_bp; // Set to the address of a breakpoint
+ // instruction that we have not yet
+ // hit, but will hit when we resume.
};
/// Constructor
@@ -382,17 +380,24 @@ class Thread : public std::enable_shared_from_this<Thread>,
virtual void SetQueueLibdispatchQueueAddress(lldb::addr_t dispatch_queue_t) {}
- /// When a thread has executed/trapped a breakpoint, set the address of that
- /// breakpoint so we know it has been hit already, and should be silently
- /// stepped past on resume.
- void SetThreadHitBreakpointAtAddr(lldb::addr_t pc) { m_hit_bp_at_addr = pc; }
-
- /// When a thread stops at a breakpoint instruction/address, but has not yet
- /// executed/triggered it, record that so we can detect when a user adds a
- /// breakpoint (or changes a thread to a breakpoint site) and we need to
- /// silently step past that when resuming.
- void SetThreadStoppedAtBreakpointSite(lldb::addr_t pc) {
- m_bpsite_at_stop_pc = pc;
+ /// When a thread stops at an enabled BreakpointSite that has not exectued,
+ /// the Process plugin should call SetThreadStoppedAtUnexecutedBP(pc).
+ /// If that BreakpointSite was actually triggered (the instruction was
+ /// executed, for a software breakpoint), regardless of wheether the
+ /// breakpoint is valid for this thread, SetThreadHitBreakpointSite()
+ /// should be called to clear that state.
+ ///
+ /// Depending on the structure of the Process plugin, it may be easiest
+ /// to call SetThreadStoppedAtUnexecutedBP(pc) unconditionally when at
+ /// a BreakpointSite, and later when it is known that it was triggered,
+ /// SetThreadHitBreakpointSite() can be called. These two methods
+ /// overwrite the same piece of state in the Thread, the last one
+ /// called on a Thread wins.
+ void SetThreadStoppedAtUnexecutedBP(lldb::addr_t pc) {
+ m_stopped_at_unexecuted_bp = pc;
+ }
+ void SetThreadHitBreakpointSite() {
+ m_stopped_at_unexecuted_bp = LLDB_INVALID_ADDRESS;
}
/// Whether this Thread already has all the Queue information cached or not
@@ -1329,17 +1334,16 @@ class Thread : public std::enable_shared_from_this<Thread>,
bool m_should_run_before_public_stop; // If this thread has "stop others"
// private work to do, then it will
// set this.
- lldb::addr_t m_hit_bp_at_addr; // If this thread originally stopped at a
- // breakpoint instruction, AND HIT IT,
- // record the address of that breakpoint.
- // LLDB_INVALID_ADDRESS if this thread did
- // not stop at a breakpoint insn, or did not
- // hit it yet.
- lldb::addr_t m_bpsite_at_stop_pc; // If this thread originally stopped at a
- // breakpoint site, record the address of
- // that breakpoint site.
- // LLDB_INVALID_ADDRESS if this thread did
- // not stop at a breakpoint site.
+ lldb::addr_t
+ m_stopped_at_unexecuted_bp; // When the thread stops at a
+ // breakpoint instruction/BreakpointSite
+ // but has not yet hit/exectued it,
+ // record the address.
+ // When the thread resumes, if the pc
+ // is still at the same address, we will
+ // hit and trigger the breakpoint.
+ // Otherwise we will silently step past
+ // the breakpoint before resuming.
const uint32_t m_index_id; ///< A unique 1 based index assigned to each thread
/// for easy UI/command line access.
lldb::RegisterContextSP m_reg_context_sp; ///< The register context for this
diff --git a/lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp b/lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp
index 8087ef1accde8..747004b9f1bef 100644
--- a/lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp
+++ b/lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp
@@ -575,6 +575,18 @@ StopInfoSP StopInfoMachException::CreateStopReasonWithMachException(
target ? target->GetArchitecture().GetMachine()
: llvm::Triple::UnknownArch;
+ ProcessSP process_sp(thread.GetProcess());
+ RegisterContextSP reg_ctx_sp(thread.GetRegisterContext());
+ BreakpointSiteSP bp_site_sp;
+ addr_t pc = LLDB_INVALID_ADDRESS;
+ if (reg_ctx_sp) {
+ pc = reg_ctx_sp->GetPC();
+ BreakpointSiteSP bp_site_sp =
+ process_sp->GetBreakpointSiteList().FindByAddress(pc);
+ if (bp_site_sp && bp_site_sp->IsEnabled())
+ thread.SetThreadStoppedAtUnexecutedBP(pc);
+ }
+
switch (exc_type) {
case 1: // EXC_BAD_ACCESS
case 2: // EXC_BAD_INSTRUCTION
@@ -685,9 +697,6 @@ StopInfoSP StopInfoMachException::CreateStopReasonWithMachException(
// may have multiple possible reasons set.
if (stopped_by_hitting_breakpoint) {
- ProcessSP process_sp(thread.GetProcess());
- RegisterContextSP reg_ctx_sp(thread.GetRegisterContext());
- BreakpointSiteSP bp_site_sp;
addr_t pc = LLDB_INVALID_ADDRESS;
if (reg_ctx_sp)
pc = reg_ctx_sp->GetPC() - pc_decrement;
@@ -700,6 +709,10 @@ StopInfoSP StopInfoMachException::CreateStopReasonWithMachException(
bp_site_sp = process_sp->GetBreakpointSiteList().FindByAddress(pc);
}
if (bp_site_sp && bp_site_sp->IsEnabled()) {
+ // We've hit this breakpoint, whether it was intended for this thread
+ // or not. Clear this in the Tread object so we step past it on resume.
+ thread.SetThreadHitBreakpointSite();
+
// If we have an operating system plug-in, we might have set a thread
// specific breakpoint using the operating system thread ID, so we
// can't make any assumptions about the thread ID so we must always
@@ -711,11 +724,9 @@ StopInfoSP StopInfoMachException::CreateStopReasonWithMachException(
// instruction in the code
if (pc_decrement > 0 && adjust_pc_if_needed && reg_ctx_sp)
reg_ctx_sp->SetPC(pc);
- thread.SetThreadHitBreakpointAtAddr(pc);
return StopInfo::CreateStopReasonWithBreakpointSiteID(
thread, bp_site_sp->GetID());
} else {
- thread.SetThreadHitBreakpointAtAddr(pc);
return StopInfoSP();
}
}
diff --git a/lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp b/lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp
index 231b22f5f1897..e89d315309fc5 100644
--- a/lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp
+++ b/lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp
@@ -377,13 +377,17 @@ void ProcessWindows::RefreshStateAfterStop() {
if (!stop_thread)
return;
- switch (active_exception->GetExceptionCode()) {
- case EXCEPTION_SINGLE_STEP: {
- RegisterContextSP register_context = stop_thread->GetRegisterContext();
+ // If we're at a BreakpointSite, mark this as an Unexecuted Breakpoint.
+ // We'll clear that state if we've actually executed the breakpoint.
+ if (RegisterContextSP register_context = stop_thread->GetRegisterContext()) {
const uint64_t pc = register_context->GetPC();
BreakpointSiteSP site(GetBreakpointSiteList().FindByAddress(pc));
if (site)
- stop_thread->SetThreadStoppedAtBreakpointSite(pc);
+ stop_thread->SetThreadStoppedAtUnexecutedBP(pc);
+ }
+
+ switch (active_exception->GetExceptionCode()) {
+ case EXCEPTION_SINGLE_STEP: {
auto *reg_ctx = static_cast<RegisterContextWindows *>(
stop_thread->GetRegisterContext().get());
uint32_t slot_id = reg_ctx->GetTriggeredHardwareBreakpointSlotId();
@@ -442,7 +446,7 @@ void ProcessWindows::RefreshStateAfterStop() {
m_session_data->m_debugger->GetProcess().GetProcessId(), pc,
site->GetID());
- stop_thread->SetThreadHitBreakpointAtAddr(pc);
+ stop_thread->SetThreadHitBreakpointSite();
if (site->ValidForThisThread(*stop_thread)) {
LLDB_LOG(log,
"Breakpoint site {0} is valid for this thread ({1:x}), "
diff --git a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
index bad42d44503fd..f2af07d10bd75 100644
--- a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -1711,8 +1711,8 @@ ThreadSP ProcessGDBRemote::SetThreadStopInfo(
addr_t pc = thread_sp->GetRegisterContext()->GetPC();
BreakpointSiteSP bp_site_sp =
thread_sp->GetProcess()->GetBreakpointSiteList().FindByAddress(pc);
- if (bp_site_sp)
- thread_sp->SetThreadStoppedAtBreakpointSite(pc);
+ if (bp_site_sp && bp_site_sp->IsEnabled())
+ thread_sp->SetThreadStoppedAtUnexecutedBP(pc);
if (exc_type != 0) {
const size_t exc_data_size = exc_data.size();
@@ -1733,11 +1733,7 @@ ThreadSP ProcessGDBRemote::SetThreadStopInfo(
thread_sp->SetStopInfo(StopInfo::CreateStopReasonToTrace(*thread_sp));
handled = true;
} else if (reason == "breakpoint") {
- addr_t pc = thread_sp->GetRegisterContext()->GetPC();
- thread_sp->SetThreadHitBreakpointAtAddr(pc);
- lldb::BreakpointSiteSP bp_site_sp =
- thread_sp->GetProcess()->GetBreakpointSiteList().FindByAddress(
- pc);
+ thread_sp->SetThreadHitBreakpointSite();
if (bp_site_sp) {
// If the breakpoint is for this thread, then we'll report the hit,
// but if it is for another thread, we can just report no reason.
@@ -1749,7 +1745,6 @@ ThreadSP ProcessGDBRemote::SetThreadStopInfo(
thread_sp->SetStopInfo(
StopInfo::CreateStopReasonWithBreakpointSiteID(
*thread_sp, bp_site_sp->GetID()));
- thread_sp->SetThreadHitBreakpointAtAddr(pc);
} else {
StopInfoSP invalid_stop_info_sp;
thread_sp->SetStopInfo(invalid_stop_info_sp);
@@ -1875,6 +1870,7 @@ ThreadSP ProcessGDBRemote::SetThreadStopInfo(
pc);
if (bp_site_sp) {
+ thread_sp->SetThreadHitBreakpointSite();
// If the breakpoint is for this thread, then we'll report the
// hit, but if it is for another thread, we can just report no
// reason.
@@ -1884,7 +1880,6 @@ ThreadSP ProcessGDBRemote::SetThreadStopInfo(
thread_sp->SetStopInfo(
StopInfo::CreateStopReasonWithBreakpointSiteID(
*thread_sp, bp_site_sp->GetID()));
- thread_sp->SetThreadHitBreakpointAtAddr(pc);
handled = true;
}
}
diff --git a/lldb/source/Plugins/Process/scripted/ScriptedThread.cpp b/lldb/source/Plugins/Process/scripted/ScriptedThread.cpp
index 8e60e5ed827f6..b65afb19fa044 100644
--- a/lldb/source/Plugins/Process/scripted/ScriptedThread.cpp
+++ b/lldb/source/Plugins/Process/scripted/ScriptedThread.cpp
@@ -229,6 +229,16 @@ bool ScriptedThread::CalculateStopInfo() {
LLVM_PRETTY_FUNCTION, "Failed to get scripted thread stop info.", error,
LLDBLog::Thread);
+ // If we're at a BreakpointSite, mark that we stopped there and
+ // need to hit the breakpoint when we resume. This will be cleared
+ // if we CreateStopReasonWithBreakpointSiteID.
+ if (RegisterContextSP reg_ctx_sp = GetRegisterContext()) {
+ addr_t pc = reg_ctx_sp->GetPC();
+ if (BreakpointSiteSP bp_site_sp =
+ GetProcess()->GetBreakpointSiteList().FindByAddress(pc))
+ SetThreadStoppedAtUnexecutedBP(pc);
+ }
+
lldb::StopInfoSP stop_info_sp;
lldb::StopReason stop_reason_type;
@@ -254,8 +264,6 @@ bool ScriptedThread::CalculateStopInfo() {
LLDB_INVALID_BREAK_ID);
stop_info_sp =
StopInfo::CreateStopReasonWithBreakpointSiteID(*this, break_id);
- if (RegisterContextSP reg_ctx_sp = GetRegisterContext())
- SetThreadHitBreakpointAtAddr(reg_ctx_sp->GetPC());
} break;
case lldb::eStopReasonSignal: {
uint32_t signal;
@@ -321,13 +329,6 @@ bool ScriptedThread::CalculateStopInfo() {
.str(),
error, LLDBLog::Thread);
}
- if (RegisterContextSP reg_ctx_sp = GetRegisterContext()) {
- addr_t pc = reg_ctx_sp->GetPC();
- if (BreakpointSiteSP bp_site_sp =
- GetProcess()->GetBreakpointSiteList().FindByAddress(pc)) {
- SetThreadStoppedAtBreakpointSite(pc);
- }
- }
if (!stop_info_sp)
return false;
diff --git a/lldb/source/Target/StopInfo.cpp b/lldb/source/Target/StopInfo.cpp
index 95f78056b1644..895306b5ef0d4 100644
--- a/lldb/source/Target/StopInfo.cpp
+++ b/lldb/source/Target/StopInfo.cpp
@@ -1365,6 +1365,8 @@ class StopInfoVForkDone : public StopInfo {
StopInfoSP StopInfo::CreateStopReasonWithBreakpointSiteID(Thread &thread,
break_id_t break_id) {
+ thread.SetThreadHitBreakpointSite();
+
return StopInfoSP(new StopInfoBreakpoint(thread, break_id));
}
diff --git a/lldb/source/Target/Thread.cpp b/lldb/source/Target/Thread.cpp
index 4f9d958648e27..2105efe9305fe 100644
--- a/lldb/source/Target/Thread.cpp
+++ b/lldb/source/Target/Thread.cpp
@@ -217,8 +217,7 @@ Thread::Thread(Process &process, lldb::tid_t tid, bool use_invalid_index_id)
m_process_wp(process.shared_from_this()), m_stop_info_sp(),
m_stop_info_stop_id(0), m_stop_info_override_stop_id(0),
m_should_run_before_public_stop(false),
- m_hit_bp_at_addr(LLDB_INVALID_ADDRESS),
- m_bpsite_at_stop_pc(LLDB_INVALID_ADDRESS),
+ m_stopped_at_unexecuted_bp(LLDB_INVALID_ADDRESS),
m_index_id(use_invalid_index_id ? LLDB_INVALID_INDEX32
: process.GetNextThreadIndexID(tid)),
m_reg_context_sp(), m_state(eStateUnloaded), m_state_mutex(),
@@ -521,8 +520,7 @@ bool Thread::CheckpointThreadState(ThreadStateCheckpoint &saved_state) {
saved_state.current_inlined_depth = GetCurrentInlinedDepth();
saved_state.m_completed_plan_checkpoint =
GetPlans().CheckpointCompletedPlans();
- saved_state.hit_bp_at_addr = m_hit_bp_at_addr;
- saved_state.bpsite_at_stop_pc = m_bpsite_at_stop_pc;
+ saved_state.stopped_at_unexecuted_bp = m_stopped_at_unexecuted_bp;
return true;
}
@@ -558,8 +556,7 @@ void Thread::RestoreThreadStateFromCheckpoint(
saved_state.current_inlined_depth);
GetPlans().RestoreCompletedPlanCheckpoint(
saved_state.m_completed_plan_checkpoint);
- m_hit_bp_at_addr = saved_state.hit_bp_at_addr;
- m_bpsite_at_stop_pc = saved_state.bpsite_at_stop_pc;
+ m_stopped_at_unexecuted_bp = saved_state.stopped_at_unexecuted_bp;
}
StateType Thread::GetState() const {
@@ -628,12 +625,15 @@ void Thread::SetupForResume() {
const addr_t thread_pc = reg_ctx_sp->GetPC();
BreakpointSiteSP bp_site_sp =
GetProcess()->GetBreakpointSiteList().FindByAddress(thread_pc);
- // Only step over a breakpoint if the thread stopped by hitting the
- // BreakpointSite at this pc value, or if we're at a breakpoint site
- // that was added at this pc while stopped/we jumped to a breakpoint
- // site.
- if (bp_site_sp &&
- (thread_pc == m_hit_bp_at_addr || thread_pc != m_bpsite_at_stop_pc)) {
+ // If we're at a BreakpointSite which we have either
+ // 1. already triggered/hit, or
+ // 2. the Breakpoint was added while stopped, or the pc was moved
+ // to this BreakpointSite
+ // Step past the breakpoint before resuming.
+ // If we stopped at a breakpoint instruction/BreakpointSite location
+ // without hitting it, and we're still at that same address on
+ // resuming, then we want to hit the BreakpointSite when we resume.
+ if (bp_site_sp && m_stopped_at_unexecuted_bp != thread_pc) {
// Note, don't assume there's a ThreadPlanStepOverBreakpoint, the
// target may not require anything special to step over a breakpoint.
@@ -722,8 +722,7 @@ bool Thread::ShouldResume(StateType resume_state) {
if (need_to_resume) {
ClearStackFrames();
- m_hit_bp_at_addr = LLDB_INVALID_ADDRESS;
- m_bpsite_at_stop_pc = LLDB_INVALID_ADDRESS;
+ m_stopped_at_unexecuted_bp = LLDB_INVALID_ADDRESS;
// Let Thread subclasses do any special work they need to prior to resuming
WillResume(resume_state);
}
@@ -1907,8 +1906,7 @@ Unwind &Thread::GetUnwinder() {
void Thread::Flush() {
ClearStackFrames();
m_reg_context_sp.reset();
- m_hit_bp_at_addr = LLDB_INVALID_ADDRESS;
- m_bpsite_at_stop_pc = LLDB_INVALID_ADDRESS;
+ m_stopped_at_unexecuted_bp = LLDB_INVALID_ADDRESS;
}
bool Thread::IsStillAtLastBreakpointHit() {
>From 356e56a792b00cb954cb82fc473a7619ac4a0894 Mon Sep 17 00:00:00 2001
From: Jason Molenda <jason at molenda.com>
Date: Mon, 1 Jul 2024 21:45:18 -0700
Subject: [PATCH 5/8] Fix two spots where I forgot to check if the
BreakpointSite was enabled.
---
lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp | 2 +-
lldb/source/Plugins/Process/scripted/ScriptedThread.cpp | 3 ++-
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp b/lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp
index e89d315309fc5..76b4d41a29128 100644
--- a/lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp
+++ b/lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp
@@ -382,7 +382,7 @@ void ProcessWindows::RefreshStateAfterStop() {
if (RegisterContextSP register_context = stop_thread->GetRegisterContext()) {
const uint64_t pc = register_context->GetPC();
BreakpointSiteSP site(GetBreakpointSiteList().FindByAddress(pc));
- if (site)
+ if (site && site->IsEnabled())
stop_thread->SetThreadStoppedAtUnexecutedBP(pc);
}
diff --git a/lldb/source/Plugins/Process/scripted/ScriptedThread.cpp b/lldb/source/Plugins/Process/scripted/ScriptedThread.cpp
index b65afb19fa044..d0d1508e85172 100644
--- a/lldb/source/Plugins/Process/scripted/ScriptedThread.cpp
+++ b/lldb/source/Plugins/Process/scripted/ScriptedThread.cpp
@@ -236,7 +236,8 @@ bool ScriptedThread::CalculateStopInfo() {
addr_t pc = reg_ctx_sp->GetPC();
if (BreakpointSiteSP bp_site_sp =
GetProcess()->GetBreakpointSiteList().FindByAddress(pc))
- SetThreadStoppedAtUnexecutedBP(pc);
+ if (bp_site_sp->IsEnabled())
+ SetThreadStoppedAtUnexecutedBP(pc);
}
lldb::StopInfoSP stop_info_sp;
>From 9c847fff5c784ed3dd9539de0d30ec6874c38e26 Mon Sep 17 00:00:00 2001
From: Jason Molenda <jmolenda at apple.com>
Date: Mon, 8 Jul 2024 18:55:19 -0700
Subject: [PATCH 6/8] Fix compile time error in ProcessWindows
I'm setting `register_context`, `site`, and `pc` before either of
the EXCEPTION_SINGLE_STEP or EXCEPTION_BREAKPOINT blocks, and they
are used/updated in both. Thanks again to Aleksandr for helping
me to avoid extra round-trips through the CI, and testing that it
behaves correctly once these are straightened out.
---
.../Process/Windows/Common/ProcessWindows.cpp | 18 ++++++++----------
1 file changed, 8 insertions(+), 10 deletions(-)
diff --git a/lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp b/lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp
index 76b4d41a29128..8b12b87eacbc6 100644
--- a/lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp
+++ b/lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp
@@ -377,14 +377,14 @@ void ProcessWindows::RefreshStateAfterStop() {
if (!stop_thread)
return;
+ RegisterContextSP register_context = stop_thread->GetRegisterContext();
+ uint64_t pc = register_context->GetPC();
+
// If we're at a BreakpointSite, mark this as an Unexecuted Breakpoint.
// We'll clear that state if we've actually executed the breakpoint.
- if (RegisterContextSP register_context = stop_thread->GetRegisterContext()) {
- const uint64_t pc = register_context->GetPC();
- BreakpointSiteSP site(GetBreakpointSiteList().FindByAddress(pc));
- if (site && site->IsEnabled())
- stop_thread->SetThreadStoppedAtUnexecutedBP(pc);
- }
+ BreakpointSiteSP site(GetBreakpointSiteList().FindByAddress(pc));
+ if (site && site->IsEnabled())
+ stop_thread->SetThreadStoppedAtUnexecutedBP(pc);
switch (active_exception->GetExceptionCode()) {
case EXCEPTION_SINGLE_STEP: {
@@ -412,8 +412,6 @@ void ProcessWindows::RefreshStateAfterStop() {
}
case EXCEPTION_BREAKPOINT: {
- RegisterContextSP register_context = stop_thread->GetRegisterContext();
-
int breakpoint_size = 1;
switch (GetTarget().GetArchitecture().GetMachine()) {
case llvm::Triple::aarch64:
@@ -436,9 +434,9 @@ void ProcessWindows::RefreshStateAfterStop() {
}
// The current PC is AFTER the BP opcode, on all architectures.
- uint64_t pc = register_context->GetPC() - breakpoint_size;
+ pc = register_context->GetPC() - breakpoint_size;
- BreakpointSiteSP site(GetBreakpointSiteList().FindByAddress(pc));
+ site = GetBreakpointSiteList().FindByAddress(pc);
if (site) {
LLDB_LOG(log,
"detected breakpoint in process {0} at address {1:x} with "
>From 72dfcdb39757642a4ef52f872bcfc2685c0ca4fe Mon Sep 17 00:00:00 2001
From: Jason Molenda <jmolenda at apple.com>
Date: Wed, 17 Jul 2024 19:00:12 -0700
Subject: [PATCH 7/8] Address Jim's feedbacks
I agreed with nearly all of his feedback and implemented them. I
replied to the handful on github where I had a different view of
something.
---
lldb/include/lldb/Target/Thread.h | 21 +--
.../Process/Utility/StopInfoMachException.cpp | 171 ++++++++++--------
.../Process/gdb-remote/ProcessGDBRemote.cpp | 68 ++++---
3 files changed, 145 insertions(+), 115 deletions(-)
diff --git a/lldb/include/lldb/Target/Thread.h b/lldb/include/lldb/Target/Thread.h
index 2f3eaba935664..45951c8166175 100644
--- a/lldb/include/lldb/Target/Thread.h
+++ b/lldb/include/lldb/Target/Thread.h
@@ -128,9 +128,7 @@ class Thread : public std::enable_shared_from_this<Thread>,
register_backup_sp; // You need to restore the registers, of course...
uint32_t current_inlined_depth;
lldb::addr_t current_inlined_pc;
- lldb::addr_t stopped_at_unexecuted_bp; // Set to the address of a breakpoint
- // instruction that we have not yet
- // hit, but will hit when we resume.
+ lldb::addr_t stopped_at_unexecuted_bp;
};
/// Constructor
@@ -383,9 +381,9 @@ class Thread : public std::enable_shared_from_this<Thread>,
/// When a thread stops at an enabled BreakpointSite that has not exectued,
/// the Process plugin should call SetThreadStoppedAtUnexecutedBP(pc).
/// If that BreakpointSite was actually triggered (the instruction was
- /// executed, for a software breakpoint), regardless of wheether the
+ /// executed, for a software breakpoint), regardless of whether the
/// breakpoint is valid for this thread, SetThreadHitBreakpointSite()
- /// should be called to clear that state.
+ /// should be called to record that fact.
///
/// Depending on the structure of the Process plugin, it may be easiest
/// to call SetThreadStoppedAtUnexecutedBP(pc) unconditionally when at
@@ -1334,16 +1332,9 @@ class Thread : public std::enable_shared_from_this<Thread>,
bool m_should_run_before_public_stop; // If this thread has "stop others"
// private work to do, then it will
// set this.
- lldb::addr_t
- m_stopped_at_unexecuted_bp; // When the thread stops at a
- // breakpoint instruction/BreakpointSite
- // but has not yet hit/exectued it,
- // record the address.
- // When the thread resumes, if the pc
- // is still at the same address, we will
- // hit and trigger the breakpoint.
- // Otherwise we will silently step past
- // the breakpoint before resuming.
+ lldb::addr_t m_stopped_at_unexecuted_bp; // Set to the address of a breakpoint
+ // instruction that we have not yet
+ // hit, but will hit when we resume.
const uint32_t m_index_id; ///< A unique 1 based index assigned to each thread
/// for easy UI/command line access.
lldb::RegisterContextSP m_reg_context_sp; ///< The register context for this
diff --git a/lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp b/lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp
index 747004b9f1bef..e809ffbc1a720 100644
--- a/lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp
+++ b/lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp
@@ -577,15 +577,18 @@ StopInfoSP StopInfoMachException::CreateStopReasonWithMachException(
ProcessSP process_sp(thread.GetProcess());
RegisterContextSP reg_ctx_sp(thread.GetRegisterContext());
- BreakpointSiteSP bp_site_sp;
- addr_t pc = LLDB_INVALID_ADDRESS;
- if (reg_ctx_sp) {
- pc = reg_ctx_sp->GetPC();
- BreakpointSiteSP bp_site_sp =
- process_sp->GetBreakpointSiteList().FindByAddress(pc);
- if (bp_site_sp && bp_site_sp->IsEnabled())
- thread.SetThreadStoppedAtUnexecutedBP(pc);
- }
+ // Caveat: with x86 KDP if we've hit a breakpoint, the pc we
+ // receive is past the breakpoint instruction.
+ // If we have a breakpoint at 0x100 (on a 1-byte original instruction)
+ // and at 0x101, we hit the 0x100 breakpoint and the pc is
+ // reported at 0x101. We will initially mark this as being at
+ // an unexecuted breakpoint at 0x101, but that will be refined
+ // later in this method after we've correctly decremented pc.
+ addr_t pc = reg_ctx_sp->GetPC();
+ BreakpointSiteSP bp_site_sp =
+ process_sp->GetBreakpointSiteList().FindByAddress(pc);
+ if (bp_site_sp && bp_site_sp->IsEnabled())
+ thread.SetThreadStoppedAtUnexecutedBP(pc);
switch (exc_type) {
case 1: // EXC_BAD_ACCESS
@@ -613,69 +616,82 @@ StopInfoSP StopInfoMachException::CreateStopReasonWithMachException(
}
break;
- // [exc_type, exc_code, exc_sub_code, exc_sub_sub_code]
- //
- // Instruction step:
- // [6, 1, 0]
- // Intel KDP [6, 3, ??]
- // armv7 [6, 0x102, <stop-pc>] Same as software breakpoint!
- //
- // Software breakpoint:
- // x86 [6, 2, 0]
- // Intel KDP [6, 2, <bp-addr + 1>]
- // arm64 [6, 1, <bp-addr>]
- // armv7 [6, 0x102, <bp-addr>] Same as instruction step!
- //
- // Hardware breakpoint:
- // x86 [6, 1, <bp-addr>, 0]
- // x86/Rosetta not implemented, see software breakpoint
- // arm64 [6, 1, <bp-addr>]
- // armv7 not implemented, see software breakpoint
- //
- // Hardware watchpoint:
- // x86 [6, 1, <accessed-addr>, 0] (both Intel hw and Rosetta)
- // arm64 [6, 0x102, <accessed-addr>, 0]
- // armv7 [6, 0x102, <accessed-addr>, 0]
- //
- // arm64 BRK instruction (imm arg not reflected in the ME)
- // [ 6, 1, <addr-of-BRK-insn>]
- //
- // In order of codes mach exceptions:
- // [6, 1, 0] - instruction step
- // [6, 1, <bp-addr>] - hardware breakpoint or watchpoint
- //
- // [6, 2, 0] - software breakpoint
- // [6, 2, <bp-addr + 1>] - software breakpoint
- //
- // [6, 3] - instruction step
- //
- // [6, 0x102, <stop-pc>] armv7 instruction step
- // [6, 0x102, <bp-addr>] armv7 software breakpoint
- // [6, 0x102, <accessed-addr>, 0] arm64/armv7 watchpoint
+ // A mach exception comes with 2-4 pieces of data.
+ // The sub-codes are only provided for certain types
+ // of mach exceptions.
+ // [exc_type, exc_code, exc_sub_code, exc_sub_sub_code]
+ //
+ // Here are all of the EXC_BREAKPOINT, exc_type==6,
+ // exceptions we can receive.
+ //
+ // Instruction step:
+ // [6, 1, 0]
+ // Intel KDP [6, 3, ??]
+ // armv7 [6, 0x102, <stop-pc>] Same as software breakpoint!
+ //
+ // Software breakpoint:
+ // x86 [6, 2, 0]
+ // Intel KDP [6, 2, <bp-addr + 1>]
+ // arm64 [6, 1, <bp-addr>]
+ // armv7 [6, 0x102, <bp-addr>] Same as instruction step!
+ //
+ // Hardware breakpoint:
+ // x86 [6, 1, <bp-addr>, 0]
+ // x86/Rosetta not implemented, see software breakpoint
+ // arm64 [6, 1, <bp-addr>]
+ // armv7 not implemented, see software breakpoint
+ //
+ // Hardware watchpoint:
+ // x86 [6, 1, <accessed-addr>, 0] (both Intel hw and Rosetta)
+ // arm64 [6, 0x102, <accessed-addr>, 0]
+ // armv7 [6, 0x102, <accessed-addr>, 0]
+ //
+ // arm64 BRK instruction (imm arg not reflected in the ME)
+ // [ 6, 1, <addr-of-BRK-insn>]
+ //
+ // In order of codes mach exceptions:
+ // [6, 1, 0] - instruction step
+ // [6, 1, <bp-addr>] - hardware breakpoint or watchpoint
+ //
+ // [6, 2, 0] - software breakpoint
+ // [6, 2, <bp-addr + 1>] - software breakpoint
+ //
+ // [6, 3] - instruction step
+ //
+ // [6, 0x102, <stop-pc>] armv7 instruction step
+ // [6, 0x102, <bp-addr>] armv7 software breakpoint
+ // [6, 0x102, <accessed-addr>, 0] arm64/armv7 watchpoint
+
case 6: // EXC_BREAKPOINT
{
bool stopped_by_hitting_breakpoint = false;
bool stopped_by_completing_stepi = false;
bool stopped_watchpoint = false;
- std::optional<addr_t> value;
+ std::optional<addr_t> address;
// exc_code 1
- if (exc_code == 1 && exc_sub_code == 0)
- stopped_by_completing_stepi = true;
- if (exc_code == 1 && exc_sub_code != 0) {
- stopped_by_hitting_breakpoint = true;
- stopped_watchpoint = true;
- value = exc_sub_code;
+ if (exc_code == 1) {
+ if (exc_sub_code == 0) {
+ stopped_by_completing_stepi = true;
+ } else {
+ // Ambiguous: could be signalling a
+ // breakpoint or watchpoint hit.
+ stopped_by_hitting_breakpoint = true;
+ stopped_watchpoint = true;
+ address = exc_sub_code;
+ }
}
// exc_code 2
- if (exc_code == 2 && exc_sub_code == 0)
- stopped_by_hitting_breakpoint = true;
- if (exc_code == 2 && exc_sub_code != 0) {
- stopped_by_hitting_breakpoint = true;
- // Intel KDP software breakpoint
- if (!pc_already_adjusted)
- pc_decrement = 1;
+ if (exc_code == 2) {
+ if (exc_sub_code == 0)
+ stopped_by_hitting_breakpoint = true;
+ else {
+ stopped_by_hitting_breakpoint = true;
+ // Intel KDP software breakpoint
+ if (!pc_already_adjusted)
+ pc_decrement = 1;
+ }
}
// exc_code 3
@@ -689,22 +705,20 @@ StopInfoSP StopInfoMachException::CreateStopReasonWithMachException(
stopped_by_completing_stepi = true;
}
stopped_watchpoint = true;
- value = exc_sub_code;
+ address = exc_sub_code;
}
- // Go through the reasons why we stopped, starting
- // with the easiest to detect unambiguously. We
- // may have multiple possible reasons set.
+ // The Mach Exception may have been ambiguous --
+ // e.g. we stopped either because of a breakpoint
+ // or a watchpoint. We'll disambiguate which it
+ // really was.
if (stopped_by_hitting_breakpoint) {
- addr_t pc = LLDB_INVALID_ADDRESS;
- if (reg_ctx_sp)
- pc = reg_ctx_sp->GetPC() - pc_decrement;
- else if (value)
- pc = *value;
-
- if (value)
- bp_site_sp = process_sp->GetBreakpointSiteList().FindByAddress(*value);
+ addr_t pc = reg_ctx_sp->GetPC() - pc_decrement;
+
+ if (address)
+ bp_site_sp =
+ process_sp->GetBreakpointSiteList().FindByAddress(*address);
if (!bp_site_sp && reg_ctx_sp) {
bp_site_sp = process_sp->GetBreakpointSiteList().FindByAddress(pc);
}
@@ -732,16 +746,21 @@ StopInfoSP StopInfoMachException::CreateStopReasonWithMachException(
}
}
- if (stopped_watchpoint && value) {
+ // Breakpoint-hit events are handled.
+ // Now handle watchpoints.
+
+ if (stopped_watchpoint && address) {
WatchpointResourceSP wp_rsrc_sp =
target->GetProcessSP()->GetWatchpointResourceList().FindByAddress(
- *value);
+ *address);
if (wp_rsrc_sp && wp_rsrc_sp->GetNumberOfConstituents() > 0) {
return StopInfo::CreateStopReasonWithWatchpointID(
thread, wp_rsrc_sp->GetConstituentAtIndex(0)->GetID());
}
}
+ // Finally, handle instruction step.
+
if (stopped_by_completing_stepi) {
if (thread.GetTemporaryResumeState() != eStateStepping)
not_stepping_but_got_singlestep_exception = true;
diff --git a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
index f2af07d10bd75..141ed353594f8 100644
--- a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -1856,33 +1856,53 @@ ThreadSP ProcessGDBRemote::SetThreadStopInfo(
// Currently we are going to assume SIGTRAP means we are either
// hitting a breakpoint or hardware single stepping.
- // If we were stepping then assume the stop was the result of the
- // trace.
- if (thread_sp->GetTemporaryResumeState() == eStateStepping) {
- thread_sp->SetStopInfo(
- StopInfo::CreateStopReasonToTrace(*thread_sp));
- handled = true;
- } else {
- addr_t pc = thread_sp->GetRegisterContext()->GetPC() +
- m_breakpoint_pc_offset;
- lldb::BreakpointSiteSP bp_site_sp =
- thread_sp->GetProcess()->GetBreakpointSiteList().FindByAddress(
- pc);
+ // We can't disambiguate between stepping-to-a-breakpointsite and
+ // hitting-a-breakpointsite.
+ //
+ // A user can instruction-step, and be stopped at a BreakpointSite.
+ // Or a user can be sitting at a BreakpointSite,
+ // instruction-step which hits the breakpoint and the pc does not
+ // advance.
+ //
+ // In both cases, we're at a BreakpointSite when stopped, and
+ // the resume state was eStateStepping.
- if (bp_site_sp) {
+ // Assume if we're at a BreakpointSite, we hit it.
+ handled = true;
+ addr_t pc =
+ thread_sp->GetRegisterContext()->GetPC() + m_breakpoint_pc_offset;
+ BreakpointSiteSP bp_site_sp =
+ thread_sp->GetProcess()->GetBreakpointSiteList().FindByAddress(
+ pc);
+
+ // We can't know if we hit it or not. So if we are stopped at
+ // a BreakpointSite, assume we hit it, and should step past the
+ // breakpoint when we resume. This is contrary to how we handle
+ // BreakpointSites in any other location, but we can't know for
+ // sure what happened so it's a reasonable default.
+ if (bp_site_sp) {
+ if (bp_site_sp->IsEnabled())
thread_sp->SetThreadHitBreakpointSite();
- // If the breakpoint is for this thread, then we'll report the
- // hit, but if it is for another thread, we can just report no
- // reason.
- if (bp_site_sp->ValidForThisThread(*thread_sp)) {
- if (m_breakpoint_pc_offset != 0)
- thread_sp->GetRegisterContext()->SetPC(pc);
- thread_sp->SetStopInfo(
- StopInfo::CreateStopReasonWithBreakpointSiteID(
- *thread_sp, bp_site_sp->GetID()));
- handled = true;
- }
+
+ if (bp_site_sp->ValidForThisThread(*thread_sp)) {
+ if (m_breakpoint_pc_offset != 0)
+ thread_sp->GetRegisterContext()->SetPC(pc);
+ thread_sp->SetStopInfo(
+ StopInfo::CreateStopReasonWithBreakpointSiteID(
+ *thread_sp, bp_site_sp->GetID()));
+ } else {
+ StopInfoSP invalid_stop_info_sp;
+ thread_sp->SetStopInfo(invalid_stop_info_sp);
}
+ } else {
+ // If we were stepping then assume the stop was the result of the
+ // trace. If we were not stepping then report the SIGTRAP.
+ if (thread_sp->GetTemporaryResumeState() == eStateStepping)
+ thread_sp->SetStopInfo(
+ StopInfo::CreateStopReasonToTrace(*thread_sp));
+ else
+ thread_sp->SetStopInfo(StopInfo::CreateStopReasonWithSignal(
+ *thread_sp, signo, description.c_str()));
}
}
if (!handled)
>From 7a224e74c49ec5ca8cacc6c4802b2e1190b3f99c Mon Sep 17 00:00:00 2001
From: Jason Molenda <jmolenda at apple.com>
Date: Fri, 19 Jul 2024 14:28:53 -0700
Subject: [PATCH 8/8] Fix typeo, rewrite a comment explaining why x86 KDP is OK
---
lldb/include/lldb/Target/Thread.h | 2 +-
.../Process/Utility/StopInfoMachException.cpp | 14 +++++++++-----
2 files changed, 10 insertions(+), 6 deletions(-)
diff --git a/lldb/include/lldb/Target/Thread.h b/lldb/include/lldb/Target/Thread.h
index 45951c8166175..78ced6fb43a77 100644
--- a/lldb/include/lldb/Target/Thread.h
+++ b/lldb/include/lldb/Target/Thread.h
@@ -378,7 +378,7 @@ class Thread : public std::enable_shared_from_this<Thread>,
virtual void SetQueueLibdispatchQueueAddress(lldb::addr_t dispatch_queue_t) {}
- /// When a thread stops at an enabled BreakpointSite that has not exectued,
+ /// When a thread stops at an enabled BreakpointSite that has not executed,
/// the Process plugin should call SetThreadStoppedAtUnexecutedBP(pc).
/// If that BreakpointSite was actually triggered (the instruction was
/// executed, for a software breakpoint), regardless of whether the
diff --git a/lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp b/lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp
index e809ffbc1a720..f1853be12638e 100644
--- a/lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp
+++ b/lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp
@@ -579,11 +579,15 @@ StopInfoSP StopInfoMachException::CreateStopReasonWithMachException(
RegisterContextSP reg_ctx_sp(thread.GetRegisterContext());
// Caveat: with x86 KDP if we've hit a breakpoint, the pc we
// receive is past the breakpoint instruction.
- // If we have a breakpoint at 0x100 (on a 1-byte original instruction)
- // and at 0x101, we hit the 0x100 breakpoint and the pc is
- // reported at 0x101. We will initially mark this as being at
- // an unexecuted breakpoint at 0x101, but that will be refined
- // later in this method after we've correctly decremented pc.
+ // If we have a breakpoints at 0x100 and 0x101, we hit the
+ // 0x100 breakpoint and the pc is reported at 0x101.
+ // We will initially mark this thread as being stopped at an
+ // unexecuted breakpoint at 0x101. Later when we see that
+ // we stopped for a Breakpoint reason, we will decrement the
+ // pc, and update the thread to record that we hit the
+ // breakpoint at 0x100.
+ // The fact that the pc may be off by one at this point
+ // (for an x86 KDP breakpoint hit) is not a problem.
addr_t pc = reg_ctx_sp->GetPC();
BreakpointSiteSP bp_site_sp =
process_sp->GetBreakpointSiteList().FindByAddress(pc);
More information about the lldb-commits
mailing list