This is an archive of the discontinued LLVM Phabricator instance.

[ELF][PPC] Don't relax ifunc toc-indirect accesses to toc-relative
ClosedPublic

Authored by MaskRay on Aug 5 2019, 9:36 AM.

Details

Summary

Fixes PR42759.

// If ifunc is taken address in -fPIC code, it may have a toc entry
.section .toc,"aw",@progbits
  .quad ifunc

// ifunc may be defined as STT_GNU_IFUNC in another object file
.type ifunc, %gnu_indirect_function

If ifunc is non-preemptable (e.g. when linking an executable), the toc
entry will be relocated by R_PPC64_IRELATIVE.

R_*_IRELATIVE represents the symbolic value of a
non-preemptable ifunc (not associated with a canonical PLT) in a writable location. It has an unknown value at
link time, so we cannot apply toc-indirect to toc-relative relaxation.

Diff Detail

Repository
rL LLVM

Event Timeline

MaskRay created this revision.Aug 5 2019, 9:36 AM
Herald added a project: Restricted Project. · View Herald Transcript
MaskRay edited the summary of this revision. (Show Details)Aug 5 2019, 9:52 AM
luporl accepted this revision.Aug 5 2019, 10:02 AM

Nice, I've confirmed that it fixes PR42759 on FreeBSD.

This revision is now accepted and ready to land.Aug 5 2019, 10:02 AM
sfertile added inline comments.Aug 6 2019, 8:54 AM
ELF/Arch/PPC64.cpp
175 ↗(On Diff #213383)

minor nit: I think it would help readability if the new comment and check were added after the existing preemptible check.

176 ↗(On Diff #213383)

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

test/ELF/ppc64-toc-relax-ifunc.s
6–7 ↗(On Diff #213383)

FileCheck run-step is missing.

MaskRay updated this revision to Diff 213639.Aug 6 2019, 9:27 AM

Add a missed FileCheck

Improve a comment

MaskRay marked 4 inline comments as done.Aug 6 2019, 9:31 AM
MaskRay added inline comments.
ELF/Arch/PPC64.cpp
175 ↗(On Diff #213383)

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.

176 ↗(On Diff #213383)

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.

test/ELF/ppc64-toc-relax-ifunc.s
6–7 ↗(On Diff #213383)

Thanks! Fixed.

MaskRay marked an inline comment as done.Aug 6 2019, 9:32 AM
sfertile accepted this revision.Aug 6 2019, 9:47 AM

LGTM.

ELF/Arch/PPC64.cpp
175 ↗(On Diff #213383)

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.

176 ↗(On Diff #213383)

Thanks, I agree: your reword covers it well enough for here.

This revision was automatically updated to reflect the committed changes.