This is an archive of the discontinued LLVM Phabricator instance.

Fix PathMappingList for relative and empty paths after recent FileSpec normalization changes
ClosedPublic

Authored by clayborg on May 17 2018, 10:33 AM.

Details

Summary

PathMappingList was broken for relative and empty paths after normalization changes in FileSpec. There were also no tests for PathMappingList so I added those.

Changes include:

  • Change PathMappingList::ReverseRemapPath() to take FileSpec objects instead of ConstString. The only client of this was doing work to convert to and from ConstString objects for no reason.
  • Normalize all paths prefix and replacements that are added to the PathMappingList vector so they match the paths that have been already normalized in the debug info
  • Unify code in the two forms of PathMappingList::RemapPath() so only one contains the actual functionality. Prior to this, there were two versions of this code.
  • Use FileSpec::AppendPathComponent() and remove a long standing TODO so paths are correctly appended to each other.
  • Correctly handle the case where someone maps "" to something else. This allows all paths to be prefixed with the replacement.
  • Added tests for absolute, relative and empty paths.

Diff Detail

Event Timeline

clayborg created this revision.May 17 2018, 10:33 AM
zturner added inline comments.May 17 2018, 10:44 AM
source/Target/PathMappingList.cpp
72

Slightly more idiomatic to say emplace_back(NormalizePath(path), NormalizePath(replacement));

169

I don't think the std::move is necessary here. Since the result of GetPath() is already a temporary (rvalue), the move assignment operator will already be invoked even without std::move.

195

llvm::StringRef path_ref(path); should be fine, no need to explicitly specify the pointer and size.

196–197

How about llvm::StringRef second = it.second.GetStringRef();

197

Can you invert this conditional and continue; if it's false?

197

It looks like after this check, the first second.size() characters of path_ref are ignored. So it sounds like you can write this as:

if (!path_ref.consume_front(second))
  continue;

Then further down you can delete the line that calls substr.

198

llvm::StringRef first = it.first.GetStringRef();

clayborg updated this revision to Diff 147354.May 17 2018, 11:06 AM

Fix issues found by Zach.

clayborg marked 7 inline comments as done.May 17 2018, 11:06 AM

Marked things as done.

Although it may not seem that way from the number of comments, the change looks good to me. The main thing is the moving of the test file, as that will fail in the cmake build. And it also looks like some code can be simplified if my assumption about not converting "" to "." is true.

Thank you for adding the test for this class.

source/Target/PathMappingList.cpp
44

s/GetCString/GetStringRef

97–98

s/insert/emplace

157

SetString(remapped)

200–203

Is this really true? Judging by the test you've had to remove in r332633, we shouldn't end up converting "" to ".".

unittests/Utility/PathMappingListTest.cpp
1 ↗(On Diff #147354)

This file should go to unittests/Target/PathMappingListTest.cpp to match where the class-under-test lives. (Also, please update the name in the header.)

39 ↗(On Diff #147354)

s/GetCString/GetStringRef (whole file)

42 ↗(On Diff #147354)

It's better to use EXPECT_EQ(expected, actual) for equality comparison, as that will print out a more useful error message when things fail.

86 ↗(On Diff #147354)

How does this work for relative paths? I take it foo.c should be remapped to /new/foo.c ? Can you add a test for that?

I will make the fixes and also test out mapping "" to "." as suggested.

source/Target/PathMappingList.cpp
203

When I thought about it, I chose to not convert to "." for path remapping. I think people would expect if the remap "" to "/prefix" that "/prefix" would be prepended to each path no matter what it is. Of course users could just specify "/" if they wish for a prefix. I could see this going either way. Let me know what you think.

unittests/Utility/PathMappingListTest.cpp
86 ↗(On Diff #147354)

It doesn't work. If "foo.c" would map to "/new/foo.c", we can unmap it correctly since it would unmap to "/foo.c". Both "foo.c" and "/foo.c" would map to to "/new/foo.c" and then we can only unmap the latter correctly.

This might bode well for saying that "" should map to "." actually. Then we won't run into this situation. I will test things out with "" mapping to "." and see how things go.

clayborg updated this revision to Diff 147587.May 18 2018, 2:30 PM
  • Fixed Pavel's issues
  • If user specifies "" as the first directory in PathMappingList, it will match "."
  • User can specify "/" as the first directory to remap all absolute paths
  • Fixed FileSpec::IsAbsolute() and added tests for issues I discovered when adding tests for relative paths in PathMappingListTests
  • Modified tests in PathMappingListTests to use a help function which simplifies code
  • Added more tests to things that shouldn't get remapped
labath accepted this revision.May 21 2018, 1:57 AM

looks good. I don't know if anyone else has an opinion on how should we treat "" for mapping purposes, but treating it as "." seems fine to me.

unittests/Utility/FileSpecTest.cpp
300

In tests like these, it's good to add something like SCOPED_TRACE(path) or similar. That way, when a test fails you will see which path caused the failure instead of just a generic "spec.IsRelative() is true" message.

This revision is now accepted and ready to land.May 21 2018, 1:57 AM
This revision was automatically updated to reflect the committed changes.