Page MenuHomePhabricator

[PowerPC] Implement R_PPC64_REL24_NOTOC local calls, callee also has no TOC
ClosedPublic

Authored by NeHuang on Jun 29 2020, 3:25 PM.

Details

Summary

The PC Relative code allows for calls that are marked with the relocation R_PPC64_REL24_NOTOC. This indicates that the caller does not have a valid TOC pointer in R2 and does not require R2 to be restored after the call.
This patch is added to support local calls to callees that also do not have a TOC and will cover the following situations:

  • Local call in same compilation unit, callee with st_other=0
  • Local call in same compilation unit, callee with st_other=1
  • Extern call that is DSO local, callee has st_other=0
  • Extern call that is DSO local, callee has st_other=1

Diff Detail

Event Timeline

NeHuang created this revision.Jun 29 2020, 3:25 PM
NeHuang retitled this revision from [LLD][PowerPC] Implement R_PPC64_REL24_NOTOC calls, callee also has no TOC. to [LLD][PowerPC] Implement R_PPC64_REL24_NOTOC calls, callee also has no TOC.Jun 30 2020, 1:15 PM

Thanks for the patch. I haven't looked into the detail closely. Some suggestions about the test inlined.

lld/test/ELF/ppc64-reloc-rel24notoc-externcall-samedso-callee-st-other0.s
3 ↗(On Diff #274257)

How about 0x10000000 or 0x10010000?

2 space indentation is sufficient.

8 ↗(On Diff #274257)

Extra space after .globl

29 ↗(On Diff #274257)

Add -NEXT: if applicable

33 ↗(On Diff #274257)
# CHECK-LABEL: <callee>:
# CHECK:         10010180:       bl 0x10010158

The idea is that functions should use -LABEL: and its covered instructions can be indented by two columns (to make it clear it is in the function). The next line

46 ↗(On Diff #274257)

8 space indentation is too wasteful. 2 is sufficient.

49 ↗(On Diff #274257)

Remove unneeded instructions to make the tested part stand out.

NeHuang updated this revision to Diff 274889.Jul 1 2020, 12:16 PM

Thanks @MaskRay for reviewing this patch!

I have addressed your comments for ppc64-reloc-rel24notoc-externcall-samedso-callee-st-other0.s Also updated the other three cases accordingly.

NeHuang marked 6 inline comments as done.Jul 1 2020, 12:16 PM
stefanp accepted this revision.Jul 1 2020, 1:07 PM

I had a minor nit.
Otherwise I think this is fine.
LGTM.

However, please wait for approval from sfertile or MaskRay before committing.

lld/ELF/Arch/PPC64.cpp
1039

nit:
Format code as clang-format says makes sense to me.

This revision is now accepted and ready to land.Jul 1 2020, 1:07 PM

Patch looks very good Victor. I've left a couple minor comments in the implementation. I have a few suggestions in relation to the testing.

You have done a very thorough job adding tests. Ideally I think we should be able to condense this down to 2 lit tests with 2 input files.

Just to make sure I understand the extent of what we want to test in this patch:

  • All calls that use the new NOTOC call relocation that do not need thunks/stubs.
  • We want to test both calls and tail calls.
  • Have callee's with both st_other=0 and st_other=1.

I think we can handle that by having one lit test that we link into a executable. You can define what you are calling the local callees (with both st_other=1 and st_other=0) in that file as well as the callers, and then define what you are calling the externalcalll-samedso in an assembly file in /Inputs/ (we already have a handful of ppc64-* files in there).

I would then have a different lit test which we link into a shared object, where the callees have hidden visibility so thjat we do not need a plt stub to call them (similarly to above define callees in the same file, and other callees in /Input/)

lld/ELF/Arch/PPC64.cpp
1041–1046

We should probably insert a couple of fatal error here:

  1. if the type in NOTOC and the symbols st_other indicates it needs the toc-pointer setup.
  2. If the type is not NOTOC but the symbols st_other indicates it tramples the toc.
lld/ELF/Thunks.cpp
950 ↗(On Diff #274889)

Insert a fatal error here if the type is R_PPC64_REL24_NOTOC with a message indicating notoc thunks are not yet supported. (With the other fatal_errors I suggested I think only a plt-stub could cause this).

lld/test/ELF/ppc64-reloc-rel24notoc-externcall-samedso-callee-st-other0.s
29 ↗(On Diff #274889)

This is a little odd. We define 'callee' with default visibility, then reference it in another object file with hidden visibility, but we are linking an exec so the visibility doesn't really affect the behavior.

NeHuang updated this revision to Diff 275896.Jul 6 2020, 8:50 PM

Thanks @sfertile and @stefanp for the review!

To Sean's question on the test coverage in this patch:

Yes, your understand is correct.

Addressed the review comments:

  • Clang-format line 1039 in lld/ELF/Arch/PPC64.cpp
  • Inserted fatal errors for the unsupported call protocols and notoc thunks.
  • Condense the 4 lit cases into 1 lit test with an input file.
  • Added another lit test with an input file for callee with hidden visibility.
NeHuang marked 3 inline comments as done.Jul 6 2020, 8:52 PM
MaskRay added inline comments.Jul 6 2020, 9:34 PM
lld/ELF/Arch/PPC64.cpp
1043

Note: assert should only be used for logically unreachable code, i.e. if the implementation is not buggy, the negative code path should not trigger.

You can use fatal(...) for unimplemented features. Please use all lowercase messages.

MaskRay added inline comments.Jul 6 2020, 9:38 PM
lld/test/ELF/ppc64-pcrel-call-to-pcrel-callee-hidden.s
22 ↗(On Diff #275896)

Align 1:

lld/test/ELF/ppc64-pcrel-call-to-pcrel.s
21

One empty line is sufficient.

23

Indent the first line

41

Make addresses indented.

43

caller1 is not a good FileCheck label.

<caller1>: is. It is unique in the llvm-objdump output.

Please fix all the occurrences.

MaskRay added inline comments.Jul 6 2020, 10:56 PM
lld/test/ELF/ppc64-pcrel-call-to-pcrel-callee-hidden.s
111 ↗(On Diff #275896)

IIUC, you can merge caller2 and caller2_tailcall and delete all instructions except:

.localentry caller2_tailcall, 1
bl callee2_stother1_local@notoc
b callee2_stother1_local@notoc
lld/test/ELF/ppc64-pcrel-call-to-pcrel.s
53

I think these instructions are entirely irrelevant to the test and should be deleted to make tests more focused/readable.

mullw 3, 3, 3
ext3sw 3,3

Please check some newer x86-64-* and aarch64-* tests. They don't have such setup instructions.

But please keep the instruction after bl ...@notoc to make it clear that the next instruction is not special as in R_PPC64_REL24

I think cleaning up the instructions can make the test smaller by at least one half.

MaskRay requested changes to this revision.Jul 6 2020, 10:56 PM
This revision now requires changes to proceed.Jul 6 2020, 10:56 PM
MaskRay added inline comments.Jul 6 2020, 10:57 PM
lld/test/ELF/ppc64-pcrel-call-to-pcrel.s
154

ppc64-pcrel-call-to-pcrel-callee-hidden.s changes the symbol bindings to STB_GLOBAL.
Do these symbols need .globl?

NeHuang updated this revision to Diff 276241.Jul 7 2020, 3:53 PM
NeHuang marked 8 inline comments as done.
NeHuang marked 5 inline comments as done and an inline comment as not done.Jul 7 2020, 4:02 PM

Thanks @MaskRay.

  • Inserted fatal() for unsupported features.
  • Cleaned up test to keep necessary instructions and combine cases.
  • Fixed some nits in comment.
lld/ELF/Arch/PPC64.cpp
1043

Added fatal(). I am using lower cases for the messages except the relocation name.

lld/test/ELF/ppc64-pcrel-call-to-pcrel-callee-hidden.s
111 ↗(On Diff #275896)

Good point. Merged them for all the test.

lld/test/ELF/ppc64-pcrel-call-to-pcrel.s
43

Added the fix for all the occurrences.

53

Thanks for the advice. Only keep necessary instructions to make the test smaller.

154

Good catch. They suppose to be .local by default. Updated ppc64-pcrel-call-to-pcrel-callee-hidden.s to make them consistent.

NeHuang updated this revision to Diff 276305.Jul 7 2020, 8:36 PM
NeHuang marked an inline comment as not done.
  • Keep only one lit test with one input file to cover all the cases:
    1. local linkage callee with st_other=0
    2. local linkage callee with st_other=1
    3. global linkage callee with st_other=0
    4. global linkage callee with st_other=1
  • Put the check of all three unimplemented protocols in PPC64::needsThunk
NeHuang marked an inline comment as done.Jul 7 2020, 8:37 PM
NeHuang marked an inline comment as done.Jul 7 2020, 8:44 PM
NeHuang added inline comments.
lld/ELF/Arch/PPC64.cpp
1041–1046

Thanks Sean for the advice. I also moved the fatal error check for the protocol "external call with R_PPC64_REL_NOTOC" here so that we are checking all unimplemented protocols in the same function.

I think we are really close now. I would suggest having a second lit test, which we link into either a static exec or pie exec which has calls using the R_PPC64_REL24_NOTOC relocation to symbols with global linkage and default visibility. I think it can be done with a single file (ie define all the callers and callees in the same file, no need for a second input file).

NeHuang updated this revision to Diff 276505.Jul 8 2020, 11:55 AM

Thanks Sean. Added a new lit test to check for global linkage and default visibility callee.

One thought. If you want to merge two test files,

ifdef global
.globl foo
.endif
foo:
  1. llvm-mc -filetype=obj
  2. llvm-mc -filetype=obj --defsym global=1
lld/test/ELF/ppc64-pcrel-call-to-pcrel.s
46

These add 3, 3, 30 & mullw 3, 3, 30 instructions are not needed.

Probably worth adding a comment before the first bl explaining that the next instruction does not need to be nop as in the R_PPC64_REL24 case.

NeHuang marked an inline comment as done.Jul 8 2020, 12:38 PM
NeHuang added inline comments.
lld/test/ELF/ppc64-pcrel-call-to-pcrel.s
46

Just want to confirm your suggestion is to remove the instruction after bl ..@notoc and add a comment for the R_PPC64_REL24_NOT cases nop is not needed. In this sense, we will only keep blr, bl and b instructions in the test case.

NeHuang marked an inline comment as not done.Jul 8 2020, 12:39 PM
NeHuang updated this revision to Diff 276572.Jul 8 2020, 3:08 PM
  • Merged two lit tests.
  • Removed the instruction after bl ..@noc and added comments..
NeHuang marked 2 inline comments as done.Jul 8 2020, 3:09 PM
MaskRay accepted this revision.Jul 8 2020, 3:16 PM

LGTM. Hope @sfertile can confirm.

lld/test/ELF/ppc64-pcrel-call-to-pcrel.s
61

We use ## for comments.

This revision is now accepted and ready to land.Jul 8 2020, 3:16 PM
sfertile accepted this revision.Jul 9 2020, 10:36 AM

One comment but otherwise LGTM.

lld/ELF/Arch/PPC64.cpp
1041–1046

👍

lld/test/ELF/ppc64-pcrel-call-to-pcrel.s
14

Nit: I suggest having a separate set of run steps for testing the exec since the steps are completely disjoint from the shared object test. ie separate out

# RUN: llvm-mc -filetype=obj -triple=powerpc64[le] -defsym GLOBAL=1 %s -o %t3.o
# RUN: ld.lld -T %t.script %t3.o -o %t
# RUN: llvm-readelf -s %t | FileCheck %s --check-prefix=SYMBOL-GLOBAL
# RUN: llvm-objdump -d --no-show-raw-insn --mcpu=pwr10 %t | FileCheck %s
NeHuang retitled this revision from [LLD][PowerPC] Implement R_PPC64_REL24_NOTOC calls, callee also has no TOC to [PowerPC] Implement R_PPC64_REL24_NOTOC local calls, callee also has no TOC.Jul 9 2020, 1:11 PM
NeHuang edited the summary of this revision. (Show Details)
This revision was automatically updated to reflect the committed changes.
NeHuang marked 5 inline comments as done.
NeHuang added inline comments.Jul 10 2020, 5:25 AM
lld/test/ELF/ppc64-pcrel-call-to-pcrel.s
14

Thanks. Addressed the nit when committing the patch.

61

Thanks. Addressed the nit when committing the patch.