[lldb] Override UpdateBreakpointSites in ProcessGDBRemote to use MultiBreakpoint

[felipepiovezan]

This concludes the implementation of MultiBreakpoint by actually using the new packet to batch breakpoint requests.

The following PRs are related to the MultiBreakpoint feature:

• [lldb] Propose MultiBreakpoint extension to GDB Remote #192910
• [debugserver] Implement MultiBreakpoint #192914
• [lldbremote][NFC] Factor out code handling breakpoint packets #192915
• [lldbremote] 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

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[github-actions]

🐧 Linux x64 Test Results

• 33455 tests passed
• 532 tests skipped

✅ The build succeeded and all tests passed.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Felipe de Azevedo Piovezan (felipepiovezan)

Changes

This concludes the implementation of MultiBreakpoint by actually using the new packet to batch breakpoint requests.

The following PRs are related to the MultiBreakpoint feature:

• [lldb] Propose MultiBreakpoint extension to GDB Remote #192910
• [debugserver] Implement MultiBreakpoint #192914
• [lldbremote][NFC] Factor out code handling breakpoint packets #192915
• [lldbremote] 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

---

Full diff: https://github.com/llvm/llvm-project/pull/192988.diff

4 Files Affected:

• (modified) lldb/include/lldb/Utility/GDBRemote.h (+3)
• (modified) lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp (+192)
• (modified) lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h (+8)
• (modified) lldb/source/Utility/GDBRemote.cpp (+4)

diff --git a/lldb/include/lldb/Utility/GDBRemote.h b/lldb/include/lldb/Utility/GDBRemote.h
index 3b839c5d79485..ab33f02ebb012 100644
--- a/lldb/include/lldb/Utility/GDBRemote.h
+++ b/lldb/include/lldb/Utility/GDBRemote.h
@@ -42,6 +42,9 @@ class StreamGDBRemote : public StreamString {
   ///     Number of bytes written.
   // TODO: Convert this function to take ArrayRef<uint8_t>
   int PutEscapedBytes(const void *s, size_t src_len);
+
+  /// Equivalent to PutEscapedBytes(str.data(), str.size());
+  int PutEscapedBytes(llvm::StringRef str);
 };
 
 /// GDB remote packet as used by the GDB remote communication history. Packets
diff --git a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
index 33d3713c948c4..0d3f9d31efd22 100644
--- a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -6392,3 +6392,195 @@ void ProcessGDBRemote::DidExec() {
   }
   Process::DidExec();
 }
