[PATCH] D102862: [lld][ELF][SPARC] Support TLS GD relocations

[LemonBoy via Phabricator via llvm-commits]

LemonBoy added inline comments.


================
Comment at: lld/ELF/Arch/SPARCV9.cpp:212-217
+  case R_SPARC_TLS_GD_HI22:
+    relocateNoSym(loc, R_SPARC_HI22, val);
+    return;
+  case R_SPARC_TLS_GD_LO10:
+    relocateNoSym(loc, R_SPARC_LO10, val);
+    return;
----------------
jrtc27 wrote:
> R_SPARC_TLS_IE_HI22/LO10? This is wrong, but I guess this is a hack that kinda works because IE isn't actually implemented yet? Which probably should happen before landing GD support... LD is less clear, but also seemingly unimplemented, only basic LE.
I wouldn't call this a hack, as long as the relocation type matches it's fine to use `R_SPARC_HI22` or `R_SPARC_LO10` here.
I have a patch for LD, I'll send it once this is merged. I don't need IE at the moment but I'll eventually look into it.


================
Comment at: lld/ELF/Arch/SPARCV9.cpp:227
+  case R_SPARC_TLS_GD_ADD:
+    // add %r1, %r2, %r3 -> mov %g7, %r2, %r3
+    write32be(loc, (read32be(loc) & ~0x0007c000) | 0x0001c000);
----------------
jrtc27 wrote:
> Do you mean `add %g7, %r2, %r3`?
Yes, it's a typo I carried over from Gold as that's the only reference for the relaxation sequences.


================
Comment at: lld/ELF/Driver.cpp:929
   return m == EM_AARCH64 || m == EM_AMDGPU || m == EM_HEXAGON || m == EM_PPC ||
-         m == EM_PPC64 || m == EM_RISCV || m == EM_X86_64;
+         m == EM_PPC64 || m == EM_RISCV || m == EM_X86_64 || m == EM_SPARCV9;
 }
----------------
jrtc27 wrote:
> Please keep this sorted
Will do, I didn't notice they were sorted.


================
Comment at: lld/ELF/Relocations.cpp:291-294
+        // These relocations are applied on call instructions like
+        //   call __tls_get_addr, %rel(sym)
+        // If this relocation won't be relaxed we have to replace it with a
+        // PLT-relative one to __tls_get_addr.
----------------
jrtc27 wrote:
> Should this be more like hexagonNeedsTLSSymbol/hexagonTLSSymbolUpdate and combined with it, which has similar weirdness?
No, I find this approach much cleaner than the late patching done for Hexagon.
I also found that adding a PLT entry that late will fail to generate a PLT section if there was none in the input files.


================
Comment at: lld/ELF/Relocations.cpp:298-299
+
+        if (!call_sym->isInGot())
+          addGotEntry(*call_sym);
+        if (!call_sym->isInPlt())
----------------
jrtc27 wrote:
> Why would it need a GOT entry? SPARC doesn't even use a .got.plt, it synthesises the addresses directly in the PLT entries, but even on .got.plt architectures calls don't use the real GOT other than MIPS abicalls.
Ah, good catch, that was a leftover from some previous experiments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102862