This is an archive of the discontinued LLVM Phabricator instance.

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

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

Details

Reviewers
smeenai
Group Reviewers
Restricted Project
Commits
rG3c9100fb785c: [lld-macho] Support dynamic linking of thread-locals
Summary

References to symbols in dylibs work very similarly regardless of
whether the symbol is a TLV. The main difference is that we have a
separate __thread_ptrs section that acts as the GOT for these
thread-locals.

We can identify thread-locals in dylibs by a flag in their export trie
entries, and we cross-check it with the relocations that refer to them
to ensure that we are not using a GOT relocation to reference a
thread-local (or vice versa).

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 dynamic linking against thread-locals to [lld-macho] Support dynamic linking of thread-locals.Aug 1 2020, 6:14 PM
compnerd added inline comments.
lld/MachO/Arch/X86_64.cpp
227

Personally I prefer TLV as it is an initialism.

lld/MachO/ExportTrie.cpp
67

What do you think of extracting this into a simple helper function that should be marked as inline?

static inline uint32_t GetSymbolFlags(const Symbol &S) {
  uint32_t flags = 0;
  if (S.isWeakDefa()) flags |= EXPORT_SYMBOL_FLAGS_WEAK_DEFINITION;
  if (S.isTLV()) flags |= EXPORT_SYMBOL_FLAGS_KIND_THREAD_LOCAL;
  ...
  return flags;
}
lld/MachO/InputFiles.cpp
398

Perhaps it makes sense to pass a Symbol & to addDylib rather than having a number of flags to the function?

lld/MachO/SyntheticSections.cpp
141

Why the removal of this assert? Seems unrelated (I suspect its related to the TLV section being filled in).

lld/MachO/SyntheticSections.h
104

Would you mind renaming this type? It is confusing with __nl_symbol_ptr being a specific section.

int3 added inline comments.Aug 6 2020, 1:16 PM
lld/MachO/Arch/X86_64.cpp
227

I picked this to match LLD-ELF's isTls()

lld/MachO/ExportTrie.cpp
67

updateOffset has the potential to query flags many times, so I wanted to avoid doing unnecessary computation. I also don't see how a helper function helps readability here...

lld/MachO/InputFiles.cpp
398

I agree that it's starting to look a bit unwieldy, but it looks like the equivalent addSymbol method in ld64 only takes these two flags, so I don't think we have to worry about a further proliferation of flags...

also passing Symbol & in wouldn't work very elegantly with our placement-new-based replaceSymbol implementation.

lld/MachO/SyntheticSections.h
104

Do you have a suggestion? I picked this name since all these sections will have the S_NON_LAZY_SYMBOL_POINTERS flag set. I initially didn't think the collision with __nl_symbol_ptr was super important since it no longer seems to be used when linking for x86_64 targets, but I've realized that it's still in use for ARM (and i386) targets

int3 added inline comments.Aug 6 2020, 1:18 PM
lld/MachO/SyntheticSections.h
104

I'm thinking NonLazyPointerBase or NonLazyPointerSectionBase

int3 added inline comments.Aug 7 2020, 10:10 AM
lld/MachO/SyntheticSections.cpp
141

yeah there's no guarantee that the TLV section will come after the GOT / before the rest of the sections. Actually there's no guarantee that the GOT comes before either, I think we just got lucky that the assert didn't get triggered earlier...

Anyway, I previously added this assert because I'd thought that BIND_OPCODE_ADD_ADDR_ULEB was adding an unsigned, arbitrary-sized integer to the address offset. But it turns out that it's actually adding a signed 64-bit value, so we can subtract from the offset here.

int3 updated this revision to Diff 284556.Aug 10 2020, 7:17 PM
int3 marked an inline comment as done.

rename

int3 updated this revision to Diff 285131.Aug 12 2020, 10:04 AM

clang-tidy

int3 added a comment.Aug 12 2020, 11:03 AM

Bump. There are a number of diffs stacked on top of this one, hoping to land them soon!

Can you also add a test with both GOT and TLV bindings, to ensure nothing's getting mixed up between the two?

lld/MachO/Arch/X86_64.cpp
228

Would be nice to include information about the file, section and offset of the incorrect relocation, if possible.

264

Same here.

lld/MachO/Symbols.h
63

It's slightly confusing to have gotIndex possibly refer to the TLV index, but I can't immediately think of a better name (short of gotOrTlvIndex or perhaps something like pointerSectionIndex).

lld/MachO/SyntheticSections.cpp
175

Can we just use idx to make this more generic, since it'll be for the TLV ones too?

lld/MachO/SyntheticSections.h
101

"they are will both be" -> "they will both be"

lld/test/MachO/tlv-dylib.s
5

The split-file utility was landed last week (rGbcea3a7a288). It might be nicer for some of these tests than the long echo lines.

21

Is this DAG just because you don't care about the order of the bind table entries, or because there's a potential for the order to be different? (I would hope the latter isn't the case?)

int3 marked 5 inline comments as done.Aug 12 2020, 5:47 PM
int3 added inline comments.
lld/MachO/Arch/X86_64.cpp
228

offset is a bit tricky due to subsection splitting, but including the other two is straightforward enough

lld/test/MachO/tlv-dylib.s
5

ooh, that is a sweet tool

21

The former -- the ordering should be arbitrary (but deterministic)

int3 updated this revision to Diff 285225.Aug 12 2020, 5:47 PM
int3 marked 2 inline comments as done.

address comments

smeenai accepted this revision.Aug 12 2020, 5:57 PM
smeenai added a subscriber: MaskRay.

Can you also add a test with both GOT and TLV bindings, to ensure nothing's getting mixed up between the two?

I don't see this one? LGTM otherwise.

lld/MachO/Symbols.h
63

Do you have any better naming ideas for this?

lld/test/MachO/tlv-dylib.s
5

CC @MaskRay :)

This revision is now accepted and ready to land.Aug 12 2020, 5:57 PM
int3 updated this revision to Diff 285231.Aug 12 2020, 6:16 PM

extend test

int3 added a comment.Aug 12 2020, 6:16 PM

I don't see this one? LGTM otherwise.

Whoops, I carefully checked off all the inline ones but forgot to look at the non-inline one 😅I've extended tlv-dylib to have a GOT entry.

lld/MachO/InputSection.cpp
61 ↗(On Diff #285225)

NB: this is the same format used by LLD-ELF

lld/MachO/Symbols.h
63

hm, I think this is fine... I personally think of the TLV section as "GOT for thread locals", so this naming doesn't seem too far off the mark

I don't see this one? LGTM otherwise.

Whoops, I carefully checked off all the inline ones but forgot to look at the non-inline one 😅I've extended tlv-dylib to have a GOT entry.

Hah, I figured. Thanks!

lld/MachO/Symbols.h
63

Sure, works for me.

This revision was automatically updated to reflect the committed changes.