[PATCH] D83669: [PowerPC] Support for R_PPC64_REL24_NOTOC calls where the caller has no TOC and the callee is not DSO local

Wed Jul 29 05:00:00 PDT 2020

sfertile added inline comments.

================
Comment at: lld/ELF/Thunks.cpp:1043
+    return (type == R_PPC64_REL24_NOTOC)
+               ? (Thunk *)make<PPC64PCRelPLTStub>(s)
+               : (Thunk *)make<PPC64PltCallStub>(s);
----------------
NeHuang wrote:
> sfertile wrote:
> > Are the casts needed?
> Yeah, the ternary conditional operator would complain since `make<PPC64PCRelPLTStub>(s)` and `make<PPC64PltCallStub>(s)` has different value type.
Thanks.

================
Comment at: lld/ELF/Thunks.cpp:1042
   if (s.isInPlt())
-    return make<PPC64PltCallStub>(s);
+    return type == R_PPC64_REL24_NOTOC
+               ? (Thunk *)make<PPC64PCRelPLTStub>(s)
----------------
nit: clang-format

================
Comment at: lld/test/ELF/ppc64-pcrel-call-to-extern.s:30
+# SYMBOL: Symbol table '.dynsym' contains 4 entries:
+# SYMBOL: 1: 0000000000000000     0 NOTYPE  GLOBAL DEFAULT [<other: 0x60>] UND callee_global_TOC
+# SYMBOL-NEXT: 2: 0000000000000000     0 NOTYPE  GLOBAL DEFAULT                UND callee_global_stother0
----------------
minor nit: add whitepace so it lines up with the following lines

================
Comment at: lld/test/ELF/ppc64-pcrel-call-to-extern.s:35
+# SYMBOL: Symbol table '.symtab' contains 12 entries:
+# SYMBOL: 2: 0000000010010000     0 NOTYPE  LOCAL  DEFAULT [<other: 0x20>]   6 caller1
+# SYMBOL-NEXT: 3: 0000000010020000     0 NOTYPE  LOCAL  DEFAULT [<other: 0x20>]   7 caller2
----------------
Can you add spaces to the lines that are missing '-NEXT' to keep the columns aligned.

================
Comment at: lld/test/ELF/ppc64-pcrel-call-to-extern.s:50
+## .plt[0] holds the address of _dl_runtime_resolve, .plt[0]:0x0000000010030140
+## .plt[1] holds the link map, .plt[1]:0x0000000010030148
+## The JMP_SLOT relocations are stored at .plt[2], .plt[3], .plt[4]
----------------
I believe plt[1] is the dynamic object identifier. Maybe its best to say the first 2 entries in the plt are reserved for the  dynamic linkers usage and skip mentioning what they are used for.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83669