[LLDB][Android] Fix Android serial number handling

[splhack]

Summary

First, recap: what is the Android serial number.
In Android ADB protocal, the serial number following the host-serial: is URL-safe string and it could be a host:port string.

https://cs.android.com/android/platform/superproject/+/763ab7bb17e9c63b238222085e1e69552d978367:packages/modules/adb/sockets.cpp;l=825?q=host-serial:

if (android::base::ConsumePrefix(&service, "host-serial:")) {
    // serial number should follow "host:" and could be a host:port string.

Second, LLDB is not able to connect it to Android device when multiple devices are connected to the host.

(lldb) platform select remote-android
(lldb) platform connect unix-abstract-connect:///data/local/tmp/debug.sock
error: Expected a single connected device, got instead 2 - try setting 'ANDROID_SERIAL'

However setting ANDROID_SERIAL environment variable is not always a viable option.
For instance, VS Code + lldb-server and debugging multiple Android devices at the same time.

The potential solution is to use platform connect URL.

(lldb) platform select remote-android
(lldb) platform connect unix-abstract-connect://emulator-5554/data/local/tmp/debug.sock

This works for USB-connected devices and emulators.
But it does not work for TCP/IP-connected devices.

(lldb) platform select remote-android
(lldb) platform connect unix-abstract-connect://127.0.0.1:5556/data/local/tmp/debug.sock

The reason is that the current code only uses the hostname part of the URL.

https://github.com/llvm/llvm-project/blob/ed9638c44bc0c95314694fb878b21006e6c87510/lldb/source/Plugins/Platform/Android/PlatformAndroid.cpp#L154-L155

So m_device_id will be 127.0.0.1. But this should be 127.0.0.1:5556
Also localhost should not be skipped when the port number is available. m_device_id should be localhost:5556

(lldb) platform connect unix-abstract-connect://localhost:5556/data/local/tmp/debug.sock

Third, the host could be IPv6, for instance [::1].

(lldb) platform connect unix-abstract-connect://[::1]:5556/data/local/tmp/debug.sock

This is a valid URL and serial number [::1]:5556.

The parsed_url does not look it has the information of IPv6 brackets. Thus, GetDeviceIDFromURL checks the raw URL again for the bracket.

Fourth, when a port number is available in the connect URL, MakeConnectURL function create a new TCP/IP forwarding with the port number. When adb devices shows a device with host:port style serial number, the port in the serial number is already connected or forwarded to the adb server. host-serial:host:port should work.

https://github.com/llvm/llvm-project/blob/ed9638c44bc0c95314694fb878b21006e6c87510/lldb/source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.cpp#L191-L192

https://github.com/llvm/llvm-project/blob/ed9638c44bc0c95314694fb878b21006e6c87510/lldb/source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.cpp#L42-L46

[splhack]

This comment was removed by splhack.

[clayborg]

Just wanted to let you know that the "platform connect" parameters are not fixed. Each platform can make their own arguments. The Android platform currently expects a URL that will get forwarded to an internal Connection class that has registered a connection URL prefix ("unix-abstract-connect" in this case). But it doesn't have to be this way.

If the "unix-abstract-connect://emulator-5554/data/local/tmp/debug.sock" is not actually just being passed to the ConnectionFileDescriptor::Connect(...) function in lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp then we might want just modify the "platform connect" in the android platform to accept extra options since each Platform plug-in could have any number of options needed to connect. So instead of adding the device ID to the URL we could use a --device option. Currently we have:

(lldb) platform connect unix-abstract-connect://emulator-5554/data/local/tmp/debug.sock

But this could be:

platform connect --device emulator-5554 unix-abstract-connect://data/local/tmp/debug.sock

Same kind of thing for the TCP address. Not sure if we would specify this with "--device 127.0.0.1:5556" or if we would add a "--tcp 127.0.0.1:5556" option?

So I just wanted to let you know that we don't have to fit all of the connection information into a single URL. I am also not sure if the current android platform code was already doing this kind of thing? The "unix-abstract-connect" is a url that should forward the contents directly to a connection plug-in where we can have connection subclasses register connection URL prefixes that any plug-in can use. So I am not sure if adding the device name to this URL ("unix-abstract-connect://data/local/tmp/debug.sock") is the right way to go.

Let me know what you think and if I have misunderstood anything of how things currently work versus how this patch is modifying things to work.

[splhack]

Either way works as long as we can pass the Android serial number to AdbClient.cpp.

This diff just followed the current code which is actually already working for USB-connected devices and emulator. So adding TCPIP-connected device support here makes sense.

m_device_id = parsed_url->hostname.str();

If platform connect will accept platform-specific flags, potentially it may want these.

• --serial (Android serial number)
• --local-gdb-port (D136465)
• --local-port (D136465)

[splhack]

@clayborg https://reviews.llvm.org/D139332 is the platform Android options version.

I'm happy with either of this connect url version https://reviews.llvm.org/D138164 or platform options version https://reviews.llvm.org/D139332.

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

 //===-- PlatformAndroid.cpp -----------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
if (!m_remote_platform_sp)
     m_remote_platform_sp = PlatformSP(new PlatformAndroidRemoteGDBServer());
 
   const char *url = args.GetArgumentAtIndex(0);
   if (!url)
     return Status("URL is null.");
   llvm::Optional<URI> parsed_url = URI::Parse(url);
   if (!parsed_url)
     return Status("Invalid URL: %s", url);
-  if (parsed_url->hostname != "localhost")
-    m_device_id = parsed_url->hostname.str();
+  m_device_id = PlatformAndroidRemoteGDBServer::GetDeviceIDFromURL(url, parsed_url);
 
   auto error = PlatformLinux::ConnectRemote(args);
   if (error.Success()) {
     AdbClient adb;
     error = AdbClient::CreateByDeviceID(m_device_id, adb);
     if (error.Fail())
       return error;
 

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

 //===-- PlatformAndroidRemoteGDBServer.h ------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 

 
 class PlatformAndroidRemoteGDBServer
     : public platform_gdb_server::PlatformRemoteGDBServer {
 public:
   PlatformAndroidRemoteGDBServer() = default;
 
   ~PlatformAndroidRemoteGDBServer() override;
 
+  static std::string GetDeviceIDFromURL(llvm::StringRef url,
+                                        llvm::Optional<URI> parsed_url);
+
   Status ConnectRemote(Args &args) override;
 
   Status DisconnectRemote() override;
 
   lldb::ProcessSP ConnectProcess(llvm::StringRef connect_url,
                                  llvm::StringRef plugin_name,
                                  lldb_private::Debugger &debugger,
                                  lldb_private::Target *target,

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

 //===-- PlatformAndroidRemoteGDBServer.cpp --------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
static Status FindUnusedPort(uint16_t &port) {
   return error;
 }
 
 PlatformAndroidRemoteGDBServer::~PlatformAndroidRemoteGDBServer() {
   for (const auto &it : m_port_forwards)
     DeleteForwardPortWithAdb(it.second, m_device_id);
 }
 
+std::string PlatformAndroidRemoteGDBServer::GetDeviceIDFromURL(
+    const llvm::StringRef url, llvm::Optional<URI> parsed_url) {
+  const llvm::StringRef kSchemeSepWithIPv6Bracket("://[");
+  auto pos = url.find(kSchemeSepWithIPv6Bracket);
+  std::string hostname;
+  if (pos == std::string::npos)
+    hostname = parsed_url->hostname.str();
+  else
+    hostname = "[" + parsed_url->hostname.str() + "]";
+
+  if (parsed_url->port.hasValue())
+    return hostname + ":" + std::to_string(parsed_url->port.value());
+  if (hostname != "localhost")
+    return hostname;
+  return std::string();
+}
+
 bool PlatformAndroidRemoteGDBServer::LaunchGDBServer(lldb::pid_t &pid,
                                                      std::string &connect_url) {
   assert(IsConnected());
   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;
return Status(
         "\"platform connect\" takes a single argument: <connect-url>");
 
   const char *url = args.GetArgumentAtIndex(0);
   if (!url)
     return Status("URL is null.");
   llvm::Optional<URI> parsed_url = URI::Parse(url);
   if (!parsed_url)
     return Status("Invalid URL: %s", url);
-  if (parsed_url->hostname != "localhost")
-    m_device_id = parsed_url->hostname.str();
+  m_device_id = GetDeviceIDFromURL(url, parsed_url);
 
   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, local_port,
-                              parsed_url->port.value_or(0), parsed_url->path,
-                              connect_url);
+  auto error = MakeConnectURL(g_remote_platform_pid, local_port, 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::ProcessSP PlatformAndroidRemoteGDBServer::ConnectProcess(
   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--, 0,
-                         parsed_url->port.value_or(0), parsed_url->path,
+  error = MakeConnectURL(s_remote_gdbserver_fake_pid--, 0, 0, parsed_url->path,
                          new_connect_url);
   if (error.Fail())
     return nullptr;
 
   return PlatformRemoteGDBServer::ConnectProcess(new_connect_url, plugin_name,
                                                  debugger, target, error);
 }

lldb/unittests/Platform/Android/CMakeLists.txt

 include_directories(${LLDB_SOURCE_DIR}/source/Plugins/Platform/Android)
 
 add_lldb_unittest(AdbClientTests
   AdbClientTest.cpp
+  PlatformAndroidRemoteGDBServerTest.cpp
 
   LINK_LIBS
     lldbPluginPlatformAndroid
   )

lldb/unittests/Platform/Android/PlatformAndroidRemoteGDBServerTest.cpp

+//===-- PlatformAndroidRemoteGDBServerTest.cpp
+//-----------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.h"
+#include "gtest/gtest.h"
+#include <cstdlib>
+
+using namespace lldb;
+using namespace lldb_private;
+
+namespace lldb_private {
+namespace platform_android {
+
+class PlatformAndroidRemoteGDBServerTest : public ::testing::Test {};
+
+TEST(PlatformAndroidRemoteGDBServerTest, GetDeviceIDFromURL) {
+  const static std::vector<std::map<std::string, std::string>> data{
+      {{"url", "unix-abstract-connect:///sock"}, {"serial", ""}},
+      {{"url", "unix-abstract-connect://localhost/sock"}, {"serial", ""}},
+      {{"url", "unix-abstract-connect://emulator-5554/sock"},
+       {"serial", "emulator-5554"}},
+      {{"url", "unix-abstract-connect://localhost:5556/sock"},
+       {"serial", "localhost:5556"}},
+      {{"url", "unix-abstract-connect://192.168.0.1:42/sock"},
+       {"serial", "192.168.0.1:42"}},
+      {{"url", "unix-abstract-connect://[::1]:5557/sock"},
+       {"serial", "[::1]:5557"}},
+      {{"url", "unix-abstract-connect://"
+               "[2001:0db8:85a3:0000:0000:8a2e:0370:7334]:5556/sock"},
+       {"serial", "[2001:0db8:85a3:0000:0000:8a2e:0370:7334]:5556"}},
+  };
+
+  for (const auto &map : data) {
+    const auto &url = map.at("url");
+    llvm::Optional<URI> parsed_url = URI::Parse(url);
+    EXPECT_TRUE(parsed_url);
+    EXPECT_EQ(
+        PlatformAndroidRemoteGDBServer::GetDeviceIDFromURL(url, parsed_url),
+        map.at("serial"));
+  }
+}
+
+} // end namespace platform_android
+} // end namespace lldb_private