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 <pa...@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 <pa...@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 <pa...@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); _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
