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

[NeHuang]

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

[MaskRay]

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]

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.

[stefanp]

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.

[sfertile]

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]

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.

[MaskRay]

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]

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]

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]

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]

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]

• 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]

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.

[sfertile]

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]

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

[MaskRay]

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]

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]

• Merged two lit tests.
• Removed the instruction after bl ..@noc and added comments..

[MaskRay]

LGTM. Hope @sfertile can confirm.

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

61

We use ## for comments.

[sfertile]

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]

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.

lld/ELF/Arch/PPC64.cpp

RelExpr PPC64::getRelExpr(RelType type, const Symbol &s,
   case R_PPC64_TOC16_HA:
   case R_PPC64_TOC16_LO_DS:
     return config->tocOptimize ? R_PPC64_RELAX_TOC : R_GOTREL;
   case R_PPC64_TOC:
     return R_PPC64_TOCBASE;
   case R_PPC64_REL14:
   case R_PPC64_REL24:
     return R_PPC64_CALL_PLT;
+  case R_PPC64_REL24_NOTOC:
+    return R_PLT_PC;
   case R_PPC64_REL16_LO:
   case R_PPC64_REL16_HA:
   case R_PPC64_REL16_HI:
   case R_PPC64_REL32:
   case R_PPC64_REL64:
   case R_PPC64_PCREL34:
     return R_PC;
   case R_PPC64_GOT_TLSGD16:
case R_PPC64_TOC:
     break;
   case R_PPC64_REL14: {
     uint32_t mask = 0x0000FFFC;
     checkInt(loc, val, 16, rel);
     checkAlignment(loc, val, 4, rel);
     write32(loc, (read32(loc) & ~mask) | (val & mask));
     break;
   }
-  case R_PPC64_REL24: {
+  case R_PPC64_REL24:
+  case R_PPC64_REL24_NOTOC: {
     uint32_t mask = 0x03FFFFFC;
     checkInt(loc, val, 26, rel);
     checkAlignment(loc, val, 4, rel);
     write32(loc, (read32(loc) & ~mask) | (val & mask));
     break;
   }
   case R_PPC64_DTPREL64:
     write64(loc, val - dynamicThreadPointerOffset);
void PPC64::relocate(uint8_t *loc, const Relocation &rel, uint64_t val) const {
   }
   default:
     llvm_unreachable("unknown relocation");
   }
 }
 
 bool PPC64::needsThunk(RelExpr expr, RelType type, const InputFile *file,
                        uint64_t branchAddr, const Symbol &s, int64_t a) const {
-  if (type != R_PPC64_REL14 && type != R_PPC64_REL24)
+  if (type != R_PPC64_REL14 && type != R_PPC64_REL24 &&
+      type != R_PPC64_REL24_NOTOC)
     return false;
 
+  // FIXME: Remove the fatal error once the call protocol is implemented.
+  if (type == R_PPC64_REL24_NOTOC && s.isInPlt())
+    fatal("unimplemented feature: external function call with the reltype"
+          " R_PPC64_REL24_NOTOC");
+
   // If a function is in the Plt it needs to be called with a call-stub.
   if (s.isInPlt())
     return true;
 
-  // This check looks at the st_other bits of the callee. If the value is 1
-  // then the callee clobbers the TOC and we need an R2 save stub.
-  if ((s.stOther >> 5) == 1)
+  // FIXME: Remove the fatal error once the call protocol is implemented.
+  if (type == R_PPC64_REL24_NOTOC && (s.stOther >> 5) > 1)
+    fatal("unimplemented feature: local function call with the reltype"
+          " R_PPC64_REL24_NOTOC and the callee needs toc-pointer setup");
+
+  // This check looks at the st_other bits of the callee with relocation
+  // R_PPC64_REL14 or R_PPC64_REL24. If the value is 1, then the callee
+  // clobbers the TOC and we need an R2 save stub.
+  if (type != R_PPC64_REL24_NOTOC && (s.stOther >> 5) == 1)
     return true;
 
   // If a symbol is a weak undefined and we are compiling an executable
   // it doesn't need a range-extending thunk since it can't be called.
   if (s.isUndefWeak() && !config->shared)
     return false;
 
   // If the offset exceeds the range of the branch type then it will need
uint32_t PPC64::getThunkSectionSpacing() const {
   // 0x2000000 = (1 << 24-1) * 4
   return 0x2000000;
 }
 
 bool PPC64::inBranchRange(RelType type, uint64_t src, uint64_t dst) const {
   int64_t offset = dst - src;
   if (type == R_PPC64_REL14)
     return isInt<16>(offset);
-  if (type == R_PPC64_REL24)
+  if (type == R_PPC64_REL24 || type == R_PPC64_REL24_NOTOC)
     return isInt<26>(offset);
   llvm_unreachable("unsupported relocation type used in branch");
 }
 
 RelExpr PPC64::adjustRelaxExpr(RelType type, const uint8_t *data,
                                RelExpr expr) const {
   if (expr == R_RELAX_TLS_GD_TO_IE)
     return R_RELAX_TLS_GD_TO_IE_GOT_OFF;

lld/test/ELF/Inputs/ppc64-callee-global-hidden.s

+func_extern:
+  blr
+
+.hidden callee3_stother0_hidden
+.globl callee3_stother0_hidden
+callee3_stother0_hidden:
+  blr
+
+.hidden callee4_stother1_hidden
+.globl callee4_stother1_hidden
+callee4_stother1_hidden:
+  .localentry callee4_stother1_hidden, 1
+  ## nop is not needed after bl for R_PPC64_REL24_NOTOC
+  bl func_extern@notoc
+  blr

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

+# REQUIRES: ppc
+# RUN: echo 'SECTIONS { \
+# RUN:   .text_default_stother0 0x10010000: { *(.text_default_stother0) } \
+# RUN:   .text_default_stother1 0x10020000: { *(.text_default_stother1) } \
+# RUN:   .text_hidden_stother0 0x10030000: { *(.text_hidden_stother0) } \
+# RUN:   .text_hidden_stother1 0x10040000: { *(.text_hidden_stother1) } \
+# RUN:   }' > %t.script
+
+# RUN: llvm-mc -filetype=obj -triple=powerpc64le -defsym HIDDEN=1 %s -o %t1.o
+# RUN: llvm-mc -filetype=obj -triple=powerpc64le %p/Inputs/ppc64-callee-global-hidden.s -o %t2.o
+# RUN: ld.lld -T %t.script -shared %t1.o %t2.o -o %t.so
+# RUN: llvm-readelf -s %t.so | FileCheck %s --check-prefix=SYMBOL
+# RUN: llvm-objdump -d --no-show-raw-insn --mcpu=pwr10 %t.so | FileCheck --check-prefix=CHECK --check-prefix=CHECK-HIDDEN %s
+
+# RUN: llvm-mc -filetype=obj -triple=powerpc64le -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
+
+# RUN: llvm-mc -filetype=obj -triple=powerpc64 -defsym HIDDEN=1 %s -o %t1.o
+# RUN: llvm-mc -filetype=obj -triple=powerpc64 %p/Inputs/ppc64-callee-global-hidden.s -o %t2.o
+# RUN: ld.lld -T %t.script -shared %t1.o %t2.o -o %t.so
+# RUN: llvm-readelf -s %t.so | FileCheck %s --check-prefix=SYMBOL
+# RUN: llvm-objdump -d --no-show-raw-insn --mcpu=pwr10 %t.so | FileCheck --check-prefix=CHECK --check-prefix=CHECK-HIDDEN %s
+
+# RUN: llvm-mc -filetype=obj -triple=powerpc64 -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
+
+# SYMBOL:      2: 0000000010010000 0 NOTYPE LOCAL DEFAULT 5 callee1_stother0_default
+# SYMBOL-NEXT: 3: 0000000010020004 0 NOTYPE LOCAL DEFAULT [<other: 0x20>] 6 callee2_stother1_default
+# SYMBOL-NEXT: 4: 0000000010010004 0 NOTYPE LOCAL DEFAULT [<other: 0x20>] 5 caller1
+# SYMBOL-NEXT: 5: 000000001002000c 0 NOTYPE LOCAL DEFAULT [<other: 0x20>] 6 caller2
+# SYMBOL-NEXT: 6: 0000000010030000 0 NOTYPE LOCAL DEFAULT [<other: 0x20>] 7 caller3
+# SYMBOL-NEXT: 7: 0000000010040000 0 NOTYPE LOCAL DEFAULT [<other: 0x20>] 8 caller4
+# SYMBOL-NEXT: 8: 0000000010020000 0 NOTYPE LOCAL DEFAULT 6 func_local
+# SYMBOL-NEXT: 9: 0000000010040008 0 NOTYPE LOCAL DEFAULT 9 func_extern
+# SYMBOL-NEXT: 10: 000000001004000c 0 NOTYPE LOCAL HIDDEN 9 callee3_stother0_hidden
+# SYMBOL-NEXT: 11: 0000000010040010 0 NOTYPE LOCAL HIDDEN [<other: 0x22>] 9 callee4_stother1_hidden
+
+# SYMBOL-GLOBAL:      2: 0000000010010004 0 NOTYPE LOCAL DEFAULT [<other: 0x20>] 1 caller1
+# SYMBOL-GLOBAL-NEXT: 3: 000000001002000c 0 NOTYPE LOCAL DEFAULT [<other: 0x20>] 2 caller2
+# SYMBOL-GLOBAL-NEXT: 4: 0000000010020000 0 NOTYPE LOCAL DEFAULT 2 func_local
+# SYMBOL-GLOBAL-NEXT: 5: 0000000010010000 0 NOTYPE GLOBAL DEFAULT 1 callee1_stother0_default
+# SYMBOL-GLOBAL-NEXT: 6: 0000000010020004 0 NOTYPE GLOBAL DEFAULT [<other: 0x20>] 2 callee2_stother1_default
+
+# CHECK-LABEL: <callee1_stother0_default>:
+# CHECK-NEXT:  10010000: blr
+
+# CHECK-LABEL: <caller1>:
+# CHECK:       10010004: bl 0x10010000
+# CHECK-NEXT:  10010008: b 0x10010000
+.section .text_default_stother0, "ax", %progbits
+.ifdef GLOBAL
+.globl callee1_stother0_default
+.endif
+callee1_stother0_default:
+  blr
+caller1:
+  .localentry caller1, 1
+  ## nop is not needed after bl for R_PPC64_REL24_NOTOC
+  bl callee1_stother0_default@notoc
+  b callee1_stother0_default@notoc
+
+# CHECK-LABEL: <func_local>:
+# CHECK-NEXT:  10020000: blr
+
+# CHECK-LABEL: <callee2_stother1_default>:
+# CHECK-NEXT:  10020004: bl 0x10020000
+# CHECK-NEXT:  10020008: blr
+
+# CHECK-LABEL: <caller2>:
+# CHECK:       1002000c: bl 0x10020004
+# CHECK-NEXT:  10020010: b 0x10020004
+.section .text_default_stother1, "ax", %progbits
+func_local:
+  blr
+.ifdef GLOBAL
+.globl callee2_stother1_default
+.endif
+callee2_stother1_default:
+  .localentry callee2_stother1_default, 1
+  ## nop is not needed after bl for R_PPC64_REL24_NOTOC
+  bl func_local@notoc
+  blr
+caller2:
+  .localentry caller2, 1
+  ## nop is not needed after bl for R_PPC64_REL24_NOTOC
+  bl callee2_stother1_default@notoc
+  b callee2_stother1_default@notoc
+
+# CHECK-HIDDEN-LABEL: <caller3>:
+# CHECK-HIDDEN-NEXT:  10030000: bl 0x1004000c
+# CHECK-HIDDEN-NEXT:  10030004: b 0x1004000c
+
+# CHECK-HIDDEN-LABEL: <caller4>:
+# CHECK-HIDDEN-NEXT:  10040000: bl 0x10040010
+# CHECK-HIDDEN-NEXT:  10040004: b 0x10040010
+
+# CHECK-HIDDEN-LABEL: <func_extern>:
+# CHECK-HIDDEN-NEXT:  10040008: blr
+
+# CHECK-HIDDEN-LABEL: <callee3_stother0_hidden>:
+# CHECK-HIDDEN-NEXT:  1004000c: blr
+
+# CHECK-HIDDEN-LABEL: <callee4_stother1_hidden>:
+# CHECK-HIDDEN-NEXT:  10040010: bl 0x10040008
+# CHECK-HIDDEN-NEXT:  10040014: blr
+.ifdef HIDDEN
+.section .text_hidden_stother0, "ax", %progbits
+caller3:
+  .localentry caller3, 1
+  ## nop is not needed after bl for R_PPC64_REL24_NOTOC
+  bl callee3_stother0_hidden@notoc
+  b callee3_stother0_hidden@notoc
+
+.section .text_hidden_stother1, "ax", %progbits
+caller4:
+  .localentry caller4, 1
+  ## nop is not needed after bl for R_PPC64_REL24_NOTOC
+  bl callee4_stother1_hidden@notoc
+  b callee4_stother1_hidden@notoc
+.endif