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

Peter Smith via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Jan 29 06:31:54 PST 2019


peter.smith added a comment.

Thanks for sharing the WIP. I think the approach will work and should match the behaviour of ld.bfd more closely. I've been trying to think of alternatives to redirecting all the symbols for the non GOT generating case. Off the top of my head:

- Keep the _PLT RelExpr variants and instead of changing the symbol alter the Expr for the non-plt non-got generating reference. This has the disadvantage of needing _PLT variants for any RelType that might fall into that category.
- Still create a new symbol for the non-plt, non-got generating reference, but instead of replacing the existing symbol, just change Sym for the affected relocations, leaving the others untouched. I think this would mean you wouldn't need to create DirectSym but it would mean having to store the symbol somewhere so that subsequent non-plt, non-got generating references could use it.

Not sure if either of those are better at this point though.



================
Comment at: lld/ELF/InputSection.cpp:613
     return Sym.getVA(A) - getARMStaticBase(Sym);
   case R_GOT:
   case R_RELAX_TLS_GD_TO_IE_ABS:
----------------
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. 


================
Comment at: lld/ELF/Relocations.cpp:1029
 
-  // If a relocation needs PLT, we create PLT and GOTPLT slots for the symbol.
-  if (needsPlt(Expr) && !Sym.isInPlt()) {
-    if (Sym.isGnuIFunc() && !Sym.IsPreemptible)
+  if (Sym.isGnuIFunc() && !Sym.IsPreemptible) {
+    // Unlike GOT references to regular symbols, GOT references to
----------------
I'd be tempted to reverse the condition to put the non-ifunc or preemptible case first; it is much simpler and more commonly encountered.


================
Comment at: lld/ELF/Relocations.cpp:1033
+    // now. This works because dynamic loaders evaluate IRELATIVE relocs
+    // eagerly.
+    if (!Sym.isInPlt()) {
----------------
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.


================
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.
----------------
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.




================
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.
----------------
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; 
}
```


Repository:
  rL LLVM

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

https://reviews.llvm.org/D57371





More information about the llvm-commits mailing list