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

nit: how about splitting those lines at 80 chars?

RUN: ... \
RUN:    -...
llvm/tools/dsymutil/DwarfStreamer.cpp
581

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

597

Any plans to implement this soon?

llvm/tools/dsymutil/SymbolMap.cpp
50

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

54

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

68

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

69

.

74

why parent_path first and then ..?

93

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

133

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

llvm/tools/dsymutil/SymbolMap.h
22

Doxygen comment?

26

ArrayRef or && or const& perhaps?

JDevlieghere marked 14 inline comments as done.

Feedback Adrian

llvm/tools/dsymutil/DwarfStreamer.cpp
597

Soon, hopefully :-)

llvm/tools/dsymutil/SymbolMap.cpp
133

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

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

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.