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

[Sean Fertile via Phabricator via llvm-commits]

sfertile accepted this revision.
sfertile added a comment.

LGTM.



================
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,
----------------
MaskRay wrote:
> 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.
Sure. My thoughts were that the Ifunc case is a special case of a defined symbol not being relaxable, splitting the code/comment out separately highlights that. I'm Ok with it the way it is as well.


================
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.
----------------
MaskRay wrote:
> 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.
Thanks, I agree: your reword covers it well enough for here.


Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D65755