This is an archive of the discontinued LLVM Phabricator instance.

[lld/mac] Honor REFERENCED_DYAMICALLY, set it on __mh_execute_header
ClosedPublic

Authored by thakis on May 17 2021, 6:20 AM.

Details

Summary

Has the effect that __mh_execute_header stays in the symbol table of
outputs even after running strip on the output. I don't know if that's
important for anything -- my motivation for the patch is just is to make
the output more similar to ld64.

(Corresponds to symbolTableInAndNeverStrip in ld64.)

Diff Detail

Event Timeline

thakis created this revision.May 17 2021, 6:20 AM
Herald added a reviewer: gkm. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
thakis requested review of this revision.May 17 2021, 6:20 AM
thakis added inline comments.May 17 2021, 7:26 AM
lld/MachO/InputFiles.cpp
445

Turns out this isn't true, and http://llvm-cs.pcc.me.uk/lib/Support/PrettyStackTrace.cpp#121 does trigger this assert. I'll update.

thakis updated this revision to Diff 345899.May 17 2021, 8:33 AM
thakis retitled this revision from [lld/mac] Set REFERENCED_DYAMICALLY on __mh_execute_header to [lld/mac] Honor REFERENCED_DYAMICALLY, set it on __mh_execute_header.

handle on arbitrary symbols (via .desc, and used in PrettyStackTrace -- but see https://reviews.llvm.org/D27683#2763729 , seems a bit pointless)

This makes the patch quite a bit bigger (see diff to patch set 1) for unclear gain. Maybe we should do patch set 1 and just remove the assert for now? On the other hand, it seems like the correct behavior, so maybe we should just do it. The patch doesn't do this for Common and Absolute symbols; if anyone needs that they can add it themselves.

int3 accepted this revision.May 17 2021, 10:19 AM

lgtm, maintaining correct behavior seems worth the complexity

lld/MachO/InputFiles.cpp
493–497

Hmm I wonder if we should have Defined's ctor take in n_desc directly and then parses those flags. The list of params is getting a bit unwieldy

lld/MachO/SyntheticSections.cpp
859–860

nit: can we use a ternary like the other two lines above?

This revision is now accepted and ready to land.May 17 2021, 10:19 AM

Thanks!

lld/MachO/InputFiles.cpp
493–497

That either splits up the isWeakDefCanBeHidden code a bit awkwardly. Mind if I punt on that for now?

lld/MachO/SyntheticSections.cpp
859–860

Sure, done. It's two lines either way, so I figured I could do the more obvious two lines. But local consistency is good too.

int3 added inline comments.May 17 2021, 10:51 AM
lld/MachO/InputFiles.cpp
493–497

yeah we can figure it out in a future diff

This revision was landed with ongoing or failed builds.May 17 2021, 11:22 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 17 2021, 11:22 AM