Page MenuHomePhabricator

[lld-macho] Support dynamic linking of thread-locals
Needs ReviewPublic

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

Details

Reviewers
None
Group Reviewers
Restricted Project
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.Sat, Aug 1, 3:26 PM
Herald added a project: Restricted Project. · View Herald TranscriptSat, Aug 1, 3:26 PM
int3 requested review of this revision.Sat, Aug 1, 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.Sat, Aug 1, 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
410

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.Thu, Aug 6, 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
410

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.Thu, Aug 6, 1:18 PM
lld/MachO/SyntheticSections.h
104

I'm thinking NonLazyPointerBase or NonLazyPointerSectionBase

int3 added inline comments.Fri, Aug 7, 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.Mon, Aug 10, 7:17 PM
int3 marked an inline comment as done.

rename

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

clang-tidy

int3 added a comment.Wed, Aug 12, 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
4

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

20

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?)