Path: enhance prefix mapping

[dankm]

Summary

Return true when a path is remapped, and allow replacing the
original path entirely. This is needed for D49466.

[Lekensteyn]

This has some broken edge cases. Consider extending the ReplacePathPrefix test in unittests/Support/Path.cpp for the new cases.

lib/Support/Path.cpp

532

Wouldn't this crash if OldPrefix is empty? Both if blocks should probably be wrapped in if (!OldPrefix.empty()) {

533

Should return false for Path="/foobar" and OldPrefix="/foo/", but it does not.

560–566

If OldPrefix is empty, then this could potentially contain C:\foo I guess.

565

Trimming *all* slashes is probably beyond the scope for this function.

[dankm]

Rewrote and simplified the replace_path_prefix function, and added a bunch of tests to catch corner cases I noticed during testing.

[dankm]

lib/Support/Path.cpp

532

Yeah it should, but didn't for me when testing. Fixed in the new patch.

533

Yup, changed all the logic here, and now it does what I expect.

565

Agreed, it's in the only consumer where this matters now.

[Lekensteyn]

include/llvm/Support/Path.h

164

Perhaps these examples can be extended with special treatment of the trailing slash?

/old/foo, /old/, /new/ -> /new/foo
/old/foo, /old/, /new -> /new/foo         // looks more desirable than /newfoo I guess?
/old/foo, /old, /new/ -> /new/foo         // looks more desirable than /new//foo I guess?
/old/foo, /old/, <empty> -> foo           // better than /foo?

The last feature is most important to me for clean stack traces (/srcdir/subfdir/foo.c + -ffile-prefix-map=/srcdir/= -> subdir/foo.c, use dir /new/srcdir in gdb to enable proper debugging anyway).

lib/Support/Path.cpp

536

Coding style: remove curly braces from if/else.

Could you also rename Prefix to OldPrefixDir to make the meaning obvious? (I stopped further review here though since the specification of this function is not yet clear.)

unittests/Support/Path.cpp

1212

Maybe call this OldPrefixSep or OldPrefixSlash to signify the relation between the two prefixes?

1246

Another corner case to check:

Path = OldPrefix; // /old
path::replace_path_prefix(Path, OldPrefix, NewPrefix); // /old/ -> /new
EXPECT_EQ(Path, "/old"); // or should this be /new?

[dankm]

Attempt to document and test the desired interface.

The only minor change doesn't appear to affect in-tree consumers; and I'm not aware of out-of-tree consumers of this interface, so I think it's safe to change.

[Lekensteyn]

lib/Support/Path.cpp

543

