llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-lldb Author: Jason Molenda (jasonmolenda) <details> <summary>Changes</summary> This is a PR to track the changes I need to make to https://github.com/llvm/llvm-project/pull/96260 to address the CI bot failures when I merged that PR, as well as investigating the windows problem Martin Storsjö saw. There's likely going to be 4-5 additional changes to this PR before it's ready to be merged again, I'll create a new PR with the patches I originally landed and fixing those separate issues in additional commits. Issues: 1. lldb-server armv7/aarch32 stepping-by-breakpoint doesn't verify the pc changed as expected, when reporting a `trace` stop-reason, so it will fail to recognize stepping over a breakpoint correctly. 2. ProcessGDBRemote needs to add a `reason = "breakpoint"` when we have a `swbreak` or `hwbreak` from the remote stub, like it does today for `watch`/`awatch`. https://github.com/llvm/llvm-project/pull/102873 3. debugserver on armv7k watches is doesn't correctly report a "step by breakpoint" as a `trace` event, need to verify that we can step / step past breakpoints correctly on old watches running that old debugserver. 4. fix debuginfo dexter fails because stepping behavior was different. 5. Look into the Martin Storsjö `finish` failure on windows. --- Patch is 35.61 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/105594.diff 9 Files Affected: - (modified) lldb/include/lldb/Target/Thread.h (+24) - (modified) lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp (+148-174) - (modified) lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp (+12-20) - (modified) lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp (+31-62) - (modified) lldb/source/Plugins/Process/scripted/ScriptedThread.cpp (+11) - (modified) lldb/source/Target/StopInfo.cpp (+2) - (modified) lldb/source/Target/Thread.cpp (+14-1) - (modified) lldb/test/API/functionalities/breakpoint/consecutive_breakpoints/TestConsecutiveBreakpoints.py (+19-7) - (modified) lldb/test/API/functionalities/breakpoint/step_over_breakpoint/TestStepOverBreakpoint.py (+5-1) ``````````diff diff --git a/lldb/include/lldb/Target/Thread.h b/lldb/include/lldb/Target/Thread.h index 38b65b2bc58490..22d452c7b4b8a3 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 @@ -380,6 +381,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 @@ -1314,6 +1335,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 25cee369d7ee3d..f1853be12638ea 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) { @@ -607,6 +575,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 @@ -633,171 +620,158 @@ 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 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. + // 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 + // 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); 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 f383b3d40a4f3a..8b12b87eacbc61 100644 --- a/lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp +++ b/lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp @@ -377,24 +377,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(); @@ -419,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: @@ -443,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 breakp... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/105594 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
