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

[Saleem Abdulrasool via Phabricator via llvm-commits]

compnerd 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");
----------------
Personally I prefer `TLV` as it is an initialism.


================
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.
----------------
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;
}
```


================
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,
----------------
Perhaps it makes sense to pass a `Symbol &` to `addDylib` rather than having a number of flags to the function?


================
Comment at: lld/MachO/SyntheticSections.cpp:141
   } else if (lastBinding.offset != offset) {
-    assert(lastBinding.offset <= offset);
     os << static_cast<uint8_t>(BIND_OPCODE_ADD_ADDR_ULEB);
----------------
Why the removal of this assert?  Seems unrelated (I suspect its related to the TLV section being filled in).


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85081