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

Wed Jan 30 23:48:26 PST 2019

pcc added a comment.

PTAL, this should be ready for review now.

================
Comment at: lld/ELF/InputSection.cpp:613
     return Sym.getVA(A) - getARMStaticBase(Sym);
   case R_GOT:
   case R_RELAX_TLS_GD_TO_IE_ABS:
----------------
peter.smith wrote:
> I think that the only use for the _PLT RelType is for redirection to ifunc PLT entries, you might be able to get rid of them from Relocations.h. 
It looks like `R_PLT` is still used for `R_MICROMIPS_26_S1`. I was able to remove `R_AARCH64_PLT_PAGE_PC`, though, by noting that the only remaining caller of `toPlt` is now in the thunk creation code which I believe operates on relocations that were relaxed by `fromPlt`.

By the way, if I comment out that call to `toPlt`, all the tests still pass. We might want to add a test for that code.

================
Comment at: lld/ELF/Relocations.cpp:1033
+    // now. This works because dynamic loaders evaluate IRELATIVE relocs
+    // eagerly.
+    if (!Sym.isInPlt()) {
----------------
peter.smith wrote:
> As this comes out of WIP, I think it would be good to expand the comment to cover ifunc handling in general as there isn't a good external reference for them. The decisions below may seem a bit arbitrary without prior knowledge.
I wrote a long comment here which is basically a brain dump of everything that I've learned about ifuncs while working on PR40474.

================
Comment at: lld/ELF/Relocations.cpp:1048
+    if (!needsPlt(Expr) && !needsGot(Expr)) {
+      // Create a canonical plt entry for the ifunc and redirect all references
+      // to point to it.
----------------
peter.smith wrote:
> Just to check I understand. This is the case where we have a non-generating address taking reference of the ifunc symbol that we need to redirect to the PLT entry for the ifunc symbol. Instead of changing the RelExpr to a _PLT form so that the PLT address of the ifunc is returned, we create a new non-ifunc symbol at the offset of the PLT entry for the ifunc symbol. This symbol inherits the PltIndex and GotIndex so that all RelExpr expressions resolve to the same address. As this symbol replaces the original one a new symbol (DirectSym) above needs to be created just for the purposes of the IRELATIVE relocation.
> 
> I think that this will have the property of changing the address of the symbol in the static and the dynamic symbol table (if the symbol is exported or a DSO references it). The former might be a little confusing but is otherwise harmless. Would be interesting to see what the dynamic loader will report to the shared library in that case. I note that ld.bfd gives an error message to recompile with PIC if the symbol is address taken using a non-got generating reference and exported; I think it would be worth following suit and giving an error.
> 
> 
> Just to check I understand. This is the case where we have a non-generating address taking reference of the ifunc symbol that we need to redirect to the PLT entry for the ifunc symbol. Instead of changing the RelExpr to a _PLT form so that the PLT address of the ifunc is returned, we create a new non-ifunc symbol at the offset of the PLT entry for the ifunc symbol. 

Correct.

> This symbol inherits the PltIndex and GotIndex so that all RelExpr expressions resolve to the same address.

Only all PLT-generating RelExprs resolve to the same address. The symbol would never have received a GotIndex because in the non-preemptible ifunc case we always just create a PLT if the relocation is GOT-generating. If a GOT is required after creating the canonical PLT (or if it was required before creating the PLT, which is now kept track of using the field GotInIgot), a GOT entry is created that points to the canonical PLT instead of the ifunc itself, to ensure address consistency between GOT and direct relocations. This handles the same case as what used to be handled by R_AARCH64_GOT_PAGE_PC_PLT.

> As this symbol replaces the original one a new symbol (DirectSym) above needs to be created just for the purposes of the IRELATIVE relocation.
> 
> I think that this will have the property of changing the address of the symbol in the static and the dynamic symbol table (if the symbol is exported or a DSO references it). 

Correct.

> The former might be a little confusing but is otherwise harmless.

While updating the tests I realised that in fact the objdump output will normally look a little nicer if the ifunc is redirected because normally an ifunc is declared as an alias of a resolver, which means that the resolver's disassembly will be labelled with the resolver's symbol name and the plt will be labelled with the ifunc's symbol name.

> Would be interesting to see what the dynamic loader will report to the shared library in that case.

Since I change the symbol type to STT_FUNC it should just be handled like a regular symbol, which means that the shared library will see the address of the plt. This is what I observed with your repro in PR40501 if I change it to take the address of fff in code rather than in an initializer.

> I note that ld.bfd gives an error message to recompile with PIC if the symbol is address taken using a non-got generating reference and exported; I think it would be worth following suit and giving an error.

That might just be because it's difficult to teach ld.bfd to redirect a dynamic symbol. We handle that case just fine so I'm inclined not to do that unless there is a need to.

================
Comment at: lld/ELF/Relocations.cpp:1051
+      //
+      // FIXME: We don't always need the indirection here. Absolute non-PLT
+      // non-GOT relocations could be turned into an IRELATIVE.
----------------
peter.smith wrote:
> One advantage of the indirection is that we don't end up with a dynamic (ish) relocation such as IRELATIVE in the .text section for the case:
> 
> ```
> extern void f2();
> void f() {
>    void (*fp)(void) = fff; 
> }
> ```
In my implementation I've added a check that the section is writable before producing the IRELATIVE. This is similar to the check before producing a RELATIVE near the top of processRelocAux.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D57371