This is an archive of the discontinued LLVM Phabricator instance.

[dsymutil] Upstream update feature.
ClosedPublic

Authored by JDevlieghere on Feb 3 2018, 2:16 AM.

Details

Summary

Now that dsymutil can generate accelerator tables, we can upstream the
update logic that, as the name implies, updates the accelerator tables
in an existing dSYM bundle. In combination with -minimize this can be
used to remove redundant .debug_(inlines|pubtypes|pubnames).

Diff Detail

Repository
rL LLVM

Event Timeline

JDevlieghere created this revision.Feb 3 2018, 2:16 AM
aprantl accepted this revision.Feb 5 2018, 9:15 AM

Couple nitpicks inline :-)

llvm/tools/dsymutil/DwarfLinker.cpp
512 ↗(On Diff #132733)

Comment about what this function does?

517 ↗(On Diff #132733)

should this be a loop instead of recursion? We previously ran into stack limits with dsymutil...

723 ↗(On Diff #132733)

doxygen comment?

llvm/tools/dsymutil/dsymutil.cpp
90 ↗(On Diff #132733)

Does this need to go into the manpage, too?

This revision is now accepted and ready to land.Feb 5 2018, 9:15 AM
JDevlieghere marked 4 inline comments as done.

CR Feedback @aprantl

aprantl accepted this revision.Feb 6 2018, 10:12 AM
davide requested changes to this revision.Feb 6 2018, 10:19 AM

Just a couple of nits.

llvm/tools/dsymutil/DwarfLinker.cpp
2890 ↗(On Diff #132959)

LLVM_UNLIKELY ? Does this really matter?

3235 ↗(On Diff #132959)

ditto.

This revision now requires changes to proceed.Feb 6 2018, 10:19 AM

Just a couple of nits.

I have to admit that I didn't measure its impact, but it's how it's currently implemented on our internal branch. The DwarfLinker isn't exactly super fast and obviously we don't want to risk it becoming slower for this (really unlikely) update path. If you're really opposed to this I can do some measurements. but in the end I don't think it hurts readability/maintainability which motivates me to err on the side of caution.

davide accepted this revision.Feb 6 2018, 10:46 AM

Nah, it's not worth discussing this further. Go ahead.

This revision is now accepted and ready to land.Feb 6 2018, 10:46 AM
This revision was automatically updated to reflect the committed changes.