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

[Shoaib Meenai via Phabricator via llvm-commits]

smeenai added a comment.

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



================
Comment at: lld/MachO/Arch/X86_64.cpp:228
+    if (sym.isTlv())
+      error("GOT relocations must not reference thread-local variables");
     break;
----------------
Would be nice to include information about the file, section and offset of the incorrect relocation, if possible.


================
Comment at: lld/MachO/Arch/X86_64.cpp:264
+    if (!sym.isTlv())
+      error("X86_64_RELOC_TLV must reference a thread-local variable");
     break;
----------------
Same here.


================
Comment at: lld/MachO/Symbols.h:63
+  // referenced by both these sections at once.
   uint32_t gotIndex = UINT32_MAX;
 
----------------
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`).


================
Comment at: lld/MachO/SyntheticSections.cpp:175
+  bool didEncode = false;
+  size_t gotIdx = 0;
+  for (const Symbol *sym : osec->getEntries()) {
----------------
Can we just use `idx` to make this more generic, since it'll be for the TLV ones too?


================
Comment at: lld/MachO/SyntheticSections.h:101
+// This is the base class for the GOT and TLVPointer sections, which are nearly
+// functionally identical -- they are will both be populated by dyld with
+// addresses to non-lazily-loaded dylib symbols. The main difference is that the
----------------
"they are will both be" -> "they will both be"


================
Comment at: lld/test/MachO/tlv-dylib.s:4
+
+# RUN: echo ".section	__DATA,__thread_vars,thread_local_variables; .globl _foo, _bar; _foo:; _bar:" |\
+# RUN:   llvm-mc -filetype=obj -triple=x86_64-apple-darwin -o %t/libtlv.o
----------------
The split-file utility was landed last week (rGbcea3a7a288). It might be nicer for some of these tests than the long echo lines.


================
Comment at: lld/test/MachO/tlv-dylib.s:20
+# CHECK-LABEL: Bind table:
+# CHECK-DAG: __DATA __thread_ptrs  0x{{0*}}[[#%x, FOO]] pointer 0   libtlv   _foo
+# CHECK-DAG: __DATA __thread_ptrs  0x{{0*}}[[#%x, BAR]] pointer 0   libtlv   _bar
----------------
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?) 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85081