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

Wed Feb 13 13:44:47 PST 2019

pcc marked an inline comment as done.
pcc added a comment.

In D57371#1396776 <https://reviews.llvm.org/D57371#1396776>, @ruiu wrote:

> LGTM
>
> Even though I believe what this patch does is correct, it seems really complicated to me, but that's not a fault of this patch. This part of lld code is mind-boggling. At this point, it is perhaps easier to rewrite it entirely than incrementally improving the code quality of this file. I don't think I would do this soon, but that's probably what we should keep in mind.

Agreed that this needs a rewrite at some point. My biggest concern was that this patch causes relocations to be processed using different code paths depending on the order in which they are encountered (hence why the test checks both orders); fixing that will probably require a fundamental change here.

================
Comment at: lld/ELF/Relocations.cpp:1209
+    else if (Config->Pic)
+      addRelativeReloc(R.Sec, R.Offset, R.Sym, 0, R_ABS, R.Sym->Type);
+    else
----------------
Reading through the patch again I noticed that this ought to be `R.Type` not `R.Sym->Type`. I'll fix it in the commit but I can't seem to construct a test case exposing the change in behaviour.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D57371