This is an archive of the discontinued LLVM Phabricator instance.

[lld-macho] Support static linking of thread-locals
ClosedPublic

Authored by int3 on Aug 1 2020, 3:26 PM.

Details

Reviewers
compnerd
smeenai
Group Reviewers
Restricted Project
Commits
rGca85e3733816: [lld-macho] Support static linking of thread-locals
Summary

Note: What ELF refers to as "TLS", Mach-O seems to refer to as "TLV", i.e.
thread-local variables.

This diff implements support for TLV relocations that reference defined
symbols. On x86_64, TLV relocations are always used with movq opcodes, so for
defined TLVs, we don't need to create a synthetic section to store the
addresses of the symbols -- we can just convert the movq to a leaq.

One notable quirk of Mach-O's TLVs is that absolute-address relocations
inside TLV-defining sections behave differently -- their addresses are
no longer absolute, but relative to the start of the target section.
(AFAICT, RIP-relative relocations are not allowed in these sections.)

Diff Detail

Event Timeline

int3 created this revision.Aug 1 2020, 3:26 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 1 2020, 3:26 PM
int3 requested review of this revision.Aug 1 2020, 3:26 PM
int3 retitled this revision from [lld-macho] Support static linking against thread-locals to [lld-macho] Support static linking of thread-locals.Aug 1 2020, 6:13 PM
compnerd added inline comments.
lld/MachO/Arch/X86_64.cpp
284

What do you think about moving this outside of this function? This way this function remains a read-only operation which is what one would expect as being a getter. Alternatively, you could rename the function if you think that this should be done in a single shot (I don't think that the access pattern is such that it would cause any performance implication).

288

This seems architecture specific. Could you please base this on the architecture via a switch?

lld/MachO/InputSection.h
42

nit: I think isThreadLocalVariable is a better name (the flag is checked on a single symbol)

int3 added inline comments.Aug 3 2020, 10:26 AM
lld/MachO/Arch/X86_64.cpp
284

I think doing it in one shot makes sense since the VA we are returning is valid only if the opcode matches. But I agree the method name is now misleading... maybe resolveSymbolVA() would be better?

288

this entire file is meant to be architecture-specific

lld/MachO/InputSection.h
42

it's being checked on a section (which can contain many TLVs), not a symbol

int3 updated this revision to Diff 282734.Aug 3 2020, 2:51 PM

getSymbolVA() -> resolveSymbolVA()

int3 marked an inline comment as done.Aug 3 2020, 2:51 PM
compnerd accepted this revision.Aug 5 2020, 12:00 PM
compnerd added inline comments.
lld/MachO/InputSection.h
42

Ah, that's confusing. Perhaps it makes sense to make this a member of OutputSection (would be a good follow up change).

This revision is now accepted and ready to land.Aug 5 2020, 12:00 PM
smeenai accepted this revision.Aug 5 2020, 5:54 PM
smeenai added a subscriber: smeenai.

LGTM

lld/MachO/InputSection.cpp
39

I'm assuming this should be resolveSymbolVA?

41

Is this not being put in resolveSymbolVA because it's target-independent?

Also, this is pretty strange ... I wonder why it's set up like this.

int3 updated this revision to Diff 283488.Aug 5 2020, 10:49 PM
int3 marked an inline comment as done.

update

int3 added inline comments.Aug 5 2020, 11:15 PM
lld/MachO/InputSection.cpp
39

yup

41

yup, I think it's target-independent

smeenai added inline comments.Aug 6 2020, 7:04 PM
lld/MachO/InputSection.cpp
41

Can you double check against ld64 to make sure that's the case? If so, this LGTM.

int3 marked an inline comment as done.Aug 6 2020, 7:17 PM
int3 added inline comments.
lld/MachO/InputSection.cpp
41

Yeah, src/ld/passes/tlvp.cpp does this in a target-independent manner.

This revision was landed with ongoing or failed builds.Aug 7 2020, 11:05 AM
This revision was automatically updated to reflect the committed changes.
int3 marked an inline comment as done.