Skip to content

Commit

Permalink
[lldb/gdb-server] Better reporting of launch errors
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@labath
labath committed Oct 6, 2022
1 parent 5614438 commit 8d1de7b
Show file tree
Hide file tree
Showing 5 changed files with 118 additions and 145 deletions.
50 changes: 24 additions & 26 deletions lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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;
}
Expand Down
149 changes: 55 additions & 94 deletions lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
18 changes: 4 additions & 14 deletions lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand All @@ -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
Expand Down
21 changes: 11 additions & 10 deletions lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand Down Expand Up @@ -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());
}
}

Expand Down
25 changes: 24 additions & 1 deletion lldb/test/API/functionalities/gdb_remote_client/TestGDBRemoteClient.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)"""
Expand Down

0 comments on commit 8d1de7b

Please sign in to comment.