Support remote-android with multiple connected devices.

[chaoren]

Summary

This change introduces a new URL scheme for platform connect:

adb://device-id:port

[ovyalov]

Please see my comments.

source/Plugins/Platform/Android/PlatformAndroid.cpp

189 ↗

(On Diff #24667)

Please check url is not null.

191 ↗

(On Diff #24667)

Could you add URL to error message?

193 ↗

(On Diff #24667)

m_device_id is already cleared at line 177.

200 ↗

(On Diff #24667)

Please remove this comment if it's no longer relevant.

source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.cpp

95 ↗

(On Diff #24667)

Add "adb" constant?

98 ↗

(On Diff #24667)

Could you make such args transformation within PlatformAndroid::ConnectRemote to make PlatformLinux::ConnectRemote accept only "connect://" protocol?

98 ↗

(On Diff #24667)

Could you just update first args item in order to preserve all other args values?

[chaoren]

source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.cpp

95 ↗

(On Diff #24667)

If I make the change in your comment below, this would only appear in PlatformAndroid::ConnectRemote, would it still be necessary?

98 ↗

(On Diff #24667)

Could you make such args transformation within PlatformAndroid::ConnectRemote to make PlatformLinux::ConnectRemote accept only "connect://" protocol?

But then PlatformAndroidRemoteGDBServer::ConnectRemote won't be able to see the adb scheme.

Or are we forgoing the connect scheme for Android altogether? There won't be a way to "just connect to that one device."

[ovyalov]

source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.cpp

98 ↗

(On Diff #24667)

As an alternative we may use "connect://localhost:port" scheme for a single connected device or "connect://localhost:port/device-id" in order to connect to a particular connected device.

[chaoren]

source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.cpp

98 ↗

(On Diff #24667)

:/ I know we're probably going to be the only people using this, but it just doesn't feel right to abuse the URI scheme like this.

Perhaps it would be more correct if we added a case in ConnectionFileDescriptor::Connect to treat adb://device-id:port as connect://localhost:port? Then it wouldn't even be necessary to rewrite the args.

[chaoren]

Update as per discussion.

[chaoren]

Accidentally included the other CL.

[ovyalov]

lgtm

[clayborg]

Looks good.

lldb/trunk/source/Host/posix/ConnectionFileDescriptorPosix.cpp

if (s && s[0])
             // unix://SOCKNAME
             return NamedSocketAccept(s + strlen("accept://"), error_ptr);
         }
         else if (strstr(s, "unix-accept://") == s)
         {
             // unix://SOCKNAME
             return NamedSocketAccept(s + strlen("unix-accept://"), error_ptr);
         }
+        else if (strstr(s, "adb://") == s)
+        {
+            int port = -1;
+            sscanf(s, "adb://%*[^:]:%d", &port);
+            char host_and_port[sizeof("localhost:65535")];
+            snprintf(host_and_port, sizeof(host_and_port), "localhost:%d", port);
+            return ConnectTCP(host_and_port, error_ptr);
+        }
         else if (strstr(s, "connect://") == s)
         {
             return ConnectTCP(s + strlen("connect://"), error_ptr);
         }
         else if (strstr(s, "tcp-connect://") == s)
         {
             return ConnectTCP(s + strlen("tcp-connect://"), error_ptr);
         }

lldb/trunk/source/Plugins/Platform/Android/AdbClient.h

 namespace platform_android {
 
 class AdbClient
 {
 public:
     using DeviceIDList = std::list<std::string>;
 
     static Error
-    CreateByDeviceID (const char* device_id, AdbClient &adb);
+    CreateByDeviceID(const std::string &device_id, AdbClient &adb);
 
     AdbClient () = default;
     explicit AdbClient (const std::string &device_id);
 
     const std::string&
     GetDeviceID() const;
 
     Error

lldb/trunk/source/Plugins/Platform/Android/AdbClient.cpp

 
 const uint32_t kConnTimeout = 10000; // 10 ms
 const char * kOKAY = "OKAY";
 const char * kFAIL = "FAIL";
 
 }  // namespace
 
 Error
-AdbClient::CreateByDeviceID (const char* device_id, AdbClient &adb)
+AdbClient::CreateByDeviceID(const std::string &device_id, AdbClient &adb)
 {
     DeviceIDList connect_devices;
-    auto error = adb.GetDevices (connect_devices);
-    if (error.Fail ())
+    auto error = adb.GetDevices(connect_devices);
+    if (error.Fail())
         return error;
 
-    if (device_id)
+    if (device_id.empty())
     {
-        auto find_it = std::find(connect_devices.begin (), connect_devices.end (), device_id);
-        if (find_it == connect_devices.end ())
-            return Error ("Device \"%s\" not found", device_id);
+        if (connect_devices.size() != 1)
+            return Error("Expected a single connected device, got instead %" PRIu64,
+                    static_cast<uint64_t>(connect_devices.size()));
 
-        adb.SetDeviceID (*find_it);
+        adb.SetDeviceID(connect_devices.front());
     }
     else
     {
-        if (connect_devices.size () != 1)
-            return Error ("Expected a single connected device, got instead %" PRIu64, static_cast<uint64_t>(connect_devices.size ()));
+        auto find_it = std::find(connect_devices.begin(), connect_devices.end(), device_id);
+        if (find_it == connect_devices.end())
+            return Error("Device \"%s\" not found", device_id.c_str());
 
-        adb.SetDeviceID (connect_devices.front ());
+        adb.SetDeviceID(*find_it);
     }
     return error;
 }
 
 AdbClient::AdbClient (const std::string &device_id)
     : m_device_id (device_id)
 {
 }

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

 //===-- PlatformAndroid.cpp -------------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 // C Includes
 // C++ Includes
 // Other libraries and framework includes
 #include "lldb/Core/Log.h"
 #include "lldb/Core/PluginManager.h"
 #include "lldb/Host/HostInfo.h"
+#include "Utility/UriParser.h"
 
 // Project includes
 #include "AdbClient.h"
 #include "PlatformAndroid.h"
 #include "PlatformAndroidRemoteGDBServer.h"
 
 using namespace lldb;
 using namespace lldb_private;

 
 ConstString
 PlatformAndroid::GetPluginName()
 {
     return GetPluginNameStatic(IsHost());
 }
 
 Error
-PlatformAndroid::ConnectRemote (Args& args)
+PlatformAndroid::ConnectRemote(Args& args)
 {
-    m_device_id.clear ();
+    m_device_id.clear();
 
     if (IsHost())
     {
         return Error ("can't connect to the host platform '%s', always connected", GetPluginName().GetCString());
     }
 
     if (!m_remote_platform_sp)
         m_remote_platform_sp = PlatformSP(new PlatformAndroidRemoteGDBServer());
 
-    auto error = PlatformLinux::ConnectRemote (args);
-    if (error.Success ())
+    int port;
+    std::string scheme, host, path;
+    const char *url = args.GetArgumentAtIndex(0);
+    if (!url)
+        return Error("URL is null.");
+    if (!UriParser::Parse(url, scheme, host, port, path))
+        return Error("Invalid URL: %s", url);
+    if (scheme == "adb")
+        m_device_id = host;
+
+    auto error = PlatformLinux::ConnectRemote(args);
+    if (error.Success())
     {
-        // Fetch the device list from ADB and if only 1 device found then use that device
-        // TODO: Handle the case when more device is available
         AdbClient adb;
-        error = AdbClient::CreateByDeviceID (nullptr, adb);
-        if (error.Fail ())
+        error = AdbClient::CreateByDeviceID(m_device_id, adb);
+        if (error.Fail())
             return error;
 
-        m_device_id = adb.GetDeviceID ();
+        m_device_id = adb.GetDeviceID();
     }
     return error;
 }
 
 const char *
 PlatformAndroid::GetCacheHostname ()
 {
     return m_device_id.c_str ();
 }

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

public:
 
     Error
     ConnectRemote (Args& args) override;
 
     Error
     DisconnectRemote () override;
 
 protected:
-    std::map<lldb::pid_t, std::pair<uint16_t, std::string>> m_port_forwards;
+    std::string m_device_id;
+    std::map<lldb::pid_t, uint16_t> m_port_forwards;
 
     uint16_t
     LaunchGDBserverAndGetPort (lldb::pid_t &pid) override;
 
     bool
     KillSpawnedProcess (lldb::pid_t pid) override;
 
     void

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

 
 static const lldb::pid_t g_remote_platform_pid = 0; // Alias for the process id of lldb-platform
 
 static Error
 ForwardPortWithAdb (uint16_t port, std::string& device_id)
 {
     Log *log(GetLogIfAllCategoriesSet (LIBLLDB_LOG_PLATFORM));
 
-    // Fetch the device list from ADB and if only 1 device found then use that device
-    // TODO: Handle the case when more device is available
     AdbClient adb;
-    auto error = AdbClient::CreateByDeviceID (nullptr, adb);
+    auto error = AdbClient::CreateByDeviceID(device_id, adb);
     if (error.Fail ())
         return error;
 
-    device_id = adb.GetDeviceID ();
+    device_id = adb.GetDeviceID();
     if (log)
         log->Printf("Connected to Android device \"%s\"", device_id.c_str ());
 
-    return adb.SetPortForwarding (port);
+    return adb.SetPortForwarding(port);
 }
 
 static Error
 DeleteForwardPortWithAdb (uint16_t port, const std::string& device_id)
 {
     AdbClient adb (device_id);
     return adb.DeletePortForwarding (port);
 }
 
 PlatformAndroidRemoteGDBServer::PlatformAndroidRemoteGDBServer ()
 {
 }
 
 PlatformAndroidRemoteGDBServer::~PlatformAndroidRemoteGDBServer ()
 {
     for (const auto& it : m_port_forwards)
-    {
-        DeleteForwardPortWithAdb (it.second.first, it.second.second);
-    }
+        DeleteForwardPortWithAdb(it.second, m_device_id);
 }
 
 uint16_t
 PlatformAndroidRemoteGDBServer::LaunchGDBserverAndGetPort (lldb::pid_t &pid)
 {
     uint16_t port = m_gdb_client.LaunchGDBserverAndGetPort (pid, "127.0.0.1");
     if (port == 0)
         return port;
 
-    std::string device_id;
-    Error error = ForwardPortWithAdb (port, device_id);
+    Error error = ForwardPortWithAdb(port, m_device_id);
     if (error.Fail ())
         return 0;
 
-    m_port_forwards[pid] = std::make_pair (port, device_id);
+    m_port_forwards[pid] = port;
 
     return port;
 }
 
 bool
 PlatformAndroidRemoteGDBServer::KillSpawnedProcess (lldb::pid_t pid)
 {
     DeleteForwardPort (pid);
     return m_gdb_client.KillSpawnedProcess (pid);
 }
 
 Error
 PlatformAndroidRemoteGDBServer::ConnectRemote (Args& args)
 {
-    if (args.GetArgumentCount () != 1)
-        return Error ("\"platform connect\" takes a single argument: <connect-url>");
-  
+    m_device_id.clear();
+
+    if (args.GetArgumentCount() != 1)
+        return Error("\"platform connect\" takes a single argument: <connect-url>");
     int port;
     std::string scheme, host, path;
     const char *url = args.GetArgumentAtIndex (0);
+    if (!url)
+        return Error("URL is null.");
     if (!UriParser::Parse (url, scheme, host, port, path))
-        return Error ("invalid uri");
+        return Error("Invalid URL: %s", url);
+    if (scheme == "adb")
+        m_device_id = host;
 
-    std::string device_id;
-    Error error = ForwardPortWithAdb (port, device_id);
-    if (error.Fail ())
+    Error error = ForwardPortWithAdb(port, m_device_id);
+    if (error.Fail())
         return error;
 
-    m_port_forwards[g_remote_platform_pid] = std::make_pair (port, device_id);
+    m_port_forwards[g_remote_platform_pid] = port;
 
-    error = PlatformRemoteGDBServer::ConnectRemote (args);
+    error = PlatformRemoteGDBServer::ConnectRemote(args);
     if (error.Fail ())
         DeleteForwardPort (g_remote_platform_pid);
 
     return error;
 }
 
 Error
 PlatformAndroidRemoteGDBServer::DisconnectRemote ()
 {
     DeleteForwardPort (g_remote_platform_pid);
     return PlatformRemoteGDBServer::DisconnectRemote ();
 }
 
 void
 PlatformAndroidRemoteGDBServer::DeleteForwardPort (lldb::pid_t pid)
 {
-    Log *log(GetLogIfAllCategoriesSet (LIBLLDB_LOG_PLATFORM));
+    Log *log(GetLogIfAllCategoriesSet(LIBLLDB_LOG_PLATFORM));
 
-    auto it = m_port_forwards.find (pid);
-    if (it == m_port_forwards.end ())
+    auto it = m_port_forwards.find(pid);
+    if (it == m_port_forwards.end())
         return;
 
-    const auto& forward_val = it->second;
-    const auto error = DeleteForwardPortWithAdb (forward_val.first, forward_val.second);
-    if (error.Fail ()) {
+    const auto port = it->second;
+    const auto error = DeleteForwardPortWithAdb(port, m_device_id);
+    if (error.Fail()) {
         if (log)
-            log->Printf ("Failed to delete port forwarding (pid=%" PRIu64 ", port=%d, device=%s): %s",
-                         pid, forward_val.first, forward_val.second.c_str (), error.AsCString ());
+            log->Printf("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);
+    m_port_forwards.erase(it);
 }

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

RunShellCommand (const char *command,            // Shouldn't be NULL
                      uint32_t timeout_sec) override; // Timeout in seconds to wait for shell program to finish
 
     void
     CalculateTrapHandlerSymbolNames () override;
 
 protected:
     process_gdb_remote::GDBRemoteCommunicationClient m_gdb_client;
     std::string m_platform_description; // After we connect we can get a more complete description of what we are connected to
+    std::string m_platform_scheme;
     std::string m_platform_hostname;
 
     // Launch the lldb-gdbserver on the remote host and return the port it is listening on or 0 on
     // failure. Subclasses should override this method if they want to do extra actions before or
     // after launching the lldb-gdbserver.
     virtual uint16_t
     LaunchGDBserverAndGetPort (lldb::pid_t &pid);
 

lldb/trunk/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp

 #include "Utility/UriParser.h"
 
 using namespace lldb;
 using namespace lldb_private;
 using namespace lldb_private::platform_gdb_server;
 
 static bool g_initialized = false;
 
-static std::string MakeGdbServerUrl (const std::string &platform_hostname, uint16_t port)
+static std::string MakeGdbServerUrl(
+        const std::string &platform_scheme,
+        const std::string &platform_hostname,
+        uint16_t port)
 {
+    const char *override_scheme = getenv("LLDB_PLATFORM_REMOTE_GDB_SERVER_SCHEME");
     const char *override_hostname = getenv("LLDB_PLATFORM_REMOTE_GDB_SERVER_HOSTNAME");
     const char *port_offset_c_str = getenv("LLDB_PLATFORM_REMOTE_GDB_SERVER_PORT_OFFSET");
     int port_offset = port_offset_c_str ? ::atoi(port_offset_c_str) : 0;
     StreamString result;
-    result.Printf("connect://%s:%u",
+    result.Printf("%s://%s:%u",
+            override_scheme ? override_scheme : platform_scheme.c_str(),
             override_hostname ? override_hostname : platform_hostname.c_str(),
             port + port_offset);
     return result.GetString();
 }
 
 void
 PlatformRemoteGDBServer::Initialize ()
 {
PlatformRemoteGDBServer::ConnectRemote (Args& args)
     {
         error.SetErrorStringWithFormat ("the platform is already connected to '%s', execute 'platform disconnect' to close the current connection", 
                                         GetHostname());
     }
     else
     {
         if (args.GetArgumentCount() == 1)
         {
-            const char *url = args.GetArgumentAtIndex(0);
-            m_gdb_client.SetConnection (new ConnectionFileDescriptor());
+            m_gdb_client.SetConnection(new ConnectionFileDescriptor());
             // we're going to reuse the hostname when we connect to the debugserver
-            std::string scheme;
             int port;
             std::string path;
-            if ( !UriParser::Parse(url, scheme, m_platform_hostname, port, path) )
-            {
-                error.SetErrorString("invalid uri");
-                return error;
-            }
+            const char *url = args.GetArgumentAtIndex(0);
+            if (!url)
+                return Error("URL is null.");
+            if (!UriParser::Parse(url, m_platform_scheme, m_platform_hostname, port, path))
+                return Error("Invalid URL: %s", url);
 
             const ConnectionStatus status = m_gdb_client.Connect(url, &error);
             if (status == eConnectionStatusSuccess)
             {
                 if (m_gdb_client.HandshakeWithServer(&error))
                 {
                     m_gdb_client.GetHostInfo();
                     // If a working directory was set prior to connecting, send it down now
if (IsRemote())
                     debugger.GetTargetList().SetSelectedTarget(target);
                     
                     // The darwin always currently uses the GDB remote debugger plug-in
                     // so even when debugging locally we are debugging remotely!
                     process_sp = target->CreateProcess (launch_info.GetListenerForProcess(debugger), "gdb-remote", NULL);
                     
                     if (process_sp)
                     {
-                        std::string connect_url = MakeGdbServerUrl(m_platform_hostname, port);
+                        std::string connect_url =
+                            MakeGdbServerUrl(m_platform_scheme, m_platform_hostname, port);
                         error = process_sp->ConnectRemote (nullptr, connect_url.c_str());
                         // Retry the connect remote one time...
                         if (error.Fail())
                             error = process_sp->ConnectRemote (nullptr, connect_url.c_str());
                         if (error.Success())
                             error = process_sp->Launch(launch_info);
                         else if (debugserver_pid != LLDB_INVALID_PROCESS_ID)
                         {
if (IsRemote())
                     debugger.GetTargetList().SetSelectedTarget(target);
                     
                     // The darwin always currently uses the GDB remote debugger plug-in
                     // so even when debugging locally we are debugging remotely!
                     process_sp = target->CreateProcess (attach_info.GetListenerForProcess(debugger), "gdb-remote", NULL);
                     
                     if (process_sp)
                     {
-                        std::string connect_url = MakeGdbServerUrl(m_platform_hostname, port);
+                        std::string connect_url =
+                            MakeGdbServerUrl(m_platform_scheme, m_platform_hostname, port);
                         error = process_sp->ConnectRemote(nullptr, connect_url.c_str());
                         if (error.Success())
                         {
                             auto listener = attach_info.GetHijackListener();
                             if (listener != nullptr)
                                 process_sp->HijackProcessEvents(listener.get());
                             error = process_sp->Attach(attach_info);
                         }