[LLVM] sys::path::replace_path_prefix fix and simplifications

[saudi]

Summary

When working on D76869, I discovered some failing scenarios for sys::path::replace_path_prefix.
However, this function is already a bit complex and the fix would make the logic harder to read.
Also, its parameters strict and style are always used with the default value.

This patch removes the 2 parameters, reducing the code greatly and made it easier to fix the failing cases.

Overall, I was planning to split my work in 3 steps:

1. This fix
2. finish D76869 (which this fix will make smaller/simpler)
3. Modifiy replace_prefix_map under windows, to replace startswith with a case and separator insensitive version (for example, C:\\OneFolder\\ would match c:/onefolder/file.cpp)

[MaskRay]

Thanks!

llvm/include/llvm/Support/Path.h

173–175

So, this change restores the original 3-parameter form of replace_path_prefix. LG

[saudi]

Added Style parameter to replace_path_prefix, as suggested.
This allowed to make the replace_path_prefix unit-test platform independent, removing a #ifdef _WIN32 that I added in a previous version of the patch.

The tests ninja check-all passed on windows.
@amccarth : Does it cover all the VFS tests you suggested that I double-check?

EDIT: Nevermind this update and comment, it was intended to another patch

clang/lib/CodeGen/CGDebugInfo.cpp

