[PATCH] D85080: [lld-macho] Support static linking of thread-locals

[Jez Ng via Phabricator via llvm-commits]

int3 added inline comments.


================
Comment at: lld/MachO/Arch/X86_64.cpp:284
+
+    // Convert the movq to a leaq.
+    assert(isa<Defined>(&sym));
----------------
compnerd wrote:
> What do you think about moving this outside of this function?  This way this function remains a read-only operation which is what one would expect as being a getter.  Alternatively, you could rename the function if you think that this should be done in a single shot (I don't think that the access pattern is such that it would cause any performance implication).
I think doing it in one shot makes sense since the VA we are returning is valid only if the opcode matches. But I agree the method name is now misleading... maybe `resolveSymbolVA()` would be better?


================
Comment at: lld/MachO/Arch/X86_64.cpp:288
+      error("X86_64_RELOC_TLV must be used with movq instructions");
+    buf[-2] = 0x8d;
+    return sym.getVA();
----------------
compnerd wrote:
> This seems architecture specific.  Could you please base this on the architecture via a switch?
this entire file is meant to be architecture-specific


================
Comment at: lld/MachO/InputSection.h:42
+
+inline bool isThreadLocalVariables(uint8_t flags) {
+  return (flags & llvm::MachO::SECTION_TYPE) ==
----------------
compnerd wrote:
> nit: I think `isThreadLocalVariable` is a better name (the flag is checked on a single symbol)
it's being checked on a section (which can contain many TLVs), not a symbol


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85080