Make ModuleCache::Get to return instantiated ModuleSP instance so already created in-memory instance can be returned instead of creating a new one.

[ovyalov]

Summary

ModuleCache::Get used to return a new ModuleSP instance for the same module spec that may lead to multiple loaded modules for the same shared library.
This CL addresses this issue by storing weak Module pointers within map<UUID, weak_ptr<Module>> - if ModuleCache::Get finds already created module by UUID and it's not null then an existing module instance will be returned.

I'm planning to use this change in future to enforce LRU policy on cache - with map of loaded modules at hand, it will be easy to detect in-use modules.

[clayborg]

Use lldb::ModuleWP instead of std::weak_ptr<Module> and this is good to go.

source/Utility/ModuleCache.h

69

Use lldb::ModuleWP instead of std::weak_ptr<Module>.

[ovyalov]

Oops - didn't notice WP forward declaration. Fixed.
Please take another look.

Thank you!

[clayborg]

Looks good.

[ovyalov]

AFFECTED FILES

/lldb/trunk/include/lldb/Target/Platform.h
/lldb/trunk/source/Target/Platform.cpp
/lldb/trunk/source/Utility/ModuleCache.cpp
/lldb/trunk/source/Utility/ModuleCache.h

USERS

ovyalov (Author)

http://reviews.llvm.org/rL232075

include/lldb/Target/Platform.h

protected:
 
         Error
         DownloadModuleSlice (const FileSpec& src_file_spec,
                              const uint64_t src_offset,
                              const uint64_t src_size,
                              const FileSpec& dst_file_spec);
 
         bool
-        GetFileFromLocalCache (const ModuleSpec& module_spec,
-                               FileSpec &cached_file_spec);
+        GetModuleFromLocalCache (const ModuleSpec& module_spec,
+                                 lldb::ModuleSP &module_sp);
 
         FileSpec GetModuleCacheRoot ();
 
     private:
         DISALLOW_COPY_AND_ASSIGN (Platform);
     };
 
     

source/Target/Platform.cpp

Platform::GetTrapHandlerSymbolNames ()
     }
     return m_trap_handlers;
 }
 
 bool
 Platform::GetCachedSharedModule (const ModuleSpec &module_spec,
                                  lldb::ModuleSP &module_sp)
 {
-    FileSpec cached_file_spec;
-    if (m_module_cache && GetFileFromLocalCache (module_spec, cached_file_spec))
-    {
-        auto cached_module_spec (module_spec);
-        cached_module_spec.GetFileSpec () = cached_file_spec;
-        cached_module_spec.GetPlatformFileSpec () = module_spec.GetFileSpec ();
-        module_sp.reset (new Module (cached_module_spec));
-
-        return true;
-    }
-    return false;
+    return (m_module_cache && GetModuleFromLocalCache (module_spec, module_sp));
 }
 
 bool
-Platform::GetFileFromLocalCache (const ModuleSpec& module_spec,
-                                 FileSpec &cached_file_spec)
+Platform::GetModuleFromLocalCache (const ModuleSpec& module_spec,
+                                   lldb::ModuleSP &module_sp)
 {
     Log *log = GetLogIfAnyCategoriesSet (LIBLLDB_LOG_PLATFORM);
 
     // Get module information from a target.
     ModuleSpec resolved_module_spec;
     if (!GetModuleSpec (module_spec.GetFileSpec (), module_spec.GetArchitecture (), resolved_module_spec))
         return false;
 
     // Check local cache for a module.
     auto error = m_module_cache->Get (GetModuleCacheRoot (),
                                       GetHostname (),
-                                      resolved_module_spec.GetUUID (),
-                                      resolved_module_spec.GetFileSpec (),
-                                      cached_file_spec);
+                                      resolved_module_spec,
+                                      module_sp);
     if (error.Success ())
         return true;
 
     if (log)
         log->Printf("Platform::%s - module %s not found in local cache: %s",
                     __FUNCTION__, resolved_module_spec.GetUUID ().GetAsString ().c_str (), error.AsCString ());
 
     // Get temporary file name for a downloaded module.
if (error.Fail ())
                         __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 (),
