r269100 - [VFS] Reconstruct the VFS overlay tree for more accurate lookup
Sean Silva via cfe-commits
cfe-commits at lists.llvm.org
Tue May 10 22:27:47 PDT 2016
On Tue, May 10, 2016 at 9:49 PM, Bruno Cardoso Lopes <
bruno.cardoso at gmail.com> wrote:
> Thanks Sean,
>
> I was about to ask for help and try a 4th attempt, as you noted after
> 3 attempts I was not yet able to make this work on windows though...
>
So sorry! It had gone quiet for a few hours and I wanted to avoid leaving
it red any longer.
> The new tests I added first failed on windows because looks like a
> missing "/" on the dir name forces "\\" to be generated while
> appending the filename instead of "/". After fixing that, now seems
> like the directory iteration on windows has a different order from
> what we are getting on the other platforms, do you know anything about
> this?
Does any platform define a guaranteed directory iteration order? I feel
like depending on it in the first place is unwise (or maybe I'm
misunderstanding? e.g. I'm thinking that we should sort the results of
directory iteration to guarantee a particular order if needed for a
comparison in a test)
> Or can you help me debug it once I land it again?
Sure, but like I mentioned the nature of this error is very troubling to me
(high-level API use running into low-level system issues). It suggests that
there is an incorrect layering or something. I hate these annoying windows
path issues as much as everyone else, so I'd rather we focus energy on
building an appropriate layering to consolidate all the special handling in
one place than to continually band-aid issues as they creep into all parts
of the codebase. VirtualFileSystem.cpp already has quite a bit of technical
debt that violates our policies in this regard (platform-specific #ifdef's
outside libSupport; I count 3 in VirtualFileSystem.cpp which is a huge
number considering how recent that code is).
> I might
> check-in the new tests separately from the rest to make it easier to
> revert.
>
I don't think there's precedent for doing this in LLVM (or at least it
would be considered as bad practice). We typically check in tests alongside
the code being tested specifically to avoid the temptation to leave
untested/broken code in tree.
-- Sean Silva
>
>
> ******************** TEST 'Clang-Unit ::
> Basic/BasicTests.exe/VFSFromYAMLTest.DirectoryIteration' FAILED
> ********************
> Note: Google Test filter = VFSFromYAMLTest.DirectoryIteration
> [==========] Running 1 test from 1 test case.
> [----------] Global test environment set-up.
> [----------] 1 test from VFSFromYAMLTest
> [ RUN ] VFSFromYAMLTest.DirectoryIteration
>
>
> C:\Buildbot\Slave\llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast\llvm.src\tools\clang\unittests\Basic\VirtualFileSystemTest.cpp(379):
> error: Value of: I->getName()
>
> Actual: { '/' (47, 0x2F), '/' (47, 0x2F), 'r' (114, 0x72), 'o' (111,
> 0x6F), 'o' (111, 0x6F), 't' (116, 0x74), '/' (47, 0x2F), 'b' (98,
> 0x62), 'a' (97, 0x61), 'z' (122, 0x7A), '/' (47, 0x2F), 'c' (99, 0x63)
> }
>
> Expected: *ExpectedIter
>
> Which is: { '/' (47, 0x2F), '/' (47, 0x2F), 'r' (114, 0x72), 'o' (111,
> 0x6F), 'o' (111, 0x6F), 't' (116, 0x74), '/' (47, 0x2F), 'b' (98,
> 0x62), 'a' (97, 0x61), 'z' (122, 0x7A), '/' (47, 0x2F), 'x' (120,
> 0x78) }
>
>
> C:\Buildbot\Slave\llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast\llvm.src\tools\clang\unittests\Basic\VirtualFileSystemTest.cpp(381):
> error: Value of: ExpectedIter
>
> Actual: 000000C72A87F7E0
>
> Expected: ExpectedEnd
> Which is: 000000C72A87F800
> [ FAILED ] VFSFromYAMLTest.DirectoryIteration (1 ms)
>
>
>
> On Tue, May 10, 2016 at 9:27 PM, Sean Silva <chisophugis at gmail.com> wrote:
> > Hi Bruno,
> >
> > I had to revert this in r269100 because it was looking like the bot was
> > going to be left red overnight.
> >
> > Changes to this VFS code seem to have a trend of breaking on windows. Any
> > idea why that is? I can understand things breaking on windows when
> writing
> > low-level parts of an FS abstraction, but this patch seems fairly
> > high-level. Is there a missing layering or something?
> >
> > -- Sean Silva
> >
> > On Tue, May 10, 2016 at 11:43 AM, Bruno Cardoso Lopes via cfe-commits
> > <cfe-commits at lists.llvm.org> wrote:
> >>
> >> Author: bruno
> >> Date: Tue May 10 13:43:00 2016
> >> New Revision: 269100
> >>
> >> URL: http://llvm.org/viewvc/llvm-project?rev=269100&view=rev
> >> Log:
> >> [VFS] Reconstruct the VFS overlay tree for more accurate lookup
> >>
> >> 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.
> >>
> >> This patch builds 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.
> >>
> >> Modified:
> >> cfe/trunk/lib/Basic/VirtualFileSystem.cpp
> >> cfe/trunk/unittests/Basic/VirtualFileSystemTest.cpp
> >>
> >> Modified: cfe/trunk/lib/Basic/VirtualFileSystem.cpp
> >> URL:
> >>
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/VirtualFileSystem.cpp?rev=269100&r1=269099&r2=269100&view=diff
> >>
> >>
> ==============================================================================
> >> --- cfe/trunk/lib/Basic/VirtualFileSystem.cpp (original)
> >> +++ cfe/trunk/lib/Basic/VirtualFileSystem.cpp Tue May 10 13:43:00 2016
> >> @@ -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<Entry> 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<Entry> &Root : FS->Roots) {
> >> + if (Name.equals(Root->getName())) {
> >> + ParentEntry = Root.get();
> >> + return ParentEntry;
> >> + }
> >> + }
> >> + } else { // Advance to the next component
> >> + auto *DE = dyn_cast<RedirectingDirectoryEntry>(ParentEntry);
> >> + for (std::unique_ptr<Entry> &Content :
> >> + llvm::make_range(DE->contents_begin(), DE->contents_end()))
> {
> >> + auto *DirContent =
> >> dyn_cast<RedirectingDirectoryEntry>(Content.get());
> >> + if (DirContent && Name.equals(Content->getName()))
> >> + return DirContent;
> >> + }
> >> + }
> >> +
> >> + // ... or create a new one
> >> + std::unique_ptr<Entry> E =
> >> llvm::make_unique<RedirectingDirectoryEntry>(
> >> + 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<RedirectingDirectoryEntry>(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<RedirectingDirectoryEntry>(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<Entry> &SubEntry :
> >> + llvm::make_range(DE->contents_begin(), DE->contents_end()))
> >> + uniqueOverlayTree(FS, SubEntry.get(), NewParentE);
> >> + break;
> >> + }
> >> + case EK_File: {
> >> + auto *FE = dyn_cast<RedirectingFileEntry>(SrcE);
> >> + assert(FE && "Must be a file");
> >> + assert(NewParentE && "Parent entry must exist");
> >> + auto *DE = dyn_cast<RedirectingDirectoryEntry>(NewParentE);
> >> + DE->addContent(llvm::make_unique<RedirectingFileEntry>(
> >> + Name, FE->getExternalContentsPath(), FE->getUseName()));
> >> + break;
> >> + }
> >> + }
> >> + }
> >> +
> >> std::unique_ptr<Entry> parseEntry(yaml::Node *N,
> RedirectingFileSystem
> >> *FS) {
> >> yaml::MappingNode *M = dyn_cast<yaml::MappingNode>(N);
> >> if (!M) {
> >> @@ -1225,6 +1296,7 @@ public:
> >> };
> >>
> >> DenseMap<StringRef, KeyStatus> Keys(std::begin(Fields),
> >> std::end(Fields));
> >> + std::vector<std::unique_ptr<Entry>> 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<Entry> 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<Entry> &E : RootEntries)
> >> + uniqueOverlayTree(FS, E.get());
> >> +
> >> return true;
> >> }
> >> };
> >>
> >> Modified: cfe/trunk/unittests/Basic/VirtualFileSystemTest.cpp
> >> URL:
> >>
> http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Basic/VirtualFileSystemTest.cpp?rev=269100&r1=269099&r2=269100&view=diff
> >>
> >>
> ==============================================================================
> >> --- cfe/trunk/unittests/Basic/VirtualFileSystemTest.cpp (original)
> >> +++ cfe/trunk/unittests/Basic/VirtualFileSystemTest.cpp Tue May 10
> >> 13:43:00 2016
> >> @@ -1022,9 +1022,14 @@ TEST_F(VFSFromYAMLTest, DirectoryIterati
> >> Lower->addDirectory("//root/");
> >> Lower->addDirectory("//root/foo");
> >> Lower->addDirectory("//root/foo/bar");
> >> + Lower->addDirectory("//root/zab");
> >> + Lower->addDirectory("//root/baz");
> >> Lower->addRegularFile("//root/foo/bar/a");
> >> Lower->addRegularFile("//root/foo/bar/b");
> >> Lower->addRegularFile("//root/file3");
> >> + Lower->addRegularFile("//root/zab/a");
> >> + Lower->addRegularFile("//root/zab/b");
> >> + Lower->addRegularFile("//root/baz/c");
> >> IntrusiveRefCntPtr<vfs::FileSystem> FS =
> >> getFromYAMLString("{ 'use-external-names': false,\n"
> >> " 'roots': [\n"
> >> @@ -1042,6 +1047,26 @@ TEST_F(VFSFromYAMLTest, DirectoryIterati
> >> " 'external-contents':
> >> '//root/foo/bar/b'\n"
> >> " }\n"
> >> " ]\n"
> >> + "},\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"
> >> "}",
> >> @@ -1054,8 +1079,12 @@ TEST_F(VFSFromYAMLTest, DirectoryIterati
> >>
> >> std::error_code EC;
> >> checkContents(O->dir_begin("//root/", EC),
> >> - {"//root/file1", "//root/file2", "//root/file3",
> >> "//root/foo"});
> >> + {"//root/file1", "//root/file2", "//root/baz",
> >> "//root/file3",
> >> + "//root/foo", "//root/zab"});
> >>
> >> checkContents(O->dir_begin("//root/foo/bar", EC),
> >> {"//root/foo/bar/a", "//root/foo/bar/b"});
> >> +
> >> + checkContents(O->dir_begin("//root/baz", EC),
> >> + {"//root/baz/x", "//root/baz/y", "//root/baz/c"});
> >> }
> >>
> >>
> >> _______________________________________________
> >> cfe-commits mailing list
> >> cfe-commits at lists.llvm.org
> >> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
> >
> >
>
>
>
> --
> Bruno Cardoso Lopes
> http://www.brunocardoso.cc
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160510/0a0b22e1/attachment-0001.html>
More information about the cfe-commits
mailing list