if (llvm::sys::path::is_absolute(RemappedFile)) {
     File = RemappedFile;
   }
   llvm::DIFile *F = DBuilder.createFile(File, Dir, CSInfo, Source);
   DIFileCache[FileName.data()].reset(F);
   return F;
 }
 
 std::string CGDebugInfo::remapDIPath(StringRef Path) const {
-  for (const auto &Entry : DebugPrefixMap)
-    if (Path.startswith(Entry.first))
-      return (Twine(Entry.second) + Path.substr(Entry.first.size())).str();
+  if (DebugPrefixMap.empty())
     return Path.str();
+
+  SmallString<256> P = Path;
+  for (const auto &Entry : DebugPrefixMap)
+    if (llvm::sys::path::replace_path_prefix(P, Entry.first, Entry.second))
+      break;
+  return P.str().str();
 }
 
 unsigned CGDebugInfo::getLineNumber(SourceLocation Loc) {
   if (Loc.isInvalid() && CurLoc.isInvalid())
     return 0;
   SourceManager &SM = CGM.getContext().getSourceManager();
   PresumedLoc PLoc = SM.getPresumedLoc(Loc.isValid() ? Loc : CurLoc);
   return PLoc.isValid() ? PLoc.getLine() : 0;

clang/lib/Lex/PPMacroExpansion.cpp

static bool isTargetEnvironment(const TargetInfo &TI,
   return TI.getTriple().getEnvironment() == Env.getEnvironment();
 }
 
 static void remapMacroPath(
     SmallString<256> &Path,
     const std::map<std::string, std::string, std::greater<std::string>>
         &MacroPrefixMap) {
   for (const auto &Entry : MacroPrefixMap)
-    if (Path.startswith(Entry.first)) {
-      Path = (Twine(Entry.second) + Path.substr(Entry.first.size())).str();
+    if (llvm::sys::path::replace_path_prefix(Path, Entry.first, Entry.second))
       break;
-    }
-}
 
 /// ExpandBuiltinMacro - If an identifier token is read that is to be expanded
 /// as a builtin macro, handle it and return the next token as 'Tok'.
 void Preprocessor::ExpandBuiltinMacro(Token &Tok) {
   // Figure out which token this is.
   IdentifierInfo *II = Tok.getIdentifierInfo();
   assert(II && "Can't be a macro without id info!");
 
if (PLoc.isValid()) {
         StringRef PLFileName = llvm::sys::path::filename(PLoc.getFilename());
         if (PLFileName != "")
           FN += PLFileName;
         else
           FN += PLoc.getFilename();
       } else {
         FN += PLoc.getFilename();
       }
-      Lexer::Stringify(FN);
       remapMacroPath(FN, PPOpts->MacroPrefixMap);
+      Lexer::Stringify(FN);
       OS << '"' << FN << '"';
     }
     Tok.setKind(tok::string_literal);
   } else if (II == Ident__DATE__) {
     Diag(Tok.getLocation(), diag::warn_pp_date_time);
     if (!DATELoc.isValid())
       ComputeDATE_TIME(DATELoc, TIMELoc, *this);
     Tok.setKind(tok::string_literal);

clang/test/Preprocessor/file_test.c

-// XFAIL: system-windows
+// UNSUPPORTED: system-windows
 // RUN: %clang -E -ffile-prefix-map=%p=/UNLIKELY_PATH/empty -c -o - %s | FileCheck %s
 // RUN: %clang -E -fmacro-prefix-map=%p=/UNLIKELY_PATH/empty -c -o - %s | FileCheck %s
 // RUN: %clang -E -fmacro-prefix-map=%p=/UNLIKELY_PATH=empty -c -o - %s | FileCheck %s -check-prefix CHECK-EVIL
 // RUN: %clang -E -fmacro-prefix-map=%p/= -c -o - %s | FileCheck %s --check-prefix CHECK-REMOVE
 
 filename: __FILE__
-#include "file_test.h"
+#include "Inputs/include-file-test/file_test.h"
 
-// CHECK: filename: "/UNLIKELY_PATH/empty{{/|\\\\}}file_test.c"
-// CHECK: filename: "/UNLIKELY_PATH/empty{{/|\\\\}}file_test.h"
-// CHECK: basefile: "/UNLIKELY_PATH/empty{{/|\\\\}}file_test.c"
+// CHECK: filename: "/UNLIKELY_PATH/empty/file_test.c"
+// CHECK: filename: "/UNLIKELY_PATH/empty/Inputs/include-file-test/file_test.h"
+// CHECK: basefile: "/UNLIKELY_PATH/empty/file_test.c"
 // CHECK-NOT: filename:
 
-// CHECK-EVIL: filename: "/UNLIKELY_PATH=empty{{/|\\\\}}file_test.c"
-// CHECK-EVIL: filename: "/UNLIKELY_PATH=empty{{/|\\\\}}file_test.h"
-// CHECK-EVIL: basefile: "/UNLIKELY_PATH=empty{{/|\\\\}}file_test.c"
+// CHECK-EVIL: filename: "/UNLIKELY_PATH=empty/file_test.c"
+// CHECK-EVIL: filename: "/UNLIKELY_PATH=empty/Inputs/include-file-test/file_test.h"
+// CHECK-EVIL: basefile: "/UNLIKELY_PATH=empty/file_test.c"
 // CHECK-EVIL-NOT: filename:
 
 // CHECK-REMOVE: filename: "file_test.c"
-// CHECK-REMOVE: filename: "file_test.h"
+// CHECK-REMOVE: filename: "Inputs/include-file-test/file_test.h"
 // CHECK-REMOVE: basefile: "file_test.c"
 // CHECK-REMOVE-NOT: filename:

clang/test/Preprocessor/file_test_windows.c

+// REQUIRES: system-windows
+// RUN: %clang -E -ffile-prefix-map=%p=A:\UNLIKELY_PATH\empty -c -o - %s | FileCheck %s
+// RUN: %clang -E -fmacro-prefix-map=%p=A:\UNLIKELY_PATH\empty -c -o - %s | FileCheck %s
+// RUN: %clang -E -fmacro-prefix-map=%p=A:\UNLIKELY_PATH=empty -c -o - %s | FileCheck %s -check-prefix CHECK-EVIL
+// RUN: %clang -E -fmacro-prefix-map=%p/iNPUTS\=A:\UNLIKELY_PATH_INC\ -fmacro-prefix-map=%p/=A:\UNLIKELY_PATH_BASE\ -c -o - %s | FileCheck %s -check-prefix CHECK-CASE
+// RUN: %clang -E -fmacro-prefix-map=%p\= -c -o - %s | FileCheck %s --check-prefix CHECK-REMOVE
+
+filename: __FILE__
+#include "Inputs/include-file-test/file_test.h"
+
+// CHECK: filename: "A:\\UNLIKELY_PATH\\empty\\file_test_windows.c"
+// CHECK: filename: "A:\\UNLIKELY_PATH\\empty/Inputs/include-file-test/file_test.h"
+// CHECK: basefile: "A:\\UNLIKELY_PATH\\empty\\file_test_windows.c"
+// CHECK-NOT: filename:
+
+// CHECK-EVIL: filename: "A:\\UNLIKELY_PATH=empty\\file_test_windows.c"
+// CHECK-EVIL: filename: "A:\\UNLIKELY_PATH=empty/Inputs/include-file-test/file_test.h"
+// CHECK-EVIL: basefile: "A:\\UNLIKELY_PATH=empty\\file_test_windows.c"
+// CHECK-EVIL-NOT: filename:
+
+// CHECK-CASE: filename: "A:\\UNLIKELY_PATH_BASE\\file_test_windows.c"
+// CHECK-CASE: filename: "A:\\UNLIKELY_PATH_INC\\include-file-test/file_test.h"
+// CHECK-CASE: basefile: "A:\\UNLIKELY_PATH_BASE\\file_test_windows.c"
+// CHECK-CASE-NOT: filename:
+
+// CHECK-REMOVE: filename: "file_test_windows.c"
+// CHECK-REMOVE: filename: "Inputs/include-file-test/file_test.h"
+// CHECK-REMOVE: basefile: "file_test_windows.c"
+// CHECK-REMOVE-NOT: filename:

llvm/include/llvm/Support/Path.h

 ///   /foo, <empty>, new => new/foo
 ///   /old/foo, /old, <empty> => /foo
 /// @endcode
 ///
 /// @param Path If \a Path starts with \a OldPrefix modify to instead
 ///        start with \a NewPrefix.
 /// @param OldPrefix The path prefix to strip from \a Path.
 /// @param NewPrefix The path prefix to replace \a NewPrefix with.
+/// @param PrefixStyle The style used to match the prefix. Exact match using
+/// Posix style, case/separator insensitive match for Windows style.
 /// @result true if \a Path begins with OldPrefix
 bool replace_path_prefix(SmallVectorImpl<char> &Path, StringRef OldPrefix,
-                         StringRef NewPrefix);
+                         StringRef NewPrefix,
+                         Style PrefixStyle = Style::native);
 
 /// Append to path.
 ///
 /// @code
 ///   /foo  + bar/f => /foo/bar/f
 ///   /foo/ + bar/f => /foo/bar/f
 ///   foo   + bar/f => foo/bar/f
 /// @endcode

