Use Android device serial number instead of hostname as a target identifier within module cache.

[ovyalov]

Summary

Use Android device serial number instead of hostname as a target identifier within module cache - otherwise modules for all devices will be stored under localhost folder.

[tberghammer]

source/Plugins/Platform/Android/AdbClient.h

33–37

What is your opinion about using a proper factory pattern instead of this combined approach? If we worry about the performance implication caused by querying the device list every time then we can make that optional.

Note: I plan to add a few more factory method to create an AdbClient based on -e (emulator) and -d (device) flags the same way as adb do it.

38–40

Can we remove this function (or make it private)?

[ovyalov]

Marked AdbClient::SetDeviceID as private.

[ovyalov]

Please see my comments.

source/Plugins/Platform/Android/AdbClient.h

33–37

Do you mean to use separate factory methods to attach either to emulator or device?

Performance shouldn't be an issue here since it's one-off call and then device_id is stored inside PlatformAndroid.

Could you give more context why we need to distinguish emulator and device flows? I mean, once we have device_id (either emulator and real device) we can distinguish Android instances even when there are a few connected devices. As for me, from LLDB perspective either emulator or device should be treated identically.

38–40

Made it private.

[tberghammer]

source/Plugins/Platform/Android/AdbClient.h

33–37

I plan to add some flags to "platform connect" in PlatformAndroid to specify which device we want to connect. My plan is to follow the same convention what is used by adb ([-s serial_number] [-e] [-d]) where if serial number is specified then connect based on that, if -e is specified then connect to the only running emulator (fail if more then 1 is running) and if -d is specified the connect to the only running device. Also fail if more then one flag is provided.

[ovyalov]

source/Plugins/Platform/Android/AdbClient.h

33–37

I see - makes sense.

[clayborg]

lgtm

[ovyalov]

AFFECTED FILES

/lldb/trunk/include/lldb/Target/Platform.h
/lldb/trunk/source/Plugins/Platform/Android/AdbClient.cpp
/lldb/trunk/source/Plugins/Platform/Android/AdbClient.h
/lldb/trunk/source/Plugins/Platform/Android/PlatformAndroid.cpp
/lldb/trunk/source/Plugins/Platform/Android/PlatformAndroid.h
/lldb/trunk/source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.cpp
/lldb/trunk/source/Target/Platform.cpp

USERS

ovyalov (Author)

http://reviews.llvm.org/rL233202

include/lldb/Target/Platform.h

protected:
                              Platform &remote_platform);
 
         Error
         DownloadModuleSlice (const FileSpec& src_file_spec,
                              const uint64_t src_offset,
                              const uint64_t src_size,
                              const FileSpec& dst_file_spec);
 
+        virtual const char *
+        GetCacheHostname ();
+
     private:
         typedef std::function<Error (const ModuleSpec &)> ModuleResolver;
 
         Error
         GetRemoteSharedModule (const ModuleSpec &module_spec,
                                Process* process,
                                lldb::ModuleSP &module_sp,
                                const ModuleResolver &module_resolver,

source/Plugins/Platform/Android/AdbClient.h

 
 #include <list>
 #include <string>
 
 // Other libraries and framework includes
 // Project includes
 
 #include "lldb/Core/Error.h"
+#include "lldb/Host/ConnectionFileDescriptor.h"
 
 namespace lldb_private {
 
 class AdbClient
 {
 public:
     using DeviceIDList = std::list<std::string>;
 
+    static Error
+    CreateByDeviceID (const char* device_id, AdbClient &adb);
+
     AdbClient () = default;
     explicit AdbClient (const std::string &device_id);
 
-    void
-    SetDeviceID (const std::string& device_id);
+    const std::string&
+    GetDeviceID() const;
 
     Error
     GetDevices (DeviceIDList &device_list);
 
     Error
     SetPortForwarding (const uint16_t port);
 
     Error
     DeletePortForwarding (const uint16_t port);
 
 private:
     Error
     Connect ();
 
+    void
+    SetDeviceID (const std::string& device_id);
+
     Error
     SendMessage (const std::string &packet);
 
     Error
     SendDeviceMessage (const std::string &packet);
 
     Error
     ReadMessage (std::string &message);

source/Plugins/Platform/Android/AdbClient.cpp

 //===-- AdbClient.cpp -------------------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 // Other libraries and framework includes
-#include "lldb/Host/ConnectionFileDescriptor.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/STLExtras.h"
 
 // Project includes
 #include "AdbClient.h"
 
+#include <algorithm>
 #include <sstream>
 
 using namespace lldb;
 using namespace lldb_private;
 
 namespace {
 
 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)
+{
+    DeviceIDList connect_devices;
+    auto error = adb.GetDevices (connect_devices);
+    if (error.Fail ())
+        return error;
+
+    if (device_id)
+    {
+        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);
+
+        adb.SetDeviceID (*find_it);
+    }
+    else
+    {
+        if (connect_devices.size () != 1)
+            return Error ("Expected a single connected device, got instead %zu", connect_devices.size ());
+
+        adb.SetDeviceID (connect_devices.front ());
+    }
+    return error;
+}
+
 AdbClient::AdbClient (const std::string &device_id)
     : m_device_id (device_id)
 {
 }
 
 void
 AdbClient::SetDeviceID (const std::string& device_id)
 {
     m_device_id = device_id;
 }
 
+const std::string&
+AdbClient::GetDeviceID() const
+{
+    return m_device_id;
+}
+
 Error
 AdbClient::Connect ()
 {
     Error error;
     m_conn.Connect ("connect://localhost:5037", &error);
 
     return error;
 }

source/Plugins/Platform/Android/PlatformAndroid.h

 //===-- PlatformAndroid.h ---------------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef liblldb_PlatformAndroid_h_
 #define liblldb_PlatformAndroid_h_
 
 // C Includes
 // C++ Includes
+
+#include <string>
+
 // Other libraries and framework includes
 // Project includes
 #include "Plugins/Platform/Linux/PlatformLinux.h"
 
 namespace lldb_private {
 
     class PlatformAndroid : public PlatformLinux
     {
public:
 
         //------------------------------------------------------------
         // lldb_private::Platform functions
         //------------------------------------------------------------
 
         lldb_private::Error
         ConnectRemote (lldb_private::Args& args) override;
 
+    protected:
+        const char *
+        GetCacheHostname () override;
+
     private:
+        std::string m_device_id;
         DISALLOW_COPY_AND_ASSIGN (PlatformAndroid);
     };
 } // namespace lldb_private
 
 #endif  // liblldb_PlatformAndroid_h_

source/Plugins/Platform/Android/PlatformAndroid.cpp

 // C Includes
 // C++ Includes
 // Other libraries and framework includes
 #include "lldb/Core/Log.h"
 #include "lldb/Core/PluginManager.h"
 #include "lldb/Host/HostInfo.h"
 
 // Project includes
+#include "AdbClient.h"
 #include "PlatformAndroid.h"
 #include "PlatformAndroidRemoteGDBServer.h"
 
 using namespace lldb;
 using namespace lldb_private;
 
 static uint32_t g_initialize_count = 0;
 

 PlatformAndroid::GetPluginName()
 {
     return GetPluginNameStatic(IsHost());
 }
 
 Error
 PlatformAndroid::ConnectRemote (Args& args)
 {
+    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());
-    return PlatformLinux::ConnectRemote (args);
+
+    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 ())
+            return error;
+
+        m_device_id = adb.GetDeviceID ();
+    }
+    return error;
+}
+
+const char *
+PlatformAndroid::GetCacheHostname ()
+{
+    return m_device_id.c_str ();
 }

