This is an archive of the discontinued LLVM Phabricator instance.

[lld-macho] Implement weak binding for branch relocations
ClosedPublic

Authored by int3 on Aug 25 2020, 1:21 PM.

Details

Reviewers
smeenai
Group Reviewers
Restricted Project
Commits
rGe263287c797f: [lld-macho] Implement weak binding for branch relocations
Summary

Since there is no "weak lazy" lookup, function calls to weak symbols are
always non-lazily bound. We emit both regular non-lazy bindings as well
as weak bindings, in order that the weak bindings may overwrite the
non-lazy bindings if an appropriate symbol is found at runtime. However,
the bound addresses will still be written (non-lazily) into the
LazyPointerSection.

Diff Detail

Event Timeline

int3 created this revision.Aug 25 2020, 1:21 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 25 2020, 1:21 PM
int3 requested review of this revision.Aug 25 2020, 1:21 PM
smeenai accepted this revision.Aug 26 2020, 9:21 PM
smeenai added a subscriber: smeenai.

LGTM

lld/MachO/Arch/X86_64.cpp
32

Out of curiosity, what makes us need the explicit macho:: qualifier here?

lld/MachO/Symbols.h
70

Nit: add a newline before this, to separate it from the block of gotIndex and its comment.

lld/test/MachO/weak-binding.s
109

If I'm understanding the code correctly, a weak non-external function would behave the same as a weak external function (i.e. it would get a weak binding entry), which is different from non-external data. Is that intended, and either way, could you add a test?

lld/test/MachO/weak-definition-order.s
28–30

Do we also wanna check the weak bind entries?

This revision is now accepted and ready to land.Aug 26 2020, 9:21 PM
int3 marked 2 inline comments as done.Aug 26 2020, 11:34 PM
int3 added inline comments.
lld/MachO/Arch/X86_64.cpp
32

it appears to be conflicting with llvm::object::Archive::Symbol. I didn't look into why it started conflicting in this diff and not before...

lld/test/MachO/weak-binding.s
109

nope... weak non-externals cannot be coalesced at runtime. I'm not sure the weak flag does anything meaningful to internal symbols actually...

will add the test

lld/test/MachO/weak-definition-order.s
28–30

the weak bind info doesn't include the dylib name, so it won't tell us which symbol got priority here

int3 marked an inline comment as done.Aug 26 2020, 11:37 PM
int3 added inline comments.
lld/test/MachO/weak-binding.s
109

oh, yeah, I see what you're saying now -- my code doesn't actually implement that properly 😅

int3 updated this revision to Diff 288211.Aug 27 2020, 12:02 AM

address comments

smeenai accepted this revision.Aug 27 2020, 10:56 AM

LGTM

lld/test/MachO/weak-definition-order.s
28–30

Ah, right.

This revision was automatically updated to reflect the committed changes.