[Lldb-commits] [PATCH] D13881: Add domain socket support to gdb-remote protocol and lldb-server.

[Oleksiy Vyalov via lldb-commits]

ovyalov added a comment.

In http://reviews.llvm.org/D13881#270878, @labath wrote:

> I *think* it looks good, but I find it quite hard to review a change of this size.
>  A wise man once said "if your commit description contains bullet points, you are doing too much". I think it would be better to split up changes like this in the future.
>
> > Make PlatformRemoteGDBServer to support retry-based connect - both for ConnectRemote and debug server connect. Since in case of domain sockets lldb-server doesn't write port into named pipe we cannot use it as a synchronization point - so it's possible that qLaunchGDBServer will be received by host before debug server is up and listens on socket.
>
>
> I would like to preserve the invariant that by the time a reply to qLaunchGDBServer is received, the server is up and ready to serve connections. How about we keep this file synchronization protocol even when using domain sockets? The server could write nothing (or the socket path, or whatever) to the file when it is up and ready. That way, we don't  have to increase the number of places that do those silly wait-and-retry loops. What do you think?


I was thinking about sending a full connect URL(instead of port) via pipe from debug server to platform/lldb host - so, it can be less transport type dependent. But there are some problems here - e.g., we cannot compose TCP connect URL properly on server side because only client knows a proper server's host address (if server is sitting behind proxies/firewalls/...). So, ended up with extending Acceptor with GetLocalSocketId method which returns a string either with TCP port or socket path so we can wait until debug server is up in either case.


================
Comment at: source/Plugins/Platform/Android/AdbClient.cpp:151
@@ +150,3 @@
+    char message[PATH_MAX];
+    snprintf (message, sizeof (message), "forward:tcp:%d;localfilesystem:%s", local_port, remote_socket_name);
+
----------------
I suspect adb fails in such situation.
Let me verify it once lldb-server/Acceptor will support protocol-based URL like unix:///tmp/platform.sock. Now it's somewhat problematic because we cannot pass colon as a part of
URL - if colon is found it's treated as $host:$port pattern and TCPSocket is created.

================
Comment at: source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.h:227-228
@@ -226,7 +226,4 @@
     lldb::UnixSignalsSP m_remote_signals_sp;
 
-    // Launch the lldb-gdbserver on the remote host and return the port it is listening on or 0 on
-    // failure. Subclasses should override this method if they want to do extra actions before or
-    // after launching the lldb-gdbserver.
-    virtual uint16_t
-    LaunchGDBserverAndGetPort (lldb::pid_t &pid);
+    // Launch the debug server on the remote host - caller connects to launched
+    // debug server using connect_url.
----------------
Done.


http://reviews.llvm.org/D13881