[vfs] Allow root paths relative to the directory of the vfsoverlay YAML file

[haowei]

Summary

VFS overlay file allow using relative paths in root->name paths (root directory of the virtual file) and external-contents (the actual files). While the 'external-contents' could be configured to relative to directory of the YAML file (change https://reviews.llvm.org/D17457), the root paths could only be relative to the current working directory.

This patch add the "root-relative" option to the YAML file format. When this option is set to "yaml-dir" the root->name will be prepend by the YAML file directory. This option is helpful when compiling on case sensitive file systems when cross compiling to Windows as we can create a vfsoverlay YAML file for the Windows libraries without using absolute paths. Related change: https://reviews.llvm.org/D125800

[haowei]

Please take a look.

[phosek]

llvm/include/llvm/Support/VirtualFileSystem.h

660

Could we make this just a boolean akin to overlay-relative since there are only two options (default to false)?

[bnbarham]

This seems reasonable to me in general. @dexonsmith in case you have any thoughts.

clang/test/VFS/Inputs/root-relative-overlay.yaml

4

I'd prefer a test without overlay-relative set to make it clear they don't depend on each other.

llvm/include/llvm/Support/VirtualFileSystem.h

660

I personally prefer being explicit here, overlay-relative is fairly confusing as it is.

overlay-relative isn't about allowing relative paths, but instead means that *all* external paths should be prefixed with the directory of the overlay. To put another way, external paths can be relative whether this is true/false, overlay-relative just *always* prepends the overlay path.

Could you add a comment to make it clear that this has no interaction with overlay-relative? If you want to add a comment to overlay-relative with something like the above that would also be appreciated :)

752

to the current working directory when the overlay is created. is maybe a little clearer to me

755

Any thoughts on something like OverlayDir instead?

926

*and the path is relative*

llvm/lib/Support/VirtualFileSystem.cpp

1903

IMO both this and CWD should be using the base FS instead. VFS didn't have a CWD previously, but now that it does it doesn't really make sense to use the process wide CWD. Especially since -working-directory doesn't change it.

[dexonsmith]

In D137473#3915497, @bnbarham wrote:

This seems reasonable to me in general. @dexonsmith in case you have any thoughts.

