[Lldb-commits] [lldb] [lldb] Clean up GDBRemoteCommunication::StartDebugserverProcess (PR #145021)
Pavel Labath via lldb-commits
lldb-commits at lists.llvm.org
Fri Jun 20 04:22:07 PDT 2025
https://github.com/labath created https://github.com/llvm/llvm-project/pull/145021
The function was extremely messy in that it, depending on the set of
arguments, it could either modify the Connection object in `this` or
not. It had a lot of arguments, with each call site passing a different
combination of null values. This PR:
- packs "url" and "comm_fd" arguments into a variant as they are
mutually exclusive
- removes the (surprising) "null url *and* null comm_fd" code path which is not used as of https://github.com/llvm/llvm-project/pull/145017
- marks the function as `static` to make it clear it (now) does not
operate on the `this` object.
Depends on #145017
(This PR consists of three commits, the first two of which are equivalent to https://github.com/llvm/llvm-project/pull/145015 and #145017, respectively. For reviewing, I recommend only looking at the last commit. If you have comments on the first two, please put them on their respective PRs.)
>From 54ef8f5e1ff3f3ea28605ffb9a90f0b0aa6b52af Mon Sep 17 00:00:00 2001
From: Pavel Labath <pavel at labath.sk>
Date: Thu, 19 Jun 2025 21:44:02 +0200
Subject: [PATCH 1/3] [lldb] Add Socket::CreatePair
It creates a pair of connected sockets using the simplest mechanism for
the given platform (TCP on windows, socketpair(2) elsewhere).
Main motivation is to remove the ugly platform-specific code in
ProcessGDBRemote::LaunchAndConnectToDebugserver, but it can also be used
in other places where we need to create a pair of connected sockets.
---
lldb/include/lldb/Host/Socket.h | 4 +++
lldb/include/lldb/Host/common/TCPSocket.h | 4 +++
lldb/include/lldb/Host/posix/DomainSocket.h | 4 +++
lldb/source/Host/common/Socket.cpp | 17 +++++++++
lldb/source/Host/common/TCPSocket.cpp | 28 +++++++++++++++
lldb/source/Host/posix/DomainSocket.cpp | 29 +++++++++++++++
.../gdb-remote/GDBRemoteCommunication.cpp | 36 +++++--------------
lldb/unittests/Core/CommunicationTest.cpp | 26 ++++++++------
lldb/unittests/Host/SocketTest.cpp | 35 ++++++++++++++++++
9 files changed, 144 insertions(+), 39 deletions(-)
diff --git a/lldb/include/lldb/Host/Socket.h b/lldb/include/lldb/Host/Socket.h
index c313aa4f6d26b..14a9660ed30b7 100644
--- a/lldb/include/lldb/Host/Socket.h
+++ b/lldb/include/lldb/Host/Socket.h
@@ -106,6 +106,10 @@ class Socket : public IOObject {
static std::unique_ptr<Socket> Create(const SocketProtocol protocol,
Status &error);
+ static llvm::Expected<
+ std::pair<std::unique_ptr<Socket>, std::unique_ptr<Socket>>>
+ CreatePair(std::optional<SocketProtocol> protocol = std::nullopt);
+
virtual Status Connect(llvm::StringRef name) = 0;
virtual Status Listen(llvm::StringRef name, int backlog) = 0;
diff --git a/lldb/include/lldb/Host/common/TCPSocket.h b/lldb/include/lldb/Host/common/TCPSocket.h
index cb950c0015ea6..e81cb82dbcba1 100644
--- a/lldb/include/lldb/Host/common/TCPSocket.h
+++ b/lldb/include/lldb/Host/common/TCPSocket.h
@@ -23,6 +23,10 @@ class TCPSocket : public Socket {
TCPSocket(NativeSocket socket, bool should_close);
~TCPSocket() override;
+ static llvm::Expected<
+ std::pair<std::unique_ptr<TCPSocket>, std::unique_ptr<TCPSocket>>>
+ CreatePair();
+
// returns port number or 0 if error
uint16_t GetLocalPortNumber() const;
diff --git a/lldb/include/lldb/Host/posix/DomainSocket.h b/lldb/include/lldb/Host/posix/DomainSocket.h
index a840d474429ec..c3a6a64bbdef8 100644
--- a/lldb/include/lldb/Host/posix/DomainSocket.h
+++ b/lldb/include/lldb/Host/posix/DomainSocket.h
@@ -19,6 +19,10 @@ class DomainSocket : public Socket {
DomainSocket(NativeSocket socket, bool should_close);
explicit DomainSocket(bool should_close);
+ static llvm::Expected<
+ std::pair<std::unique_ptr<DomainSocket>, std::unique_ptr<DomainSocket>>>
+ CreatePair();
+
Status Connect(llvm::StringRef name) override;
Status Listen(llvm::StringRef name, int backlog) override;
diff --git a/lldb/source/Host/common/Socket.cpp b/lldb/source/Host/common/Socket.cpp
index 5c5cd653c3d9e..c9dec0f8ea22a 100644
--- a/lldb/source/Host/common/Socket.cpp
+++ b/lldb/source/Host/common/Socket.cpp
@@ -234,6 +234,23 @@ std::unique_ptr<Socket> Socket::Create(const SocketProtocol protocol,
return socket_up;
}
+llvm::Expected<std::pair<std::unique_ptr<Socket>, std::unique_ptr<Socket>>>
+Socket::CreatePair(std::optional<SocketProtocol> protocol) {
+ constexpr SocketProtocol kBestProtocol =
+ LLDB_ENABLE_POSIX ? ProtocolUnixDomain : ProtocolTcp;
+ switch (protocol.value_or(kBestProtocol)) {
+ case ProtocolTcp:
+ return TCPSocket::CreatePair();
+#if LLDB_ENABLE_POSIX
+ case ProtocolUnixDomain:
+ case ProtocolUnixAbstract:
+ return DomainSocket::CreatePair();
+#endif
+ default:
+ return llvm::createStringError("Unsupported protocol");
+ }
+}
+
llvm::Expected<std::unique_ptr<Socket>>
Socket::TcpConnect(llvm::StringRef host_and_port) {
Log *log = GetLog(LLDBLog::Connection);
diff --git a/lldb/source/Host/common/TCPSocket.cpp b/lldb/source/Host/common/TCPSocket.cpp
index 3d0dea1c61dd6..34f249746149e 100644
--- a/lldb/source/Host/common/TCPSocket.cpp
+++ b/lldb/source/Host/common/TCPSocket.cpp
@@ -52,6 +52,34 @@ TCPSocket::TCPSocket(NativeSocket socket, bool should_close)
TCPSocket::~TCPSocket() { CloseListenSockets(); }
+llvm::Expected<
+ std::pair<std::unique_ptr<TCPSocket>, std::unique_ptr<TCPSocket>>>
+TCPSocket::CreatePair() {
+ auto listen_socket_up = std::make_unique<TCPSocket>(true);
+ if (Status error = listen_socket_up->Listen("localhost:0", 5); error.Fail())
+ return error.takeError();
+
+ std::string connect_address =
+ llvm::StringRef(listen_socket_up->GetListeningConnectionURI()[0])
+ .split("://")
+ .second.str();
+
+ auto connect_socket_up = std::make_unique<TCPSocket>(true);
+ if (Status error = connect_socket_up->Connect(connect_address); error.Fail())
+ return error.takeError();
+
+ // Connection has already been made above, so a short timeout is sufficient.
+ Socket *accept_socket;
+ if (Status error =
+ listen_socket_up->Accept(std::chrono::seconds(1), accept_socket);
+ error.Fail())
+ return error.takeError();
+
+ return std::make_pair(
+ std::move(connect_socket_up),
+ std::unique_ptr<TCPSocket>(static_cast<TCPSocket *>(accept_socket)));
+}
+
bool TCPSocket::IsValid() const {
return m_socket != kInvalidSocketValue || m_listen_sockets.size() != 0;
}
diff --git a/lldb/source/Host/posix/DomainSocket.cpp b/lldb/source/Host/posix/DomainSocket.cpp
index 4f76e0c16d4c7..0202dea45a8e1 100644
--- a/lldb/source/Host/posix/DomainSocket.cpp
+++ b/lldb/source/Host/posix/DomainSocket.cpp
@@ -13,9 +13,11 @@
#endif
#include "llvm/Support/Errno.h"
+#include "llvm/Support/Error.h"
#include "llvm/Support/FileSystem.h"
#include <cstddef>
+#include <fcntl.h>
#include <memory>
#include <sys/socket.h>
#include <sys/un.h>
@@ -76,6 +78,33 @@ DomainSocket::DomainSocket(SocketProtocol protocol, NativeSocket socket,
m_socket = socket;
}
+llvm::Expected<
+ std::pair<std::unique_ptr<DomainSocket>, std::unique_ptr<DomainSocket>>>
+DomainSocket::CreatePair() {
+ int sockets[2];
+ int type = SOCK_STREAM;
+#ifdef SOCK_CLOEXEC
+ type |= SOCK_CLOEXEC;
+#endif
+ if (socketpair(AF_UNIX, type, 0, sockets) == -1)
+ return llvm::errorCodeToError(llvm::errnoAsErrorCode());
+
+#ifndef SOCK_CLOEXEC
+ for (int s : sockets) {
+ int r = fcntl(s, F_SETFD, FD_CLOEXEC | fcntl(s, F_GETFD));
+ assert(r == 0);
+ (void)r;
+ }
+#endif
+
+ return std::make_pair(std::unique_ptr<DomainSocket>(
+ new DomainSocket(ProtocolUnixDomain, sockets[0],
+ /*should_close=*/true)),
+ std::unique_ptr<DomainSocket>(
+ new DomainSocket(ProtocolUnixDomain, sockets[1],
+ /*should_close=*/true)));
+}
+
Status DomainSocket::Connect(llvm::StringRef name) {
sockaddr_un saddr_un;
socklen_t saddr_un_len;
diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
index 2aea7c6b781d7..590d78e98f9a0 100644
--- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
@@ -1141,34 +1141,14 @@ void GDBRemoteCommunication::DumpHistory(Stream &strm) { m_history.Dump(strm); }
llvm::Error
GDBRemoteCommunication::ConnectLocally(GDBRemoteCommunication &client,
GDBRemoteCommunication &server) {
- const int backlog = 5;
- TCPSocket listen_socket(true);
- if (llvm::Error error =
- listen_socket.Listen("localhost:0", backlog).ToError())
- return error;
-
- llvm::SmallString<32> remote_addr;
- llvm::raw_svector_ostream(remote_addr)
- << "connect://localhost:" << listen_socket.GetLocalPortNumber();
-
- std::unique_ptr<ConnectionFileDescriptor> conn_up(
- new ConnectionFileDescriptor());
- Status status;
- if (conn_up->Connect(remote_addr, &status) != lldb::eConnectionStatusSuccess)
- return llvm::createStringError(llvm::inconvertibleErrorCode(),
- "Unable to connect: %s", status.AsCString());
-
- // The connection was already established above, so a short timeout is
- // sufficient.
- Socket *accept_socket = nullptr;
- if (Status accept_status =
- listen_socket.Accept(std::chrono::seconds(1), accept_socket);
- accept_status.Fail())
- return accept_status.takeError();
-
- client.SetConnection(std::move(conn_up));
- server.SetConnection(
- std::make_unique<ConnectionFileDescriptor>(accept_socket));
+ auto expected_socket_pair = Socket::CreatePair();
+ if (!expected_socket_pair)
+ return expected_socket_pair.takeError();
+
+ client.SetConnection(std::make_unique<ConnectionFileDescriptor>(
+ expected_socket_pair->first.release()));
+ server.SetConnection(std::make_unique<ConnectionFileDescriptor>(
+ expected_socket_pair->second.release()));
return llvm::Error::success();
}
diff --git a/lldb/unittests/Core/CommunicationTest.cpp b/lldb/unittests/Core/CommunicationTest.cpp
index df9ff089a0d73..369d1534ae32a 100644
--- a/lldb/unittests/Core/CommunicationTest.cpp
+++ b/lldb/unittests/Core/CommunicationTest.cpp
@@ -7,14 +7,14 @@
//===----------------------------------------------------------------------===//
#include "lldb/Core/Communication.h"
+#include "TestingSupport/SubsystemRAII.h"
#include "lldb/Core/ThreadedCommunication.h"
#include "lldb/Host/Config.h"
#include "lldb/Host/ConnectionFileDescriptor.h"
#include "lldb/Host/Pipe.h"
+#include "lldb/Host/Socket.h"
#include "llvm/Testing/Support/Error.h"
#include "gtest/gtest.h"
-#include "TestingSupport/Host/SocketTestUtilities.h"
-#include "TestingSupport/SubsystemRAII.h"
#include <chrono>
#include <thread>
@@ -31,15 +31,17 @@ class CommunicationTest : public testing::Test {
};
static void CommunicationReadTest(bool use_read_thread) {
- std::unique_ptr<TCPSocket> a, b;
- ASSERT_TRUE(CreateTCPConnectedSockets("localhost", &a, &b));
+ auto maybe_socket_pair = Socket::CreatePair();
+ ASSERT_THAT_EXPECTED(maybe_socket_pair, llvm::Succeeded());
+ Socket &a = *maybe_socket_pair->first;
size_t num_bytes = 4;
- ASSERT_THAT_ERROR(a->Write("test", num_bytes).ToError(), llvm::Succeeded());
+ ASSERT_THAT_ERROR(a.Write("test", num_bytes).ToError(), llvm::Succeeded());
ASSERT_EQ(num_bytes, 4U);
ThreadedCommunication comm("test");
- comm.SetConnection(std::make_unique<ConnectionFileDescriptor>(b.release()));
+ comm.SetConnection(std::make_unique<ConnectionFileDescriptor>(
+ maybe_socket_pair->second.release()));
comm.SetCloseOnEOF(true);
if (use_read_thread) {
@@ -73,7 +75,7 @@ static void CommunicationReadTest(bool use_read_thread) {
EXPECT_THAT_ERROR(error.ToError(), llvm::Failed());
// This read should return EOF.
- ASSERT_THAT_ERROR(a->Close().ToError(), llvm::Succeeded());
+ ASSERT_THAT_ERROR(a.Close().ToError(), llvm::Succeeded());
error.Clear();
EXPECT_EQ(
comm.Read(buf, sizeof(buf), std::chrono::seconds(5), status, &error), 0U);
@@ -118,17 +120,19 @@ TEST_F(CommunicationTest, ReadThread) {
}
TEST_F(CommunicationTest, SynchronizeWhileClosing) {
- std::unique_ptr<TCPSocket> a, b;
- ASSERT_TRUE(CreateTCPConnectedSockets("localhost", &a, &b));
+ auto maybe_socket_pair = Socket::CreatePair();
+ ASSERT_THAT_EXPECTED(maybe_socket_pair, llvm::Succeeded());
+ Socket &a = *maybe_socket_pair->first;
ThreadedCommunication comm("test");
- comm.SetConnection(std::make_unique<ConnectionFileDescriptor>(b.release()));
+ comm.SetConnection(std::make_unique<ConnectionFileDescriptor>(
+ maybe_socket_pair->second.release()));
comm.SetCloseOnEOF(true);
ASSERT_TRUE(comm.StartReadThread());
// Ensure that we can safely synchronize with the read thread while it is
// closing the read end (in response to us closing the write end).
- ASSERT_THAT_ERROR(a->Close().ToError(), llvm::Succeeded());
+ ASSERT_THAT_ERROR(a.Close().ToError(), llvm::Succeeded());
comm.SynchronizeWithReadThread();
ASSERT_TRUE(comm.StopReadThread());
diff --git a/lldb/unittests/Host/SocketTest.cpp b/lldb/unittests/Host/SocketTest.cpp
index 77366593f05f8..3630b63242707 100644
--- a/lldb/unittests/Host/SocketTest.cpp
+++ b/lldb/unittests/Host/SocketTest.cpp
@@ -74,6 +74,41 @@ TEST_F(SocketTest, DecodeHostAndPort) {
llvm::HasValue(Socket::HostAndPort{"abcd:12fg:AF58::1", 12345}));
}
+TEST_F(SocketTest, CreatePair) {
+ std::vector<std::optional<Socket::SocketProtocol>> functional_protocols = {
+ std::nullopt,
+ Socket::ProtocolTcp,
+#if LLDB_ENABLE_POSIX
+ Socket::ProtocolUnixDomain,
+ Socket::ProtocolUnixAbstract,
+#endif
+ };
+ for (auto p : functional_protocols) {
+ auto expected_socket_pair = Socket::CreatePair(p);
+ ASSERT_THAT_EXPECTED(expected_socket_pair, llvm::Succeeded());
+ Socket &a = *expected_socket_pair->first;
+ Socket &b = *expected_socket_pair->second;
+ size_t num_bytes = 1;
+ ASSERT_THAT_ERROR(a.Write("a", num_bytes).takeError(), llvm::Succeeded());
+ ASSERT_EQ(num_bytes, 1);
+ char c;
+ ASSERT_THAT_ERROR(b.Read(&c, num_bytes).takeError(), llvm::Succeeded());
+ ASSERT_EQ(num_bytes, 1);
+ ASSERT_EQ(c, 'a');
+ }
+
+ std::vector<Socket::SocketProtocol> erroring_protocols = {
+#if !LLDB_ENABLE_POSIX
+ Socket::ProtocolUnixDomain,
+ Socket::ProtocolUnixAbstract,
+#endif
+ };
+ for (auto p : erroring_protocols) {
+ ASSERT_THAT_EXPECTED(Socket::CreatePair(p),
+ llvm::FailedWithMessage("Unsupported protocol"));
+ }
+}
+
#if LLDB_ENABLE_POSIX
TEST_F(SocketTest, DomainListenConnectAccept) {
llvm::SmallString<64> Path;
>From 9b05351e330e379e054657ca8dd58777020e4dad Mon Sep 17 00:00:00 2001
From: Pavel Labath <pavel at labath.sk>
Date: Fri, 20 Jun 2025 10:29:23 +0200
Subject: [PATCH 2/3] [lldb] Use Socket::CreatePair for launching debugserver
This lets get rid of platform-specific code in ProcessGDBRemote and use the
same code path (module differences in socket types) everywhere. It also unlocks
further cleanups in the debugserver launching code.
---
.../Process/gdb-remote/ProcessGDBRemote.cpp | 145 +++++++-----------
1 file changed, 55 insertions(+), 90 deletions(-)
diff --git a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
index f18bdd5175f2e..ba806767f78da 100644
--- a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -3447,115 +3447,80 @@ ProcessGDBRemote::EstablishConnectionIfNeeded(const ProcessInfo &process_info) {
}
return error;
}
-#if !defined(_WIN32)
-#define USE_SOCKETPAIR_FOR_LOCAL_CONNECTION 1
-#endif
-
-#ifdef USE_SOCKETPAIR_FOR_LOCAL_CONNECTION
-static bool SetCloexecFlag(int fd) {
-#if defined(FD_CLOEXEC)
- int flags = ::fcntl(fd, F_GETFD);
- if (flags == -1)
- return false;
- return (::fcntl(fd, F_SETFD, flags | FD_CLOEXEC) == 0);
-#else
- return false;
-#endif
-}
-#endif
Status ProcessGDBRemote::LaunchAndConnectToDebugserver(
const ProcessInfo &process_info) {
using namespace std::placeholders; // For _1, _2, etc.
- Status error;
- if (m_debugserver_pid == LLDB_INVALID_PROCESS_ID) {
- // If we locate debugserver, keep that located version around
- static FileSpec g_debugserver_file_spec;
+ if (m_debugserver_pid != LLDB_INVALID_PROCESS_ID)
+ return Status();
- ProcessLaunchInfo debugserver_launch_info;
- // Make debugserver run in its own session so signals generated by special
- // terminal key sequences (^C) don't affect debugserver.
- debugserver_launch_info.SetLaunchInSeparateProcessGroup(true);
+ ProcessLaunchInfo debugserver_launch_info;
+ // Make debugserver run in its own session so signals generated by special
+ // terminal key sequences (^C) don't affect debugserver.
+ debugserver_launch_info.SetLaunchInSeparateProcessGroup(true);
- const std::weak_ptr<ProcessGDBRemote> this_wp =
- std::static_pointer_cast<ProcessGDBRemote>(shared_from_this());
- debugserver_launch_info.SetMonitorProcessCallback(
- std::bind(MonitorDebugserverProcess, this_wp, _1, _2, _3));
- debugserver_launch_info.SetUserID(process_info.GetUserID());
+ const std::weak_ptr<ProcessGDBRemote> this_wp =
+ std::static_pointer_cast<ProcessGDBRemote>(shared_from_this());
+ debugserver_launch_info.SetMonitorProcessCallback(
+ std::bind(MonitorDebugserverProcess, this_wp, _1, _2, _3));
+ debugserver_launch_info.SetUserID(process_info.GetUserID());
#if defined(__APPLE__)
- // On macOS 11, we need to support x86_64 applications translated to
- // arm64. We check whether a binary is translated and spawn the correct
- // debugserver accordingly.
- int mib[] = { CTL_KERN, KERN_PROC, KERN_PROC_PID,
- static_cast<int>(process_info.GetProcessID()) };
- struct kinfo_proc processInfo;
- size_t bufsize = sizeof(processInfo);
- if (sysctl(mib, (unsigned)(sizeof(mib)/sizeof(int)), &processInfo,
- &bufsize, NULL, 0) == 0 && bufsize > 0) {
- if (processInfo.kp_proc.p_flag & P_TRANSLATED) {
- FileSpec rosetta_debugserver("/Library/Apple/usr/libexec/oah/debugserver");
- debugserver_launch_info.SetExecutableFile(rosetta_debugserver, false);
- }
+ // On macOS 11, we need to support x86_64 applications translated to
+ // arm64. We check whether a binary is translated and spawn the correct
+ // debugserver accordingly.
+ int mib[] = {CTL_KERN, KERN_PROC, KERN_PROC_PID,
+ static_cast<int>(process_info.GetProcessID())};
+ struct kinfo_proc processInfo;
+ size_t bufsize = sizeof(processInfo);
+ if (sysctl(mib, (unsigned)(sizeof(mib) / sizeof(int)), &processInfo, &bufsize,
+ NULL, 0) == 0 &&
+ bufsize > 0) {
+ if (processInfo.kp_proc.p_flag & P_TRANSLATED) {
+ FileSpec rosetta_debugserver(
+ "/Library/Apple/usr/libexec/oah/debugserver");
+ debugserver_launch_info.SetExecutableFile(rosetta_debugserver, false);
}
+ }
#endif
- shared_fd_t communication_fd = SharedSocket::kInvalidFD;
-#ifdef USE_SOCKETPAIR_FOR_LOCAL_CONNECTION
- // Use a socketpair on non-Windows systems for security and performance
- // reasons.
- int sockets[2]; /* the pair of socket descriptors */
- if (socketpair(AF_UNIX, SOCK_STREAM, 0, sockets) == -1) {
- error = Status::FromErrno();
- return error;
- }
+ auto expected_socket_pair = Socket::CreatePair();
+ if (!expected_socket_pair)
+ return Status::FromError(expected_socket_pair.takeError());
- int our_socket = sockets[0];
- int gdb_socket = sockets[1];
- auto cleanup_our = llvm::make_scope_exit([&]() { close(our_socket); });
- auto cleanup_gdb = llvm::make_scope_exit([&]() { close(gdb_socket); });
+ Status error;
+ SharedSocket shared_socket(expected_socket_pair->first.get(), error);
+ if (error.Fail())
+ return error;
- // Don't let any child processes inherit our communication socket
- SetCloexecFlag(our_socket);
- communication_fd = gdb_socket;
-#endif
+ error = m_gdb_comm.StartDebugserverProcess(
+ nullptr, GetTarget().GetPlatform().get(), debugserver_launch_info,
+ nullptr, nullptr, shared_socket.GetSendableFD());
- error = m_gdb_comm.StartDebugserverProcess(
- nullptr, GetTarget().GetPlatform().get(), debugserver_launch_info,
- nullptr, nullptr, communication_fd);
+ if (error.Fail()) {
+ Log *log = GetLog(GDBRLog::Process);
- if (error.Success())
- m_debugserver_pid = debugserver_launch_info.GetProcessID();
- else
- m_debugserver_pid = LLDB_INVALID_PROCESS_ID;
-
- if (m_debugserver_pid != LLDB_INVALID_PROCESS_ID) {
-#ifdef USE_SOCKETPAIR_FOR_LOCAL_CONNECTION
- // Our process spawned correctly, we can now set our connection to use
- // our end of the socket pair
- cleanup_our.release();
- m_gdb_comm.SetConnection(
- std::make_unique<ConnectionFileDescriptor>(our_socket, true));
-#endif
- StartAsyncThread();
- }
+ LLDB_LOGF(log, "failed to start debugserver process: %s",
+ error.AsCString());
+ return error;
+ }
- if (error.Fail()) {
- Log *log = GetLog(GDBRLog::Process);
+ m_debugserver_pid = debugserver_launch_info.GetProcessID();
+ shared_socket.CompleteSending(m_debugserver_pid);
- LLDB_LOGF(log, "failed to start debugserver process: %s",
- error.AsCString());
- return error;
- }
+ // Our process spawned correctly, we can now set our connection to use
+ // our end of the socket pair
+ m_gdb_comm.SetConnection(std::make_unique<ConnectionFileDescriptor>(
+ expected_socket_pair->second.release()));
+ StartAsyncThread();
- if (m_gdb_comm.IsConnected()) {
- // Finish the connection process by doing the handshake without
- // connecting (send NULL URL)
- error = ConnectToDebugserver("");
- } else {
- error = Status::FromErrorString("connection failed");
- }
+ if (m_gdb_comm.IsConnected()) {
+ // Finish the connection process by doing the handshake without
+ // connecting (send NULL URL)
+ error = ConnectToDebugserver("");
+ } else {
+ error = Status::FromErrorString("connection failed");
}
return error;
}
>From c77e2ad2eb8ef7b87d509aed31020001ceceb64c Mon Sep 17 00:00:00 2001
From: Pavel Labath <pavel at labath.sk>
Date: Fri, 20 Jun 2025 11:20:10 +0200
Subject: [PATCH 3/3] [lldb] Clean up
GDBRemoteCommunication::StartDebugserverProcess
The function was extremely messy in that it, depending on the set of
arguments, it could either modify the Connection object in `this` or
not. It had a lot of arguments, with each call site passing a different
combination of null values. This PR:
- packs "url" and "comm_fd" arguments into a variant as they are
mutually exclusive
- removes the "null url *and* null comm_fd" which is not used as of #145017
- marks the function as `static` to make it clear it (now) does not
operate on the `this` object.
---
.../gdb-remote/GDBRemoteCommunication.cpp | 169 ++++++------------
.../gdb-remote/GDBRemoteCommunication.h | 10 +-
.../GDBRemoteCommunicationServerPlatform.cpp | 30 ++--
.../Process/gdb-remote/ProcessGDBRemote.cpp | 6 +-
4 files changed, 78 insertions(+), 137 deletions(-)
diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
index 590d78e98f9a0..776b0c878a835 100644
--- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
@@ -31,6 +31,7 @@
#include <climits>
#include <cstring>
#include <sys/stat.h>
+#include <variant>
#if defined(__APPLE__)
#define DEBUGSERVER_BASENAME "debugserver"
@@ -894,11 +895,9 @@ FileSpec GDBRemoteCommunication::GetDebugserverPath(Platform *platform) {
}
Status GDBRemoteCommunication::StartDebugserverProcess(
- const char *url, Platform *platform, ProcessLaunchInfo &launch_info,
- uint16_t *port, const Args *inferior_args, shared_fd_t pass_comm_fd) {
+ std::variant<llvm::StringRef, shared_fd_t> comm, Platform *platform,
+ ProcessLaunchInfo &launch_info, const Args *inferior_args) {
Log *log = GetLog(GDBRLog::Process);
- LLDB_LOG(log, "Starting debug server: url={0}, port={1}",
- url ? url : "<empty>", port ? *port : uint16_t(0));
FileSpec debugserver_file_spec = GetDebugserverPath(platform);
if (!debugserver_file_spec)
@@ -911,89 +910,58 @@ Status GDBRemoteCommunication::StartDebugserverProcess(
#if !defined(__APPLE__)
// First argument to lldb-server must be mode in which to run.
- debugserver_args.AppendArgument(llvm::StringRef("gdbserver"));
+ debugserver_args.AppendArgument("gdbserver");
#endif
- // If a url is supplied then use it
- if (url && url[0])
- debugserver_args.AppendArgument(llvm::StringRef(url));
-
- if (pass_comm_fd != SharedSocket::kInvalidFD) {
- StreamString fd_arg;
- fd_arg.Printf("--fd=%" PRIi64, (int64_t)pass_comm_fd);
- debugserver_args.AppendArgument(fd_arg.GetString());
- // Send "pass_comm_fd" down to the inferior so it can use it to
- // communicate back with this process. Ignored on Windows.
- launch_info.AppendDuplicateFileAction((int64_t)pass_comm_fd,
- (int64_t)pass_comm_fd);
- }
-
// use native registers, not the GDB registers
- debugserver_args.AppendArgument(llvm::StringRef("--native-regs"));
+ debugserver_args.AppendArgument("--native-regs");
if (launch_info.GetLaunchInSeparateProcessGroup())
- debugserver_args.AppendArgument(llvm::StringRef("--setsid"));
+ debugserver_args.AppendArgument("--setsid");
llvm::SmallString<128> named_pipe_path;
// socket_pipe is used by debug server to communicate back either
- // TCP port or domain socket name which it listens on.
- // The second purpose of the pipe to serve as a synchronization point -
+ // TCP port or domain socket name which it listens on. However, we're not
+ // interested in the actualy value here.
+ // The only reason for using the pipe is to serve as a synchronization point -
// once data is written to the pipe, debug server is up and running.
Pipe socket_pipe;
- std::unique_ptr<TCPSocket> sock_up;
+ // If a url is supplied then use it
+ if (shared_fd_t *comm_fd = std::get_if<shared_fd_t>(&comm)) {
+ LLDB_LOG(log, "debugserver communicates over fd {0}", comm_fd);
+ assert(*comm_fd != SharedSocket::kInvalidFD);
+ debugserver_args.AppendArgument(llvm::formatv("--fd={0}", *comm_fd).str());
+ // Send "comm_fd" down to the inferior so it can use it to communicate back
+ // with this process.
+ launch_info.AppendDuplicateFileAction((int64_t)*comm_fd, (int64_t)*comm_fd);
+ } else {
+ llvm::StringRef url = std::get<llvm::StringRef>(comm);
+ LLDB_LOG(log, "debugserver listens on: {0}", url);
+ debugserver_args.AppendArgument(url);
- // port is null when debug server should listen on domain socket - we're
- // not interested in port value but rather waiting for debug server to
- // become available.
- if (pass_comm_fd == SharedSocket::kInvalidFD) {
- if (url) {
-// Create a temporary file to get the stdout/stderr and redirect the output of
-// the command into this file. We will later read this file if all goes well
-// and fill the data into "command_output_ptr"
#if defined(__APPLE__)
- // Binding to port zero, we need to figure out what port it ends up
- // using using a named pipe...
- Status error = socket_pipe.CreateWithUniqueName("debugserver-named-pipe",
- false, named_pipe_path);
- if (error.Fail()) {
- LLDB_LOG(log, "named pipe creation failed: {0}", error);
- return error;
- }
- debugserver_args.AppendArgument(llvm::StringRef("--named-pipe"));
- debugserver_args.AppendArgument(named_pipe_path);
+ // Using a named pipe as debugserver does not support --pipe.
+ Status error = socket_pipe.CreateWithUniqueName("debugserver-named-pipe",
+ false, named_pipe_path);
+ if (error.Fail()) {
+ LLDB_LOG(log, "named pipe creation failed: {0}", error);
+ return error;
+ }
+ debugserver_args.AppendArgument(llvm::StringRef("--named-pipe"));
+ debugserver_args.AppendArgument(named_pipe_path);
#else
- // Binding to port zero, we need to figure out what port it ends up
- // using using an unnamed pipe...
- Status error = socket_pipe.CreateNew(true);
- if (error.Fail()) {
- LLDB_LOG(log, "unnamed pipe creation failed: {0}", error);
- return error;
- }
- pipe_t write = socket_pipe.GetWritePipe();
- debugserver_args.AppendArgument(llvm::StringRef("--pipe"));
- debugserver_args.AppendArgument(llvm::to_string(write));
- launch_info.AppendCloseFileAction(socket_pipe.GetReadFileDescriptor());
-#endif
- } else {
- // No host and port given, so lets listen on our end and make the
- // debugserver connect to us..
- if (llvm::Expected<std::unique_ptr<TCPSocket>> expected_sock =
- Socket::TcpListen("127.0.0.1:0"))
- sock_up = std::move(*expected_sock);
- else
- return Status::FromError(expected_sock.takeError());
-
- uint16_t port_ = sock_up->GetLocalPortNumber();
- // Send the host and port down that debugserver and specify an option
- // so that it connects back to the port we are listening to in this
- // process
- debugserver_args.AppendArgument(llvm::StringRef("--reverse-connect"));
- debugserver_args.AppendArgument(
- llvm::formatv("127.0.0.1:{0}", port_).str());
- if (port)
- *port = port_;
+ // Using an unnamed pipe as it's simpler.
+ Status error = socket_pipe.CreateNew(true);
+ if (error.Fail()) {
+ LLDB_LOG(log, "unnamed pipe creation failed: {0}", error);
+ return error;
}
+ pipe_t write = socket_pipe.GetWritePipe();
+ debugserver_args.AppendArgument(llvm::StringRef("--pipe"));
+ debugserver_args.AppendArgument(llvm::to_string(write));
+ launch_info.AppendCloseFileAction(socket_pipe.GetReadFileDescriptor());
+#endif
}
Environment host_env = Host::GetEnvironment();
@@ -1070,7 +1038,7 @@ Status GDBRemoteCommunication::StartDebugserverProcess(
return error;
}
- if (pass_comm_fd != SharedSocket::kInvalidFD)
+ if (std::holds_alternative<shared_fd_t>(comm))
return Status();
Status error;
@@ -1084,55 +1052,30 @@ Status GDBRemoteCommunication::StartDebugserverProcess(
if (socket_pipe.CanWrite())
socket_pipe.CloseWriteFileDescriptor();
- if (socket_pipe.CanRead()) {
- // Read port from pipe with 10 second timeout.
- std::string port_str;
- while (error.Success()) {
- char buf[10];
- if (llvm::Expected<size_t> num_bytes =
- socket_pipe.Read(buf, std::size(buf), std::chrono::seconds(10))) {
- if (*num_bytes == 0)
- break;
- port_str.append(buf, *num_bytes);
- } else {
- error = Status::FromError(num_bytes.takeError());
- }
- }
- if (error.Success() && (port != nullptr)) {
- // NB: Deliberately using .c_str() to stop at embedded '\0's
- llvm::StringRef port_ref = port_str.c_str();
- uint16_t child_port = 0;
- // FIXME: improve error handling
- llvm::to_integer(port_ref, child_port);
- if (*port == 0 || *port == child_port) {
- *port = child_port;
- LLDB_LOG(log, "debugserver listens on port {0}", *port);
- } else {
- LLDB_LOG(log,
- "debugserver listening on port {0} but requested port was {1}",
- child_port, (*port));
- }
+ assert(socket_pipe.CanRead());
+
+ // Read port from the pipe -- and ignore it (see comment above).
+ while (error.Success()) {
+ char buf[10];
+ if (llvm::Expected<size_t> num_bytes =
+ socket_pipe.Read(buf, std::size(buf), std::chrono::seconds(10))) {
+ if (*num_bytes == 0)
+ break;
} else {
- LLDB_LOG(log, "failed to read a port value from pipe {0}: {1}",
- named_pipe_path, error);
+ error = Status::FromError(num_bytes.takeError());
}
- socket_pipe.Close();
}
+ if (error.Fail()) {
+ LLDB_LOG(log, "failed to read a port value from pipe {0}: {1}",
+ named_pipe_path, error);
+ }
+ socket_pipe.Close();
if (named_pipe_path.size() > 0) {
if (Status err = socket_pipe.Delete(named_pipe_path); err.Fail())
LLDB_LOG(log, "failed to delete pipe {0}: {1}", named_pipe_path, err);
}
- if (error.Success() && sock_up) {
- Socket *accepted_socket = nullptr;
- error = sock_up->Accept(/*timeout=*/std::nullopt, accepted_socket);
- if (accepted_socket) {
- SetConnection(
- std::make_unique<ConnectionFileDescriptor>(accepted_socket));
- }
- }
-
return error;
}
diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h
index ee87629d9077b..4ee18412be6b6 100644
--- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h
+++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h
@@ -135,17 +135,15 @@ class GDBRemoteCommunication : public Communication {
std::chrono::seconds GetPacketTimeout() const { return m_packet_timeout; }
// Get the debugserver path and check that it exist.
- FileSpec GetDebugserverPath(Platform *platform);
+ static FileSpec GetDebugserverPath(Platform *platform);
// Start a debugserver instance on the current host using the
// supplied connection URL.
- Status StartDebugserverProcess(
- const char *url,
+ static Status StartDebugserverProcess(
+ std::variant<llvm::StringRef, shared_fd_t> comm,
Platform *platform, // If non nullptr, then check with the platform for
// the GDB server binary if it can't be located
- ProcessLaunchInfo &launch_info, uint16_t *port, const Args *inferior_args,
- shared_fd_t pass_comm_fd); // Communication file descriptor to pass during
- // fork/exec to avoid having to connect/accept
+ ProcessLaunchInfo &launch_info, const Args *inferior_args);
void DumpHistory(Stream &strm);
diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.cpp b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.cpp
index 89fdfa74bc025..7506cf64def38 100644
--- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.cpp
@@ -94,7 +94,16 @@ GDBRemoteCommunicationServerPlatform::~GDBRemoteCommunicationServerPlatform() =
Status GDBRemoteCommunicationServerPlatform::LaunchGDBServer(
const lldb_private::Args &args, lldb::pid_t &pid, std::string &socket_name,
shared_fd_t fd) {
- std::ostringstream url;
+ Log *log = GetLog(LLDBLog::Platform);
+
+ ProcessLaunchInfo debugserver_launch_info;
+ // Do not run in a new session so that it can not linger after the platform
+ // closes.
+ debugserver_launch_info.SetLaunchInSeparateProcessGroup(false);
+ debugserver_launch_info.SetMonitorProcessCallback(
+ [](lldb::pid_t, int, int) {});
+
+ Status error;
if (fd == SharedSocket::kInvalidFD) {
if (m_socket_protocol == Socket::ProtocolTcp) {
// Just check that GDBServer exists. GDBServer must be launched after
@@ -104,31 +113,22 @@ Status GDBRemoteCommunicationServerPlatform::LaunchGDBServer(
return Status();
}
+ std::ostringstream url;
// debugserver does not accept the URL scheme prefix.
#if !defined(__APPLE__)
url << Socket::FindSchemeByProtocol(m_socket_protocol) << "://";
#endif
socket_name = GetDomainSocketPath("gdbserver").GetPath();
url << socket_name;
+ error = StartDebugserverProcess(url.str(), nullptr, debugserver_launch_info,
+ &args);
} else {
if (m_socket_protocol != Socket::ProtocolTcp)
return Status::FromErrorString("protocol must be tcp");
+ error =
+ StartDebugserverProcess(fd, nullptr, debugserver_launch_info, &args);
}
- // Spawn a debugserver and try to get the port it listens to.
- ProcessLaunchInfo debugserver_launch_info;
- Log *log = GetLog(LLDBLog::Platform);
- LLDB_LOG(log, "Launching debugserver url='{0}', fd={1}...", url.str(), fd);
-
- // Do not run in a new session so that it can not linger after the platform
- // closes.
- debugserver_launch_info.SetLaunchInSeparateProcessGroup(false);
- debugserver_launch_info.SetMonitorProcessCallback(
- [](lldb::pid_t, int, int) {});
-
- Status error = StartDebugserverProcess(
- url.str().c_str(), nullptr, debugserver_launch_info, nullptr, &args, fd);
-
if (error.Success()) {
pid = debugserver_launch_info.GetProcessID();
AddSpawnedProcess(pid);
diff --git a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
index ba806767f78da..473580129fc30 100644
--- a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -3494,9 +3494,9 @@ Status ProcessGDBRemote::LaunchAndConnectToDebugserver(
if (error.Fail())
return error;
- error = m_gdb_comm.StartDebugserverProcess(
- nullptr, GetTarget().GetPlatform().get(), debugserver_launch_info,
- nullptr, nullptr, shared_socket.GetSendableFD());
+ error = m_gdb_comm.StartDebugserverProcess(shared_socket.GetSendableFD(),
+ GetTarget().GetPlatform().get(),
+ debugserver_launch_info, nullptr);
if (error.Fail()) {
Log *log = GetLog(GDBRLog::Process);
More information about the lldb-commits
mailing list