Page MenuHomePhabricator

Add domain socket support to gdb-remote protocol and lldb-server.
ClosedPublic

Authored by ovyalov on Oct 19 2015, 4:31 PM.

Details

Summary

This CL provides logic for running lldb-server on domain sockets and required gdb-remote infrastructure changes (qLaunchGDBServer request in particular).
Main highlights:

  • Make lldb-server to listen either on TCP and domain sockets. If colon is found in socket name (i.e. hostname:port format) lldb-server starts in TCP mode, otherwise uses domain sockets. Further improvement will be to pass a pseudo protocol prefix as part of socket name (tcp://localhost:5555 or unix-domain:///tmp/platform.sock) - this will allow to add new transport types more easily (for example, linux-abstract://socket for Linux abstract sockets).
  • Add socket_name response field to qLaunchGDBServer.
  • 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.
  • In case of Android platform rewrite input platform and gdbserver URLs with TCP URLs - i.e. connect://localhost:$local_port

Diff Detail

Event Timeline

ovyalov updated this revision to Diff 37813.Oct 19 2015, 4:31 PM
ovyalov retitled this revision from to Add domain socket support to gdb-remote protocol and lldb-server..
ovyalov updated this object.
ovyalov added reviewers: clayborg, labath, tberghammer.
ovyalov added a subscriber: lldb-commits.
labath edited edge metadata.Oct 20 2015, 2:57 AM

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?

source/Plugins/Platform/Android/AdbClient.cpp
151

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

source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.h
227–228

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 ?

tools/lldb-server/lldb-gdbserver.cpp
266

I like that this is finally going away.

clayborg accepted this revision.Oct 20 2015, 10:05 AM
clayborg edited edge metadata.

Minor nit over ordering of #include, but other than that it looks good.

source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.cpp
38

llvm includes should go before lldb includes.

This revision is now accepted and ready to land.Oct 20 2015, 10:05 AM
ovyalov updated this revision to Diff 37928.Oct 20 2015, 3:02 PM
ovyalov edited edge metadata.
  • Renamed LaunchGDBserverAndGetPort into LaunchGDBServer within PlatformAndroidRemoteGDBServer/PlatformRemoteGDBServer/GDBRemoteCommunicationClient
  • Fixed includes' order.
  • Added Acceptor::LocalSocketId which returns either TCP port and domain socket name - in order to write this value down into named pipe so it can be used by platform as a synchronization point.

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.

source/Plugins/Platform/Android/AdbClient.cpp
151

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.

source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.h
227–228

Done.

labath accepted this revision.Oct 21 2015, 2:19 AM
labath edited edge metadata.

looks good, thanks.

source/Plugins/Platform/Android/AdbClient.cpp
151

Ok, makes sense.

tberghammer accepted this revision.Oct 21 2015, 3:08 AM
tberghammer edited edge metadata.
tberghammer added inline comments.
source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp
39

Please don't include <thread> here if it isn't absolutely necessary as there is a bug on windows in the header.

If you need it then you will most likely have to add the same workaround I added in rL250833 to make it compile on Windows

ovyalov closed this revision.Oct 21 2015, 12:37 PM

Files:

/lldb/trunk/lldb.xcodeproj/project.pbxproj
/lldb/trunk/source/Host/posix/ConnectionFileDescriptorPosix.cpp
/lldb/trunk/source/Plugins/Platform/Android/AdbClient.cpp
/lldb/trunk/source/Plugins/Platform/Android/AdbClient.h
/lldb/trunk/source/Plugins/Platform/Android/PlatformAndroid.cpp
/lldb/trunk/source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.cpp
/lldb/trunk/source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.h
/lldb/trunk/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp
/lldb/trunk/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.h
/lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
/lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h
/lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
/lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
/lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.cpp
/lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.h
/lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
/lldb/trunk/tools/lldb-server/Acceptor.cpp
/lldb/trunk/tools/lldb-server/Acceptor.h
/lldb/trunk/tools/lldb-server/CMakeLists.txt
/lldb/trunk/tools/lldb-server/lldb-gdbserver.cpp
/lldb/trunk/tools/lldb-server/lldb-platform.cpp

Users:

ovyalov (Author)

http://reviews.llvm.org/rL250933