This is an archive of the discontinued LLVM Phabricator instance.

[dsymutil] Update unobfuscation logic.
ClosedPublic

Authored by JDevlieghere on Jan 4 2019, 6:31 PM.

Details

Summary

The unobufscation support for BCSymbolMaps was the last piece of code that hasn't been upstreamed yet. This patch contains a reworked version of the existing code and relevant tests.

Diff Detail

Repository
rL LLVM

Event Timeline

JDevlieghere created this revision.Jan 4 2019, 6:31 PM
aprantl added inline comments.Jan 7 2019, 9:45 AM
llvm/test/tools/dsymutil/ARM/obfuscated.test
5 ↗(On Diff #180351)

nit: how about splitting those lines at 80 chars?

RUN: ... \
RUN:    -...
llvm/tools/dsymutil/DwarfStreamer.cpp
581 ↗(On Diff #180351)

comment should be on the declaration not the implementation, I believe.

597 ↗(On Diff #180351)

Any plans to implement this soon?

llvm/tools/dsymutil/SymbolMap.cpp
50 ↗(On Diff #180351)

s/that weird/cross reference to the MC code that explains why we do this/ ?

54 ↗(On Diff #180351)

this is in namespace llvm, does this need to be qualified?

68 ↗(On Diff #180351)

It's unfortunate that dsymutil on non-macOS will behave differently... but this is not a regression

69 ↗(On Diff #180351)

.

74 ↗(On Diff #180351)

why parent_path first and then ..?

93 ↗(On Diff #180351)

perhaps introduce a local var to make this more readable with 80 chars?

133 ↗(On Diff #180351)

perhaps use StringRef::consume() for premature optimization?

llvm/tools/dsymutil/SymbolMap.h
22 ↗(On Diff #180351)

Doxygen comment?

26 ↗(On Diff #180351)

ArrayRef or && or const& perhaps?

JDevlieghere marked 14 inline comments as done.

Feedback Adrian

llvm/tools/dsymutil/DwarfStreamer.cpp
597 ↗(On Diff #180351)

Soon, hopefully :-)

llvm/tools/dsymutil/SymbolMap.cpp
133 ↗(On Diff #180351)

I tried but this makes the code more complicated because we need to account for either the version number missing or the whole thing missing.

llvm/tools/dsymutil/SymbolMap.h
26 ↗(On Diff #180351)

Neither would improve performance as we're moving it into a member. I believe it's customary to take the argument by value in this case (although an r-value reference would be equivalent).

aprantl accepted this revision.Jan 7 2019, 2:55 PM
aprantl added inline comments.
llvm/tools/dsymutil/dsymutil.cpp
515 ↗(On Diff #180564)

That comment doesn't carry a lot of information :-)

This revision is now accepted and ready to land.Jan 7 2019, 2:55 PM
This revision was automatically updated to reflect the committed changes.