[Thumb,ELF] Fix access to dso_preemptable __stack_chk_guard with static relocation model #78950

MaskRay · 2024-01-22T08:29:12Z

PR #70014 fixes A32 to use GOT for dso_preemptable __stack_chk_guard
with static relocation model (e.g. -fPIE/-fPIC LTO compiles with -no-pie
linking).

This patch fixes such __stack_chk_guard access for Thumb1 and Thumb2.
Note: t2LDRLIT_ga_pcrel is only for ELF. mingw needs
.refptr.__stack_chk_guard (https://reviews.llvm.org/D92738).

Fix #64999

Created using spr 1.3.4

llvmbot · 2024-01-22T08:29:47Z

@llvm/pr-subscribers-backend-arm

Author: Fangrui Song (MaskRay)

Changes

After #70014, A32 correctly uses GOT for dso_preemptable
__stack_chk_guard with static relocation model (e.g. -fPIE/-fPIC LTO
compiles with -no-pie linking).

This patch fixes Thumb1 and Thumb2.

Fix #64999

Full diff: https://github.com/llvm/llvm-project/pull/78950.diff

3 Files Affected:

(modified) llvm/lib/Target/ARM/Thumb1InstrInfo.cpp (+3-2)
(modified) llvm/lib/Target/ARM/Thumb2InstrInfo.cpp (+1-1)
(modified) llvm/test/CodeGen/ARM/stack-guard-elf.ll (+24-6)

diff --git a/llvm/lib/Target/ARM/Thumb1InstrInfo.cpp b/llvm/lib/Target/ARM/Thumb1InstrInfo.cpp
index e2f3fad20079047..e3104e8ee765f9c 100644
--- a/llvm/lib/Target/ARM/Thumb1InstrInfo.cpp
+++ b/llvm/lib/Target/ARM/Thumb1InstrInfo.cpp
@@ -135,14 +135,15 @@ void Thumb1InstrInfo::loadRegFromStackSlot(MachineBasicBlock &MBB,
 void Thumb1InstrInfo::expandLoadStackGuard(
     MachineBasicBlock::iterator MI) const {
   MachineFunction &MF = *MI->getParent()->getParent();
-  const TargetMachine &TM = MF.getTarget();
   const ARMSubtarget &ST = MF.getSubtarget<ARMSubtarget>();
+  const GlobalValue *GV =
+      cast<GlobalValue>((*MI->memoperands_begin())->getValue());
 
   assert(MF.getFunction().getParent()->getStackProtectorGuard() != "tls" &&
          "TLS stack protector not supported for Thumb1 targets");
 
   unsigned Instr;
-  if (TM.isPositionIndependent())
+  if (!GV->isDSOLocal())
     Instr = ARM::tLDRLIT_ga_pcrel;
   else if (ST.genExecuteOnly() && ST.hasV8MBaselineOps())
     Instr = ARM::t2MOVi32imm;
diff --git a/llvm/lib/Target/ARM/Thumb2InstrInfo.cpp b/llvm/lib/Target/ARM/Thumb2InstrInfo.cpp
index 2ea0eaa0aad8f56..9e4b51616b56ecc 100644
--- a/llvm/lib/Target/ARM/Thumb2InstrInfo.cpp
+++ b/llvm/lib/Target/ARM/Thumb2InstrInfo.cpp
@@ -264,7 +264,7 @@ void Thumb2InstrInfo::expandLoadStackGuard(
   const GlobalValue *GV =
       cast<GlobalValue>((*MI->memoperands_begin())->getValue());
 
-  if (MF.getSubtarget<ARMSubtarget>().isGVInGOT(GV))
+  if (!GV->isDSOLocal())
     expandLoadStackGuardBase(MI, ARM::t2LDRLIT_ga_pcrel, ARM::t2LDRi12);
   else if (MF.getTarget().isPositionIndependent())
     expandLoadStackGuardBase(MI, ARM::t2MOV_ga_pcrel, ARM::t2LDRi12);
diff --git a/llvm/test/CodeGen/ARM/stack-guard-elf.ll b/llvm/test/CodeGen/ARM/stack-guard-elf.ll
index 250f2ad9ed1093b..d0e5db7e5711b0c 100644
--- a/llvm/test/CodeGen/ARM/stack-guard-elf.ll
+++ b/llvm/test/CodeGen/ARM/stack-guard-elf.ll
@@ -59,6 +59,8 @@ define i32 @test1() #0 {
 ; THUMB1-NEXT:    .pad #16
 ; THUMB1-NEXT:    sub sp, #16
 ; THUMB1-NEXT:    ldr r0, .LCPI0_0
+; THUMB1-NEXT:  .LPC0_0:
+; THUMB1-NEXT:    add r0, pc
 ; THUMB1-NEXT:    ldr r0, [r0]
 ; THUMB1-NEXT:    ldr r0, [r0]
 ; THUMB1-NEXT:    add r1, sp, #904
@@ -67,7 +69,9 @@ define i32 @test1() #0 {
 ; THUMB1-NEXT:    bl foo
 ; THUMB1-NEXT:    add r0, sp, #904
 ; THUMB1-NEXT:    ldr r0, [r0, #124]
-; THUMB1-NEXT:    ldr r1, .LCPI0_0
+; THUMB1-NEXT:    ldr r1, .LCPI0_1
+; THUMB1-NEXT:  .LPC0_1:
+; THUMB1-NEXT:    add r1, pc
 ; THUMB1-NEXT:    ldr r1, [r1]
 ; THUMB1-NEXT:    ldr r1, [r1]
 ; THUMB1-NEXT:    cmp r1, r0
@@ -83,7 +87,11 @@ define i32 @test1() #0 {
 ; THUMB1-NEXT:    .p2align 2
 ; THUMB1-NEXT:  @ %bb.3:
 ; THUMB1-NEXT:  .LCPI0_0:
-; THUMB1-NEXT:    .long __stack_chk_guard
+; THUMB1-NEXT:  .Ltmp0:
+; THUMB1-NEXT:    .long __stack_chk_guard(GOT_PREL)-((.LPC0_0+4)-.Ltmp0)
+; THUMB1-NEXT:  .LCPI0_1:
+; THUMB1-NEXT:  .Ltmp1:
+; THUMB1-NEXT:    .long __stack_chk_guard(GOT_PREL)-((.LPC0_1+4)-.Ltmp1)
 ;
 ; THUMB1-PIC-LABEL: test1:
 ; THUMB1-PIC:       @ %bb.0:
@@ -136,16 +144,18 @@ define i32 @test1() #0 {
 ; THUMB2-NEXT:    push {r7, lr}
 ; THUMB2-NEXT:    .pad #1032
 ; THUMB2-NEXT:    sub.w sp, sp, #1032
-; THUMB2-NEXT:    movw r0, :lower16:__stack_chk_guard
-; THUMB2-NEXT:    movt r0, :upper16:__stack_chk_guard
+; THUMB2-NEXT:    ldr r0, .LCPI0_0
+; THUMB2-NEXT:  .LPC0_0:
+; THUMB2-NEXT:    add r0, pc
 ; THUMB2-NEXT:    ldr r0, [r0]
 ; THUMB2-NEXT:    ldr r0, [r0]
 ; THUMB2-NEXT:    str.w r0, [sp, #1028]
 ; THUMB2-NEXT:    add r0, sp, #4
 ; THUMB2-NEXT:    bl foo
-; THUMB2-NEXT:    movw r1, :lower16:__stack_chk_guard
 ; THUMB2-NEXT:    ldr.w r0, [sp, #1028]
-; THUMB2-NEXT:    movt r1, :upper16:__stack_chk_guard
+; THUMB2-NEXT:    ldr r1, .LCPI0_1
+; THUMB2-NEXT:  .LPC0_1:
+; THUMB2-NEXT:    add r1, pc
 ; THUMB2-NEXT:    ldr r1, [r1]
 ; THUMB2-NEXT:    ldr r1, [r1]
 ; THUMB2-NEXT:    cmp r1, r0
@@ -155,6 +165,14 @@ define i32 @test1() #0 {
 ; THUMB2-NEXT:    popeq {r7, pc}
 ; THUMB2-NEXT:  .LBB0_1:
 ; THUMB2-NEXT:    bl __stack_chk_fail
+; THUMB2-NEXT:    .p2align 2
+; THUMB2-NEXT:  @ %bb.2:
+; THUMB2-NEXT:  .LCPI0_0:
+; THUMB2-NEXT:  .Ltmp0:
+; THUMB2-NEXT:    .long __stack_chk_guard(GOT_PREL)-((.LPC0_0+4)-.Ltmp0)
+; THUMB2-NEXT:  .LCPI0_1:
+; THUMB2-NEXT:  .Ltmp1:
+; THUMB2-NEXT:    .long __stack_chk_guard(GOT_PREL)-((.LPC0_1+4)-.Ltmp1)
 ;
 ; THUMB2-PIC-LABEL: test1:
 ; THUMB2-PIC:       @ %bb.0:

nickdesaulniers · 2024-01-22T18:59:00Z

llvm/lib/Target/ARM/Thumb1InstrInfo.cpp

+  const GlobalValue *GV =
+      cast<GlobalValue>((*MI->memoperands_begin())->getValue());


const auto *GV = ...

https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable

(this can be fixed in llvm/lib/Target/ARM/Thumb2InstrInfo.cpp L264 as well)

Thanks for the suggestion. Done

nickdesaulniers · 2024-01-22T18:59:23Z

(the commit message seems odd; intentional?)

Created using spr 1.3.4

MaskRay · 2024-01-22T19:52:53Z

(the commit message seems odd; intentional?)

Hopefully clarified:)

nickdesaulniers · 2024-01-22T20:35:05Z

llvm/lib/Target/ARM/Thumb1InstrInfo.cpp


  assert(MF.getFunction().getParent()->getStackProtectorGuard() != "tls" &&
         "TLS stack protector not supported for Thumb1 targets");

  unsigned Instr;
-  if (TM.isPositionIndependent())
+  if (!GV->isDSOLocal())


does Thumb1 need similar guards for whether the target is elf?

I believe the answer is no.

Based on my archaeology to the Thumb1InstrInfo::expandLoadStackGuard function, the changes haven't been related to Thumb1-only microarchitectures (before v6T2/v7) for non-ELF object file formats.

https://reviews.llvm.org/D75453 says that Windows AArch32 requires at least v7.

nico · 2024-01-22T21:35:27Z

Is http://45.33.8.238/linux/128720/step_12.txt due to this? Is this a problem on my end?

gulfemsavrun · 2024-01-22T22:06:20Z

Is http://45.33.8.238/linux/128720/step_12.txt due to this? Is this a problem on my end?

We started seeing the same issue on our Clang toolchain builders on Linux:

******************** TEST 'LLVM :: CodeGen/Thumb/stack_guard_remat.ll' FAILED ********************
Exit Code: 1

Command Output (stderr):
--
RUN: at line 1: /b/s/w/ir/x/w/llvm_build/bin/llc < /b/s/w/ir/x/w/llvm-llvm-project/llvm/test/CodeGen/Thumb/stack_guard_remat.ll -mtriple=thumb-apple-darwin -relocation-model=pic -no-integrated-as | /b/s/w/ir/x/w/llvm_build/bin/FileCheck /b/s/w/ir/x/w/llvm-llvm-project/llvm/test/CodeGen/Thumb/stack_guard_remat.ll -check-prefix=PIC
+ /b/s/w/ir/x/w/llvm_build/bin/llc -mtriple=thumb-apple-darwin -relocation-model=pic -no-integrated-as
+ /b/s/w/ir/x/w/llvm_build/bin/FileCheck /b/s/w/ir/x/w/llvm-llvm-project/llvm/test/CodeGen/Thumb/stack_guard_remat.ll -check-prefix=PIC
warning: inline asm clobber list contains reserved registers: R7, R9, SP
note: Reserved registers on the clobber list may not be preserved across the asm statement, and clobbering them may lead to undefined behaviour.
RUN: at line 2: /b/s/w/ir/x/w/llvm_build/bin/llc < /b/s/w/ir/x/w/llvm-llvm-project/llvm/test/CodeGen/Thumb/stack_guard_remat.ll -mtriple=thumb-apple-darwin -relocation-model=static -no-integrated-as | /b/s/w/ir/x/w/llvm_build/bin/FileCheck /b/s/w/ir/x/w/llvm-llvm-project/llvm/test/CodeGen/Thumb/stack_guard_remat.ll -check-prefix=NO-PIC  -check-prefix=STATIC
+ /b/s/w/ir/x/w/llvm_build/bin/llc -mtriple=thumb-apple-darwin -relocation-model=static -no-integrated-as
+ /b/s/w/ir/x/w/llvm_build/bin/FileCheck /b/s/w/ir/x/w/llvm-llvm-project/llvm/test/CodeGen/Thumb/stack_guard_remat.ll -check-prefix=NO-PIC -check-prefix=STATIC
warning: inline asm clobber list contains reserved registers: R7, R9, SP
note: Reserved registers on the clobber list may not be preserved across the asm statement, and clobbering them may lead to undefined behaviour.
/b/s/w/ir/x/w/llvm-llvm-project/llvm/test/CodeGen/Thumb/stack_guard_remat.ll:23:15: error: NO-PIC-NEXT: is not on the line after the previous match
;NO-PIC-NEXT: ldr [[ORIGINAL_GUARD]], [[[ORIGINAL_GUARD]]]
              ^
<stdin>:35:2: note: 'next' match was here
 ldr r1, [r1]
 ^
<stdin>:32:17: note: previous match ended here
 ldr r1, LCPI0_2
                ^
<stdin>:33:1: note: non-matching line after previous match is here
LPC0_2:
^

Input file: <stdin>
Check file: /b/s/w/ir/x/w/llvm-llvm-project/llvm/test/CodeGen/Thumb/stack_guard_remat.ll

-dump-input=help explains the following input dump.

Input was:
<<<<<<
         .
         .
         .
        30:  add r0, sp, #904 
        31:  ldr r0, [r0, #124] 
        32:  ldr r1, LCPI0_2 
        33: LPC0_2: 
        34:  add r1, pc 
        35:  ldr r1, [r1] 
next:23      !~~~~~~~~~~~  error: match on wrong line
        36:  cmp r1, r0 
        37:  bne LBB0_2 
        38: @ %bb.1: 
        39:  movs r0, #0 
        40:  subs r6, r7, #7 
         .
         .
         .
>>>>>>

--

********************

https://luci-milo.appspot.com/ui/p/fuchsia/builders/toolchain.ci/clang-linux-x64/b8758166179301265441/overview

MaskRay · 2024-01-22T22:20:41Z

Is http://45.33.8.238/linux/128720/step_12.txt due to this? Is this a problem on my end?

Sorry, I missed this failure. llvm/test/CodeGen/Thumb/stack_guard_remat.ll needed adjustment (relocation model pic/dynamic-no-pic should be used without "PIC Level" if thumb-apple-darwin doesn't use LTO).
I fixed it in eaef645

gulfemsavrun · 2024-01-22T23:19:16Z

Is http://45.33.8.238/linux/128720/step_12.txt due to this? Is this a problem on my end?

Sorry, I missed this failure. llvm/test/CodeGen/Thumb/stack_guard_remat.ll needed adjustment (relocation model pic/dynamic-no-pic should be used without "PIC Level" if thumb-apple-darwin doesn't use LTO). I fixed it in eaef645

eaef645 fixed the test failure, thanks!

[𝘀𝗽𝗿] initial version

2ce5748

Created using spr 1.3.4

MaskRay requested a review from ardbiesheuvel January 22, 2024 08:29

llvmbot added the backend:ARM label Jan 22, 2024

MaskRay requested review from efriedma-quic, nickdesaulniers and wolfy1961 January 22, 2024 08:29

MaskRay mentioned this pull request Jan 22, 2024

[LTO][PIC][ARM][SSP] extra indirection accessing __stack_chk_guard #64999

Closed

nickdesaulniers reviewed Jan 22, 2024

View reviewed changes

fix mingw. use 'auto'

7bd52da

Created using spr 1.3.4

nickdesaulniers reviewed Jan 22, 2024

View reviewed changes

nickdesaulniers approved these changes Jan 22, 2024

View reviewed changes

MaskRay merged commit 4cb90ca into main Jan 22, 2024

MaskRay deleted the users/MaskRay/spr/thumbelf-fix-access-to-dso_preemptable-__stack_chk_guard-with-static-relocation-model branch January 22, 2024 21:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Thumb,ELF] Fix access to dso_preemptable __stack_chk_guard with static relocation model #78950

[Thumb,ELF] Fix access to dso_preemptable __stack_chk_guard with static relocation model #78950

Uh oh!

MaskRay commented Jan 22, 2024 •

edited

Loading

Uh oh!

llvmbot commented Jan 22, 2024

Uh oh!

nickdesaulniers Jan 22, 2024

Uh oh!

MaskRay Jan 22, 2024

Uh oh!

nickdesaulniers commented Jan 22, 2024

Uh oh!

MaskRay commented Jan 22, 2024

Uh oh!

nickdesaulniers Jan 22, 2024

Uh oh!

MaskRay Jan 22, 2024 •

edited

Loading

Uh oh!

nico commented Jan 22, 2024

Uh oh!

gulfemsavrun commented Jan 22, 2024

Uh oh!

MaskRay commented Jan 22, 2024

Uh oh!

gulfemsavrun commented Jan 22, 2024

Uh oh!

Uh oh!

		const GlobalValue *GV =
		cast<GlobalValue>((*MI->memoperands_begin())->getValue());

[Thumb,ELF] Fix access to dso_preemptable __stack_chk_guard with static relocation model #78950

[Thumb,ELF] Fix access to dso_preemptable __stack_chk_guard with static relocation model #78950

Uh oh!

Conversation

MaskRay commented Jan 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Jan 22, 2024

Uh oh!

nickdesaulniers Jan 22, 2024

Choose a reason for hiding this comment

Uh oh!

MaskRay Jan 22, 2024

Choose a reason for hiding this comment

Uh oh!

nickdesaulniers commented Jan 22, 2024

Uh oh!

MaskRay commented Jan 22, 2024

Uh oh!

nickdesaulniers Jan 22, 2024

Choose a reason for hiding this comment

Uh oh!

MaskRay Jan 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nico commented Jan 22, 2024

Uh oh!

gulfemsavrun commented Jan 22, 2024

Uh oh!

MaskRay commented Jan 22, 2024

Uh oh!

gulfemsavrun commented Jan 22, 2024

Uh oh!

Uh oh!

MaskRay commented Jan 22, 2024 •

edited

Loading

MaskRay Jan 22, 2024 •

edited

Loading