[Clang] Restore replace_path_prefix instead of startswith

[saudi]

Summary

In D49466, sys::path::replace_path_prefix was used instead startswith.
However those were reverted later (commit rG3bb24bf25767ef5bbcef958b484e7a06d8689204) due to broken Windows tests.

This patch is to restore those replace_path_prefix calls. The objective, in a follow-up patch, is to allow the prefix matching to be case-insensitive under Windows.

This patch reverts commit rG3bb24bf25767ef5bbcef958b484e7a06d8689204 except for the test fix it contains.

[MaskRay]

Well C:\hello match both C:/hello and c:\hello but not C:\helloa? This behavior will look reasonable to me, though I should note that GCC does something similar to startswith.

Maybe we should tell them (https://gcc.gnu.org/pipermail/gcc/2020-March/) that we will match path components.

[saudi]

In D76869#1944420, @MaskRay wrote:

Well C:\hello match both C:/hello and c:\hello but not C:\helloa? This behavior will look reasonable to me, though I should note that GCC does something similar to startswith.

Maybe we should tell them (https://gcc.gnu.org/pipermail/gcc/2020-March/) that we will match path components.

The examples you gave (C:\hello matching both C:/hello and c:\hello) is what I would like to do as a next step, in another patch, and would be only when compiling on Windows.

But indeed, this patch changes the behavior and matches entirely the last component for macro/debug prefix replacement : -fdebug-prefix-map=C:\hello=xxx would not replace C:\helloa anymore
This is because replace_path_prefix 's strict param is set to true in those cases.

Note that this was originally done in D49466 and got reverted because of the breaking Windows tests.
I restored it, but this change of behavior is not really required for the next step I was mentionning.

[saudi]

Update patch, where I removed the change of behavior (not using the strict mode of replace_path_prefix).
We can change that behavior separately in a later commit.

I also fixed and added a unit test for replace_path_prefix, for a failing scenario discovered running the regression tests.

[saudi]

Added checks for empty map to avoid unneeded string copies.

[MaskRay]

LGTM.

clang/test/Preprocessor/file_test.c

10–12

Thanks for making win/non-win separated. This is clearer.

[saudi]

Update patch after the simplifications done in D77223.

I'm modifying this patch to also include step 3) as described in D77223 : make the Windows version of replace_path_prefix case and separator insensitive.

Without the separator-insensitive part, the regression test CHECK-REMOVE in file_test_windows.c would fail.
This is because the generated code contains both cases of separators after the prefix (\ the .c file, and / for the .h file).

[saudi]

Ping!

I had to do some modifications after the patch was accepted, since a test was broken. See the comment with my last update.

[saudi]

Ping - @MaskRay would you have time to take a quick look, please?

[MaskRay]

clang/test/Preprocessor/file_test_windows.c

13

Can {{/|\\\\}} be simplified?

18

ditto

[saudi]

clang/test/Preprocessor/file_test_windows.c

13

Yes, however it would be /, which looks odd as compared to the line above that uses \ for the file_test_windows.c file.
This is because of how the include file full paths are resolved, where / are introduced.

Should I try to fix that behavior?
Or maybe I could leave this test with / instead of {{/|\\\\}}, leaving the coherence fix for a separate patch. What do you think?

[MaskRay]

clang/test/Preprocessor/file_test_windows.c

13

The coherence fix can be made in a separate patch. When that get fixes, the updated test will clearly show which has been change.

[saudi]

rebase, and apply suggestions (removed regex matching in test file_test_windows.c test).

[MaskRay]

llvm/lib/Support/Path.cpp

510

I am sure toLower works when Unicode is considered.

@amccarth has made many useful comments in this area before, so I added him as a blocking reviewer.

510

s/am sure/am not sure/

[amccarth]

Background:

The llvm::sys::Path functions are designed to deal with paths that are either Posix-style or (a subset of) Windows styles. We also have the virtual file system (VFS), specifically the redirecting file system, which can truly be hybrids, mixing Posix with Windows styles in a single path.

Attempts to canonicalize VFS paths to one or the other wasn't feasible. It would require many tests to be duplicated for Posix or Windows. Since one of the benefits of VFS is to write single tests that work on both file systems, this didn't seem the right way to go. Furthermore, the general clang approach to file paths is not to canonicalize but to blindly concatenate.

I got nearly all of the VFS tests, and several others, working on Windows mostly by intentionally avoiding the llvm::sys::Path APIs when the path in question may be one of these hybrid styles.

I'll take a closer look at the details in this patch later today.

[amccarth]

Thanks for including me in this discussion.

I think this can work and not interfere with hybrid VFS paths, but please make sure to test on the all VFS tests both in both LLVM and Clang.

The case blindness is incomplete as noted in the inline comments, but maybe it's "good enough." I don't know how strictly other parts of LLVM handle this. Doing better will be hard because you'd have to compare code point by code point rather than UTF-8 code unit by code unit. Also note that Windows allows for case-sensitive folders as an experimental feature. (NTFS actually is case sensitive, but the OS hides that detail.)

As noted inline, please add a path style parameter to replace_path_prefix and pass it along to starts_with.

llvm/lib/Support/Path.cpp

510

If I recall correctly, the case insensitivity in Windows paths is not locale-aware. In fact, NTFS has a hard-coded case table. So that's not an issue. Also not an issue is Unicode normalization. I'm pretty sure you can name one file "Ä" and another "Ä" and they will remain distinct because one is a pre-composited code point and the other is a regular "A" with a combining mark. So that's another thing you don't have to worry about.

But it looks like toLower only handles basic ASCII, so, yeah, Unicode path names won't benefit from this attempt at case-blind comparison. I'm not sure what facilities LLVM has for case folding for the rest of the BMP. Maybe this is good enough for now? I don't have any data on how common it is for people to expect proper case-blind comparisons of non-ASCII characters in Windows paths.

524

I wonder if replace_path_prefix should take a path style (defaulted to native) and pass it along to starts_with. That would give future users of replace_path_prefix the power to control whether they get the Windows treatment.

[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?

[amccarth]

I think this is pretty close now.

llvm/lib/Support/Path.cpp

510

I don't have an opinion on how to handle the Unicode/case issues. I don't know how important "perfect" case handling is or whether it should block "good enough" given that we don't have it know and this will help many. So I'm not going to block on that unless somebody with more info or stronger opinions chimes in.

[I just read that Windows 10 is now letting people experiment with case-sensitive portions of a file system. So who knows where this is going in the future!]

519

I like that this has the style parameter now. Thanks!

But naming it PrefixStyle seems odd. It suggests that it applies to all of the OldPrefix and NewPrefix but not the Path. Can you call it simply style to be consistent with most everything else in this path support file?

llvm/unittests/Support/Path.cpp

1342

No #ifdef in the test now. Yay!

[amccarth]

In D76869#2021088, @saudi wrote:

@amccarth : Does it cover all the VFS tests you suggested that I double-check?

Yes. Thanks.

[dankm]

I like the idea of this, and fits what I had in mind for the original D49466 review. I want to take a closer look, but right now LGTM.

[saudi]

Renamed the argument PrefixStyle to style.

Not that it matches the rest of other arguments declared in Path.h, however it doesn't follow the LLVM code guidelines.

replace_path_prefix is currently the only function there with CamelCase arguments OldPrefix and NewPrefix.
Should-I rename those to oldPrefix and newPrefix to be more homogenous with the rest of the file?

I suppose the other choice would be to rename the style parameter, e.g. Style St

[saudi]

(missed rename PrefixStyle to style in comment)

[amccarth]

LGTM. Thanks.

I'm less worried about the details of style (like camel casing), since I have the impression things are changing. In general, stick to prevalent style of the file when it's discernable. My issue with PrefixStyle was that the name could be confusing.

[saudi]

In D76869#2022784, @amccarth wrote:

LGTM. Thanks.

I'm less worried about the details of style (like camel casing), since I have the impression things are changing. In general, stick to prevalent style of the file when it's discernable. My issue with PrefixStyle was that the name could be confusing.

Thanks!
@MaskRay, are you ok with this patch? I think it needs your approval, as it doesn't show as "Ready to land".

[saudi]

Ping! @MaskRay you are a blocking reviewer, would you mind taking a final look please?

[MaskRay]

Ah sorry for the delay. I just trust Adrian's resolution:)

[MaskRay]

clang/lib/Lex/PPMacroExpansion.cpp

1545

Does this need to be swapped?

[saudi]

clang/lib/Lex/PPMacroExpansion.cpp

1545

Lexer::Stringify doubles backslashes in Windows-style paths.

So, without this patch, using the -fmacro-prefix-map=[X]:[Y] requires doubling backslashes for [X], whereas -fdebug-prefix-map= requires single backslashes.

Also, in the tests : %clang -E -fmacro-prefix-map=%p=A:\UNLIKELY_PATH=empty would never match %p prefix on Windows platform, as %p contains single blackslashes.

[saudi]

Commited - 7a8edcb2124b60941ef6ea4bb4b38a9eb0d70137

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 style 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 style = 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 style = Style::native) {
+  // Windows prefix matching : case and separator insensitive
+  if (real_style(style) == 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], style);
+      bool SepPrefix = is_separator(Prefix[I], style);
+      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 style) {
   if (OldPrefix.empty() && NewPrefix.empty())
     return false;
 
   StringRef OrigPath(Path.begin(), Path.size());
-  if (!OrigPath.startswith(OldPrefix))
+  if (!starts_with(OrigPath, OldPrefix, style))
     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));