[PATCH] D65995: [ELF] Don't special case symbolic relocations with 0 addend to ifunc in writable locations

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

> If I've understood correctly, this will make the case "address of an ifunc is taken from RW but there are no other relocations that would create a canonical PLT entry" worse as we would always create the canonical PLT entry even if it isn't strictly needed. The trade off is simpler code and a possible size saving from only needing one irelative relocation?

We have probably overloaded the meaning of "canonical PLT" here:) It is created for pointer equality, but it can have different purposes.

1. STT_FUNC, undefined or defined in a DSO. The canonical PLT exposes one more symbol to the executable, which can preempt a real definition in a DSO. This can be seen as address leak.
2. STT_IFUNC, defined in the executable, referenced by a non-PLT-generating-non-GOT-generating relocation. The canonical PLT doesn't add a new entry to the static/dynamic symbol tables. If the STT_GNU_IFUNC symbol was exposed before, the converted STT_FUNC is still exposed. If the STT_GNU_IFUNC was hidden, the new STT_FUNC is still hidden.



================
Comment at: ELF/Relocations.cpp:1095
 
 struct IRelativeReloc {
   RelType type;
----------------
peter.smith wrote:
> I think that this is only used by code that you have deleted. Could be removed as well?
Thanks! Will delete.


================
Comment at: test/ELF/aarch64-gnu-ifunc-nonpreemptable2.s:1
+# REQUIRES: aarch64
+# RUN: llvm-mc -filetype=obj -triple=aarch64-none-linux-gnu %s -o %t.o
----------------
peter.smith wrote:
> Assuming I've not made a mistake somewhere, this test is passing with lld prior to this change. Is this intentional?
> 
> Possibly the reference from RO is causing the canonical PLT entry even with the previous LLD?
Yes, the reference from .rodata creates the canonical PLT. This test is to increase test coverage. It passes prior to this change.

If I delete .data, there is a canonical PLT.

If I delete .rodata, there is no canonical PLT, but there are 2 R_*_IRELATIVE. The R_*_IRELATIVE relocating .got.plt is redundant. We can move the `if (expr == R_ABS ...` code before `if (!sym.isInPlt())` to decrease one. That was what I experimented before I thought: we should probably just simplify the cases.




Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D65995