[lldb] Handle accelerator plugin breakpoint actions on the client

[satyajanga]

Wire up the client (ProcessGDBRemote) side of the accelerator plugin breakpoint protocol so the breakpoints requested by lldb-server accelerator plugins are actually set, hit, and acted upon.

The below is a related packet sequence from the test could help to better see things in action.

python3.12       < 104> send packet: $qSupported:xmlRegisters=i386,arm,mips,arc;multiprocess+;fork-events+;vfork-events+;swbreak+;hwbreak+#cd
python3.12       < 299> read packet: $PacketSize=131072;QStartNoAckMode+;qEcho+;native-signals+;QThreadSuffixSupported+;QListThreadsInStopReply+;qXfer:features:read+;QNonStop+;jMultiBreakpoint+;QPassSignals+;qXfer:auxv:read+;qXfer:libraries-svr4:read+;qXfer:siginfo:read+;accelerator-plugins+;multiprocess+;fork-events+;vfork-events+#86
python3.12       <  32> send packet: $jAcceleratorPluginInitialize#50
python3.12       < 238> read packet: $[{"breakpoints":[{"by_address":null,"by_name":{"function_name":"mock_gpu_accelerator_initialize","shlib":null}],"identifier":1,"symbol_names":["mock_gpu_accelerator_compute"]}]],"identifier":1,"plugin_name":"mock","session_name":""}]]#2f
python3.12       < 104> send packet: $qSupported:xmlRegisters=i386,arm,mips,arc;multiprocess+;fork-events+;vfork-events+;swbreak+;hwbreak+#cd
python3.12       < 299> read packet: $PacketSize=131072;QStartNoAckMode+;qEcho+;native-signals+;QThreadSuffixSupported+;QListThreadsInStopReply+;qXfer:features:read+;QNonStop+;jMultiBreakpoint+;QPassSignals+;qXfer:auxv:read+;qXfer:libraries-svr4:read+;qXfer:siginfo:read+;accelerator-plugins+;multiprocess+;fork-events+;vfork-events+#86
intern-state     < 314> send packet: $jAcceleratorPluginBreakpointHit:{"breakpoint":{"by_address":null,"by_name":{"function_name":"mock_gpu_accelerator_initialize","shlib":null}],"identifier":1,"symbol_names":["mock_gpu_accelerator_compute"]}],"plugin_name":"mock","symbol_values":[{"name":"mock_gpu_accelerator_compute","value":94768064848192}]]}]#ef
intern-state     < 360> read packet: ${"actions":{"breakpoints":[{"by_address":null,"by_name":{"function_name":"mock_gpu_accelerator_finish","shlib":"a.out"}],"identifier":3,"symbol_names":[]}],{"by_address":{"load_address":94768064848192}],"by_name":null,"identifier":2,"symbol_names":[]}]],"identifier":2,"plugin_name":"mock","session_name":""}],"auto_resume_native":false,"disable_bp":true}]#44
intern-state     < 188> send packet: $jAcceleratorPluginBreakpointHit:{"breakpoint":{"by_address":{"load_address":94768064848192}],"by_name":null,"identifier":2,"symbol_names":[]}],"plugin_name":"mock","symbol_values":[]}]#fd
intern-state     < 220> send packet: $jAcceleratorPluginBreakpointHit:{"breakpoint":{"by_address":null,"by_name":{"function_name":"mock_gpu_accelerator_finish","shlib":"a.out"}],"identifier":3,"symbol_names":[]}],"plugin_name":"mock","symbol_values":[]}]#d7
satyajanga@devgpu011:llvm-project  (accelerator-plugin-handle-actions)$

Code changes:

• GDBRemoteCommunicationClient learns the "accelerator-plugins+" feature from qSupported and gains GetAcceleratorInitializeActions() and AcceleratorBreakpointHit() to drive the jAcceleratorPluginInitialize and jAcceleratorPluginBreakpointHit packets.
• ProcessGDBRemote::HandleAcceleratorActions() sets the requested breakpoints as internal breakpoints with a synchronous callback. When a callback fires it resolves any requested symbol values, notifies the plugin, disables the breakpoint and/or auto-resumes the native process per the response, and handles any further actions (e.g. new breakpoints) the plugin returns. Initial actions are fetched in DidLaunchOrAttach.
• The mock accelerator plugin now sets its initialize breakpoint on a dedicated "accelerator_initialize" hook (rather than "main") so it only affects this test's inferior, and exercises all three breakpoint types: by name, by name scoped to a shared library, and by address.
• Add an end-to-end API test that launches a real process and verifies the breakpoints are set, hit, and chained, plus updated packet-level coverage.

