Make remote-android local ports configurable

[mark2185]

Summary

The local ports for platform connect and attach were always random, this allows the user to configure them.
This is useful for debugging a truly remote android (when the android in question is connected to a remote server).

There is a lengthier discussion on github - https://github.com/llvm/llvm-project/issues/58114

[DavidSpickett]

I don't have the expertise to review, but have one question.

Does this account for the situation where you spawn many gdbserver from the platform and therefore need more ports? Does it even need to? (just guessing that that could have been the reason for selecting random ports, though not for the platform)

[mark2185]

In D136465#3878581, @DavidSpickett wrote:

Does this account for the situation where you spawn many gdbserver from the platform and therefore need more ports? Does it even need to? (just guessing that that could have been the reason for selecting random ports, though not for the platform)

To be honest, I don't know. In all of my testing of practical use cases (i.e. launching an app and debugging it either through CodeLLDB/vscode-lldb or Android Studio), I've only ever seen two ports. One for the platform (when running platform connect), and one for gdb (when trying to attach to a PID).

git blame points to this, as previously it was using the port specified when launching lldb-server for the local port, but it turns out it's better to have a random one and then forward it. And it makes sense, since the device you're trying to debug is very likely plugged into the same machine as you're running ADB on, which is perfect for local development.

But that wasn't good enough for the use case I have.

[DavidSpickett]

Ok, so this change means that the randomisation still happens, unless you override it with these environment variables? This seems like a good way to do it.

Where do these env vars need to be set? On the local machine, on the lldb-server machine, on the device?

[mark2185]

In D136465#3878640, @DavidSpickett wrote:

Ok, so this change means that the randomisation still happens, unless you override it with these environment variables? This seems like a good way to do it.

Exactly, this is completely opt-in. This approach with environment variables was easy enough, and it was used in other things related to adb so it seemed fitting. Maybe it would be better to set it through platform settings, but I'd rather have someone chip in on that.

Also the burden of making sure the ports are actually free is on the user, but I'm guessing the user is aware of this since they are using custom ports.

Where do these env vars need to be set? On the local machine, on the lldb-server machine, on the device?

These are client-side variables, since that is where the user is launching lldb from. adb will only get a request forward tcp:<my-port> tcp:<platform-port> instead of forward tcp:<random> tcp:<platform-port>.

[clayborg]

Is there any way to test this?

lldb/source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.cpp

95–97

remove {} for single line if statements per LLVM coding guidelines

136–138

remove {} for single line if statements per LLVM coding guidelines

[mark2185]

In D136465#3880809, @clayborg wrote:

Is there any way to test this?

Through the test suite? I couldn't find any android platform related tests, and this would require adb to be running in the background, so I'm not sure how to approach it.

As for manually testing, three shells are needed (one for lldb-server, one for lldb, and one to check the ports).

(shell-1) $> adb push /path/to/correct/cpu/arch/lldb-server /data/local/tmp   # (android studio comes with prebuilt servers)
(shell-1) $> adb shell /data/local/tmp/lldb-server platform --listen "*:5555"

(shell-2) $> ANDROID_PLATFORM_LOCAL_PORT=8888 lldb
(lldb) $> platform select remote-android
(lldb) $> platform connect connect://localhost:5555

(shell-3) $> adb forward --list
<phone-serial-ID> tcp:8888 tcp:5555

If one were to omit the ANDROID_PLATFORM_LOCAL_PORT variable, the forwarded port will be random.

As for testing the ANDROID_PLATFORM_LOCAL_GDB_PORT, one would need to:

• install an app on the device
• run the lldb-server as that app ( adb shell run-as com.full.package.App ./lldb-server platform... ) so it can attach to its PID
• after platform connect run attach <PID>
• adb forward --list will now show two forwarded ports
  • the ANDROID_PLATFORM_LOCAL_PORT from before
  • the ANDROID_PLATFORM_LOCAL_GDB_PORT, forwarded to some port on the device (that destination port is also random, unless lldb-server is started with the --gdbserver-port argument, or --min-gdbserver-port and --max-gdbserver-port that limit its possible range)

