[VFS] Add property 'fallthrough' that controls fallback to real file system.

[vsapsai]

Summary

Default property value 'true' preserves current behavior. Value 'false' can be
used to create VFS "root", file system that gives better control over which
files compiler can use during compilation as there are no unpredictable
accesses to real file system.

Non-fallthrough use case changes how we treat multiple VFS overlay
files. Instead of all of them being at the same level just above a real
file system, now they are nested and subsequent overlays can refer to
files in previous overlays.

rdar://problem/39465552

[vsapsai]

Known issues:

• No support for 'fallthrough' in crash reproducer.
• Current way of working with modules in VFS "root" is clunky and error-prone.

Also I don't like that VFSFromYamlDirIterImpl is similar to overlay iterator but doesn't share code. At the moment I think it's not worth it to rework those abstractions to make more code reusable. If you've noticed problems with those iterators before, maybe it makes sense to try to find a better approach.

[vsapsai]

Rebased on top of trunk and checked that this is still working. Please take a look.

[vsapsai]

Ping.

[bruno]

Hi Volodymyr,

Thanks for working on this, really nice work with the tests! Comments below.

• No support for 'fallthrough' in crash reproducer.

That'd be nice to have at some point to make sure we never escape into the real fs.

• Current way of working with modules in VFS "root" is clunky and error-prone.

Why?

Also I don't like that VFSFromYamlDirIterImpl is similar to overlay iterator but doesn't share code. At the moment I think it's not worth it to rework those abstractions to make more code reusable. If you've noticed problems with those iterators before, maybe it makes sense to try to find a better approach.

Maybe add FIXMEs for it?

clang/lib/Basic/VirtualFileSystem.cpp

934 ↗

