Fix a buglet in remove_dots().

[ppluzhnikov]

Summary

The function promises to canonicalize the path, but neglected to do so
for the root component.

For example, calling remove_dots("/tmp/foo.c", Style::windows_backslash)
resulted in "/tmp\foo.c". Now it produces "\tmp\foo.c".

Also fix FIXME in the corresponding test.

[ppluzhnikov]

Fix failed tests.

[ppluzhnikov]

Rebase to ToT.

[ppluzhnikov]

Fix InMemoryFileSystemTest bug.

[ppluzhnikov]

Fix ProxyFileSystemTest.Basic on Win/x64

[ppluzhnikov]

One more ProxyFileSystemTest fixe.

[ppluzhnikov]

Fix compile failure.

[ppluzhnikov]

Fix formatting.

[rnk]

lgtm, sorry for the delay, do you need someone to push this?

[ppluzhnikov]

In D126412#3553796, @rnk wrote:

do you need someone to push this?

Yes, please.
I do not have commit rights.

clang/unittests/Basic/FileManagerTest.cpp

TEST_F(FileManagerTest, getVirtualFileWithDifferentName) {
   // Check that the contents of the UFE are not overwritten by the entry in the
   // filesystem:
   EXPECT_EQ(123, (*file2)->getSize());
 }
 
 #endif  // !_WIN32
 
 static StringRef getSystemRoot() {
-  return is_style_windows(llvm::sys::path::Style::native) ? "C:/" : "/";
+  return is_style_windows(llvm::sys::path::Style::native) ? "C:\\" : "/";
 }
 
 TEST_F(FileManagerTest, makeAbsoluteUsesVFS) {
   // FIXME: Should this be using a root path / call getSystemRoot()? For now,
   // avoiding that and leaving the test as-is.
   SmallString<64> CustomWorkingDir =
       is_style_windows(llvm::sys::path::Style::native) ? StringRef("C:")
                                                        : StringRef("/");

clang/unittests/Frontend/PCHPreambleTest.cpp

 #include "llvm/Support/Path.h"
 #include "gtest/gtest.h"
 
 using namespace llvm;
 using namespace clang;
 
 namespace {
 
+std::string Canonicalize(const Twine &Path) {
+  SmallVector<char, 128> PathVec;
+  Path.toVector(PathVec);
+  llvm::sys::path::remove_dots(PathVec, true);
+  return std::string(PathVec.begin(), PathVec.end());
+}
+
 class ReadCountingInMemoryFileSystem : public vfs::InMemoryFileSystem
 {
   std::map<std::string, unsigned> ReadCounts;
 
 public:
   ErrorOr<std::unique_ptr<vfs::File>> openFileForRead(const Twine &Path) override
   {
-    SmallVector<char, 128> PathVec;
-    Path.toVector(PathVec);
-    llvm::sys::path::remove_dots(PathVec, true);
-    ++ReadCounts[std::string(PathVec.begin(), PathVec.end())];
+    ++ReadCounts[Canonicalize(Path)];
     return InMemoryFileSystem::openFileForRead(Path);
   }
 
   unsigned GetReadCount(const Twine &Path) const
   {
-    auto it = ReadCounts.find(Path.str());
+    auto it = ReadCounts.find(Canonicalize(Path));
     return it == ReadCounts.end() ? 0 : it->second;
   }
 };
 
 class PCHPreambleTest : public ::testing::Test {
   IntrusiveRefCntPtr<ReadCountingInMemoryFileSystem> VFS;
   StringMap<std::string> RemappedFiles;
   std::shared_ptr<PCHContainerOperations> PCHContainerOpts;

llvm/lib/Support/Path.cpp

if (component.empty() || component == ".") {
       } else if (!absolute) {
         components.push_back(component);
       }
     } else {
       components.push_back(component);
     }
   }
 
+  SmallString<256> buffer = root;
+  // "root" could be "/", which may need to be translated into "\".
+  make_preferred(buffer, style);
+  needs_change |= root != buffer;
+
   // Avoid rewriting the path unless we have to.
   if (!needs_change)
     return false;
 
-  SmallString<256> buffer = root;
   if (!components.empty()) {
     buffer += components[0];
     for (StringRef C : makeArrayRef(components).drop_front()) {
       buffer += preferred_separator(style);
       buffer += C;
     }
   }
   the_path.swap(buffer);

llvm/unittests/Support/Path.cpp

TEST(Support, RemoveDots) {
   EXPECT_EQ("c", remove_dots(".\\.\\c", true, path::Style::windows));
   EXPECT_EQ("..\\a\\c",
             remove_dots("..\\a\\b\\..\\c", true, path::Style::windows));
   EXPECT_EQ("..\\..\\a\\c",
             remove_dots("..\\..\\a\\b\\..\\c", true, path::Style::windows));
   EXPECT_EQ("C:\\a\\c", remove_dots("C:\\foo\\bar//..\\..\\a\\c", true,
                                     path::Style::windows));
 
-  // FIXME: These leading forward slashes are emergent behavior. VFS depends on
-  // this behavior now.
-  EXPECT_EQ("C:/bar",
+  EXPECT_EQ("C:\\bar",
             remove_dots("C:/foo/../bar", true, path::Style::windows));
-  EXPECT_EQ("C:/foo\\bar",
+  EXPECT_EQ("C:\\foo\\bar",
             remove_dots("C:/foo/bar", true, path::Style::windows));
-  EXPECT_EQ("C:/foo\\bar",
+  EXPECT_EQ("C:\\foo\\bar",
             remove_dots("C:/foo\\bar", true, path::Style::windows));
-  EXPECT_EQ("/", remove_dots("/", true, path::Style::windows));
-  EXPECT_EQ("C:/", remove_dots("C:/", true, path::Style::windows));
+  EXPECT_EQ("\\", remove_dots("/", true, path::Style::windows));
+  EXPECT_EQ("C:\\", remove_dots("C:/", true, path::Style::windows));
 
   // Some clients of remove_dots expect it to remove trailing slashes. Again,
   // this is emergent behavior that VFS relies on, and not inherently part of
   // the specification.
   EXPECT_EQ("C:\\foo\\bar",
             remove_dots("C:\\foo\\bar\\", true, path::Style::windows));
   EXPECT_EQ("/foo/bar",
             remove_dots("/foo/bar/", true, path::Style::posix));
 
   // A double separator is rewritten.
-  EXPECT_EQ("C:/foo\\bar", remove_dots("C:/foo//bar", true, path::Style::windows));
+  EXPECT_EQ("C:\\foo\\bar",
+            remove_dots("C:/foo//bar", true, path::Style::windows));
 
   SmallString<64> Path1(".\\.\\c");
   EXPECT_TRUE(path::remove_dots(Path1, true, path::Style::windows));
   EXPECT_EQ("c", Path1);
 
   EXPECT_EQ("foolz/wat",
             remove_dots("././/foolz/wat", false, path::Style::posix));
   EXPECT_EQ("", remove_dots("./////", false, path::Style::posix));

llvm/unittests/Support/VirtualFileSystemTest.cpp

TEST(ProxyFileSystemTest, Basic) {
   I.increment(EC);
   ASSERT_FALSE(EC);
   ASSERT_EQ(vfs::directory_iterator(), I);
 
   ASSERT_FALSE(PFS.setCurrentWorkingDirectory("/"));
 
   auto PWD = PFS.getCurrentWorkingDirectory();
   ASSERT_FALSE(PWD.getError());
-  ASSERT_EQ("/", *PWD);
+  ASSERT_EQ("/", getPosixPath(*PWD));
 
   SmallString<16> Path;
   ASSERT_FALSE(PFS.getRealPath("a", Path));
-  ASSERT_EQ("/a", Path);
+  ASSERT_EQ("/a", getPosixPath(Path));
 
   bool Local = true;
   ASSERT_FALSE(PFS.isLocal("/a", Local));
   EXPECT_FALSE(Local);
 }
 
 class InMemoryFileSystemTest : public ::testing::Test {
 protected:
TEST_F(InMemoryFileSystemTest, UniqueID) {
   EXPECT_EQ(FS.status("/a/b")->getUniqueID(), FS.status("/a/b")->getUniqueID());
   EXPECT_NE(FS.status("/a/b")->getUniqueID(), FS.status("/c/d")->getUniqueID());
   EXPECT_EQ(FS.status("/a/b")->getUniqueID(), FS.status("/e/f")->getUniqueID());
   EXPECT_EQ(FS.status("/a")->getUniqueID(), FS.status("/a")->getUniqueID());
   EXPECT_NE(FS.status("/a")->getUniqueID(), FS.status("/c")->getUniqueID());
   EXPECT_NE(FS.status("/a")->getUniqueID(), FS.status("/e")->getUniqueID());
 
   // Recreating the "same" FS yields the same UniqueIDs.
-  vfs::InMemoryFileSystem FS2;
+  // Note: FS2 should match FS with respect to path normalization.
+  vfs::InMemoryFileSystem FS2(/*UseNormalizedPath=*/false);
   ASSERT_TRUE(FS2.addFile("/a/b", 0, MemoryBuffer::getMemBuffer("text")));
   EXPECT_EQ(FS.status("/a/b")->getUniqueID(),
             FS2.status("/a/b")->getUniqueID());
   EXPECT_EQ(FS.status("/a")->getUniqueID(), FS2.status("/a")->getUniqueID());
 }
 
 // NOTE: in the tests below, we use '//root/' as our root directory, since it is
 // a legal *absolute* path on Windows as well as *nix.