Page MenuHomePhabricator

Allow multiple simultaneous connections to platform.
ClosedPublic

Authored by flackr on Mar 19 2015, 10:41 AM.

Details

Summary

Adds the --server argument to lldb-server platform which when specified will allow multiple simultaneous connections by forking off to handle each individual connection. This will allow us to run the remote tests in parallel.

Diff Detail

Repository
rL LLVM

Event Timeline

flackr updated this revision to Diff 22278.Mar 19 2015, 10:41 AM
flackr retitled this revision from to Allow multiple simultaneous connections to platform..
flackr updated this object.
flackr edited the test plan for this revision. (Show Details)
flackr set the repository for this revision to rL LLVM.

I like having a separate process per platform connection. It might be a tiny bit less efficient than a threaded solution but there aren't likely to be a significant number of platform connections and it provides isolation in the event of a crash in platform.

tools/lldb-server/lldb-platform.cpp
62 ↗(On Diff #22278)

daemon?

288 ↗(On Diff #22278)

Are you collecting child zombies somewhere?

vharron added inline comments.Mar 19 2015, 11:26 AM
tools/lldb-server/lldb-platform.cpp
55 ↗(On Diff #22278)

I think that --daemon might replace --stay-alive. What do you think?

269 ↗(On Diff #22278)

const bool children_inherit_listen_socket = false;
Error error = Socket::TcpListen(listen_host_port.c_str(), children_inherit_listen_socket, socket, NULL);

279 ↗(On Diff #22278)

const bool children_inherit_accept_socket = true;
error = listening_socket_up->BlockingAccept(listen_host_port.c_str(), children_inherit_accept_socket, socket);

flackr updated this revision to Diff 22291.Mar 19 2015, 12:13 PM
  • Reap zombie children
  • Use consts for child inherits descriptor flags
  • Remove g_stay_alive

Please take another look, thanks.

tools/lldb-server/lldb-platform.cpp
55 ↗(On Diff #22278)

For most purposes, I'm sure it does. I was thinking that you might want to debug platform (i.e. not have to worry about forked processes) over multiple runs but I can't imagine any scenario where that would be necessary. Removed stay-alive.

62 ↗(On Diff #22278)

Correct me if I'm mistaken, but doesn't daemon imply that the process runs in the background, whereas in this case the main (listening) process still runs interactively.

269 ↗(On Diff #22278)

Done.

279 ↗(On Diff #22278)

Done.

288 ↗(On Diff #22278)

Done.

vharron updated this object.Mar 19 2015, 2:04 PM
vharron edited the test plan for this revision. (Show Details)
vharron added a reviewer: clayborg.
vharron edited subscribers, added: Unknown Object (MLST); removed: vharron.

Hi Greg, What do you think about this? supporting multiple connections to lldb-platform will allow us to trivially run the test suite in parallel which is going to be incredibly important to us testing against 6 remote Linux architectures and 6 remote Android architectures.

clayborg edited edge metadata.Mar 23 2015, 5:28 PM

Do we want the have one thread that listens for incoming connections, and once a connection is made, make a new GDBRemoteCommunicationServerPlatform shared pointer and launch a new thread that would communicate over the newly bound port? Then we could have multiple connections to the same lldb-server that could be independent.

Currently this is just a cleanup of the --stay-alive option renaming it to --server. Do we want to take the extra step of allowing multiple simultaneous connections where each connection runs on its own thread? Each GDBRemoteCommunicationServerPlatform object would contain its own state (current working directory, etc).

I don't see how this does multiple connections? Seems like the do/while loop would need to be in a thread?

I don't see how this does multiple connections? Seems like the do/while loop would need to be in a thread?

The key change in this patch is a call to fork() (line 288) to create a new process to handle each connection. The do/while continues on the parent (line 291) immediately listens for new connections (without ever closing the listen socket) while the child process inherits the accepted socket (children_inherit_accept_socket == true), handling it with GDBRemoteCommunicationPlatform and then terminates.

The parent process can then accept as many new connections as it wants forking off a new child process to handle each one.

clayborg accepted this revision.Mar 24 2015, 9:49 AM
clayborg edited edge metadata.

Gotcha. Separate processes are ok as long as there are no race conditions between two GDBRemoteCommunicationServerPlatform instances in different processes. I don't believe we currently have any caches or any other data we store in a temporary cache, so I believe this is ok. If we ever have some cache that more than one instance of GDBRemoteCommunicationServerPlatform would like to use, we might want to then keep the GDBRemoteCommunicationServerPlatform instances in a single process so they can coordinate more easily. But this should do just fine.

This revision is now accepted and ready to land.Mar 24 2015, 9:49 AM
This revision was automatically updated to reflect the committed changes.