Add proper error handling for GPU packets

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"
@@ -37,7 +38,6 @@
 #include "lldb/Utility/GPUGDBRemotePackets.h"
 #include "lldb/Utility/StringExtractorGDBRemote.h"
 
-
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/StringSwitch.h"
 #include "llvm/Config/llvm-config.h" // for LLVM_ENABLE_ZLIB
@@ -367,7 +367,7 @@ void GDBRemoteCommunicationClient::GetRemoteQSupported() {
   m_x_packet_state.reset();
   m_supports_reverse_continue = eLazyBoolNo;
   m_supports_reverse_step = eLazyBoolNo;
-  m_supports_gpu_plugins =  eLazyBoolNo;
+  m_supports_gpu_plugins = eLazyBoolNo;
   m_max_packet_size = UINT64_MAX; // It's supposed to always be there, but if
                                   // not, we assume no limit
 
@@ -430,7 +430,7 @@ void GDBRemoteCommunicationClient::GetRemoteQSupported() {
         m_supports_reverse_continue = eLazyBoolYes;
       else if (x == "ReverseStep+")
         m_supports_reverse_step = eLazyBoolYes;
-      
+
       // Look for a list of compressions in the features list e.g.
       // qXfer:features:read+;PacketSize=20000;qEcho+;SupportedCompressions=zlib-
       // deflate,lzma
@@ -606,14 +606,14 @@ StructuredData::ObjectSP GDBRemoteCommunicationClient::GetThreadsInfo() {
   return object_sp;
 }
 
-std::optional<std::vector<GPUActions>> 
+std::optional<std::vector<GPUActions>>
 GDBRemoteCommunicationClient::GetGPUInitializeActions() {
   // Get JSON information containing any breakpoints and other information
   // required by any GPU plug-ins using the "jGPUPluginInitialize" packet.
   //
   // GPU plug-ins might require breakpoints to be set in the native program
   // that controls the GPUs. This packet allows the GPU plug-ins to set one or
-  // more breakpoints in the native program and get a callback when those 
+  // more breakpoints in the native program and get a callback when those
   // breakpoints get hit.
 
   if (m_supports_gpu_plugins == eLazyBoolYes) {
@@ -623,20 +623,31 @@ GDBRemoteCommunicationClient::GetGPUInitializeActions() {
         PacketResult::Success) {
       if (response.IsUnsupportedResponse()) {
         m_supports_gpu_plugins = eLazyBoolNo;
-      } else if (!response.Empty()) {
-        if (llvm::Expected<std::vector<GPUActions>> info = 
-                llvm::json::parse<std::vector<GPUActions>>(response.Peek(), 
-                                                            "GPUActions"))
-          return std::move(*info);
-        else
-          llvm::consumeError(info.takeError());
+        return std::nullopt;
+      }
+      if (response.IsErrorResponse()) {
+        Debugger::ReportError(response.GetStatus().AsCString());
+        return std::nullopt;
+      }
+      if (llvm::Expected<std::vector<GPUActions>> info =
+              llvm::json::parse<std::vector<GPUActions>>(response.Peek(),
+                                                         "GPUActions")) {
+        return std::move(*info);
+      } else {
+        // We don't show JSON parsing errors to the user because they won't
+        // make sense to them.
+        llvm::consumeError(info.takeError());
+        Debugger::ReportError(
+            llvm::formatv("malformed jGPUPluginInitialize response packet. {0}",
+                          response.GetStringRef()));
       }
     }
   }
+
   return std::nullopt;
 }
 
-std::optional<GPUPluginBreakpointHitResponse> 
+std::optional<GPUPluginBreakpointHitResponse>
 GDBRemoteCommunicationClient::GPUBreakpointHit(
     const GPUPluginBreakpointHitArgs &args) {
   StreamGDBRemote packet;
@@ -645,20 +656,28 @@ GDBRemoteCommunicationClient::GPUBreakpointHit(
   StringExtractorGDBRemote response;
   if (SendPacketAndWaitForResponse(packet.GetString(), response) ==
       PacketResult::Success) {
-    if (!response.Empty()) {
-      if (llvm::Expected<GPUPluginBreakpointHitResponse> info = 
-          llvm::json::parse<GPUPluginBreakpointHitResponse>(response.Peek(), 
-              "GPUPluginBreakpointHitResponse")) {
-        return *info;
-      } else {
-        llvm::consumeError(info.takeError());
-      }
+    if (response.IsErrorResponse()) {
+      Debugger::ReportError(response.GetStatus().AsCString());
+      return std::nullopt;
+    }
+    if (llvm::Expected<GPUPluginBreakpointHitResponse> info =
+            llvm::json::parse<GPUPluginBreakpointHitResponse>(
+                response.Peek(), "GPUPluginBreakpointHitResponse")) {
+      return *info;
+    } else {
+      // We don't show JSON parsing errors to the user because they won't
+      // make sense to them.
+      llvm::consumeError(info.takeError());
+      Debugger::ReportError(llvm::formatv("malformed jGPUPluginBreakpointHit "
+                                          "response packet. {0}",
+                                          response.GetStringRef()));
     }
   }
+
   return std::nullopt;
 }
 
-std::optional<GPUDynamicLoaderResponse> 
+std::optional<GPUDynamicLoaderResponse>
 GDBRemoteCommunicationClient::GetGPUDynamicLoaderLibraryInfos(
     const GPUDynamicLoaderArgs &args) {
   StreamGDBRemote packet;
@@ -667,16 +686,26 @@ GDBRemoteCommunicationClient::GetGPUDynamicLoaderLibraryInfos(
   StringExtractorGDBRemote response;
   if (SendPacketAndWaitForResponse(packet.GetString(), response) ==
       PacketResult::Success) {
-    if (!response.Empty()) {
-      if (llvm::Expected<GPUDynamicLoaderResponse> info = 
-          llvm::json::parse<GPUDynamicLoaderResponse>(response.Peek(), 
-              "GPUDynamicLoaderResponse")) {
-        return *info;
-      } else {
-        llvm::consumeError(info.takeError());
-      }
+    if (response.IsErrorResponse()) {
+      Debugger::ReportError(response.GetStatus().AsCString());
+      return std::nullopt;
+    }
+
+    if (llvm::Expected<GPUDynamicLoaderResponse> info =
+            llvm::json::parse<GPUDynamicLoaderResponse>(
+                response.Peek(), "GPUDynamicLoaderResponse")) {
+      return *info;
+    } else {
+      // We don't show JSON parsing errors to the user because they won't
+      // make sense to them.
+      llvm::consumeError(info.takeError());
+      Debugger::ReportError(
+          llvm::formatv("malformed jGPUPluginGetDynamicLoaderLibraryInfo "
+                        "response packet. {0}",
+                        response.GetStringRef()));
     }
   }
+
   return std::nullopt;
 }
 

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp

@@ -3718,10 +3718,13 @@ GDBRemoteCommunicationServerLLGS::Handle_jGPUPluginBreakpointHit(
 
   for (auto &plugin_up: m_plugins) {
     if (plugin_up->GetPluginName() == args->plugin_name) {
-      GPUPluginBreakpointHitResponse bp_response = 
+      Expected<GPUPluginBreakpointHitResponse> bp_response = 
           plugin_up->BreakpointWasHit(*args);
+      if (!bp_response)
+        return SendErrorResponse(bp_response.takeError());
+
       StreamGDBRemote response;
-      response.PutAsJSON(bp_response, /*hex_ascii=*/false);
+      response.PutAsJSON(*bp_response, /*hex_ascii=*/false);
       return SendPacketNoLock(response.GetString());
     }
   }

lldb/source/Plugins/Process/gdb-remote/LLDBServerPlugin.h

@@ -16,16 +16,16 @@
 #include "llvm/Support/JSON.h"
 
 #include <functional>
+#include <mutex>
 #include <optional>
 #include <stdint.h>
 #include <string>
-#include <mutex>
 
 namespace lldb_private {
 
 namespace process_gdb_remote {
-  class GDBRemoteCommunicationServerLLGS;
-}
+class GDBRemoteCommunicationServerLLGS;
+} // namespace process_gdb_remote
 
 namespace lldb_server {
 
@@ -125,11 +125,11 @@ class LLDBServerPlugin {
   /// calling m_process.SetBreakpoint(...) to help implement funcionality,
   /// such as dynamic library loading in GPUs or to synchronize in any other
   /// way with the native process.
-  virtual GPUPluginBreakpointHitResponse
+  virtual llvm::Expected<GPUPluginBreakpointHitResponse>
   BreakpointWasHit(GPUPluginBreakpointHitArgs &args) = 0;
 
-  protected:
-    std::mutex m_connect_mutex;
+protected:
+  std::mutex m_connect_mutex;
 };
 
 } // namespace lldb_server

lldb/tools/lldb-server/Plugins/MockGPU/LLDBServerPluginMockGPU.cpp

@@ -181,7 +181,7 @@ std::optional<GPUActions> LLDBServerPluginMockGPU::NativeProcessIsStopping() {
   return std::nullopt;
 }
 
-GPUPluginBreakpointHitResponse 
+llvm::Expected<GPUPluginBreakpointHitResponse>
 LLDBServerPluginMockGPU::BreakpointWasHit(GPUPluginBreakpointHitArgs &args) {
   Log *log = GetLog(GDBRLog::Plugin);
   std::string json_string;

lldb/tools/lldb-server/Plugins/MockGPU/LLDBServerPluginMockGPU.h

@@ -49,13 +49,13 @@ int main(int argc, const char **argv) {
 */
 // If the above code is run, you will be stopped at the breakpoint and the Mock
 // GPU target will be selected. Try doing a "reg read --all" to see the state
-// of the GPU registers. Then you can select the native process target with 
+// of the GPU registers. Then you can select the native process target with
 // "target select 0" and issue commands to the native process, and then select
 // the GPU target with "target select 1" and issue commands to the GPU target.
 
 namespace lldb_private {
-  
-  class TCPSocket;
+
+class TCPSocket;
 
 namespace lldb_server {
 
@@ -67,8 +67,8 @@ class LLDBServerPluginMockGPU : public LLDBServerPlugin {
   int GetEventFileDescriptorAtIndex(size_t idx) override;
   bool HandleEventFileDescriptorEvent(int fd) override;
   GPUActions GetInitializeActions() override;
-  std::optional<struct GPUActions> NativeProcessIsStopping() override;  
-  GPUPluginBreakpointHitResponse 
+  std::optional<struct GPUActions> NativeProcessIsStopping() override;
+  llvm::Expected<GPUPluginBreakpointHitResponse>
   BreakpointWasHit(GPUPluginBreakpointHitArgs &args) override;
 
 private: