[ELF][PPC64] Fix assertion failure for branches to hidden undefined weak for -no-pie

[MaskRay]

Summary

Reported by Stefan Pintilie in D119773.

For a branch to a hidden undefined weak symbol, there is an
assert(sym->getVA()); failure in PPC64LongBranchTargetSection::writeTo for a
-no-pie link. The root cause is that we unnecessarily produce a dynamic
relocation for a -no-pie link.

Fix this by changing the condition to just s.isUndefined(). See the inline
comment.

Rename ppc64-weak-undef-call.s to ppc64-undefined-weak.s to be consistent with
other architectures.

[MaskRay]

@stefanp I use this opportunity to clean up some tests, so I have created a new patch.

ppc64 psABI may want to clarify the behavior (aarch64 psABI defines the behavior for a branch to an undefined weak, though I think "branch to the next instruction" is a bad choice).
I have documented what I know in https://maskray.me/blog/2021-04-25-weak-symbol#branch-relocations

I find that GNU ld's powerpc64 port may change the bl instruction to nop in some cases. I think that is not great, either, since the run-time undefined behavior (branch to undefined weak) is not detected.

[MaskRay]

I will back port whatever fix into release/14.x

[MaskRay]

add a test for undefined non-weak

[stefanp]

I checked and this fixes the issue we were seeing.
Thank you for fixing this!

LGTM

[sfertile]

LGTM.

[Miss_Grape]

lld/ELF/Arch/PPC64.cpp

1386

Why externally referenced symbols, this is not the type of undefined symbols here？I would like to know what kind of scenario is being dealt with here

[MaskRay]

lld/ELF/Arch/PPC64.cpp

1386

I do not understand the question. "externally referenced symbol" is not a linker term. The symbol type is irrelevant. The condition has been explained in the updated comment.

lld/ELF/Arch/PPC64.cpp

bool PPC64::needsThunk(RelExpr expr, RelType type, const InputFile *file,
   // 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 (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)
+  // An undefined weak symbol not in a PLT does not need a thunk. If it is
+  // hidden, its binding has been converted to local, so we just check
+  // isUndefined() here. A undefined non-weak symbol has been errored.
+  if (s.isUndefined())
     return false;
 
   // If the offset exceeds the range of the branch type then it will need
   // a range-extending thunk.
   // See the comment in getRelocTargetVA() about R_PPC64_CALL.
   return !inBranchRange(type, branchAddr,
                         s.getVA(a) +
                             getPPC64GlobalEntryToLocalEntryOffset(s.stOther));

lld/test/ELF/ppc64-undefined-weak.s

+# REQUIRES: ppc
+
+# RUN: llvm-mc -filetype=obj -triple=powerpc64le %s -o %t.o
+# RUN: ld.lld %t.o -o %t
+# RUN: llvm-objdump -d --no-show-raw-insn %t | FileCheck %s --check-prefix=PDE
+# RUN: ld.lld -pie %t.o -o %t
+# RUN: llvm-objdump -d --no-show-raw-insn %t | FileCheck %s --check-prefix=PIC
+# RUN: ld.lld -shared %t.o -o %t.so
+# RUN: llvm-objdump -d --no-show-raw-insn %t.so | FileCheck %s --check-prefix=PIC
+
+# RUN: llvm-mc -filetype=obj -triple=powerpc64 %s -o %t.o
+# RUN: ld.lld %t.o -o %t
+# RUN: llvm-objdump -d --no-show-raw-insn %t | FileCheck %s --check-prefix=PDE
+
+## Branches to an undefined weak symbol need a thunk iff a dynamic relocation is
+## produced. undefweak2 is hidden and does not need a dynamic relocation, so we
+## suppress the thunk. undefweak1 needs a thunk iff -pie or -shared.
+
+# PDE-LABEL: <_start>:
+# PDE-NEXT:    bl {{.*}} <_start>
+# PDE-NEXT:    nop
+# PDE-NEXT:    bl {{.*}} <_start+0x8>
+# PDE-NEXT:    nop
+
+# PIC-LABEL: <_start>:
+# PIC-NEXT:    bl {{.*}} <__plt_undefweak1>
+# PIC-NEXT:    ld 2, 24(1)
+# PIC-NEXT:    bl {{.*}} <_start+0x8>
+# PIC-NEXT:    nop
+
+.text
+.global _start
+_start:
+  bl undefweak1
+  nop
+  bl undefweak2
+  nop
+
+.weak undefweak1, undefweak2
+.hidden undefweak2

lld/test/ELF/ppc64-undefined.s

+# REQUIRES: ppc
+
+# RUN: llvm-mc -filetype=obj -triple=powerpc64le %s -o %t.o
+# RUN: not ld.lld %t.o -o /dev/null 2>&1 | FileCheck %s --check-prefix=ERR
+
+# ERR: error: undefined symbol: undef
+
+.global _start
+_start:
+  bl undef
+  nop

lld/test/ELF/ppc64-weak-undef-call-shared.s

-# REQUIRES: ppc
-
-# RUN: llvm-mc -filetype=obj -triple=powerpc64le-unknown-linux %s -o %t.o
-# RUN: ld.lld -shared %t.o -o %t.so
-# RUN: llvm-readobj --symbols -r --dyn-syms %t.so | FileCheck %s
-
-# RUN: llvm-mc -filetype=obj -triple=powerpc64-unknown-linux %s -o %t.o
-# RUN: ld.lld -shared %t.o -o %t.so
-# RUN: llvm-readobj --symbols -r --dyn-syms %t.so | FileCheck %s
-
-.section        ".toc","aw"
-.quad weakfunc
-// CHECK-NOT: R_PPC64_RELATIVE
-
-.text
-.Lfoo:
-  bl weakfunc
-  nop
-// CHECK-NOT: R_PPC64_REL24
-
-.weak weakfunc

lld/test/ELF/ppc64-weak-undef-call.s

-# REQUIRES: ppc
-
-# RUN: llvm-mc -filetype=obj -triple=powerpc64le-unknown-linux %s -o %t
-# RUN: ld.lld %t -o %t2
-# RUN: llvm-objdump -d --no-show-raw-insn %t2 | FileCheck %s
-
-# RUN: llvm-mc -filetype=obj -triple=powerpc64-unknown-linux %s -o %t
-# RUN: ld.lld %t -o %t2
-# RUN: llvm-objdump -d --no-show-raw-insn %t2 | FileCheck %s
-
-# CHECK: Disassembly of section .text:
-# CHECK-EMPTY:
-
-.text
-.global _start
-_start:
-  bl weakfunc
-  nop
-  blr
-
-.weak weakfunc
-
-# It does not really matter how we fixup the bl, if at all, because it needs to
-# be unreachable. But, we should link successfully. We should not, however,
-# generate a .plt entry (this would be wasted space). For now, we do nothing
-# (leaving the zero relative offset present in the input).
-# CHECK: 10010158:       bl 0x10010158
-# CHECK: 1001015c:       nop
-# CHECK: 10010160:       blr