[PATCH] D65755: [ELF][PPC] Don't relax ifunc toc-indirect accesses to toc-relative

[Fangrui Song via Phabricator via llvm-commits]

MaskRay marked 4 inline comments as done.
MaskRay added inline comments.


================
Comment at: ELF/Arch/PPC64.cpp:175
   // Only non-preemptable defined symbols can be relaxed.
-  if (!d || d->isPreemptible)
+  //
+  // The toc entry of a non-preemptable ifunc is relocated by R_PPC64_IRELATIVE,
----------------
sfertile wrote:
> minor nit:  I think it would help readability if the new comment and check were added after the existing preemptible check.
I think it is probably fine to have them on one line.. The first sentence makes it clear that preemptable and non-Defined symbols are excluded.

> Only non-preemptable defined symbols can be relaxed.

The ifunc comment discusses the possible IRELATIVE case.


================
Comment at: ELF/Arch/PPC64.cpp:176
+  //
+  // The toc entry of a non-preemptable ifunc is relocated by R_PPC64_IRELATIVE,
+  // not representable as a toc-relative value. Exclude ifunc.
----------------
sfertile wrote:
> It might be better to spell out why the address is not representable as a toc-relative value. IIUC: The symbol being referenced is a resolver function that the dynamic loader will run to determine what function should be used. Relaxing to a toc-relative reference will manifest the address of the resolver function as opposed to the address of the resolved function (which is not know until load time).
We have more detailed comment at Relocations.cpp:1231. I reworded the comment here but I think we probably don't have to delve too much into the details.


================
Comment at: test/ELF/ppc64-toc-relax-ifunc.s:6-7
+# RUN:   llvm-mc -filetype=obj -triple=powerpc64le - -o %t1.o
+# RUN: ld.lld %t.o %t1.o -o %t
+
+## ifunc is a non-preemptable STT_GNU_IFUNC. Its toc entry will be
----------------
sfertile wrote:
> FileCheck run-step is missing.
Thanks! Fixed.


Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D65755