[mark2185]

Remove {} for single line if statements per LLVM coding guidelines

[mark2185]

Did I just overwrite the initial commit with a new one, instead of just creating a diff based on the two comments?

I'm terribly sorry, should I just squash my two commits and run arc diff --update=D136465 again?

[DavidSpickett]

Yes, somehow, and yes just update again.

Happens to us all, I have learned to not try to be clever with arc and just keep one commit per review.

Tip: if you look in the "Revision Contents" box there is a "history" and you can diff between versions of the patch, if you're ever not sure of what actually changed.

[mark2185]

Make remote-android local ports configurable

The local ports for platform connect and attach were always random, this allows the user to configure them.
This is useful for debugging a truly remote android (when the android in question is connected to a remote server).

[clayborg]

Looks ok to me. I don't do Android stuff on a daily basis. As long as the old way of connecting still works I think this is ok.

It would be great to get more stuff in the Android remove platform connection working at some point. Users still have to manually install things and a lot of the work gets done by the Android Studio IDE kind of like Xcode does. I would love to us add some features in the future:

• allow a lldb_private::Target to have an application bundle FileSpec, which could be pointed to a .apk file for Android and a .ipa file for iOS
• Implement PlatformAndroid::LaunchProcess
  • implement "Status PlatformAndroid::Install(const FileSpec &src, const FileSpec &dst)" and handle APK files from target by doing "adb install" if needed
  • Have PlatformAndroid::LaunchProcess do everything that is needed to debug the app
    • start lldb-server for the new process via the connect to the "lldb-server platform" process
    • forward any ports needed

It would be great at some point to be able to do something like:

(lldb) platform select remote-android
(lldb) platform connect ...
(lldb) target create --app-bundle /path/to/foo.apk
(lldb) run

And have PlatformAndroid take care of everything for us.

[mark2185]

In D136465#3884047, @clayborg wrote:

Looks ok to me. I don't do Android stuff on a daily basis. As long as the old way of connecting still works I think this is ok.

Great! I'd just like to note that I do not have commit access, per the guide's instructions.

It would be great to get more stuff in the Android remove platform connection working at some point. Users still have to manually install things and a lot of the work gets done by the Android Studio IDE kind of like Xcode does. I would love to us add some features in the future:

• allow a lldb_private::Target to have an application bundle FileSpec, which could be pointed to a .apk file for Android and a .ipa file for iOS
• Implement PlatformAndroid::LaunchProcess
  • implement "Status PlatformAndroid::Install(const FileSpec &src, const FileSpec &dst)" and handle APK files from target by doing "adb install" if needed
  • Have PlatformAndroid::LaunchProcess do everything that is needed to debug the app
    • start lldb-server for the new process via the connect to the "lldb-server platform" process
    • forward any ports needed

It would be great at some point to be able to do something like:

(lldb) platform select remote-android
(lldb) platform connect ...
(lldb) target create --app-bundle /path/to/foo.apk
(lldb) run

And have PlatformAndroid take care of everything for us.

That does sound pretty useful, and even Android Studio would benefit from it since currently it just pushes a shell script that launches the lldb-server. I'll see what I can do.

[DavidSpickett]

Great! I'd just like to note that I do not have commit access, per the guide's instructions.

What name/email address do you want on the commit?

[mark2185]

In D136465#3884755, @DavidSpickett wrote:

What name/email address do you want on the commit?

Luka Markušić ([e-mail])

Thanks!

[DavidSpickett]

Landed as https://github.com/llvm/llvm-project/commit/1e210abf9925ad08fb7c79894b4ec5ef8f0ef173.

[DavidSpickett]

Thanks for your efforts here, very much appreciated.

