This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC] Support PCRelative Callees for R_PPC64_REL24 Relocation
ClosedPublic

Authored by stefanp on Jul 1 2020, 4:58 AM.

Details

Summary

The R_PPC64_REL24 is used in function calls when the caller requires a
valid TOC pointer. If the callee shares the same TOC or does not clobber
the TOC pointer then a direct call can be made. If the callee does not
share the TOC a thunk must be added to save the TOC pointer for the caller.

Up until PC Relative was introduced all local calls on medium and large code
models were assumed to share a TOC. This is no longer the case because
if the caller requires a TOC and the callee is PC Relative then the callee
can clobber the TOC even if it is in the same DSO.

This patch is to add support for a TOC caller calling a PC Relative callee that
clobbers the TOC.

Diff Detail

Unit TestsFailed

Event Timeline

stefanp created this revision.Jul 1 2020, 4:58 AM
stefanp updated this revision to Diff 274883.Jul 1 2020, 11:52 AM

Updated initial test case to clean up some of the instruction.
Added a second test to check that an error is reported when the nop after the
call is missing.

anil9 added a subscriber: anil9.Jul 2 2020, 6:42 PM
anil9 added inline comments.
lld/ELF/Thunks.cpp
284

guaranteee - > guarantee

285

need the save -> need to save

286

This stub does: -> This stub:

287

Branch -> Branches/Jumps

sfertile added inline comments.Jul 3 2020, 7:20 AM
lld/ELF/Thunks.cpp
287

Alternatively: 2) Tails calls the callee

842

What happens if offset doesn't fit within 26 bits?

846

This is being named the same as a branch extending thunk (ie a trampoline for when the call is too far to represent with a single call instruction). The name we create shoud represent the thunk type, it makes reading disassembly much easier. How about "__toc_save_` instead?

lld/test/ELF/ppc64-error-toc-local-call.s
37

nit: remove the compiler generated comments.

lld/test/ELF/ppc64-toc-call-to-pcrel.s
62

nit: remove extra blank line.

stefanp updated this revision to Diff 276164.Jul 7 2020, 11:47 AM
stefanp marked an inline comment as done.

Fixed some nits in comments.
Added a fatal error and a test case case to cover the situation where the offset
for the branch is more than 26 bits.

lld/ELF/Thunks.cpp
842

Good point.
Right now we just drop the extra bits from the offset and jump to an incorrect offset.

I'll add an error to fail at this point if the offset does not fit in the 26 bits.

846

The reason I had used this name was because GCC used long_branch.callee as the name as well so I figured I would do the same thing. However, I do like the idea of calling it __toc_save better so I'm going to use that instead.

MaskRay added inline comments.Jul 7 2020, 4:02 PM
lld/ELF/Thunks.cpp
843

Drop trailing period. http://llvm.org/docs/CodingStandards.html#error-and-warning-messages

"R2 save stub branch offset is too large: " + Twine(offset)

lld/test/ELF/ppc64-error-toc-local-call.s
16

Delete addi 3, 3, 5 and extsw. They are irrelevant.

26

Delete irrelevant mr and add

stefanp updated this revision to Diff 276281.Jul 7 2020, 5:45 PM

Updated text for error message.
Test cleanup be deleting lines that are not needed.

stefanp updated this revision to Diff 276284.Jul 7 2020, 5:49 PM

Cleaned up third test as well.

MaskRay added inline comments.Jul 7 2020, 7:01 PM
lld/ELF/Thunks.cpp
979

This needs a comment.

lld/test/ELF/ppc64-error-toc-local-call.s
8

Use ## for comments. Newer tests conform to this rule.

lld/test/ELF/ppc64-toc-call-to-pcrel-long-jump.s
29 ↗(On Diff #276284)

add 3, 4, 3 is irrelevant and can be deleted.

35 ↗(On Diff #276284)

.size can be deleted

stefanp updated this revision to Diff 276373.Jul 8 2020, 4:28 AM
stefanp marked an inline comment as done.

Fixed a couple of conditions and added comments for them.
Added a test for the new condition.
Cleaned up existing tests.

lld/ELF/Thunks.cpp
979

I've added a comment.
Adding the comment forced me to re-assess the condition statement so I've also changed the condition to be just:

if ((s.stOther >> 5) == 1)

There was no reason to exclude R_PPC64_REL14 from this condition.
I have also added a test for R_PPC64_REL14.

sfertile accepted this revision.Jul 8 2020, 8:52 AM
sfertile marked an inline comment as done.

LGTM.

lld/ELF/Thunks.cpp
979

👍

This revision is now accepted and ready to land.Jul 8 2020, 8:52 AM
This revision was automatically updated to reflect the committed changes.