[llvm] [BasicBlockUtils] Fix funclet deletion crash (PR #158435)
Gábor Spaits via llvm-commits
llvm-commits at lists.llvm.org
Sun Sep 14 03:06:01 PDT 2025
https://github.com/spaits updated https://github.com/llvm/llvm-project/pull/158435
>From 55b7d84e1252eea0c11ba1ac0f445f2e4d5ca862 Mon Sep 17 00:00:00 2001
From: Gabor Spaits <gaborspaits1 at gmail.com>
Date: Sat, 13 Sep 2025 20:20:07 +0200
Subject: [PATCH 1/2] [BasicBlockUtils] Fix funclet deletion crash
This is a fix for a crash reported under PR #157363 that fixes issue #148052.
This PR contains two things:
- Properly detaching and emptying the catchret/cleanupret block.
- Not handling catchswitches anymore.
---
llvm/lib/Transforms/Utils/BasicBlockUtils.cpp | 104 +++++++++---------
.../unreachable-multi-basic-block-funclet.ll | 37 +++++++
2 files changed, 88 insertions(+), 53 deletions(-)
diff --git a/llvm/lib/Transforms/Utils/BasicBlockUtils.cpp b/llvm/lib/Transforms/Utils/BasicBlockUtils.cpp
index d2391e166f942..42b06679840b1 100644
--- a/llvm/lib/Transforms/Utils/BasicBlockUtils.cpp
+++ b/llvm/lib/Transforms/Utils/BasicBlockUtils.cpp
@@ -58,36 +58,64 @@ static cl::opt<unsigned> MaxDeoptOrUnreachableSuccessorCheckDepth(
"is followed by a block that either has a terminating "
"deoptimizing call or is terminated with an unreachable"));
-static void replaceFuncletPadsRetWithUnreachable(Instruction &I) {
+static void zapAllInstructionInDeadBasicBlock(BasicBlock *BB) {
+ while (!BB->empty()) {
+ Instruction &I = BB->back();
+ // If this instruction is used, replace uses with an arbitrary value.
+ // Because control flow can't get here, we don't care what we replace the
+ // value with. Note that since this block is unreachable, and all values
+ // contained within it must dominate their uses, that all uses will
+ // eventually be removed (they are themselves dead).
+ if (!I.use_empty())
+ I.replaceAllUsesWith(PoisonValue::get(I.getType()));
+ BB->back().eraseFromParent();
+ }
+ new UnreachableInst(BB->getContext(), BB);
+ assert(BB->size() == 1 && isa<UnreachableInst>(BB->getTerminator()) &&
+ "The successor list of BB isn't empty before "
+ "applying corresponding DTU updates.");
+}
+
+static void deleteBasicBlockFromSuccessor(
+ BasicBlock *BB, SmallVectorImpl<DominatorTree::UpdateType> *Updates,
+ bool KeepOneInputPHIs) {
+ SmallPtrSet<BasicBlock *, 4> UniqueSuccessors;
+ for (BasicBlock *Succ : successors(BB)) {
+ Succ->removePredecessor(BB, KeepOneInputPHIs);
+ if (Updates && UniqueSuccessors.insert(Succ).second)
+ Updates->push_back({DominatorTree::Delete, BB, Succ});
+ }
+}
+
+static void replaceFuncletPadsRetWithUnreachable(
+ Instruction &I, SmallVectorImpl<DominatorTree::UpdateType> *Updates,
+ bool KeepOneInputPHIs) {
assert(isa<FuncletPadInst>(I) && "Instruction must be a funclet pad!");
for (User *User : make_early_inc_range(I.users())) {
Instruction *ReturnInstr = dyn_cast<Instruction>(User);
+ // If we have a cleanupret or catchret block, replace it with just an
+ // unreachable.
if (isa<CatchReturnInst>(ReturnInstr) ||
isa<CleanupReturnInst>(ReturnInstr)) {
BasicBlock *ReturnInstrBB = ReturnInstr->getParent();
- ReturnInstr->eraseFromParent();
- new UnreachableInst(ReturnInstrBB->getContext(), ReturnInstrBB);
+ // This catchret or catchpad basic block is detached now. Let the
+ // successors know it.
+ deleteBasicBlockFromSuccessor(ReturnInstrBB, Updates, KeepOneInputPHIs);
+ zapAllInstructionInDeadBasicBlock(ReturnInstrBB);
}
}
}
-void llvm::detachDeadBlocks(
- ArrayRef<BasicBlock *> BBs,
- SmallVectorImpl<DominatorTree::UpdateType> *Updates,
- bool KeepOneInputPHIs) {
+void llvm::detachDeadBlocks(ArrayRef<BasicBlock *> BBs,
+ SmallVectorImpl<DominatorTree::UpdateType> *Updates,
+ bool KeepOneInputPHIs) {
for (auto *BB : BBs) {
// Loop through all of our successors and make sure they know that one
// of their predecessors is going away.
SmallPtrSet<BasicBlock *, 4> UniqueSuccessors;
- for (BasicBlock *Succ : successors(BB)) {
- Succ->removePredecessor(BB, KeepOneInputPHIs);
- if (Updates && UniqueSuccessors.insert(Succ).second)
- Updates->push_back({DominatorTree::Delete, BB, Succ});
- }
-
- // Zap all the instructions in the block.
- while (!BB->empty()) {
- Instruction &I = BB->back();
+ auto NonFirstPhiIt = BB->getFirstNonPHIIt();
+ if (NonFirstPhiIt != BB->end()) {
+ Instruction &I = *NonFirstPhiIt;
// Exception handling funclets need to be explicitly addressed.
// These funclets must begin with cleanuppad or catchpad and end with
// cleanupred or catchret. The return instructions can be in different
@@ -95,42 +123,12 @@ void llvm::detachDeadBlocks(
// first block, the we would have possible cleanupret and catchret
// instructions with poison arguments, which wouldn't be valid.
if (isa<FuncletPadInst>(I))
- replaceFuncletPadsRetWithUnreachable(I);
-
- // Catchswitch instructions have handlers, that must be catchpads and
- // an unwind label, that is either a catchpad or catchswitch.
- if (CatchSwitchInst *CSI = dyn_cast<CatchSwitchInst>(&I)) {
- // Iterating over the handlers and the unwind basic block and processing
- // catchpads. If the unwind label is a catchswitch, we just replace the
- // label with poison later on.
- for (unsigned I = 0; I < CSI->getNumSuccessors(); I++) {
- BasicBlock *SucBlock = CSI->getSuccessor(I);
- Instruction &SucFstInst = *(SucBlock->getFirstNonPHIIt());
- if (isa<FuncletPadInst>(SucFstInst)) {
- replaceFuncletPadsRetWithUnreachable(SucFstInst);
- // There may be catchswitch instructions using the catchpad.
- // Just replace those with poison.
- if (!SucFstInst.use_empty())
- SucFstInst.replaceAllUsesWith(
- PoisonValue::get(SucFstInst.getType()));
- SucFstInst.eraseFromParent();
- }
- }
- }
-
- // Because control flow can't get here, we don't care what we replace the
- // value with. Note that since this block is unreachable, and all values
- // contained within it must dominate their uses, that all uses will
- // eventually be removed (they are themselves dead).
- if (!I.use_empty())
- I.replaceAllUsesWith(PoisonValue::get(I.getType()));
- BB->back().eraseFromParent();
+ replaceFuncletPadsRetWithUnreachable(I, Updates, KeepOneInputPHIs);
}
- new UnreachableInst(BB->getContext(), BB);
- assert(BB->size() == 1 &&
- isa<UnreachableInst>(BB->getTerminator()) &&
- "The successor list of BB isn't empty before "
- "applying corresponding DTU updates.");
+
+ // Detaching and emptying the current basic block.
+ deleteBasicBlockFromSuccessor(BB, Updates, KeepOneInputPHIs);
+ zapAllInstructionInDeadBasicBlock(BB);
}
}
@@ -1704,8 +1702,8 @@ BranchInst *llvm::GetIfCondition(BasicBlock *BB, BasicBlock *&IfTrue,
// We can only handle branches. Other control flow will be lowered to
// branches if possible anyway.
- BranchInst *Pred1Br = dyn_cast<BranchInst>(Pred1->getTerminator());
- BranchInst *Pred2Br = dyn_cast<BranchInst>(Pred2->getTerminator());
+ BranchInst *Pred1Br = dyn_cast_or_null<BranchInst>(Pred1->getTerminator());
+ BranchInst *Pred2Br = dyn_cast_or_null<BranchInst>(Pred2->getTerminator());
if (!Pred1Br || !Pred2Br)
return nullptr;
diff --git a/llvm/test/Transforms/SimplifyCFG/unreachable-multi-basic-block-funclet.ll b/llvm/test/Transforms/SimplifyCFG/unreachable-multi-basic-block-funclet.ll
index d2fccae6770db..d915b0c5272ee 100644
--- a/llvm/test/Transforms/SimplifyCFG/unreachable-multi-basic-block-funclet.ll
+++ b/llvm/test/Transforms/SimplifyCFG/unreachable-multi-basic-block-funclet.ll
@@ -166,4 +166,41 @@ bb13:
cleanupret from %cleanuppad unwind label %funclet_bb10
}
+%struct.foo = type { ptr, %struct.eggs, ptr }
+%struct.eggs = type { ptr, ptr, ptr }
+
+declare x86_thiscallcc ptr @quux(ptr, ptr, i32)
+
+define x86_thiscallcc ptr @baz(ptr %arg, ptr %arg1, ptr %arg2, i1 %arg3, ptr %arg4) personality ptr null {
+bb:
+ %alloca = alloca [2 x %struct.foo], align 4
+ %invoke = invoke x86_thiscallcc ptr @quux(ptr null, ptr null, i32 0)
+ to label %bb5 unwind label %bb10
+
+bb5: ; preds = %bb
+ %getelementptr = getelementptr i8, ptr %arg, i32 20
+ %call = call x86_thiscallcc ptr null(ptr null, ptr null, i32 0)
+ br label %bb6
+
+bb6: ; preds = %bb10, %bb5
+ %phi = phi ptr [ null, %bb10 ], [ null, %bb5 ]
+ ret ptr %phi
+
+bb7: ; No predecessors!
+ %cleanuppad = cleanuppad within none []
+ %getelementptr8 = getelementptr i8, ptr %arg2, i32 -20
+ %icmp = icmp eq ptr %arg, null
+ br label %bb9
+
+bb9: ; preds = %bb7
+ cleanupret from %cleanuppad unwind label %bb10
+
+bb10: ; preds = %bb9, %bb
+ %phi11 = phi ptr [ %arg, %bb9 ], [ null, %bb ]
+ %cleanuppad12 = cleanuppad within none []
+ %getelementptr13 = getelementptr i8, ptr %phi11, i32 -20
+ store i32 0, ptr %phi11, align 4
+ br label %bb6
+}
+
declare void @func(i64, i64, ptr)
>From 914670bfc9abe4b2167291e2e83121844c1ade6e Mon Sep 17 00:00:00 2001
From: Gabor Spaits <gaborspaits1 at gmail.com>
Date: Sun, 14 Sep 2025 12:05:46 +0200
Subject: [PATCH 2/2] Remove unnecessary change
---
llvm/lib/Transforms/Utils/BasicBlockUtils.cpp | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/llvm/lib/Transforms/Utils/BasicBlockUtils.cpp b/llvm/lib/Transforms/Utils/BasicBlockUtils.cpp
index 42b06679840b1..69d530b3cf52c 100644
--- a/llvm/lib/Transforms/Utils/BasicBlockUtils.cpp
+++ b/llvm/lib/Transforms/Utils/BasicBlockUtils.cpp
@@ -1702,8 +1702,8 @@ BranchInst *llvm::GetIfCondition(BasicBlock *BB, BasicBlock *&IfTrue,
// We can only handle branches. Other control flow will be lowered to
// branches if possible anyway.
- BranchInst *Pred1Br = dyn_cast_or_null<BranchInst>(Pred1->getTerminator());
- BranchInst *Pred2Br = dyn_cast_or_null<BranchInst>(Pred2->getTerminator());
+ BranchInst *Pred1Br = dyn_cast<BranchInst>(Pred1->getTerminator());
+ BranchInst *Pred2Br = dyn_cast<BranchInst>(Pred2->getTerminator());
if (!Pred1Br || !Pred2Br)
return nullptr;
More information about the llvm-commits
mailing list