Improve VFS compatibility on Windows

[amccarth]

Summary

Keys in a virtual file system can be in Posix or Windows form or even a combination of the two. Many VFS tests (and a few Clang tests) were XFAILed on Windows because of false negatives when comparing paths.

First, we default CaseSenstive to false on Windows. This allows drive letters like "D:" to match "d:". Windows filesystems are, by default, case insensitive, so this makes sense even beyond the drive letter.

Second, we allow slashes to match backslashes when they're used as the root component of a path.

Both of these changes are limited to RedirectingFileSystems, so there's little chance of affecting other path handling.

These changes allow eleven of the VFS tests to pass on Windows as well as three other Clang tests, so they have re-enabled (partially addressing PR43272).

[rnk]

This looks good to me. I had one suggestion, and I would also like to hear from another reviewer who has more ownership over VFS.

llvm/include/llvm/Support/VirtualFileSystem.h

657

IIUC, this method receives path components, which must not contain path separators, or the root path component, \ or /. Is there some way to express that invariant? Maybe just comment like "A path component must not contain path separators, unless it is the root component, which is represented as a single path separator," or an assert with the same effect.

llvm/lib/Support/VirtualFileSystem.cpp

1666

Unrelated, but I wonder if this needs to be while instead of if to handle repeated foo/./././bar.

[amccarth]

Friendly ping for any VFS experts to comment.

llvm/include/llvm/Support/VirtualFileSystem.h

657

I felt the assumption is implicit in "path component," but I've made the comment more explicit. Note that the slash/backslash checks are checking the entire string. I hope that reinforces the idea that these would be path roots.

llvm/lib/Support/VirtualFileSystem.cpp

1666

I'm fairly certain I'll be looking into this further as I try to resolve the remaining VFS tests that don't run on Windows. I'd love to eliminate another #ifdef, but I want to keep this patch focused.

[amccarth]

Modified comment per feedback.

[Bigcheese]

lgtm. Looking at the blame it would be best if @JDevlieghere @vsapsai or @benlangmuir could take a look also.

[vsapsai]

There is one issue with a comment, apart of that everything looks good to me. Looked into replacing slash and backslash literals with calls to llvm::sys::path::get_separator or llvm::sys::path::is_separator but in my attempts failed to improve existing patch.

llvm/include/llvm/Support/VirtualFileSystem.h

535

I believe changing default CaseSensitive value makes this comment incorrect. Have no strong opinion on making case-insensitive default on Windows because a better requirement is for VFS generators to specify 'case-sensitive' explicitly instead of relying on default values.

[amccarth]

Thanks for the review.

In D69958#1744861, @vsapsai wrote:

There is one issue with a comment, apart of that everything looks good to me. Looked into replacing slash and backslash literals with calls to llvm::sys::path::get_separator or llvm::sys::path::is_separator but in my attempts failed to improve existing patch.

Yes, I originally started down the path of trying to "normalize" path styles (like slash direction), but that always led to collateral damage because we literally have situations where a path is a hybrid of both Windows and Posix. Rather than writing Windows and Posix versions of the tests, this patch takes the approach that choosing a path style is out-of-scope, which I'm told is Clang's general strategy.

llvm/include/llvm/Support/VirtualFileSystem.h

535

Make CaseSensitive system-dependent aligns with UseCanonicalizePaths, so this feels pretty consistent. (Perhaps in a future path, we can fix both of those.) In the mean time, I'll update the comment.

[amccarth]

Corrected comment about default for case sensitivity.

clang/test/Index/index-module-with-vfs.m

-// FIXME: PR43272
-// XFAIL: system-windows
-
 @import ModuleNeedsVFS;
 
 void foo() {
   module_needs_vfs();
   base_module_needs_vfs();
 }
 
 // RUN: rm -rf %t.cache
 // RUN: sed -e "s@INPUT_DIR@%/S/Inputs@g" -e "s@OUT_DIR@%/t@g" %S/Inputs/vfsoverlay.yaml > %t.yaml
 // RUN: c-index-test -index-file %s -fmodules-cache-path=%t.cache -fmodules -F %t -I %t \
 // RUN:              -ivfsoverlay %t.yaml -Xclang -fdisable-module-hash | FileCheck %s
 
-// CHECK: [importedASTFile]: {{.*}}ModuleNeedsVFS.pcm | loc: 4:1 | name: "ModuleNeedsVFS" | isImplicit: 0
+// CHECK: [importedASTFile]: {{.*}}ModuleNeedsVFS.pcm | loc: 1:1 | name: "ModuleNeedsVFS" | isImplicit: 0
 // CHECK: [indexEntityReference]: kind: function | name: module_needs_vfs
 // CHECK: [indexEntityReference]: kind: function | name: base_module_needs_vfs
 
 // RUN: c-index-test -index-tu %t.cache/ModuleNeedsVFS.pcm | FileCheck %s -check-prefix=CHECK-MOD
 
 // CHECK-MOD: [ppIncludedFile]: {{.*}}module_needs_vfs.h 
 // CHECK-MOD: [importedASTFile]: {{.*}}BaseModuleNeedsVFS.pcm
 // CHECK-MOD: [indexEntityReference]: kind: function | name: base_module_needs_vfs
 
 // RUN: c-index-test -index-tu %t.cache/BaseModuleNeedsVFS.pcm | FileCheck %s -check-prefix=CHECK-MOD2
 
 // CHECK-MOD2: [ppIncludedFile]: {{.*}}base_module_needs_vfs.h

clang/test/Modules/double-quotes.m

-// FIXME: PR43272
-// XFAIL: system-windows
-
 // RUN: rm -rf %t
 // RUN: mkdir %t
 
 // RUN: %hmaptool write %S/Inputs/double-quotes/a.hmap.json %t/a.hmap
 // RUN: %hmaptool write %S/Inputs/double-quotes/x.hmap.json %t/x.hmap
 
 // RUN: sed -e "s@TEST_DIR@%/S/Inputs/double-quotes@g" \
 // RUN:   %S/Inputs/double-quotes/z.yaml > %t/z.yaml

clang/test/Modules/framework-public-includes-private.m

-// FIXME: PR43272
-// XFAIL: system-windows
-
 // RUN: rm -rf %t
 // RUN: mkdir %t
 
 // RUN: %hmaptool write %S/Inputs/framework-public-includes-private/a.hmap.json %t/a.hmap
 // RUN: %hmaptool write %S/Inputs/framework-public-includes-private/z.hmap.json %t/z.hmap
 
 // RUN: sed -e "s@TEST_DIR@%/S/Inputs/framework-public-includes-private@g" \
 // RUN:   %S/Inputs/framework-public-includes-private/z.yaml > %t/z.yaml

clang/test/VFS/external-names.c

 // RUN: sed -e "s@INPUT_DIR@%/S/Inputs@g" -e "s@OUT_DIR@%/t@g" -e "s@EXTERNAL_NAMES@true@" %S/Inputs/use-external-names.yaml > %t.external.yaml
 // RUN: sed -e "s@INPUT_DIR@%/S/Inputs@g" -e "s@OUT_DIR@%/t@g" -e "s@EXTERNAL_NAMES@false@" %S/Inputs/use-external-names.yaml > %t.yaml
 
-// FIXME: PR43272
-// XFAIL: system-windows
-
 #include "external-names.h"
 #ifdef REINCLUDE
 #include "external-names.h"
 #endif
 
 ////
 // Preprocessor (__FILE__ macro and # directives):
 

clang/test/VFS/framework-import.m

 // RUN: sed -e "s@INPUT_DIR@%/S/Inputs@g" -e "s@OUT_DIR@%/t@g" %S/Inputs/vfsoverlay.yaml > %t.yaml
 // RUN: %clang_cc1 -Werror -F %t -ivfsoverlay %t.yaml -fsyntax-only %s
 
-// FIXME: PR43272
-// XFAIL: system-windows
-
 #import <SomeFramework/public_header.h>
 
 void foo() {
   from_framework();
 }

clang/test/VFS/implicit-include.c

 // RUN: sed -e "s@INPUT_DIR@%/S/Inputs@g" -e "s@OUT_DIR@%/t@g" %S/Inputs/vfsoverlay.yaml > %t.yaml
 // RUN: %clang_cc1 -Werror -ivfsoverlay %t.yaml -I %t -include "not_real.h" -fsyntax-only %s
 
-// FIXME: PR43272
-// XFAIL: system-windows
-
 void foo() {
   bar();
 }

clang/test/VFS/include-mixed-real-and-virtual.c

 // RUN: rm -rf %t
 // RUN: mkdir -p %t
 // RUN: echo "void baz(void);" > %t/real.h
 // RUN: sed -e "s@INPUT_DIR@%/S/Inputs@g" -e "s@OUT_DIR@%/t@g" %S/Inputs/vfsoverlay.yaml > %t.yaml
 // RUN: %clang_cc1 -Werror -ivfsoverlay %t.yaml -I %t -fsyntax-only %s
 
-// FIXME: PR43272
-// XFAIL: system-windows
-
 #include "not_real.h"
 #include "real.h"
 
 void foo() {
   bar();
   baz();
 }

clang/test/VFS/include-real-from-virtual.c

 // RUN: rm -rf %t
 // RUN: mkdir -p %t
 // RUN: echo "void baz(void);" > %t/real.h
 // RUN: sed -e "s@INPUT_DIR@%/S/Inputs@g" -e "s@OUT_DIR@%/t@g" %S/Inputs/vfsoverlay.yaml > %t.yaml
 // RUN: %clang_cc1 -Werror -ivfsoverlay %t.yaml -I %t -fsyntax-only %s
 
-// FIXME: PR43272
-// XFAIL: system-windows
-
 #include "include_real.h"
 
 void foo() {
   baz();
 }

clang/test/VFS/include-virtual-from-real.c

 // RUN: rm -rf %t
 // RUN: mkdir -p %t
 // RUN: echo '#include "not_real.h"' > %t/include_not_real.h
 // RUN: sed -e "s@INPUT_DIR@%/S/Inputs@g" -e "s@OUT_DIR@%/t@g" %S/Inputs/vfsoverlay.yaml > %t.yaml
 // RUN: %clang_cc1 -Werror -ivfsoverlay %t.yaml -I %t -fsyntax-only %s
 
-// FIXME: PR43272
-// XFAIL: system-windows
-
 #include "include_not_real.h"
 
 void foo() {
   bar();
 }

clang/test/VFS/include.c

 // RUN: sed -e "s@INPUT_DIR@%/S/Inputs@g" -e "s@OUT_DIR@%/t@g" %S/Inputs/vfsoverlay.yaml > %t.yaml
 // RUN: %clang_cc1 -Werror -I %t -ivfsoverlay %t.yaml -fsyntax-only %s
 
-// FIXME: PR43272
-// XFAIL: system-windows
-
 #include "not_real.h"
 
 void foo() {
   bar();
 }

clang/test/VFS/incomplete-umbrella.m

 // RUN: rm -rf %t
 // RUN: mkdir -p %t/Incomplete.framework/Headers
 // RUN: echo '// IncompleteReal.h' > %t/Incomplete.framework/Headers/IncompleteReal.h
 // RUN: sed -e "s@INPUT_DIR@%/S/Inputs@g" -e "s@OUT_DIR@%/t@g" %S/Inputs/vfsoverlay.yaml > %t.yaml
 // RUN: not %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t \
 // RUN:     -ivfsoverlay %t.yaml -F %t -fsyntax-only %s 2>&1 | FileCheck %s
 
-// FIXME: PR43272
-// XFAIL: system-windows
-
 @import Incomplete;
 // CHECK: umbrella header for module 'Incomplete' {{.*}}IncompleteVFS.h
 // CHECK: umbrella header for module 'Incomplete' {{.*}}IncompleteReal.h
 // CHECK: could not build module 'Incomplete'

clang/test/VFS/module-import.m

 // RUN: rm -rf %t
 // RUN: sed -e "s@INPUT_DIR@%/S/Inputs@g" -e "s@OUT_DIR@%/t@g" %S/Inputs/vfsoverlay.yaml > %t.yaml
 // RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -ivfsoverlay %t.yaml -I %t -fsyntax-only %s
 
-// FIXME: PR43272
-// XFAIL: system-windows
-
 @import not_real;
 
 void foo() {
   bar();
 }
 
 // Import a submodule that is defined in actual_module2.map, which is only
 // mapped in vfsoverlay2.yaml.

clang/test/VFS/real-path-found-first.m

 // This test is for cases where we lookup a file by its 'real' path before we
 // use its VFS-mapped path. If we accidentally use the real path in header
 // search, we will not find a module for the headers.  To test that we
 // intentionally rebuild modules, since the precompiled module file refers to
 // the dependency files by real path.
 
-// FIXME: PR43272
-// XFAIL: system-windows
-
 // RUN: rm -rf %t %t-cache %t.pch
 // RUN: mkdir -p %t/SomeFramework.framework/Modules
 // RUN: cat %S/Inputs/some_frame_module.map > %t/SomeFramework.framework/Modules/module.modulemap
 // RUN: sed -e "s@INPUT_DIR@%/S/Inputs@g" -e "s@OUT_DIR@%/t@g" %S/Inputs/vfsoverlay.yaml > %t.yaml
 
 // Build
 // RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t-cache -F %t \
 // RUN:     -ivfsoverlay %t.yaml -fsyntax-only %s -verify -Wauto-import \

clang/test/VFS/relative-path.c

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

clang/test/VFS/umbrella-framework-import-skipnonexist.m

 // REQUIRES: crash-recovery
 
