From 0b720b84d1dd87a5c2089636c51abd4e6b5d5d47 Mon Sep 17 00:00:00 2001 From: Walter Erquinigo Date: Thu, 5 Jun 2025 16:59:49 +0000 Subject: [PATCH] [To upstream] Add proper error handling for GPU packets --- .../GDBRemoteCommunicationClient.cpp | 89 ++++++++++++------- .../GDBRemoteCommunicationServerLLGS.cpp | 7 +- .../Process/gdb-remote/LLDBServerPlugin.h | 12 +-- .../MockGPU/LLDBServerPluginMockGPU.cpp | 2 +- .../Plugins/MockGPU/LLDBServerPluginMockGPU.h | 10 +-- 5 files changed, 76 insertions(+), 44 deletions(-) diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp index f24817df688ac..fd7154715e5d3 100644 --- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp +++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp @@ -15,6 +15,7 @@ #include #include +#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::optional> 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> info = - llvm::json::parse>(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> info = + llvm::json::parse>(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 +std::optional 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 info = - llvm::json::parse(response.Peek(), - "GPUPluginBreakpointHitResponse")) { - return *info; - } else { - llvm::consumeError(info.takeError()); - } + if (response.IsErrorResponse()) { + Debugger::ReportError(response.GetStatus().AsCString()); + return std::nullopt; + } + if (llvm::Expected info = + llvm::json::parse( + 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 +std::optional 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 info = - llvm::json::parse(response.Peek(), - "GPUDynamicLoaderResponse")) { - return *info; - } else { - llvm::consumeError(info.takeError()); - } + if (response.IsErrorResponse()) { + Debugger::ReportError(response.GetStatus().AsCString()); + return std::nullopt; + } + + if (llvm::Expected info = + llvm::json::parse( + 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; } diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp index b0e0e14f2a4b2..0d543175ce49d 100644 --- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp +++ b/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 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()); } } diff --git a/lldb/source/Plugins/Process/gdb-remote/LLDBServerPlugin.h b/lldb/source/Plugins/Process/gdb-remote/LLDBServerPlugin.h index 671529c08cbe6..82f449c5c088c 100644 --- a/lldb/source/Plugins/Process/gdb-remote/LLDBServerPlugin.h +++ b/lldb/source/Plugins/Process/gdb-remote/LLDBServerPlugin.h @@ -16,16 +16,16 @@ #include "llvm/Support/JSON.h" #include +#include #include #include #include -#include 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 BreakpointWasHit(GPUPluginBreakpointHitArgs &args) = 0; - protected: - std::mutex m_connect_mutex; +protected: + std::mutex m_connect_mutex; }; } // namespace lldb_server diff --git a/lldb/tools/lldb-server/Plugins/MockGPU/LLDBServerPluginMockGPU.cpp b/lldb/tools/lldb-server/Plugins/MockGPU/LLDBServerPluginMockGPU.cpp index 03a50f4220870..41a6ed5921ba8 100644 --- a/lldb/tools/lldb-server/Plugins/MockGPU/LLDBServerPluginMockGPU.cpp +++ b/lldb/tools/lldb-server/Plugins/MockGPU/LLDBServerPluginMockGPU.cpp @@ -181,7 +181,7 @@ std::optional LLDBServerPluginMockGPU::NativeProcessIsStopping() { return std::nullopt; } -GPUPluginBreakpointHitResponse +llvm::Expected LLDBServerPluginMockGPU::BreakpointWasHit(GPUPluginBreakpointHitArgs &args) { Log *log = GetLog(GDBRLog::Plugin); std::string json_string; diff --git a/lldb/tools/lldb-server/Plugins/MockGPU/LLDBServerPluginMockGPU.h b/lldb/tools/lldb-server/Plugins/MockGPU/LLDBServerPluginMockGPU.h index f153579b1160d..620fdcaf9e7c4 100644 --- a/lldb/tools/lldb-server/Plugins/MockGPU/LLDBServerPluginMockGPU.h +++ b/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 NativeProcessIsStopping() override; - GPUPluginBreakpointHitResponse + std::optional NativeProcessIsStopping() override; + llvm::Expected BreakpointWasHit(GPUPluginBreakpointHitArgs &args) override; private: