Page MenuHomePhabricator

Add Utility/ModuleCache class and integrate it with PlatformGDBRemoteServer - in order to allow modules caching from remote targets.
ClosedPublic

Authored by ovyalov on Mar 3 2015, 10:07 AM.

Details

Reviewers
vharron
clayborg
Summary

It's initial version of ModuleCache which I integrated with existing caching plumbing - using "platform connect -c ..." or SBPlatform::ConnectRemote that takes local cache directory path.
Each module is cached using the following path - ${local_cache_directory}/${UUID}/${platform_module_path}.
ModuleCache::Options has m_max_size field which is no-op now - I'm planning to put LRU logic in with a subsequent CL.

Please let me know whether such approach looks reasonable to you.

Diff Detail

Event Timeline

ovyalov updated this revision to Diff 21113.Mar 3 2015, 10:07 AM
ovyalov retitled this revision from to Add Utility/ModuleCache class and integrate it with PlatformGDBRemoteServer - in order to allow modules caching from remote targets..
ovyalov updated this object.
ovyalov edited the test plan for this revision. (Show Details)
ovyalov added reviewers: clayborg, vharron.
ovyalov added a subscriber: Unknown Object (MLST).
clayborg requested changes to this revision.Mar 3 2015, 10:39 AM
clayborg edited edge metadata.

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
2 - allow a writeable cache that is auto generated for all platforms, but only if they opt into calling Platform::GetCachedSharedModule()
3 - implement a global file cache per platform that each hostname can then symlink to

Let me know what you think.

include/lldb/Core/ModuleSpec.h
17

Remove this?

source/Plugins/Platform/POSIX/PlatformPOSIX.h
35–40

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?

125–126

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

source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.h
220–232

Use lldb_private::ModuleSpec instead of a new class?

233–247

Can we move this into Platform.h and just use virtual API calls to implement this for all platforms?

This revision now requires changes to proceed.Mar 3 2015, 10:39 AM

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

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.

include/lldb/Core/ModuleSpec.h
17

I added this include since the class has "mutable Mutex m_mutex;" - otherwise got compilation errors.

source/Plugins/Platform/POSIX/PlatformPOSIX.h
35–40

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?

125–126

Yes, it might be easier then passing local path through SB API calls.

source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.h
220–232

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?

Like your plan as detailed in your inlined comments and what you agree with from my initial comments. I look forward to seeing this next patch!

source/Plugins/Platform/POSIX/PlatformPOSIX.h
35–40

Yes that would be ok.

ovyalov updated this revision to Diff 21243.Mar 4 2015, 4:48 PM
ovyalov edited edge metadata.

Redesigned CL accordingly to the plan discussed with Greg.
The change has become pretty big - please let me know if you want me to split it down to a few change lists.

Please take another look.
Thank you.

Friendly ping.

Please let me know if you have any suggestions on this CL - otherwise I'm going to submit it soon.

There were inlined comments that weren't addressed. Please check the comments and let me know.

There were inlined comments that weren't addressed. Please check the comments and let me know.

I didn't implement --sysroot option for "platform select" command because I was thinking about doing this in a separate CL - sorry that didn't mention this before.

Inlined comments that didn't get submitted...

include/lldb/Core/ModuleSpec.h
17

Does this need to be in the header file?

source/Core/Debugger.cpp
700–703

Does this play well with the other "platform" settings that are already there?

platform.plugin.linux.use-llgs-for-local (boolean) = false
platform.plugin.darwin-kernel.search-locally-for-kexts (boolean) = true
platform.plugin.darwin-kernel.kext-directories (file-list) =

Sorry, looks like my inlined comments were never submitted. Check the bug again, I think I got them to appear in the public version of the bug...

Please see my comments.

include/lldb/Core/ModuleSpec.h
17

ModuleSpec::m_mutex needs it - https://github.com/llvm-mirror/lldb/blob/master/include/lldb/Core/ModuleSpec.h#L589, otherwise compilation is failing for me.

It seems to me that ModuleSpec has grown pretty big as for inlined class - we may add ModuleSpec.cpp and move methods' implementation over there. In this case we can use std::unique<Mutex> instead of Mutex, so it will be enough to have Mutex forward declaration inside of the header file.

