Add reverse-connect functionality to the gdb-remote command

[fjricci]

Summary

Add an optional "reverse" option to the gdb-remote command,
which causes lldb to listen on the specified [host:]port,
waiting for the remote server to reverse-connect back to lldb.

[fjricci]

Accidentally uploaded the wrong commit. Sorry about that

[zturner]

Looks like there's a few inconsistencies with our coding style. Can you please run clang-format on this patch?

[fjricci]

Apply clang-format

[fjricci]

Perhaps I ran clang-format in the wrong configuration, but it looks like it made the function declaration style in include/lldb/Target/Platform.h a bit inconsistent. Is this the desired behavior?

[zturner]

In D17099#349136, @fjricci wrote:

Perhaps I ran clang-format in the wrong configuration, but it looks like it made the function declaration style in include/lldb/Target/Platform.h a bit inconsistent. Is this the desired behavior?

The way it formatted the code is consistent with our rules, so it looks fine to me.

[zturner]

(The problem is that surrounding code was not clang-formatted)

[labath]

I think this would be better if you could specify the connection direction in the url. Also, a test for this functionality would be great.

source/Interpreter/CommandInterpreter.cpp

638

how about just using listen://... to mean reverse connection?

source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.h

222 ↗

(On Diff #47534)

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?

[fjricci]

Would it be better to add a test in the same commit or a separate one?

source/Interpreter/CommandInterpreter.cpp

638

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.

source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.h

222 ↗

(On Diff #47534)

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.

[fjricci]

Refactor to use listen urls

[labath]

I like this more, but I'd like to take it a bit further. I think we won't have to parse the url string at all (see comments).

I believe the best way to avoid breaking (and being broken by) other changes is to make sure this fits in nicely in the existing architecture. Putting this logic into ConnectToDebugServer seems like the logical place.

You can wait with the test until we have settled on the final implementation, but I'd like the test to go in together with the change.

source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp

726 ↗

(On Diff #47681)

If we can get ConnectToDebugserver working with a listen url then we will get rid of this url parsing hack...

1088 ↗

(On Diff #47681)

ConnectionFileDescriptor::Connect seems to be able to handle listen:// urls. What would happen if you just passed the listen url to ConnectToDebugserver

[fjricci]

It looks like passing a listen:// url to ConnectToDebugserver actually works "out of the box". Thanks! I'll work on writing a unit test.

[fjricci]

Remove unnecessary special-case logic for reverse connecting

[fjricci]

This functionality can already be accessed by using the "process connect" command with a listen url directly.

source/Interpreter/CommandInterpreter.cpp

CommandInterpreter::LoadCommandDictionary ()
     {
         if (undisplay_regex_cmd_ap->AddRegexCommand("^([0-9]+)$", "target stop-hook delete %1"))
         {
             CommandObjectSP undisplay_regex_cmd_sp(undisplay_regex_cmd_ap.release());
             m_command_dict[undisplay_regex_cmd_sp->GetCommandName ()] = undisplay_regex_cmd_sp;
         }
     }
 
-    std::unique_ptr<CommandObjectRegexCommand>
-    connect_gdb_remote_cmd_ap(new CommandObjectRegexCommand (*this,
-                                                             "gdb-remote",
-                                                             "Connect to a remote GDB server.  If no hostname is provided, localhost is assumed.",
-                                                             "gdb-remote [<hostname>:]<portnum>",
-                                                             2,
-                                                             0,
-                                                             false));
+    std::unique_ptr<CommandObjectRegexCommand> connect_gdb_remote_cmd_ap(
+        new CommandObjectRegexCommand(*this, "gdb-remote", "Connect to a remote GDB server.  If no "
+                                                           "hostname is provided, localhost is assumed.",
+                                      "gdb-remote [<hostname>:]<portnum> [reverse]", 2, 0, false));
     if (connect_gdb_remote_cmd_ap.get())
     {
-        if (connect_gdb_remote_cmd_ap->AddRegexCommand("^([^:]+:[[:digit:]]+)$", "process connect --plugin gdb-remote connect://%1") &&
-            connect_gdb_remote_cmd_ap->AddRegexCommand("^([[:digit:]]+)$", "process connect --plugin gdb-remote connect://localhost:%1"))
+        if (connect_gdb_remote_cmd_ap->AddRegexCommand("^([^:]+:[[:digit:]]+)$",
+                                                       "process connect --plugin gdb-remote connect://%1") &&
+            connect_gdb_remote_cmd_ap->AddRegexCommand("^([[:digit:]]+)$",
+                                                       "process connect --plugin gdb-remote connect://localhost:%1") &&
+            connect_gdb_remote_cmd_ap->AddRegexCommand("^([^:]+:[[:digit:]]+) reverse$",
+                                                       "process connect --plugin gdb-remote listen://%1") &&
+            connect_gdb_remote_cmd_ap->AddRegexCommand("^([[:digit:]]+) reverse$",
+                                                       "process connect --plugin gdb-remote "
+                                                       "listen://localhost:%1"))
         {
             CommandObjectSP command_sp(connect_gdb_remote_cmd_ap.release());
             m_command_dict[command_sp->GetCommandName ()] = command_sp;
         }
     }
 
     std::unique_ptr<CommandObjectRegexCommand>
     connect_kdp_remote_cmd_ap(new CommandObjectRegexCommand (*this,