[lldb] Have lldb-server assign ports to children in platform mode

[Awfa]

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:

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.

[github-actions]

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be
notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write
permissions for the repository. In which case you can instead tag reviewers by
name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review
by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate
is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Anthony Ha (Awfa)

Changes

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:

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.

For review:

• I haven't used C++ optionals before this, and was going off of surrounding related code. Let me know if I got something wrong.
• I'm not sure where this code could be tested. This was only manually tested by me so far.

---

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

1 Files Affected:

• (modified) lldb/tools/lldb-server/lldb-platform.cpp (+28-13)

diff --git a/lldb/tools/lldb-server/lldb-platform.cpp b/lldb/tools/lldb-server/lldb-platform.cpp
index 3e126584eb25b4..384709ba79b656 100644
--- a/lldb/tools/lldb-server/lldb-platform.cpp
+++ b/lldb/tools/lldb-server/lldb-platform.cpp
@@ -282,17 +282,10 @@ 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());
 
+  do {
     const bool children_inherit_accept_socket = true;
     Connection *conn = nullptr;
     error = acceptor_up->Accept(children_inherit_accept_socket, conn);
@@ -301,13 +294,31 @@ int main_platform(int argc, char *argv[]) {
       exit(socket_error);
     }
     printf("Connection established.\n");
+
+    std::optional<uint16_t> port = 0;
     if (g_server) {
       // Collect child zombie processes.
 #if !defined(_WIN32)
-      while (waitpid(-1, nullptr, WNOHANG) > 0)
-        ;
+      auto waitResult = waitpid(-1, nullptr, WNOHANG);
+      while (waitResult > 0) {
+        // waitResult is the child pid
+        gdbserver_portmap.FreePortForProcess(waitResult);
+        waitResult = waitpid(-1, nullptr, WNOHANG);
+      }
 #endif
-      if (fork()) {
+      llvm::Expected<uint16_t> available_port =
+          gdbserver_portmap.GetNextAvailablePort();
+      if (available_port)
+        port = *available_port;
+
+      else {
+        fprintf(stderr, "no available port for connection - dropping...\n");
+        delete conn;
+        continue;
+      }
+      auto childPid = fork();
+      if (childPid) {
+        gdbserver_portmap.AssociatePortWithProcess(*available_port, childPid);
         // Parent doesn't need a connection to the lldb client
         delete conn;
 
@@ -324,6 +335,10 @@ int main_platform(int argc, char *argv[]) {
       // connections while a connection is active.
       acceptor_up.reset();
     }
+
+    GDBRemoteCommunicationServerPlatform::PortMap portmap_for_child;
+    portmap_for_child.AllowPort(*port);
+    platform.SetPortMap(std::move(portmap_for_child));
     platform.SetConnection(std::unique_ptr<Connection>(conn));
 
     if (platform.IsConnected()) {

[DavidSpickett]

I think this will fix #47549. And make the note at the end of lldb/docs/use/qemu-testing.rst outdated.

For testing we might be able to send the platform the gdbserver spawn packets and check that it doesn't return the same port each time. There are tests in lldb/test/API/tools/lldb-server that expect certain responses. Problem with that is we won't know what port range is available on the test host so what would we pass to the min and max?

Given the only existing tests are for the command line validation, I'd be ok not adding tests for this.

[DavidSpickett]

And make the note at the end of lldb/docs/use/qemu-testing.rst outdated.

Removing this is the last thing to do here.

[Awfa]

And make the note at the end of lldb/docs/use/qemu-testing.rst outdated.

Removing this is the last thing to do here.

Fixed! If this looks good to you, would you also be able to merge it? I don't have write permissions.

[Awfa]

Is there a way to invoke BuildBot on this branch before it's merged in by the way?

I got bit on another PR where it caught some failures I wished were caught before merge.

[DavidSpickett]

Nope, that would be lovely and is closer than ever with the move to GitHub, but not here yet.

[github-actions]

@Awfa Congratulations on having your first Pull Request (PR) merged into the LLVM Project!

Your changes will be combined with recent changes from other authors, then tested
by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR.

Please check whether problems have been caused by your change specifically, as
the builds can include changes from many authors. It is not uncommon for your
change to be included in a build that fails due to someone else's changes, or
infrastructure issues.

How to do this, and the rest of the post-merge process, is covered in detail here.

If your change does cause a problem, it may be reverted, or you can revert it yourself.
This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again.

If you don't get any reports, no action is required from you. Your changes are working as expected, well done!

[vvereschaka]

Hi @Awfa,

we start getting failures for some LLDB API tests after these changes. The tests are running on Jetson AGX/Cortex a78 (aarch64) board with Ubuntu Linux 22.04 on it. The failed tests get failed with the following error message:

AssertionError: No value is not true : Could not create a valid process for a.out: unable to launch a GDB server on 'jetson-agx-2198'

We run these tests in 8 threads. The lldb-server is executing on the target board with the following arguments:

./lldb-server p --log-channels 'lldb all' --listen '*:1234' --server --min-gdbserver-port 1236 --max-gdbserver-port 1246

The build host is Windows, the remote target host is Ubuntu Linux.

Affected tests:

• lldb-api :: commands/process/handle/TestProcessHandle.py
• lldb-api :: commands/target/basic/TestTargetCommand.py
• lldb-api :: functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py
• lldb-api :: functionalities/breakpoint/breakpoint_conditions/TestBreakpointConditions.py
• lldb-api :: functionalities/breakpoint/breakpoint_locations/TestBreakpointLocations.py
• lldb-api :: functionalities/breakpoint/breakpoint_reset_upon_run/TestBreakpointResetUponRun.py
• lldb-api :: functionalities/rerun/TestRerun.py
• lldb-api :: functionalities/rerun_and_expr/TestRerunAndExpr.py
• lldb-api :: functionalities/signal/raise/TestRaise.py
• lldb-api :: source-manager/TestSourceManager.py

Reverting of these changes fixes the problem.

Thanks.
Vlad.