[lldb] Implement delayed breakpoints#192971
Conversation
🐧 Linux x64 Test Results
✅ The build succeeded and all tests passed. |
felipepiovezan
force-pushed
the
users/felipepiovezan/delayed_bps_p3
branch
from
c48d567 to
474bad5
Compare
felipepiovezan
force-pushed
the
users/felipepiovezan/delayed_bps_p2
branch
from
8f4104e to
2070181
Compare
felipepiovezan
force-pushed
the
users/felipepiovezan/delayed_bps_p3
branch
from
474bad5 to
9a7fcf5
Compare
felipepiovezan
force-pushed
the
users/felipepiovezan/delayed_bps_p2
branch
from
2070181 to
e180c10
Compare
felipepiovezan
force-pushed
the
users/felipepiovezan/delayed_bps_p3
branch
from
9a7fcf5 to
35a2784
Compare
This describes the packet discussed in the RFC: https://discourse.llvm.org/t/rfc-a-new-packet-to-set-remove-multiple-breakpoints/90623 The following PRs are related: * [[lldb] Propose MultiBreakpoint extension to GDB Remote](#192910) * [[debugserver] Implement MultiBreakpoint](#192914) * [[lldb-server][NFC] Factor out code handling breakpoint packets](#192915) * [[lldb-server] Implement support for MultiBreakpoint packet](#192919) * [[lldb][GDBRemote] Parse MultiBreakpoint+ capability](#192962) * [[lldb][NFC] Move BreakpointSite::IsEnabled/SetEnabled into Process](#192964) * [[lldb] Implement delayed breakpoints](#192971) * [[lldb] Override UpdateBreakpointSites in ProcessGDBRemote to use MultiBreakpoint](#192988)
…92910) This describes the packet discussed in the RFC: https://discourse.llvm.org/t/rfc-a-new-packet-to-set-remove-multiple-breakpoints/90623 The following PRs are related: * [[lldb] Propose MultiBreakpoint extension to GDB Remote](llvm/llvm-project#192910) * [[debugserver] Implement MultiBreakpoint](llvm/llvm-project#192914) * [[lldb-server][NFC] Factor out code handling breakpoint packets](llvm/llvm-project#192915) * [[lldb-server] Implement support for MultiBreakpoint packet](llvm/llvm-project#192919) * [[lldb][GDBRemote] Parse MultiBreakpoint+ capability](llvm/llvm-project#192962) * [[lldb][NFC] Move BreakpointSite::IsEnabled/SetEnabled into Process](llvm/llvm-project#192964) * [[lldb] Implement delayed breakpoints](llvm/llvm-project#192971) * [[lldb] Override UpdateBreakpointSites in ProcessGDBRemote to use MultiBreakpoint](llvm/llvm-project#192988)
…92910) This describes the packet discussed in the RFC: https://discourse.llvm.org/t/rfc-a-new-packet-to-set-remove-multiple-breakpoints/90623 The following PRs are related: * [[lldb] Propose MultiBreakpoint extension to GDB Remote](llvm/llvm-project#192910) * [[debugserver] Implement MultiBreakpoint](llvm/llvm-project#192914) * [[lldb-server][NFC] Factor out code handling breakpoint packets](llvm/llvm-project#192915) * [[lldb-server] Implement support for MultiBreakpoint packet](llvm/llvm-project#192919) * [[lldb][GDBRemote] Parse MultiBreakpoint+ capability](llvm/llvm-project#192962) * [[lldb][NFC] Move BreakpointSite::IsEnabled/SetEnabled into Process](llvm/llvm-project#192964) * [[lldb] Implement delayed breakpoints](llvm/llvm-project#192971) * [[lldb] Override UpdateBreakpointSites in ProcessGDBRemote to use MultiBreakpoint](llvm/llvm-project#192988)
…92910) This describes the packet discussed in the RFC: https://discourse.llvm.org/t/rfc-a-new-packet-to-set-remove-multiple-breakpoints/90623 The following PRs are related: * [[lldb] Propose MultiBreakpoint extension to GDB Remote](llvm/llvm-project#192910) * [[debugserver] Implement MultiBreakpoint](llvm/llvm-project#192914) * [[lldb-server][NFC] Factor out code handling breakpoint packets](llvm/llvm-project#192915) * [[lldb-server] Implement support for MultiBreakpoint packet](llvm/llvm-project#192919) * [[lldb][GDBRemote] Parse MultiBreakpoint+ capability](llvm/llvm-project#192962) * [[lldb][NFC] Move BreakpointSite::IsEnabled/SetEnabled into Process](llvm/llvm-project#192964) * [[lldb] Implement delayed breakpoints](llvm/llvm-project#192971) * [[lldb] Override UpdateBreakpointSites in ProcessGDBRemote to use MultiBreakpoint](llvm/llvm-project#192988)
felipepiovezan
force-pushed
the
users/felipepiovezan/delayed_bps_p2
branch
from
e180c10 to
3794843
Compare
felipepiovezan
force-pushed
the
users/felipepiovezan/delayed_bps_p3
branch
from
35a2784 to
84d12c8
Compare
|
@llvm/pr-subscribers-lldb Author: Felipe de Azevedo Piovezan (felipepiovezan) ChangesThis 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. The following PRs are related to the MultiBreakpoint feature:
Patch is 20.73 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/192971.diff 9 Files Affected:
diff --git a/lldb/include/lldb/Target/Process.h b/lldb/include/lldb/Target/Process.h
index f90c11813ba48..88e9ac4bc802f 100644
--- a/lldb/include/lldb/Target/Process.h
+++ b/lldb/include/lldb/Target/Process.h
@@ -114,6 +114,7 @@ class ProcessProperties : public Properties {
Args GetAlwaysRunThreadNames() const;
FollowForkMode GetFollowForkMode() const;
bool TrackMemoryCacheChanges() const;
+ bool GetUseDelayedBreakpoints() const;
protected:
Process *m_process; // Can be nullptr for global ProcessProperties
@@ -2246,6 +2247,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());
@@ -2256,6 +2260,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
@@ -2280,7 +2292,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);
@@ -2293,6 +2306,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);
@@ -3541,6 +3556,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 145d69d6b6d64..33d3713c948c4 100644
--- a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -1848,7 +1848,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) {
@@ -2032,7 +2032,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)) {
@@ -3429,7 +3429,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)",
@@ -3450,7 +3450,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)",
@@ -6112,7 +6112,7 @@ void ProcessGDBRemote::DidForkSwitchSoftwareBreakpoints(
GetBreakpointSiteList().ForEach([this, enable, entry_addr,
log](BreakpointSite *bp_site) {
- if (IsBreakpointSiteEnabled(bp_site) &&
+ if (IsBreakpointSitePhysicallyEnabled(*bp_site) &&
(bp_site->GetType() == BreakpointSite::eSoftware ||
bp_site->GetType() == BreakpointSite::eExternal)) {
// During expression evaluation, retain the expression-return trap
@@ -6136,7 +6136,7 @@ void ProcessGDBRemote::DidForkSwitchSoftwareBreakpoints(
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/Plugins/Process/scripted/ScriptedProcess.cpp b/lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
index b0718b771295b..c254a6841b707 100644
--- a/lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
+++ b/lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
@@ -265,7 +265,7 @@ size_t ScriptedProcess::DoWriteMemory(lldb::addr_t vm_addr, const void *buf,
Status ScriptedProcess::EnableBreakpointSite(BreakpointSite *bp_site) {
assert(bp_site != nullptr);
- if (IsBreakpointSiteEnabled(bp_site)) {
+ if (IsBreakpointSitePhysicallyEnabled(*bp_site)) {
return {};
}
diff --git a/lldb/source/Plugins/Process/scripted/ScriptedThread.cpp b/lldb/source/Plugins/Process/scripted/ScriptedThread.cpp
index aacfcc29e5f97..d11d134295579 100644
--- a/lldb/source/Plugins/Process/scripted/ScriptedThread.cpp
+++ b/lldb/source/Plugins/Process/scripted/ScriptedThread.cpp
@@ -316,7 +316,7 @@ bool ScriptedThread::CalculateStopInfo() {
ProcessSP proc = GetProcess();
if (BreakpointSiteSP bp_site_sp =
proc->GetBreakpointSiteList().FindByAddress(pc))
- if (proc->IsBreakpointSiteEnabled(bp_site_sp))
+ if (proc->IsBreakpointSitePhysicallyEnabled(*bp_site_sp))
SetThreadStoppedAtUnexecutedBP(pc);
}
diff --git a/lldb/source/Target/Process.cpp b/lldb/source/Target/Process.cpp
index f9b6c4a79dd09..1a2323f5de136 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>(
@@ -1546,12 +1563,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);
@@ -1559,12 +1576,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);
@@ -1573,12 +1592,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);
@@ -1588,12 +1629,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) {
@@ -1654,6 +1704,31 @@ static addr_t ComputeConstituentLoadAddress(BreakpointLocation &constituent,
return resolved_address.GetOpcodeLoadAddress(&target);
}
+llvm::Error Process::FlushDelayedBreakpoints() {
+ // Clear the cache in m_delayed_breakpoints so it can't affect the actual
+ // enabling of breakpoints. For
+ // example, if `EnableSoftwareBreakpoint` is called outside of
+ // FlushDelayedBreakpoints, it needs to check the delayed breakpoints and
+ // possibly early return. However, when called from FlushDelayedBreakpoints,
+ // the queue better be empty so that no early returns take place.
+ auto site_to_action = std::move(m_delayed_breakpoints.m_site_to_action);
+
+ auto error = UpdateBreakpointSites(site_to_action);
+ 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) {
@@ -1698,7 +1773,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());
}
}
@@ -3441,6 +3517,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();
@@ -3647,6 +3726,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()) {
@@ -3716,6 +3799,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 223a12e059258..0a46125594ab6 100644
--- a/lldb/source/Target/TargetProperties.td
+++ b/lldb/source/Target/TargetProperties.td
@@ -319,6 +319,11 @@ let Definition = "process", Path = "target.process" in {
Desc<"A list of thread names. Threads with any of these names will "
"always be resumed when the process resumes, even when other "
"threads are suspended during single-stepping operations.">;
+ 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_a...
[truncated]
|
DavidSpickett
left a comment
DavidSpickett
left a comment
There was a problem hiding this comment.
We should have a release note about this but it's fine to wait until you've landed your stack and you know what to include.
| } | ||
| void Clear() { m_site_to_action.clear(); } | ||
|
|
||
| std::map<lldb::BreakpointSiteSP, BreakpointAction> m_site_to_action; |
There was a problem hiding this comment.
Do we have ordering requirements for this?
There was a problem hiding this comment.
Actually, we do. Iteration order must not depend on pointer values.
I wanted to use a SetVector here, but this data structure requires some boilerplate to work with shared pointers, and it also is not great when deletions are frequent (which we expect to be the case with breakpoints). I will add a custom comparison functor.
| // FlushDelayedBreakpoints, it needs to check the delayed breakpoints and | ||
| // possibly early return. However, when called from FlushDelayedBreakpoints, | ||
| // the queue better be empty so that no early returns take place. | ||
| auto site_to_action = std::move(m_delayed_breakpoints.m_site_to_action); |
There was a problem hiding this comment.
What exactly is the purpose of move here?
I am wary of a use after move with m_site_to_action later. std::map might be ok with this but I don't know if it has to be the case.
There was a problem hiding this comment.
Objects must be left in an usable state after a move, but I agree this is a bit too subtle. The purpose of the move is to clean the container / safely allow for iteration (with the rationale above). I can throw in an extra .clear() there.
| : Property<"use-delayed-breakpoints", "Boolean">, | ||
| DefaultTrue, | ||
| Desc<"Specify whether to delay setting breakpoints until the process " | ||
| "is about to resume.">; |
There was a problem hiding this comment.
Do we have any exceptions to this at this time?
We talked about hardware breaks for example. I'm not sure whether we need that exception actually but if you've implemented it already it should be noted here.
And on that topic, users could turn off this setting when hardware resources are a concern couldn't they.
There was a problem hiding this comment.
Also this says "setting" which sounds like one half of the story. "setting or removing", "updating", something like that. Updating sounds a bit like it only works for ones that already exist. So some "X an Y-ing" is probably better.
There was a problem hiding this comment.
Do we have any exceptions to this at this time?
We do, like when the BreakpointSite is created for the first time. But these are very low level explanations that I really would want to avoid providing in the help string.... I'll reword this to say "may be delayed" instead of "is delayed".
And on that topic, users could turn off this setting when hardware resources are a concern couldn't they.
yes!
I'm not sure whether we need that exception actually but if you've implemented it already it should be noted here.
I haven't implemented that kind of exception yet. If we feel like we should do it together with this patch, I can have a look now. One idea @jimingham had was to check if we have already reached the number of hardware breakpoints the hardware allows, as we should, in theory, be able to calculate that.
There was a problem hiding this comment.
Also this says "setting" which sounds like one half of the story.
true, will update to "updating".
This implements the packet as described in llvm#192910 The following PRs are related to the MultiBreakpoint feature: * llvm#192910 * llvm#192914 * llvm#192915 * llvm#192919 * llvm#192962 * llvm#192964 * llvm#192971 * llvm#192988
This commit extracts the code handling breakpoint packets into a helper function that can be used by a future implementation of the MultiBreakpointPacket. It is meant to be purely NFC. There are two functions handling breakpoint packets (`handle_Z` and `handle_z`) with a lot of repeated code. This commit did not attempt to merge the two, as that would make the diff much larger due to subtle differences in the error message produced by the two. The only deduplication done is in the code processing a GDBStoppointType, where a helper struct (`BreakpointKind`) and function (`std::optional<BreakpointKind> getBreakpointKind(GDBStoppointType stoppoint_type)`) was created. The following PRs are related to the MultiBreakpoint feature: * #192910 * #192914 * #192915 * #192919 * #192962 * #192964 * #192971 * #192988
This implements the packet as described in llvm/llvm-project#192910 The following PRs are related to the MultiBreakpoint feature: * llvm/llvm-project#192910 * llvm/llvm-project#192914 * llvm/llvm-project#192915 * llvm/llvm-project#192919 * llvm/llvm-project#192962 * llvm/llvm-project#192964 * llvm/llvm-project#192971 * llvm/llvm-project#192988
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. The following PRs are related to the MultiBreakpoint feature: * llvm/llvm-project#192910 * llvm/llvm-project#192914 * llvm/llvm-project#192915 * llvm/llvm-project#192919 * llvm/llvm-project#192962 * llvm/llvm-project#192964 * llvm/llvm-project#192971 * llvm/llvm-project#192988
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. The following PRs are related to the MultiBreakpoint feature: * llvm/llvm-project#192910 * llvm/llvm-project#192914 * llvm/llvm-project#192915 * llvm/llvm-project#192919 * llvm/llvm-project#192962 * llvm/llvm-project#192964 * llvm/llvm-project#192971 * llvm/llvm-project#192988
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. The following PRs are related to the MultiBreakpoint feature: * llvm/llvm-project#192910 * llvm/llvm-project#192914 * llvm/llvm-project#192915 * llvm/llvm-project#192919 * llvm/llvm-project#192962 * llvm/llvm-project#192964 * llvm/llvm-project#192971 * llvm/llvm-project#192988
|
https://lab.llvm.org/buildbot/#/builders/211/builds/7890 and shell test Please take a look. |
|
Also look at this issue #191222. |
Yeah... I requested a rerun just in case. If it reproduces, I will probably disable delayed breakpoints on windows, since this seems to be a known problem there |
|
I think there may be a problem with this patch (and that the windows fix maybe just papers over the issue). Let me illustrate what I mean this with the following packet log. I'm sorry for the length -- I tried to remove the really boring parts, but I didn't want to leave out too much context. The most interesting lines are marked with This is a log of me evaluating two expressions. The problematic part is that during the evaluation of the first expression, we set the entry breakpoint (which is supposed to be hit when the expression gets evaluation) -- but this breakpoint doesn't get cleared. Then, when we evaluate the second expression, we set the breakpoint again -- and then we also clear it (presumably intending to clear the breakpoint from the first run). Now, on lldb-server and debugserver this does not actually matter -- because they implement the set/clear breakpoint packets using a refcount. This means the breakpoint remains set when evaluating the expression -- as expected. However, it's not clear to me that this is actually the expected way to implement these packets. The gdb protocol spec doesn't say much on the matter, but if we consider gdbserver to be a "reference implementation" of the spec, then this would point to it not being the case: Now, the reason I noticed this is because we have an (internal) stub, which implements these packets the "gdb way", which means that the entry breakpoint is not set when evaluating the second expression, and the expression ends up starting the binary all over again (which, predictably, blows up). I'm not sure if this is a case for the windows implementation. Being in-process it doesn't use the same code as lldb-server, but I'm not completely sure in how it interacts with the rest of lldb. However, the failure mode is at least consistent with the behavior I've seen using our stub. I've also looked at how qemu (a debug stub built for gdb that I have access to) implements this and -- to my surprise -- it does implement the reference counting behavior. I haven't looked at the source code, so I don't know if this is intentional or not (I can imagine this kind of behavior falling out "naturally" in some implementations). Given all of this, I think we need to make a decision: do expect stub to support reference counted breakpoints or not? The list of stubs which are currently known to not support this is not particularly compelling:
However, I am worried about all the random debug stub implementations out there, potentially embedded in jtag probes or whatnot. I don't have access to these to verify, but I understand that people are using these with lldb, and these sound like the kind of things that would be hard to modify. This is why my vote, whatever its worth, would be to fix this on the lldb side, and ideally revert this patch in the mean time. If it helps, I volunteer to rip out the reference counting code from within lldb-server to make this easier to test (and make sure it doesn't happen again). |
I've created #195652 for that, which we can submit if we come to some sort of a consensus. Another issue I noticed when playing around with this is that the packet log for expression evaluation contains a lot of disable-enable breakpoint packet pairs: These correspond to internal breakpoints (which I guess are somehow re-enabled for every expression): This isn't necessarily a (correctness?) issue, but it strikes me as rather odd as the goal of this patch series is to reduce the number of breakpoint packets. And the patch contains code which seems to try to prevent these, so I'm wondering if this is actually working as intended. |
|
@labath Felipe still needs to merge (this one final?) PR #192988 to have ProcessGDBRemote start sending MultiBreakpoint's; we are still doing the breakpoint enabling and disabling with separate packets right now. The behavior should be the same as the final goal, though: all breakpoint enabling/disabling is queued, until we go to resume the target, and then lldb sends all of them all together. |
|
Playing around with llvm.org main a little while watching packets, one thing I'm a little surprised by is that some breakpoint-set's are sent eagerly, some are delayed. At first, I saw hardware watchpoints were eager, which makes sense because there's a real possibility of them failing and maybe the developer will decide to use a software breakpoint (debugserver silently uses a software breakpoint, which is probably a bit unhelpful if you really needed hardware watchpoints) But also Haven't looked at the sources closely to understand the differences, but I'm not sure I understand how we decide what is sent immediately vrs. enqueued yet. a minor detail, just noting it. |
We get a breakpoint on LC_MAIN (0x100000328) that the expression returns to, and a bunch of internal breakpoints are enabled If I do the expression again, I'll see all of these disabled and re-enabled I think what's happening is that the expression return breakpoint is set immediately, not enqueued. Notice how we send the
If breakpoints are refcounted by the stub, this will work. But if it doesn't, we've deleted the expression-return breakpoint. sorry this is probably exactly the same thing Pavel said, I was just figuring it out slowly for myself. |
|
If the expression-return breakpoint setting was enqueued instead of immediate (could this be the same reason that |
|
Just to try it out, I hacked debugserver to not refcount its breakpoints and the second expression evaluation in a row will fail. |
|
Yeah I re-read through Pavel's packet log, he's facing the same issue as the one I'm talking about. |
lol I'm guessing the reason address breakpoints (which includes the expression-return breakpoint) are eager is because a user might put a breakpoint in unmapped memory by accident, and we'd like to show them an error message right away so they can correct the address. The removal of these cannot fail, so they're done non-eagerly. I don't know if we should have some way of distinguishing between "a user specified address breakpoint" and something lldb has internally calculated so it can be delayed. Or if we want to check the list of enqueued breakpoint actions and see if any have the same address and making it non-eager, assuming that it is a valid address because we're already doing breakpoint things on it? |
|
Jason's right about why we decided to make address breakpoints eager. I also want this because ThreadPlans tend to set address breakpoints in their ShouldStop, and if the breakpoint didn't work they would want to know in ShouldStop so they could make other plans. For the most part, if a user wants lldb to calculate an address that you intend to use in a breakpoint you get the resolver to do it for you, and that wouldn't be a AddressResolver. Those are really only used when you have computed an Address by hand and want to break on that Address. |
|
I think the tl;dr on this is that mixing eager breakpoints and delayed breakpoints can lead to chronological mistakes and bugs like these. I wonder if our solutions might look, generally, like
Right now when we have an eager breakpoint, we are sending two breakpoint requests: The eager-breakpoint right now, and then when we start execution, any delayed breakpoints in a multi-breakpoint packet. If we do (1), we're still sending two breakpoint requests, it's not any actual performance loss. I guess someone does an eager breakpoint (runs an expression) which flushes the delayed breakpoints - 2 packets. Then they do a Maybe that's the right approach (1), because it's the simplest and doesn't try to be fancier. We simply don't allow delayed breakpoints to exist when doing an eager breakpoint. And if we find we are losing performance in common cases by this approach, we might need to revisit it? |
|
Thanks for checking this out, Jason. (1) makes most sense to me as well. I'll just add that, if you want to reduce roundtrips, in principle it should possible to include the eager breakpoint request into the multi-packet flushing the delayed requests. The response includes individual error messages for each sub-request, so it should be possible to see whether the eager request failed or not.
I understand that multi-packet is not enabled yet, but my point is that you don't need the multi packet to optimize this sort of sequence. The code can already see it is going to disable and reenable a breakpoint at the same address, so it could just cancel those two out locally. In fact, the code already tries to do that ( This isn't a regression, since those packets would get sent even before this series, but I think you could call it a missed opportunity. Seeing the two packets back-to-back is sort of ridiculous and I can't help but wonder how much speedup you could get by eliminating these -- even without the new packet. Even if adding the packet is still worthwhile, this sort of thing could help with older stubs, which I believe is something that you care about. I definitely haven't thought through all the implications, but an obvious idea is to change to logic to cluster breakpoint requests based on breakpoint address (as that is what ultimately gets sent), rather than the BreakpointSite pointer. |
Oh, of course, good point. I think we might need a short-term fix for this right away, and sending the extra packet right now to keep it simple might be good, but that's a better idea. Let's see what Felipe thinks. |
|
Thanks for investigating this, everyone. Let me try to address some of the points mentioned here. First, the efficiency point, which is not related to the main bug discussed.
This one is intentional because of a detail about how the BreakpointSite class is designed. If we disable a BreakpointSite which had exactly one BreakpointLocation associated with it, the BreakpointSite is removed from the site list of the Process. This has always been the case. Now, if we re-enable a breakpoint on the same address, a new site will be created, because the code will look for a BreakpointSite in the site list with a matching address and find none. This has also always been the case. The interaction with this patch is that now there will be two breakpoint operations queued for the same address (the disable, and then the enable). This is not problematic, but:
You may point out that we could, in theory, search both the Process site list and the delayed breakpoint queue for a BreakpointSite with a matching address. If there was a BreakpointSite there already enqueued for disabling, simply move it back to the Process site list. This works, except for this case:
In this scenario, it would simply be incorrect to re-use the site and do nothing. A new site must be created. The first one must be disabled first. Also worth pointing out that I believe LLDB, before this patch, doesn't handle well the case there you have multiple (non-hardware) breakpoints on the same address, and you try to create a new hardware breakpoint on the same address. |
Yeah, to me this makes the most sense as well. The whole eager business was a late addition, so it slipped through the cracks, but it is clearly wrong to not flush the cache while doing the eager breakpoint. |
|
I'll create a PR now. |
That makes sense, but I don't think it prevents us from coalescing breakpoint requests of the same kind (I didn't mention that, but when I said that breakpoints could be clustered "based on breakpoint address", I meant that this should only apply within a breakpoint class. So like, if there's a |
8 participants
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.
The following PRs are related to the MultiBreakpoint feature: