Enable most VFS tests on Windows

[amccarth]

Summary

Inconsistencies in when/if paths are cannoicalized prevented VFS tests from working on Windows. This fixes the primary cause and enables most of the tests (15/20).

Remaining VFS tests are still XFAILed on Windows because there are additional causes in some code paths. I plan to diagnose and correct those in a future patch.

[rnk]

+VFS people, mostly just to let them know
@arphaman @Bigcheese @vsapsai

Code changes all look good to me, but I had a bunch of questions.

clang/lib/Lex/HeaderSearch.cpp

782–783

I'm surprised this just works. :) You ran the whole clang test suite, and nothing broke, right? I would've expected something to accidentally depend on this behavior.

clang/test/VFS/external-names.c

4–5

You made changes to this file, but it's still XFAILed. Is that intentional?

llvm/lib/Support/VirtualFileSystem.cpp

1619–1620

After reading the code for a while, I see that drive letters are handled as just another component in the path, so I guess this code all works without explicitly considering drive letters.

1839

I think this will ultimately produce paths that look like: \C:\foo\bar\baz. Ultimately, we loop over the components above and repeatedly call sys::path::append on them.

Clang only uses this function for collecting module dependency info, it seems. Maybe that's what the remaining failures are about?

[amccarth]

Somehow Phabricator failed to notify me that you'd already left comments. I even searched my inbox to see if I'd just missed the notification. Nope. Nothing. I came here today to ping you for the review and discovered you'd already done your part.

Anyway, I'll update the patch and ping again when that's done.

Thanks.

clang/lib/Lex/HeaderSearch.cpp

782–783

Yeah, I recall it all working before, but, double-checking today, I'm seeing some collateral damage. Investigating.

clang/test/VFS/external-names.c

4–5

The change removes one obstacle, but the test still fails on Windows because there's still an outstanding bug. If you like, I can hoist this change out until I have a patch to fix the outstanding issue.

llvm/lib/Support/VirtualFileSystem.cpp

1839

I didn't encounter anything like \C:\foo. Perhaps the (enabled) VFS tests all prime the roots so that never happens here. I'll see if I can add a test to uncover that problem.

And, yes, use of this function seems limited to reading VFS paths from the YAML, so its scope is pretty limited. At least some of the remaining problems have to do with the fact that, while the target paths can always be expressed in the host style, the key paths can be in either Posix or Windows format, which leads to confusion when you try to get answers from functions like sys::path::is_absolute where you have to pick the right style to get the right answer.

[amccarth]

I create a new review thread for my improved patch shortly. These changes were too wide-ranging.

clang/lib/Lex/HeaderSearch.cpp

