[llvm] [AArch64][PAC] Fix creating check instructions for BBs without an epilog (PR #92508)
Igor Kudrin via llvm-commits
llvm-commits at lists.llvm.org
Thu May 23 19:39:52 PDT 2024
https://github.com/igorkudrin updated https://github.com/llvm/llvm-project/pull/92508
>From 3e5325ab2a5c5a99d66f537ccc1eb03c294fbc3a Mon Sep 17 00:00:00 2001
From: Igor Kudrin <ikudrin at accesssoftek.com>
Date: Thu, 16 May 2024 22:26:19 -0700
Subject: [PATCH 1/4] [AArch64][PAC][NFC] Make checkAuthenticatedRegister()
return void
The return value is not used. This change simplifies an upcoming patch.
---
llvm/lib/Target/AArch64/AArch64PointerAuth.cpp | 10 +++++-----
llvm/lib/Target/AArch64/AArch64PointerAuth.h | 12 ++++--------
2 files changed, 9 insertions(+), 13 deletions(-)
diff --git a/llvm/lib/Target/AArch64/AArch64PointerAuth.cpp b/llvm/lib/Target/AArch64/AArch64PointerAuth.cpp
index abde099be382b..90bf089dbebf7 100644
--- a/llvm/lib/Target/AArch64/AArch64PointerAuth.cpp
+++ b/llvm/lib/Target/AArch64/AArch64PointerAuth.cpp
@@ -231,7 +231,7 @@ MachineMemOperand *createCheckMemOperand(MachineFunction &MF,
} // namespace
-MachineBasicBlock &llvm::AArch64PAuth::checkAuthenticatedRegister(
+void llvm::AArch64PAuth::checkAuthenticatedRegister(
MachineBasicBlock::iterator MBBI, AuthCheckMethod Method,
Register AuthenticatedReg, Register TmpReg, bool UseIKey, unsigned BrkImm) {
@@ -246,13 +246,13 @@ MachineBasicBlock &llvm::AArch64PAuth::checkAuthenticatedRegister(
default:
break;
case AuthCheckMethod::None:
- return MBB;
+ return;
case AuthCheckMethod::DummyLoad:
BuildMI(MBB, MBBI, DL, TII->get(AArch64::LDRWui), getWRegFromXReg(TmpReg))
.addReg(AuthenticatedReg)
.addImm(0)
.addMemOperand(createCheckMemOperand(MF, Subtarget));
- return MBB;
+ return;
}
// Control flow has to be changed, so arrange new MBBs.
@@ -287,7 +287,7 @@ MachineBasicBlock &llvm::AArch64PAuth::checkAuthenticatedRegister(
.addReg(TmpReg)
.addImm(62)
.addMBB(BreakBlock);
- return *SuccessBlock;
+ return;
case AuthCheckMethod::XPACHint:
assert(AuthenticatedReg == AArch64::LR &&
"XPACHint mode is only compatible with checking the LR register");
@@ -304,7 +304,7 @@ MachineBasicBlock &llvm::AArch64PAuth::checkAuthenticatedRegister(
BuildMI(CheckBlock, DL, TII->get(AArch64::Bcc))
.addImm(AArch64CC::NE)
.addMBB(BreakBlock);
- return *SuccessBlock;
+ return;
}
llvm_unreachable("Unknown AuthCheckMethod enum");
}
diff --git a/llvm/lib/Target/AArch64/AArch64PointerAuth.h b/llvm/lib/Target/AArch64/AArch64PointerAuth.h
index e1ceaed58abe4..4ffda74782245 100644
--- a/llvm/lib/Target/AArch64/AArch64PointerAuth.h
+++ b/llvm/lib/Target/AArch64/AArch64PointerAuth.h
@@ -98,14 +98,10 @@ enum class AuthCheckMethod {
/// using an I-key or D-key and which register can be used as temporary.
/// If an explicit BRK instruction is used to generate an exception, BrkImm
/// specifies its immediate operand.
-///
-/// \returns The machine basic block containing the code that is executed
-/// after the check succeeds.
-MachineBasicBlock &checkAuthenticatedRegister(MachineBasicBlock::iterator MBBI,
- AuthCheckMethod Method,
- Register AuthenticatedReg,
- Register TmpReg, bool UseIKey,
- unsigned BrkImm);
+void checkAuthenticatedRegister(MachineBasicBlock::iterator MBBI,
+ AuthCheckMethod Method,
+ Register AuthenticatedReg, Register TmpReg,
+ bool UseIKey, unsigned BrkImm);
/// Returns the number of bytes added by checkAuthenticatedRegister.
unsigned getCheckerSizeInBytes(AuthCheckMethod Method);
>From a3039508f7bf9eeacbb4739460468cb3e71ba133 Mon Sep 17 00:00:00 2001
From: Igor Kudrin <ikudrin at accesssoftek.com>
Date: Thu, 16 May 2024 22:26:32 -0700
Subject: [PATCH 2/4] test
---
.../AArch64/sign-return-address-tailcall.ll | 32 +++++++++++++++++++
1 file changed, 32 insertions(+)
diff --git a/llvm/test/CodeGen/AArch64/sign-return-address-tailcall.ll b/llvm/test/CodeGen/AArch64/sign-return-address-tailcall.ll
index cf033cb8208cc..0cc707298e458 100644
--- a/llvm/test/CodeGen/AArch64/sign-return-address-tailcall.ll
+++ b/llvm/test/CodeGen/AArch64/sign-return-address-tailcall.ll
@@ -129,4 +129,36 @@ define i32 @tailcall_ib_key() "sign-return-address"="all" "sign-return-address-k
ret i32 %call
}
+define i32 @tailcall_two_branches(i1 %0) "sign-return-address"="all" {
+; COMMON-LABEL: tailcall_two_branches:
+; COMMON: tbz w0, #0, .[[ELSE:LBB[_0-9]+]]
+; COMMON: str x30, [sp, #-16]!
+; COMMON: bl callee2
+; COMMON: ldr x30, [sp], #16
+; COMMON-NEXT: [[AUTIASP]]
+; COMMON-NEXT: .[[ELSE]]:
+
+; LDR-NEXT: ldr w16, [x30]
+;
+; BITS-NOTBI-NEXT: eor x16, x30, x30, lsl #1
+; BITS-NOTBI-NEXT: tbnz x16, #62, .[[FAIL:LBB[_0-9]+]]
+;
+; XPAC-NEXT: mov x16, x30
+; XPAC-NEXT: [[XPACLRI]]
+; XPAC-NEXT: cmp x16, x30
+; XPAC-NEXT: b.ne .[[FAIL:LBB[_0-9]+]]
+;
+; COMMON-NEXT: b callee
+; BRK-NEXT: .[[FAIL]]:
+; BRK-NEXT: brk #0xc470
+ br i1 %0, label %2, label %3
+2:
+ call void @callee2()
+ br label %3
+3:
+ %call = tail call i32 @callee()
+ ret i32 %call
+}
+
declare i32 @callee()
+declare void @callee2()
>From 2641fe82837455b422d6c8229cc2f3d3736de4da Mon Sep 17 00:00:00 2001
From: Igor Kudrin <ikudrin at accesssoftek.com>
Date: Thu, 16 May 2024 22:26:40 -0700
Subject: [PATCH 3/4] [AArch64][PAC] Fix creating check instructions for BBs
without an epilog
`AArch64PAuth::checkAuthenticatedRegister()` splits the basic block
containing the tail call instruction to add check instructions, assuming
at least one more instruction before the call. This assumption is
incorrect in cases where some execution paths lead to the termination
block without creating the stack frame. This patch rearranges the
creation of the checks so that the prior splitting is not required.
---
.../lib/Target/AArch64/AArch64PointerAuth.cpp | 23 ++++++-------------
1 file changed, 7 insertions(+), 16 deletions(-)
diff --git a/llvm/lib/Target/AArch64/AArch64PointerAuth.cpp b/llvm/lib/Target/AArch64/AArch64PointerAuth.cpp
index 90bf089dbebf7..60d3d533d9c10 100644
--- a/llvm/lib/Target/AArch64/AArch64PointerAuth.cpp
+++ b/llvm/lib/Target/AArch64/AArch64PointerAuth.cpp
@@ -257,21 +257,12 @@ void llvm::AArch64PAuth::checkAuthenticatedRegister(
// Control flow has to be changed, so arrange new MBBs.
- // At now, at least an AUT* instruction is expected before MBBI
- assert(MBBI != MBB.begin() &&
- "Cannot insert the check at the very beginning of MBB");
- // The block to insert check into.
- MachineBasicBlock *CheckBlock = &MBB;
- // The remaining part of the original MBB that is executed on success.
- MachineBasicBlock *SuccessBlock = MBB.splitAt(*std::prev(MBBI));
-
// The block that explicitly generates a break-point exception on failure.
MachineBasicBlock *BreakBlock =
MF.CreateMachineBasicBlock(MBB.getBasicBlock());
MF.push_back(BreakBlock);
- MBB.splitSuccessor(SuccessBlock, BreakBlock);
+ MBB.addSuccessor(BreakBlock);
- assert(CheckBlock->getFallThrough() == SuccessBlock);
BuildMI(BreakBlock, DL, TII->get(AArch64::BRK)).addImm(BrkImm);
switch (Method) {
@@ -279,11 +270,11 @@ void llvm::AArch64PAuth::checkAuthenticatedRegister(
case AuthCheckMethod::DummyLoad:
llvm_unreachable("Should be handled above");
case AuthCheckMethod::HighBitsNoTBI:
- BuildMI(CheckBlock, DL, TII->get(AArch64::EORXrs), TmpReg)
+ BuildMI(MBB, MBBI, DL, TII->get(AArch64::EORXrs), TmpReg)
.addReg(AuthenticatedReg)
.addReg(AuthenticatedReg)
.addImm(1);
- BuildMI(CheckBlock, DL, TII->get(AArch64::TBNZX))
+ BuildMI(MBB, MBBI, DL, TII->get(AArch64::TBNZX))
.addReg(TmpReg)
.addImm(62)
.addMBB(BreakBlock);
@@ -292,16 +283,16 @@ void llvm::AArch64PAuth::checkAuthenticatedRegister(
assert(AuthenticatedReg == AArch64::LR &&
"XPACHint mode is only compatible with checking the LR register");
assert(UseIKey && "XPACHint mode is only compatible with I-keys");
- BuildMI(CheckBlock, DL, TII->get(AArch64::ORRXrs), TmpReg)
+ BuildMI(MBB, MBBI, DL, TII->get(AArch64::ORRXrs), TmpReg)
.addReg(AArch64::XZR)
.addReg(AArch64::LR)
.addImm(0);
- BuildMI(CheckBlock, DL, TII->get(AArch64::XPACLRI));
- BuildMI(CheckBlock, DL, TII->get(AArch64::SUBSXrs), AArch64::XZR)
+ BuildMI(MBB, MBBI, DL, TII->get(AArch64::XPACLRI));
+ BuildMI(MBB, MBBI, DL, TII->get(AArch64::SUBSXrs), AArch64::XZR)
.addReg(TmpReg)
.addReg(AArch64::LR)
.addImm(0);
- BuildMI(CheckBlock, DL, TII->get(AArch64::Bcc))
+ BuildMI(MBB, MBBI, DL, TII->get(AArch64::Bcc))
.addImm(AArch64CC::NE)
.addMBB(BreakBlock);
return;
>From bc4eab673698bab493badb3efe5107916f1300a0 Mon Sep 17 00:00:00 2001
From: Igor Kudrin <ikudrin at accesssoftek.com>
Date: Thu, 23 May 2024 19:20:47 -0700
Subject: [PATCH 4/4] fixup: add an assert
---
llvm/lib/Target/AArch64/AArch64PointerAuth.cpp | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/llvm/lib/Target/AArch64/AArch64PointerAuth.cpp b/llvm/lib/Target/AArch64/AArch64PointerAuth.cpp
index 60d3d533d9c10..e900f6881620f 100644
--- a/llvm/lib/Target/AArch64/AArch64PointerAuth.cpp
+++ b/llvm/lib/Target/AArch64/AArch64PointerAuth.cpp
@@ -241,6 +241,14 @@ void llvm::AArch64PAuth::checkAuthenticatedRegister(
const AArch64InstrInfo *TII = Subtarget.getInstrInfo();
DebugLoc DL = MBBI->getDebugLoc();
+ // All terminator instructions should be grouped at the end of the machine
+ // basic block, with no non-terminator instructions between them. Depending on
+ // the method requested, we will insert some regular instructions, maybe
+ // followed by a conditional branch instruction, which is a terminator, before
+ // MBBI. Thus, MBBI is expected to be the first terminator of its MBB.
+ assert(MBBI->isTerminator() && MBBI == MBB.getFirstTerminator() &&
+ "MBBI should be the first terminator in MBB");
+
// First, handle the methods not requiring creating extra MBBs.
switch (Method) {
default:
More information about the llvm-commits
mailing list