lldb/docs/use/remote.rst

 After this, you should be able to debug normally. You can use the process
 attach to attach to an existing remote process or target create, process launch
 to start a new one. The platform plugin will transparently take care of
 uploading or downloading the executable in order to be able to debug. If your
 application needs additional files, you can transfer them using the platform
 commands: get-file, put-file, mkdir, etc. The environment can be prepared
 further using the platform shell command.
 
+When using the "remote-android" platform, the client LLDB forwards two ports, one
+for connecting to the platform, and another for connecting to the gdbserver.
+The client ports are configurable through the environment variables
+ANDROID_PLATFORM_PORT and ANDROID_PLATFORM_GDB_PORT, respectively.
+
 Launching a locally built process on the remote machine
 -------------------------------------------------------
 
 Install and run in the platform working directory
 *************************************************
 
 To launch a locally built process on the remote system in the platform working
 directory:

lldb/source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.h

protected:
   llvm::Optional<AdbClient::UnixSocketNamespace> m_socket_namespace;
 
   bool LaunchGDBServer(lldb::pid_t &pid, std::string &connect_url) override;
 
   bool KillSpawnedProcess(lldb::pid_t pid) override;
 
   void DeleteForwardPort(lldb::pid_t pid);
 
-  Status MakeConnectURL(const lldb::pid_t pid, const uint16_t remote_port,
+  Status MakeConnectURL(const lldb::pid_t pid, const uint16_t local_port,
+                        const uint16_t remote_port,
                         llvm::StringRef remote_socket_name,
                         std::string &connect_url);
 
 private:
   PlatformAndroidRemoteGDBServer(const PlatformAndroidRemoteGDBServer &) =
       delete;
   const PlatformAndroidRemoteGDBServer &
   operator=(const PlatformAndroidRemoteGDBServer &) = delete;
 };
 
 } // namespace platform_android
 } // namespace lldb_private
 
 #endif // LLDB_SOURCE_PLUGINS_PLATFORM_ANDROID_PLATFORMANDROIDREMOTEGDBSERVER_H

lldb/source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.cpp