Optional<FileEntryRef> HeaderSearch::LookupFile(
   StringRef MSFE_Name;
 
   // Unless disabled, check to see if the file is in the #includer's
   // directory.  This cannot be based on CurDir, because each includer could be
   // a #include of a subdirectory (#include "foo/bar.h") and a subsequent
   // include of "baz.h" should resolve to "whatever/foo/baz.h".
   // This search is not done for <> headers.
   if (!Includers.empty() && !isAngled && !NoCurDirSearch) {
-    SmallString<1024> TmpDir;
+    SmallString<1024> TmpPath;
     bool First = true;
     for (const auto &IncluderAndDir : Includers) {
       const FileEntry *Includer = IncluderAndDir.first;
 
       // Concatenate the requested file onto the directory.
-      // FIXME: Portability.  Filename concatenation should be in sys::Path.
-      TmpDir = IncluderAndDir.second->getName();
-      TmpDir.push_back('/');
-      TmpDir.append(Filename.begin(), Filename.end());
+      TmpPath = IncluderAndDir.second->getName();
+      llvm::sys::path::append(TmpPath, Filename);
 
       // FIXME: We don't cache the result of getFileInfo across the call to
       // getFileAndSuggestModule, because it's a reference to an element of
       // a container that could be reallocated across this call.
       //
       // If we have no includer, that means we're processing a #include
       // from a module build. We should treat this as a system header if we're
       // building a [system] module.
       bool IncluderIsSystemHeader =
           Includer ? getFileInfo(Includer).DirInfo != SrcMgr::C_User :
           BuildSystemModule;
       if (Optional<FileEntryRef> FE = getFileAndSuggestModule(
-              TmpDir, IncludeLoc, IncluderAndDir.second, IncluderIsSystemHeader,
+              TmpPath, IncludeLoc, IncluderAndDir.second, IncluderIsSystemHeader,
               RequestingModule, SuggestedModule)) {
         if (!Includer) {
           assert(First && "only first includer can have no file");
           return FE;
         }
 
         // Leave CurDir unset.
         // This file is a system header or C++ unfriendly if the old file is.

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):
 
 // RUN: %clang_cc1 -I %t -ivfsoverlay %t.external.yaml -E %s | FileCheck -check-prefix=CHECK-PP-EXTERNAL %s
-// CHECK-PP-EXTERNAL: # {{[0-9]*}} "[[NAME:.*Inputs.external-names.h]]"
+// CHECK-PP-EXTERNAL: # {{[0-9]*}} "[[NAME:.*Inputs(\\\\|/)external-names.h]]"
 // CHECK-PP-EXTERNAL-NEXT: void foo(char **c) {
 // CHECK-PP-EXTERNAL-NEXT: *c = "[[NAME]]";
 
 // RUN: %clang_cc1 -I %t -ivfsoverlay %t.yaml -E %s | FileCheck -check-prefix=CHECK-PP %s
 // CHECK-PP-NOT: Inputs
 
 ////
 // Diagnostics:
 
 // RUN: %clang_cc1 -I %t -ivfsoverlay %t.external.yaml -fsyntax-only %s 2>&1 | FileCheck -check-prefix=CHECK-DIAG-EXTERNAL %s
 // CHECK-DIAG-EXTERNAL: {{.*}}Inputs{{.}}external-names.h:{{[0-9]*:[0-9]*}}: warning: incompatible pointer
 
 // RUN: %clang_cc1 -I %t -ivfsoverlay %t.yaml -fsyntax-only %s 2>&1 | FileCheck -check-prefix=CHECK-DIAG %s
 // CHECK-DIAG-NOT: Inputs
 
 ////
 // Debug info
 
 // RUN: %clang_cc1 -I %t -ivfsoverlay %t.external.yaml -triple %itanium_abi_triple -debug-info-kind=limited -emit-llvm %s -o - | FileCheck -check-prefix=CHECK-DEBUG-EXTERNAL %s
 // CHECK-DEBUG-EXTERNAL: !DISubprogram({{.*}}file: ![[Num:[0-9]+]]
-// CHECK-DEBUG-EXTERNAL: ![[Num]] = !DIFile(filename: "{{[^"]*}}Inputs{{.}}external-names.h"
+// CHECK-DEBUG-EXTERNAL: ![[Num]] = !DIFile(filename: "{{[^"]*}}Inputs(\5C|/)external-names.h"
 
 // RUN: %clang_cc1 -I %t -ivfsoverlay %t.yaml -triple %itanium_abi_triple -debug-info-kind=limited -emit-llvm %s -o - | FileCheck -check-prefix=CHECK-DEBUG %s
 // CHECK-DEBUG-NOT: Inputs
 
 ////
 // Dependency file
 
 // RUN: %clang_cc1 -D REINCLUDE -I %t -ivfsoverlay %t.external.yaml -Eonly %s -MTfoo -dependency-file %t.external.dep
 // RUN: echo "EOF" >> %t.external.dep
 // RUN: cat %t.external.dep | FileCheck --check-prefix=CHECK-DEP-EXTERNAL %s
 // CHECK-DEP-EXTERNAL: Inputs{{.}}external-names.h
 // CHECK-DEP-EXTERNAL-NEXT: EOF
 
 // RUN: %clang_cc1 -D REINCLUDE -I %t -ivfsoverlay %t.yaml -Eonly %s -MTfoo -dependency-file %t.dep
 // RUN: cat %t.dep | FileCheck --check-prefix=CHECK-DEP %s
 // CHECK-DEP-NOT: Inputs

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/lib/Support/VirtualFileSystem.cpp

for (auto &I : *M) {
         return nullptr;
 
       StringRef Value;
       if (Key == "name") {
         if (!parseScalarString(I.getValue(), Value, Buffer))
           return nullptr;
 
         NameValueNode = I.getValue();
-        if (FS->UseCanonicalizedPaths) {
-          SmallString<256> Path(Value);
+        if (FS->UseCanonicalizedPaths) {
           // Guarantee that old YAML files containing paths with ".." and "."
           // are properly canonicalized before read into the VFS.
           Path = sys::path::remove_leading_dotslash(Path);
           sys::path::remove_dots(Path, /*remove_dot_dot=*/true);
-          Name = Path.str();
-        } else {
-          Name = Value;
         }
+        sys::path::native(Path);
+        Name = Path.str();
       } else if (Key == "type") {
         if (!parseScalarString(I.getValue(), Value, Buffer))
           return nullptr;
         if (Value == "file")
           Kind = RedirectingFileSystem::EK_File;
         else if (Value == "directory")
           Kind = RedirectingFileSystem::EK_Directory;
         else {
for (auto &I : *M) {
         }
 
         if (FS->UseCanonicalizedPaths) {
           // Guarantee that old YAML files containing paths with ".." and "."
           // are properly canonicalized before read into the VFS.
           FullPath = sys::path::remove_leading_dotslash(FullPath);
           sys::path::remove_dots(FullPath, /*remove_dot_dot=*/true);
         }
+        sys::path::native(FullPath);
         ExternalContentsPath = FullPath.str();
       } else if (Key == "use-external-name") {
         bool Val;
         if (!parseScalarBool(I.getValue(), Val))
           return nullptr;
         UseExternalName =
             Val ? RedirectingFileSystem::RedirectingFileEntry::NK_External
                 : RedirectingFileSystem::RedirectingFileEntry::NK_Virtual;
RedirectingFileSystem::lookupPath(const Twine &Path_) const {
   if (UseCanonicalizedPaths) {
     Path = sys::path::remove_leading_dotslash(Path);
     sys::path::remove_dots(Path, /*remove_dot_dot=*/true);
   }
 
   if (Path.empty())
     return make_error_code(llvm::errc::invalid_argument);
 
   sys::path::const_iterator Start = sys::path::begin(Path);
   sys::path::const_iterator End = sys::path::end(Path);
   for (const auto &Root : Roots) {
     ErrorOr<RedirectingFileSystem::Entry *> Result =
         lookupPath(Start, End, Root.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);
 }
void vfs::collectVFSFromYAML(std::unique_ptr<MemoryBuffer> Buffer,
                              SourceMgr::DiagHandlerTy DiagHandler,
                              StringRef YAMLFilePath,
                              SmallVectorImpl<YAMLVFSEntry> &CollectedEntries,
                              void *DiagContext,
                              IntrusiveRefCntPtr<FileSystem> ExternalFS) {
   RedirectingFileSystem *VFS = RedirectingFileSystem::create(
       std::move(Buffer), DiagHandler, YAMLFilePath, DiagContext,
       std::move(ExternalFS));
-  ErrorOr<RedirectingFileSystem::Entry *> RootE = VFS->lookupPath("/");
+  ErrorOr<RedirectingFileSystem::Entry *> RootE =
+      VFS->lookupPath(sys::path::get_separator());
   if (!RootE)
     return;
   SmallVector<StringRef, 8> Components;
-  Components.push_back("/");
+  Components.push_back(sys::path::get_separator());
   getVFSEntries(*RootE, Components, CollectedEntries);
 }
 
 UniqueID vfs::getNextVirtualUniqueID() {
   static std::atomic<unsigned> UID;
   unsigned ID = ++UID;
   // The following assumes that uint64_t max will never collide with a real
   // dev_t value from the OS.