llvm/lib/DWARFLinker/DWARFLinker.cpp

auto DwoId = dwarf::toUnsigned(
       CUDie.find({dwarf::DW_AT_dwo_id, dwarf::DW_AT_GNU_dwo_id}));
   if (DwoId)
     return *DwoId;
   return 0;
 }
 
 static std::string remapPath(StringRef Path,
                              const objectPrefixMap &ObjectPrefixMap) {
-  for (const auto &Entry : ObjectPrefixMap)
-    if (Path.startswith(Entry.first))
-      return (Twine(Entry.second) + Path.substr(Entry.first.size())).str();
+  if (ObjectPrefixMap.empty())
     return Path.str();
+
+  SmallString<256> p = Path;
+  for (const auto &Entry : ObjectPrefixMap)
+    if (llvm::sys::path::replace_path_prefix(p, Entry.first, Entry.second))
+      break;
+  return p.str().str();
 }
 
 bool DWARFLinker::registerModuleReference(
     DWARFDie CUDie, const DWARFUnit &Unit, const DwarfFile &File,
     OffsetsStringPool &StringPool, UniquingStringPool &UniquingStringPool,
     DeclContextTree &ODRContexts, uint64_t ModulesEndOffset, unsigned &UnitID,
     bool IsLittleEndian, unsigned Indent, bool Quiet) {
   std::string PCMfile = dwarf::toString(

llvm/lib/MC/MCContext.cpp

 
 void MCContext::addDebugPrefixMapEntry(const std::string &From,
                                        const std::string &To) {
   DebugPrefixMap.insert(std::make_pair(From, To));
 }
 
 void MCContext::RemapDebugPaths() {
   const auto &DebugPrefixMap = this->DebugPrefixMap;
+  if (DebugPrefixMap.empty())
+    return;
+
   const auto RemapDebugPath = [&DebugPrefixMap](std::string &Path) {
-    for (const auto &Entry : DebugPrefixMap)
-      if (StringRef(Path).startswith(Entry.first)) {
-        std::string RemappedPath =
-            (Twine(Entry.second) + Path.substr(Entry.first.size())).str();
-        Path.swap(RemappedPath);
-      }
+    SmallString<256> P(Path);
+    for (const auto &Entry : DebugPrefixMap) {
+      if (llvm::sys::path::replace_path_prefix(P, Entry.first, Entry.second)) {
+        Path = P.str().str();
+        break;
+      }
   };
 
   // Remap compilation directory.
   std::string CompDir = std::string(CompilationDir.str());
   RemapDebugPath(CompDir);
   CompilationDir = CompDir;
 
   // Remap MCDwarfDirs in all compilation units.

llvm/lib/Support/Path.cpp

void replace_extension(SmallVectorImpl<char> &path, const Twine &extension,
   // Append '.' if needed.
   if (ext.size() > 0 && ext[0] != '.')
     path.push_back('.');
 
   // Append extension.
   path.append(ext.begin(), ext.end());
 }
 
+static bool starts_with(StringRef Path, StringRef Prefix,
+                        Style PrefixStyle = Style::native) {
+  // Windows prefix matching : case and separator insensitive
+  if (real_style(PrefixStyle) == Style::windows) {
+    if (Path.size() < Prefix.size())
+      return false;
+    for (size_t I = 0, E = Prefix.size(); I != E; ++I) {
+      bool SepPath = is_separator(Path[I], PrefixStyle);
+      bool SepPrefix = is_separator(Prefix[I], PrefixStyle);
+      if (SepPath != SepPrefix)
+        return false;
+      if (!SepPath && toLower(Path[I]) != toLower(Prefix[I]))
+        return false;
+    }
+    return true;
+  }
+  return Path.startswith(Prefix);
+}
+
 bool replace_path_prefix(SmallVectorImpl<char> &Path, StringRef OldPrefix,
-                         StringRef NewPrefix) {
+                         StringRef NewPrefix, Style PrefixStyle) {
   if (OldPrefix.empty() && NewPrefix.empty())
     return false;
 
   StringRef OrigPath(Path.begin(), Path.size());
-  if (!OrigPath.startswith(OldPrefix))
+  if (!starts_with(OrigPath, OldPrefix, PrefixStyle))
     return false;
 
   // If prefixes have the same size we can simply copy the new one over.
   if (OldPrefix.size() == NewPrefix.size()) {
     llvm::copy(NewPrefix, Path.begin());
     return true;
   }
 

llvm/unittests/Support/Path.cpp

TEST(Support, RemoveDots) {
   EXPECT_TRUE(path::remove_dots(Path2, true, path::Style::posix));
   EXPECT_EQ("c", Path2);
 }
 
 TEST(Support, ReplacePathPrefix) {
   SmallString<64> Path1("/foo");
   SmallString<64> Path2("/old/foo");
   SmallString<64> Path3("/oldnew/foo");
+  SmallString<64> Path4("C:\\old/foo\\bar");
   SmallString<64> OldPrefix("/old");
   SmallString<64> OldPrefixSep("/old/");
+  SmallString<64> OldPrefixWin("c:/oLD/F");
   SmallString<64> NewPrefix("/new");
   SmallString<64> NewPrefix2("/longernew");
   SmallString<64> EmptyPrefix("");
+  bool Found;
 
   SmallString<64> Path = Path1;
-  path::replace_path_prefix(Path, OldPrefix, NewPrefix);
+  Found = path::replace_path_prefix(Path, OldPrefix, NewPrefix);
+  EXPECT_FALSE(Found);
   EXPECT_EQ(Path, "/foo");
   Path = Path2;
-  path::replace_path_prefix(Path, OldPrefix, NewPrefix);
+  Found = path::replace_path_prefix(Path, OldPrefix, NewPrefix);
+  EXPECT_TRUE(Found);
   EXPECT_EQ(Path, "/new/foo");
   Path = Path2;
-  path::replace_path_prefix(Path, OldPrefix, NewPrefix2);
+  Found = path::replace_path_prefix(Path, OldPrefix, NewPrefix2);
+  EXPECT_TRUE(Found);
   EXPECT_EQ(Path, "/longernew/foo");
   Path = Path1;
-  path::replace_path_prefix(Path, EmptyPrefix, NewPrefix);
+  Found = path::replace_path_prefix(Path, EmptyPrefix, NewPrefix);
+  EXPECT_TRUE(Found);
   EXPECT_EQ(Path, "/new/foo");
   Path = Path2;
-  path::replace_path_prefix(Path, OldPrefix, EmptyPrefix);
+  Found = path::replace_path_prefix(Path, OldPrefix, EmptyPrefix);
+  EXPECT_TRUE(Found);
   EXPECT_EQ(Path, "/foo");
   Path = Path2;
-  path::replace_path_prefix(Path, OldPrefixSep, EmptyPrefix);
+  Found = path::replace_path_prefix(Path, OldPrefixSep, EmptyPrefix);
+  EXPECT_TRUE(Found);
   EXPECT_EQ(Path, "foo");
   Path = Path3;
-  path::replace_path_prefix(Path, OldPrefix, NewPrefix);
+  Found = path::replace_path_prefix(Path, OldPrefix, NewPrefix);
+  EXPECT_TRUE(Found);
   EXPECT_EQ(Path, "/newnew/foo");
   Path = Path3;
-  path::replace_path_prefix(Path, OldPrefix, NewPrefix2);
+  Found = path::replace_path_prefix(Path, OldPrefix, NewPrefix2);
+  EXPECT_TRUE(Found);
   EXPECT_EQ(Path, "/longernewnew/foo");
   Path = Path1;
-  path::replace_path_prefix(Path, EmptyPrefix, NewPrefix);
+  Found = path::replace_path_prefix(Path, EmptyPrefix, NewPrefix);
+  EXPECT_TRUE(Found);
   EXPECT_EQ(Path, "/new/foo");
   Path = OldPrefix;
-  path::replace_path_prefix(Path, OldPrefix, NewPrefix);
+  Found = path::replace_path_prefix(Path, OldPrefix, NewPrefix);
+  EXPECT_TRUE(Found);
   EXPECT_EQ(Path, "/new");
   Path = OldPrefixSep;
-  path::replace_path_prefix(Path, OldPrefix, NewPrefix);
+  Found = path::replace_path_prefix(Path, OldPrefix, NewPrefix);
+  EXPECT_TRUE(Found);
   EXPECT_EQ(Path, "/new/");
   Path = OldPrefix;
-  path::replace_path_prefix(Path, OldPrefixSep, NewPrefix);
+  Found = path::replace_path_prefix(Path, OldPrefixSep, NewPrefix);
+  EXPECT_FALSE(Found);
   EXPECT_EQ(Path, "/old");
+  Path = Path4;
+  Found = path::replace_path_prefix(Path, OldPrefixWin, NewPrefix,
+                                    path::Style::windows);
+  EXPECT_TRUE(Found);
+  EXPECT_EQ(Path, "/newoo\\bar");
+  Path = Path4;
+  Found = path::replace_path_prefix(Path, OldPrefixWin, NewPrefix,
+                                    path::Style::posix);
+  EXPECT_FALSE(Found);
+  EXPECT_EQ(Path, "C:\\old/foo\\bar");
 }
 
 TEST_F(FileSystemTest, OpenFileForRead) {
   // Create a temp file.
   int FileDescriptor;
   SmallString<64> TempPath;
   ASSERT_NO_ERROR(
       fs::createTemporaryFile("prefix", "temp", FileDescriptor, TempPath));