https://github.com/labath created https://github.com/llvm/llvm-project/pull/195858
We did not say so explictly, but I'd argue that via #195815, we are supporting stubs which do not refcount breakpoints. In these stubs the set/clear breakpoint packets are idempotent: - setting a breakpoint for the second time is a no-op (returns OK) - clearing a breakpoint clears it, regardless of how many times it has been set - clearing a non-existent breakpoint (either because it was already cleared, or because it was never set) returns an error This makes lldb-server one of those stubs, which makes the code slightly simpler, but more importantly, ensures we do not regress this behavior. >From 0bc7469136334aa92849aa83013b938edb936b3f Mon Sep 17 00:00:00 2001 From: Pavel Labath <[email protected]> Date: Tue, 5 May 2026 15:48:08 +0200 Subject: [PATCH] [lldb] Do not refcount breakpoints in lldb-server We did not say so explictly, but I'd argue that via #195815, we are supporting stubs which do not refcount breakpoints. In these stubs the set/clear breakpoint packets are idempotent: - setting a breakpoint for the second time is a no-op (returns OK) - clearing a breakpoint clears it, regardless of how many times it has been set - clearing a non-existent breakpoint (either because it was already cleared, or because it was never set) returns an error This makes lldb-server one of those stubs, which makes the code slightly simpler, but more importantly, ensures we do not regress this behavior. --- .../lldb/Host/common/NativeProcessProtocol.h | 1 - lldb/source/Host/common/NativeProcessProtocol.cpp | 14 ++++---------- .../multi-breakpoint/TestMultiBreakpoint.py | 10 ++++++++-- 3 files changed, 12 insertions(+), 13 deletions(-) diff --git a/lldb/include/lldb/Host/common/NativeProcessProtocol.h b/lldb/include/lldb/Host/common/NativeProcessProtocol.h index 06b36c2cc9eb5..186a3d0f2f612 100644 --- a/lldb/include/lldb/Host/common/NativeProcessProtocol.h +++ b/lldb/include/lldb/Host/common/NativeProcessProtocol.h @@ -419,7 +419,6 @@ class NativeProcessProtocol { protected: struct SoftwareBreakpoint { - uint32_t ref_count; llvm::SmallVector<uint8_t, 4> saved_opcodes; llvm::ArrayRef<uint8_t> breakpoint_opcodes; }; diff --git a/lldb/source/Host/common/NativeProcessProtocol.cpp b/lldb/source/Host/common/NativeProcessProtocol.cpp index 196f54b93538d..dbffdc619ef42 100644 --- a/lldb/source/Host/common/NativeProcessProtocol.cpp +++ b/lldb/source/Host/common/NativeProcessProtocol.cpp @@ -344,10 +344,8 @@ Status NativeProcessProtocol::SetSoftwareBreakpoint(lldb::addr_t addr, LLDB_LOG(log, "addr = {0:x}, size_hint = {1}", addr, size_hint); auto it = m_software_breakpoints.find(addr); - if (it != m_software_breakpoints.end()) { - ++it->second.ref_count; + if (it != m_software_breakpoints.end()) return Status(); - } auto expected_bkpt = EnableSoftwareBreakpoint(addr, size_hint); if (!expected_bkpt) return Status::FromError(expected_bkpt.takeError()); @@ -362,14 +360,10 @@ Status NativeProcessProtocol::RemoveSoftwareBreakpoint(lldb::addr_t addr) { auto it = m_software_breakpoints.find(addr); if (it == m_software_breakpoints.end()) return Status::FromErrorString("Breakpoint not found."); - assert(it->second.ref_count > 0); - if (--it->second.ref_count > 0) - return Status(); // Remove the entry from m_software_breakpoints rightaway, so that we don't - // leave behind an entry with ref_count == 0 in case one of the following - // conditions returns an error. The breakpoint is moved so that it can be - // accessed below. + // leave behind an entry in case one of the following conditions returns an + // error. The breakpoint is moved so that it can be accessed below. SoftwareBreakpoint bkpt = std::move(it->second); m_software_breakpoints.erase(it); @@ -503,7 +497,7 @@ NativeProcessProtocol::EnableSoftwareBreakpoint(lldb::addr_t addr, } LLDB_LOG(log, "addr = {0:x}: SUCCESS", addr); - return SoftwareBreakpoint{1, saved_opcode_bytes, *expected_trap}; + return SoftwareBreakpoint{saved_opcode_bytes, *expected_trap}; } llvm::Expected<llvm::ArrayRef<uint8_t>> diff --git a/lldb/test/API/functionalities/multi-breakpoint/TestMultiBreakpoint.py b/lldb/test/API/functionalities/multi-breakpoint/TestMultiBreakpoint.py index eb9e2952d5a49..8e21884ea93b9 100644 --- a/lldb/test/API/functionalities/multi-breakpoint/TestMultiBreakpoint.py +++ b/lldb/test/API/functionalities/multi-breakpoint/TestMultiBreakpoint.py @@ -17,6 +17,9 @@ # Runs on systems where we can always predict the software break size @skipIf(archs=no_match(["x86_64", "arm64", "aarch64"])) class TestMultiBreakpoint(TestBase): + + NO_DEBUG_INFO_TESTCASE = True + def check_invalid_packet(self, packet_str): reply = lldbutil.send_packet_get_reply(self, packet_str) if reply.startswith("E"): @@ -60,6 +63,9 @@ def get_function_address(self, name): return f"{addr:x}" def test_multi_breakpoint(self): + # Debugserver uses refcounted breakpoints + breakpoints_are_refcounted = self.platformIsDarwin() + self.build() source_file = lldb.SBFileSpec("main.c") self.target, process, thread, bkpt = lldbutil.run_to_source_breakpoint( @@ -151,7 +157,7 @@ def make_packet(array): # Clean up both. array = [f"z0,{addr_a},{bp_kind}", f"z0,{addr_a},{bp_kind}"] reply = self.send_packet(make_packet(array)) - self.assertMultiResponse(reply, ["OK", "OK"]) + self.assertMultiResponse(reply, ["OK", "OK" if breakpoints_are_refcounted else "error"]) # --- Set the same breakpoint twice, but remove it thrice. array = [f"Z0,{addr_a},{bp_kind}", f"Z0,{addr_a},{bp_kind}"] @@ -163,7 +169,7 @@ def make_packet(array): f"z0,{addr_a},{bp_kind}", ] reply = self.send_packet(make_packet(array)) - self.assertMultiResponse(reply, ["OK", "OK", "error"]) + self.assertMultiResponse(reply, ["OK", "OK" if breakpoints_are_refcounted else "error", "error"]) # --- Set and remove the same address in a single packet --- # The spec requires requests to be executed in order, so the set _______________________________________________ lldb-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
