This is an archive of the discontinued LLVM Phabricator instance.

[FileSpec] Re-implmenet removeLastPathComponent
ClosedPublic

Authored by JDevlieghere on May 29 2018, 1:46 PM.

Details

Summary

When reading DBGSourcePathRemapping from a dSYM, we remove the last two
path components to make the source lookup more general. However, when
dealing with a relative path that has less than 2 components, we ended
up with an invalid (empty) FileSpec.

This patch changes the behavior of removeLastPathComponent to remove the
last path component, if possible. It does this by checking whether a
parent path exists, and if so using that as the new path. We rely
entirely on LLVM's path implementation to do the heavy lifting.

We now also return a boolean which indicates whether the operator was
successful or not.

Diff Detail

Event Timeline

JDevlieghere created this revision.May 29 2018, 1:46 PM

This looks good. My original change in r311622 to add this "remove the last two file path components" should have explicitly said that this is only done for DBGVersion == 2 to make it clearer for people reading the code.

At this point the parsing code behaves like:

DBGVersion 1 (or missing DBGVersion) - ignore the DBGSourcePathRemapping dictionary
DBGVersion 2 - Use DBGSourcePathRemapping, but chop off two components from both source and destination (assumption: the last two components are PROJECT-NAME/PROJECT-VERSION-NUMBER)
DBGVersion 3 - Use DBGSourcePathRemapping without any modifications

These interpretations of the DBGSourcePathRemapping / DBGVersion have been a little ad-hoc, as we've discovered issues in deployed plists that needed to be accommodated by lldb.

clayborg requested changes to this revision.May 29 2018, 2:38 PM

Remove the argument since we don't need it. Just assume we keep ".".

include/lldb/Utility/FileSpec.h
535 ↗(On Diff #148976)

Why add this? If the path is just "." to begin with, there is nothing to do. I would vote to not add this argument since we don't need it. Add it back if we ever do.

source/Host/macosx/Symbols.cpp
412–415 ↗(On Diff #148976)

revert

source/Plugins/SymbolVendor/MacOSX/SymbolVendorMacOSX.cpp
263–266 ↗(On Diff #148976)

revert

source/Utility/FileSpec.cpp
788

Remove arg.

This revision now requires changes to proceed.May 29 2018, 2:38 PM
clayborg added inline comments.May 29 2018, 2:38 PM
source/Utility/FileSpec.cpp
821

revert whitespace change.

JDevlieghere added inline comments.May 29 2018, 2:43 PM
include/lldb/Utility/FileSpec.h
535 ↗(On Diff #148976)

The reason I opted for a flag rather than changing the behavior is that other (llvm & foundation) implementation of similar functions behave the same as the current implementation. However I think an argument can be made for this as well, so I'm happy to change it :-)

JDevlieghere edited the summary of this revision. (Show Details)
  • Address Greg's feedback
JDevlieghere marked 2 inline comments as done.May 29 2018, 2:47 PM
This revision is now accepted and ready to land.May 29 2018, 2:48 PM

I was also thinking whether this behavior needs to be conditional. If nothing depends on this, then I'm all for changing the condition. However, my question is whether "." is the only path we should treat this way. I'm thinking it would be more consistent to give the root directory the same treatment too (so, "/".RemoveLastComponent() == "/", "//net".RemoveLastComponent()=="//net", etc). I guess you're unlikely to encounter an absolute path with less than two components in the path mappings, but it sounds like this is the behavior you would want there anyway.

Also, a test for the new behavior of the FileSpec function would be in order, as there are some interesting corner cases which I am not sure you get right (e.g, what is the value of "foo".RemoveLastComponent()?)

source/Utility/FileSpec.cpp
796

This won't change the result of this particular comparison, but it's best to get in the habit of specifying the path syntax when constructing FileSpecs. "native" is not always the right choice.

JDevlieghere marked an inline comment as done.
  • Replace custom logic with LLVM's path logic.
  • Add tests.
JDevlieghere retitled this revision from Support relative paths with less than two components in DBGSourcePathRemapping to [FileSpec] Re-implmenet removeLastPathComponent.May 30 2018, 4:10 AM
JDevlieghere edited the summary of this revision. (Show Details)
labath added inline comments.May 30 2018, 5:35 AM
unittests/Utility/FileSpecTest.cpp
342 ↗(On Diff #149083)

Is this the behavior you want here? I was thinking we could fold this all the way to "." (arguably "." is a parent of "foo", though I can see how that may be thought to be too magical)

JDevlieghere added inline comments.May 30 2018, 5:45 AM
unittests/Utility/FileSpecTest.cpp
342 ↗(On Diff #149083)

I like this approach it doesn't consider . to be a special case and therefore things are consistent and straightforward. My worry is that if we add special cases, we risk ending up with missing edge cases (like the previous implementation).

labath accepted this revision.May 30 2018, 6:01 AM
labath added inline comments.
unittests/Utility/FileSpecTest.cpp
342 ↗(On Diff #149083)

Ok, if that works for your use case (as far as path remappings go, "." is a more generic mapping than "foo"), then that's fine by me. I like how you were able to concisely describe the new semantics of this function.

This revision was automatically updated to reflect the committed changes.