[PATCH] D85081: [lld-macho] Support dynamic linking of thread-locals

[Jez Ng via Phabricator via llvm-commits]

int3 added inline comments.


================
Comment at: lld/MachO/Arch/X86_64.cpp:227
     in.got->addEntry(sym);
+    if (sym.isTlv())
+      error("GOT relocations must not reference thread-local variables");
----------------
compnerd wrote:
> Personally I prefer `TLV` as it is an initialism.
I picked this to match LLD-ELF's `isTls()`


================
Comment at: lld/MachO/ExportTrie.cpp:67
+    if (sym.isTlv())
+      flags |= EXPORT_SYMBOL_FLAGS_KIND_THREAD_LOCAL;
+    // TODO: Add proper support for re-exports & stub-and-resolver flags.
----------------
compnerd wrote:
> 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;
> }
> ```
`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...


================
Comment at: lld/MachO/InputFiles.cpp:398
     if (symbol->getArchitectures().has(config->arch))
       symbols.push_back(symtab->addDylib(saver.save(symbol->getName()),
+                                         umbrella, /*isWeakDef=*/false,
----------------
compnerd wrote:
> Perhaps it makes sense to pass a `Symbol &` to `addDylib` rather than having a number of flags to the function?
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.


================
Comment at: lld/MachO/SyntheticSections.h:104
+// that the TLVPointerSection stores references to thread-local variables.
+class NonLazyPointerSection : public SyntheticSection {
 public:
----------------
compnerd wrote:
> Would you mind renaming this type?  It is confusing with `__nl_symbol_ptr` being a specific section.
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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85081/new/

https://reviews.llvm.org/D85081