bool PlatformAndroidRemoteGDBServer::LaunchGDBServer(lldb::pid_t &pid,
   uint16_t remote_port = 0;
   std::string socket_name;
   if (!m_gdb_client_up->LaunchGDBServer("127.0.0.1", pid, remote_port,
                                         socket_name))
     return false;
 
   Log *log = GetLog(LLDBLog::Platform);
 
-  auto error =
-      MakeConnectURL(pid, remote_port, socket_name.c_str(), connect_url);
+  uint16_t local_port = 0;
+  const char *gdbstub_port = std::getenv("ANDROID_PLATFORM_LOCAL_GDB_PORT");
+  if (gdbstub_port)
+    local_port = std::stoi(gdbstub_port);
+
+  auto error = MakeConnectURL(pid, local_port, remote_port, socket_name.c_str(),
+                              connect_url);
   if (error.Success() && log)
     LLDB_LOGF(log, "gdbserver connect URL: %s", connect_url.c_str());
 
   return error.Success();
 }
 
 bool PlatformAndroidRemoteGDBServer::KillSpawnedProcess(lldb::pid_t pid) {
   assert(IsConnected());
if (parsed_url->hostname != "localhost")
     m_device_id = parsed_url->hostname.str();
 
   m_socket_namespace.reset();
   if (parsed_url->scheme == "unix-connect")
     m_socket_namespace = AdbClient::UnixSocketNamespaceFileSystem;
   else if (parsed_url->scheme == "unix-abstract-connect")
     m_socket_namespace = AdbClient::UnixSocketNamespaceAbstract;
 
+  uint16_t local_port = 0;
+  const char *platform_local_port = std::getenv("ANDROID_PLATFORM_LOCAL_PORT");
+  if (platform_local_port)
+    local_port = std::stoi(platform_local_port);
+
   std::string connect_url;
-  auto error =
-      MakeConnectURL(g_remote_platform_pid, parsed_url->port.value_or(0),
-                     parsed_url->path, connect_url);
+  auto error = MakeConnectURL(g_remote_platform_pid, local_port,
+                              parsed_url->port.value_or(0), parsed_url->path,
+                              connect_url);
 
   if (error.Fail())
     return error;
 
   args.ReplaceArgumentAtIndex(0, connect_url);
 
   Log *log = GetLog(LLDBLog::Platform);
   LLDB_LOGF(log, "Rewritten platform connect URL: %s", connect_url.c_str());
LLDB_LOGF(log,
               "Failed to delete port forwarding (pid=%" PRIu64
               ", port=%d, device=%s): %s",
               pid, port, m_device_id.c_str(), error.AsCString());
   }
   m_port_forwards.erase(it);
 }
 
 Status PlatformAndroidRemoteGDBServer::MakeConnectURL(
-    const lldb::pid_t pid, const uint16_t remote_port,
-    llvm::StringRef remote_socket_name, std::string &connect_url) {
+    const lldb::pid_t pid, const uint16_t local_port,
+    const uint16_t remote_port, llvm::StringRef remote_socket_name,
+    std::string &connect_url) {
   static const int kAttempsNum = 5;
 
   Status error;
+
+  auto forward = [&](const uint16_t local, const uint16_t remote) {
+    error = ForwardPortWithAdb(local, remote, remote_socket_name,
+                               m_socket_namespace, m_device_id);
+    if (error.Success()) {
+      m_port_forwards[pid] = local;
+      std::ostringstream url_str;
+      url_str << "connect://127.0.0.1:" << local;
+      connect_url = url_str.str();
+    }
+    return error;
+  };
+
+  if (local_port != 0)
+    return forward(local_port, remote_port);
+
   // There is a race possibility that somebody will occupy a port while we're
   // in between FindUnusedPort and ForwardPortWithAdb - adding the loop to
   // mitigate such problem.
   for (auto i = 0; i < kAttempsNum; ++i) {
     uint16_t local_port = 0;
     error = FindUnusedPort(local_port);
     if (error.Fail())
       return error;
 
-    error = ForwardPortWithAdb(local_port, remote_port, remote_socket_name,
-                               m_socket_namespace, m_device_id);
-    if (error.Success()) {
-      m_port_forwards[pid] = local_port;
-      std::ostringstream url_str;
-      url_str << "connect://127.0.0.1:" << local_port;
-      connect_url = url_str.str();
+    if (forward(local_port, remote_port).Success())
       break;
-    }
-  }
 
   return error;
 }
 
 lldb::ProcessSP PlatformAndroidRemoteGDBServer::ConnectProcess(
     llvm::StringRef connect_url, llvm::StringRef plugin_name,
     lldb_private::Debugger &debugger, lldb_private::Target *target,
     lldb_private::Status &error) {
   // We don't have the pid of the remote gdbserver when it isn't started by us
   // but we still want to store the list of port forwards we set up in our port
   // forward map. Generate a fake pid for these cases what won't collide with
   // any other valid pid on android.
   static lldb::pid_t s_remote_gdbserver_fake_pid = 0xffffffffffffffffULL;
 
   llvm::Optional<URI> parsed_url = URI::Parse(connect_url);
   if (!parsed_url) {
     error.SetErrorStringWithFormat("Invalid URL: %s",
                                    connect_url.str().c_str());
     return nullptr;
   }
 
   std::string new_connect_url;
-  error = MakeConnectURL(s_remote_gdbserver_fake_pid--,
+  error = MakeConnectURL(s_remote_gdbserver_fake_pid--, 0,
                          parsed_url->port.value_or(0), parsed_url->path,
                          new_connect_url);
   if (error.Fail())
     return nullptr;
 
   return PlatformRemoteGDBServer::ConnectProcess(new_connect_url, plugin_name,
                                                  debugger, target, error);
 }