-                                 resolved_module_spec.GetUUID (),
-                                 resolved_module_spec.GetFileSpec (),
+                                 resolved_module_spec,
                                  tmp_download_file_spec);
     if (error.Fail ())
     {
         if (log)
             log->Printf("Platform::%s - failed to put module %s into cache: %s",
                         __FUNCTION__, resolved_module_spec.GetUUID ().GetAsString ().c_str (),
                         error.AsCString ());
         return false;
     }
 
     error = m_module_cache->Get (GetModuleCacheRoot (),
                                  GetHostname (),
-                                 resolved_module_spec.GetUUID (),
-                                 resolved_module_spec.GetFileSpec (),
-                                 cached_file_spec);
+                                 resolved_module_spec,
+                                 module_sp);
     return error.Success ();
 }
 
 Error
 Platform::DownloadModuleSlice (const FileSpec& src_file_spec,
                                const uint64_t src_offset,
                                const uint64_t src_size,
                                const FileSpec& dst_file_spec)

source/Utility/ModuleCache.h

 
 #include "lldb/lldb-types.h"
 #include "lldb/lldb-forward.h"
 
 #include "lldb/Core/Error.h"
 #include "lldb/Host/FileSpec.h"
 
 #include <string>
+#include <unordered_map>
 
 namespace lldb_private {
 
+class Module;
 class UUID;
 
 //----------------------------------------------------------------------
 /// @class ModuleCache ModuleCache.h "Utility/ModuleCache.h"
 /// @brief A module cache class.
 ///
 /// Caches locally modules that are downloaded from remote targets.
 /// Each cached module maintains 2 views:

 //----------------------------------------------------------------------
 
 class ModuleCache
 {
 public:
     Error
     Put (const FileSpec &root_dir_spec,
          const char *hostname,
-         const UUID &uuid,
-         const FileSpec &platform_module_spec,
+         const ModuleSpec &module_spec,
          const FileSpec &tmp_file);
 
     Error
     Get (const FileSpec &root_dir_spec,
          const char *hostname,
-         const UUID &uuid,
-         const FileSpec &platform_module_spec,
-         FileSpec &cached_module_spec);
+         const ModuleSpec &module_spec,
+         lldb::ModuleSP &cached_module_sp);
 
 private:
     static FileSpec
     GetModuleDirectory (const FileSpec &root_dir_spec, const UUID &uuid);
 
     static FileSpec
     GetHostSysRootModulePath (const FileSpec &root_dir_spec, const char *hostname, const FileSpec &platform_module_spec);
 
     static Error
     CreateHostSysRootModuleSymLink (const FileSpec &sysroot_module_path_spec, const FileSpec &module_file_path);
+
+    std::unordered_map<std::string, lldb::ModuleWP> m_loaded_modules;
 };
 
 } // namespace lldb_private
 
 #endif  // utility_ModuleCache_h_

source/Utility/ModuleCache.cpp

 //===--------------------- ModuleCache.cpp ----------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "ModuleCache.h"
 
 #include "lldb/Core/Module.h"
+#include "lldb/Core/ModuleSpec.h"
 #include "lldb/Host/FileSystem.h"
 #include "llvm/Support/FileSystem.h"
 
 #include <assert.h>
 
 #include <cstdio>
 #
 using namespace lldb;
