[Lldb-commits] [PATCH] Add Utility/ModuleCache class and integrate it with PlatformGDBRemoteServer - in order to allow modules caching from remote targets.
ovyalov at google.com
Tue Mar 3 12:02:44 PST 2015
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:
> Then in the host specific directory we could symlink to this:
> 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 @@
> 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 @@
+ 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;
> 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;
> 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:
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 @@
+ 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)
> 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?
More information about the lldb-commits