[PATCH] D40248: [ELF][ARM] Refine check for when undefined weak needs a Thunk

[Rafael Avila de Espindola via llvm-commits]

LGTM

Thanks,
Rafael

Peter Smith via Phabricator <reviews at reviews.llvm.org> writes:

> peter.smith created this revision.
> Herald added subscribers: kristof.beyls, arichardson, javed.absar, emaste, aemerson.
>
> When an undefined weak reference has a PLT entry we must generate a range extension thunk for any B or BL that can't reach the PLT entry.
>
> This change explicitly looks for whether a PLT entry exists rather than assuming that weak references never need PLT entries unless Config->Shared is in operation. This covers the case where we are linking an executable with dynamic linking, hence a PLT entry will be needed for undefined weak references. This case comes up in real programs over 32 Mb in size as there is a B to a weak reference __gmon__start__ in the Arm crti.o for glibc.
>
> Discovered while linking chrome on arm, error message: /usr/lib/../lib/crti.o:(.text+0x18): relocation R_ARM_JUMP24 out of range
>
>
> https://reviews.llvm.org/D40248
>
> Files:
>   ELF/Arch/ARM.cpp
>   test/ELF/arm-branch-undef-weak-plt-thunk.s
>
>
> Index: test/ELF/arm-branch-undef-weak-plt-thunk.s
> ===================================================================
> --- /dev/null
> +++ test/ELF/arm-branch-undef-weak-plt-thunk.s
> @@ -0,0 +1,35 @@
> +// RUN: llvm-mc -filetype=obj -triple=armv7a-none-linux-gnueabi %S/Inputs/arm-shared.s -o %t
> +// RUN: ld.lld %t --shared -o %t.so
> +// RUN: llvm-mc -filetype=obj -triple=armv7a-none-linux-gnueabi %s -o %t2
> +// RUN: ld.lld %t2 %t.so -o %t3
> +// RUN: llvm-objdump -d -triple=armv7a-none-linux-gnueabi -start-address=69632 -stop-address=69664 %t3 | FileCheck %s
> +// REQUIRES: arm
> +
> +// When we are dynamic linking, undefined weak references have a PLT entry so
> +// we must create a thunk for the branch to the PLT entry.
> +
> + .text
> + .globl bar2
> + .weak undefined_weak_we_expect_a_plt_entry_for
> +_start:
> + .globl _start
> + .type _start, %function
> + b undefined_weak_we_expect_a_plt_entry_for
> + bl bar2
> +// Create 32 Mb gap between the call to the weak reference and the PLT so that
> +// the b and bl need a range-extension thunk.
> + .section .text.1, "ax", %progbits
> + .space 32 * 1024 * 1024
> +
> +// CHECK: Disassembly of section .text:
> +// CHECK-NEXT: _start:
> +// CHECK-NEXT:    11000:       00 00 00 ea     b       #0 <__ARMv7ABSLongThunk_undefined_weak_we_expect_a_plt_entry_for>
> +// CHECK-NEXT:    11004:       02 00 00 eb     bl      #8 <__ARMv7ABSLongThunk_bar2>
> +// CHECK: __ARMv7ABSLongThunk_undefined_weak_we_expect_a_plt_entry_for:
> +// CHECK-NEXT:    11008:       34 c0 01 e3     movw    r12, #4148
> +// CHECK-NEXT:    1100c:       01 c2 40 e3     movt    r12, #513
> +// CHECK-NEXT:    11010:       1c ff 2f e1     bx      r12
> +// CHECK: __ARMv7ABSLongThunk_bar2:
> +// CHECK-NEXT:    11014:       44 c0 01 e3     movw    r12, #4164
> +// CHECK-NEXT:    11018:       01 c2 40 e3     movt    r12, #513
> +// CHECK-NEXT:    1101c:       1c ff 2f e1     bx      r12
> Index: ELF/Arch/ARM.cpp
> ===================================================================
> --- ELF/Arch/ARM.cpp
> +++ ELF/Arch/ARM.cpp
> @@ -229,10 +229,9 @@
>  
>  bool ARM::needsThunk(RelExpr Expr, RelType Type, const InputFile *File,
>                       uint64_t BranchAddr, const Symbol &S) const {
> -  // If S is an undefined weak symbol in an executable we don't need a Thunk.
> -  // In a DSO calls to undefined symbols, including weak ones get PLT entries
> -  // which may need a thunk.
> -  if (S.isUndefWeak() && !Config->Shared)
> +  // If S is an undefined weak symbol and does not have a PLT entry then it
> +  // will be resolved as a branch to the next instruction.
> +  if (S.isUndefWeak() && !S.isInPlt())
>      return false;
>    // A state change from ARM to Thumb and vice versa must go through an
>    // interworking thunk if the relocation type is not R_ARM_CALL or
>
>
> Index: test/ELF/arm-branch-undef-weak-plt-thunk.s
> ===================================================================
> --- /dev/null
> +++ test/ELF/arm-branch-undef-weak-plt-thunk.s
> @@ -0,0 +1,35 @@
> +// RUN: llvm-mc -filetype=obj -triple=armv7a-none-linux-gnueabi %S/Inputs/arm-shared.s -o %t
> +// RUN: ld.lld %t --shared -o %t.so
> +// RUN: llvm-mc -filetype=obj -triple=armv7a-none-linux-gnueabi %s -o %t2
> +// RUN: ld.lld %t2 %t.so -o %t3
> +// RUN: llvm-objdump -d -triple=armv7a-none-linux-gnueabi -start-address=69632 -stop-address=69664 %t3 | FileCheck %s
> +// REQUIRES: arm
> +
> +// When we are dynamic linking, undefined weak references have a PLT entry so
> +// we must create a thunk for the branch to the PLT entry.
> +
> + .text
> + .globl bar2
> + .weak undefined_weak_we_expect_a_plt_entry_for
> +_start:
> + .globl _start
> + .type _start, %function
> + b undefined_weak_we_expect_a_plt_entry_for
> + bl bar2
> +// Create 32 Mb gap between the call to the weak reference and the PLT so that
> +// the b and bl need a range-extension thunk.
> + .section .text.1, "ax", %progbits
> + .space 32 * 1024 * 1024
> +
> +// CHECK: Disassembly of section .text:
> +// CHECK-NEXT: _start:
> +// CHECK-NEXT:    11000:       00 00 00 ea     b       #0 <__ARMv7ABSLongThunk_undefined_weak_we_expect_a_plt_entry_for>
> +// CHECK-NEXT:    11004:       02 00 00 eb     bl      #8 <__ARMv7ABSLongThunk_bar2>
> +// CHECK: __ARMv7ABSLongThunk_undefined_weak_we_expect_a_plt_entry_for:
> +// CHECK-NEXT:    11008:       34 c0 01 e3     movw    r12, #4148
> +// CHECK-NEXT:    1100c:       01 c2 40 e3     movt    r12, #513
> +// CHECK-NEXT:    11010:       1c ff 2f e1     bx      r12
> +// CHECK: __ARMv7ABSLongThunk_bar2:
> +// CHECK-NEXT:    11014:       44 c0 01 e3     movw    r12, #4164
> +// CHECK-NEXT:    11018:       01 c2 40 e3     movt    r12, #513
> +// CHECK-NEXT:    1101c:       1c ff 2f e1     bx      r12
> Index: ELF/Arch/ARM.cpp
> ===================================================================
> --- ELF/Arch/ARM.cpp
> +++ ELF/Arch/ARM.cpp
> @@ -229,10 +229,9 @@
>  
>  bool ARM::needsThunk(RelExpr Expr, RelType Type, const InputFile *File,
>                       uint64_t BranchAddr, const Symbol &S) const {
> -  // If S is an undefined weak symbol in an executable we don't need a Thunk.
> -  // In a DSO calls to undefined symbols, including weak ones get PLT entries
> -  // which may need a thunk.
> -  if (S.isUndefWeak() && !Config->Shared)
> +  // If S is an undefined weak symbol and does not have a PLT entry then it
> +  // will be resolved as a branch to the next instruction.
> +  if (S.isUndefWeak() && !S.isInPlt())
>      return false;
>    // A state change from ARM to Thumb and vice versa must go through an
>    // interworking thunk if the relocation type is not R_ARM_CALL or