From f6a0a72dbe8f848bdc21c6546895ad8cf7e251d8 Mon Sep 17 00:00:00 2001 From: Bruno Cardoso Lopes Date: Thu, 12 May 2016 19:13:07 +0000 Subject: [PATCH] [VFS] Reapply #2: Reconstruct the VFS overlay tree for more accurate lookup Reapply r269100 and r269270, reverted due to https://llvm.org/bugs/show_bug.cgi?id=27725. Isolate the testcase that corresponds to the new feature side of this commit and skip it on windows hosts until we find why it does not work on these platforms. Original commit message: The way we currently build the internal VFS overlay representation leads to inefficient path search and might yield wrong answers when asked for recursive or regular directory iteration. Currently, when reading an YAML file, each YAML root entry is placed inside a new root in the filesystem overlay. In the crash reproducer, a simple "@import Foundation" currently maps to 43 roots, and when looking up paths, we traverse a directory tree for each of these different roots, until we find a match (or don't). This has two consequences: - It's slow. - Directory iteration gives incomplete results since it only return results within one root - since contents of the same directory can be declared inside different roots, the result isn't accurate. This is in part fault of the way we currently write out the YAML file when emitting the crash reproducer - we could generate only one root and that would make it fast and correct again. However, we should not rely on how the client writes the YAML, but provide a good internal representation regardless. Build a proper virtual directory tree out of the YAML representation, allowing faster search and proper iteration. Besides the crash reproducer, this potentially benefits other VFS clients. llvm-svn: 269327 --- clang/lib/Basic/VirtualFileSystem.cpp | 81 ++++++++++++++++++- .../unittests/Basic/VirtualFileSystemTest.cpp | 56 +++++++++++++ 2 files changed, 136 insertions(+), 1 deletion(-) diff --git a/clang/lib/Basic/VirtualFileSystem.cpp b/clang/lib/Basic/VirtualFileSystem.cpp index 7cffabc67de0..34d1b0592ddb 100644 --- a/clang/lib/Basic/VirtualFileSystem.cpp +++ b/clang/lib/Basic/VirtualFileSystem.cpp @@ -719,7 +719,13 @@ public: Status S) : Entry(EK_Directory, Name), Contents(std::move(Contents)), S(std::move(S)) {} + RedirectingDirectoryEntry(StringRef Name, Status S) + : Entry(EK_Directory, Name), S(std::move(S)) {} Status getStatus() { return S; } + void addContent(std::unique_ptr Content) { + Contents.push_back(std::move(Content)); + } + Entry *getLastContent() const { return Contents.back().get(); } typedef decltype(Contents)::iterator iterator; iterator contents_begin() { return Contents.begin(); } iterator contents_end() { return Contents.end(); } @@ -747,6 +753,7 @@ public: return UseName == NK_NotSet ? GlobalUseExternalName : (UseName == NK_External); } + NameKind getUseName() const { return UseName; } static bool classof(const Entry *E) { return E->getKind() == EK_File; } }; @@ -1023,6 +1030,70 @@ class RedirectingFileSystemParser { return true; } + Entry *lookupOrCreateEntry(RedirectingFileSystem *FS, StringRef Name, + Entry *ParentEntry = nullptr) { + if (!ParentEntry) { // Look for a existent root + for (const std::unique_ptr &Root : FS->Roots) { + if (Name.equals(Root->getName())) { + ParentEntry = Root.get(); + return ParentEntry; + } + } + } else { // Advance to the next component + auto *DE = dyn_cast(ParentEntry); + for (std::unique_ptr &Content : + llvm::make_range(DE->contents_begin(), DE->contents_end())) { + auto *DirContent = dyn_cast(Content.get()); + if (DirContent && Name.equals(Content->getName())) + return DirContent; + } + } + + // ... or create a new one + std::unique_ptr E = llvm::make_unique( + Name, Status("", getNextVirtualUniqueID(), sys::TimeValue::now(), 0, 0, + 0, file_type::directory_file, sys::fs::all_all)); + + if (!ParentEntry) { // Add a new root to the overlay + FS->Roots.push_back(std::move(E)); + ParentEntry = FS->Roots.back().get(); + return ParentEntry; + } + + auto *DE = dyn_cast(ParentEntry); + DE->addContent(std::move(E)); + return DE->getLastContent(); + } + + void uniqueOverlayTree(RedirectingFileSystem *FS, Entry *SrcE, + Entry *NewParentE = nullptr) { + StringRef Name = SrcE->getName(); + switch (SrcE->getKind()) { + case EK_Directory: { + auto *DE = dyn_cast(SrcE); + assert(DE && "Must be a directory"); + // Empty directories could be present in the YAML as a way to + // describe a file for a current directory after some of its subdir + // is parsed. This only leads to redundant walks, ignore it. + if (!Name.empty()) + NewParentE = lookupOrCreateEntry(FS, Name, NewParentE); + for (std::unique_ptr &SubEntry : + llvm::make_range(DE->contents_begin(), DE->contents_end())) + uniqueOverlayTree(FS, SubEntry.get(), NewParentE); + break; + } + case EK_File: { + auto *FE = dyn_cast(SrcE); + assert(FE && "Must be a file"); + assert(NewParentE && "Parent entry must exist"); + auto *DE = dyn_cast(NewParentE); + DE->addContent(llvm::make_unique( + Name, FE->getExternalContentsPath(), FE->getUseName())); + break; + } + } + } + std::unique_ptr parseEntry(yaml::Node *N, RedirectingFileSystem *FS) { yaml::MappingNode *M = dyn_cast(N); if (!M) { @@ -1225,6 +1296,7 @@ public: }; DenseMap Keys(std::begin(Fields), std::end(Fields)); + std::vector> RootEntries; // Parse configuration and 'roots' for (yaml::MappingNode::iterator I = Top->begin(), E = Top->end(); I != E; @@ -1247,7 +1319,7 @@ public: for (yaml::SequenceNode::iterator I = Roots->begin(), E = Roots->end(); I != E; ++I) { if (std::unique_ptr E = parseEntry(&*I, FS)) - FS->Roots.push_back(std::move(E)); + RootEntries.push_back(std::move(E)); else return false; } @@ -1288,6 +1360,13 @@ public: if (!checkMissingKeys(Top, Keys)) return false; + + // Now that we sucessefully parsed the YAML file, canonicalize the internal + // representation to a proper directory tree so that we can search faster + // inside the VFS. + for (std::unique_ptr &E : RootEntries) + uniqueOverlayTree(FS, E.get()); + return true; } }; diff --git a/clang/unittests/Basic/VirtualFileSystemTest.cpp b/clang/unittests/Basic/VirtualFileSystemTest.cpp index 992f0d886757..a33f2b1f7c2d 100644 --- a/clang/unittests/Basic/VirtualFileSystemTest.cpp +++ b/clang/unittests/Basic/VirtualFileSystemTest.cpp @@ -8,7 +8,9 @@ //===----------------------------------------------------------------------===// #include "clang/Basic/VirtualFileSystem.h" +#include "llvm/ADT/Triple.h" #include "llvm/Support/Errc.h" +#include "llvm/Support/Host.h" #include "llvm/Support/MemoryBuffer.h" #include "llvm/Support/Path.h" #include "llvm/Support/SourceMgr.h" @@ -680,6 +682,12 @@ public: VersionPlusContent += Content.slice(Content.find('{') + 1, StringRef::npos); return getFromYAMLRawString(VersionPlusContent, ExternalFS); } + + // This is intended as a "XFAIL" for windows hosts. + bool supportsSameDirMultipleYAMLEntries() { + Triple Host(Triple::normalize(sys::getProcessTriple())); + return !Host.isOSWindows(); + } }; TEST_F(VFSFromYAMLTest, BasicVFSFromYAML) { @@ -1066,3 +1074,51 @@ TEST_F(VFSFromYAMLTest, DirectoryIteration) { checkContents(O->dir_begin("//root/foo/bar", EC), {"//root/foo/bar/a", "//root/foo/bar/b"}); } + +TEST_F(VFSFromYAMLTest, DirectoryIterationSameDirMultipleEntries) { + // https://llvm.org/bugs/show_bug.cgi?id=27725 + if (!supportsSameDirMultipleYAMLEntries()) + return; + + IntrusiveRefCntPtr Lower(new DummyFileSystem()); + Lower->addDirectory("//root/zab"); + Lower->addDirectory("//root/baz"); + Lower->addRegularFile("//root/zab/a"); + Lower->addRegularFile("//root/zab/b"); + IntrusiveRefCntPtr FS = getFromYAMLString( + "{ 'use-external-names': false,\n" + " 'roots': [\n" + "{\n" + " 'type': 'directory',\n" + " 'name': '//root/baz/',\n" + " 'contents': [ {\n" + " 'type': 'file',\n" + " 'name': 'x',\n" + " 'external-contents': '//root/zab/a'\n" + " }\n" + " ]\n" + "},\n" + "{\n" + " 'type': 'directory',\n" + " 'name': '//root/baz/',\n" + " 'contents': [ {\n" + " 'type': 'file',\n" + " 'name': 'y',\n" + " 'external-contents': '//root/zab/b'\n" + " }\n" + " ]\n" + "}\n" + "]\n" + "}", + Lower); + ASSERT_TRUE(FS.get() != nullptr); + + IntrusiveRefCntPtr O( + new vfs::OverlayFileSystem(Lower)); + O->pushOverlay(FS); + + std::error_code EC; + + checkContents(O->dir_begin("//root/baz/", EC), + {"//root/baz/x", "//root/baz/y"}); +}