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

[Pavel Labath via lldb-commits]

labath added a comment.

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?


================
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);
+
----------------
Any special quoting needed here? (e.g. if the socket name contains ':')

Although, I wouldn't be surprised if adb just falls over in that case...

================
Comment at: source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.h:228
@@ -227,3 +227,3 @@
 
-    // 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
+    // Launch the lldb-gdbserver on the remote host and return true it is listening on
+    // Subclasses should override this method if they want to do extra actions before or
----------------
This comment makes no sense.

Also, given the new functionality, the function name isn't the best choice. Maybe simply `LaunchGDBServer`, with the comment saying that the url it listens on is returned through the second argument ?

================
Comment at: tools/lldb-server/lldb-gdbserver.cpp:266
@@ -265,3 +254,1 @@
 
-static lldb::thread_result_t
-ListenThread (lldb::thread_arg_t /* arg */)
----------------
I like that this is finally going away.


http://reviews.llvm.org/D13881