Allow multiple simultaneous connections to platform.

[flackr]

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.

[vharron]

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]

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]

• Reap zombie children
• Use consts for child inherits descriptor flags
• Remove g_stay_alive

[flackr]

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]

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]

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).

[clayborg]

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

[flackr]

In D8452#145735, @clayborg wrote:

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]

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.

lldb/trunk/include/lldb/Host/posix/ConnectionFileDescriptorPosix.h

 
 class ConnectionFileDescriptor : public Connection
 {
   public:
     ConnectionFileDescriptor(bool child_processes_inherit = false);
 
     ConnectionFileDescriptor(int fd, bool owns_fd);
 
+    ConnectionFileDescriptor(Socket* socket);
+
     virtual ~ConnectionFileDescriptor();
 
     bool IsConnected() const override;
 
     lldb::ConnectionStatus Connect(const char *s, Error *error_ptr) override;
 
     lldb::ConnectionStatus Disconnect(Error *error_ptr) override;
 
protected:
     std::atomic<bool> m_shutting_down; // This marks that we are shutting down so if we get woken up from
                                        // BytesAvailable to disconnect, we won't try to read again.
     bool m_waiting_for_accept;
     bool m_child_processes_inherit;
 
     std::string m_uri;
 
   private:
+    void InitializeSocket(Socket* socket);
+
     DISALLOW_COPY_AND_ASSIGN(ConnectionFileDescriptor);
 };
 
 } // namespace lldb_private
 
 #endif // liblldb_ConnectionFileDescriptor_h_

lldb/trunk/source/Host/posix/ConnectionFileDescriptorPosix.cpp

ConnectionFileDescriptor::ConnectionFileDescriptor(int fd, bool owns_fd)
 
     Log *log(lldb_private::GetLogIfAnyCategoriesSet(LIBLLDB_LOG_CONNECTION | LIBLLDB_LOG_OBJECT));
     if (log)
         log->Printf("%p ConnectionFileDescriptor::ConnectionFileDescriptor (fd = %i, owns_fd = %i)", static_cast<void *>(this), fd,
                     owns_fd);
     OpenCommandPipe();
 }
 
+ConnectionFileDescriptor::ConnectionFileDescriptor(Socket* socket)
+    : Connection()
+    , m_pipe()
+    , m_mutex(Mutex::eMutexTypeRecursive)
+    , m_shutting_down(false)
+    , m_waiting_for_accept(false)
+    , m_child_processes_inherit(false)
+{
+    InitializeSocket(socket);
+}
+
 ConnectionFileDescriptor::~ConnectionFileDescriptor()
 {
     Log *log(lldb_private::GetLogIfAnyCategoriesSet(LIBLLDB_LOG_CONNECTION | LIBLLDB_LOG_OBJECT));
     if (log)
         log->Printf("%p ConnectionFileDescriptor::~ConnectionFileDescriptor ()", static_cast<void *>(this));
     Disconnect(NULL);
     CloseCommandPipe();
 }
ConnectionFileDescriptor::SocketListenAndAccept(const char *s, Error *error_ptr)
     socket = nullptr;
     error = listening_socket_up->BlockingAccept(s, m_child_processes_inherit, socket);
     listening_socket_up.reset();
     if (error_ptr)
         *error_ptr = error;
     if (error.Fail())
         return eConnectionStatusError;
 
-    m_write_sp.reset(socket);
-    m_read_sp = m_write_sp;
-    if (error.Fail())
-    {
-        return eConnectionStatusError;
-    }
-    StreamString strm;
-    strm.Printf("connect://%s:%u",socket->GetRemoteIPAddress().c_str(), socket->GetRemotePortNumber());
-    m_uri.swap(strm.GetString());
+    InitializeSocket(socket);
     return eConnectionStatusSuccess;
 }
 
 ConnectionStatus
 ConnectionFileDescriptor::ConnectTCP(const char *s, Error *error_ptr)
 {
     Socket *socket = nullptr;
     Error error = Socket::TcpConnect(s, m_child_processes_inherit, socket);
ConnectionFileDescriptor::GetChildProcessesInherit() const
     return m_child_processes_inherit;
 }
 
 void
 ConnectionFileDescriptor::SetChildProcessesInherit(bool child_processes_inherit)
 {
     m_child_processes_inherit = child_processes_inherit;
 }
+
+void
+ConnectionFileDescriptor::InitializeSocket(Socket* socket)
+{
+    m_write_sp.reset(socket);
+    m_read_sp = m_write_sp;
+    StreamString strm;
+    strm.Printf("connect://%s:%u",socket->GetRemoteIPAddress().c_str(), socket->GetRemotePortNumber());
+    m_uri.swap(strm.GetString());
+}

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

 #if defined(__APPLE__)
 #include <netinet/in.h>
 #endif
 #include <signal.h>
 #include <stdint.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
+#include <sys/wait.h>
 
 // C++ Includes
 
 // Other libraries and framework includes
 #include "lldb/Core/Error.h"
 #include "lldb/Core/ConnectionMachPort.h"
 #include "lldb/Core/Debugger.h"
 #include "lldb/Core/StreamFile.h"
 #include "lldb/Host/ConnectionFileDescriptor.h"
 #include "lldb/Host/HostGetOpt.h"
 #include "lldb/Host/OptionParser.h"
