[lldb] Add Socket::CreatePair #145015

labath · 2025-06-20T10:58:47Z

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.

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.

llvmbot · 2025-06-20T10:59:22Z

@llvm/pr-subscribers-lldb

Author: Pavel Labath (labath)

Changes

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.

Full diff: https://github.com/llvm/llvm-project/pull/145015.diff

9 Files Affected:

(modified) lldb/include/lldb/Host/Socket.h (+4)
(modified) lldb/include/lldb/Host/common/TCPSocket.h (+4)
(modified) lldb/include/lldb/Host/posix/DomainSocket.h (+4)
(modified) lldb/source/Host/common/Socket.cpp (+17)
(modified) lldb/source/Host/common/TCPSocket.cpp (+28)
(modified) lldb/source/Host/posix/DomainSocket.cpp (+29)
(modified) lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp (+8-28)
(modified) lldb/unittests/Core/CommunicationTest.cpp (+15-11)
(modified) lldb/unittests/Host/SocketTest.cpp (+35)

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;

JDevlieghere

LGTM

JDevlieghere · 2025-06-20T18:12:04Z

lldb/include/lldb/Host/Socket.h

+  static llvm::Expected<
+      std::pair<std::unique_ptr<Socket>, std::unique_ptr<Socket>>>


Nit: Maybe add a Socket::Pair or Socket::SocketPair typedef to make these declarations a little less verbose?

using Pair = std::pair<std::unique_ptr<Socket>, std::unique_ptr<Socket>>;

Good idea. That means I can also get rid of all the autos I was too lazy to type.

JDevlieghere · 2025-06-20T18:16:28Z

lldb/source/Host/common/TCPSocket.cpp

+  // 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);


Can you think of a reason why we couldn't change the signature of Socket::Accept to return an llvm::Expected<std::unique_ptr>> instead of this Status + Socket* out parameter? Not trying to sign you up for it but it stands out.

No reason. I sort of actually have a patch stack for a major overhaul of the socket classes, and it fixes this (among other things). This PR is kind of a part of a very long and windy way of upstreaming it. :)

Originally added for reproducers, it is now only used for test code. While we could make it a test helper, I think that after llvm#145015 it is simple enough to not be needed. Also squeeze in a change to make ConnectionFileDescriptor accept a unique_ptr<Socket>.

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.

Originally added for reproducers, it is now only used for test code. While we could make it a test helper, I think that after #145015 it is simple enough to not be needed. Also squeeze in a change to make ConnectionFileDescriptor accept a unique_ptr<Socket>.

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. The main effect of this change is that lldb on windows will now use the `--fd` lldb-server argument for "local remote" debug sessions instead of having lldb-server connect back to lldb. This is the same method used by lldb on non-windows platforms (for many years) and "lldb-server platform" on windows for truly remote debug sessions (for ~one year). Depends on #145015.

Originally added for reproducers, it is now only used for test code. While we could make it a test helper, I think that after llvm#145015 it is simple enough to not be needed. Also squeeze in a change to make ConnectionFileDescriptor accept a unique_ptr<Socket>.

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. The main effect of this change is that lldb on windows will now use the `--fd` lldb-server argument for "local remote" debug sessions instead of having lldb-server connect back to lldb. This is the same method used by lldb on non-windows platforms (for many years) and "lldb-server platform" on windows for truly remote debug sessions (for ~one year). Depends on llvm#145015.

Originally added for reproducers, it is now only used for test code. While we could make it a test helper, I think that after llvm#145015 it is simple enough to not be needed. Also squeeze in a change to make ConnectionFileDescriptor accept a unique_ptr<Socket>.

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. The main effect of this change is that lldb on windows will now use the `--fd` lldb-server argument for "local remote" debug sessions instead of having lldb-server connect back to lldb. This is the same method used by lldb on non-windows platforms (for many years) and "lldb-server platform" on windows for truly remote debug sessions (for ~one year). Depends on llvm#145015.

labath requested a review from DavidSpickett June 20, 2025 10:58

labath requested a review from JDevlieghere as a code owner June 20, 2025 10:58

llvmbot added the lldb label Jun 20, 2025

This was referenced Jun 20, 2025

[lldb] Use Socket::CreatePair for launching debugserver #145017

Merged

[lldb] Clean up GDBRemoteCommunication::StartDebugserverProcess #145021

Merged

JDevlieghere approved these changes Jun 20, 2025

View reviewed changes

add Pair typedef

58eb339

labath merged commit c5629f2 into llvm:main Jun 23, 2025
7 checks passed

labath deleted the socketpair branch June 23, 2025 08:52

labath mentioned this pull request Jun 23, 2025

[lldb] Remove GDBRemoteCommunication::ConnectLocally #145293

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[lldb] Add Socket::CreatePair #145015

[lldb] Add Socket::CreatePair #145015

Uh oh!

labath commented Jun 20, 2025

Uh oh!

llvmbot commented Jun 20, 2025

Uh oh!

JDevlieghere left a comment

Uh oh!

JDevlieghere Jun 20, 2025

Uh oh!

labath Jun 23, 2025

Uh oh!

JDevlieghere Jun 20, 2025

Uh oh!

labath Jun 23, 2025

Uh oh!

Uh oh!

Uh oh!

		static llvm::Expected<
		std::pair<std::unique_ptr<Socket>, std::unique_ptr<Socket>>>

[lldb] Add Socket::CreatePair #145015

[lldb] Add Socket::CreatePair #145015

Uh oh!

Conversation

labath commented Jun 20, 2025

Uh oh!

llvmbot commented Jun 20, 2025

Uh oh!

JDevlieghere left a comment

Choose a reason for hiding this comment

Uh oh!

JDevlieghere Jun 20, 2025

Choose a reason for hiding this comment

Uh oh!

labath Jun 23, 2025

Choose a reason for hiding this comment

Uh oh!

JDevlieghere Jun 20, 2025

Choose a reason for hiding this comment

Uh oh!

labath Jun 23, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!