+
+llvm::Error ProcessGDBRemote::UpdateBreakpointSitesNotBatched(
+    const std::map<lldb::BreakpointSiteSP, Process::BreakpointAction>
+        &site_to_action) {
+  llvm::Error joined = llvm::Error::success();
+  for (auto &[site, action] : site_to_action) {
+    llvm::Error error = action == Process::BreakpointAction::Enable
+                            ? DoEnableBreakpointSite(*site)
+                            : DoDisableBreakpointSite(*site);
+    joined = llvm::joinErrors(std::move(joined), std::move(error));
+  }
+  return joined;
+}
+
+static llvm::Expected<StringExtractorGDBRemote>
+SendMultiBreakpointPacket(GDBRemoteCommunicationClient &gdb_comm,
+                          llvm::StringRef packet_str,
+                          std::chrono::seconds interrupt_timeout) {
+  StringExtractorGDBRemote response;
+  GDBRemoteCommunication::PacketResult packet_result =
+      gdb_comm.SendPacketAndWaitForResponse(packet_str, response,
+                                            interrupt_timeout);
+  if (packet_result != GDBRemoteCommunication::PacketResult::Success)
+    return llvm::createStringErrorV(
+        "MultiBreakpoint failed to send packet: '{0}'", packet_str);
+
+  if (response.IsUnsupportedResponse())
+    return llvm::createStringErrorV(
+        "MultiBreakpoint unsupported response: '{0}'", response.GetStringRef());
+
+  return response;
+}
+
+/// Parse a MultiBreakpoint response into per-request results.
+/// Returns a vector of results: std::nullopt means OK, a uint8_t value is the
+/// error code from an Exx response.
+static llvm::SmallVector<std::optional<uint8_t>>
+ParseMultiBreakpointResponse(llvm::StringRef response_str) {
+  llvm::SmallVector<std::optional<uint8_t>> results;
+
+  StructuredData::ObjectSP parsed = StructuredData::ParseJSON(response_str);
+  StructuredData::Dictionary *dict =
+      parsed ? parsed->GetAsDictionary() : nullptr;
+  StructuredData::Array *array = nullptr;
+  if (dict)
+    dict->GetValueForKeyAsArray("results", array);
+  if (!array)
+    return results;
+
+  array->ForEach([&results](StructuredData::Object *object) -> bool {
+    llvm::StringRef token;
+    if (auto *string = object->GetAsString())
+      token = string->GetValue();
+    if (token == "OK") {
+      results.push_back(std::nullopt);
+      return true;
+    }
+    if (token.size() != 3 || !token.starts_with("E")) {
+      results.push_back(uint8_t(0xff));
+      return true;
+    }
+    uint8_t error_code = 0;
+    if (token.drop_front(1).getAsInteger(16, error_code))
+      results.push_back(0xff);
+    else
+      results.push_back(error_code);
+    return true;
+  });
+  return results;
+}
+
+/// Determine the GDB stoppoint type for a breakpoint site by checking which
+/// packet types the remote supports (for insertions), or by checking the site
+/// type (for deletions).
+static std::optional<GDBStoppointType>
+GetStoppointType(BreakpointSite &site, bool insert,
+                 GDBRemoteCommunicationClient &gdb_comm) {
+  if (insert) {
+    if (!site.HardwareRequired() &&
+        gdb_comm.SupportsGDBStoppointPacket(eBreakpointSoftware))
+      return eBreakpointSoftware;
+    if (gdb_comm.SupportsGDBStoppointPacket(eBreakpointHardware))
+      return eBreakpointHardware;
+    return std::nullopt;
+  }
+
+  switch (site.GetType()) {
+  case BreakpointSite::eExternal:
+    return eBreakpointSoftware;
+  case BreakpointSite::eHardware:
+    return eBreakpointHardware;
+  case BreakpointSite::eSoftware:
+    return std::nullopt;
+  }
+  llvm_unreachable("unhandled BreakpointSite type");
+}
+
+namespace {
+struct BreakpointPacketInfo {
+  BreakpointSite &site;
+  size_t trap_opcode_size;
+  GDBStoppointType type;
+  bool is_enable;
+};
+
+std::string to_string(const BreakpointPacketInfo &info) {
+  char packet = info.is_enable ? 'Z' : 'z';
+  return llvm::formatv("{0}{1},{2:x-},{3:x-}", packet,
+                       static_cast<int>(info.type), info.site.GetLoadAddress(),
+                       info.trap_opcode_size)
+      .str();
+}
+} // namespace
+
+llvm::Error ProcessGDBRemote::UpdateBreakpointSites(
+    const std::map<lldb::BreakpointSiteSP, BreakpointAction> &site_to_action) {
+  if (site_to_action.empty())
+    return llvm::Error::success();
+  if (!m_gdb_comm.GetMultiBreakpointSupported())
+    return UpdateBreakpointSitesNotBatched(site_to_action);
+
+  Log *log = GetLog(GDBRLog::Breakpoints);
+
+  std::vector<BreakpointPacketInfo> breakpoint_infos;
+
+  for (auto [site, action] : site_to_action) {
+    addr_t addr = site->GetLoadAddress();
+    size_t trap_opcode_size = GetSoftwareBreakpointTrapOpcode(site.get());
+    std::optional<GDBStoppointType> type =
+        GetStoppointType(*site, action == BreakpointAction::Enable, m_gdb_comm);
+    if (!type) {
+      LLDB_LOG(log, "MultiBreakpoint: site {0} at {1:x} can't be batched",
+               site->GetID(), addr);
+      return UpdateBreakpointSitesNotBatched(site_to_action);
+    }
+    breakpoint_infos.push_back(
+        {*site, trap_opcode_size, *type, action == BreakpointAction::Enable});
+  }
+
+  StreamString stream;
+  stream << "jMultiBreakpoint:";
+
+  auto args_array = std::make_shared<StructuredData::Array>();
+  for (auto &bp_info : breakpoint_infos)
+    args_array->AddStringItem(to_string(bp_info));
+
+  StructuredData::Dictionary packet_dict;
+  packet_dict.AddItem("breakpoint_requests", args_array);
+  packet_dict.Dump(stream, false);
+
+  StreamGDBRemote escaped_stream;
+  escaped_stream.PutEscapedBytes(stream.GetString());
+  llvm::Expected<StringExtractorGDBRemote> response = SendMultiBreakpointPacket(
+      m_gdb_comm, escaped_stream.GetString(), GetInterruptTimeout());
+
+  if (!response) {
+    LLDB_LOG_ERROR(log, response.takeError(), "jMultiBreakpoint failed: {0}");
+    return UpdateBreakpointSitesNotBatched(site_to_action);
+  }
+
+  llvm::SmallVector<std::optional<uint8_t>> results =
+      ParseMultiBreakpointResponse(response->GetStringRef());
+
+  // This is a protocol violation, do nothing.
+  if (results.size() != site_to_action.size())
+    return llvm::createStringErrorV(
+        "MultiBreakpoint response count mismatch (expected {0}, got {1})",
+        site_to_action.size(), results.size());
+
+  // Process results: mark successful sites as enabled/disabled, retry failed
+  // sites individually.
+  llvm::Error joined = llvm::Error::success();
+  for (auto [error_code, bp_info] : llvm::zip(results, breakpoint_infos)) {
+    BreakpointSite &site = bp_info.site;
+    if (error_code == std::nullopt) {
+      SetBreakpointSiteEnabled(site, bp_info.is_enable);
+      if (bp_info.is_enable)
+        site.SetType(bp_info.type == eBreakpointHardware
+                         ? BreakpointSite::eHardware
+                         : BreakpointSite::eExternal);
+      continue;
+    }
+    LLDB_LOG(log,
+             "MultiBreakpoint: site {0} at {1:x} failed (E{2:X-2}), retrying",
+             site.GetID(), site.GetLoadAddress(), *error_code);
+    llvm::Error error = bp_info.is_enable ? DoEnableBreakpointSite(site)
+                                          : DoDisableBreakpointSite(site);
+    joined = llvm::joinErrors(std::move(joined), std::move(error));
+  }
+
+  return joined;
+}
diff --git a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
index 7c2877fa71d49..fa59655fe8a89 100644
--- a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
+++ b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
@@ -170,6 +170,10 @@ class ProcessGDBRemote : public Process,
   // Process Breakpoints
   Status EnableBreakpointSite(BreakpointSite *bp_site) override;
 
