From 3ef33e69de085cb6bc028b4fc4dd39087631ac12 Mon Sep 17 00:00:00 2001 From: Jonas Devlieghere Date: Fri, 27 Mar 2020 15:02:24 -0700 Subject: [PATCH] [VirtualFileSystem] Support directory entries in the YAMLVFSWriter 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. It's possible to partially 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 problem 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. The FileCollector for example 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 store whether an entry should be emitted as a file or directory. Differential revision: https://reviews.llvm.org/D76670 --- llvm/include/llvm/Support/VirtualFileSystem.h | 9 +++++-- llvm/lib/Support/VirtualFileSystem.cpp | 34 ++++++++++++++++++------ llvm/unittests/Support/VirtualFileSystemTest.cpp | 18 +++++-------- 3 files changed, 39 insertions(+), 22 deletions(-) diff --git a/llvm/include/llvm/Support/VirtualFileSystem.h b/llvm/include/llvm/Support/VirtualFileSystem.h index 9b8d057..f13140c 100644 --- a/llvm/include/llvm/Support/VirtualFileSystem.h +++ b/llvm/include/llvm/Support/VirtualFileSystem.h @@ -506,10 +506,12 @@ getVFSFromYAML(std::unique_ptr Buffer, struct YAMLVFSEntry { template - YAMLVFSEntry(T1 &&VPath, T2 &&RPath) - : VPath(std::forward(VPath)), RPath(std::forward(RPath)) {} + YAMLVFSEntry(T1 &&VPath, T2 &&RPath, bool IsDirectory = false) + : VPath(std::forward(VPath)), RPath(std::forward(RPath)), + IsDirectory(IsDirectory) {} std::string VPath; std::string RPath; + bool IsDirectory = false; }; class VFSFromYamlDirIterImpl; @@ -771,10 +773,13 @@ class YAMLVFSWriter { Optional 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; diff --git a/llvm/lib/Support/VirtualFileSystem.cpp b/llvm/lib/Support/VirtualFileSystem.cpp index 737116c..22b4a08 100644 --- a/llvm/lib/Support/VirtualFileSystem.cpp +++ b/llvm/lib/Support/VirtualFileSystem.cpp @@ -1912,11 +1912,21 @@ UniqueID vfs::getNextVirtualUniqueID() { return UniqueID(std::numeric_limits::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 { @@ -2017,7 +2027,10 @@ void JSONWriter::write(ArrayRef Entries, 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) { @@ -2027,13 +2040,18 @@ void JSONWriter::write(ArrayRef Entries, RPath = RPath.slice(OverlayDirLen, RPath.size()); } - writeEntry(path::filename(Entry.VPath), RPath); + 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()) - OS << ",\n"; - else { + StringRef Dir = + Entry.IsDirectory ? Entry.VPath : path::parent_path(Entry.VPath); + if (Dir == DirStack.back()) { + if (!first_entry_is_directory) { + OS << ",\n"; + first_entry_is_directory = false; + } + } else { while (!DirStack.empty() && !containedIn(DirStack.back(), Dir)) { OS << "\n"; endDirectory(); diff --git a/llvm/unittests/Support/VirtualFileSystemTest.cpp b/llvm/unittests/Support/VirtualFileSystemTest.cpp index 8ac454b..f4e999d 100644 --- a/llvm/unittests/Support/VirtualFileSystemTest.cpp +++ b/llvm/unittests/Support/VirtualFileSystemTest.cpp @@ -2200,20 +2200,14 @@ TEST_F(VFSFromYAMLTest, YAMLVFSWriterTest) { 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); @@ -2236,11 +2230,11 @@ TEST_F(VFSFromYAMLTest, YAMLVFSWriterTest) { 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)); } -- 2.7.4