[lldbremote][NFC] Factor out code handling breakpoint packets

[felipepiovezan]

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:

• [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

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Felipe de Azevedo Piovezan (felipepiovezan)

Changes

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.

#192910

---

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

2 Files Affected:

• (modified) lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp (+148-106)
• (modified) lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h (+28)

diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
index 6a47a4ab37540..4c77de9f34adc 100644
--- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
@@ -2900,155 +2900,172 @@ GDBRemoteCommunicationServerLLGS::Handle_qMemoryRegionInfo(
   return SendPacketNoLock(response.GetString());
 }
 
-GDBRemoteCommunication::PacketResult
-GDBRemoteCommunicationServerLLGS::Handle_Z(StringExtractorGDBRemote &packet) {
+namespace {
+/// Helper struct to expand a GDBStoppointType into flags.
+struct BreakpointKind {
+  bool want_hardware;
+  bool want_breakpoint;
+  uint32_t watch_flags;
+
+  /// Invalid types must be handled prior to calling this.
+  BreakpointKind(GDBStoppointType stoppoint_type) {
+    switch (stoppoint_type) {
+    case eBreakpointSoftware:
+      want_hardware = false;
+      want_breakpoint = true;
+      break;
+    case eBreakpointHardware:
+      want_hardware = true;
+      want_breakpoint = true;
+      break;
+    case eWatchpointWrite:
+      watch_flags = 1;
+      want_hardware = true;
+      want_breakpoint = false;
+      break;
+    case eWatchpointRead:
+      watch_flags = 2;
+      want_hardware = true;
+      want_breakpoint = false;
+      break;
+    case eWatchpointReadWrite:
+      watch_flags = 3;
+      want_hardware = true;
+      want_breakpoint = false;
+      break;
+    default:
+      llvm_unreachable("unhandled GDBStoppointType");
+    }
+  }
+};
+
+/// If stoppoint_type is a valid type, create a BreakpointKind, otherwise
+/// returns nullopt.
+std::optional<BreakpointKind>
+getBreakpointKind(GDBStoppointType stoppoint_type) {
+  switch (stoppoint_type) {
+  case eBreakpointSoftware:
+  case eBreakpointHardware:
+  case eWatchpointWrite:
+  case eWatchpointRead:
+  case eWatchpointReadWrite:
+    return BreakpointKind(stoppoint_type);
+  case eStoppointInvalid:
+    break;
+  }
+  return std::nullopt;
+}
+} // namespace
+
+GDBRemoteCommunicationServerLLGS::BreakpointResult
+GDBRemoteCommunicationServerLLGS::ExecuteSetBreakpoint(
+    llvm::StringRef packet_str) {
   // Ensure we have a process.
   if (!m_current_process ||
       (m_current_process->GetID() == LLDB_INVALID_PROCESS_ID)) {
     Log *log = GetLog(LLDBLog::Process);
     LLDB_LOG(log, "failed, no process available");
-    return SendErrorResponse(0x15);
+    return BreakpointResult::CreateError(0x15);
   }
 
+  StringExtractorGDBRemote packet(packet_str);
+
   // Parse out software or hardware breakpoint or watchpoint requested.
   packet.SetFilePos(strlen("Z"));
   if (packet.GetBytesLeft() < 1)
-    return SendIllFormedResponse(
-        packet, "Too short Z packet, missing software/hardware specifier");
-
-  bool want_breakpoint = true;
-  bool want_hardware = false;
-  uint32_t watch_flags = 0;
+    return BreakpointResult::CreateIllFormed(
+        "Too short Z packet, missing software/hardware specifier");
 
   const GDBStoppointType stoppoint_type =
       GDBStoppointType(packet.GetS32(eStoppointInvalid));
-  switch (stoppoint_type) {
-  case eBreakpointSoftware:
-    want_hardware = false;
-    want_breakpoint = true;
-    break;
-  case eBreakpointHardware:
-    want_hardware = true;
-    want_breakpoint = true;
-    break;
-  case eWatchpointWrite:
-    watch_flags = 1;
-    want_hardware = true;
-    want_breakpoint = false;
-    break;
-  case eWatchpointRead:
-    watch_flags = 2;
-    want_hardware = true;
-    want_breakpoint = false;
-    break;
-  case eWatchpointReadWrite:
-    watch_flags = 3;
-    want_hardware = true;
-    want_breakpoint = false;
-    break;
-  case eStoppointInvalid:
-    return SendIllFormedResponse(
-        packet, "Z packet had invalid software/hardware specifier");
-  }
+  std::optional<BreakpointKind> bp_kind = getBreakpointKind(stoppoint_type);
+  if (!bp_kind)
+    return BreakpointResult::CreateIllFormed(
+        "Z packet had invalid software/hardware specifier");
 
   if ((packet.GetBytesLeft() < 1) || packet.GetChar() != ',')
-    return SendIllFormedResponse(
-        packet, "Malformed Z packet, expecting comma after stoppoint type");
+    return BreakpointResult::CreateIllFormed(
+        "Malformed Z packet, expecting comma after stoppoint type");
 
   // Parse out the stoppoint address.
   if (packet.GetBytesLeft() < 1)
-    return SendIllFormedResponse(packet, "Too short Z packet, missing address");
+    return BreakpointResult::CreateIllFormed(
+        "Too short Z packet, missing address");
   const lldb::addr_t addr = packet.GetHexMaxU64(false, 0);
 
   if ((packet.GetBytesLeft() < 1) || packet.GetChar() != ',')
-    return SendIllFormedResponse(
-        packet, "Malformed Z packet, expecting comma after address");
+    return BreakpointResult::CreateIllFormed(
+        "Malformed Z packet, expecting comma after address");
 
   // Parse out the stoppoint size (i.e. size hint for opcode size).
   const uint32_t size =
       packet.GetHexMaxU32(false, std::numeric_limits<uint32_t>::max());
   if (size == std::numeric_limits<uint32_t>::max())
-    return SendIllFormedResponse(
-        packet, "Malformed Z packet, failed to parse size argument");
+    return BreakpointResult::CreateIllFormed(
+        "Malformed Z packet, failed to parse size argument");
 
-  if (want_breakpoint) {
+  if (bp_kind->want_breakpoint) {
     // Try to set the breakpoint.
     const Status error =
-        m_current_process->SetBreakpoint(addr, size, want_hardware);
+        m_current_process->SetBreakpoint(addr, size, bp_kind->want_hardware);
     if (error.Success())
-      return SendOKResponse();
+      return BreakpointResult::CreateOK();
     Log *log = GetLog(LLDBLog::Breakpoints);
     LLDB_LOG(log, "pid {0} failed to set breakpoint: {1}",
              m_current_process->GetID(), error);
-    return SendErrorResponse(0x09);
-  } else {
-    // Try to set the watchpoint.
-    const Status error = m_current_process->SetWatchpoint(
-        addr, size, watch_flags, want_hardware);
-    if (error.Success())
-      return SendOKResponse();
-    Log *log = GetLog(LLDBLog::Watchpoints);
-    LLDB_LOG(log, "pid {0} failed to set watchpoint: {1}",
-             m_current_process->GetID(), error);
-    return SendErrorResponse(0x09);
-  }
+    return BreakpointResult::CreateError(0x09);
+  }
+
+  // Try to set the watchpoint.
+  const Status error = m_current_process->SetWatchpoint(
+      addr, size, bp_kind->watch_flags, bp_kind->want_hardware);
+  if (error.Success())
+    return BreakpointResult::CreateOK();
+  Log *log = GetLog(LLDBLog::Watchpoints);
+  LLDB_LOG(log, "pid {0} failed to set watchpoint: {1}",
+           m_current_process->GetID(), error);
+  return BreakpointResult::CreateError(0x09);
 }
 
-GDBRemoteCommunication::PacketResult
-GDBRemoteCommunicationServerLLGS::Handle_z(StringExtractorGDBRemote &packet) {
+GDBRemoteCommunicationServerLLGS::BreakpointResult
+GDBRemoteCommunicationServerLLGS::ExecuteRemoveBreakpoint(
+    llvm::StringRef packet_str) {
   // Ensure we have a process.
   if (!m_current_process ||
       (m_current_process->GetID() == LLDB_INVALID_PROCESS_ID)) {
     Log *log = GetLog(LLDBLog::Process);
     LLDB_LOG(log, "failed, no process available");
-    return SendErrorResponse(0x15);
+    return BreakpointResult::CreateError(0x15);
   }
 
+  StringExtractorGDBRemote packet(packet_str);
+
   // Parse out software or hardware breakpoint or watchpoint requested.
   packet.SetFilePos(strlen("z"));
   if (packet.GetBytesLeft() < 1)
-    return SendIllFormedResponse(
-        packet, "Too short z packet, missing software/hardware specifier");
-
-  bool want_breakpoint = true;
-  bool want_hardware = false;
+    return BreakpointResult::CreateIllFormed(
+        "Too short z packet, missing software/hardware specifier");
 
   const GDBStoppointType stoppoint_type =
       GDBStoppointType(packet.GetS32(eStoppointInvalid));
-  switch (stoppoint_type) {
-  case eBreakpointHardware:
-    want_breakpoint = true;
-    want_hardware = true;
-    break;
-  case eBreakpointSoftware:
-    want_breakpoint = true;
-    break;
-  case eWatchpointWrite:
-    want_breakpoint = false;
-    break;
-  case eWatchpointRead:
-    want_breakpoint = false;
-    break;
-  case eWatchpointReadWrite:
-    want_breakpoint = false;
-    break;
-  default:
-    return SendIllFormedResponse(
-        packet, "z packet had invalid software/hardware specifier");
-  }
+  std::optional<BreakpointKind> bp_kind = getBreakpointKind(stoppoint_type);
+  if (!bp_kind)
+    return BreakpointResult::CreateIllFormed(
+        "z packet had invalid software/hardware specifier");
 
   if ((packet.GetBytesLeft() < 1) || packet.GetChar() != ',')
-    return SendIllFormedResponse(
-        packet, "Malformed z packet, expecting comma after stoppoint type");
+    return BreakpointResult::CreateIllFormed(
+        "Malformed z packet, expecting comma after stoppoint type");
 
   // Parse out the stoppoint address.
   if (packet.GetBytesLeft() < 1)
-    return SendIllFormedResponse(packet, "Too short z packet, missing address");
+    return BreakpointResult::CreateIllFormed(
+        "Too short z packet, missing address");
   const lldb::addr_t addr = packet.GetHexMaxU64(false, 0);
 
   if ((packet.GetBytesLeft() < 1) || packet.GetChar() != ',')
-    return SendIllFormedResponse(
-        packet, "Malformed z packet, expecting comma after address");
+    return BreakpointResult::CreateIllFormed(
+        "Malformed z packet, expecting comma after address");
 
   /*
   // Parse out the stoppoint size (i.e. size hint for opcode size).
@@ -3059,26 +3076,51 @@ GDBRemoteCommunicationServerLLGS::Handle_z(StringExtractorGDBRemote &packet) {
   size argument");
   */
 
-  if (want_breakpoint) {
+  if (bp_kind->want_breakpoint) {
     // Try to clear the breakpoint.
     const Status error =
-        m_current_process->RemoveBreakpoint(addr, want_hardware);
+        m_current_process->RemoveBreakpoint(addr, bp_kind->want_hardware);
     if (error.Success())
-      return SendOKResponse();
+      return BreakpointResult::CreateOK();
     Log *log = GetLog(LLDBLog::Breakpoints);
     LLDB_LOG(log, "pid {0} failed to remove breakpoint: {1}",
              m_current_process->GetID(), error);
-    return SendErrorResponse(0x09);
-  } else {
-    // Try to clear the watchpoint.
-    const Status error = m_current_process->RemoveWatchpoint(addr);
-    if (error.Success())
-      return SendOKResponse();
-    Log *log = GetLog(LLDBLog::Watchpoints);
-    LLDB_LOG(log, "pid {0} failed to remove watchpoint: {1}",
-             m_current_process->GetID(), error);
-    return SendErrorResponse(0x09);
+    return BreakpointResult::CreateError(0x09);
+  }
+  // Try to clear the watchpoint.
+  const Status error = m_current_process->RemoveWatchpoint(addr);
+  if (error.Success())
+    return BreakpointResult::CreateOK();
+  Log *log = GetLog(LLDBLog::Watchpoints);
+  LLDB_LOG(log, "pid {0} failed to remove watchpoint: {1}",
+           m_current_process->GetID(), error);
+  return BreakpointResult::CreateError(0x09);
+}
+
+GDBRemoteCommunication::PacketResult
+GDBRemoteCommunicationServerLLGS::SendBreakpointResponse(
+    StringExtractorGDBRemote &packet, const BreakpointResult &result) {
+  switch (result.kind) {
+  case BreakpointResult::Kind::OK:
+    return SendOKResponse();
+  case BreakpointResult::Kind::Error:
+    return SendErrorResponse(result.error_code);
+  case BreakpointResult::Kind::IllFormed:
+    return SendIllFormedResponse(packet, result.message.c_str());
   }
+  llvm_unreachable("unhandled BreakpointResult kind");
+}
+
+GDBRemoteCommunication::PacketResult
+GDBRemoteCommunicationServerLLGS::Handle_Z(StringExtractorGDBRemote &packet) {
+  return SendBreakpointResponse(packet,
+                                ExecuteSetBreakpoint(packet.GetStringRef()));
+}
+
+GDBRemoteCommunication::PacketResult
+GDBRemoteCommunicationServerLLGS::Handle_z(StringExtractorGDBRemote &packet) {
+  return SendBreakpointResponse(packet,
+                                ExecuteRemoveBreakpoint(packet.GetStringRef()));
 }
 
 GDBRemoteCommunication::PacketResult
diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
index b400cc2fc21cd..a663c0b949744 100644
--- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
+++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
@@ -307,6 +307,34 @@ class GDBRemoteCommunicationServerLLGS
 private:
   llvm::Expected<std::unique_ptr<llvm::MemoryBuffer>> BuildTargetXml();
 
+  /// Helper struct for the Execute{Set,Remove}Breakpoint methods.
+  struct BreakpointResult {
+    enum class Kind { OK, Error, IllFormed };
+
+    Kind kind;
+    uint8_t error_code = 0; // Only meaningful when kind == Error.
+    std::string message;    // Only meaningful when kind == IllFormed.
+
+    static BreakpointResult CreateOK() { return {Kind::OK, 0, {}}; }
+    static BreakpointResult CreateError(uint8_t code) {
+      return {Kind::Error, code, {}};
+    }
+    static BreakpointResult CreateIllFormed(std::string msg) {
+      return {Kind::IllFormed, 0, std::move(msg)};
+    }
+  };
+
+  /// Core logic for a Z (set breakpoint/watchpoint) request.
+  BreakpointResult ExecuteSetBreakpoint(llvm::StringRef packet_str);
+
+  /// Core logic for a z (remove breakpoint/watchpoint) request.
+  BreakpointResult ExecuteRemoveBreakpoint(llvm::StringRef packet_str);
+
+  /// Convert a BreakpointResult into a PacketResult, sending the appropriate
+  /// response.
+  PacketResult SendBreakpointResponse(StringExtractorGDBRemote &packet,
+                                      const BreakpointResult &result);
+
   void HandleInferiorState_Exited(NativeProcessProtocol *process);
 
   void HandleInferiorState_Stopped(NativeProcessProtocol *process);

[JDevlieghere]

Should we move this out of the default case (so we get a warning if a enum value gets added) and retain the benefits of the unreachable by moving it beyond the switch and doing a return in the supported cases?