https://github.com/jasonmolenda created https://github.com/llvm/llvm-project/pull/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 https://github.com/llvm/llvm-project/pull/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 >From 1c39eef50d3b34f4ab7303d9e357b9eccc6f91a1 Mon Sep 17 00:00:00 2001 From: Jason Molenda <jmole...@apple.com> Date: Wed, 12 Feb 2025 15:55:50 -0800 Subject: [PATCH] [lldb] Change lldb's breakpoint handling behavior, reland 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 https://github.com/llvm/llvm-project/pull/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 --- lldb/include/lldb/Target/Thread.h | 24 ++ .../Process/Utility/StopInfoMachException.cpp | 315 ++++++++---------- .../Process/Windows/Common/ProcessWindows.cpp | 32 +- .../Process/gdb-remote/ProcessGDBRemote.cpp | 93 ++---- .../Process/scripted/ScriptedThread.cpp | 11 + lldb/source/Target/StopInfo.cpp | 2 + lldb/source/Target/Thread.cpp | 15 +- .../TestConsecutiveBreakpoints.py | 26 +- .../TestStepOverBreakpoint.py | 6 +- 9 files changed, 263 insertions(+), 261 deletions(-) diff --git a/lldb/include/lldb/Target/Thread.h b/lldb/include/lldb/Target/Thread.h index 9749fd8d575a1..1d1e3dcfc1dc6 100644 --- a/lldb/include/lldb/Target/Thread.h +++ b/lldb/include/lldb/Target/Thread.h @@ -131,6 +131,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; }; /// Constructor @@ -383,6 +384,26 @@ 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 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 + /// breakpoint is valid for this thread, SetThreadHitBreakpointSite() + /// 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 + /// 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 /// /// A Thread may be associated with a libdispatch work Queue at a given @@ -1337,6 +1358,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; // 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 698ba0f0f720f..29a64a2a03bf0 100644 --- a/lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp +++ b/lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp @@ -491,38 +491,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) { @@ -610,6 +578,25 @@ StopInfoSP StopInfoMachException::CreateStopReasonWithMachException( target ? target->GetArchitecture().GetMachine() : llvm::Triple::UnknownArch; + ProcessSP process_sp(thread.GetProcess()); + 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 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); + if (bp_site_sp && bp_site_sp->IsEnabled()) + thread.SetThreadStoppedAtUnexecutedBP(pc); + switch (exc_type) { case 1: // EXC_BAD_ACCESS case 2: // EXC_BAD_INSTRUCTION @@ -636,166 +623,154 @@ StopInfoSP StopInfoMachException::CreateStopReasonWithMachException( } break; + // 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 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; + bool stopped_by_hitting_breakpoint = false; + bool stopped_by_completing_stepi = false; + bool stopped_watchpoint = false; + std::optional<addr_t> address; + + // exc_code 1 + 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) { + 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; } - break; + } - 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 3 + if (exc_code == 3) + stopped_by_completing_stepi = true; - 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 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; + address = exc_sub_code; } - default: - break; - } + // 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 (is_actual_breakpoint) { - RegisterContextSP reg_ctx_sp(thread.GetRegisterContext()); + if (stopped_by_hitting_breakpoint) { addr_t pc = reg_ctx_sp->GetPC() - pc_decrement; - ProcessSP process_sp(thread.CalculateProcess()); - - lldb::BreakpointSiteSP bp_site_sp; - if (process_sp) + 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); + } 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 (bp_site_sp->ValidForThisThread(thread)) + // 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 (bp_site_sp->ValidForThisThread(thread)) { + // Update the PC if we were asked to do so, but only do so if we find + // a breakpoint that we know about because 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); + return StopInfo::CreateStopReasonWithBreakpointSiteID( thread, bp_site_sp->GetID()); - else if (is_trace_if_actual_breakpoint_missing) - return StopInfo::CreateStopReasonToTrace(thread); - else + } else { 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); + // Breakpoint-hit events are handled. + // Now handle watchpoints. + + if (stopped_watchpoint && address) { + WatchpointResourceSP wp_rsrc_sp = + target->GetProcessSP()->GetWatchpointResourceList().FindByAddress( + *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; + 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 1bdacec221695..1528c8bbc2a36 100644 --- a/lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp +++ b/lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp @@ -410,24 +410,17 @@ void ProcessWindows::RefreshStateAfterStop() { if (!stop_thread) return; - switch (active_exception->GetExceptionCode()) { - case EXCEPTION_SINGLE_STEP: { - 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); + RegisterContextSP register_context = stop_thread->GetRegisterContext(); + uint64_t pc = register_context->GetPC(); - return; - } + // If we're at a BreakpointSite, mark this as an Unexecuted Breakpoint. + // We'll clear that state if we've actually executed the breakpoint. + BreakpointSiteSP site(GetBreakpointSiteList().FindByAddress(pc)); + if (site && site->IsEnabled()) + 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(); @@ -452,8 +445,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: @@ -476,9 +467,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 " @@ -486,6 +477,7 @@ void ProcessWindows::RefreshStateAfterStop() { m_session_data->m_debugger->GetProcess().GetProcessId(), pc, site->GetID()); + 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 07b4470d06191..f36595145e035 100644 --- a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp +++ b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp @@ -1600,29 +1600,8 @@ bool ProcessGDBRemote::CalculateThreadStopInfo(ThreadGDBRemote *thread) { // If we have "jstopinfo" then we have stop descriptions for all threads // that have stop reasons, and if there is no entry for a thread, then it // has no stop reason. - 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; } @@ -1727,6 +1706,12 @@ ThreadSP ProcessGDBRemote::SetThreadStopInfo( if (!thread_sp->StopInfoIsUpToDate()) { thread_sp->SetStopInfo(StopInfoSP()); + addr_t pc = thread_sp->GetRegisterContext()->GetPC(); + BreakpointSiteSP bp_site_sp = + thread_sp->GetProcess()->GetBreakpointSiteList().FindByAddress(pc); + if (bp_site_sp && bp_site_sp->IsEnabled()) + thread_sp->SetThreadStoppedAtUnexecutedBP(pc); + if (exc_type != 0) { // For thread plan async interrupt, creating stop info on the // original async interrupt request thread instead. If interrupt thread @@ -1753,26 +1738,10 @@ 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(); - 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. @@ -1888,39 +1857,41 @@ 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. + + // 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. + + // Assume if we're at a BreakpointSite, we hit it. handled = true; addr_t pc = thread_sp->GetRegisterContext()->GetPC() + m_breakpoint_pc_offset; - lldb::BreakpointSiteSP bp_site_sp = + 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 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->IsEnabled()) + thread_sp->SetThreadHitBreakpointSite(); + if (bp_site_sp->ValidForThisThread(*thread_sp)) { if (m_breakpoint_pc_offset != 0) thread_sp->GetRegisterContext()->SetPC(pc); @@ -1934,8 +1905,6 @@ ThreadSP ProcessGDBRemote::SetThreadStopInfo( } 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)); diff --git a/lldb/source/Plugins/Process/scripted/ScriptedThread.cpp b/lldb/source/Plugins/Process/scripted/ScriptedThread.cpp index 88a4ca3b0389f..d0d1508e85172 100644 --- a/lldb/source/Plugins/Process/scripted/ScriptedThread.cpp +++ b/lldb/source/Plugins/Process/scripted/ScriptedThread.cpp @@ -229,6 +229,17 @@ 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)) + if (bp_site_sp->IsEnabled()) + SetThreadStoppedAtUnexecutedBP(pc); + } + lldb::StopInfoSP stop_info_sp; lldb::StopReason stop_reason_type; diff --git a/lldb/source/Target/StopInfo.cpp b/lldb/source/Target/StopInfo.cpp index 356917a45b7b3..092d78d87a2b1 100644 --- a/lldb/source/Target/StopInfo.cpp +++ b/lldb/source/Target/StopInfo.cpp @@ -1445,6 +1445,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 b526131097061..50f7c73f2c4c1 100644 --- a/lldb/source/Target/Thread.cpp +++ b/lldb/source/Target/Thread.cpp @@ -223,6 +223,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_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(), @@ -525,6 +526,7 @@ bool Thread::CheckpointThreadState(ThreadStateCheckpoint &saved_state) { saved_state.current_inlined_depth = GetCurrentInlinedDepth(); saved_state.m_completed_plan_checkpoint = GetPlans().CheckpointCompletedPlans(); + saved_state.stopped_at_unexecuted_bp = m_stopped_at_unexecuted_bp; return true; } @@ -560,6 +562,7 @@ void Thread::RestoreThreadStateFromCheckpoint( saved_state.current_inlined_depth); GetPlans().RestoreCompletedPlanCheckpoint( saved_state.m_completed_plan_checkpoint); + m_stopped_at_unexecuted_bp = saved_state.stopped_at_unexecuted_bp; } StateType Thread::GetState() const { @@ -636,7 +639,15 @@ bool Thread::SetupForResume() { const addr_t thread_pc = reg_ctx_sp->GetPC(); BreakpointSiteSP bp_site_sp = GetProcess()->GetBreakpointSiteList().FindByAddress(thread_pc); - if (bp_site_sp) { + // 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. @@ -727,6 +738,7 @@ bool Thread::ShouldResume(StateType resume_state) { if (need_to_resume) { ClearStackFrames(); + m_stopped_at_unexecuted_bp = LLDB_INVALID_ADDRESS; // Let Thread subclasses do any special work they need to prior to resuming WillResume(resume_state); } @@ -1923,6 +1935,7 @@ Unwind &Thread::GetUnwinder() { void Thread::Flush() { ClearStackFrames(); m_reg_context_sp.reset(); + m_stopped_at_unexecuted_bp = 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) _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
