[llvm][ARM] Correct the properties of trap instructions #113287
Merged
Conversation
This file contains 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
Fixes llvm#113154 The encodings used for llvm.trap() on ARM were all marked as barriers and terminators. This lead to stack frame destroy code being inserted before the trap if the trap was the last thing in the function and it had no return statement. ``` void fn() { volatile int i = 0; __builtin_trap(); } ``` Produced: ``` fn: push {r11, lr} << stack frame create <...> mov sp, r11 pop {r11, lr} << stack frame destroy .inst 0xe7ffdefe << trap bx lr ``` All the other targets don't mark them this way, instead they mark them with isTrap. I've changed ARM to do this, which fixes the code generation: ``` fn: push {r11, lr} << stack frame create <...> .inst 0xe7ffdefe << trap mov sp, r11 pop {r11, lr} << stack frame destroy bx lr ``` I've updated the existing trap test to force the need for a stack frame, then check that the instruction immediately after the trap is resetting the stack pointer.
|
@llvm/pr-subscribers-backend-arm Author: David Spickett (DavidSpickett) ChangesFixes #113154 The encodings used for llvm.trap() on ARM were all marked as barriers and terminators. This lead to stack frame destroy code being inserted before the trap if the trap was the last thing in the function and it had no return statement. Produced: All the other targets don't mark them this way, instead they mark them with isTrap. I've changed ARM to do this, which fixes the code generation: I've updated the existing trap test to force the need for a stack frame, then check that the instruction immediately after the trap is resetting the stack pointer. Full diff: https://github.com/llvm/llvm-project/pull/113287.diff 3 Files Affected:
diff --git a/llvm/lib/Target/ARM/ARMInstrInfo.td b/llvm/lib/Target/ARM/ARMInstrInfo.td
index ed68c6ff20cde5..d24d4af36f0d86 100644
--- a/llvm/lib/Target/ARM/ARMInstrInfo.td
+++ b/llvm/lib/Target/ARM/ARMInstrInfo.td
@@ -2378,13 +2378,13 @@ def UDF : AInoP<(outs), (ins imm0_65535:$imm16), MiscFrm, NoItinerary,
* - In ARM: UDF #60896;
* - In Thumb: UDF #254 followed by a branch-to-self.
*/
-let isBarrier = 1, isTerminator = 1 in
+let isTrap = 1 in
def TRAPNaCl : AXI<(outs), (ins), MiscFrm, NoItinerary,
"trap", [(trap)]>,
Requires<[IsARM,UseNaClTrap]> {
let Inst = 0xe7fedef0;
}
-let isBarrier = 1, isTerminator = 1 in
+let isTrap = 1 in
def TRAP : AXI<(outs), (ins), MiscFrm, NoItinerary,
"trap", [(trap)]>,
Requires<[IsARM,DontUseNaClTrap]> {
diff --git a/llvm/lib/Target/ARM/ARMInstrThumb.td b/llvm/lib/Target/ARM/ARMInstrThumb.td
index 2ad78f8cd8d4c7..b92f42874bbddb 100644
--- a/llvm/lib/Target/ARM/ARMInstrThumb.td
+++ b/llvm/lib/Target/ARM/ARMInstrThumb.td
@@ -675,7 +675,7 @@ def tSVC : T1pI<(outs), (ins imm0_255:$imm), IIC_Br,
}
// The assembler uses 0xDEFE for a trap instruction.
-let isBarrier = 1, isTerminator = 1 in
+let isTrap = 1 in
def tTRAP : TI<(outs), (ins), IIC_Br,
"trap", [(trap)]>, Encoding16, Sched<[WriteBr]> {
let Inst = 0xdefe;
diff --git a/llvm/test/CodeGen/ARM/trap.ll b/llvm/test/CodeGen/ARM/trap.ll
index fe046e70166553..a5d2b2081f28f3 100644
--- a/llvm/test/CodeGen/ARM/trap.ll
+++ b/llvm/test/CodeGen/ARM/trap.ll
@@ -1,3 +1,6 @@
+;; Check encodings of trap instructions and that their properties are set
+;; correctly so that they are not placed after the stack frame is destroyed.
+
; RUN: llc < %s -mtriple=arm-apple-darwin | FileCheck %s -check-prefix=DARWIN
; RUN: llc < %s -mtriple=arm-apple-darwin -trap-func=_trap | FileCheck %s -check-prefix=FUNC
; RUN: llc < %s -mtriple=arm-apple-darwin -trap-func=_trap -O0 | FileCheck %s -check-prefix=FUNC
@@ -29,22 +32,31 @@
; rdar://7961298
; rdar://9249183
-define void @t() nounwind {
+define void @t() noinline optnone {
entry:
+ ;; So that we have a stack frame.
+ %1 = alloca i32, align 4
+ store volatile i32 0, ptr %1, align 4
+
; DARWIN-LABEL: t:
-; DARWIN: trap
+; DARWIN: trap
+; DARWIN-NEXT: add sp, sp, #4
; FUNC-LABEL: t:
-; FUNC: bl __trap
+; FUNC: bl __trap
+; FUNC-NEXT: add sp, sp, #4
; NACL-LABEL: t:
-; NACL: .inst 0xe7fedef0
+; NACL: .inst 0xe7fedef0
+; NACL-NEXT: add sp, sp, #4
; ARM-LABEL: t:
-; ARM: .inst 0xe7ffdefe
+; ARM: .inst 0xe7ffdefe
+; ARM-NEXT: add sp, sp, #4
; THUMB-LABEL: t:
-; THUMB: .inst.n 0xdefe
+; THUMB: .inst.n 0xdefe
+; THUMB-NEXT: add sp, #4
; ENCODING-NACL: e7fedef0 trap
@@ -53,25 +65,34 @@ entry:
; ENCODING-THUMB: defe trap
call void @llvm.trap()
- unreachable
+ ret void
}
-define void @t2() nounwind {
+define void @t2() {
entry:
+ ;; So that we have a stack frame.
+ %1 = alloca i32, align 4
+ store volatile i32 0, ptr %1, align 4
+
; DARWIN-LABEL: t2:
-; DARWIN: udf #254
+; DARWIN: udf #254
+; DARWIN-NEXT: add sp, sp, #4
; FUNC-LABEL: t2:
-; FUNC: bl __trap
+; FUNC: bl __trap
+; FUNC-NEXT: add sp, sp, #4
; NACL-LABEL: t2:
-; NACL: bkpt #0
+; NACL: bkpt #0
+; NACL-NEXT: add sp, sp, #4
; ARM-LABEL: t2:
-; ARM: bkpt #0
+; ARM: bkpt #0
+; ARM-NEXT: add sp, sp, #4
; THUMB-LABEL: t2:
-; THUMB: bkpt #0
+; THUMB: bkpt #0
+; THUMB-NEXT: add sp, #4
; ENCODING-NACL: e1200070 bkpt #0
@@ -80,7 +101,7 @@ entry:
; ENCODING-THUMB: be00 bkpt #0
call void @llvm.debugtrap()
- unreachable
+ ret void
}
declare void @llvm.trap() nounwind
|
efriedma-quic
approved these changes
Oct 22, 2024
There was a problem hiding this comment.
LGTM
(It looks like this was discovered before in https://reviews.llvm.org/D53614, but it wasn't fixed at the time for some reason.)
Closed
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.
Fixes #113154
The encodings used for llvm.trap() on ARM were all marked as barriers and terminators. This lead to stack frame destroy code being inserted before the trap if the trap was the last thing in the function and it had no return statement.
Produced:
All the other targets don't mark them this way, instead they mark them with isTrap. I've changed ARM to do this, which fixes the code generation:
I've updated the existing trap test to force the need for a stack frame, then check that the instruction immediately after the trap is resetting the stack pointer.
debugtrap was already working but I've added the same checks for it anyway.