[PATCH] D57371: ELF: Allow GOT relocs pointing to non-preemptable ifunc to resolve to an IRELATIVE where possible.

[Peter Smith via Phabricator via llvm-commits]

peter.smith added a comment.

I've made a few suggestions on the comments, otherwise looks good to me. I think it is better to keep the complexity in one place and document it than spreading it out. It would be good to get some input from George and Rui, or someone else that hasn't been deep diving on ifuncs recently to see if they had any problems following the code. I've run this on some address comparison tests I had lying around for AArch64 and it looks good on these. I'll run these on Arm tomorrow, as that puts the .igot.plt in the .got and will comment if I find any problems, but I'm not expecting any.

I seem to remember FreeBsd making heavy use of ifuncs for some configuration, may be worth adding emaste as a reviewer to see if he can arrange for someone from that project to take a look and maybe run some tests.



================
Comment at: lld/ELF/Relocations.cpp:1115
+
+      // Create a copy of the symbol for the iplt in case we make the PLT
+      // canonical later, which would overwrite the original symbol.
----------------
May I suggest highlighting the use of DirectSym as it wasn't immediately apparent what canonical meant in the context, although with the comment above explaining canonical this might be less of a problem. Maybe something like:
"Create a copy of the symbol to use as the target of the IRELATIVE relocation in the igotPlt. This is in case we make the PLT canonical later, which would overwrite the original symbol."



================
Comment at: lld/ELF/Relocations.cpp:1142
+      // entry. If we did and we make the PLT canonical later, we'll need to
+      // create a GOT pointing to the PLT.
+      Sym.GotInIgot = true;
----------------
Maybe worth expanding last part to "create a GOT entry pointing to the PLT entry for Sym."


================
Comment at: lld/ELF/Relocations.cpp:1160
+      if (Sym.GotInIgot) {
+        // We previously saw a reference to a GOT, so we need to create one. We
+        // don't need to worry about creating a MIPS GOT here because ifuncs
----------------
I initially missed the Sym.GotInIgot = false; and thought about what might happen if there was a GOT generating reference after canonicalisation. Perhaps something like: 
"We previously encountered a GOT generating reference that we redirected to the Igot. Now that the PLT entry is canonical we must clear the redirection to the Igot and add a GOT entry. As we've changed the symbol type to STT_FUNC future got generating references will naturally use this GOT entry."


================
Comment at: lld/test/ELF/gnu-ifunc-canon.s:2
+// REQUIRES: x86
+// RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %s -o %t.o
+// RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %S/Inputs/gnu-ifunc-canon1.s -o %t1.o
----------------
I found it difficult to track the inputs from the outputs. For example it is easy to lose the type of reference from %t1.o, %t2.o and %t3.o. For example %t1.o has a RO pc-relative reference, %t2.o has a RO absolute reference and %t3.o has a RW  absolute + offset reference.  Maybe worth using %t-ro-pcrel.o etc.

Alternatively a comment above each ld.lld invocation with a description of the inputs, for example non-got-generating reference, expect canonical PLT.

I've not had a chance to check through each case yet. Will do that tomorrow and will post if I find any problems.  


Repository:
  rL LLVM

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

https://reviews.llvm.org/D57371