[Lldb-commits] [PATCH] D17099: Add reverse-connect functionality to the gdb-remote command
Francis Ricci via lldb-commits
lldb-commits at lists.llvm.org
Thu Feb 11 09:28:57 PST 2016
fjricci added a comment.
Would it be better to add a test in the same commit or a separate one?
Comment at: source/Interpreter/CommandInterpreter.cpp:638
@@ +637,3 @@
+ connect_gdb_remote_cmd_ap->AddRegexCommand("^([^:]+:[[:digit:]]+) reverse$",
+ "process connect --plugin gdb-remote connect://%1 reverse") &&
+ connect_gdb_remote_cmd_ap->AddRegexCommand("^([[:digit:]]+) reverse$",
> how about just using `listen://...` to mean reverse connection?
My concern with doing it this way was that ProcessGDBRemote::ConnectToDebugserver() seems to implicitly require a `connect://...` url, and I didn't want to risk breaking future changes. However, it still works correctly if I pass a `listen://...` url, so it seems like a reasonable change.
Comment at: source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.h:222
@@ -226,1 +221,3 @@
+ ConnectProcess(const char *connect_url, const char *plugin_name, lldb_private::Debugger &debugger,
+ lldb_private::Target *target, lldb_private::Error &error, bool reverse = false) override;
> I don't think we need to pass this `reverse=false` around everywhere. We already support `listen://host:port` url scheme (see e.g. ConnectionFileDescriptor::Connect`). Could you use that to specify connection direction?
I had considered that, but wasn't sure whether parsing the direction out of the url string every time it was needed would be better or worse than passing it around as a bool. I'll make the change.
More information about the lldb-commits