[github-actions]

🪟 Windows x64 Test Results

• 33026 tests passed
• 892 tests skipped

✅ The build succeeded and all tests passed.

[llvmorg-github-actions]

@llvm/pr-subscribers-lldb

Author: satyanarayana reddy janga (satyajanga)

Changes

Wire up the client (ProcessGDBRemote) side of the accelerator plugin breakpoint protocol so the breakpoints requested by lldb-server accelerator plugins are actually set, hit, and acted upon.

• GDBRemoteCommunicationClient learns the "accelerator-plugins+" feature from qSupported and gains GetAcceleratorInitializeActions() and AcceleratorBreakpointHit() to drive the jAcceleratorPluginInitialize and jAcceleratorPluginBreakpointHit packets.
• ProcessGDBRemote::HandleAcceleratorActions() sets the requested breakpoints as internal breakpoints with a synchronous callback. When a callback fires it resolves any requested symbol values, notifies the plugin, disables the breakpoint and/or auto-resumes the native process per the response, and handles any further actions (e.g. new breakpoints) the plugin returns. Initial actions are fetched in DidLaunchOrAttach.
• The mock accelerator plugin now sets its initialize breakpoint on a dedicated "accelerator_initialize" hook (rather than "main") so it only affects this test's inferior, and exercises all three breakpoint types: by name, by name scoped to a shared library, and by address.
• Add an end-to-end API test that launches a real process and verifies the breakpoints are set, hit, and chained, plus updated packet-level coverage.

---

Patch is 30.56 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/201489.diff

9 Files Affected:

• (modified) lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp (+79)
• (modified) lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h (+15)
• (modified) lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp (+181)
• (modified) lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h (+26)
• (added) lldb/test/API/accelerator/mock/TestMockAcceleratorBreakpoints.py (+92)
• (modified) lldb/test/API/accelerator/mock/TestMockAcceleratorPlugin.py (+19-8)
• (modified) lldb/test/API/accelerator/mock/main.c (+15-1)
• (modified) lldb/tools/lldb-server/Plugins/Accelerator/Mock/LLDBServerMockAcceleratorPlugin.cpp (+41-7)
• (modified) lldb/tools/lldb-server/Plugins/Accelerator/Mock/LLDBServerMockAcceleratorPlugin.h (+7-1)

diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
index 8df7936786b04..316312822b8a2 100644
--- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
@@ -15,6 +15,7 @@
 #include <optional>
 #include <sstream>
 
+#include "lldb/Core/Debugger.h"
 #include "lldb/Core/ModuleSpec.h"
 #include "lldb/Host/HostInfo.h"
 #include "lldb/Host/SafeMachO.h"
@@ -23,6 +24,7 @@
 #include "lldb/Target/MemoryRegionInfo.h"
 #include "lldb/Target/Target.h"
 #include "lldb/Target/UnixSignals.h"
+#include "lldb/Utility/AcceleratorGDBRemotePackets.h"
 #include "lldb/Utility/Args.h"
 #include "lldb/Utility/DataBufferHeap.h"
 #include "lldb/Utility/LLDBAssert.h"
@@ -223,6 +225,79 @@ bool GDBRemoteCommunicationClient::GetMultiBreakpointSupported() {
   return m_supports_multi_breakpoint == eLazyBoolYes;
 }
 