+#include "lldb/Host/Socket.h"
 #include "lldb/Interpreter/CommandInterpreter.h"
 #include "lldb/Interpreter/CommandReturnObject.h"
 #include "Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.h"
 #include "Plugins/Process/gdb-remote/ProcessGDBRemoteLog.h"
 
 using namespace lldb;
 using namespace lldb_private;
 
 //----------------------------------------------------------------------
 // option descriptors for getopt_long_only()
 //----------------------------------------------------------------------
 
 static int g_debug = 0;
 static int g_verbose = 0;
-static int g_stay_alive = 0;
+static int g_server = 0;
 
 static struct option g_long_options[] =
 {
     { "debug",              no_argument,        &g_debug,           1   },
     { "verbose",            no_argument,        &g_verbose,         1   },
-    { "stay-alive",         no_argument,        &g_stay_alive,      1   },
     { "listen",             required_argument,  NULL,               'L' },
     { "port-offset",        required_argument,  NULL,               'p' },
     { "gdbserver-port",     required_argument,  NULL,               'P' },
     { "min-gdbserver-port", required_argument,  NULL,               'm' },
     { "max-gdbserver-port", required_argument,  NULL,               'M' },
     { "lldb-command",       required_argument,  NULL,               'c' },
+    { "server",             no_argument,        &g_server,          1   },
     { NULL,                 0,                  NULL,               0   }
 };
 
 #if defined (__APPLE__)
 #define LOW_PORT    (IPPORT_RESERVED)
 #define HIGH_PORT   (IPPORT_HIFIRSTAUTO)
 #else
 #define LOW_PORT    (1024u)
main_platform (int argc, char *argv[])
     GDBRemoteCommunicationServerPlatform::PortMap gdbserver_portmap;
     int min_gdbserver_port = 0;
     int max_gdbserver_port = 0;
     uint16_t port_offset = 0;
     
     std::vector<std::string> lldb_commands;
     bool show_usage = false;
     int option_error = 0;
+    int socket_error = -1;
     
     std::string short_options(OptionParser::GetShortOptionString(g_long_options));
                             
 #if __GLIBC__
     optind = 0;
 #else
     optreset = 1;
     optind = 1;
for (const auto &lldb_command : lldb_commands)
         lldb_private::CommandReturnObject result;
         printf("(lldb) %s\n", lldb_command.c_str());
         debugger_sp->GetCommandInterpreter().HandleCommand(lldb_command.c_str(), eLazyBoolNo, result);
         const char *output = result.GetOutputData();
         if (output && output[0])
             puts(output);
     }
 
+    std::unique_ptr<Socket> listening_socket_up;
+    Socket *socket = nullptr;
+    printf ("Listening for a connection from %s...\n", listen_host_port.c_str());
+    const bool children_inherit_listen_socket = false;
+    error = Socket::TcpListen(listen_host_port.c_str(), children_inherit_listen_socket, socket, NULL);
+    if (error.Fail())
+    {
+        printf("error: %s\n", error.AsCString());
+        exit(socket_error);
+    }
+    listening_socket_up.reset(socket);
 
     do {
         GDBRemoteCommunicationServerPlatform platform;
         
         if (port_offset > 0)
             platform.SetPortOffset(port_offset);
 
         if (!gdbserver_portmap.empty())
         {
             platform.SetPortMap(std::move(gdbserver_portmap));
         }
 
-        if (!listen_host_port.empty())
+        const bool children_inherit_accept_socket = true;
+        socket = nullptr;
+        error = listening_socket_up->BlockingAccept(listen_host_port.c_str(), children_inherit_accept_socket, socket);
+        if (error.Fail())
         {
-            std::unique_ptr<ConnectionFileDescriptor> conn_ap(new ConnectionFileDescriptor());
-            if (conn_ap.get())
-            {
-                std::string connect_url ("listen://");
-                connect_url.append(listen_host_port.c_str());
-
-                printf ("Listening for a connection from %s...\n", listen_host_port.c_str());
-                if (conn_ap->Connect(connect_url.c_str(), &error) == eConnectionStatusSuccess)
-                {
-                    printf ("Connection established.\n");
-                    platform.SetConnection (conn_ap.release());
-                }
-                else
-                {
-                    printf ("error: %s\n", error.AsCString());
-                }
-            }
+            printf ("error: %s\n", error.AsCString());
+            exit(socket_error);
+        }
+        printf ("Connection established.\n");
+        if (g_server)
+            // Collect child zombie processes.
+            while (waitpid(-1, nullptr, WNOHANG) > 0);
+            if (fork())
+                // Parent will continue to listen for new connections.
+                continue;
+                // Child process will handle the connection and exit.
+                g_server = 0;
+                // Listening socket is owned by parent process.
+                listening_socket_up.release();
+        else
+        {
+            // If not running as a server, this process will not accept
+            // connections while a connection is active.
+            listening_socket_up.reset();
+        }
+        platform.SetConnection (new ConnectionFileDescriptor(socket));
 
-            if (platform.IsConnected())
-            {
-                // After we connected, we need to get an initial ack from...
-                if (platform.HandshakeWithClient(&error))
-                {
-                    bool interrupt = false;
-                    bool done = false;
-                    while (!interrupt && !done)
-                    {
-                        if (platform.GetPacketAndSendResponse (UINT32_MAX, error, interrupt, done) != GDBRemoteCommunication::PacketResult::Success)
-                            break;
-                    }
-                    
-                    if (error.Fail())
-                    {
-                        fprintf(stderr, "error: %s\n", error.AsCString());
-                    }
-                }
-                else
-                {
-                    fprintf(stderr, "error: handshake with client failed\n");
-                }
-            }
-        }
-    } while (g_stay_alive);
+    } while (g_server);
 
     fprintf(stderr, "lldb-server exiting...\n");
 
     return 0;
 }