[Lldb-commits] [PATCH] Add Utility/ModuleCache class and integrate it with PlatformGDBRemoteServer - in order to allow modules caching from remote targets.

[Oleksiy Vyalov]

Thank you for comments and suggestions - please see my comments inline:

In http://reviews.llvm.org/D8037#133641, @clayborg wrote:

> See my inline comments and let me know what you think. I think it would be nice if we had a global directory that we used for all locally cached files. The module cache would be writeable, and if someone specifies a system root with "platform select --sysroot=/path/to/read/only/root" the system root would be read only. We then cache files locally into /tmp/<platform-name>/<hostname>. We would store a global repository in /tmp/<platform-name> where we store the value using the UUID:
>
> /tmp/<platform-name>/.cache/<uuid>/<cached-file>
>
> Then in the host specific directory we could symlink to this:
>
> /tmp/<platform-name>/<hostname1>/usr/lib/<symlink-to-cached-file>
>  /tmp/<platform-name>/<hostname2>/usr/lib/<symlink-to-cached-file>
>
> The above two files could be symlinks to the same file. This way if you are connecting to a bunch of somewhat related linux distros, you might be able to share many files between then in your local cache.
>
> So the main ideas here are:
>  1 - allow a read only sysroot to be specified with --sysroot when selecting the platform, or specify it after the fact


Do you mean if --sysroot  is specified we should use modules from /tmp/<platform-name>/<hostname> instead of local libraries and without matching UUIDs for modules in cache and on a remote target?

> 2 - allow a writeable cache that is auto generated for all platforms, but only if they opt into calling Platform::GetCachedSharedModule()


Is it okay to introduce a new property like platform.use-file-cache with default true value to control whether we're allowed to use local caching?

> 3 - implement a global file cache per platform that each hostname can then symlink to

> 

> Let me know what you think.



================
Comment at: include/lldb/Core/ModuleSpec.h:17
@@ -16,2 +16,3 @@
 #include "lldb/Host/FileSpec.h"
+#include "lldb/Host/Mutex.h"
 #include "lldb/Target/PathMappingList.h"
----------------
clayborg wrote:
> Remove this?
I added this include since the class has "mutable Mutex m_mutex;" - otherwise got compilation errors.

================
Comment at: source/Plugins/Platform/POSIX/PlatformPOSIX.h:35-40
@@ +34,8 @@
+
+    lldb_private::Error
+    GetSharedModule (const lldb_private::ModuleSpec &module_spec,
+                     lldb::ModuleSP &module_sp,
+                     const lldb_private::FileSpecList *module_search_paths_ptr,
+                     lldb::ModuleSP *old_module_sp_ptr,
+                     bool *did_create_ptr) override;
+
----------------
clayborg wrote:
> Move this to Platform.h and rename the function to be GetCachedSharedModule? Then all platforms can take advantage of the local cache. It would be nice if we can create a global platform cache that just works for all platforms. The Platform::GetCachedSharedModule() would check the local cache directory for the module first and use it from there and if it isn't there download it via the Platform::GetFile() and update the cache?
I like the idea of having Platform::GetCachedSharedModule. But let me ask you next question - how we can get UUID by path, triple (i.e. call qModuleInfo request) from within Platform? Is it okay to lay out it in following way:
 - Add ModuleSpec Platform::GetModuleSpec(const FileSpec&, const Arch&) method which in default implementation just returns ModuleSpec for local module.
 - Make PlatformGDBRemoteServer::GetModuleSpec to call qModuleInfo request and return info for remote module.
 - PlatformPOSIX and other platforms that have m_remote_platform field will override GetModuleSpec and delegate it to m_remote_platform if not a host.

Does it sound okay to you?

================
Comment at: source/Plugins/Platform/POSIX/PlatformPOSIX.h:127-128
@@ +126,4 @@
+
+    virtual void
+    SetLocalCacheDirectory (const char* local) override;
+
----------------
clayborg wrote:
> Maybe we should just auto compute this path using the current platform name by picking a cache directory from the platform name, hostname, etc. Lets say we have a new setting:
> 
> (lldb) settings set platform.file-cache-directory /tmp
> 
> This setting would default to the current temporary directory, but it could be changed if desired so that it never goes away (some path into your home directory).
> 
> Then we can encode the cache as:
> 
> /tmp/<platform-name>/<hostname>/usr/lib/libc.so
> 
> 
Yes, it might be easier then passing local path through SB API calls.

================
Comment at: source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.h:234-246
@@ -215,1 +233,15 @@
 protected:
+    struct ModuleInfo
+    {
+        lldb_private::UUID m_uuid;
+        lldb_private::ArchSpec m_arch;
+        uint64_t file_offset;
+        uint64_t file_size;
+
+        ModuleInfo ()
+            : file_offset (0),
+              file_size (0)
+        {
+        }
+    };
+
----------------
clayborg wrote:
> Use lldb_private::ModuleSpec instead of a new class?
I thought about this but I see two main problems here:

- Is it safe to put md5 hash (if module doesn't have UUID) into ModuleSpec::m_uuid? I'm concerned about https://github.com/llvm-mirror/lldb/blob/master/source/Core/Module.cpp#L188 which tries to match UUIDs and if  ModuleSpec::m_uuid contains MD5 hash then FindMatchingModuleSpec fails because module's UUID is empty. I can always clear ModuleSpec::m_uuid every time I create new Module instance to avoid this check. As an alternative we may have new ModuleSpec::m_hash field but I don't like this idea.

- New ModuleSpec::m_object_size field will be needed. Is it okay to bring File API into ModuleSpec in order to initialize m_object_size with size of ModuleSpec::m_file?

http://reviews.llvm.org/D8037

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/