This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC] Support for R_PPC64_REL24_NOTOC calls where the caller has no TOC and the callee is not DSO local
ClosedPublic

Authored by NeHuang on Jul 13 2020, 4:39 AM.

Details

Summary

This patch supports the situation where caller does not have a valid TOC and
calls using the R_PPC64_REL24_NOTOC relocation and the callee is not DSO local.
In this case the call cannot be made directly since the callee may or may not
require a valid TOC pointer. As a result this situation requires an R12 setup
plt stub.

Diff Detail

Event Timeline

stefanp created this revision.Jul 13 2020, 4:39 AM
NeHuang added inline comments.
lld/ELF/Arch/PPC64.cpp
1037–1038

nit: remove this comment

lld/ELF/Thunks.cpp
1041

Shall we merge it with the if check below if it does not hurt the readability?

if (s.isInPlt())
  return (type == R_PPC64_REL24_NOTOC) ?  make<PPC64R12SetupStub>(s) : make<PPC64PltCallStub>(s);
sfertile added inline comments.Jul 13 2020, 1:20 PM
lld/ELF/Thunks.cpp
1042

I'll need to check what instruction we are using in the R12SetupStub, I see the comment says pld right now, but where is it loading from?
destination.getVA() - getThunkTargetSym()->getVA() is the offset from the program counter to the (non-premptable) definition so I'm guessing its meant to be a paddi instead. Either way, that offset calculation is wrong for a symbol in the plt. Instead we want to load the address of the function from the plt.

lld/test/ELF/ppc64-pcrel-call-to-extern.s
48

Whats the offset that gets encoded into the instruction? What offset are you expecting?

NeHuang commandeered this revision.Jul 16 2020, 8:28 AM
NeHuang added a reviewer: stefanp.
NeHuang updated this revision to Diff 278499.Jul 16 2020, 8:37 AM
  • Added a new plt gep setup stub.
  • Updated the test case and checks if the offset func@plt@pcrel properly computed.
  • Re-based the patch with ToT master and merged in latest changes in https://reviews.llvm.org/D83504
NeHuang marked 6 inline comments as done.Jul 16 2020, 8:44 AM
NeHuang added inline comments.
lld/ELF/Thunks.cpp
1042

Thanks Sean.

  • Updated the patch to load the address of function from the plt.
  • The offset is computed by destination.getGotPltVA() - getThunkTargetSym()->getVA(); instead.
lld/test/ELF/ppc64-pcrel-call-to-extern.s
48
  • The offset computed is from the program counter to the plt entry address storing the address of the callee function.
  • Updated the test case to check if the offset properly computed.
NeHuang marked 2 inline comments as done.Jul 16 2020, 8:49 AM
NeHuang updated this revision to Diff 278635.Jul 16 2020, 5:03 PM

Updated instruction encode computation for pld

sfertile added inline comments.Jul 20 2020, 7:46 AM
lld/ELF/Arch/PPC64.cpp
1037–1038

No need for a extra case to handle the NOTOC plt calls, the following case handles it perfectly.

lld/ELF/Thunks.cpp
308

If you look at the existing plt stub it also sets up R12, this is expected of any plt stub variety on PPC64. The important details about this stub type is that it is a plt stub that uses pc-relative instructions, that it sets up R12 is just an implementation detail.

// PPC64 PLT R12 Setup Stub

-->

// PPC64 PC-relative PLT Stub
315

Change the name to PPC64PCRelPLTStub.

911

Both plt stub types perform the setup for the gep so the name doesn't really disambiguate which stub type we have. I suggest trying to use pc-rel in the name instead of gep.

NeHuang updated this revision to Diff 279321.Jul 20 2020, 12:15 PM
NeHuang marked 4 inline comments as done.

Thanks Sean.

  • Removed the redundant check in needsThunk
  • Renamed the new stub as suggested, updated comments and the test case accordingly.
stefanp accepted this revision as: stefanp.Jul 23 2020, 6:45 PM

I only had one nit.
LGTM.

lld/ELF/Thunks.cpp
911

nit:
Try to use __plt_pcrel_ instead of __plt_pc-rel_.
The - is probably fine as part of a function name but since I'm not 100% sure I would not risk it.

This revision is now accepted and ready to land.Jul 23 2020, 6:45 PM
MaskRay added inline comments.Jul 23 2020, 7:17 PM
lld/ELF/Thunks.cpp
1042–1043

Remove unneeded parens.

lld/test/ELF/Inputs/ppc64-callee-global.s
1 ↗(On Diff #279321)

Use extract?

or llvm-mc --defsym AUX=1 and

.ifdef AUX
...
.else
...
.endif
lld/test/ELF/ppc64-pcrel-call-to-extern.s
27

-NEXT:

sfertile added inline comments.Jul 24 2020, 7:00 AM
lld/ELF/Thunks.cpp
911

+1. Stefan's suggestions in more consistent with the other stub names.

1043

Are the casts needed?

NeHuang updated this revision to Diff 280506.Jul 24 2020, 9:58 AM
NeHuang marked 6 inline comments as done.

Thank you all for the review!

  • Addressed the nits and update the test case.
  • Combined ppc64-callee-global.s with ppc64-pcrel-call-to-extern.s using extract.
NeHuang added inline comments.Jul 24 2020, 10:10 AM
lld/ELF/Thunks.cpp
1043

Yeah, the ternary conditional operator would complain since make<PPC64PCRelPLTStub>(s) and make<PPC64PltCallStub>(s) has different value type.

NeHuang updated this revision to Diff 281374.EditedJul 28 2020, 3:09 PM

As the patch https://reviews.llvm.org/D83834 is reverted, updated the test case accordingly to combine ppc64-callee-global.s and ppc64-pcrel-call-to-extern.s with .ifdef AUX

sfertile added inline comments.Jul 29 2020, 4:59 AM
lld/ELF/Thunks.cpp
1042–1043

nit: clang-format

1043

Thanks.

lld/test/ELF/ppc64-pcrel-call-to-extern.s
31

minor nit: add whitepace so it lines up with the following lines

36

Can you add spaces to the lines that are missing '-NEXT' to keep the columns aligned.

51

I believe plt[1] is the dynamic object identifier. Maybe its best to say the first 2 entries in the plt are reserved for the dynamic linkers usage and skip mentioning what they are used for.

NeHuang updated this revision to Diff 281532.EditedJul 29 2020, 5:30 AM
NeHuang marked 5 inline comments as done.
NeHuang added a subscriber: Sean.

Thanks Sean for the review. Addressed the nits for clang-format and alignment issues.

NeHuang removed a subscriber: Sean.Jul 29 2020, 5:30 AM
sfertile accepted this revision as: sfertile.Jul 29 2020, 9:47 AM

LGTM.

This revision was landed with ongoing or failed builds.Jul 29 2020, 12:51 PM
This revision was automatically updated to reflect the committed changes.