[Lldb-commits] [lldb] 6aed0ab - [lldb] Have lldb-server assign ports to children in platform mode (#88845)

[via lldb-commits]

Author: Anthony Ha
Date: 2024-05-07T09:45:07+01:00
New Revision: 6aed0ab6547f577cceaccfc6d710f96b645c3af7

URL: https://github.com/llvm/llvm-project/commit/6aed0ab6547f577cceaccfc6d710f96b645c3af7
DIFF: https://github.com/llvm/llvm-project/commit/6aed0ab6547f577cceaccfc6d710f96b645c3af7.diff

LOG: [lldb] Have lldb-server assign ports to children in platform mode (#88845)

Fixes #47549

`lldb-server`'s platform mode seems to have an issue with its
`--min-gdbserver-port` `--max-gdbserver-port` flags (and probably the
`--gdbserver-port` flag, but I didn't test it).

How the platform code seems to work is that it listens on a port, and
whenever there's an incoming connection, it forks the process to handle
the connection. To handle the port flags, the main process uses an
instance of the helper class
`GDBRemoteCommunicationServerPlatform::PortMap`, that can be configured
and track usages of ports. The child process handling the platform
connection, can then use the port map to allocate a port for the
gdb-server connection it will make (this is another process it spawns).

However, in the current code, this works only once. After the first
connection is handled by forking a child process, the main platform
listener code loops around, and then 'forgets' about the port map. This
is because this code:
```cpp
GDBRemoteCommunicationServerPlatform platform(
    acceptor_up->GetSocketProtocol(), acceptor_up->GetSocketScheme());
if (!gdbserver_portmap.empty()) {
  platform.SetPortMap(std::move(gdbserver_portmap));
}
```
is within the connection listening loop. This results in the
`gdbserver_portmap` being moved into the platform object at the
beginning of the first iteration of the loop, but on the second
iteration, after the first fork, the next instance of the platform
object will not have its platform port mapped.
The result of this bug is that subsequent connections to the platform,
when spawning the gdb-remote connection, will be supplied a random port
- which isn't bounded by the `--min-gdbserver-port` and
`--max-gdbserver--port` parameters passed in by the user.

This PR fixes this issue by having the port map be maintained by the
parent platform listener process. On connection, the listener allocates
a single available port from the port map, associates the child process
pid with the port, and lets the connection handling child use that
single port number.

Additionally, when cleaning up child processes, the main listener
process tracks the child that exited to deallocate the previously
associated port, so it can be reused for a new connection.

Added: 
    

Modified: 
    lldb/docs/use/qemu-testing.rst
    lldb/tools/lldb-server/lldb-platform.cpp

Removed: 
    


################################################################################
diff  --git a/lldb/docs/use/qemu-testing.rst b/lldb/docs/use/qemu-testing.rst
index 6e282141864cc1..51a30b11717a87 100644
--- a/lldb/docs/use/qemu-testing.rst
+++ b/lldb/docs/use/qemu-testing.rst
@@ -172,6 +172,7 @@ forwarded for this to work.
 
 .. note::
   These options are used to create a "port map" within ``lldb-server``.
-  Unfortunately this map is not shared across all the processes it may create,
+  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

diff  --git a/lldb/tools/lldb-server/lldb-platform.cpp b/lldb/tools/lldb-server/lldb-platform.cpp
index 3e126584eb25b4..cfd0a3797d810a 100644
--- a/lldb/tools/lldb-server/lldb-platform.cpp
+++ b/lldb/tools/lldb-server/lldb-platform.cpp
@@ -282,17 +282,12 @@ int main_platform(int argc, char *argv[]) {
     }
   }
 
-  do {
-    GDBRemoteCommunicationServerPlatform platform(
-        acceptor_up->GetSocketProtocol(), acceptor_up->GetSocketScheme());
-
-    if (port_offset > 0)
-      platform.SetPortOffset(port_offset);
-
-    if (!gdbserver_portmap.empty()) {
-      platform.SetPortMap(std::move(gdbserver_portmap));
-    }
+  GDBRemoteCommunicationServerPlatform platform(
+      acceptor_up->GetSocketProtocol(), acceptor_up->GetSocketScheme());
+  if (port_offset > 0)
+    platform.SetPortOffset(port_offset);
 
+  do {
     const bool children_inherit_accept_socket = true;
     Connection *conn = nullptr;
     error = acceptor_up->Accept(children_inherit_accept_socket, conn);
@@ -301,13 +296,37 @@ int main_platform(int argc, char *argv[]) {
       exit(socket_error);
     }
     printf("Connection established.\n");
+
     if (g_server) {
       // Collect child zombie processes.
 #if !defined(_WIN32)
-      while (waitpid(-1, nullptr, WNOHANG) > 0)
-        ;
+      ::pid_t waitResult;
+      while ((waitResult = waitpid(-1, nullptr, WNOHANG)) > 0) {
+        // waitResult is the child pid
+        gdbserver_portmap.FreePortForProcess(waitResult);
+      }
 #endif
-      if (fork()) {
+      // TODO: Clean up portmap for Windows when children die
+      // See https://github.com/llvm/llvm-project/issues/90923
+
+      // After collecting zombie ports, get the next available
+      GDBRemoteCommunicationServerPlatform::PortMap portmap_for_child;
+      llvm::Expected<uint16_t> available_port =
+          gdbserver_portmap.GetNextAvailablePort();
+      if (available_port)
+        portmap_for_child.AllowPort(*available_port);
+      else {
+        llvm::consumeError(available_port.takeError());
+        fprintf(stderr,
+                "no available gdbserver port for connection - dropping...\n");
+        delete conn;
+        continue;
+      }
+      platform.SetPortMap(std::move(portmap_for_child));
+
+      auto childPid = fork();
+      if (childPid) {
+        gdbserver_portmap.AssociatePortWithProcess(*available_port, childPid);
         // Parent doesn't need a connection to the lldb client
         delete conn;
 
@@ -323,7 +342,11 @@ int main_platform(int argc, char *argv[]) {
       // If not running as a server, this process will not accept
       // connections while a connection is active.
       acceptor_up.reset();
+
+      // When not running in server mode, use all available ports
+      platform.SetPortMap(std::move(gdbserver_portmap));
     }
+
     platform.SetConnection(std::unique_ptr<Connection>(conn));
 
     if (platform.IsConnected()) {