Suggestion: guard all of the previous code in if (!OldPrefix.empty()) {.

Use OrigPath.size() > OldPrefixDir.size() && !is_separator(OrigPath[OldPrefixDir.size()], style) to avoid an out-of-bounds read. (operator[Index] requires Index < Length.)

If you build LLVM with assertions enabled, this should have tripped.

555

This does not handle all substitutions:

• /old, /new - handled
• /old/, /new - handled
• /old/, /new/ - not handled - should be accepted (old and new prefix are of the same length)
• /old, /new/ - not handled - should be accepted. OrigPath started with /old and followed by /, so it is safe to overwrite it. One exception: if OrigPath was /old, then the new buffer is too short, so it needs a check for OrigPath.size() >= NewPrefix.size()`.

Observe that the only the length is necessary, so you do not have to copy the string contents. Something like:

size_t NewPrefixDirLength = NewPrefix.size();

if (!NewPrefix.empty() && is_separator(NewPrefix.back(), style))
  --NewPrefixDirLength;
// ...
if (OldPrefixDir.size() == NewPrefixDirLength) {

563

RelPath[0] could be invalid if OldPath is the same as OldPrefix.

Is the relative_path call necessary? What happens if you call path::append(NewPath, "/foo")? Will it produce /foo or <NewPath>/foo?

If NewPrefix is empty, will this correctly avoid the leading directory separator?

unittests/Support/Path.cpp

1231–1258

Is it desired to always turn paths into absolute paths into relative when the prefix matches? This test currently checks:

• /old/foo, /old, "" => foo

The old behavior was:

• /old/foo, /old, "" => /foo

(consider: -ffile-prefix-map=/buildroot= for /buildroot/usr/src/foo.c)

If this is really what you want, then users now have to use:

• /old/foo, /old, /
• /old/foo, /old/, /

1237

This case is already covered by the previous test (which rejects a partial prefix:

• /oldnew/foo, /old, /new

What about removing that test and adding these instead:

• /old/foo, /old/, "" => "foo" (Path2, OldPrefixSep, EmptyPrefix)
• /old/foo, /old, "/" => "/foo" (Path2, OldPrefix, ...)

And perhaps to exercise replacements by a non-root directory:

• /old/foo, /old/, /longernew => "/longernew/foo" (Path2, OldPrefixSep, NewPrefix2)
• /old/foo, /old, /longernew => "/longernew/bar" (Path2, OldPrefix, NewPrefix2)
• /old/foo, /old, /longernew/ => "/longernew/bar" (Path2, OldPrefix, ...)

[dankm]

Nail down the contract a bit.
Add a flag for strict path matching.
I think there's still unresolved issues with tests.

[Lekensteyn]

Should this be abandoned for D49466?

[dankm]

In D56769#1624345, @Lekensteyn wrote:

Should this be abandoned for D49466?

Yes, yes it should.

include/llvm/Support/Path.h

164

Oh yes, /old/foo, /old/, <empty> -> foo is actually what it does now. That's new behaviour as of this patch, I'll update the examples. Good idea with the other examples too.

include/llvm/Support/Path.h

 ///                  prepended.
 void replace_extension(SmallVectorImpl<char> &path, const Twine &extension,
                        Style style = Style::native);
 
 /// Replace matching path prefix with another path.
 ///
 /// @code
 ///   /foo, /old, /new => /foo
+///   /old, /old, /new => /new
+///   /old, /old/, /new, false => /old
+///   /old, /old/, /new, true => /new
 ///   /old/foo, /old, /new => /new/foo
+///   /old/foo, /old/, /new => /new/foo
+///   /old/foo, /old/, /new/ => /new/foo
+///   /oldfoo, /old, /new => /oldfoo
 ///   /foo, <empty>, /new => /new/foo
-///   /old/foo, /old, <empty> => /foo
+///   /foo, <empty>, new => new/foo
+///   /old/foo, /old, <empty>, false => /foo
+///   /old/foo, /old, <empty>, true => 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 OldPrefix The path prefix to strip from \a Path. Any trailing
+///        path separator is ignored if strict is true.
 /// @param NewPrefix The path prefix to replace \a NewPrefix with.
-void replace_path_prefix(SmallVectorImpl<char> &Path,
+/// @param style The path separator style
+/// @param strict Strict prefix path checking
+/// @result true if \a Path begins with OldPrefix
+bool replace_path_prefix(SmallVectorImpl<char> &Path,
                          const StringRef &OldPrefix, const StringRef &NewPrefix,
-                         Style style = Style::native);
+                         Style style = Style::native, bool strict = false);
+static inline bool replace_path_prefix(SmallVectorImpl<char> &Path,
+                         const StringRef &OldPrefix, const StringRef &NewPrefix,
+                         bool strict, Style style = Style::native) {
+  return replace_path_prefix(Path, OldPrefix, NewPrefix, style, strict);
+}
 
 /// Append to path.
 ///
 /// @code
 ///   /foo  + bar/f => /foo/bar/f
 ///   /foo/ + bar/f => /foo/bar/f
 ///   foo   + bar/f => foo/bar/f
 /// @endcode

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());
 }
 
-void replace_path_prefix(SmallVectorImpl<char> &Path,
+bool replace_path_prefix(SmallVectorImpl<char> &Path,
                          const StringRef &OldPrefix, const StringRef &NewPrefix,
-                         Style style) {
+                         Style style, bool strict) {
   if (OldPrefix.empty() && NewPrefix.empty())
-    return;
+    return false;
 
   StringRef OrigPath(Path.begin(), Path.size());
-  if (!OrigPath.startswith(OldPrefix))
-    return;
+  StringRef OldPrefixDir;
+
+  if (!strict && OldPrefix.size() > OrigPath.size())
+    return false;
+
+  if (!strict && OldPrefix.size() > OrigPath.size())
+    return false;
+
+  // Ensure OldPrefixDir does not have a trailing separator.
+  if (!OldPrefix.empty() && is_separator(OldPrefix.back()))
+    OldPrefixDir = parent_path(OldPrefix, style);
+  else
+    OldPrefixDir = OldPrefix;
+
+  if (!OrigPath.startswith(OldPrefixDir))
+    return false;
+
+  if (OrigPath.size() > OldPrefixDir.size())
+    if (!is_separator(OrigPath[OldPrefixDir.size()], style) && strict)
+      return false;
 
   // If prefixes have the same size we can simply copy the new one over.
-  if (OldPrefix.size() == NewPrefix.size()) {
+  if (OldPrefixDir.size() == NewPrefix.size() && !strict) {
     llvm::copy(NewPrefix, Path.begin());
-    return;
+    return true;
   }
 
-  StringRef RelPath = OrigPath.substr(OldPrefix.size());
+  StringRef RelPath = OrigPath.substr(OldPrefixDir.size());
   SmallString<256> NewPath;
   path::append(NewPath, style, NewPrefix);
+  if (!RelPath.empty()) {
+    if (!is_separator(RelPath[0], style) || !strict)
       path::append(NewPath, style, RelPath);
+    else
+      path::append(NewPath, style, relative_path(RelPath, style));
+  }
+
   Path.swap(NewPath);
+
+  return true;
 }
 
 void native(const Twine &path, SmallVectorImpl<char> &result, Style style) {
   assert((!path.isSingleStringRef() ||
           path.getSingleStringRef().data() != result.data()) &&
          "path and result are not allowed to overlap!");
   // Clear result.
   result.clear();

unittests/Support/Path.cpp

TEST(Support, RemoveDots) {
   SmallString<64> Path2("././c");
   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> OldPrefix("/old");
+  SmallString<64> OldPrefixSep("/old/");
   SmallString<64> NewPrefix("/new");
   SmallString<64> NewPrefix2("/longernew");
   SmallString<64> EmptyPrefix("");
 
   SmallString<64> Path = Path1;
   path::replace_path_prefix(Path, OldPrefix, NewPrefix);
   EXPECT_EQ(Path, "/foo");
   Path = Path2;
   path::replace_path_prefix(Path, OldPrefix, NewPrefix);
   EXPECT_EQ(Path, "/new/foo");
   Path = Path2;
   path::replace_path_prefix(Path, OldPrefix, NewPrefix2);
   EXPECT_EQ(Path, "/longernew/foo");
   Path = Path1;
   path::replace_path_prefix(Path, EmptyPrefix, NewPrefix);
   EXPECT_EQ(Path, "/new/foo");
   Path = Path2;
   path::replace_path_prefix(Path, OldPrefix, EmptyPrefix);
   EXPECT_EQ(Path, "/foo");
+  Path = Path2;
+  path::replace_path_prefix(Path, OldPrefix, EmptyPrefix, true);
+  EXPECT_EQ(Path, "foo");
+  Path = Path3;
+  path::replace_path_prefix(Path, OldPrefix, NewPrefix, false);
+  EXPECT_EQ(Path, "/newnew/foo");
+  Path = Path3;
+  path::replace_path_prefix(Path, OldPrefix, NewPrefix, true);
+  EXPECT_EQ(Path, "/oldnew/foo");
+  Path = Path3;
+  path::replace_path_prefix(Path, OldPrefixSep, NewPrefix, true);
+  EXPECT_EQ(Path, "/oldnew/foo");
+  Path = Path1;
+  path::replace_path_prefix(Path, EmptyPrefix, NewPrefix);
+  EXPECT_EQ(Path, "/new/foo");
+  Path = OldPrefix;
+  path::replace_path_prefix(Path, OldPrefix, NewPrefix);
+  EXPECT_EQ(Path, "/new");
+  Path = OldPrefixSep;
+  path::replace_path_prefix(Path, OldPrefix, NewPrefix);
+  EXPECT_EQ(Path, "/new/");
+  Path = OldPrefix;
+  path::replace_path_prefix(Path, OldPrefixSep, NewPrefix, false);
+  EXPECT_EQ(Path, "/old");
+  Path = OldPrefix;
+  path::replace_path_prefix(Path, OldPrefixSep, NewPrefix, true);
+  EXPECT_EQ(Path, "/new");
 }
 
 TEST_F(FileSystemTest, OpenFileForRead) {
   // Create a temp file.
   int FileDescriptor;
   SmallString<64> TempPath;
   ASSERT_NO_ERROR(
       fs::createTemporaryFile("prefix", "temp", FileDescriptor, TempPath));