-
Notifications
You must be signed in to change notification settings - Fork 14.7k
[AArch64][BTI] Mark EH landing pads as jump targets #149680
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
8ea51a3
to
baf4849
Compare
✅ With the latest revision this PR passed the undef deprecator. |
Landing pads reached by the unwinder are entered via 'br', so they must start with BTI j when -mbranch-protection requests BTI. Add isEHPad() to the jump-target test. Size/perf: +4 B per pad only when BTI is enabled. Test: new CodeGen/AArch64/bti-ehpad.ll. Signed-off-by: Shashi Shankar <[email protected]>
baf4849
to
38698e6
Compare
@llvm/pr-subscribers-backend-aarch64 Author: Shashi Shankar (shashforge) ChangesClang wasn’t putting a BTI “jump” hint at the start of C++ catch/cleanup blocks. Fix
Impact
Test New lit test bti-ehpad.ll checks that the landing pad starts with bti. No other code paths touched; full Fixes #149267 Full diff: https://github.com/llvm/llvm-project/pull/149680.diff 2 Files Affected:
diff --git a/llvm/lib/Target/AArch64/AArch64BranchTargets.cpp b/llvm/lib/Target/AArch64/AArch64BranchTargets.cpp
index 3436dc9ef4521..1999195051aa5 100644
--- a/llvm/lib/Target/AArch64/AArch64BranchTargets.cpp
+++ b/llvm/lib/Target/AArch64/AArch64BranchTargets.cpp
@@ -100,7 +100,7 @@ bool AArch64BranchTargets::runOnMachineFunction(MachineFunction &MF) {
// If the block itself is address-taken, it could be indirectly branched
// to, but not called.
if (MBB.isMachineBlockAddressTaken() || MBB.isIRBlockAddressTaken() ||
- JumpTableTargets.count(&MBB))
+ JumpTableTargets.count(&MBB) || MBB.isEHPad())
CouldJump = true;
if (CouldCall || CouldJump) {
diff --git a/llvm/test/CodeGen/AArch64/bti-ehpad.ll b/llvm/test/CodeGen/AArch64/bti-ehpad.ll
new file mode 100644
index 0000000000000..70e43ff5c5d5d
--- /dev/null
+++ b/llvm/test/CodeGen/AArch64/bti-ehpad.ll
@@ -0,0 +1,26 @@
+; llvm/test/CodeGen/AArch64/bti-ehpad.ll
+; REQUIRES: aarch64-registered-target
+; RUN: llc -mtriple=aarch64-none-linux-gnu %s -o - | FileCheck %s
+
+declare i32 @__gxx_personality_v0(...)
+
+define void @test() #0 personality ptr @__gxx_personality_v0 {
+entry:
+ invoke void @may_throw()
+ to label %ret unwind label %lpad
+lpad: ; catch.dispatch
+ landingpad { ptr, i32 }
+ cleanup
+ ret void
+ret:
+ ret void
+}
+
+declare void @may_throw()
+
+attributes #0 = { "branch-target-enforcement"="true" }
+
+; Function needs both the architectural feature *and* the enforcement request.
+attributes #0 = { "branch-target-enforcement"="true" "target-features"="+bti" }
+
+; CHECK: bti
|
Thanks ! I'll will verify locally as well and report back |
So I tested a quick backport to llvm15 (default on AL2023) and it didn't work:
That said I might have made a mistake, juggling too many plates today. I'll test on Fedora's llvm20 asap and check llvm15 again & will report. |
So it looks like (with clang15) the |
Same with llvm20 on Fedora:
After:
But it's still crashing. I've confirmed that libgcc branches past the
|
Additionally with that patch, clang20 build on f42 fails with
Is there another change elsewhere that also needs to be backported or is this fix simply incomplete ? Next I will try to build all of llvm from git, but that will require more time. |
Probably the bti instruction is getting inserted before the EH_LABEL pseudo-instruction. |
So bear with me, I have no idea what I'm doing, never looked at LLVM code that deep, and this is probably completely wrong ... but hacking
Seems to do the trick :-) I don't know how the |
That's basically correct. I think you only want to skip past the EH_LABEL if you're handling an EHPad, though: in other cases EH_LABEL is likely tied to a subsequent call instruction you don't want to mess with. You shouldn't need any EH_LABEL handling with HasWinCFI; Windows uses funclets, and funclets don't start with EH_LABEL. Maybe worth adding a testcase for Windows if you don't mind. (Don't use the same IR, though; exception handling IR is very different. You can use something like |
@ozbenh Great catch — the unwinder’s LSDA symbol is that leading EH_LABEL, so inserting the hint before it meant the indirect branch still landed after the BTI. I’ve adopted exactly your idea: for EH pads we now walk past any initial EH_LABELs before emitting the hint. Thanks again for digging into this! |
@efriedma-quic > Totally agree this should only happen for EH pads. Appreciate the detailed guidance — thank you! |
f68561b
to
0b972ee
Compare
You can test this locally with the following command:git-clang-format --diff HEAD~1 HEAD --extensions cpp -- llvm/lib/Target/AArch64/AArch64BranchTargets.cpp View the diff from clang-format here.diff --git a/llvm/lib/Target/AArch64/AArch64BranchTargets.cpp b/llvm/lib/Target/AArch64/AArch64BranchTargets.cpp
index e98044712..24419e47f 100644
--- a/llvm/lib/Target/AArch64/AArch64BranchTargets.cpp
+++ b/llvm/lib/Target/AArch64/AArch64BranchTargets.cpp
@@ -149,12 +149,10 @@ void AArch64BranchTargets::addBTI(MachineBasicBlock &MBB, bool CouldCall,
MBBI = MBB.begin();
if (MBB.isEHPad()) {
- while (MBBI != MBB.end() &&
- MBBI->getOpcode() == TargetOpcode::EH_LABEL)
+ while (MBBI != MBB.end() && MBBI->getOpcode() == TargetOpcode::EH_LABEL)
++MBBI;
}
- BuildMI(MBB, MBBI, MBB.findDebugLoc(MBBI),
- TII->get(AArch64::HINT))
+ BuildMI(MBB, MBBI, MBB.findDebugLoc(MBBI), TII->get(AArch64::HINT))
.addImm(HintNum);
}
|
Did you forget to add bti-funclet-windows.ll to the latest commit? |
It in progress currently it failing..I am on it . It look like BTI is disabled for Windows. I am on it Failed reason
|
My |
he guard on MBB.isEHPad() ensures we only advance the iterator when we’re in an EH pad, so for a normal basic block the loop/while immediately terminates and MBBI remains at MBB.begin(). |
Sure, what I meant is that my original proposed implementation did that without the extra if() by simply putting the test for MBB.isEHPad() in the loop condition. So the loop wouldn't have incremented the iterator for a non-EHPad either. But I don't care either way as long as the end result works :) The only other difference with your version is that I would look for an EH_LABEL anywhere in the MBB while your variant only works if the EH_LABEL is at the beginning. I don't know if this matters (could there be some other pseudo-op before the EH_LABEL ?), I don't now enough about llvm here. |
@shashforge What's the next step here ? |
attributes #0 = { "branch-target-enforcement"="true" } | ||
|
||
; Function needs both the architectural feature *and* the enforcement request. | ||
attributes #0 = { "branch-target-enforcement"="true" "target-features"="+bti" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't define attribute #0
twice?
; llvm/test/CodeGen/AArch64/bti-ehpad.ll | ||
; REQUIRES: aarch64-registered-target |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove these, the REQUIRES is handled by the folder.
Also it looks like it would be worth running update_llc_test_checks.py to generate a good set of test checks.
Clang wasn’t putting a BTI “jump” hint at the start of C++ catch/cleanup blocks.
With GCC 14’s runtime, those pads are entered via an indirect br, and BTI-enforced kernels kill the program with SIGILL.
Fix
Treat every isEHPad() block as a possible indirect-jump target in AArch64BranchTargets.cpp, so the existing pass adds bti j.
Impact
Test
New lit test bti-ehpad.ll checks that the landing pad starts with bti.
No other code paths touched; full
check-all
passes.Fixes #149267