[Lldb-commits] [lldb] [lldb] Add Socket::CreatePair (PR #145015)

[via lldb-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-lldb

Author: Pavel Labath (labath)

<details>
<summary>Changes</summary>

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
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;

``````````

</details>


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