source/Core/Debugger.cpp
700–703

I didn't notice any runtime issues with existing properties - here is the partial output of "settings show" from my Ubuntu:

...
target.process.thread.trace-thread (boolean) = false
platform.use-module-cache (boolean) = true
platform.module-cache-directory (file) = "/usr/local/google/home/ovyalov/lldb/module_cache/lldb"
platform.plugin.linux.use-llgs-for-local (boolean) = true
interpreter.expand-regex-aliases (boolean) = false

....

on OSX:

...
target.process.thread.trace-thread (boolean) = false

platform.use-module-cache (boolean) = true

platform.module-cache-directory (file) = "/Users/ovyalov/lldb/module_cache/lldb"

platform.plugin.linux.use-llgs-for-local (boolean) = true

platform.plugin.darwin-kernel.search-locally-for-kexts (boolean) = true

platform.plugin.darwin-kernel.kext-directories (file-list) =

interpreter.expand-regex-aliases (boolean) = false

interpreter.prompt-on-quit (boolean) = true

Since Platform a base class we're not allowed to use DebuggerInitialize method to initialize settings as PlatformLinux, for instance, does.

Will upload fixed version of the CL shortly to use user's home directory as a root folder for module cache instead of LLDB temp directory (which is cleaned when a process exits).

ovyalov updated this revision to Diff 21519.Mar 9 2015, 2:56 PM
ovyalov edited edge metadata.

Use "~/lldb/module_cache" as module cache root instead of LLDB temporary directory which is cleaned after a process exits.

vharron edited edge metadata.Mar 9 2015, 3:14 PM

.lldb?

clayborg requested changes to this revision.Mar 9 2015, 3:24 PM
clayborg edited edge metadata.

We should make a new temp directory that doesn't get cleaned up after LLDB exits. We could add a new ePathTypeGlobalLLDBTempSystemDir path type and just have it return a path that isn't process specific...

This revision now requires changes to proceed.Mar 9 2015, 3:24 PM
ovyalov updated this revision to Diff 21529.Mar 9 2015, 4:04 PM
ovyalov edited edge metadata.

Added ePathTypeGlobalLLDBTempSystemDir and made Platform to use it as a root cache directory.

Please take another look.

clayborg accepted this revision.Mar 9 2015, 4:59 PM
clayborg edited edge metadata.

Looks good.

This revision is now accepted and ready to land.Mar 9 2015, 4:59 PM
ovyalov closed this revision.Mar 9 2015, 6:19 PM

AFFECTED FILES

/lldb/trunk/include/lldb/Core/ModuleSpec.h
/lldb/trunk/include/lldb/Host/HostInfoBase.h
/lldb/trunk/include/lldb/Target/Platform.h
/lldb/trunk/include/lldb/lldb-enumerations.h
/lldb/trunk/lldb.xcodeproj/project.pbxproj
/lldb/trunk/source/Core/Debugger.cpp
/lldb/trunk/source/Core/UUID.cpp
/lldb/trunk/source/Host/common/HostInfoBase.cpp
/lldb/trunk/source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOSXDYLD.cpp
/lldb/trunk/source/Plugins/ObjectContainer/BSD-Archive/ObjectContainerBSDArchive.cpp
/lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
/lldb/trunk/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
/lldb/trunk/source/Plugins/Platform/FreeBSD/PlatformFreeBSD.cpp
/lldb/trunk/source/Plugins/Platform/FreeBSD/PlatformFreeBSD.h
/lldb/trunk/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp
/lldb/trunk/source/Plugins/Platform/POSIX/PlatformPOSIX.h
/lldb/trunk/source/Plugins/Platform/Windows/PlatformWindows.cpp
/lldb/trunk/source/Plugins/Platform/Windows/PlatformWindows.h
/lldb/trunk/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp
/lldb/trunk/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.h
/lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
/lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp
/lldb/trunk/source/Target/Platform.cpp
/lldb/trunk/source/Utility/CMakeLists.txt
/lldb/trunk/source/Utility/ModuleCache.cpp
/lldb/trunk/source/Utility/ModuleCache.h
/lldb/trunk/test/python_api/hello_world/TestHelloWorld.py

USERS

ovyalov (Author)

http://reviews.llvm.org/rL231734