[VirtualFileSystem] Support directory entries in the YAMLVFSWriter

[JDevlieghere]

Summary

The current implementation of the JSONWriter does not support writing out directory entries. Earlier today I added a unit test to illustrate the problem. When an entry is added to the YAMLVFSWriter and the path is a directory, it will incorrectly emit the directory as a file, and any files inside that directory will not be found by the VFS.

Usually it's possible to work around the issue by only adding leaf nodes (files) to the YAMLVFSWriter. However, this doesn't work for representing empty directories. This is a real issue for clients of the VFS that want to iterate over a directory. The directory not being there is not the same as the directory being empty.

This is not just a hypothetical problem but a real issue. The FileCollector does not differentiate between file and directory paths. I temporarily worked around the issue for LLDB by ignoring directories, but I suspect this will prove problematic sooner rather than later.

This patch fixes the issue by extending the JSONWriter to support writing out directory entries. We have to special-case the first entry, but otherwise it's just a matter of emitting a directory entry instead of a file entry.

[arphaman]

It looks like you're now adding a dependency on a real filesystem to the VFS writer due to calls to fs::is_directory. I think it should be possible to use a VFS write to construct a VFS file without an underlying filesystem, or with an underlying filesystem that doesn't represent the state of your constructed VFS. Is there another way to achieve this goal without depending on the underlying filesystem?

[JDevlieghere]

In D76670#1946630, @arphaman wrote:

It looks like you're now adding a dependency on a real filesystem to the VFS writer due to calls to fs::is_directory. I think it should be possible to use a VFS write to construct a VFS file without an underlying filesystem, or with an underlying filesystem that doesn't represent the state of your constructed VFS. Is there another way to achieve this goal without depending on the underlying filesystem?

Good point. We could move the check out of the writer and make it the responsibility of the caller, so instead of having just addFile we'd also have an addDirectory. I'll update the patch.

[arphaman]

Great, that looks better, thanks! LGTM

