[PATCH] D87318: [LLD][PowerPC] Add support for R_PPC64_GOT_TLSGD_PCREL34 used in TLS General Dynamic

[Victor Huang via Phabricator via llvm-commits]

NeHuang added inline comments.


================
Comment at: lld/ELF/Arch/PPC64.cpp:731
+  case R_PPC64_GOT_TLSGD_PCREL34:
+    writePrefixedInstruction(loc, 0x06000000386d0000); // paddi r3, r13
+    relocateNoSym(loc, R_PPC64_TPREL34, val);
----------------
nit: can we add the comment to elaborate `Relax from ... to ...` ?


================
Comment at: lld/ELF/Arch/PPC64.cpp:735
+  case R_PPC64_TLSGD: {
+    const uintptr_t locAsInt = reinterpret_cast<uintptr_t>(loc);
+    if ((locAsInt % 4) == 0) {
----------------
ditto


================
Comment at: lld/ELF/Arch/PPC64.cpp:745
+    } else if ((locAsInt % 4) == 1) {
+      write32(loc - 1, NOP);     // nop
+    } else {
----------------
nit: clang-format needed.


================
Comment at: lld/ELF/Arch/PPC64.cpp:1411
+  case R_PPC64_GOT_TLSGD_PCREL34: {
+    // pld r3, sym at got@tprel at pcrel
+    writePrefixedInstruction(loc, 0x04100000e4600000);
----------------
nit: can we add more comments as existing cases like // Relax from ...... to .....?


================
Comment at: lld/ELF/Arch/PPC64.cpp:1414
+    relocateNoSym(loc, R_PPC64_GOT_TPREL_PCREL34, val);
+    break;
+  }
----------------
return?


================
Comment at: lld/ELF/Arch/PPC64.cpp:1416
+  }
+  case R_PPC64_TLSGD: {
+    const uintptr_t locAsInt = reinterpret_cast<uintptr_t>(loc);
----------------
ditto, add the comment `Relax from ... to ...`


================
Comment at: lld/ELF/Relocations.cpp:1365
+                    getLocation(sec, sym, offset));
+        return;
+      }
----------------
Will the `return` be executed after the `errorOrWarn`?


================
Comment at: lld/ELF/Relocations.cpp:1371
+                    getLocation(sec, sym, offset));
+        return;
+      }
----------------
ditto


================
Comment at: lld/test/ELF/ppc64-tls-pcrel-gd.s:67
+
+# GDTOLE-RELOC: There are no relocations in this file.
+
----------------
Are we missing the  relocation `R_PPC64_TPREL34` here for GDTOLE?


================
Comment at: lld/test/ELF/ppc64-tls-pcrel-gd.s:143
+GDTwoVal:
+        paddi 3, 0, x at got@tlsgd at pcrel, 1
+	bl __tls_get_addr at notoc(x at tlsgd)
----------------
alignment issue or phabricator display issue?


================
Comment at: lld/test/ELF/ppc64-tls-pcrel-gd.s:181
+GDIncrementVal:
+        paddi 3, 0, y at got@tlsgd at pcrel, 1
+	bl __tls_get_addr at notoc(y at tlsgd)
----------------
ditto


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87318