[lldb] Remove GDBRemoteCommunication::ConnectLocally (#145293)

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

Author: labath

lldb/include/lldb/Host/posix/ConnectionFileDescriptorPosix.h

@@ -18,13 +18,10 @@
 #include "lldb/Host/Pipe.h"
 #include "lldb/Host/Socket.h"
 #include "lldb/Utility/Connection.h"
-#include "lldb/Utility/IOObject.h"
 
 namespace lldb_private {
 
 class Status;
-class Socket;
-class SocketAddress;
 
 class ConnectionFileDescriptor : public Connection {
 public:
@@ -35,7 +32,7 @@ class ConnectionFileDescriptor : public Connection {
 
   ConnectionFileDescriptor(int fd, bool owns_fd);
 
-  ConnectionFileDescriptor(Socket *socket);
+  ConnectionFileDescriptor(std::unique_ptr<Socket> socket_up);
 
   ~ConnectionFileDescriptor() override;
 
@@ -136,8 +133,6 @@ class ConnectionFileDescriptor : public Connection {
   std::string m_uri;
 
 private:
-  void InitializeSocket(Socket *socket);
-
   ConnectionFileDescriptor(const ConnectionFileDescriptor &) = delete;
   const ConnectionFileDescriptor &
   operator=(const ConnectionFileDescriptor &) = delete;

lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp

@@ -72,9 +72,11 @@ ConnectionFileDescriptor::ConnectionFileDescriptor(int fd, bool owns_fd)
   OpenCommandPipe();
 }
 
-ConnectionFileDescriptor::ConnectionFileDescriptor(Socket *socket)
-    : Connection(), m_pipe(), m_mutex(), m_shutting_down(false) {
-  InitializeSocket(socket);
+ConnectionFileDescriptor::ConnectionFileDescriptor(
+    std::unique_ptr<Socket> socket_up)
+    : m_shutting_down(false) {
+  m_uri = socket_up->GetRemoteConnectionURI();
+  m_io_sp = std::move(socket_up);
 }
 
 ConnectionFileDescriptor::~ConnectionFileDescriptor() {
@@ -796,8 +798,3 @@ ConnectionStatus ConnectionFileDescriptor::ConnectSerialPort(
 #endif // LLDB_ENABLE_POSIX
   llvm_unreachable("this function should be only called w/ LLDB_ENABLE_POSIX");
 }
-
-void ConnectionFileDescriptor::InitializeSocket(Socket *socket) {
-  m_io_sp.reset(socket);
-  m_uri = socket->GetRemoteConnectionURI();
-}

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp

@@ -1128,8 +1128,8 @@ Status GDBRemoteCommunication::StartDebugserverProcess(
     Socket *accepted_socket = nullptr;
     error = sock_up->Accept(/*timeout=*/std::nullopt, accepted_socket);
     if (accepted_socket) {
-      SetConnection(
-          std::make_unique<ConnectionFileDescriptor>(accepted_socket));
+      SetConnection(std::make_unique<ConnectionFileDescriptor>(
+          std::unique_ptr<Socket>(accepted_socket)));
     }
   }
 
@@ -1138,20 +1138,6 @@ Status GDBRemoteCommunication::StartDebugserverProcess(
 
 void GDBRemoteCommunication::DumpHistory(Stream &strm) { m_history.Dump(strm); }
 
-llvm::Error
-GDBRemoteCommunication::ConnectLocally(GDBRemoteCommunication &client,
-                                       GDBRemoteCommunication &server) {
-  llvm::Expected<Socket::Pair> pair = Socket::CreatePair();
-  if (!pair)
-    return pair.takeError();
-
-  client.SetConnection(
-      std::make_unique<ConnectionFileDescriptor>(pair->first.release()));
-  server.SetConnection(
-      std::make_unique<ConnectionFileDescriptor>(pair->second.release()));
-  return llvm::Error::success();
-}
-
 GDBRemoteCommunication::ScopedTimeout::ScopedTimeout(
     GDBRemoteCommunication &gdb_comm, std::chrono::seconds timeout)
     : m_gdb_comm(gdb_comm), m_saved_timeout(0), m_timeout_modified(false) {

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h

@@ -149,9 +149,6 @@ class GDBRemoteCommunication : public Communication {
 
   void DumpHistory(Stream &strm);
 
-  static llvm::Error ConnectLocally(GDBRemoteCommunication &client,
-                                    GDBRemoteCommunication &server);
-
   /// Expand GDB run-length encoding.
   static std::optional<std::string> ExpandRLE(std::string);
 

lldb/tools/lldb-server/lldb-platform.cpp

@@ -485,8 +485,8 @@ int main_platform(int argc, char *argv[]) {
 
     GDBRemoteCommunicationServerPlatform platform(socket->GetSocketProtocol(),
                                                   gdbserver_port);
-    platform.SetConnection(std::unique_ptr<Connection>(
-        new ConnectionFileDescriptor(socket.release())));
+    platform.SetConnection(
+        std::make_unique<ConnectionFileDescriptor>(std::move(socket)));
     client_handle(platform, inferior_arguments);
     return 0;
   }

lldb/unittests/Core/CommunicationTest.cpp

@@ -41,7 +41,7 @@ static void CommunicationReadTest(bool use_read_thread) {
 
   ThreadedCommunication comm("test");
   comm.SetConnection(
-      std::make_unique<ConnectionFileDescriptor>(pair->second.release()));
+      std::make_unique<ConnectionFileDescriptor>(std::move(pair->second)));
   comm.SetCloseOnEOF(true);
 
   if (use_read_thread) {
@@ -126,7 +126,7 @@ TEST_F(CommunicationTest, SynchronizeWhileClosing) {
 
   ThreadedCommunication comm("test");
   comm.SetConnection(
-      std::make_unique<ConnectionFileDescriptor>(pair->second.release()));
+      std::make_unique<ConnectionFileDescriptor>(std::move(pair->second)));
   comm.SetCloseOnEOF(true);
   ASSERT_TRUE(comm.StartReadThread());
 

lldb/unittests/Host/ConnectionFileDescriptorTest.cpp

@@ -22,11 +22,11 @@ class ConnectionFileDescriptorTest : public testing::Test {
     std::unique_ptr<TCPSocket> socket_a_up;
     std::unique_ptr<TCPSocket> socket_b_up;
     CreateTCPConnectedSockets(ip, &socket_a_up, &socket_b_up);
-    auto *socket = socket_a_up.release();
-    ConnectionFileDescriptor connection_file_descriptor(socket);
+    uint16_t socket_a_remote_port = socket_a_up->GetRemotePortNumber();
+    ConnectionFileDescriptor connection_file_descriptor(std::move(socket_a_up));
 
     std::string uri(connection_file_descriptor.GetURI());
-    EXPECT_EQ((URI{"connect", ip, socket->GetRemotePortNumber(), "/"}),
+    EXPECT_EQ((URI{"connect", ip, socket_a_remote_port, "/"}),
               *URI::Parse(uri));
   }
 };

lldb/unittests/Process/gdb-remote/GDBRemoteClientBaseTest.cpp

@@ -10,6 +10,7 @@
 #include "GDBRemoteTestUtils.h"
 #include "Plugins/Process/Utility/LinuxSignals.h"
 #include "Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h"
+#include "lldb/Host/ConnectionFileDescriptor.h"
 #include "lldb/Utility/GDBRemote.h"
 #include "lldb/Utility/Listener.h"
 #include "llvm/ADT/StringRef.h"
@@ -51,8 +52,12 @@ struct TestClient : public GDBRemoteClientBase {
 class GDBRemoteClientBaseTest : public GDBRemoteTest {
 public:
   void SetUp() override {
-    ASSERT_THAT_ERROR(GDBRemoteCommunication::ConnectLocally(client, server),
-                      llvm::Succeeded());
+    llvm::Expected<Socket::Pair> pair = Socket::CreatePair();
+    ASSERT_THAT_EXPECTED(pair, llvm::Succeeded());
+    client.SetConnection(
+        std::make_unique<ConnectionFileDescriptor>(std::move(pair->first)));
+    server.SetConnection(
+        std::make_unique<ConnectionFileDescriptor>(std::move(pair->second)));
     ASSERT_EQ(TestClient::eBroadcastBitRunPacketSent,
               listener_sp->StartListeningForEvents(
                   &client, TestClient::eBroadcastBitRunPacketSent));

lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp

@@ -8,6 +8,7 @@
 #include "Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h"
 #include "GDBRemoteTestUtils.h"
 #include "lldb/Core/ModuleSpec.h"
+#include "lldb/Host/ConnectionFileDescriptor.h"
 #include "lldb/Host/XML.h"
 #include "lldb/Target/MemoryRegionInfo.h"
 #include "lldb/Utility/DataBuffer.h"
@@ -63,8 +64,12 @@ std::string one_register_hex = "41424344";
 class GDBRemoteCommunicationClientTest : public GDBRemoteTest {
 public:
   void SetUp() override {
-    ASSERT_THAT_ERROR(GDBRemoteCommunication::ConnectLocally(client, server),
-                      llvm::Succeeded());
+    llvm::Expected<Socket::Pair> pair = Socket::CreatePair();
+    ASSERT_THAT_EXPECTED(pair, llvm::Succeeded());
+    client.SetConnection(
+        std::make_unique<ConnectionFileDescriptor>(std::move(pair->first)));
+    server.SetConnection(
+        std::make_unique<ConnectionFileDescriptor>(std::move(pair->second)));
   }
 
 protected:

lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationTest.cpp

@@ -5,7 +5,9 @@
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
+
 #include "GDBRemoteTestUtils.h"
+#include "lldb/Host/ConnectionFileDescriptor.h"
 #include "llvm/Testing/Support/Error.h"
 
 using namespace lldb_private::process_gdb_remote;
@@ -28,8 +30,12 @@ class TestClient : public GDBRemoteCommunication {
 class GDBRemoteCommunicationTest : public GDBRemoteTest {
 public:
   void SetUp() override {
-    ASSERT_THAT_ERROR(GDBRemoteCommunication::ConnectLocally(client, server),
-                      llvm::Succeeded());
+    llvm::Expected<Socket::Pair> pair = Socket::CreatePair();
+    ASSERT_THAT_EXPECTED(pair, llvm::Succeeded());
+    client.SetConnection(
+        std::make_unique<ConnectionFileDescriptor>(std::move(pair->first)));
+    server.SetConnection(
+        std::make_unique<ConnectionFileDescriptor>(std::move(pair->second)));
   }
 
 protected:

lldb/unittests/tools/lldb-server/tests/TestClient.cpp

@@ -125,7 +125,8 @@ TestClient::launchCustom(StringRef Log, bool disable_stdio,
           listen_socket.Accept(2 * GetDefaultTimeout(), accept_socket)
               .takeError())
     return E;
-  auto Conn = std::make_unique<ConnectionFileDescriptor>(accept_socket);
+  auto Conn = std::make_unique<ConnectionFileDescriptor>(
+      std::unique_ptr<Socket>(accept_socket));
   auto Client = std::unique_ptr<TestClient>(new TestClient(std::move(Conn)));
 
   if (Error E = Client->initializeConnection())