labath added a comment.
Overall, I think the patch is pretty good. I made a bunch of inline
suggestions/questions/comments, but they're all fairly minor. From a high-level
view the two comments I have are:
- I am slightly concerned about the non-temporality of this approach. Lldb does
a fair amount of filesystem write operations, and this may be hard to capture
in a static filesystem image. It seems you already had to work around some of
these issues when you skip copying deleted files. I think that's a bridge we
can cross when we reach it, but I'm just saying I see some potential trouble
ahead...
- I think it would be good nice to have a more granular test for this
functionality. The existing test is sort of a sledgehammer (and apparently only
runs on darwin). It is not obvious from it for instance, whether it actually
tests any of the special symlink-handling code you have. (I guess it does,
because you needed to write that code, but it's not clear whether that will
still be true one year from now, or on someone else's machine). It sounds like
it shouldn't be too hard to test the finer details of this implementation via
unit tests, either by going through the FileSystem, or by going straight to the
FileCollector class. What do you think?
================
Comment at: include/lldb/Host/FileSystem.h:47
static void Initialize();
+ static void Initialize(FileCollector *collector);
+ static void Initialize(const FileSpec &mapping);
----------------
If this pointer should always be non-null, maybe this could be a reference?
================
Comment at: lit/Reproducer/TestFileRepro.test:1
+# REQUIRES: system-darwin
+
----------------
Would this test be expected to work on other targets using gdb-remote process
plugin too?
================
Comment at: source/Host/common/FileSystem.cpp:100-101
+ m_fs->getBufferForFile(mapping.GetPath());
+ if (!buffer)
+ return;
+ m_fs = llvm::vfs::getVFSFromYAML(std::move(buffer.get()), nullptr, "");
----------------
Maybe do this in the `Initialize` function, so you can return errors from there?
================
Comment at: source/Host/common/FileSystem.cpp:443
+
+ // If VFS mapped we know the underlying FS is a RedirectingFileSystem.
+ ErrorOr<vfs::Entry *> E =
----------------
What are the chances of making this slightly more official by making
`getVFSFromYAML` return a `IntrusiveRefCntPtr<RedirectingFileSystem>` instead
of a generic pointer?
================
Comment at: source/Initialization/SystemInitializerCommon.cpp:82
+ if (repro::Loader *loader = r.GetLoader()) {
+ if (auto info = loader->GetProviderInfo("files")) {
+ FileSpec vfs_mapping(loader->GetRoot());
----------------
Would it be possible to avoid magic strings here, and use something like
`GetProviderInfo<FileProvider>()` instead?
================
Comment at: source/Initialization/SystemInitializerCommon.cpp:87
+ } else {
+ // No file provider, continue with the real filesystem.
+ FileSystem::Initialize();
----------------
When can this happen? Should this be an error or maybe even an assert?
Repository:
rLLDB LLDB
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D54617/new/
https://reviews.llvm.org/D54617
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
