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/
_______________________________________________
lldb-commits mailing list
lldb-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