+bool GDBRemoteCommunicationClient::GetAcceleratorPluginsSupported() {
+  if (m_supports_accelerator_plugins == eLazyBoolCalculate)
+    GetRemoteQSupported();
+  return m_supports_accelerator_plugins == eLazyBoolYes;
+}
+
+std::optional<std::vector<AcceleratorActions>>
+GDBRemoteCommunicationClient::GetAcceleratorInitializeActions() {
+  // Get the initial actions (e.g. breakpoints to set) requested by any
+  // accelerator plugins using the "jAcceleratorPluginInitialize" packet. This
+  // is sent once when a native process is launched or attached.
+  if (!GetAcceleratorPluginsSupported())
+    return std::nullopt;
+
+  StringExtractorGDBRemote response;
+  response.SetResponseValidatorToJSON();
+  if (SendPacketAndWaitForResponse("jAcceleratorPluginInitialize", response) !=
+      PacketResult::Success)
+    return std::nullopt;
+
+  if (response.IsUnsupportedResponse())
+    return std::nullopt;
+
+  if (response.IsErrorResponse()) {
+    Debugger::ReportError(response.GetStatus().AsCString());
+    return std::nullopt;
+  }
+
+  llvm::Expected<std::vector<AcceleratorActions>> actions =
+      llvm::json::parse<std::vector<AcceleratorActions>>(response.Peek(),
+                                                         "AcceleratorActions");
+  if (actions)
+    return std::move(*actions);
+
+  // JSON parsing errors won't make sense to the user, so don't show them.
+  llvm::consumeError(actions.takeError());
+  Debugger::ReportError(
+      llvm::formatv("malformed jAcceleratorPluginInitialize response: {0}",
+                    response.GetStringRef()));
+  return std::nullopt;
+}
+
+std::optional<AcceleratorBreakpointHitResponse>
+GDBRemoteCommunicationClient::AcceleratorBreakpointHit(
+    const AcceleratorBreakpointHitArgs &args) {
+  StreamGDBRemote packet;
+  packet.PutCString("jAcceleratorPluginBreakpointHit:");
+  packet.PutAsJSON(args, /*hex_ascii=*/false);
+
+  StringExtractorGDBRemote response;
+  if (SendPacketAndWaitForResponse(packet.GetString(), response) !=
+      PacketResult::Success)
+    return std::nullopt;
+
+  if (response.IsErrorResponse()) {
+    Debugger::ReportError(response.GetStatus().AsCString());
+    return std::nullopt;
+  }
+
+  llvm::Expected<AcceleratorBreakpointHitResponse> hit_response =
+      llvm::json::parse<AcceleratorBreakpointHitResponse>(
+          response.Peek(), "AcceleratorBreakpointHitResponse");
+  if (hit_response)
+    return std::move(*hit_response);
+
+  // JSON parsing errors won't make sense to the user, so don't show them.
+  llvm::consumeError(hit_response.takeError());
+  Debugger::ReportError(
+      llvm::formatv("malformed jAcceleratorPluginBreakpointHit response: {0}",
+                    response.GetStringRef()));
+  return std::nullopt;
+}
+
 bool GDBRemoteCommunicationClient::QueryNoAckModeSupported() {
   if (m_supports_not_sending_acks == eLazyBoolCalculate) {
     m_send_acks = true;
@@ -321,6 +396,7 @@ void GDBRemoteCommunicationClient::ResetDiscoverableSettings(bool did_exec) {
     m_x_packet_state.reset();
     m_supports_reverse_continue = eLazyBoolCalculate;
     m_supports_reverse_step = eLazyBoolCalculate;
+    m_supports_accelerator_plugins = eLazyBoolCalculate;
     m_supports_qProcessInfoPID = true;
     m_supports_qfProcessInfo = true;
     m_supports_qUserName = true;
@@ -381,6 +457,7 @@ void GDBRemoteCommunicationClient::GetRemoteQSupported() {
   m_supports_reverse_step = eLazyBoolNo;
   m_supports_multi_mem_read = eLazyBoolNo;
   m_supports_multi_breakpoint = eLazyBoolNo;
+  m_supports_accelerator_plugins = eLazyBoolNo;
 
   m_max_packet_size = UINT64_MAX; // It's supposed to always be there, but if
                                   // not, we assume no limit
@@ -444,6 +521,8 @@ void GDBRemoteCommunicationClient::GetRemoteQSupported() {
         m_supports_multi_mem_read = eLazyBoolYes;
       else if (x == "jMultiBreakpoint+")
         m_supports_multi_breakpoint = eLazyBoolYes;
+      else if (x == "accelerator-plugins+")
+        m_supports_accelerator_plugins = eLazyBoolYes;
       // Look for a list of compressions in the features list e.g.
       // qXfer:features:read+;PacketSize=20000;qEcho+;SupportedCompressions=zlib-
       // deflate,lzma
diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
index 79ca0bcd3ed22..09ebd8ef726d7 100644
--- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
+++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
@@ -19,6 +19,7 @@
 #include <vector>
 
 #include "lldb/Host/File.h"
+#include "lldb/Utility/AcceleratorGDBRemotePackets.h"
 #include "lldb/Utility/AddressableBits.h"
 #include "lldb/Utility/ArchSpec.h"
 #include "lldb/Utility/GDBRemote.h"
@@ -346,6 +347,19 @@ class GDBRemoteCommunicationClient : public GDBRemoteClientBase {
 
   bool GetMultiBreakpointSupported();
 
+  bool GetAcceleratorPluginsSupported();
+
+  /// Send the "jAcceleratorPluginInitialize" packet and return the actions
+  /// requested by each accelerator plugin installed in lldb-server.
+  std::optional<std::vector<AcceleratorActions>>
+  GetAcceleratorInitializeActions();
+
+  /// Send the "jAcceleratorPluginBreakpointHit" packet to notify the
+  /// accelerator plugin that one of its requested breakpoints was hit, and
+  /// return the plugin's response.
+  std::optional<AcceleratorBreakpointHitResponse>
+  AcceleratorBreakpointHit(const AcceleratorBreakpointHitArgs &args);
+
   LazyBool SupportsAllocDeallocMemory() // const
   {
     // Uncomment this to have lldb pretend the debug server doesn't respond to
@@ -582,6 +596,7 @@ class GDBRemoteCommunicationClient : public GDBRemoteClientBase {
   LazyBool m_supports_reverse_step = eLazyBoolCalculate;
   LazyBool m_supports_multi_mem_read = eLazyBoolCalculate;
   LazyBool m_supports_multi_breakpoint = eLazyBoolCalculate;
+  LazyBool m_supports_accelerator_plugins = eLazyBoolCalculate;
 
   bool m_supports_qProcessInfoPID : 1, m_supports_qfProcessInfo : 1,
       m_supports_qUserName : 1, m_supports_qGroupName : 1,
diff --git a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
index f6eaf5851338b..0bb1675f0c313 100644
--- a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -23,6 +23,8 @@
 #include <ctime>
 #include <sys/types.h>
 
+#include "lldb/Breakpoint/Breakpoint.h"
+#include "lldb/Breakpoint/StoppointCallbackContext.h"
 #include "lldb/Breakpoint/Watchpoint.h"
 #include "lldb/Breakpoint/WatchpointAlgorithms.h"
 #include "lldb/Breakpoint/WatchpointResource.h"
@@ -52,6 +54,8 @@
 #include "lldb/Interpreter/Options.h"
 #include "lldb/Interpreter/Property.h"
 #include "lldb/Symbol/ObjectFile.h"
+#include "lldb/Symbol/Symbol.h"
+#include "lldb/Symbol/SymbolContext.h"
 #include "lldb/Target/ABI.h"
 #include "lldb/Target/DynamicLoader.h"
 #include "lldb/Target/MemoryRegionInfo.h"
@@ -61,7 +65,9 @@
 #include "lldb/Target/TargetList.h"
 #include "lldb/Target/ThreadPlanCallFunction.h"
 #include "lldb/Utility/Args.h"
+#include "lldb/Utility/Baton.h"
 #include "lldb/Utility/FileSpec.h"
+#include "lldb/Utility/FileSpecList.h"
 #include "lldb/Utility/LLDBLog.h"
 #include "lldb/Utility/State.h"
 #include "lldb/Utility/StreamString.h"
@@ -1060,6 +1066,19 @@ void ProcessGDBRemote::DidLaunchOrAttach(ArchSpec &process_arch) {
     else
       SetUnixSignals(UnixSignals::Create(GetTarget().GetArchitecture()));
   }
+
+  // Ask any accelerator plugins installed in lldb-server for their initial
+  // actions (e.g. breakpoints to set in the native process).
+  if (std::optional<std::vector<AcceleratorActions>> init_actions =
+          m_gdb_comm.GetAcceleratorInitializeActions()) {
+    for (const AcceleratorActions &actions : *init_actions) {
+      if (Status error = HandleAcceleratorActions(actions); error.Fail())
+        Debugger::ReportError(llvm::formatv(
+            "failed to handle accelerator actions for plugin "
+            "'{0}': {1}\nActions:\n{2}\n",
+            actions.plugin_name, error.AsCString(), toJSON(actions)));
+    }
+  }
 }
 
 void ProcessGDBRemote::LoadStubBinaries() {
@@ -4124,6 +4143,168 @@ bool ProcessGDBRemote::NewThreadNotifyBreakpointHit(
   return false;
 }
 
+namespace {
+/// Baton that carries the breakpoint hit arguments to the accelerator plugin
+/// breakpoint callback.
+class AcceleratorBreakpointCallbackBaton
+    : public TypedBaton<AcceleratorBreakpointHitArgs> {
+public:
+  explicit AcceleratorBreakpointCallbackBaton(
+      std::unique_ptr<AcceleratorBreakpointHitArgs> data)
+      : TypedBaton(std::move(data)) {}
+};
+} // namespace
+
+Status
+ProcessGDBRemote::HandleAcceleratorActions(const AcceleratorActions &actions) {
+  Log *log = GetLog(GDBRLog::Process);
+
+  // The same set of actions can be delivered to the client more than once: a
+  // plugin may keep reporting the same actions (with the same identifier) on
+  // subsequent native stops until its state advances. The identifier uniquely
+  // names a set of actions for a plugin, so skip any set we have already
+  // processed to avoid re-running its side effects (e.g. setting the same
+  // breakpoints again).
+  auto it = m_processed_accelerator_actions.find(actions.plugin_name);
+  if (it != m_processed_accelerator_actions.end() &&
+      it->second == actions.identifier) {
+    LLDB_LOG(log,
+             "ProcessGDBRemote::HandleAcceleratorActions skipping already "
+             "processed actions for plugin '{0}' with identifier {1}",
+             actions.plugin_name, actions.identifier);
+    return Status();
+  }
+  m_processed_accelerator_actions[actions.plugin_name] = actions.identifier;
+
+  if (!actions.breakpoints.empty())
+    return HandleAcceleratorBreakpoints(actions);
+
+  return Status();
+}
+
+Status ProcessGDBRemote::HandleAcceleratorBreakpoints(
+    const AcceleratorActions &actions) {
+  Target &target = GetTarget();
+  Status error;
+  for (const AcceleratorBreakpointInfo &bp : actions.breakpoints) {
+    // Carry data with the breakpoint so the callback can notify the plugin
+    // when the breakpoint is hit.
+    auto args_up = std::make_unique<AcceleratorBreakpointHitArgs>();
+    args_up->plugin_name = actions.plugin_name;
+    args_up->breakpoint = bp;
+
+    BreakpointSP bp_sp;
+    if (bp.by_name) {
+      FileSpecList bp_modules;
+      if (bp.by_name->shlib && !bp.by_name->shlib->empty())
+        bp_modules.Append(FileSpec(*bp.by_name->shlib));
+      bp_sp = target.CreateBreakpoint(
+          bp_modules.GetSize() ? &bp_modules : nullptr, // Containing modules.
+          nullptr,                                      // Containing source.
+          bp.by_name->function_name.c_str(),            // Function name.
+          eFunctionNameTypeFull,                        // Function name type.
+          eLanguageTypeUnknown,                         // Language type.
+          0,                                            // Byte offset.
+          false,                                        // Offset is insn count.
+          eLazyBoolNo,                                  // Skip prologue.
+          true,                                         // Internal breakpoint.
+          false);                                       // Request hardware.
+    } else if (bp.by_address) {
+      bp_sp = target.CreateBreakpoint(bp.by_address->load_address,
+                                      /*internal=*/true,
+                                      /*request_hardware=*/false);
+    } else {
+      // Record the first error but keep setting the remaining breakpoints.
+      if (error.Success())
+        error = Status::FromErrorStringWithFormatv(
+            "accelerator breakpoint {0} has neither a by_name nor a by_address "
+            "specification",
+            bp.identifier);
+      continue;
+    }
+
+    if (!bp_sp) {
+      if (error.Success())
+        error = Status::FromErrorStringWithFormatv(
+            "failed to set accelerator breakpoint {0}", bp.identifier);
+      continue;
+    }
+
+    // Give the internal breakpoint a meaningful description for stop reasons.
+    bp_sp->SetBreakpointKind("accelerator-plugin");
+    auto baton_sp = std::make_shared<AcceleratorBreakpointCallbackBaton>(
+        std::move(args_up));
+    bp_sp->SetCallback(AcceleratorBreakpointHitCallback, baton_sp,
+                       /*is_synchronous=*/true);
+  }
+  return error;
+}
+
+bool ProcessGDBRemote::AcceleratorBreakpointHitCallback(
+    void *baton, StoppointCallbackContext *context, lldb::user_id_t break_id,
+    lldb::user_id_t break_loc_id) {
+  ProcessSP process_sp = context->exe_ctx_ref.GetProcessSP();
+  ProcessGDBRemote *process = static_cast<ProcessGDBRemote *>(process_sp.get());
+  return process->AcceleratorBreakpointHit(baton, context, break_id,
+                                           break_loc_id);
+}
+
+bool ProcessGDBRemote::AcceleratorBreakpointHit(
+    void *baton, StoppointCallbackContext *context, lldb::user_id_t break_id,
+    lldb::user_id_t break_loc_id) {
+  AcceleratorBreakpointHitArgs *callback_data =
+      static_cast<AcceleratorBreakpointHitArgs *>(baton);
+  // Copy the args so we can fill in requested symbol values before notifying
+  // lldb-server.
+  AcceleratorBreakpointHitArgs args = *callback_data;
+  Target &target = GetTarget();
+
+  const std::vector<std::string> &symbol_names = args.breakpoint.symbol_names;
+  args.symbol_values.resize(symbol_names.size());
+  for (size_t i = 0; i < symbol_names.size(); ++i) {
+    args.symbol_values[i].name = symbol_names[i];
+    SymbolContextList sc_list;
+    target.GetImages().FindSymbolsWithNameAndType(ConstString(symbol_names[i]),
+                                                  eSymbolTypeAny, sc_list);
+    for (const SymbolContext &sc : sc_list) {
+      if (!sc.symbol)
+        continue;
+      addr_t load_addr = sc.symbol->GetAddress().GetLoadAddress(&target);
+      if (load_addr != LLDB_INVALID_ADDRESS) {
+        args.symbol_values[i].value = load_addr;
+        break;
+      }
+    }
+  }
+
+  std::optional<AcceleratorBreakpointHitResponse> response =
+      m_gdb_comm.AcceleratorBreakpointHit(args);
+  if (!response)
+    return false;
+
+  // Disable the breakpoint if requested, but keep it around so its hit count
+  // and other stats remain visible.
+  if (response->disable_bp) {
+    if (BreakpointSP bp_sp = target.GetBreakpointByID(break_id))
+      bp_sp->SetEnabled(false);
+  }
+
+  // The plugin may request new actions (e.g. additional breakpoints) in
+  // response to this breakpoint being hit.
+  if (response->actions) {
+    if (Status error = HandleAcceleratorActions(*response->actions);
+        error.Fail())
+      Debugger::ReportError(
+          llvm::formatv("failed to handle accelerator actions for plugin "
+                        "'{0}': {1}\nActions:\n{2}\n",
+                        response->actions->plugin_name, error.AsCString(),
+                        toJSON(*response->actions)));
+  }
+
+  // Returning true stops the native process; false auto-resumes it.
+  return !response->auto_resume_native;
+}
+
 Status ProcessGDBRemote::UpdateAutomaticSignalFiltering() {
   Log *log = GetLog(GDBRLog::Process);
   LLDB_LOG(log, "Check if need to update ignored signals");
diff --git a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
index c6d67b3aa5350..6be1977dde83f 100644
--- a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
+++ b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
@@ -478,6 +478,32 @@ class ProcessGDBRemote : public Process,
   /// Remove the breakpoints associated with thread creation from the Target.
   void RemoveNewThreadBreakpoints();
 
+  /// Handle a set of actions requested by an accelerator plugin. Currently this
+  /// only sets the breakpoints requested in \a actions.
+  Status HandleAcceleratorActions(const AcceleratorActions &actions);
+
+  /// Set the breakpoints requested by an accelerator plugin as internal
+  /// breakpoints with a callback that notifies the plugin when they are hit.
+  /// Returns an error if any breakpoint could not be set; the remaining
+  /// breakpoints are still set.
+  Status HandleAcceleratorBreakpoints(const AcceleratorActions &actions);
+
+  /// Breakpoint callback invoked when an accelerator-plugin-requested
+  /// breakpoint is hit. Resolves any requested symbol values, notifies the
+  /// plugin via the "jAcceleratorPluginBreakpointHit" packet, and handles the
+  /// response.
+  static bool AcceleratorBreakpointHitCallback(
+      void *baton, StoppointCallbackContext *context, lldb::user_id_t break_id,
+      lldb::user_id_t break_loc_id);
+
+  bool AcceleratorBreakpointHit(void *baton, StoppointCallbackContext *context,
+                                lldb::user_id_t break_id,
+                                lldb::user_id_t break_loc_id);
+
+  /// Tracks the last action identifier handled per accelerator plugin so the
+  /// same actions are not processed more than once.
+  std::map<std::string, int64_t> m_processed_accelerator_actions;
+
   // ContinueDelegate interface
   void HandleAsyncStdout(llvm::StringRef out) override;
   void HandleAsyncMisc(llvm::StringRef data) override;
diff --git a/lldb/test/API/accelerator/mock/TestMockAcceleratorBreakpoints.py b/lldb/test/API/accelerator/mock/TestMockAcceleratorBreakpoints.py
new file mode 100644
index 0000000000000..dbc19708c538d
--- /dev/null
+++ b/lldb/test/API/accelerator/mock/TestMockAcceleratorBreakpoints.py
@@ -0,0 +1,92 @@
+"""
+End-to-end test for accelerator plugin breakpoints.
+
+Launches a real process against an lldb-server that has the mock accelerator
+plugin enabled and verifies that the breakpoints requested by the plugin are
+set in the native process, hit, and that hitting one breakpoint can request
+further breakpoints. This exercises all three breakpoint types: by name, by
+name scoped to a shared library, and by address.
+"""
+
+import lldb
+import lldbsuite.test.lldbutil as lldbutil
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import configuration
+
+
+class MockAcceleratorBreakpointsTestCase(TestBase):
+    NO_DEBUG_INFO_TESTCASE = True
+
+    def setUp(self):
+        super().setUp()
+        if "mock-accelerator" not in configuration.enabled_plugins:
+            self.skipTest("mock-accelerator plugin is not enabled")
+
+    def check_accelerator_stop(self, process, function_name):
+        """Verify the process stopped at an internal accelerator breakpoint in
+        the given function, and return that breakpoint."""
+        self.assertState(process.GetState(), lldb.eStateStopped)
+        thread = process.GetSelectedThread()
+
+        # The stop must be due to a breakpoint, and the frame must be in the
+        # expected function.
+        self.assertStopReason(thread.GetStopReason(), lldb.eStopReasonBreakpoint)
+        frame = thread.GetFrameAtIndex(0)
+        self.assertEqual(frame.GetFunctionName(), function_name)
+
+        # The breakpoint id is carried in the stop reason data. Accelerator
+        # breakpoints are internal, so they are not in the public breakpoint
+        # list, but can still be looked up by id.
+        sel...
[truncated]

[DavidSpickett]

What's the logic for using Debugger::ReportError to report problems rather than logging them? Nothing else in the GDB client uses it.

I guess the justification is:

• Logging is ok if you know it exists, but only if, and is better for things that don't break the whole session (degraded register information for example).
• The errors that non-plugin users would want to ignore are pretty easy to avoid. For instance the malformed response to listing the plugins. This could happen to anyone in theory but is unlikely given that lldb-server is the one making that response.
• The remaining things should be dealt with ASAP if the user has any hope of the accelerator part working.

Which makes sense to me, but tell me if you thought along the same lines.

We could always change it later anyway. For as long as the entire accelerator plugin thing is opt in at build time, we don't have to think about what happens to random distro builds of LLDB and their users who probably don't care about accelerators.

[DavidSpickett]

It would help my understanding a lot if you included an overview of the packet sequence / event sequence with these PRs. Each PR would not be the whole sequence of course but you could point out what part the PR is making changes to.

One thing I wasn't clear on was all the points where actions could be received. It seems those points are:

• The intialise packet.
• In response to telling the server we hit a breakpoint that a plugin asked for.
• In stop reply packets, but I'm not sure what this looks like in practice.

Which means we could get duplicates and you seem to handle that, but are you testing that behaviour too? (I didn't look too closely)

Another thing I noticed was that https://lldb.llvm.org/resources/lldbgdbremote.html#jacceleratorpluginbreakpointhit does not say what to do if the client cannot find one of the symbol's requested plugins. Should it include the key with no value, or a null value, or just leave out the key? Not a big deal but good to mention it and double check that the server side code is handling that situation.

(this area is on my mind since I recently worked on https://lldb.llvm.org/resources/lldbgdbremote.html#qsymbol which is a very similar idea, but triggered by library load rather than a breakpoint)

[DavidSpickett]

I had to read this a few times to understand it. I think you mean that showing the user "missing comma at line 4" is useless without showing them all the lines, right?

So show them the response in full and they can throw it at whatever json parser they want, or likely, spot it by eye.

Which is great, but you could also present both the error and the response? Assuming it has some form of line number that matches up. If not, the response only is fine.

Then make the comment clearer, for instance:

// JSON parsing errors won't make sense to the user, even if we include the response as well. So just print the whole response and let the user spot the problem.

[clayborg]

If we can print the syntax error and the JSON content, that would be best. But it should probably be in a log message only unless the error can be propagated up to the caller where the caller can do something useful.

[satyajanga]

updated the logging

[DavidSpickett]

It assume it would be nice to know the name of the plugin too. accelerator plugin (AMDGPU) maybe.

If you do add it, first check what SetBreakpointKind assumes about the lifetime of the string.

[clayborg]

It stores it in a std::string, so lifetime is good. Good idea about adding the plug-in name

[DavidSpickett]

Also I think now would be a good time to document the expected format of the breakpoints to be set in the native process. (in https://lldb.llvm.org/resources/lldbgdbremote.html#jacceleratorplugininitialize)

[clayborg]

What's the logic for using Debugger::ReportError to report problems rather than logging them? Nothing else in the GDB client uses it.

I guess the justification is:

• Logging is ok if you know it exists, but only if, and is better for things that don't break the whole session (degraded register information for example).
• The errors that non-plugin users would want to ignore are pretty easy to avoid. For instance the malformed response to listing the plugins. This could happen to anyone in theory but is unlikely given that lldb-server is the one making that response.
• The remaining things should be dealt with ASAP if the user has any hope of the accelerator part working.

Which makes sense to me, but tell me if you thought along the same lines.

We could always change it later anyway. For as long as the entire accelerator plugin thing is opt in at build time, we don't have to think about what happens to random distro builds of LLDB and their users who probably don't care about accelerators.

Yeah, I noticed this as well. I believe the Debugger::ReportError(...) shouldn't be used and we should either return a llvm::Error from any function that runs into this so callers can respond correctly. So I agree, lets not spew things to the debugger console and use logging and llvm::Error when and where it makes sense.

[clayborg]

It would help my understanding a lot if you included an overview of the packet sequence / event sequence with these PRs. Each PR would not be the whole sequence of course but you could point out what part the PR is making changes to.

One thing I wasn't clear on was all the points where actions could be received. It seems those points are:

• The intialise packet.
• In response to telling the server we hit a breakpoint that a plugin asked for.
• In stop reply packets, but I'm not sure what this looks like in practice.

This is correct. We can document this. For the stop reply packets, we had been toying with a new stop reason eStopReasonDyld that could have some AcceleratorActions that could indicate that we needed to load shared libraries and then the process auto resumes. I don't believe we are using this anymore, so we can leave out the stop reply packets from this documentation and only have the init packet and breakpoint hit packet for now unless we use the stop reply.

One tricky thing is sometimes GPUs have nothing to show when they are not running any kernels, but then a kernel gets loaded and we need to load libraries and we need to notify LLDB. So we needed to stop the GPU GDB server, but there was nothing to show as there were no active threads, but GDB remote requires a thread when stopping, so we had to invent a synthetic thread. If we actually stopped the GPU process at this point with a public LLDB stop we couldn't display anything useful, so we started toying with the idea of a stop that would auto resume. That is where the eStopReasonDyld stuff was being used. We would stop, load the libraries and then auto continue so that we can debug the kernel. But as I mentioned, I don't believe we are doing this anymore, this is just background for why actions were being included in stop reply packets.

In the future, if we have an action that can be asynchronous, we might want to be able to have one of these transparent stops that can do some AcceleratorActions and auto continue.

Which means we could get duplicates and you seem to handle that, but are you testing that behaviour too? (I didn't look too closely)

Yes, this is where the map of AccleratorAction ID for each plug-in comes into play. We don't test this but we had run into issues when we had AccleratorActions in stop reply packets. We can test this if we need to still handle stop reply packets that contain actions, but not if we don't.

Another thing I noticed was that https://lldb.llvm.org/resources/lldbgdbremote.html#jacceleratorpluginbreakpointhit does not say what to do if the client cannot find one of the symbol's requested plugins. Should it include the key with no value, or a null value, or just leave out the key? Not a big deal but good to mention it and double check that the server side code is handling that situation.

We should document this for sure. It is ok if a symbol isn't available and we need the clients to know if there was a value or not. I would say we should send a null for the value if LLDB wasn't able to find the symbol or if it couldn't convert the symbol value to a load address.

(this area is on my mind since I recently worked on https://lldb.llvm.org/resources/lldbgdbremote.html#qsymbol which is a very similar idea, but triggered by library load rather than a breakpoint)

[clayborg]

Also I think now would be a good time to document the expected format of the breakpoints to be set in the native process. (in https://lldb.llvm.org/resources/lldbgdbremote.html#jacceleratorplugininitialize)

Agreed

[DavidSpickett]

Thanks for explaining. If I understood correctly, you don't strictly need the code here that checks for duplicate actions, unless you bring back the stop events later.

For now, maybe keep the check and emit a log message. In case of mistakes on the server side.

[satyajanga]

It would help my understanding a lot if you included an overview of the packet sequence / event sequence with these PRs. Each PR would not be the whole sequence of course but you could point out what part the PR is making changes to.

One thing I wasn't clear on was all the points where actions could be received. It seems those points are:

• The intialise packet.
• In response to telling the server we hit a breakpoint that a plugin asked for.
• In stop reply packets, but I'm not sure what this looks like in practice.

Which means we could get duplicates and you seem to handle that, but are you testing that behaviour too? (I didn't look too closely)

Another thing I noticed was that https://lldb.llvm.org/resources/lldbgdbremote.html#jacceleratorpluginbreakpointhit does not say what to do if the client cannot find one of the symbol's requested plugins. Should it include the key with no value, or a null value, or just leave out the key? Not a big deal but good to mention it and double check that the server side code is handling that situation.

(this area is on my mind since I recently worked on https://lldb.llvm.org/resources/lldbgdbremote.html#qsymbol which is a very similar idea, but triggered by library load rather than a breakpoint)

updated PR summary with the packet log.
and also updated documentation for the jAcceleratorpluginbreakpointhit regarding the missing symbols. and double checked on the server side that it handles.

[clayborg]

LGTM. @DavidSpickett do you have any more things you want to see changed?

[DavidSpickett]

Looks fine to me.