[lldb] Removed gdbserver ports map from lldb-server

Listen to gdbserver-port, accept the connection and run lldb-server gdbserver --fd on all platforms.
Refactored Acceptor to listen on 2 ports in the main thread.

Parameters --min-gdbserver-port and --max-gdbserver-port are deprecated now.

This is the part 2 of llvm#101283.

Fixes llvm#97537, fixes llvm#101475.

Author: slydiman

lldb/docs/man/lldb-server.rst

@@ -147,15 +147,8 @@ GDB-SERVER CONNECTIONS
 
 .. option:: --gdbserver-port <port>
 
- Define a port to be used for gdb-server connections. Can be specified multiple
- times to allow multiple ports. Has no effect if --min-gdbserver-port
- and --max-gdbserver-port are specified.
-
-.. option:: --min-gdbserver-port <port>
-.. option:: --max-gdbserver-port <port>
-
- Specify the range of ports that can be used for gdb-server connections. Both
- options need to be specified simultaneously. Overrides --gdbserver-port.
+ Define a port to be used for gdb-server connections. This port is used for
+ multiple connections.
 
 .. option:: --port-offset <offset>
 

lldb/docs/resources/qemu-testing.rst

@@ -149,30 +149,21 @@ to the host (refer to QEMU's manuals for the specific options).
 * At least one to connect to the intial ``lldb-server``.
 * One more if you want to use ``lldb-server`` in ``platform mode``, and have it
   start a ``gdbserver`` instance for you.
-* A bunch more if you want to run tests against the ``lldb-server`` platform.
 
 If you are doing either of the latter 2 you should also restrict what ports
 ``lldb-server tries`` to use, otherwise it will randomly pick one that is almost
 certainly not forwarded. An example of this is shown below.
 
 ::
 
-  $ lldb-server plaform --server --listen 0.0.0.0:54321 \
-    --min-gdbserver-port 49140 --max-gdbserver-port 49150
+  $ lldb-server plaform --server --listen 0.0.0.0:54321 --gdbserver-port 49140
 
 The result of this is that:
 
 * ``lldb-server`` platform mode listens externally on port ``54321``.
 
-* When it is asked to start a new gdbserver mode instance, it will use a port
-  in the range ``49140`` to ``49150``.
+* When it is asked to start a new gdbserver mode instance, it will use the port
+  ``49140``.
 
-Your VM configuration should have ports ``54321``, and ``49140`` to ``49150``
-forwarded for this to work.
-
-.. note::
-  These options are used to create a "port map" within ``lldb-server``.
-  Unfortunately this map is not cleaned up on Windows on connection close,
-  and across a few uses you may run out of valid ports. To work around this,
-  restart the platform every so often, especially after running a set of tests.
-  This is tracked here: https://github.com/llvm/llvm-project/issues/90923
+Your VM configuration should have ports ``54321`` and ``49140`` forwarded for
+this to work.

lldb/include/lldb/Host/Socket.h

@@ -34,9 +34,11 @@ namespace lldb_private {
 #if defined(_WIN32)
 typedef SOCKET NativeSocket;
 typedef lldb::pipe_t shared_fd_t;
+typedef const char *set_socket_option_arg_type;
 #else
 typedef int NativeSocket;
 typedef NativeSocket shared_fd_t;
+typedef const void *set_socket_option_arg_type;
 #endif
 class Socket;
 class TCPSocket;
@@ -132,12 +134,8 @@ class Socket : public IOObject {
   // If this Socket is connected then return the URI used to connect.
   virtual std::string GetRemoteConnectionURI() const { return ""; };
 
-protected:
-  Socket(SocketProtocol protocol, bool should_close,
-         bool m_child_process_inherit);
-
-  virtual size_t Send(const void *buf, const size_t num_bytes);
-
+  static int CloseSocket(NativeSocket sockfd);
+  static Status GetLastError();
   static void SetLastError(Status &error);
   static NativeSocket CreateSocket(const int domain, const int type,
                                    const int protocol,
@@ -146,6 +144,12 @@ class Socket : public IOObject {
                                    socklen_t *addrlen,
                                    bool child_processes_inherit, Status &error);
 
+protected:
+  Socket(SocketProtocol protocol, bool should_close,
+         bool m_child_process_inherit);
+
+  virtual size_t Send(const void *buf, const size_t num_bytes);
+
   SocketProtocol m_protocol;
   NativeSocket m_socket;
   bool m_child_processes_inherit;

lldb/source/Host/common/Socket.cpp

@@ -386,11 +386,7 @@ Status Socket::Close() {
   LLDB_LOGF(log, "%p Socket::Close (fd = %" PRIu64 ")",
             static_cast<void *>(this), static_cast<uint64_t>(m_socket));
 
-#if defined(_WIN32)
-  bool success = closesocket(m_socket) == 0;
-#else
-  bool success = ::close(m_socket) == 0;
-#endif
+  bool success = CloseSocket(m_socket) == 0;
   // A reference to a FD was passed in, set it to an invalid value
   m_socket = kInvalidSocketValue;
   if (!success) {
@@ -427,6 +423,24 @@ void Socket::SetLastError(Status &error) {
 #endif
 }
 
+Status Socket::GetLastError() {
+  std::error_code EC;
+#ifdef _WIN32
+  EC = llvm::mapWindowsError(WSAGetLastError());
+#else
+  EC = std::error_code(errno, std::generic_category());
+#endif
+  return EC;
+}
+
+int Socket::CloseSocket(NativeSocket sockfd) {
+#ifdef _WIN32
+  return ::closesocket(sockfd);
+#else
+  return ::close(sockfd);
+#endif
+}
+
 NativeSocket Socket::CreateSocket(const int domain, const int type,
                                   const int protocol,
                                   bool child_processes_inherit, Status &error) {

lldb/source/Host/common/TCPSocket.cpp

@@ -32,28 +32,9 @@
 #include <winsock2.h>
 #endif
 
-#ifdef _WIN32
-#define CLOSE_SOCKET closesocket
-typedef const char *set_socket_option_arg_type;
-#else
-#include <unistd.h>
-#define CLOSE_SOCKET ::close
-typedef const void *set_socket_option_arg_type;
-#endif
-
 using namespace lldb;
 using namespace lldb_private;
 
-static Status GetLastSocketError() {
-  std::error_code EC;
-#ifdef _WIN32
-  EC = llvm::mapWindowsError(WSAGetLastError());
-#else
-  EC = std::error_code(errno, std::generic_category());
-#endif
-  return EC;
-}
-
 static const int kType = SOCK_STREAM;
 
 TCPSocket::TCPSocket(bool should_close, bool child_processes_inherit)
@@ -212,7 +193,7 @@ Status TCPSocket::Listen(llvm::StringRef name, int backlog) {
         reinterpret_cast<set_socket_option_arg_type>(&option_value);
     if (::setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, option_value_p,
                      sizeof(option_value)) == -1) {
-      CLOSE_SOCKET(fd);
+      CloseSocket(fd);
       continue;
     }
 
@@ -228,8 +209,8 @@ Status TCPSocket::Listen(llvm::StringRef name, int backlog) {
       err = ::listen(fd, backlog);
 
     if (err == -1) {
-      error = GetLastSocketError();
-      CLOSE_SOCKET(fd);
+      error = GetLastError();
+      CloseSocket(fd);
       continue;
     }
 
@@ -250,7 +231,7 @@ Status TCPSocket::Listen(llvm::StringRef name, int backlog) {
 
 void TCPSocket::CloseListenSockets() {
   for (auto socket : m_listen_sockets)
-    CLOSE_SOCKET(socket.first);
+    CloseSocket(socket.first);
   m_listen_sockets.clear();
 }
 
@@ -295,7 +276,7 @@ Status TCPSocket::Accept(Socket *&conn_socket) {
     lldb_private::SocketAddress &AddrIn = m_listen_sockets[listen_sock];
     if (!AddrIn.IsAnyAddr() && AcceptAddr != AddrIn) {
       if (sock != kInvalidSocketValue) {
-        CLOSE_SOCKET(sock);
+        CloseSocket(sock);
         sock = kInvalidSocketValue;
       }
       llvm::errs() << llvm::formatv(

lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp

@@ -162,8 +162,10 @@ ConnectionFileDescriptor::Connect(llvm::StringRef path,
             .Case("unix-connect", &ConnectionFileDescriptor::ConnectNamedSocket)
             .Case("unix-abstract-connect",
                   &ConnectionFileDescriptor::ConnectAbstractSocket)
-#if LLDB_ENABLE_POSIX
+#if LLDB_ENABLE_POSIX || defined(_WIN32)
             .Case("fd", &ConnectionFileDescriptor::ConnectFD)
+#endif
+#if LLDB_ENABLE_POSIX
             .Case("file", &ConnectionFileDescriptor::ConnectFile)
             .Case("serial", &ConnectionFileDescriptor::ConnectSerialPort)
 #endif
@@ -667,7 +669,22 @@ ConnectionStatus
 ConnectionFileDescriptor::ConnectFD(llvm::StringRef s,
                                     socket_id_callback_type socket_id_callback,
                                     Status *error_ptr) {
-#if LLDB_ENABLE_POSIX
+#ifdef _WIN32
+  int64_t fd = -1;
+  if (!s.getAsInteger(0, fd)) {
+    // Assume we own fd.
+    std::unique_ptr<TCPSocket> tcp_socket =
+        std::make_unique<TCPSocket>((NativeSocket)fd, true, false);
+    m_io_sp = std::move(tcp_socket);
+    m_uri = s.str();
+    return eConnectionStatusSuccess;
+  }
+  if (error_ptr)
+    *error_ptr = Status::FromErrorStringWithFormat(
+        "invalid file descriptor: \"%s\"", s.str().c_str());
+  m_io_sp.reset();
+  return eConnectionStatusError;
+#elif LLDB_ENABLE_POSIX
   // Just passing a native file descriptor within this current process that
   // is already opened (possibly from a service or other source).
   int fd = -1;

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

@@ -879,20 +879,12 @@ lldb::thread_result_t GDBRemoteCommunication::ListenThread() {
   return {};
 }
 
-Status GDBRemoteCommunication::StartDebugserverProcess(
-    const char *url, Platform *platform, ProcessLaunchInfo &launch_info,
-    uint16_t *port, const Args *inferior_args, int pass_comm_fd) {
+bool GDBRemoteCommunication::GetDebugserverPath(
+    Platform *platform, FileSpec &debugserver_file_spec) {
   Log *log = GetLog(GDBRLog::Process);
-  LLDB_LOGF(log, "GDBRemoteCommunication::%s(url=%s, port=%" PRIu16 ")",
-            __FUNCTION__, url ? url : "<empty>", port ? *port : uint16_t(0));
-
-  Status error;
   // If we locate debugserver, keep that located version around
   static FileSpec g_debugserver_file_spec;
 
-  char debugserver_path[PATH_MAX];
-  FileSpec &debugserver_file_spec = launch_info.GetExecutableFile();
-
   Environment host_env = Host::GetEnvironment();
 
   // Always check to see if we have an environment override for the path to the
@@ -943,8 +935,20 @@ Status GDBRemoteCommunication::StartDebugserverProcess(
       }
     }
   }
+  return debugserver_exists;
+}
 
-  if (debugserver_exists) {
+Status GDBRemoteCommunication::StartDebugserverProcess(
+    const char *url, Platform *platform, ProcessLaunchInfo &launch_info,
+    uint16_t *port, const Args *inferior_args, shared_fd_t pass_comm_fd) {
+  Log *log = GetLog(GDBRLog::Process);
+  LLDB_LOGF(log, "GDBRemoteCommunication::%s(url=%s, port=%" PRIu16 ")",
+            __FUNCTION__, url ? url : "<empty>", port ? *port : uint16_t(0));
+
+  Status error;
+  FileSpec &debugserver_file_spec = launch_info.GetExecutableFile();
+  if (GetDebugserverPath(platform, debugserver_file_spec)) {
+    char debugserver_path[PATH_MAX];
     debugserver_file_spec.GetPath(debugserver_path, sizeof(debugserver_path));
 
     Args &debugserver_args = launch_info.GetArguments();
@@ -959,16 +963,19 @@ Status GDBRemoteCommunication::StartDebugserverProcess(
 #endif
 
     // If a url is supplied then use it
-    if (url)
+    if (url && url[0])
       debugserver_args.AppendArgument(llvm::StringRef(url));
 
-    if (pass_comm_fd >= 0) {
+    if (pass_comm_fd != SharedSocket::kInvalidFD) {
       StreamString fd_arg;
-      fd_arg.Printf("--fd=%i", pass_comm_fd);
+      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
-      launch_info.AppendDuplicateFileAction(pass_comm_fd, pass_comm_fd);
+      // communicate back with this process. Ignored on Windows.
+#ifndef _WIN32
+      launch_info.AppendDuplicateFileAction((int)pass_comm_fd,
+                                            (int)pass_comm_fd);
+#endif
     }
 
     // use native registers, not the GDB registers
@@ -988,7 +995,7 @@ Status GDBRemoteCommunication::StartDebugserverProcess(
     // 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 == -1) {
+    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
@@ -1059,6 +1066,8 @@ Status GDBRemoteCommunication::StartDebugserverProcess(
         }
       }
     }
+
+    Environment host_env = Host::GetEnvironment();
     std::string env_debugserver_log_file =
         host_env.lookup("LLDB_DEBUGSERVER_LOG_FILE");
     if (!env_debugserver_log_file.empty()) {
@@ -1132,7 +1141,7 @@ Status GDBRemoteCommunication::StartDebugserverProcess(
 
     if (error.Success() &&
         (launch_info.GetProcessID() != LLDB_INVALID_PROCESS_ID) &&
-        pass_comm_fd == -1) {
+        pass_comm_fd == SharedSocket::kInvalidFD) {
       if (named_pipe_path.size() > 0) {
         error = socket_pipe.OpenAsReader(named_pipe_path, false);
         if (error.Fail())

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

@@ -21,6 +21,7 @@
 #include "lldb/Core/Communication.h"
 #include "lldb/Host/Config.h"
 #include "lldb/Host/HostThread.h"
+#include "lldb/Host/Socket.h"
 #include "lldb/Utility/Args.h"
 #include "lldb/Utility/Listener.h"
 #include "lldb/Utility/Predicate.h"
@@ -146,15 +147,18 @@ class GDBRemoteCommunication : public Communication {
 
   std::chrono::seconds GetPacketTimeout() const { return m_packet_timeout; }
 
+  // Get the debugserver path and check that it exist.
+  bool GetDebugserverPath(Platform *platform, FileSpec &debugserver_file_spec);
+
   // Start a debugserver instance on the current host using the
   // supplied connection URL.
   Status StartDebugserverProcess(
       const char *url,
       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,
-      int pass_comm_fd); // Communication file descriptor to pass during
-                         // fork/exec to avoid having to connect/accept
+      shared_fd_t pass_comm_fd); // Communication file descriptor to pass during
+                                 // fork/exec to avoid having to connect/accept
 
   void DumpHistory(Stream &strm);