This is an archive of the discontinued LLVM Phabricator instance.

[LLD][PowerPC] Add support for R_PPC64_GOT_PCREL34
ClosedPublic

Authored by stefanp on Jun 16 2020, 9:56 AM.

Details

Summary

Add support for the 34bit relocation R_PPC64_GOT_PCREL34 for PC Relative in LLD.

Diff Detail

Event Timeline

stefanp created this revision.Jun 16 2020, 9:56 AM
sfertile added inline comments.Jun 17 2020, 11:17 AM
lld/ELF/Arch/PPC64.cpp
1017

@MaskRay comments on the other patch about lowercase hex literals applies here as well. Ditto for the suggested test change (ie labels instead of sections)

lld/test/ELF/ppc64-got-pcrel34-reloc.s
33 ↗(On Diff #271128)

It this test just to show the linker doesn't modify the load instruction that follows the PC-relative load from the got?

MaskRay added inline comments.
lld/ELF/Arch/PPC64.cpp
1017

In https://reviews.llvm.org/D81986#inline-753874 @grimar asked whether we should use const. It seems to me that is a good idea. Many other components of LLVM use const whenever appropriate.

lld/test/ELF/ppc64-got-pcrel34-reloc.s
1 ↗(On Diff #271128)

The naming is ppc64-reloc-* (x86-64-reloc- tests stick with this rule more consistently)

18 ↗(On Diff #271128)

Consider using a label instead of a .section as the anchor in the output. This allows you to use -NEXT: which makes tests easier to update when something goes off.

# CHECK:      <foo>:
# CHECK-NEXT:   pld 3, ...   # I tend to indent instructions to make it clear the region is covered by the previous label

Omit unneeded addresses unless you do some arithmetic and somewhere else in the test requires exact addresses. In that case, you should write the formula in a comment (example: ppc64-tls-*.s)

33 ↗(On Diff #271128)

If you want to check not TOC (a variant of GOT) optimization is performed, use -no-pie (default) or -pie, instead of -shared.

44 ↗(On Diff #271128)

This creates a large file. Use a linker script like ppc32-long-thunk.s

stefanp marked an inline comment as done.Jun 18 2020, 4:16 AM
stefanp added inline comments.
lld/test/ELF/ppc64-got-pcrel34-reloc.s
33 ↗(On Diff #271128)

Yes, this test is just to make sure that we don't make changes to the lwa. It may not be necessary from that perspective because that instruction does not have a relocation on it so there really is no risk of this problem. However, in my head they are the part of the same set so I want to check them both.

I'm not trying to check for not TOC. We have removed all of the TOC references on the compiler side. Or, maybe I didn't understand your comment...

stefanp updated this revision to Diff 271656.Jun 18 2020, 4:17 AM

Addressed review comments.

MaskRay accepted this revision.Jun 18 2020, 12:57 PM

LGTM, with one last nit. Please wait for @sfertile's approval.

lld/test/ELF/ppc64-reloc-got-pcrel34.s
28

It seems strange that lwa is indented differently. Please fix it.

This revision is now accepted and ready to land.Jun 18 2020, 12:57 PM
sfertile accepted this revision as: sfertile.Jun 19 2020, 10:07 AM

LGTM.

lld/ELF/Arch/PPC64.cpp
1017

Big +1 to judiciously adding const wherever appropriate.

lld/test/ELF/ppc64-got-pcrel34-reloc.s
33 ↗(On Diff #271128)

If it was to test we don't try to perform toc-optimizations then I was going to say you need to make it an executable like MaskRay suggested. But since its to make sure we don't modify past the end of the prefixed instructions its good the way you have it, this test won't change when you do go to implement the got optimizations which is how we want it I think 👍

lld/test/ELF/ppc64-reloc-got-pcrel34.s
18

If the 'SYMBOL' checks are to verify the referenced symbols make it into the dynamic symbol table maybe have a first check line:

SYMBOL: Symbol table '.dynsym' contains 4 entries:

stefanp updated this revision to Diff 272791.Jun 23 2020, 12:11 PM

Updated with last nits.

This revision was automatically updated to reflect the committed changes.