[CodeGen][ARM] Bug fix InsertStackProtectors with EH_SJ_LJ #147411
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
when exception handling with setjmp/longjmp (exception-mode=eh_sjlj is enabled, eh_sjlj_callsite intrinsic is inserted in same basic block as the throwing/exception instruction. This fix ensures stack protector insertion code does not split the block and move these apart into different basic blocks.
|
@llvm/pr-subscribers-backend-arm Author: Abhay Kanhere (AbhayKanhere) Changeswhen exception handling with setjmp/longjmp (exception-mode=eh_sjlj is enabled, Full diff: https://github.com/llvm/llvm-project/pull/147411.diff 2 Files Affected:
diff --git a/llvm/lib/CodeGen/StackProtector.cpp b/llvm/lib/CodeGen/StackProtector.cpp
index 5f866eea7d4e7..a2bc69ef5ff4d 100644
--- a/llvm/lib/CodeGen/StackProtector.cpp
+++ b/llvm/lib/CodeGen/StackProtector.cpp
@@ -588,7 +588,14 @@ bool InsertStackProtectors(const TargetMachine *TM, Function *F,
continue;
Instruction *CheckLoc = dyn_cast<ReturnInst>(BB.getTerminator());
if (!CheckLoc && !DisableCheckNoReturn)
- for (auto &Inst : BB)
+ for (auto &Inst : BB) {
+ if(auto *IB = dyn_cast<IntrinsicInst>(&Inst) )
+ if(IB->getIntrinsicID()==Intrinsic::eh_sjlj_callsite) {
+ // eh_sjlj_callsite has to be in same BB as the
+ // bb terminator. Dont insert within this range.
+ CheckLoc = IB;
+ break;
+ }
if (auto *CB = dyn_cast<CallBase>(&Inst))
// Do stack check before noreturn calls that aren't nounwind (e.g:
// __cxa_throw).
@@ -596,6 +603,7 @@ bool InsertStackProtectors(const TargetMachine *TM, Function *F,
CheckLoc = CB;
break;
}
+ }
if (!CheckLoc)
continue;
diff --git a/llvm/test/CodeGen/ARM/stack-protector-eh-sjlj.ll b/llvm/test/CodeGen/ARM/stack-protector-eh-sjlj.ll
new file mode 100644
index 0000000000000..860d4d468505d
--- /dev/null
+++ b/llvm/test/CodeGen/ARM/stack-protector-eh-sjlj.ll
@@ -0,0 +1,166 @@
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 5
+; RUN: llc -O0 -mtriple=thumbv7s-apple-darwin < %s | FileCheck %s
+target datalayout = "e-m:o-p:32:32-Fi8-f64:32:64-v64:32:64-v128:32:128-a:0:32-n32-S32"
+
+; Function Attrs: mustprogress noinline optnone ssp
+define ptr @foo() #0 personality ptr @__gxx_personality_sj0 {
+; CHECK-LABEL: foo:
+; CHECK: Lfunc_begin0:
+; CHECK-NEXT: @ %bb.0:
+; CHECK-NEXT: push {r4, r5, r6, r7, lr}
+; CHECK-NEXT: add r7, sp, #12
+; CHECK-NEXT: push.w {r8, r10, r11}
+; CHECK-NEXT: sub.w r4, sp, #64
+; CHECK-NEXT: bfc r4, #0, #4
+; CHECK-NEXT: mov sp, r4
+; CHECK-NEXT: vst1.64 {d8, d9, d10, d11}, [r4:128]!
+; CHECK-NEXT: vst1.64 {d12, d13, d14, d15}, [r4:128]
+; CHECK-NEXT: sub sp, #96
+; CHECK-NEXT: movw r0, :lower16:(L___stack_chk_guard$non_lazy_ptr-(LPC0_2+4))
+; CHECK-NEXT: movt r0, :upper16:(L___stack_chk_guard$non_lazy_ptr-(LPC0_2+4))
+; CHECK-NEXT: LPC0_2:
+; CHECK-NEXT: add r0, pc
+; CHECK-NEXT: ldr r0, [r0]
+; CHECK-NEXT: ldr r0, [r0]
+; CHECK-NEXT: movw r0, :lower16:(L___stack_chk_guard$non_lazy_ptr-(LPC0_3+4))
+; CHECK-NEXT: movt r0, :upper16:(L___stack_chk_guard$non_lazy_ptr-(LPC0_3+4))
+; CHECK-NEXT: LPC0_3:
+; CHECK-NEXT: add r0, pc
+; CHECK-NEXT: ldr r0, [r0]
+; CHECK-NEXT: ldr r0, [r0]
+; CHECK-NEXT: str r0, [sp, #92]
+; CHECK-NEXT: movw r0, :lower16:(L___gxx_personality_sj0$non_lazy_ptr-(LPC0_4+4))
+; CHECK-NEXT: movt r0, :upper16:(L___gxx_personality_sj0$non_lazy_ptr-(LPC0_4+4))
+; CHECK-NEXT: LPC0_4:
+; CHECK-NEXT: add r0, pc
+; CHECK-NEXT: ldr r0, [r0]
+; CHECK-NEXT: str r0, [sp, #36]
+; CHECK-NEXT: ldr r0, LCPI0_0
+; CHECK-NEXT: LPC0_0:
+; CHECK-NEXT: add r0, pc
+; CHECK-NEXT: str r0, [sp, #40]
+; CHECK-NEXT: str r7, [sp, #44]
+; CHECK-NEXT: mov r0, sp
+; CHECK-NEXT: str r0, [sp, #52]
+; CHECK-NEXT: ldr r0, LCPI0_1
+; CHECK-NEXT: orr r0, r0, #1
+; CHECK-NEXT: LPC0_1:
+; CHECK-NEXT: add r0, pc
+; CHECK-NEXT: str r0, [sp, #48]
+; CHECK-NEXT: add r0, sp, #12
+; CHECK-NEXT: bl __Unwind_SjLj_Register
+; CHECK-NEXT: movs r0, #1
+; CHECK-NEXT: str r0, [sp, #16]
+; CHECK-NEXT: movw r0, :lower16:(L___stack_chk_guard$non_lazy_ptr-(LPC0_5+4))
+; CHECK-NEXT: movt r0, :upper16:(L___stack_chk_guard$non_lazy_ptr-(LPC0_5+4))
+; CHECK-NEXT: LPC0_5:
+; CHECK-NEXT: add r0, pc
+; CHECK-NEXT: ldr r0, [r0]
+; CHECK-NEXT: ldr r0, [r0]
+; CHECK-NEXT: ldr r1, [sp, #92]
+; CHECK-NEXT: cmp r0, r1
+; CHECK-NEXT: bne LBB0_7
+; CHECK-NEXT: @ %bb.1: @ %SP_return
+; CHECK-NEXT: Ltmp0:
+; CHECK-NEXT: movs r2, #0
+; CHECK-NEXT: mov r0, r2
+; CHECK-NEXT: mov r1, r2
+; CHECK-NEXT: blx r2
+; CHECK-NEXT: Ltmp1:
+; CHECK-NEXT: b LBB0_2
+; CHECK-NEXT: LBB0_2:
+; CHECK-NEXT: movs r0, #2
+; CHECK-NEXT: str r0, [sp, #16]
+; CHECK-NEXT: movw r0, :lower16:(L___stack_chk_guard$non_lazy_ptr-(LPC0_6+4))
+; CHECK-NEXT: movt r0, :upper16:(L___stack_chk_guard$non_lazy_ptr-(LPC0_6+4))
+; CHECK-NEXT: LPC0_6:
+; CHECK-NEXT: add r0, pc
+; CHECK-NEXT: ldr r0, [r0]
+; CHECK-NEXT: ldr r0, [r0]
+; CHECK-NEXT: ldr r1, [sp, #92]
+; CHECK-NEXT: cmp r0, r1
+; CHECK-NEXT: bne LBB0_7
+; CHECK-NEXT: @ %bb.3: @ %SP_return2
+; CHECK-NEXT: Ltmp2:
+; CHECK-NEXT: movs r3, #0
+; CHECK-NEXT: mov r0, r3
+; CHECK-NEXT: mov r1, r3
+; CHECK-NEXT: mov r2, r3
+; CHECK-NEXT: blx r3
+; CHECK-NEXT: Ltmp3:
+; CHECK-NEXT: b LBB0_6
+; CHECK-NEXT: LBB0_4:
+; CHECK-NEXT: Ltmp4:
+; CHECK-NEXT: ldr r0, [sp, #20]
+; CHECK-NEXT: ldr r0, [sp, #24]
+; CHECK-NEXT: add r0, sp, #12
+; CHECK-NEXT: bl __Unwind_SjLj_Unregister
+; CHECK-NEXT: movw r0, :lower16:(L___stack_chk_guard$non_lazy_ptr-(LPC0_7+4))
+; CHECK-NEXT: movt r0, :upper16:(L___stack_chk_guard$non_lazy_ptr-(LPC0_7+4))
+; CHECK-NEXT: LPC0_7:
+; CHECK-NEXT: add r0, pc
+; CHECK-NEXT: ldr r0, [r0]
+; CHECK-NEXT: ldr r0, [r0]
+; CHECK-NEXT: ldr r1, [sp, #92]
+; CHECK-NEXT: cmp r0, r1
+; CHECK-NEXT: bne LBB0_7
+; CHECK-NEXT: @ %bb.5: @ %SP_return3
+; CHECK-NEXT: movs r0, #0
+; CHECK-NEXT: add r4, sp, #96
+; CHECK-NEXT: vld1.64 {d8, d9, d10, d11}, [r4:128]!
+; CHECK-NEXT: vld1.64 {d12, d13, d14, d15}, [r4:128]
+; CHECK-NEXT: sub.w r4, r7, #24
+; CHECK-NEXT: mov sp, r4
+; CHECK-NEXT: pop.w {r8, r10, r11}
+; CHECK-NEXT: pop {r4, r5, r6, r7, pc}
+; CHECK-NEXT: LBB0_6:
+; CHECK-NEXT: trap
+; CHECK-NEXT: LBB0_7: @ %CallStackCheckFailBlk
+; CHECK-NEXT: bl ___stack_chk_fail
+; CHECK-NEXT: LBB0_8:
+; CHECK-NEXT: ldr r0, [sp, #16]
+; CHECK-NEXT: str r0, [sp, #8] @ 4-byte Spill
+; CHECK-NEXT: cmp r0, #2
+; CHECK-NEXT: bhi LBB0_12
+; CHECK-NEXT: @ %bb.9:
+; CHECK-NEXT: ldr r1, [sp, #8] @ 4-byte Reload
+; CHECK-NEXT: LCPI0_2:
+; CHECK-NEXT: tbb [pc, r1]
+; CHECK-NEXT: @ %bb.10:
+; CHECK-NEXT: LJTI0_0:
+; CHECK-NEXT: .data_region jt8
+; CHECK-NEXT: .byte (LBB0_11-(LCPI0_2+4))/2
+; CHECK-NEXT: .byte (LBB0_11-(LCPI0_2+4))/2
+; CHECK-NEXT: .end_data_region
+; CHECK-NEXT: .p2align 1
+; CHECK-NEXT: LBB0_11:
+; CHECK-NEXT: b LBB0_4
+; CHECK-NEXT: LBB0_12:
+; CHECK-NEXT: trap
+; CHECK-NEXT: .p2align 2
+; CHECK-NEXT: @ %bb.13:
+ %1 = alloca [14 x i8], align 16
+ %2 = invoke i32 null(ptr noundef null, ptr noundef null) #1
+ to label %3 unwind label %4
+
+3: ; preds = %0
+ invoke void null(ptr null, ptr null, ptr null) #2
+ to label %6 unwind label %4
+
+4: ; preds = %3, %0
+ %5 = landingpad { ptr, i32 }
+ cleanup
+ ret ptr null
+
+6: ; preds = %3
+ unreachable
+}
+
+declare i32 @__gxx_personality_sj0(...)
+
+; uselistorder directives
+uselistorder ptr null, { 2, 3, 4, 5, 0, 6, 7, 1, 8, 9 }
+
+attributes #0 = { mustprogress ssp "frame-pointer"="all" "no-builtin-calloc" "no-builtin-stpcpy" "no-trapping-math"="true" "stack-protector-buffer-size"="8" }
+attributes #1 = { "no-builtin-calloc" "no-builtin-stpcpy" }
+attributes #2 = { noreturn }
|
You can test this locally with the following command:git-clang-format --diff HEAD~1 HEAD --extensions cpp -- llvm/lib/CodeGen/StackProtector.cppView the diff from clang-format here.diff --git a/llvm/lib/CodeGen/StackProtector.cpp b/llvm/lib/CodeGen/StackProtector.cpp
index a2bc69ef5..e31083702 100644
--- a/llvm/lib/CodeGen/StackProtector.cpp
+++ b/llvm/lib/CodeGen/StackProtector.cpp
@@ -589,13 +589,13 @@ bool InsertStackProtectors(const TargetMachine *TM, Function *F,
Instruction *CheckLoc = dyn_cast<ReturnInst>(BB.getTerminator());
if (!CheckLoc && !DisableCheckNoReturn)
for (auto &Inst : BB) {
- if(auto *IB = dyn_cast<IntrinsicInst>(&Inst) )
- if(IB->getIntrinsicID()==Intrinsic::eh_sjlj_callsite) {
- // eh_sjlj_callsite has to be in same BB as the
- // bb terminator. Dont insert within this range.
- CheckLoc = IB;
- break;
- }
+ if (auto *IB = dyn_cast<IntrinsicInst>(&Inst))
+ if (IB->getIntrinsicID() == Intrinsic::eh_sjlj_callsite) {
+ // eh_sjlj_callsite has to be in same BB as the
+ // bb terminator. Dont insert within this range.
+ CheckLoc = IB;
+ break;
+ }
if (auto *CB = dyn_cast<CallBase>(&Inst))
// Do stack check before noreturn calls that aren't nounwind (e.g:
// __cxa_throw).
|
jroelofs
reviewed
Jul 7, 2025
| if(auto *IB = dyn_cast<IntrinsicInst>(&Inst) ) | ||
| if(IB->getIntrinsicID()==Intrinsic::eh_sjlj_callsite) { | ||
| // eh_sjlj_callsite has to be in same BB as the | ||
| // bb terminator. Dont insert within this range. |
There was a problem hiding this comment.
Suggested change
| // bb terminator. Dont insert within this range. | |
| // bb terminator. Don't insert within this range. |
| @@ -588,14 +588,22 @@ bool InsertStackProtectors(const TargetMachine *TM, Function *F, | |||
| continue; | |||
| Instruction *CheckLoc = dyn_cast<ReturnInst>(BB.getTerminator()); | |||
| if (!CheckLoc && !DisableCheckNoReturn) | |||
| for (auto &Inst : BB) | |||
| for (auto &Inst : BB) { | |||
| if(auto *IB = dyn_cast<IntrinsicInst>(&Inst) ) | |||
There was a problem hiding this comment.
The clang-format bot is complaining about the spacing of things here. You should apply the diff it gave you in this comment: #147411 (comment)
| to label %3 unwind label %4 | ||
|
|
||
| 3: ; preds = %0 | ||
| invoke void null(ptr null, ptr null, ptr null) #2 |
There was a problem hiding this comment.
Tests shouldn't have UB, unless the UB is the thing the test is trying to check, so instead of invoke void null(, this should be calling an actual callee. Likewise for the other invoke.
| uselistorder ptr null, { 2, 3, 4, 5, 0, 6, 7, 1, 8, 9 } | ||
|
|
||
| attributes #0 = { mustprogress ssp "frame-pointer"="all" "no-builtin-calloc" "no-builtin-stpcpy" "no-trapping-math"="true" "stack-protector-buffer-size"="8" } | ||
| attributes #1 = { "no-builtin-calloc" "no-builtin-stpcpy" } |
There was a problem hiding this comment.
I think attributes #1 can go away.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
when exception handling with setjmp/longjmp (exception-mode=eh_sjlj is enabled,
eh_sjlj_callsite intrinsic is inserted in same basic block as the throwing/exception instruction. This fix ensures stack protector insertion code does not split the block and move these apart into different basic blocks.