return FileSystem::MakeDirectory (dir_path.GetPath ().c_str (),
                                       eFilePermissionsDirectoryDefault);
 }
 
 }  // namespace
 
 Error
 ModuleCache::Put (const FileSpec &root_dir_spec,
                   const char *hostname,
-                  const UUID &uuid,
-                  const FileSpec &platform_module_spec,
+                  const ModuleSpec &module_spec,
                   const FileSpec &tmp_file)
 {
-    const auto module_spec_dir = GetModuleDirectory (root_dir_spec, uuid);
+    const auto module_spec_dir = GetModuleDirectory (root_dir_spec, module_spec.GetUUID ());
     auto error = MakeDirectory (module_spec_dir);
     if (error.Fail ())
         return error;
 
-    const auto module_file_path = JoinPath (module_spec_dir, platform_module_spec.GetFilename ().AsCString ());
+    const auto module_file_path = JoinPath (module_spec_dir, module_spec.GetFileSpec ().GetFilename ().AsCString ());
 
     const auto tmp_file_path = tmp_file.GetPath ();
     const auto err_code = llvm::sys::fs::copy_file (tmp_file_path.c_str (), module_file_path.GetPath ().c_str ());
     if (err_code)
     {
         error.SetErrorStringWithFormat ("failed to copy file %s to %s: %s",
                                         tmp_file_path.c_str (),
                                         module_file_path.GetPath ().c_str (),
                                         err_code.message ().c_str ());
     }
 
     // Create sysroot link to a module.
-    const auto sysroot_module_path_spec = GetHostSysRootModulePath (root_dir_spec, hostname, platform_module_spec);
+    const auto sysroot_module_path_spec = GetHostSysRootModulePath (root_dir_spec, hostname, module_spec.GetFileSpec ());
     return CreateHostSysRootModuleSymLink (sysroot_module_path_spec, module_file_path);
 }
 
 Error
 ModuleCache::Get (const FileSpec &root_dir_spec,
                   const char *hostname,
-                  const UUID &uuid,
-                  const FileSpec &platform_module_spec,
-                  FileSpec &cached_module_spec)
+                  const ModuleSpec &module_spec,
+                  ModuleSP &cached_module_sp)
 {
-    cached_module_spec.Clear ();
+    const auto find_it = m_loaded_modules.find (module_spec.GetUUID ().GetAsString());
+    if (find_it != m_loaded_modules.end ())
+    {
+        cached_module_sp = (*find_it).second.lock ();
+        if (cached_module_sp)
+            return Error ();
+        m_loaded_modules.erase (find_it);
+    }
 
-    const auto module_spec_dir = GetModuleDirectory (root_dir_spec, uuid);
-    const auto module_file_path = JoinPath (module_spec_dir, platform_module_spec.GetFilename ().AsCString ());
+    const auto module_spec_dir = GetModuleDirectory (root_dir_spec,  module_spec.GetUUID ());
+    const auto module_file_path = JoinPath (module_spec_dir, module_spec.GetFileSpec ().GetFilename ().AsCString ());
 
-    Error error;
     if (!module_file_path.Exists ())
-    {
-        error.SetErrorStringWithFormat ("module %s not found", module_file_path.GetPath ().c_str ());
-        return error;
-    }
-    cached_module_spec = module_file_path;
+        return Error ("module %s not found", module_file_path.GetPath ().c_str ());
 
     // We may have already cached module but downloaded from an another host - in this case let's create a symlink to it.
-    const auto sysroot_module_path_spec = GetHostSysRootModulePath (root_dir_spec, hostname, platform_module_spec);
+    const auto sysroot_module_path_spec = GetHostSysRootModulePath (root_dir_spec, hostname, module_spec.GetFileSpec ());
     if (!sysroot_module_path_spec.Exists ())
-        CreateHostSysRootModuleSymLink (sysroot_module_path_spec, cached_module_spec);
+        CreateHostSysRootModuleSymLink (sysroot_module_path_spec, module_spec.GetFileSpec ());
 
-    return error;
+    auto cached_module_spec (module_spec);
+    cached_module_spec.GetUUID ().Clear ();  // Clear UUID since it may contain md5 content hash instead of real UUID.
+    cached_module_spec.GetFileSpec () = module_file_path;
+    cached_module_spec.GetPlatformFileSpec () = module_spec.GetFileSpec ();
+    cached_module_sp.reset (new Module (cached_module_spec));
+
+    m_loaded_modules.insert (std::make_pair (module_spec.GetUUID ().GetAsString (), cached_module_sp));
+
+    return Error ();
 }
 
 FileSpec
 ModuleCache::GetModuleDirectory (const FileSpec &root_dir_spec, const UUID &uuid)
 {
     const auto modules_dir_spec = JoinPath (root_dir_spec, kModulesSubdir);
     return JoinPath (modules_dir_spec, uuid.GetAsString ().c_str ());
 }