Page MenuHomePhabricator

Path: enhance prefix mapping
Needs ReviewPublic

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

Details

Summary

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.

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.

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