(On Diff #160032)

Can you add comments to these new members? Specifically, what's the purpose of IsExternalFSCurrent and how it connects to ExternalFS

940 ↗

(On Diff #160032)

Same for these methods

1738 ↗

(On Diff #160032)

Is passing ShouldCheckExternalFS really necessary? Why checking the status of the member isn't enough?

2041 ↗

(On Diff #160032)

incrementImpl(true/*IsFirstTime*/)

2045 ↗

(On Diff #160032)

incrementImpl(false/*IsFirstTime*/)

[vsapsai]

• Rebase on top of the latest changes in trunk.
• Address review comments.

I expect diff between diffs to be noisy due to rebase.

[vsapsai]

clang/lib/Basic/VirtualFileSystem.cpp

934 ↗

(On Diff #160032)

Added comments.

940 ↗

(On Diff #160032)

Added comments. Not entirely happy with them, would like to hear opinion of others.

1738 ↗

(On Diff #160032)

Not required anymore, simplified the code.

[vsapsai]

In D50539#1241034, @bruno wrote:

• Current way of working with modules in VFS "root" is clunky and error-prone.

Why?

Mostly because when you approach it in a straightforward way, you cannot read written modules because they aren't in VFS and "rootness" correctly prevents you from reading random files. You can add -fdisable-module-hash as I've done in the test but it has its own downsides when used not in a test but in a real project.

Also I don't like that VFSFromYamlDirIterImpl is similar to overlay iterator but doesn't share code. At the moment I think it's not worth it to rework those abstractions to make more code reusable. If you've noticed problems with those iterators before, maybe it makes sense to try to find a better approach.

Maybe add FIXMEs for it?

Good idea, added.

[bruno]

LGTM with some minor changes.

llvm/lib/Support/VirtualFileSystem.cpp

1008

Please use /// instead of // to get doxygen's auto brief.

1010

Same here

1018

Same here

1020

Same here

1022

Same here

[vsapsai]

In D50539#1272924, @bruno wrote:

LGTM with some minor changes.

I was using // instead of /// on purpose. Class VFSFromYamlDirIterImpl resides entirely in .cpp file and isn't available outside of it. Comments are supposed to cover implementation details and intention, not class interface. That's why I think those comments shouldn't be consumed by Doxygen. Does it make sense or do you think it would be better to have those comments in Doxygen?

[dexonsmith]

In D50539#1273654, @vsapsai wrote:

I was using // instead of /// on purpose. Class VFSFromYamlDirIterImpl resides entirely in .cpp file and isn't available outside of it. Comments are supposed to cover implementation details and intention, not class interface. That's why I think those comments shouldn't be consumed by Doxygen. Does it make sense or do you think it would be better to have those comments in Doxygen?

IMO we should have the doxygen comments for any developer that's trying to expand/use/understand the impl class. In my experience the main reasons we stuff things in .cpp files are to optimize compile time and to enable better optimizations within a TU.

[vsapsai]

In D50539#1273688, @dexonsmith wrote:

In D50539#1273654, @vsapsai wrote:

I was using // instead of /// on purpose. Class VFSFromYamlDirIterImpl resides entirely in .cpp file and isn't available outside of it. Comments are supposed to cover implementation details and intention, not class interface. That's why I think those comments shouldn't be consumed by Doxygen. Does it make sense or do you think it would be better to have those comments in Doxygen?

IMO we should have the doxygen comments for any developer that's trying to expand/use/understand the impl class. In my experience the main reasons we stuff things in .cpp files are to optimize compile time and to enable better optimizations within a TU.

OK, I'll change to Doxygen-style comments.

[vsapsai]

• Rebase once again.
• Use Doxygen comments in some places.

clang/lib/Frontend/CompilerInvocation.cpp

 }
 
 IntrusiveRefCntPtr<llvm::vfs::FileSystem> createVFSFromCompilerInvocation(
     const CompilerInvocation &CI, DiagnosticsEngine &Diags,
     IntrusiveRefCntPtr<llvm::vfs::FileSystem> BaseFS) {
   if (CI.getHeaderSearchOpts().VFSOverlayFiles.empty())
     return BaseFS;
 
-  IntrusiveRefCntPtr<llvm::vfs::OverlayFileSystem> Overlay(
-      new llvm::vfs::OverlayFileSystem(BaseFS));
+  IntrusiveRefCntPtr<llvm::vfs::FileSystem> Result = BaseFS;
   // earlier vfs files are on the bottom
   for (const auto &File : CI.getHeaderSearchOpts().VFSOverlayFiles) {
     llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>> Buffer =
-        BaseFS->getBufferForFile(File);
+        Result->getBufferForFile(File);
     if (!Buffer) {
       Diags.Report(diag::err_missing_vfs_overlay_file) << File;
       continue;
     }
 
     IntrusiveRefCntPtr<llvm::vfs::FileSystem> FS = llvm::vfs::getVFSFromYAML(
-        std::move(Buffer.get()), /*DiagHandler*/ nullptr, File);
-    if (FS)
-      Overlay->pushOverlay(FS);
-    else
+        std::move(Buffer.get()), /*DiagHandler*/ nullptr, File,
+        /*DiagContext*/ nullptr, Result);
+    if (!FS) {
       Diags.Report(diag::err_invalid_vfs_overlay) << File;
+      continue;
+    }
+
+    Result = FS;
   }
-  return Overlay;
+  return Result;
 }
 
 } // namespace clang

clang/test/VFS/Inputs/Broken.framework/Headers/Error.h

+// Error.h
+
+#error Should not include this header in a module

clang/test/VFS/Inputs/Broken.framework/Modules/module.modulemap

+framework module Broken [extern_c] {
+  umbrella "Headers"
+  export *
+  module * { export * }
+}
+

clang/test/VFS/Inputs/Broken.framework/VFSHeaders/A.h

+// A.h

clang/test/VFS/Inputs/vfsroot.yaml

+{
+  'version': 0,
+  'use-external-names': false,
+  'fallthrough': false,
+  'roots': [
+    { 'name': '/tests', 'type': 'directory',
+      'contents': [
+        { 'name': 'vfsroot-include.c', 'type': 'file',
+          'external-contents': 'TEST_DIR/vfsroot-include.c'
+        },
+        { 'name': 'vfsroot-with-overlay.c', 'type': 'file',
+          'external-contents': 'TEST_DIR/vfsroot-with-overlay.c'
+        },
+        { 'name': 'vfsroot-module.m', 'type': 'file',
+          'external-contents': 'TEST_DIR/vfsroot-module.m'
+        }
+      ]
+    },
+    { 'name': '/direct-vfs-root-files', 'type': 'directory',
+      'contents': [
+        { 'name': 'not_real.h', 'type': 'file',
+          'external-contents': 'TEST_DIR/Inputs/actual_header.h'
+        },
+        { 'name': 'vfsoverlay.yaml', 'type': 'file',
+          'external-contents': 'OUT_DIR/vfsoverlay.yaml'
+        }
+      ]
+    },
+    { 'name': '/indirect-vfs-root-files', 'type': 'directory',
+      'contents': [
+        { 'name': 'actual_header.h', 'type': 'file',
+          'external-contents': 'TEST_DIR/Inputs/actual_header.h'
+        }
+      ]
+    },
+    { 'name': 'TEST_DIR/Inputs/Broken.framework', 'type': 'directory',
+      'contents': [
+        { 'name': 'Headers/A.h', 'type': 'file',
+          'external-contents': 'TEST_DIR/Inputs/Broken.framework/VFSHeaders/A.h'
+        },
+        { 'name': 'Modules/module.modulemap', 'type': 'file',
+          'external-contents': 'TEST_DIR/Inputs/Broken.framework/Modules/module.modulemap'
+        }
+      ]
+    },
+    # Locations for modules.
+    { 'name': 'OUT_DIR/cache', 'type': 'directory',
+      'contents': [
+        { 'name': 'Broken.pcm', 'type': 'file',
+          'external-contents': 'OUT_DIR/cache/Broken.pcm'
+        }
+      ]
+    }
+  ]
+}

clang/test/VFS/vfsroot-include.c

+// REQUIRES: shell
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: sed -e "s:TEST_DIR:%S:g" -e "s:OUT_DIR:%t:g" %S/Inputs/vfsroot.yaml > %t.yaml
+// RUN: not %clang_cc1 -Werror -ivfsoverlay %t.yaml -I %S/Inputs -I /direct-vfs-root-files -fsyntax-only /tests/vfsroot-include.c 2>&1 | FileCheck %s
+// The line above tests that the compiler input file is looked up through VFS.
+
+// Test successful include through the VFS.
+#include "not_real.h"
+
+// Test that a file missing from the VFS root is not found, even if it is
+// discoverable through the real file system. Fatal error should be the last
+// in the file as it hides other errors.
+#include "actual_header.h"
+// CHECK: fatal error: 'actual_header.h' file not found
+// CHECK: 1 error generated.
+// CHECK-NOT: error

clang/test/VFS/vfsroot-module.m

+// REQUIRES: shell
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: sed -e "s:TEST_DIR:%S:g" -e "s:OUT_DIR:%t:g" %S/Inputs/vfsroot.yaml > %t.yaml
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fdisable-module-hash -fmodules-cache-path=%t/cache -ivfsoverlay %t.yaml -F %S/Inputs -fsyntax-only /tests/vfsroot-module.m
+
+// Test that a file missing from the VFS root is not found, even if it is
+// discoverable through the real file system at location that is a part of
+// the framework.
+@import Broken;

clang/test/VFS/vfsroot-with-overlay.c

+// REQUIRES: shell
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: sed -e "s:TEST_DIR:%S:g" -e "s:OUT_DIR:%t:g" %S/Inputs/vfsroot.yaml > %t.yaml
+// RUN: sed -e "s:INPUT_DIR:/indirect-vfs-root-files:g" -e "s:OUT_DIR:/overlay-dir:g" %S/Inputs/vfsoverlay.yaml > %t/vfsoverlay.yaml
+// RUN: %clang_cc1 -Werror -ivfsoverlay %t.yaml -ivfsoverlay /direct-vfs-root-files/vfsoverlay.yaml -I /overlay-dir -fsyntax-only /tests/vfsroot-with-overlay.c
+
+#include "not_real.h"
+
+void foo() {
+  bar();
+}

llvm/lib/Support/VirtualFileSystem.cpp

return UseName == NK_NotSet ? GlobalUseExternalName
                                 : (UseName == NK_External);
   }
 
   NameKind getUseName() const { return UseName; }
 
   static bool classof(const Entry *E) { return E->getKind() == EK_File; }
 };
 
+// FIXME: reuse implementation common with OverlayFSDirIterImpl as these
+// iterators are conceptually similar.
 class VFSFromYamlDirIterImpl : public llvm::vfs::detail::DirIterImpl {
   std::string Dir;
   RedirectingDirectoryEntry::iterator Current, End;
 
-  std::error_code incrementImpl();
+  // To handle 'fallthrough' mode we need to iterate at first through
+  // RedirectingDirectoryEntry and then through ExternalFS. These operations are
+  // done sequentially, we just need to keep a track of what kind of iteration
+  // we are currently performing.
+
+  /// Flag telling if we should iterate through ExternalFS or stop at the last
+  /// RedirectingDirectoryEntry::iterator.
+  bool IterateExternalFS;
+  /// Flag telling if we have switched to iterating through ExternalFS.
+  bool IsExternalFSCurrent = false;
+  FileSystem &ExternalFS;
+  directory_iterator ExternalDirIter;
+  llvm::StringSet<> SeenNames;
+
+  /// To combine multiple iterations, different methods are responsible for
+  /// different iteration steps.
+  /// @{
+
+  /// Responsible for dispatching between RedirectingDirectoryEntry iteration
+  /// and ExternalFS iteration.
+  std::error_code incrementImpl(bool IsFirstTime);
+  /// Responsible for RedirectingDirectoryEntry iteration.
+  std::error_code incrementContent(bool IsFirstTime);
+  /// Responsible for ExternalFS iteration.
+  std::error_code incrementExternal();
+  /// @}
 
 public:
   VFSFromYamlDirIterImpl(const Twine &Path,
                          RedirectingDirectoryEntry::iterator Begin,
                          RedirectingDirectoryEntry::iterator End,
+                         bool IterateExternalFS, FileSystem &ExternalFS,
                          std::error_code &EC);
 
   std::error_code increment() override;
 };
 
 /// A virtual file system parsed from a YAML file.
 ///
 /// Currently, this class allows creating virtual directories and mapping

 ///            ]
 /// }
 /// \endverbatim
 ///
 /// All configuration options are optional.
 ///   'case-sensitive': <boolean, default=true>
 ///   'use-external-names': <boolean, default=true>
 ///   'overlay-relative': <boolean, default=false>
+///   'fallthrough': <boolean, default=true>
 ///
 /// Virtual directories are represented as
 /// \verbatim
 /// {
 ///   'type': 'directory',
 ///   'name': <string>,
 ///   'contents': [ <file or directory entries> ]
 /// }
class RedirectingFileSystem : public vfs::FileSystem {
 
   /// IsRelativeOverlay marks whether a ExternalContentsPrefixDir path must
   /// be prefixed in every 'external-contents' when reading from YAML files.
   bool IsRelativeOverlay = false;
 
   /// Whether to use to use the value of 'external-contents' for the
   /// names of files.  This global value is overridable on a per-file basis.
   bool UseExternalNames = true;
+
+  /// Whether to attempt a file lookup in external file system after it wasn't
+  /// found in VFS.
+  bool IsFallthrough = true;
   /// @}
 
   /// Virtual file paths and external files could be canonicalized without "..",
   /// "." and "./" in their paths. FIXME: some unittests currently fail on
   /// win32 when using remove_dots and remove_leading_dotslash on paths.
   bool UseCanonicalizedPaths =
 #ifdef _WIN32
       false;
public:
   std::error_code setCurrentWorkingDirectory(const Twine &Path) override {
     return ExternalFS->setCurrentWorkingDirectory(Path);
   }
 
   directory_iterator dir_begin(const Twine &Dir, std::error_code &EC) override {
     ErrorOr<Entry *> E = lookupPath(Dir);
     if (!E) {
       EC = E.getError();
+      if (IsFallthrough && EC == errc::no_such_file_or_directory)
+        return ExternalFS->dir_begin(Dir, EC);
       return {};
     }
     ErrorOr<Status> S = status(Dir, *E);
     if (!S) {
       EC = S.getError();
       return {};
     }
     if (!S->isDirectory()) {
       EC = std::error_code(static_cast<int>(errc::not_a_directory),
                            std::system_category());
       return {};
     }
 
     auto *D = cast<RedirectingDirectoryEntry>(*E);
     return directory_iterator(std::make_shared<VFSFromYamlDirIterImpl>(
-        Dir, D->contents_begin(), D->contents_end(), EC));
+        Dir, D->contents_begin(), D->contents_end(),
+        /*IterateExternalFS=*/IsFallthrough, *ExternalFS, EC));
   }
 
   void setExternalContentsPrefixDir(StringRef PrefixDir) {
     ExternalContentsPrefixDir = PrefixDir.str();
   }
 
   StringRef getExternalContentsPrefixDir() const {
     return ExternalContentsPrefixDir;
if (!Top) {
       return false;
     }
 
     KeyStatusPair Fields[] = {
         KeyStatusPair("version", true),
         KeyStatusPair("case-sensitive", false),
         KeyStatusPair("use-external-names", false),
         KeyStatusPair("overlay-relative", false),
+        KeyStatusPair("fallthrough", false),
         KeyStatusPair("roots", true),
     };
 
     DenseMap<StringRef, KeyStatus> Keys(std::begin(Fields), std::end(Fields));
     std::vector<std::unique_ptr<Entry>> RootEntries;
 
     // Parse configuration and 'roots'
     for (auto &I : *Top) {
for (auto &I : *Top) {
         if (!parseScalarBool(I.getValue(), FS->CaseSensitive))
           return false;
       } else if (Key == "overlay-relative") {
         if (!parseScalarBool(I.getValue(), FS->IsRelativeOverlay))
           return false;
       } else if (Key == "use-external-names") {
         if (!parseScalarBool(I.getValue(), FS->UseExternalNames))
           return false;
+      } else if (Key == "fallthrough") {
+        if (!parseScalarBool(I.getValue(), FS->IsFallthrough))
+          return false;
       } else {
         llvm_unreachable("key missing from Keys");
       }
     }
 
     if (Stream.failed())
       return false;
 
ErrorOr<Status> RedirectingFileSystem::status(const Twine &Path, Entry *E) {
   } else { // directory
     auto *DE = cast<RedirectingDirectoryEntry>(E);
     return Status::copyWithNewName(DE->getStatus(), Path.str());
   }
 }
 
 ErrorOr<Status> RedirectingFileSystem::status(const Twine &Path) {
   ErrorOr<Entry *> Result = lookupPath(Path);
-  if (!Result)
+  if (!Result) {
+    if (IsFallthrough &&
+        Result.getError() == llvm::errc::no_such_file_or_directory) {
+      return ExternalFS->status(Path);
+    }
     return Result.getError();
+  }
   return status(Path, *Result);
 }
 
 namespace {
 
 /// Provide a file wrapper with an overriden status.
 class FileWithFixedStatus : public File {
   std::unique_ptr<File> InnerFile;
public:
   std::error_code close() override { return InnerFile->close(); }
 };
 
 } // namespace
 
 ErrorOr<std::unique_ptr<File>>
 RedirectingFileSystem::openFileForRead(const Twine &Path) {
   ErrorOr<Entry *> E = lookupPath(Path);
-  if (!E)
+  if (!E) {
+    if (IsFallthrough &&
+        E.getError() == llvm::errc::no_such_file_or_directory) {
+      return ExternalFS->openFileForRead(Path);
+    }
     return E.getError();
+  }
 
   auto *F = dyn_cast<RedirectingFileEntry>(*E);
   if (!F) // FIXME: errc::not_a_file?
     return make_error_code(llvm::errc::invalid_argument);
 
   auto Result = ExternalFS->openFileForRead(F->getExternalContentsPath());
   if (!Result)
     return Result;
void YAMLVFSWriter::write(llvm::raw_ostream &OS) {
   });
 
   JSONWriter(OS).write(Mappings, UseExternalNames, IsCaseSensitive,
                        IsOverlayRelative, OverlayDir);
 }
 
 VFSFromYamlDirIterImpl::VFSFromYamlDirIterImpl(
     const Twine &_Path, RedirectingDirectoryEntry::iterator Begin,
-    RedirectingDirectoryEntry::iterator End, std::error_code &EC)
-    : Dir(_Path.str()), Current(Begin), End(End) {
-  EC = incrementImpl();
+    RedirectingDirectoryEntry::iterator End, bool IterateExternalFS,
+    FileSystem &ExternalFS, std::error_code &EC)
+    : Dir(_Path.str()), Current(Begin), End(End),
+      IterateExternalFS(IterateExternalFS), ExternalFS(ExternalFS) {
+  EC = incrementImpl(/*IsFirstTime=*/true);
 }
 
 std::error_code VFSFromYamlDirIterImpl::increment() {
-  assert(Current != End && "cannot iterate past end");
-  ++Current;
-  return incrementImpl();
+  return incrementImpl(/*IsFirstTime=*/false);
 }
 
-std::error_code VFSFromYamlDirIterImpl::incrementImpl() {
+std::error_code VFSFromYamlDirIterImpl::incrementExternal() {
+  assert(!(IsExternalFSCurrent && ExternalDirIter == directory_iterator()) &&
+         "incrementing past end");
+  std::error_code EC;
+  if (IsExternalFSCurrent) {
+    ExternalDirIter.increment(EC);
+  } else if (IterateExternalFS) {
+    ExternalDirIter = ExternalFS.dir_begin(Dir, EC);
+    IsExternalFSCurrent = true;
+    if (EC && EC != errc::no_such_file_or_directory)
+      return EC;
+    EC = {};
+  }
+  if (EC || ExternalDirIter == directory_iterator()) {
+    CurrentEntry = directory_entry();
+  } else {
+    CurrentEntry = *ExternalDirIter;
+  }
+  return EC;
+}
+
+std::error_code VFSFromYamlDirIterImpl::incrementContent(bool IsFirstTime) {
+  assert(IsFirstTime || Current != End && "cannot iterate past end");
+  if (!IsFirstTime)
+    ++Current;
   while (Current != End) {
     SmallString<128> PathStr(Dir);
     llvm::sys::path::append(PathStr, (*Current)->getName());
     sys::fs::file_type Type;
     switch ((*Current)->getKind()) {
     case EK_Directory:
       Type = sys::fs::file_type::directory_file;
       break;
     case EK_File:
       Type = sys::fs::file_type::regular_file;
       break;
     }
     CurrentEntry = directory_entry(PathStr.str(), Type);
-    break;
-  }
+    return {};
+  }
+  return incrementExternal();
 
-  if (Current == End)
-    CurrentEntry = directory_entry();
-  return {};
+std::error_code VFSFromYamlDirIterImpl::incrementImpl(bool IsFirstTime) {
+  while (true) {
+    std::error_code EC = IsExternalFSCurrent ? incrementExternal()
+                                             : incrementContent(IsFirstTime);
+    if (EC || CurrentEntry.path().empty())
+      return EC;
+    StringRef Name = llvm::sys::path::filename(CurrentEntry.path());
+    if (SeenNames.insert(Name).second)
+      return EC; // name not seen before
+  }
+  llvm_unreachable("returned above");
 }
 
 vfs::recursive_directory_iterator::recursive_directory_iterator(
     FileSystem &FS_, const Twine &Path, std::error_code &EC)
     : FS(&FS_) {
   directory_iterator I = FS->dir_begin(Path, EC);
   if (I != directory_iterator()) {
     State = std::make_shared<IterState>();

llvm/unittests/Support/VirtualFileSystemTest.cpp

FS = getFromYAMLString(
       "    'contents': []\n"
       "  }\n"
       "] }",
       Lower);
   EXPECT_EQ(nullptr, FS.get());
 
   EXPECT_EQ(3, NumDiagnostics);
 }
+
+TEST_F(VFSFromYAMLTest, NonFallthroughDirectoryIteration) {
+  IntrusiveRefCntPtr<DummyFileSystem> Lower(new DummyFileSystem());
+  Lower->addDirectory("//root/");
+  Lower->addRegularFile("//root/a");
+  Lower->addRegularFile("//root/b");
+  IntrusiveRefCntPtr<vfs::FileSystem> FS = getFromYAMLString(
+      "{ 'use-external-names': false,\n"
+      "  'fallthrough': false,\n"
+      "  'roots': [\n"
+      "{\n"
+      "  'type': 'directory',\n"
+      "  'name': '//root/',\n"
+      "  'contents': [ {\n"
+      "                  'type': 'file',\n"
+      "                  'name': 'c',\n"
+      "                  'external-contents': '//root/a'\n"
+      "                }\n"
+      "              ]\n"
+      "}\n"
+      "]\n"
+      "}",
+      Lower);
+  ASSERT_TRUE(FS.get() != nullptr);
+
+  std::error_code EC;
+  checkContents(FS->dir_begin("//root/", EC),
+                {"//root/c"});
+}
+
+TEST_F(VFSFromYAMLTest, DirectoryIterationWithDuplicates) {
+  IntrusiveRefCntPtr<DummyFileSystem> Lower(new DummyFileSystem());
+  Lower->addDirectory("//root/");
+  Lower->addRegularFile("//root/a");
+  Lower->addRegularFile("//root/b");
+  IntrusiveRefCntPtr<vfs::FileSystem> FS = getFromYAMLString(
+      "{ 'use-external-names': false,\n"
+      "  'roots': [\n"
+      "{\n"
+      "  'type': 'directory',\n"
+      "  'name': '//root/',\n"
+      "  'contents': [ {\n"
+      "                  'type': 'file',\n"
+      "                  'name': 'a',\n"
+      "                  'external-contents': '//root/a'\n"
+      "                }\n"
+      "              ]\n"
+      "}\n"
+      "]\n"
+      "}",
+    Lower);
+  ASSERT_TRUE(FS.get() != nullptr);
+
+  std::error_code EC;
+  checkContents(FS->dir_begin("//root/", EC),
+                {"//root/a", "//root/b"});
+}
+
+TEST_F(VFSFromYAMLTest, DirectoryIterationErrorInVFSLayer) {
+  IntrusiveRefCntPtr<DummyFileSystem> Lower(new DummyFileSystem());
+  Lower->addDirectory("//root/");
+  Lower->addDirectory("//root/foo");
+  Lower->addRegularFile("//root/foo/a");
+  Lower->addRegularFile("//root/foo/b");
+  IntrusiveRefCntPtr<vfs::FileSystem> FS = getFromYAMLString(
+      "{ 'use-external-names': false,\n"
+      "  'roots': [\n"
+      "{\n"
+      "  'type': 'directory',\n"
+      "  'name': '//root/',\n"
+      "  'contents': [ {\n"
+      "                  'type': 'file',\n"
+      "                  'name': 'bar/a',\n"
+      "                  'external-contents': '//root/foo/a'\n"
+      "                }\n"
+      "              ]\n"
+      "}\n"
+      "]\n"
+      "}",
+      Lower);
+  ASSERT_TRUE(FS.get() != nullptr);
+
+  std::error_code EC;
+  checkContents(FS->dir_begin("//root/foo", EC),
+                {"//root/foo/a", "//root/foo/b"});
+}