https://github.com/felipepiovezan created https://github.com/llvm/llvm-project/pull/192971
This patch changes the Process class so that it delays *physically* enabling/disabling breakpoints until the process is about to resume/detach/be destroyed, potentially reducing the packets transmitted by batching all breakpoints together. Most classes only need to know whether a breakpoint is "logically" enabled, as opposed to "physically" enabled (i.e. the remote server has actually enabled the breakpoint). However, lower level classes like derived Process classes, or StopInfo may actually need to know whether the breakpoint was physically enabled. As such, this commit also adds a "IsPhysicallyEnabled" API. https://github.com/llvm/llvm-project/pull/192910 >From c48d5679627e721abe711ef3f28f87d8ea6b20c0 Mon Sep 17 00:00:00 2001 From: Felipe de Azevedo Piovezan <[email protected]> Date: Thu, 9 Apr 2026 15:07:18 +0100 Subject: [PATCH] [lldb] Implement delayed breakpoints This patch changes the Process class so that it delays *physically* enabling/disabling breakpoints until the process is about to resume/detach/be destroyed, potentially reducing the packets transmitted by batching all breakpoints together. Most classes only need to know whether a breakpoint is "logically" enabled, as opposed to "physically" enabled (i.e. the remote server has actually enabled the breakpoint). However, lower level classes like derived Process classes, or StopInfo may actually need to know whether the breakpoint was physically enabled. As such, this commit also adds a "IsPhysicallyEnabled" API. https://github.com/llvm/llvm-project/pull/192910 --- lldb/include/lldb/Target/Process.h | 31 +++++- .../Process/MacOSX-Kernel/ProcessKDP.cpp | 4 +- .../Process/Utility/StopInfoMachException.cpp | 8 +- .../Process/gdb-remote/ProcessGDBRemote.cpp | 12 +-- lldb/source/Target/Process.cpp | 96 +++++++++++++++++-- lldb/source/Target/TargetProperties.td | 5 + .../Target/ThreadPlanStepOverBreakpoint.cpp | 6 +- 7 files changed, 140 insertions(+), 22 deletions(-) diff --git a/lldb/include/lldb/Target/Process.h b/lldb/include/lldb/Target/Process.h index f1807e26a3641..8171057f80ab1 100644 --- a/lldb/include/lldb/Target/Process.h +++ b/lldb/include/lldb/Target/Process.h @@ -113,6 +113,7 @@ class ProcessProperties : public Properties { bool GetSteppingRunsAllThreads() const; FollowForkMode GetFollowForkMode() const; bool TrackMemoryCacheChanges() const; + bool GetUseDelayedBreakpoints() const; protected: Process *m_process; // Can be nullptr for global ProcessProperties @@ -2243,6 +2244,9 @@ class Process : public std::enable_shared_from_this<Process>, // Process Breakpoints size_t GetSoftwareBreakpointTrapOpcode(BreakpointSite *bp_site); + enum class BreakpointAction { Enable, Disable }; + +protected: virtual Status EnableBreakpointSite(BreakpointSite *bp_site) { return Status::FromErrorStringWithFormatv( "error: {0} does not support enabling breakpoints", GetPluginName()); @@ -2253,6 +2257,14 @@ class Process : public std::enable_shared_from_this<Process>, "error: {0} does not support disabling breakpoints", GetPluginName()); } + virtual llvm::Error UpdateBreakpointSites( + const std::map<lldb::BreakpointSiteSP, BreakpointAction> &site_to_action); + +public: + Status ExecuteBreakpointSiteAction(BreakpointSite &site, + Process::BreakpointAction action, + bool force_now = false); + // This is implemented completely using the lldb::Process API. Subclasses // don't need to implement this function unless the standard flow of read // existing opcode, write breakpoint opcode, verify breakpoint opcode doesn't @@ -2277,7 +2289,8 @@ class Process : public std::enable_shared_from_this<Process>, lldb::break_id_t CreateBreakpointSite(const lldb::BreakpointLocationSP &owner, bool use_hardware); - Status DisableBreakpointSiteByID(lldb::user_id_t break_id); + Status DisableBreakpointSiteByID(lldb::user_id_t break_id, + bool force_now = false); Status EnableBreakpointSiteByID(lldb::user_id_t break_id); @@ -2290,6 +2303,8 @@ class Process : public std::enable_shared_from_this<Process>, lldb::user_id_t constituent_id, lldb::BreakpointSiteSP &bp_site_sp); + bool IsBreakpointSitePhysicallyEnabled(BreakpointSite &site); + // Process Watchpoints (optional) virtual Status EnableWatchpoint(lldb::WatchpointSP wp_sp, bool notify = true); @@ -3538,6 +3553,20 @@ void PruneThreadPlans(); /// GetExtendedCrashInformation. StructuredData::DictionarySP m_crash_info_dict_sp; + struct DelayedBreakpointCache { + void Enqueue(lldb::BreakpointSiteSP site, BreakpointAction action); + void RemoveSite(lldb::BreakpointSiteSP site) { + m_site_to_action.erase(site); + } + void Clear() { m_site_to_action.clear(); } + + std::map<lldb::BreakpointSiteSP, BreakpointAction> m_site_to_action; + }; + + DelayedBreakpointCache m_delayed_breakpoints; + + llvm::Error FlushDelayedBreakpoints(); + size_t RemoveBreakpointOpcodesFromBuffer(lldb::addr_t addr, size_t size, uint8_t *buf) const; diff --git a/lldb/source/Plugins/Process/MacOSX-Kernel/ProcessKDP.cpp b/lldb/source/Plugins/Process/MacOSX-Kernel/ProcessKDP.cpp index 6b3354aad09e6..6166096a4e1d3 100644 --- a/lldb/source/Plugins/Process/MacOSX-Kernel/ProcessKDP.cpp +++ b/lldb/source/Plugins/Process/MacOSX-Kernel/ProcessKDP.cpp @@ -633,7 +633,7 @@ Status ProcessKDP::EnableBreakpointSite(BreakpointSite *bp_site) { if (m_comm.LocalBreakpointsAreSupported()) { Status error; - if (!IsBreakpointSiteEnabled(bp_site)) { + if (!IsBreakpointSitePhysicallyEnabled(*bp_site)) { if (m_comm.SendRequestBreakpoint(true, bp_site->GetLoadAddress())) { SetBreakpointSiteEnabled(*bp_site); bp_site->SetType(BreakpointSite::eExternal); @@ -649,7 +649,7 @@ Status ProcessKDP::EnableBreakpointSite(BreakpointSite *bp_site) { Status ProcessKDP::DisableBreakpointSite(BreakpointSite *bp_site) { if (m_comm.LocalBreakpointsAreSupported()) { Status error; - if (IsBreakpointSiteEnabled(bp_site)) { + if (IsBreakpointSitePhysicallyEnabled(*bp_site)) { BreakpointSite::Type bp_type = bp_site->GetType(); if (bp_type == BreakpointSite::eExternal) { if (m_destroy_in_process && m_comm.IsRunning()) { diff --git a/lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp b/lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp index 9951fe1d9e293..5fbc1f62a065f 100644 --- a/lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp +++ b/lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp @@ -636,7 +636,7 @@ StopInfoSP StopInfoMachException::CreateStopReasonWithMachException( addr_t pc = reg_ctx_sp->GetPC(); BreakpointSiteSP bp_site_sp = process_sp->GetBreakpointSiteList().FindByAddress(pc); - if (bp_site_sp && process_sp->IsBreakpointSiteEnabled(bp_site_sp)) + if (bp_site_sp && process_sp->IsBreakpointSitePhysicallyEnabled(*bp_site_sp)) thread.SetThreadStoppedAtUnexecutedBP(pc); switch (exc_type) { @@ -771,7 +771,8 @@ StopInfoSP StopInfoMachException::CreateStopReasonWithMachException( if (!bp_site_sp && reg_ctx_sp) { bp_site_sp = process_sp->GetBreakpointSiteList().FindByAddress(pc); } - if (bp_site_sp && process_sp->IsBreakpointSiteEnabled(bp_site_sp)) { + if (bp_site_sp && + process_sp->IsBreakpointSitePhysicallyEnabled(*bp_site_sp)) { // 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(); @@ -865,7 +866,8 @@ bool StopInfoMachException::WasContinueInterrupted(Thread &thread) { // We have a hardware breakpoint -- this is the kernel bug. auto &bp_site_list = process_sp->GetBreakpointSiteList(); for (auto &site : bp_site_list.Sites()) { - if (site->IsHardware() && process_sp->IsBreakpointSiteEnabled(site)) { + if (site->IsHardware() && + process_sp->IsBreakpointSitePhysicallyEnabled(*site)) { LLDB_LOGF(log, "Thread stopped with insn-step completed mach exception but " "thread was not stepping; there is a hardware breakpoint set."); diff --git a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp index c3abb1a2e50cc..e4e0c00a8f648 100644 --- a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp +++ b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp @@ -1843,7 +1843,7 @@ ThreadSP ProcessGDBRemote::SetThreadStopInfo( addr_t pc = thread_sp->GetRegisterContext()->GetPC(); BreakpointSiteSP bp_site_sp = thread_sp->GetProcess()->GetBreakpointSiteList().FindByAddress(pc); - if (bp_site_sp && IsBreakpointSiteEnabled(bp_site_sp)) + if (bp_site_sp && IsBreakpointSitePhysicallyEnabled(*bp_site_sp)) thread_sp->SetThreadStoppedAtUnexecutedBP(pc); if (exc_type != 0) { @@ -2027,7 +2027,7 @@ ThreadSP ProcessGDBRemote::SetThreadStopInfo( // 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 (IsBreakpointSiteEnabled(bp_site_sp)) + if (IsBreakpointSitePhysicallyEnabled(*bp_site_sp)) thread_sp->SetThreadHitBreakpointSite(); if (bp_site_sp->ValidForThisThread(*thread_sp)) { @@ -3370,7 +3370,7 @@ Status ProcessGDBRemote::EnableBreakpointSite(BreakpointSite *bp_site) { site_id, (uint64_t)addr); // Breakpoint already exists and is enabled - if (IsBreakpointSiteEnabled(bp_site)) { + if (IsBreakpointSitePhysicallyEnabled(*bp_site)) { LLDB_LOGF(log, "ProcessGDBRemote::EnableBreakpointSite (size_id = %" PRIu64 ") address = 0x%" PRIx64 " -- SUCCESS (already enabled)", @@ -3391,7 +3391,7 @@ Status ProcessGDBRemote::DisableBreakpointSite(BreakpointSite *bp_site) { ") addr = 0x%8.8" PRIx64, site_id, (uint64_t)addr); - if (!IsBreakpointSiteEnabled(bp_site)) { + if (!IsBreakpointSitePhysicallyEnabled(*bp_site)) { LLDB_LOGF(log, "ProcessGDBRemote::DisableBreakpointSite (site_id = %" PRIu64 ") addr = 0x%8.8" PRIx64 " -- SUCCESS (already disabled)", @@ -6041,7 +6041,7 @@ CommandObject *ProcessGDBRemote::GetPluginCommandObject() { void ProcessGDBRemote::DidForkSwitchSoftwareBreakpoints(bool enable) { GetBreakpointSiteList().ForEach([this, enable](BreakpointSite *bp_site) { - if (IsBreakpointSiteEnabled(bp_site) && + if (IsBreakpointSitePhysicallyEnabled(*bp_site) && (bp_site->GetType() == BreakpointSite::eSoftware || bp_site->GetType() == BreakpointSite::eExternal)) { m_gdb_comm.SendGDBStoppointTypePacket( @@ -6054,7 +6054,7 @@ void ProcessGDBRemote::DidForkSwitchSoftwareBreakpoints(bool enable) { void ProcessGDBRemote::DidForkSwitchHardwareTraps(bool enable) { if (m_gdb_comm.SupportsGDBStoppointPacket(eBreakpointHardware)) { GetBreakpointSiteList().ForEach([this, enable](BreakpointSite *bp_site) { - if (IsBreakpointSiteEnabled(bp_site) && + if (IsBreakpointSitePhysicallyEnabled(*bp_site) && bp_site->GetType() == BreakpointSite::eHardware) { m_gdb_comm.SendGDBStoppointTypePacket( eBreakpointHardware, enable, bp_site->GetLoadAddress(), diff --git a/lldb/source/Target/Process.cpp b/lldb/source/Target/Process.cpp index 3a648029bc1a4..0133f1024d679 100644 --- a/lldb/source/Target/Process.cpp +++ b/lldb/source/Target/Process.cpp @@ -79,6 +79,17 @@ using namespace lldb; using namespace lldb_private; using namespace std::chrono; +void Process::DelayedBreakpointCache::Enqueue(lldb::BreakpointSiteSP site, + BreakpointAction action) { + auto [previous, inserted] = m_site_to_action.insert({site, action}); + // New site or already enqueued for the same action + if (inserted || previous->second == action) + return; + // Previously enqueued for the opposite action, don't update the site. + m_site_to_action.erase(previous); + assert(site->m_enabled == (action == BreakpointAction::Enable)); +} + class ProcessOptionValueProperties : public Cloneable<ProcessOptionValueProperties, OptionValueProperties> { public: @@ -322,6 +333,12 @@ bool ProcessProperties::GetStopOnExec() const { idx, g_process_properties[idx].default_uint_value != 0); } +bool ProcessProperties::GetUseDelayedBreakpoints() const { + const uint32_t idx = ePropertyUseDelayedBreakpoints; + return GetPropertyAtIndexAs<bool>( + idx, g_process_properties[idx].default_uint_value != 0); +} + std::chrono::seconds ProcessProperties::GetUtilityExpressionTimeout() const { const uint32_t idx = ePropertyUtilityExpressionTimeout; uint64_t value = GetPropertyAtIndexAs<uint64_t>( @@ -1539,12 +1556,12 @@ Process::GetBreakpointSiteList() const { void Process::DisableAllBreakpointSites() { m_breakpoint_site_list.ForEach([this](BreakpointSite *bp_site) -> void { - DisableBreakpointSite(bp_site); + ExecuteBreakpointSiteAction(*bp_site, BreakpointAction::Disable); }); } Status Process::ClearBreakpointSiteByID(lldb::user_id_t break_id) { - Status error(DisableBreakpointSiteByID(break_id)); + Status error(DisableBreakpointSiteByID(break_id, /*force_now=*/true)); if (error.Success()) m_breakpoint_site_list.Remove(break_id); @@ -1552,12 +1569,14 @@ Status Process::ClearBreakpointSiteByID(lldb::user_id_t break_id) { return error; } -Status Process::DisableBreakpointSiteByID(lldb::user_id_t break_id) { +Status Process::DisableBreakpointSiteByID(lldb::user_id_t break_id, + bool force_now) { Status error; BreakpointSiteSP bp_site_sp = m_breakpoint_site_list.FindByID(break_id); if (bp_site_sp) { if (IsBreakpointSiteEnabled(bp_site_sp)) - error = DisableBreakpointSite(bp_site_sp.get()); + error = ExecuteBreakpointSiteAction(*bp_site_sp, + BreakpointAction::Disable, force_now); } else { error = Status::FromErrorStringWithFormat( "invalid breakpoint site ID: %" PRIu64, break_id); @@ -1566,12 +1585,34 @@ Status Process::DisableBreakpointSiteByID(lldb::user_id_t break_id) { return error; } +Status Process::ExecuteBreakpointSiteAction(BreakpointSite &site, + BreakpointAction action, + bool force_now) { + if (!force_now && GetUseDelayedBreakpoints()) { + m_delayed_breakpoints.Enqueue(site.shared_from_this(), action); + return Status(); + } + + auto site_sp = site.shared_from_this(); + m_delayed_breakpoints.RemoveSite(site_sp); + + switch (action) { + case BreakpointAction::Enable: + return EnableBreakpointSite(site_sp.get()); + case BreakpointAction::Disable: + return DisableBreakpointSite(site_sp.get()); + } + + llvm_unreachable("Unhandled BreakpointAction"); +} + Status Process::EnableBreakpointSiteByID(lldb::user_id_t break_id) { Status error; BreakpointSiteSP bp_site_sp = m_breakpoint_site_list.FindByID(break_id); if (bp_site_sp) { if (!IsBreakpointSiteEnabled(bp_site_sp)) - error = EnableBreakpointSite(bp_site_sp.get()); + error = + ExecuteBreakpointSiteAction(*bp_site_sp, BreakpointAction::Enable); } else { error = Status::FromErrorStringWithFormat( "invalid breakpoint site ID: %" PRIu64, break_id); @@ -1581,12 +1622,21 @@ Status Process::EnableBreakpointSiteByID(lldb::user_id_t break_id) { bool Process::IsBreakpointSiteEnabled(const lldb::BreakpointSiteSP &site) { assert(site); - return site->m_enabled; + auto it = m_delayed_breakpoints.m_site_to_action.find(site); + // If no actions are delayed, use the current state of the site. + if (it == m_delayed_breakpoints.m_site_to_action.end()) + return site->m_enabled; + + return it->second == BreakpointAction::Enable; } bool Process::IsBreakpointSiteEnabled(BreakpointSite *site) { assert(site); - return site->m_enabled; + return IsBreakpointSiteEnabled(site->shared_from_this()); +} + +bool Process::IsBreakpointSitePhysicallyEnabled(BreakpointSite &site) { + return site.m_enabled; } static bool ShouldShowError(Process &process) { @@ -1647,6 +1697,24 @@ static addr_t ComputeConstituentLoadAddress(BreakpointLocation &constituent, return resolved_address.GetOpcodeLoadAddress(&target); } +llvm::Error Process::FlushDelayedBreakpoints() { + auto error = UpdateBreakpointSites(m_delayed_breakpoints.m_site_to_action); + m_delayed_breakpoints.Clear(); + return error; +} + +llvm::Error Process::UpdateBreakpointSites( + const std::map<lldb::BreakpointSiteSP, BreakpointAction> &site_to_action) { + llvm::Error error = llvm::Error::success(); + for (auto [site, action] : site_to_action) { + Status new_error = action == BreakpointAction::Enable + ? EnableBreakpointSite(site.get()) + : DisableBreakpointSite(site.get()); + error = llvm::joinErrors(std::move(error), new_error.takeError()); + } + return error; +} + lldb::break_id_t Process::CreateBreakpointSite(const BreakpointLocationSP &constituent, bool use_hardware) { @@ -1691,7 +1759,8 @@ void Process::RemoveConstituentFromBreakpointSite( if (num_constituents == 0) { // Don't try to disable the site if we don't have a live process anymore. if (IsAlive()) - DisableBreakpointSite(bp_site_sp.get()); + ExecuteBreakpointSiteAction(*bp_site_sp, BreakpointAction::Disable, + /*force_now=*/true); m_breakpoint_site_list.RemoveByAddress(bp_site_sp->GetLoadAddress()); } } @@ -3434,6 +3503,9 @@ Status Process::PrivateResume() { "Process::PrivateResume PreResumeActions failed, not resuming."); } else { m_mod_id.BumpResumeID(); + if (auto E = FlushDelayedBreakpoints()) + LLDB_LOG_ERROR(log, std::move(E), + "Failed to update some delayed breakpoints: {0}"); error = DoResume(direction); if (error.Success()) { DidResume(); @@ -3640,6 +3712,10 @@ Status Process::Detach(bool keep_stopped) { m_thread_list.DiscardThreadPlans(); DisableAllBreakpointSites(); + if (auto error = FlushDelayedBreakpoints()) + LLDB_LOG_ERROR( + GetLog(LLDBLog::Process), std::move(error), + "Failed to update some delayed breakpoints during detach: {0}"); error = DoDetach(keep_stopped); if (error.Success()) { @@ -3709,6 +3785,10 @@ Status Process::DestroyImpl(bool force_kill) { // doing this now. m_thread_list.DiscardThreadPlans(); DisableAllBreakpointSites(); + if (auto error = FlushDelayedBreakpoints()) + LLDB_LOG_ERROR( + GetLog(LLDBLog::Process), std::move(error), + "Failed to update some delayed breakpoints during destroy: {0}"); } error = DoDestroy(); diff --git a/lldb/source/Target/TargetProperties.td b/lldb/source/Target/TargetProperties.td index e36d233fc2469..73eb4da402b71 100644 --- a/lldb/source/Target/TargetProperties.td +++ b/lldb/source/Target/TargetProperties.td @@ -310,6 +310,11 @@ let Definition = "process", Path = "target.process" in { def TrackMemoryCacheChanges: Property<"track-memory-cache-changes", "Boolean">, DefaultTrue, Desc<"If true, memory cache modifications (which happen often during expressions evaluation) will bump process state ID (and invalidate all synthetic children). Disabling this option helps to avoid synthetic children reevaluation when pretty printers heavily use expressions. The downside of disabled setting is that convenience variables won't reevaluate synthetic children automatically.">; + def UseDelayedBreakpoints + : Property<"use-delayed-breakpoints", "Boolean">, + DefaultTrue, + Desc<"Specify whether to delay setting breakpoints until the process " + "is about to resume.">; } let Definition = "platform", Path = "platform" in { diff --git a/lldb/source/Target/ThreadPlanStepOverBreakpoint.cpp b/lldb/source/Target/ThreadPlanStepOverBreakpoint.cpp index 597577c208f8b..b215b4be76602 100644 --- a/lldb/source/Target/ThreadPlanStepOverBreakpoint.cpp +++ b/lldb/source/Target/ThreadPlanStepOverBreakpoint.cpp @@ -121,7 +121,8 @@ bool ThreadPlanStepOverBreakpoint::DoWillResume(StateType resume_state, BreakpointSiteSP bp_site_sp( m_process.GetBreakpointSiteList().FindByAddress(m_breakpoint_addr)); if (bp_site_sp && m_process.IsBreakpointSiteEnabled(bp_site_sp)) { - m_process.DisableBreakpointSite(bp_site_sp.get()); + m_process.ExecuteBreakpointSiteAction(*bp_site_sp, + Process::BreakpointAction::Disable); m_reenabled_breakpoint_site = false; } } @@ -167,7 +168,8 @@ void ThreadPlanStepOverBreakpoint::ReenableBreakpointSite() { if (BreakpointSiteSP bp_site_sp = m_process.GetBreakpointSiteList().FindByAddress( m_breakpoint_addr)) - m_process.EnableBreakpointSite(bp_site_sp.get()); + m_process.ExecuteBreakpointSiteAction( + *bp_site_sp, Process::BreakpointAction::Enable); } } } _______________________________________________ llvm-branch-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
