[Lldb-commits] [PATCH] D54617: [Reproducers] Add file provider

[Pavel Labath via Phabricator via lldb-commits]

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