-// FIXME: PR43272
-// XFAIL: system-windows
-
 // RUN: rm -rf %t
 // RUN: mkdir -p %t/vdir %t/outdir %t/cache
 // RUN: cp -R %S/Inputs/Bar.framework %t/outdir/
 //
 // RUN: sed -e "s@VDIR@%/t/vdir@g" -e "s@OUT_DIR@%/t/outdir@g" %S/Inputs/bar-headers.yaml > %t/vdir/bar-headers.yaml
 // RUN: rm -f %t/outdir/Bar.framework/Headers/B.h
 // RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/cache -ivfsoverlay %t/vdir/bar-headers.yaml -F %t/vdir -fsyntax-only %s
 
 @import Bar;

llvm/include/llvm/Support/VirtualFileSystem.h

 ///   <optional configuration>
 ///   'roots': [
 ///              <directory entries>
 ///            ]
 /// }
 /// \endverbatim
 ///
 /// All configuration options are optional.
-///   'case-sensitive': <boolean, default=true>
+///   'case-sensitive': <boolean, default=(true for Posix, false for Windows)>
 ///   'use-external-names': <boolean, default=true>
 ///   'overlay-relative': <boolean, default=false>
 ///   'fallthrough': <boolean, default=true>
 ///
 /// Virtual directories are represented as
 /// \verbatim
 /// {
 ///   'type': 'directory',

 private:
   friend class VFSFromYamlDirIterImpl;
   friend class RedirectingFileSystemParser;
 
   bool shouldUseExternalFS() const {
     return ExternalFSValidWD && IsFallthrough;
   }
 
+  // In a RedirectingFileSystem, keys can be specified in Posix or Windows
+  // style (or even a mixture of both), so this comparison helper allows
+  // slashes (representing a root) to match backslashes (and vice versa).  Note
+  // that, other than the root, patch components should not contain slashes or
+  // backslashes.
+  bool pathComponentMatches(llvm::StringRef lhs, llvm::StringRef rhs) const {
+    if ((CaseSensitive ? lhs.equals(rhs) : lhs.equals_lower(rhs)))
+      return true;
+    return (lhs == "/" && rhs == "\\") || (lhs == "\\" && rhs == "/");
+  }
+
   /// The root(s) of the virtual file system.
   std::vector<std::unique_ptr<Entry>> Roots;
 
   /// The current working directory of the file system.
   std::string WorkingDirectory;
 
   /// Whether the current working directory is valid for the external FS.
   bool ExternalFSValidWD = false;
 
   /// 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;
 
   /// @name Configuration
   /// @{
 
   /// Whether to perform case-sensitive comparisons.
   ///
   /// Currently, case-insensitive matching only works correctly with ASCII.
-  bool CaseSensitive = true;
+  bool CaseSensitive =
+#ifdef _WIN32
+      false;
+#else
+      true;
+#endif
 
   /// 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;

llvm/lib/Support/VirtualFileSystem.cpp

RedirectingFileSystem::lookupPath(sys::path::const_iterator Start,
                                   RedirectingFileSystem::Entry *From) const {
 #ifndef _WIN32
   assert(!isTraversalComponent(*Start) &&
          !isTraversalComponent(From->getName()) &&
          "Paths should not contain traversal components");
 #else
   // FIXME: this is here to support windows, remove it once canonicalized
   // paths become globally default.
   if (Start->equals("."))
     ++Start;
 #endif
 
   StringRef FromName = From->getName();
 
   // Forward the search to the next component in case this is an empty one.
   if (!FromName.empty()) {
-    if (CaseSensitive ? !Start->equals(FromName)
-                      : !Start->equals_lower(FromName))
-      // failure to match
+    if (!pathComponentMatches(*Start, FromName))
       return make_error_code(llvm::errc::no_such_file_or_directory);
 
     ++Start;
 
     if (Start == End) {
       // Match!
       return From;
     }
   }
 
   auto *DE = dyn_cast<RedirectingFileSystem::RedirectingDirectoryEntry>(From);
   if (!DE)
     return make_error_code(llvm::errc::not_a_directory);
 
   for (const std::unique_ptr<RedirectingFileSystem::Entry> &DirEntry :
        llvm::make_range(DE->contents_begin(), DE->contents_end())) {
     ErrorOr<RedirectingFileSystem::Entry *> Result =
         lookupPath(Start, End, DirEntry.get());
     if (Result || Result.getError() != llvm::errc::no_such_file_or_directory)
       return Result;
   }
+
   return make_error_code(llvm::errc::no_such_file_or_directory);
 }
 
 static Status getRedirectedFileStatus(const Twine &Path, bool UseExternalNames,
                                       Status ExternalStatus) {
   Status S = ExternalStatus;
   if (!UseExternalNames)
     S = Status::copyWithNewName(S, Path);