SGTM! (I haven't reviewed in detail but I figure @bnbarham is on it...)

[bnbarham]

clang/test/VFS/Inputs/root-relative-overlay.yaml

4

There's also unit tests in llvm/unittests/Support/VirtualFileSystemTest.cpp that you could add to.

[haowei]

clang/test/VFS/Inputs/root-relative-overlay.yaml

4

I need some time to patch this unit test for testing the new option.
Existing functions in this unit test file does not support defining the YAMLFilePath.

I will update this patch when I finish.

4

I updated the yaml file and add unit tests with overlay-relative set to true and false.

llvm/include/llvm/Support/VirtualFileSystem.h

660

I also think overlay-relative is a bit misleading and it should be an enum instead of a boolean option. And it should only work if the external paths are relative instead of blindly prepend the overlay dir to every external paths. But changing this will be a breaking change so I prefer to avoid it in this patch.

I updated the descriptions.

752

Added into the comments.

755

I think OverlayDir is better.

llvm/lib/Support/VirtualFileSystem.cpp

1903

Could you clarify a bit more about the "base FS" please? I am still quite new to the LLVM VFS system. Which API should I use to get the appropriate working directory instead of the the process wide CWD?

I avoided changing the default behavior (relative to the process's current working directory) as I am a bit concerned breaking other people's use cases.

I looked up a bit, the value "--working-directory" will be writen into "VFS->setCurrentWorkingDirectory()" Do you think it is a better idea to use this value instead of the process current working directory? Though it would still be a behavior change for users rely on process current working directories.

[bnbarham]

llvm/include/llvm/Support/VirtualFileSystem.h

660

FWIW it was added explicitly to always prepend, it's used for reproducers where the overlay may already have absolute paths. I was just trying to explain what overlay-relative does.

llvm/lib/Support/VirtualFileSystem.cpp

1903

Could you clarify a bit more about the "base FS" please?

By "base FS" I just meant the filesystem that's passed down when creating the RedirectingFileSystem, ie. ExternalFS in getVFSFromYAML.

Which API should I use to get the appropriate working directory instead of the the process wide CWD?

ExternalFS->getCurrentWorkingDirectory() and ExternalFS->makeAbsolute is what I'd expect to be used here.

Do you think it is a better idea to use this value instead of the process current working directory? Though it would still be a behavior change for users rely on process current working directories.

Yes, I think it's surprising that it currently *isn't* using it. It's just a hold over from when the VFS didn't have the concept of CWD. I'd be happy with accepting this patch as is, with a separate patch after that changes to using the VFS CWD.

[haowei]

Add additional unit test.

[haowei]

clang/test/VFS/Inputs/root-relative-overlay.yaml

4

I updated llvm/unittests/Support/VirtualFileSystemTest.cpp to include tests on root-relative option.

llvm/lib/Support/VirtualFileSystem.cpp

1903

Yes, I think it's surprising that it currently *isn't* using it. It's just a hold over from when the VFS didn't have the concept of CWD. I'd be happy with accepting this patch as is, with a separate patch after that changes to using the VFS CWD.

I see, I can upload a follow up change after this one is landed. to use VFS CWD instead of process CWD here. It is also easier to revert if anything goes wrong.

[haowei]

Unit test failures under Windows should be fixed now.

Root->name will also use base FS's current directory instead of process current directory now. Please take a look.

[bnbarham]

llvm/include/llvm/Support/VirtualFileSystem.h

1000

Should be private IMO

llvm/lib/Support/VirtualFileSystem.cpp

1363

Did you find this was needed? It's already checked by the normal makeAbsolute (which checks this already) and the only other caller is when we're using the overlay directory path, which should always be absolute.

1901

This is unused now. Maybe we should just merge the else if and else branches and just grab either getOverlayFileDir or getCurrentWorkingDirectory depending on RootRelative. They should otherwise be identical.

llvm/unittests/Support/VirtualFileSystemTest.cpp

1875 ↗

(On Diff #475347)

Just override the previous FS/S IMO - that way we avoid the accidental FS->status a few lines down 😅

[haowei]

I have to revert back to use process cwd instead of base fs cwd. There are a few tests relied the behavior of using process cwd that I need to take a closer look.

[haowei]

Address review comments

[haowei]

llvm/include/llvm/Support/VirtualFileSystem.h

1000

Moved it to private section and added comments

llvm/lib/Support/VirtualFileSystem.cpp

1363

No, it is not needed for now. As the OverlayDir is always abs when it was set. The normal makeAbsolute function checks "Path" param but not checking the returned WorkDir, which is OK because the return value from getCurrentWorkingDirectory will be abs or empty.

It is more to be future proof if someone decide to use this function for other purpose and putting a non-abs path in WorkingDir field will causes unexpected behaviors.

I also need to exclude empty WorkingDir though because in unit tests, the WorkingDir is always empty.

1901

I merged these 2 else block.
I have to use sys::fs::make_absolute function for now as using FS's CWD will break a few tests. It is better to address these test in a separate patch.

llvm/unittests/Support/VirtualFileSystemTest.cpp

1875 ↗

(On Diff #475347)

Thanks for pointing that out 😅

[haowei]

Fixing Windows test failures.
overlay-relative flag will always using native path separator, therefore, it needs a separate base FS and OverlayYAML file setup on Windows. This diff adds this.

[bnbarham]

llvm/lib/Support/VirtualFileSystem.cpp

1919–1921

windows_slash is going to be windows_backslash here. I assume this was fine since sys::fs::make_absolute(Name) would always return the native style, but that isn't the case now. It's also unfortunate we calculate this again when makeAbsolute only just did it.

1920

Why the canonicalization?

llvm/unittests/Support/VirtualFileSystemTest.cpp

1878 ↗

(On Diff #475884)

separator -> separator

As per my comment above, this is only true because we're either using posix or windows_backslash. I'm not sure what we really want here, should we always convert to native? Any thoughts @dexonsmith?

FWIW the non-windows case is identical to the above block. It'd be interesting to test / and \ on both filesystems.

[dexonsmith]

llvm/unittests/Support/VirtualFileSystemTest.cpp

1878 ↗

(On Diff #475884)

I think we might need a Windows person to help answer; @rnk, can you suggest someone to help answer @bnbarham's question?

[rnk]

llvm/unittests/Support/VirtualFileSystemTest.cpp

1878 ↗

(On Diff #475884)

I can try suggesting @hans, @compnerd, or @Bigcheese

[haowei]

llvm/lib/Support/VirtualFileSystem.cpp

1919–1921

Thanks for pointing that out. I need to consider the windows forward slash case.

There is a bit in consistency in the VFS implementation. for Windows. we can use forward slashes('/') on Windows. Most Windows API/Syscalls accept it. But some APIs may not work if you mix '/' with '\' in the path on Windows. What RedirectFileSystem::makeAbsolute is trying to do is, it first determine the slashes used in the WorkDir(either from process working directory or from command line flags, or OverlayDir), in which it can be a forward slash on Windows. Then it uses the same separator when appending the relative directory. Using sys::fs::make_absolute will break that.

The reason that I use makeAbsolute here is that the OverlayDir will likely use forward slash on Windows if people uses CMake options LINK_FLAG="/vfsoverlay:C:/path/to/overlay.xml". In that case, sys::fs::make_absolute will generate some paths like C:/path/to/foo\\bar, which may cause issues.

I will rewrite this to consider the forward slashes.

1920

If the Name here somehow has something like //foo/./../bar, it will cause match failures. Canonicalize function removes these dots.
The setOverlayFileDir does not canonicalize the dir so this is quite common to see.

llvm/unittests/Support/VirtualFileSystemTest.cpp

1878 ↗

(On Diff #475884)

The non-windows case enable the overlay-relative option. The above block didn't. And that is where I discover the issue with overlay-relative always uses native separator instead of trying to match OverlayDir.

The best approach for root-relative and overlay-relative option would probably be matching the style of OverlayDir/WorkDir and unify the path separator if they are mixed in a path.
But this change will likely affects a few tests.

[phosek]

llvm/lib/Support/VirtualFileSystem.cpp

1919–1921

This was also discussed in D87732 which may provide some additional context.

[haowei]

I changed some comments and add more details.
If there is no objection, can I get an approval on this change so I can land it? It will unblock our development on Windows cross compilation support.

llvm/lib/Support/VirtualFileSystem.cpp

1919–1921

I looked into the implementation of llvm path library and how windows_backslash and windows_slash are implemented and used.
TL;DR,
The difference between windows_backslash and windows_slash is that when llvm::get_separator is called, it returns a different slash. The rest of the API behaviors are the same. So in this VirtualFileSystem.cpp, it doesn't matter if the path_style is assigned to
sys::path::Style::windows_backslash or sys::path::Style::windows_slash here as the output of this function parseEntry will be the same.

llvm/unittests/Support/VirtualFileSystemTest.cpp

1878 ↗

(On Diff #475884)

I corrected the typo.

I did some Windows API tests in the past week and I can confirm on Windows 10 (but should be apply to all NT6 family Windows), the forward slashes "/" and backward slashes "\" are both legit path separators and they be mixed in a path in any combinations. They will behave properly and no errors will be reported. However, the cmd.exe and some other Windows program will have some issue if forward slashes is used or mixed with backward slashes because it looks like flag switches, probably because these programs parse the user Input and doing their own path processing before invoking Windows APIs.

In LLVM's case, since File IO will rely on Windows APIs, I think how my current code handling the path separators under relative path scenario is sufficient. It shouldn't bring new issues.

On Linux and other POSIX OS, the backward slash is a legit path character.
e.g.
/foo/bar/\\a/b means

foo
├- bar
      ├ `\a`
            ├ b

Therefore, if backward slashes are used in the overlay file under Linux, they should not be treated as path separators. This is how it is currently implemented in LLVM path library and VFS library didn't perform any path separator conversion as well. My patch didn't change this behavior.

Do you think we still need to add additional tests?

[haowei]

Rebased the change to solve the merge conflicts on the bots.

[bnbarham]

LGTM. If you could put up a PR after to fix the use of sys::fs::make_absolute that would be appreciated 🙇.

[haowei]

In D137473#3993584, @bnbarham wrote:

LGTM. If you could put up a PR after to fix the use of sys::fs::make_absolute that would be appreciated 🙇.

Thanks, I will start working on the fix.

clang/test/VFS/Inputs/root-relative-overlay.yaml

+{
+  'version': 0,
+  'case-sensitive': false,
+  'overlay-relative': true,
+  'root-relative': 'yaml-dir',
+  'roots': [
+    { 'name': 'not_real.h', 'type': 'file',
+      'external-contents': 'actual_header.h'
+    },
+  ]
+}

clang/test/VFS/relative-path.c

 // RUN: mkdir -p %t
 // RUN: cd %t
 // RUN: sed -e "s@INPUT_DIR@%{/S:regex_replacement}/Inputs@g" -e "s@OUT_DIR@%{/t:regex_replacement}@g" %S/Inputs/vfsoverlay.yaml > %t.yaml
 // RUN: %clang_cc1 -Werror -I . -ivfsoverlay %t.yaml -fsyntax-only %s
 
+// RUN: %clang_cc1 -Werror -I %S/Inputs -ivfsoverlay %S/Inputs/root-relative-overlay.yaml -fsyntax-only %s
+
 #include "not_real.h"
 
 void foo(void) {
   bar();
 }

llvm/include/llvm/Support/VirtualFileSystem.h

 ///   <optional configuration>
 ///   'roots': [
 ///              <directory entries>
 ///            ]
 /// }
 /// \endverbatim
 ///
 /// The roots may be absolute or relative. If relative they will be made
-/// absolute against the current working directory.
+/// absolute against either current working directory or the directory where
+/// the YAML file is located, depending on the 'root-relative' configuration.
 ///
 /// All configuration options are optional.
 ///   'case-sensitive': <boolean, default=(true for Posix, false for Windows)>
 ///   'use-external-names': <boolean, default=true>
+///   'root-relative': <string, one of 'cwd' or 'yaml-dir', default='cwd'>
 ///   'overlay-relative': <boolean, default=false>
 ///   'fallthrough': <boolean, default=true, deprecated - use 'redirecting-with'
 ///                   instead>
 ///   'redirecting-with': <string, one of 'fallthrough', 'fallback', or
 ///                        'redirect-only', default='fallthrough'>
 ///
 /// Virtual directories that list their contents are represented as
 /// \verbatim
enum class RedirectKind {
     /// Lookup the provided path first and if that fails, "fallback" to a
     /// lookup of the redirected path.
     Fallback,
     /// Only lookup the redirected path, do not lookup the originally provided
     /// path.
     RedirectOnly
   };
 
+  /// The type of relative path used by Roots.
+  enum class RootRelativeKind {
+    /// The roots are relative to the current working directory.
+    CWD,
+    /// The roots are relative to the directory where the YAML file locates.
+    YAMLDir
+  };
+
   /// A single file or directory in the VFS.
   class Entry {
     EntryKind Kind;
     std::string Name;
 
   public:
     Entry(EntryKind K, StringRef Name) : Kind(K), Name(Name) {}
     virtual ~Entry() = default;
private:
   std::vector<std::unique_ptr<Entry>> Roots;
 
   /// The current working directory of the file system.
   std::string WorkingDirectory;
 
   /// The file system to use for external references.
   IntrusiveRefCntPtr<FileSystem> ExternalFS;
 
-  /// If IsRelativeOverlay is set, this represents the directory
-  /// path that should be prefixed to each 'external-contents' entry
-  /// when reading from YAML files.
-  std::string ExternalContentsPrefixDir;
+  /// This represents the directory path that the YAML file is located.
+  /// This will be prefixed to each 'external-contents' if IsRelativeOverlay
+  /// is set. This will also be prefixed to each 'roots->name' if RootRelative
+  /// is set to RootRelativeKind::YAMLDir.
+  std::string YAMLFileDir;
 
   /// @name Configuration
   /// @{
 
   /// Whether to perform case-sensitive comparisons.
   ///
   /// Currently, case-insensitive matching only works correctly with ASCII.
   bool CaseSensitive = is_style_posix(sys::path::Style::native);
 
-  /// IsRelativeOverlay marks whether a ExternalContentsPrefixDir path must
+  /// IsRelativeOverlay marks whether a YAMLFileDir 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;
 
   /// Determines the lookups to perform, as well as their order. See
   /// \c RedirectKind for details.
   RedirectKind Redirection = RedirectKind::Fallthrough;
+
+  /// Determine the prefix directory if the roots are relative paths. See
+  /// \c RootRelativeKind for details.
+  RootRelativeKind RootRelative = RootRelativeKind::CWD;
   /// @}
 
   RedirectingFileSystem(IntrusiveRefCntPtr<FileSystem> ExternalFS);
 
   /// Looks up the path <tt>[Start, End)</tt> in \p From, possibly recursing
   /// into the contents of \p From if it is a directory. Returns a LookupResult
   /// giving the matched entry and, if that entry is a FileEntry or
   /// DirectoryRemapEntry, the path it redirects to in the external file system.
public:
   llvm::ErrorOr<std::string> getCurrentWorkingDirectory() const override;
 
   std::error_code setCurrentWorkingDirectory(const Twine &Path) override;
 
   std::error_code isLocal(const Twine &Path, bool &Result) override;
 
   std::error_code makeAbsolute(SmallVectorImpl<char> &Path) const override;
 
   directory_iterator dir_begin(const Twine &Dir, std::error_code &EC) override;
 
-  void setExternalContentsPrefixDir(StringRef PrefixDir);
+  void setYAMLFileDir(StringRef PrefixDir);
 
-  StringRef getExternalContentsPrefixDir() const;
+  StringRef getYAMLFileDir() const;
 
   /// Sets the redirection kind to \c Fallthrough if true or \c RedirectOnly
   /// otherwise. Will removed in the future, use \c setRedirection instead.
   void setFallthrough(bool Fallthrough);
 
   void setRedirection(RedirectingFileSystem::RedirectKind Kind);
 
   std::vector<llvm::StringRef> getRoots() const;

llvm/lib/Support/VirtualFileSystem.cpp

std::error_code RedirectingFileSystem::makeAbsolute(SmallVectorImpl<char> &Path) const {
   auto WorkingDir = getCurrentWorkingDirectory();
   if (!WorkingDir)
     return WorkingDir.getError();
 
   // We can't use sys::fs::make_absolute because that assumes the path style
   // is native and there is no way to override that.  Since we know WorkingDir
   // is absolute, we can use it to determine which style we actually have and
   // append Path ourselves.
   sys::path::Style style = sys::path::Style::windows_backslash;
   if (sys::path::is_absolute(WorkingDir.get(), sys::path::Style::posix)) {
     style = sys::path::Style::posix;
   } else {
     // Distinguish between windows_backslash and windows_slash; getExistingStyle
     // returns posix for a path with windows_slash.
     if (getExistingStyle(WorkingDir.get()) !=
         sys::path::Style::windows_backslash)
       style = sys::path::Style::windows_slash;
directory_iterator RedirectingFileSystem::dir_begin(const Twine &Dir,
 
   directory_iterator Combined{
       std::make_shared<CombiningDirIterImpl>(Iters, EC)};
   if (EC)
     return {};
   return Combined;
 }
 
-void RedirectingFileSystem::setExternalContentsPrefixDir(StringRef PrefixDir) {
-  ExternalContentsPrefixDir = PrefixDir.str();
+void RedirectingFileSystem::setYAMLFileDir(StringRef Dir) {
+  YAMLFileDir = Dir.str();
 }
 
-StringRef RedirectingFileSystem::getExternalContentsPrefixDir() const {
-  return ExternalContentsPrefixDir;
-}
+StringRef RedirectingFileSystem::getYAMLFileDir() const { return YAMLFileDir; }
 
 void RedirectingFileSystem::setFallthrough(bool Fallthrough) {
   if (Fallthrough) {
     Redirection = RedirectingFileSystem::RedirectKind::Fallthrough;
   } else {
     Redirection = RedirectingFileSystem::RedirectKind::RedirectOnly;
   }
 }
parseRedirectKind(yaml::Node *N) {
     } else if (Value.equals_insensitive("fallback")) {
       return RedirectingFileSystem::RedirectKind::Fallback;
     } else if (Value.equals_insensitive("redirect-only")) {
       return RedirectingFileSystem::RedirectKind::RedirectOnly;
     }
     return None;
   }
 
+  Optional<RedirectingFileSystem::RootRelativeKind>
+  parseRootRelativeKind(yaml::Node *N) {
+    SmallString<9> Storage;
+    StringRef Value;
+    if (!parseScalarString(N, Value, Storage))
+      return None;
+    if (Value.equals_insensitive("cwd")) {
+      return RedirectingFileSystem::RootRelativeKind::CWD;
+    } else if (Value.equals_insensitive("yaml-dir")) {
+      return RedirectingFileSystem::RootRelativeKind::YAMLDir;
+    }
+    return None;
+  }
+
   struct KeyStatus {
     bool Required;
     bool Seen = false;
 
     KeyStatus(bool Required = false) : Required(Required) {}
   };
 
   using KeyStatusPair = std::pair<StringRef, KeyStatus>;
for (auto &I : *M) {
           return nullptr;
         }
         ContentsField = CF_External;
         if (!parseScalarString(I.getValue(), Value, Buffer))
           return nullptr;
 
         SmallString<256> FullPath;
         if (FS->IsRelativeOverlay) {
-          FullPath = FS->getExternalContentsPrefixDir();
+          FullPath = FS->getYAMLFileDir();
           assert(!FullPath.empty() &&
                  "External contents prefix directory must exist");
           llvm::sys::path::append(FullPath, Value);
         } else {
           FullPath = Value;
         }
 
         // Guarantee that old YAML files containing paths with ".." and "."
parseEntry(yaml::Node *N, RedirectingFileSystem *FS, bool IsRootEntry) {
     if (IsRootEntry) {
       // VFS root entries may be in either Posix or Windows style.  Figure out
       // which style we have, and use it consistently.
       if (sys::path::is_absolute(Name, sys::path::Style::posix)) {
         path_style = sys::path::Style::posix;
       } else if (sys::path::is_absolute(Name,
                                         sys::path::Style::windows_backslash)) {
         path_style = sys::path::Style::windows_backslash;
+      } else if (FS->RootRelative ==
+                 RedirectingFileSystem::RootRelativeKind::YAMLDir) {
+        SmallString<256> FullPath = FS->getYAMLFileDir();
+        assert(!FullPath.empty() && "YAML file directory must exist");
+        sys::fs::make_absolute(FS->getYAMLFileDir(), Name);
+        Name = canonicalize(Name);
+        path_style = sys::path::is_absolute(Name, sys::path::Style::posix)
+                         ? sys::path::Style::posix
+                         : sys::path::Style::windows_backslash;
       } else {
         // Relative VFS root entries are made absolute to the current working
         // directory, then we can determine the path style from that.
         auto EC = sys::fs::make_absolute(Name);
         if (EC) {
           assert(NameValueNode && "Name presence should be checked earlier");
           error(
               NameValueNode,
               "entry with relative path at the root level is not discoverable");
           return nullptr;
         }
         path_style = sys::path::is_absolute(Name, sys::path::Style::posix)
                          ? sys::path::Style::posix
                          : sys::path::Style::windows_backslash;
       }
     }
 
     // Remove trailing slash(es), being careful not to remove the root path
     StringRef Trimmed = Name;
     size_t RootPathLen = sys::path::root_path(Trimmed, path_style).size();
     while (Trimmed.size() > RootPathLen &&
            sys::path::is_separator(Trimmed.back(), path_style))
if (!Top) {
       error(Root, "expected mapping node");
       return false;
     }
 
     KeyStatusPair Fields[] = {
         KeyStatusPair("version", true),
         KeyStatusPair("case-sensitive", false),
         KeyStatusPair("use-external-names", false),
+        KeyStatusPair("root-relative", false),
         KeyStatusPair("overlay-relative", false),
         KeyStatusPair("fallthrough", false),
         KeyStatusPair("redirecting-with", false),
         KeyStatusPair("roots", true),
     };
 
     DenseMap<StringRef, KeyStatus> Keys(std::begin(Fields), std::end(Fields));
     std::vector<std::unique_ptr<RedirectingFileSystem::Entry>> RootEntries;
for (auto &I : *Top) {
         }
 
         if (auto Kind = parseRedirectKind(I.getValue())) {
           FS->Redirection = *Kind;
         } else {
           error(I.getValue(), "expected valid redirect kind");
           return false;
         }
+      } else if (Key == "root-relative") {
+        if (auto Kind = parseRootRelativeKind(I.getValue())) {
+          FS->RootRelative = *Kind;
+        } else {
+          error(I.getValue(), "expected valid root-relative kind");
+          return false;
+        }
       } else {
         llvm_unreachable("key missing from Keys");
       }
     }
 
     if (Stream.failed())
       return false;
 
RedirectingFileSystem::create(std::unique_ptr<MemoryBuffer> Buffer,
 
   if (!YAMLFilePath.empty()) {
     // Use the YAML path from -ivfsoverlay to compute the dir to be prefixed
     // to each 'external-contents' path.
     //
     // Example:
     //    -ivfsoverlay dummy.cache/vfs/vfs.yaml
     // yields:
-    //  FS->ExternalContentsPrefixDir => /<absolute_path_to>/dummy.cache/vfs
+    //  FS->YAMLFileDir => /<absolute_path_to>/dummy.cache/vfs
     //
     SmallString<256> OverlayAbsDir = sys::path::parent_path(YAMLFilePath);
     std::error_code EC = llvm::sys::fs::make_absolute(OverlayAbsDir);
     assert(!EC && "Overlay dir final path must be absolute");
     (void)EC;
-    FS->setExternalContentsPrefixDir(OverlayAbsDir);
+    FS->setYAMLFileDir(OverlayAbsDir);
   }
 
   if (!P.parse(Root, FS.get()))
     return nullptr;
 
   return FS;
 }