[VFS] More consistent support for Windows

[amccarth]

Summary

Removed some #ifdefs specific to Windows handling of VFS paths. This eliminates most of the differences between the Windows and non-Windows code paths.

Making this work required some changes to account for the fact that VFS file paths can be Posix style or Windows style, so you cannot just assume that they use the host's native path style. In one case, this means implementing our own version of make_absolute, since the filesystem code in Support doesn't have styles in the sense that the path code does.

[amccarth]

Hold off on this one. It needs an explicit test for canonicalization.

[rnk]

I would add, take a look at the existing unit tests (llvm/unittests/Support/VirtualFileSystemTests.cpp) and audit for _WIN32 ifdefs that we can remove after this change.

[amccarth]

cc: +thakis, who expressed interest in seeing a fix for the text exempted on Windows.

[amccarth]

Ping?

[rnk]

I was worried there would be more duplication. :) As far as rebuilding the wheel square goes, this seems totally reasonable. I had a question and a nit, though.

llvm/lib/Support/VirtualFileSystem.cpp

1112–1114

It looks like you assign posix style if the working directory has windows path style. Is that what you meant to do?

1118

Result.endswith(sys::path::get_separator(style)) saves a boolean condition

[amccarth]

llvm/lib/Support/VirtualFileSystem.cpp

1112–1114

Oops! Nice catch.

Although some of the tests exercise this path, none of them caught the problem because the effect here is very minor (direction of a slash). I've turned the logic around, which makes the more common case (posix) dependent upon the condition.

1118

I hadn't realized ends_with was added to std::string. But it's a C++20 enhancement, which I think is too new for us right now. I'll use StringRef::endswith.

[amccarth]

Addressed rnk's comments: fixed path style detection bug, and used StringRef::endswith.

[rnk]

lgtm, thanks!

[amccarth]

This landed in da45bd232165eab5d6ec4f1f4f18db8644289142

It wasn't automatically closed because I misspelled "Differential Revision."

clang/test/VFS/external-names.c

 // RUN: sed -e "s@INPUT_DIR@%{/S:regex_replacement}/Inputs@g" -e "s@OUT_DIR@%{/t:regex_replacement}@g" -e "s@EXTERNAL_NAMES@true@" %S/Inputs/use-external-names.yaml > %t.external.yaml
 // RUN: sed -e "s@INPUT_DIR@%{/S:regex_replacement}/Inputs@g" -e "s@OUT_DIR@%{/t:regex_replacement}@g" -e "s@EXTERNAL_NAMES@false@" %S/Inputs/use-external-names.yaml > %t.yaml
 
 #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:

llvm/include/llvm/Support/VirtualFileSystem.h

#endif
   /// 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;
-#else
-      true;
-#endif
-
   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.
   ErrorOr<Entry *> lookupPath(llvm::sys::path::const_iterator Start,
                               llvm::sys::path::const_iterator End,
                               Entry *From) const;
 

llvm/lib/Support/VirtualFileSystem.cpp

 
 } // namespace vfs
 } // namespace llvm
 
 //===-----------------------------------------------------------------------===/
 // RedirectingFileSystem implementation
 //===-----------------------------------------------------------------------===/
 
+namespace {
+
+/// Removes leading "./" as well as path components like ".." and ".".
+static llvm::SmallString<256> canonicalize(llvm::StringRef Path) {
+  // First detect the path style in use by checking the first separator.
+  llvm::sys::path::Style style = llvm::sys::path::Style::native;
+  const size_t n = Path.find_first_of("/\\");
+  if (n != static_cast<size_t>(-1))
+    style = (Path[n] == '/') ? llvm::sys::path::Style::posix
+                             : llvm::sys::path::Style::windows;
+
+  // Now remove the dots.  Explicitly specifying the path style prevents the
+  // direction of the slashes from changing.
+  llvm::SmallString<256> result =
+      llvm::sys::path::remove_leading_dotslash(Path, style);
+  llvm::sys::path::remove_dots(result, /*remove_dot_dot=*/true, style);
+  return result;
+}
+
+} // anonymous namespace
+
+
 RedirectingFileSystem::RedirectingFileSystem(IntrusiveRefCntPtr<FileSystem> FS)
     : ExternalFS(std::move(FS)) {
   if (ExternalFS)
     if (auto ExternalWorkingDirectory =
             ExternalFS->getCurrentWorkingDirectory()) {
       WorkingDirectory = *ExternalWorkingDirectory;
       ExternalFSValidWD = true;
     }
std::error_code RedirectingFileSystem::makeAbsolute(SmallVectorImpl<char> &Path) const {
   if (llvm::sys::path::is_absolute(Path, llvm::sys::path::Style::posix) ||
       llvm::sys::path::is_absolute(Path, llvm::sys::path::Style::windows))
     return {};
 
   auto WorkingDir = getCurrentWorkingDirectory();
   if (!WorkingDir)
     return WorkingDir.getError();
 
-  llvm::sys::fs::make_absolute(WorkingDir.get(), Path);
+  // 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;
+  if (sys::path::is_absolute(WorkingDir.get(), sys::path::Style::posix)) {
+    style = sys::path::Style::posix;
+  }
+
+  std::string Result = WorkingDir.get();
+  StringRef Dir(Result);
+  if (!Dir.endswith(sys::path::get_separator(style))) {
+    Result += sys::path::get_separator(style);
+  }
+  Result.append(Path.data(), Path.size());
+  Path.assign(Result.begin(), Result.end());
+
   return {};
 }
 
 directory_iterator RedirectingFileSystem::dir_begin(const Twine &Dir,
                                                     std::error_code &EC) {
   ErrorOr<RedirectingFileSystem::Entry *> E = lookupPath(Dir);
   if (!E) {
     EC = E.getError();
KeyStatusPair Fields[] = {
         KeyStatusPair("use-external-name", false),
     };
 
     DenseMap<StringRef, KeyStatus> Keys(std::begin(Fields), std::end(Fields));
 
     bool HasContents = false; // external or otherwise
     std::vector<std::unique_ptr<RedirectingFileSystem::Entry>>
         EntryArrayContents;
-    std::string ExternalContentsPath;
-    std::string Name;
+    SmallString<256> ExternalContentsPath;
+    SmallString<256> Name;
     yaml::Node *NameValueNode = nullptr;
     auto UseExternalName =
         RedirectingFileSystem::RedirectingFileEntry::NK_NotSet;
     RedirectingFileSystem::EntryKind Kind;
 
     for (auto &I : *M) {
       StringRef Key;
       // Reuse the buffer for key and value, since we don't look at key after
       // parsing value.
       SmallString<256> Buffer;
       if (!parseScalarString(I.getKey(), Key, Buffer))
         return nullptr;
 
       if (!checkDuplicateOrUnknownKey(I.getKey(), Key, Keys))
         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);
-          // 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 = std::string(Path.str());
-        } else {
-          Name = std::string(Value);
-        }
+        Name = canonicalize(Value).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) {
           FullPath = FS->getExternalContentsPrefixDir();
           assert(!FullPath.empty() &&
                  "External contents prefix directory must exist");
           llvm::sys::path::append(FullPath, Value);
         } else {
           FullPath = Value;
         }
 
-        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);
-        }
-        ExternalContentsPath = std::string(FullPath.str());
+        FullPath = canonicalize(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;
       } else {

 RedirectingFileSystem::lookupPath(const Twine &Path_) const {
   SmallString<256> Path;
   Path_.toVector(Path);
 
   // Handle relative paths
   if (std::error_code EC = makeAbsolute(Path))
     return EC;
 
-  // Canonicalize path by removing ".", "..", "./", etc components. This is
-  // a VFS request, do bot bother about symlinks in the path components
+  // Canonicalize path by removing ".", "..", "./", components. This is
+  // a VFS request, do not bother about symlinks in the path components
   // but canonicalize in order to perform the correct entry search.
-  if (UseCanonicalizedPaths) {
-    Path = sys::path::remove_leading_dotslash(Path);
-    sys::path::remove_dots(Path, /*remove_dot_dot=*/true);
-  }
-
+  Path = canonicalize(Path);
   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);
 }
 
 ErrorOr<RedirectingFileSystem::Entry *>
 RedirectingFileSystem::lookupPath(sys::path::const_iterator Start,
                                   sys::path::const_iterator End,
                                   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 (!pathComponentMatches(*Start, FromName))
       return make_error_code(llvm::errc::no_such_file_or_directory);
 

llvm/unittests/Support/VirtualFileSystemTest.cpp

TEST_F(VFSFromYAMLTest, WorkingDirectoryFallthrough) {
 
   EC = FS->setCurrentWorkingDirectory("//root/bar");
   ASSERT_FALSE(EC);
 
   Status = FS->status("./baz/a");
   ASSERT_FALSE(Status.getError());
   EXPECT_TRUE(Status->exists());
 
-#if !defined(_WIN32)
   Status = FS->status("../bar/baz/a");
   ASSERT_FALSE(Status.getError());
   EXPECT_TRUE(Status->exists());
-#endif
 }
 
 TEST_F(VFSFromYAMLTest, WorkingDirectoryFallthroughInvalid) {
   IntrusiveRefCntPtr<ErrorDummyFileSystem> Lower(new ErrorDummyFileSystem());
   Lower->addDirectory("//root/");
   Lower->addDirectory("//root/foo");
   Lower->addRegularFile("//root/foo/a");
   Lower->addRegularFile("//root/foo/b");