From 8d1de7b34af46a089eb5433c700419ad9b2923ee Mon Sep 17 00:00:00 2001 From: Pavel Labath Date: Tue, 27 Sep 2022 20:04:10 +0200 Subject: [PATCH] [lldb/gdb-server] Better reporting of launch errors Use our "rich error" facility to propagate error reported by the stub to the user. lldb-server reports rich launch errors as of D133352. To make this easier to implement, and reduce code duplication, I have moved the vRun/A/qLaunchSuccess handling into a single GDBRemoteCommunicationClient function. Differential Revision: https://reviews.llvm.org/D134754 --- .../gdb-server/PlatformRemoteGDBServer.cpp | 50 +++--- .../GDBRemoteCommunicationClient.cpp | 149 +++++++----------- .../gdb-remote/GDBRemoteCommunicationClient.h | 18 +-- .../Process/gdb-remote/ProcessGDBRemote.cpp | 21 +-- .../gdb_remote_client/TestGDBRemoteClient.py | 25 ++- 5 files changed, 118 insertions(+), 145 deletions(-) diff --git a/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp b/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp index 7f92b669641df..a20643ad21c23 100644 --- a/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp +++ b/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp @@ -29,6 +29,7 @@ #include "lldb/Utility/Status.h" #include "lldb/Utility/StreamString.h" #include "lldb/Utility/UriParser.h" +#include "llvm/Support/FormatAdapters.h" #include "Plugins/Process/Utility/GDBRemoteSignals.h" #include "Plugins/Process/gdb-remote/ProcessGDBRemote.h" @@ -361,39 +362,36 @@ Status PlatformRemoteGDBServer::LaunchProcess(ProcessLaunchInfo &launch_info) { "PlatformRemoteGDBServer::%s() set launch architecture triple to '%s'", __FUNCTION__, arch_triple ? arch_triple : ""); - int arg_packet_err; { // Scope for the scoped timeout object process_gdb_remote::GDBRemoteCommunication::ScopedTimeout timeout( *m_gdb_client_up, std::chrono::seconds(5)); - arg_packet_err = m_gdb_client_up->SendArgumentsPacket(launch_info); + // Since we can't send argv0 separate from the executable path, we need to + // make sure to use the actual executable path found in the launch_info... + Args args = launch_info.GetArguments(); + if (FileSpec exe_file = launch_info.GetExecutableFile()) + args.ReplaceArgumentAtIndex(0, exe_file.GetPath(false)); + if (llvm::Error err = m_gdb_client_up->LaunchProcess(args)) { + error.SetErrorStringWithFormatv("Cannot launch '{0}': {1}", + args.GetArgumentAtIndex(0), + llvm::fmt_consume(std::move(err))); + return error; + } } - if (arg_packet_err == 0) { - std::string error_str; - if (m_gdb_client_up->GetLaunchSuccess(error_str)) { - const auto pid = m_gdb_client_up->GetCurrentProcessID(false); - if (pid != LLDB_INVALID_PROCESS_ID) { - launch_info.SetProcessID(pid); - LLDB_LOGF(log, - "PlatformRemoteGDBServer::%s() pid %" PRIu64 - " launched successfully", - __FUNCTION__, pid); - } else { - LLDB_LOGF(log, - "PlatformRemoteGDBServer::%s() launch succeeded but we " - "didn't get a valid process id back!", - __FUNCTION__); - error.SetErrorString("failed to get PID"); - } - } else { - error.SetErrorString(error_str.c_str()); - LLDB_LOGF(log, "PlatformRemoteGDBServer::%s() launch failed: %s", - __FUNCTION__, error.AsCString()); - } + const auto pid = m_gdb_client_up->GetCurrentProcessID(false); + if (pid != LLDB_INVALID_PROCESS_ID) { + launch_info.SetProcessID(pid); + LLDB_LOGF(log, + "PlatformRemoteGDBServer::%s() pid %" PRIu64 + " launched successfully", + __FUNCTION__, pid); } else { - error.SetErrorStringWithFormat("'A' packet returned an error: %i", - arg_packet_err); + LLDB_LOGF(log, + "PlatformRemoteGDBServer::%s() launch succeeded but we " + "didn't get a valid process id back!", + __FUNCTION__); + error.SetErrorString("failed to get PID"); } return error; } diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp index 7e4688030c5b2..f03fd4231c74a 100644 --- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp +++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp @@ -746,108 +746,69 @@ lldb::pid_t GDBRemoteCommunicationClient::GetCurrentProcessID(bool allow_lazy) { return LLDB_INVALID_PROCESS_ID; } -bool GDBRemoteCommunicationClient::GetLaunchSuccess(std::string &error_str) { - error_str.clear(); - StringExtractorGDBRemote response; - if (SendPacketAndWaitForResponse("qLaunchSuccess", response) == - PacketResult::Success) { - if (response.IsOKResponse()) - return true; - // GDB does not implement qLaunchSuccess -- but if we used vRun, - // then we already received a successful launch indication via stop - // reason. - if (response.IsUnsupportedResponse() && m_supports_vRun) - return true; - if (response.GetChar() == 'E') { - // A string the describes what failed when launching... - error_str = std::string(response.GetStringRef().substr(1)); - } else { - error_str.assign("unknown error occurred launching process"); +llvm::Error GDBRemoteCommunicationClient::LaunchProcess(const Args &args) { + if (!args.GetArgumentAtIndex(0)) + return llvm::createStringError(llvm::inconvertibleErrorCode(), + "Nothing to launch"); + // try vRun first + if (m_supports_vRun) { + StreamString packet; + packet.PutCString("vRun"); + for (const Args::ArgEntry &arg : args) { + packet.PutChar(';'); + packet.PutStringAsRawHex8(arg.ref()); } - } else { - error_str.assign("timed out waiting for app to launch"); - } - return false; -} -int GDBRemoteCommunicationClient::SendArgumentsPacket( - const ProcessLaunchInfo &launch_info) { - // Since we don't get the send argv0 separate from the executable path, we - // need to make sure to use the actual executable path found in the - // launch_info... - std::vector argv; - FileSpec exe_file = launch_info.GetExecutableFile(); - std::string exe_path; - const char *arg = nullptr; - const Args &launch_args = launch_info.GetArguments(); - if (exe_file) - exe_path = exe_file.GetPath(false); - else { - arg = launch_args.GetArgumentAtIndex(0); - if (arg) - exe_path = arg; - } - if (!exe_path.empty()) { - argv.push_back(exe_path.c_str()); - for (uint32_t i = 1; (arg = launch_args.GetArgumentAtIndex(i)) != nullptr; - ++i) { - if (arg) - argv.push_back(arg); - } - } - if (!argv.empty()) { - // try vRun first - if (m_supports_vRun) { - StreamString packet; - packet.PutCString("vRun"); - for (const char *arg : argv) { - packet.PutChar(';'); - packet.PutBytesAsRawHex8(arg, strlen(arg)); - } + StringExtractorGDBRemote response; + if (SendPacketAndWaitForResponse(packet.GetString(), response) != + PacketResult::Success) + return llvm::createStringError(llvm::inconvertibleErrorCode(), + "Sending vRun packet failed"); - StringExtractorGDBRemote response; - if (SendPacketAndWaitForResponse(packet.GetString(), response) != - PacketResult::Success) - return -1; + if (response.IsErrorResponse()) + return response.GetStatus().ToError(); - if (response.IsErrorResponse()) { - uint8_t error = response.GetError(); - if (error) - return error; - return -1; - } - // vRun replies with a stop reason packet - // FIXME: right now we just discard the packet and LLDB queries - // for stop reason again - if (!response.IsUnsupportedResponse()) - return 0; + // vRun replies with a stop reason packet + // FIXME: right now we just discard the packet and LLDB queries + // for stop reason again + if (!response.IsUnsupportedResponse()) + return llvm::Error::success(); - m_supports_vRun = false; - } + m_supports_vRun = false; + } - // fallback to A - StreamString packet; - packet.PutChar('A'); - for (size_t i = 0, n = argv.size(); i < n; ++i) { - arg = argv[i]; - const int arg_len = strlen(arg); - if (i > 0) - packet.PutChar(','); - packet.Printf("%i,%i,", arg_len * 2, (int)i); - packet.PutBytesAsRawHex8(arg, arg_len); - } + // fallback to A + StreamString packet; + packet.PutChar('A'); + llvm::ListSeparator LS(","); + for (const auto &arg : llvm::enumerate(args)) { + packet << LS; + packet.Format("{0},{1},", arg.value().ref().size() * 2, arg.index()); + packet.PutStringAsRawHex8(arg.value().ref()); + } - StringExtractorGDBRemote response; - if (SendPacketAndWaitForResponse(packet.GetString(), response) == - PacketResult::Success) { - if (response.IsOKResponse()) - return 0; - uint8_t error = response.GetError(); - if (error) - return error; - } + StringExtractorGDBRemote response; + if (SendPacketAndWaitForResponse(packet.GetString(), response) != + PacketResult::Success) { + return llvm::createStringError(llvm::inconvertibleErrorCode(), + "Sending A packet failed"); } - return -1; + if (!response.IsOKResponse()) + return response.GetStatus().ToError(); + + if (SendPacketAndWaitForResponse("qLaunchSuccess", response) != + PacketResult::Success) { + return llvm::createStringError(llvm::inconvertibleErrorCode(), + "Sending qLaunchSuccess packet failed"); + } + if (response.IsOKResponse()) + return llvm::Error::success(); + if (response.GetChar() == 'E') { + return llvm::createStringError(llvm::inconvertibleErrorCode(), + response.GetStringRef().substr(1)); + } + return llvm::createStringError(llvm::inconvertibleErrorCode(), + "unknown error occurred launching process"); } int GDBRemoteCommunicationClient::SendEnvironment(const Environment &env) { diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h index da060cd09dc9f..c5911b9f0784f 100644 --- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h +++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h @@ -80,8 +80,6 @@ class GDBRemoteCommunicationClient : public GDBRemoteClientBase { lldb::pid_t GetCurrentProcessID(bool allow_lazy = true); - bool GetLaunchSuccess(std::string &error_str); - bool LaunchGDBServer(const char *remote_accept_hostname, lldb::pid_t &pid, uint16_t &port, std::string &socket_name); @@ -90,19 +88,11 @@ class GDBRemoteCommunicationClient : public GDBRemoteClientBase { bool KillSpawnedProcess(lldb::pid_t pid); - /// Sends a GDB remote protocol 'A' packet that delivers program - /// arguments to the remote server. - /// - /// \param[in] launch_info - /// A NULL terminated array of const C strings to use as the - /// arguments. + /// Launch the process using the provided arguments. /// - /// \return - /// Zero if the response was "OK", a positive value if the - /// the response was "Exx" where xx are two hex digits, or - /// -1 if the call is unsupported or any other unexpected - /// response was received. - int SendArgumentsPacket(const ProcessLaunchInfo &launch_info); + /// \param[in] args + /// A list of program arguments. The first entry is the program being run. + llvm::Error LaunchProcess(const Args &args); /// Sends a "QEnvironment:NAME=VALUE" packet that will build up the /// environment that will get used when launching an application diff --git a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp index 09b4436e8fa00..ecd9606106ba6 100644 --- a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp +++ b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp @@ -84,6 +84,7 @@ #include "llvm/ADT/ScopeExit.h" #include "llvm/ADT/StringSwitch.h" +#include "llvm/Support/FormatAdapters.h" #include "llvm/Support/Threading.h" #include "llvm/Support/raw_ostream.h" @@ -799,17 +800,17 @@ Status ProcessGDBRemote::DoLaunch(lldb_private::Module *exe_module, GDBRemoteCommunication::ScopedTimeout timeout(m_gdb_comm, std::chrono::seconds(10)); - int arg_packet_err = m_gdb_comm.SendArgumentsPacket(launch_info); - if (arg_packet_err == 0) { - std::string error_str; - if (m_gdb_comm.GetLaunchSuccess(error_str)) { - SetID(m_gdb_comm.GetCurrentProcessID()); - } else { - error.SetErrorString(error_str.c_str()); - } + // Since we can't send argv0 separate from the executable path, we need to + // make sure to use the actual executable path found in the launch_info... + Args args = launch_info.GetArguments(); + if (FileSpec exe_file = launch_info.GetExecutableFile()) + args.ReplaceArgumentAtIndex(0, exe_file.GetPath(false)); + if (llvm::Error err = m_gdb_comm.LaunchProcess(args)) { + error.SetErrorStringWithFormatv("Cannot launch '{0}': {1}", + args.GetArgumentAtIndex(0), + llvm::fmt_consume(std::move(err))); } else { - error.SetErrorStringWithFormat("'A' packet returned an error: %i", - arg_packet_err); + SetID(m_gdb_comm.GetCurrentProcessID()); } } diff --git a/lldb/test/API/functionalities/gdb_remote_client/TestGDBRemoteClient.py b/lldb/test/API/functionalities/gdb_remote_client/TestGDBRemoteClient.py index e99192a6a670f..cde08f855df2a 100644 --- a/lldb/test/API/functionalities/gdb_remote_client/TestGDBRemoteClient.py +++ b/lldb/test/API/functionalities/gdb_remote_client/TestGDBRemoteClient.py @@ -86,7 +86,30 @@ def A(self, packet): error = lldb.SBError() target.Launch(lldb.SBListener(), None, None, None, None, None, None, 0, True, error) - self.assertEquals("'A' packet returned an error: 71", error.GetCString()) + self.assertRegex(error.GetCString(), "Cannot launch '.*a': Error 71") + + def test_launch_rich_error(self): + class MyResponder(MockGDBServerResponder): + def qC(self): + return "E42" + + def qfThreadInfo(self): + return "OK" # No threads. + + # Then, when we are asked to attach, error out. + def vRun(self, packet): + return "Eff;" + seven.hexlify("I'm a teapot") + + self.server.responder = MyResponder() + + target = self.createTarget("a.yaml") + process = self.connect(target) + lldbutil.expect_state_changes(self, self.dbg.GetListener(), process, [lldb.eStateConnected]) + + error = lldb.SBError() + target.Launch(lldb.SBListener(), None, None, None, None, None, + None, 0, True, error) + self.assertRegex(error.GetCString(), "Cannot launch '.*a': I'm a teapot") def test_read_registers_using_g_packets(self): """Test reading registers using 'g' packets (default behavior)"""