source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.cpp

 //===-- PlatformAndroidRemoteGDBServer.cpp ----------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 // Other libraries and framework includes
 #include "lldb/Core/Error.h"
 #include "lldb/Core/Log.h"
-#include "lldb/Host/ConnectionFileDescriptor.h"
-#include "llvm/ADT/StringRef.h"
 
 // Project includes
 #include "AdbClient.h"
 #include "PlatformAndroidRemoteGDBServer.h"
 #include "Utility/UriParser.h"
 
 using namespace lldb;
 using namespace lldb_private;
 
 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(lldb_private::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;
-
-    AdbClient::DeviceIDList connect_devices;
-    auto error = adb.GetDevices (connect_devices);
+    auto error = AdbClient::CreateByDeviceID (nullptr, adb);
     if (error.Fail ())
         return error;
 
-    if (connect_devices.size () != 1)
-        return Error ("Expected a single connected device, got instead %zu", connect_devices.size ());
-
-    device_id = connect_devices.front ();
     if (log)
-        log->Printf("Connected to Android device \"%s\"", device_id.c_str ());
+        log->Printf("Connected to Android device \"%s\"", adb.GetDeviceID ().c_str ());
 
-    adb.SetDeviceID (device_id);
     return adb.SetPortForwarding (port);
 }
 
 static Error
 DeleteForwardPortWithAdb (uint16_t port, const std::string& device_id)
 {
     AdbClient adb (device_id);
     return adb.DeletePortForwarding (port);

source/Target/Platform.cpp

Platform::GetCachedSharedModule (const ModuleSpec &module_spec,
     if (IsHost() ||
         !GetGlobalPlatformProperties ()->GetUseModuleCache ())
         return false;
 
     Log *log = GetLogIfAnyCategoriesSet (LIBLLDB_LOG_PLATFORM);
 
     // Check local cache for a module.
     auto error = m_module_cache->Get (GetModuleCacheRoot (),
-                                      GetHostname (),
+                                      GetCacheHostname (),
                                       module_spec,
                                       module_sp,
                                       did_create_ptr);
     if (error.Success ())
         return true;
 
     if (log)
         log->Printf("Platform::%s - module %s not found in local cache: %s",
if (error.Fail ())
             log->Printf("Platform::%s - failed to download %s to %s: %s",
                         __FUNCTION__, module_spec.GetFileSpec ().GetPath ().c_str (),
                         tmp_download_file_path.c_str (), error.AsCString ());
         return false;
     }
 
     // Put downloaded file into local module cache.
     error = m_module_cache->Put (GetModuleCacheRoot (),
-                                 GetHostname (),
+                                 GetCacheHostname (),
                                  module_spec,
                                  tmp_download_file_spec);
     if (error.Fail ())
     {
         if (log)
             log->Printf("Platform::%s - failed to put module %s into cache: %s",
                         __FUNCTION__, module_spec.GetUUID ().GetAsString ().c_str (),
                         error.AsCString ());
         return false;
     }
 
     error = m_module_cache->Get (GetModuleCacheRoot (),
-                                 GetHostname (),
+                                 GetCacheHostname (),
                                  module_spec,
                                  module_sp,
                                  did_create_ptr);
     return error.Success ();
 }
 
 Error
 Platform::DownloadModuleSlice (const FileSpec& src_file_spec,

 
 FileSpec
 Platform::GetModuleCacheRoot ()
 {
     auto dir_spec = GetGlobalPlatformProperties ()->GetModuleCacheDirectory ();
     dir_spec.AppendPathComponent (GetName ().AsCString ());
     return dir_spec;
 }
+
+const char *
+Platform::GetCacheHostname ()
+{
+    return GetHostname ();
+}