+  llvm::Error UpdateBreakpointSites(
+      const std::map<lldb::BreakpointSiteSP, BreakpointAction> &site_to_action)
+      override;
+
   Status DisableBreakpointSite(BreakpointSite *bp_site) override;
 
   // Process Watchpoints
@@ -462,6 +466,10 @@ class ProcessGDBRemote : public Process,
   /// z packet or restoring the original instruction.
   llvm::Error DoDisableBreakpointSite(BreakpointSite &bp_site);
 
+  llvm::Error UpdateBreakpointSitesNotBatched(
+      const std::map<lldb::BreakpointSiteSP, Process::BreakpointAction>
+          &site_to_action);
+
   static bool NewThreadNotifyBreakpointHit(void *baton,
                                            StoppointCallbackContext *context,
                                            lldb::user_id_t break_id,
diff --git a/lldb/source/Utility/GDBRemote.cpp b/lldb/source/Utility/GDBRemote.cpp
index f987ebcd4f63e..bd322a5e9e540 100644
--- a/lldb/source/Utility/GDBRemote.cpp
+++ b/lldb/source/Utility/GDBRemote.cpp
@@ -24,6 +24,10 @@ StreamGDBRemote::StreamGDBRemote(uint32_t flags, ByteOrder byte_order)
 
 StreamGDBRemote::~StreamGDBRemote() = default;
 
+int StreamGDBRemote::PutEscapedBytes(llvm::StringRef str) {
+  return PutEscapedBytes(str.data(), str.size());
+}
+
 int StreamGDBRemote::PutEscapedBytes(const void *s, size_t src_len) {
   int bytes_written = 0;
   const uint8_t *src = static_cast<const uint8_t *>(s);

[DavidSpickett]

Are there specific situations where we could get different results from the multi packet vs. sending the request on its own?

I can only think of:

• Dependent breakpoints, which is basically undefined behaviour in the way we've described the packet. So we could choose to retry, or not.
• A subset of that - hardware resources running out. If you had set then remove, the set could fail, the remove frees a hardware break, then the retry succeeds.
• Bugs in the multibreakpoint of the server, as the individual handlers will be more battle tested.
• The server supporting some subset of breakpoint packet types within that packet. This seems more likely as time goes on and a new breakpoint packet could have been added.

So it does seem worth it to have this fallback, but is there anything else you were thinking about?

[DavidSpickett]

(I also don't think it's cryptic to have this fallback, because the first port of call will be the packet log and you'll see very clearly the multibreak then the single ones)

[felipepiovezan]

I can't think of other scenarios

The server supporting some subset of breakpoint packet types within that packet. This seems more likely as time goes on and a new breakpoint packet could have been added.

Hopefully anyone implementing this packet would do what we did here: re-use code, so that the subset of packets supported is identical.

[github-actions]

✅ With the latest revision this PR passed the Python code formatter.

[felipepiovezan]

I've rebased this on top of main, all tests are passing.
Notably, I've removed all the "fallback to individual packets" paths. This simplifies the code and makes it actually correct. We have to trust that stubs will implement packets correctly, trying to dodge that is a path that leads to bugs and complexity.

[jasonmolenda]

LGTM

[felipepiovezan]

Any other comments?

[JDevlieghere]

🚢

[felipepiovezan]

I'll go ahead and merge this, happy to address any other concerns in follow-ups.