Page MenuHomePhabricator

Path: enhance prefix mapping

Authored by dankm on Jan 15 2019, 7:20 PM.



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

Diff Detail

Event Timeline

dankm created this revision.Jan 15 2019, 7:20 PM
Lekensteyn requested changes to this revision.Jan 16 2019, 6:17 AM

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


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


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


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


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

This revision now requires changes to proceed.Jan 16 2019, 6:17 AM
dankm updated this revision to Diff 182119.Jan 16 2019, 12:26 PM

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

dankm marked 6 inline comments as done.Jan 16 2019, 12:28 PM
dankm added inline comments.

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


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


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

Lekensteyn added inline comments.Jan 16 2019, 1:51 PM

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).


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.)


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


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 updated this revision to Diff 182183.Jan 16 2019, 4:13 PM
dankm marked 4 inline comments as done.

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 added inline comments.Jan 17 2019, 1:28 PM

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.


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))
// ...
if (OldPrefixDir.size() == NewPrefixDirLength) {

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?


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/, /

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 updated this revision to Diff 183239.Jan 23 2019, 7:37 PM

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

Should this be abandoned for D49466?

Herald added a project: Restricted Project. · View Herald TranscriptAug 10 2019, 7:56 AM
dankm abandoned this revision.Nov 25 2019, 2:47 PM

Should this be abandoned for D49466?

Yes, yes it should.


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.