Skip to content

Commit 6aed0ab

Browse files
authored
[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.
1 parent 50da768 commit 6aed0ab

File tree

2 files changed

+38
-14
lines changed

2 files changed

+38
-14
lines changed

lldb/docs/use/qemu-testing.rst

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -172,6 +172,7 @@ forwarded for this to work.
172172

173173
.. note::
174174
These options are used to create a "port map" within ``lldb-server``.
175-
Unfortunately this map is not shared across all the processes it may create,
175+
Unfortunately this map is not cleaned up on Windows on connection close,
176176
and across a few uses you may run out of valid ports. To work around this,
177177
restart the platform every so often, especially after running a set of tests.
178+
This is tracked here: https://github.com/llvm/llvm-project/issues/90923

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

Lines changed: 36 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -282,17 +282,12 @@ int main_platform(int argc, char *argv[]) {
282282
}
283283
}
284284

285-
do {
286-
GDBRemoteCommunicationServerPlatform platform(
287-
acceptor_up->GetSocketProtocol(), acceptor_up->GetSocketScheme());
288-
289-
if (port_offset > 0)
290-
platform.SetPortOffset(port_offset);
291-
292-
if (!gdbserver_portmap.empty()) {
293-
platform.SetPortMap(std::move(gdbserver_portmap));
294-
}
285+
GDBRemoteCommunicationServerPlatform platform(
286+
acceptor_up->GetSocketProtocol(), acceptor_up->GetSocketScheme());
287+
if (port_offset > 0)
288+
platform.SetPortOffset(port_offset);
295289

290+
do {
296291
const bool children_inherit_accept_socket = true;
297292
Connection *conn = nullptr;
298293
error = acceptor_up->Accept(children_inherit_accept_socket, conn);
@@ -301,13 +296,37 @@ int main_platform(int argc, char *argv[]) {
301296
exit(socket_error);
302297
}
303298
printf("Connection established.\n");
299+
304300
if (g_server) {
305301
// Collect child zombie processes.
306302
#if !defined(_WIN32)
307-
while (waitpid(-1, nullptr, WNOHANG) > 0)
308-
;
303+
::pid_t waitResult;
304+
while ((waitResult = waitpid(-1, nullptr, WNOHANG)) > 0) {
305+
// waitResult is the child pid
306+
gdbserver_portmap.FreePortForProcess(waitResult);
307+
}
309308
#endif
310-
if (fork()) {
309+
// TODO: Clean up portmap for Windows when children die
310+
// See https://github.com/llvm/llvm-project/issues/90923
311+
312+
// After collecting zombie ports, get the next available
313+
GDBRemoteCommunicationServerPlatform::PortMap portmap_for_child;
314+
llvm::Expected<uint16_t> available_port =
315+
gdbserver_portmap.GetNextAvailablePort();
316+
if (available_port)
317+
portmap_for_child.AllowPort(*available_port);
318+
else {
319+
llvm::consumeError(available_port.takeError());
320+
fprintf(stderr,
321+
"no available gdbserver port for connection - dropping...\n");
322+
delete conn;
323+
continue;
324+
}
325+
platform.SetPortMap(std::move(portmap_for_child));
326+
327+
auto childPid = fork();
328+
if (childPid) {
329+
gdbserver_portmap.AssociatePortWithProcess(*available_port, childPid);
311330
// Parent doesn't need a connection to the lldb client
312331
delete conn;
313332

@@ -323,7 +342,11 @@ int main_platform(int argc, char *argv[]) {
323342
// If not running as a server, this process will not accept
324343
// connections while a connection is active.
325344
acceptor_up.reset();
345+
346+
// When not running in server mode, use all available ports
347+
platform.SetPortMap(std::move(gdbserver_portmap));
326348
}
349+
327350
platform.SetConnection(std::unique_ptr<Connection>(conn));
328351

329352
if (platform.IsConnected()) {

0 commit comments

Comments
 (0)