llvm/include/llvm/Support/VirtualFileSystem.h

 IntrusiveRefCntPtr<FileSystem>
 getVFSFromYAML(std::unique_ptr<llvm::MemoryBuffer> Buffer,
                llvm::SourceMgr::DiagHandlerTy DiagHandler,
                StringRef YAMLFilePath, void *DiagContext = nullptr,
                IntrusiveRefCntPtr<FileSystem> ExternalFS = getRealFileSystem());
 
 struct YAMLVFSEntry {
   template <typename T1, typename T2>
-  YAMLVFSEntry(T1 &&VPath, T2 &&RPath)
-      : VPath(std::forward<T1>(VPath)), RPath(std::forward<T2>(RPath)) {}
+  YAMLVFSEntry(T1 &&VPath, T2 &&RPath, bool IsDirectory = false)
+      : VPath(std::forward<T1>(VPath)), RPath(std::forward<T2>(RPath)),
+        IsDirectory(IsDirectory) {}
   std::string VPath;
   std::string RPath;
+  bool IsDirectory = false;
 };
 
 class VFSFromYamlDirIterImpl;
 class RedirectingFileSystemParser;
 
 /// A virtual file system parsed from a YAML file.
 ///
 /// Currently, this class allows creating virtual directories and mapping

 
 class YAMLVFSWriter {
   std::vector<YAMLVFSEntry> Mappings;
   Optional<bool> IsCaseSensitive;
   Optional<bool> IsOverlayRelative;
   Optional<bool> UseExternalNames;
   std::string OverlayDir;
 
+  void addEntry(StringRef VirtualPath, StringRef RealPath, bool IsDirectory);
+
 public:
   YAMLVFSWriter() = default;
 
   void addFileMapping(StringRef VirtualPath, StringRef RealPath);
+  void addDirectoryMapping(StringRef VirtualPath, StringRef RealPath);
 
   void setCaseSensitivity(bool CaseSensitive) {
     IsCaseSensitive = CaseSensitive;
   }
 
   void setUseExternalNames(bool UseExtNames) { UseExternalNames = UseExtNames; }
 
   void setOverlayDir(StringRef OverlayDirectory) {

llvm/lib/Support/VirtualFileSystem.cpp

 UniqueID vfs::getNextVirtualUniqueID() {
   static std::atomic<unsigned> UID;
   unsigned ID = ++UID;
   // The following assumes that uint64_t max will never collide with a real
   // dev_t value from the OS.
   return UniqueID(std::numeric_limits<uint64_t>::max(), ID);
 }
 
-void YAMLVFSWriter::addFileMapping(StringRef VirtualPath, StringRef RealPath) {
+void YAMLVFSWriter::addEntry(StringRef VirtualPath, StringRef RealPath,
+                             bool IsDirectory) {
   assert(sys::path::is_absolute(VirtualPath) && "virtual path not absolute");
   assert(sys::path::is_absolute(RealPath) && "real path not absolute");
   assert(!pathHasTraversal(VirtualPath) && "path traversal is not supported");
-  Mappings.emplace_back(VirtualPath, RealPath);
+  Mappings.emplace_back(VirtualPath, RealPath, IsDirectory);
+}
+
+void YAMLVFSWriter::addFileMapping(StringRef VirtualPath, StringRef RealPath) {
+  addEntry(VirtualPath, RealPath, /*IsDirectory=*/false);
+}
+
+void YAMLVFSWriter::addDirectoryMapping(StringRef VirtualPath,
+                                        StringRef RealPath) {
+  addEntry(VirtualPath, RealPath, /*IsDirectory=*/true);
 }
 
 namespace {
 
 class JSONWriter {
   llvm::raw_ostream &OS;
   SmallVector<StringRef, 16> DirStack;
 
if (IsOverlayRelative.hasValue()) {
     UseOverlayRelative = IsOverlayRelative.getValue();
     OS << "  'overlay-relative': '" << (UseOverlayRelative ? "true" : "false")
        << "',\n";
   }
   OS << "  'roots': [\n";
 
   if (!Entries.empty()) {
     const YAMLVFSEntry &Entry = Entries.front();
-    startDirectory(path::parent_path(Entry.VPath));
+    bool first_entry_is_directory = Entry.IsDirectory;
+    StringRef Dir =
+        first_entry_is_directory ? Entry.VPath : path::parent_path(Entry.VPath);
+    startDirectory(Dir);
 
     StringRef RPath = Entry.RPath;
     if (UseOverlayRelative) {
       unsigned OverlayDirLen = OverlayDir.size();
       assert(RPath.substr(0, OverlayDirLen) == OverlayDir &&
              "Overlay dir must be contained in RPath");
       RPath = RPath.slice(OverlayDirLen, RPath.size());
     }
 
+    if (!first_entry_is_directory)
       writeEntry(path::filename(Entry.VPath), RPath);
 
     for (const auto &Entry : Entries.slice(1)) {
-      StringRef Dir = path::parent_path(Entry.VPath);
-      if (Dir == DirStack.back())
+      StringRef Dir =
+          Entry.IsDirectory ? Entry.VPath : path::parent_path(Entry.VPath);
+      if (Dir == DirStack.back()) {
+        if (!first_entry_is_directory) {
           OS << ",\n";
-      else {
+          first_entry_is_directory = false;
+        }
+      } else {
         while (!DirStack.empty() && !containedIn(DirStack.back(), Dir)) {
           OS << "\n";
           endDirectory();
         }
         OS << ",\n";
         startDirectory(Dir);
       }
       StringRef RPath = Entry.RPath;

llvm/unittests/Support/VirtualFileSystemTest.cpp

TEST_F(VFSFromYAMLTest, YAMLVFSWriterTest) {
   ScopedFile _ab(TestDirectory + "/a/b", "");
   ScopedDir _c(TestDirectory + "/c");
   ScopedFile _cd(TestDirectory + "/c/d", "");
   ScopedDir _e(TestDirectory + "/e");
   ScopedDir _ef(TestDirectory + "/e/f");
   ScopedDir _g(TestDirectory + "/g");
   ScopedFile _h(TestDirectory + "/h", "");
 
-  // This test exposes a bug/shortcoming in the YAMLVFSWriter. Below we call
-  // addFileMapping for _a and _e, which causes _ab and _ef not to exists in
-  // the deserialized file system, because _a and _e got emitted as regular
-  // files. The counter example is _c, if we only call addFileMapping for _cd,
-  // things work as expected.
-
   vfs::YAMLVFSWriter VFSWriter;
-  VFSWriter.addFileMapping(_a.Path, "//root/a");
+  VFSWriter.addDirectoryMapping(_a.Path, "//root/a");
   VFSWriter.addFileMapping(_ab.Path, "//root/a/b");
   VFSWriter.addFileMapping(_cd.Path, "//root/c/d");
-  VFSWriter.addFileMapping(_e.Path, "//root/e");
-  VFSWriter.addFileMapping(_ef.Path, "//root/e/f");
+  VFSWriter.addDirectoryMapping(_e.Path, "//root/e");
+  VFSWriter.addDirectoryMapping(_ef.Path, "//root/e/f");
   VFSWriter.addFileMapping(_g.Path, "//root/g");
-  VFSWriter.addFileMapping(_h.Path, "//root/h");
+  VFSWriter.addDirectoryMapping(_h.Path, "//root/h");
 
   std::string Buffer;
   raw_string_ostream OS(Buffer);
   VFSWriter.write(OS);
   OS.flush();
 
   IntrusiveRefCntPtr<ErrorDummyFileSystem> Lower(new ErrorDummyFileSystem());
   Lower->addDirectory("//root/");
   Lower->addDirectory("//root/a");
   Lower->addRegularFile("//root/a/b");
   Lower->addDirectory("//root/b");
   Lower->addDirectory("//root/c");
   Lower->addRegularFile("//root/c/d");
   Lower->addDirectory("//root/e");
   Lower->addDirectory("//root/e/f");
   Lower->addDirectory("//root/g");
   Lower->addRegularFile("//root/h");
 
   IntrusiveRefCntPtr<vfs::FileSystem> FS = getFromYAMLRawString(Buffer, Lower);
   ASSERT_TRUE(FS.get() != nullptr);
 
   EXPECT_TRUE(FS->exists(_a.Path));
-  EXPECT_FALSE(FS->exists(_ab.Path)); // FIXME: See explanation above.
+  EXPECT_TRUE(FS->exists(_ab.Path));
   EXPECT_TRUE(FS->exists(_c.Path));
   EXPECT_TRUE(FS->exists(_cd.Path));
   EXPECT_TRUE(FS->exists(_e.Path));
-  EXPECT_FALSE(FS->exists(_ef.Path)); // FIXME: See explanation above.
+  EXPECT_TRUE(FS->exists(_ef.Path));
   EXPECT_TRUE(FS->exists(_g.Path));
   EXPECT_TRUE(FS->exists(_h.Path));
 }