[Lldb-commits] [lldb] 8d1de7b - [lldb/gdb-server] Better reporting of launch errors
Pavel Labath via lldb-commits
lldb-commits at lists.llvm.org
Thu Oct 6 08:19:08 PDT 2022
Author: Pavel Labath
Date: 2022-10-06T17:18:51+02:00
New Revision: 8d1de7b34af46a089eb5433c700419ad9b2923ee
URL: https://github.com/llvm/llvm-project/commit/8d1de7b34af46a089eb5433c700419ad9b2923ee
DIFF: https://github.com/llvm/llvm-project/commit/8d1de7b34af46a089eb5433c700419ad9b2923ee.diff
LOG: [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
Added:
Modified:
lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
lldb/test/API/functionalities/gdb_remote_client/TestGDBRemoteClient.py
Removed:
################################################################################
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 : "<NULL>");
- 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<const char *> 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)"""
More information about the lldb-commits
mailing list