Reland [BasicBlockUtils] Handle funclets when detaching EH pad blocks #159379
+305
−28
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
…llvm#157363) Fixes llvm#148052 . When removing EH Pad blocks, the value defined by them becomes poison. These poison values are then used by `catchret` and `cleanupret`, which is invalid. This commit replaces those unreachable `catchret` and `cleanupret` instructions with `unreachable`.
|
@llvm/pr-subscribers-llvm-transforms Author: Gábor Spaits (spaits) ChangesFixes #148052 . Last PR did not account for the scenario, when more than one instruction used the Here is the diff from the last version of this PR: #158435 diff --git a/llvm/lib/Transforms/Utils/BasicBlockUtils.cpp b/llvm/lib/Transforms/Utils/BasicBlockUtils.cpp
index 91e245e5e8f5..1dd8cb4ee584 100644
--- a/llvm/lib/Transforms/Utils/BasicBlockUtils.cpp
+++ b/llvm/lib/Transforms/Utils/BasicBlockUtils.cpp
@@ -106,7 +106,8 @@ void llvm::detachDeadBlocks(ArrayRef<BasicBlock *> BBs,
// first block, the we would have possible cleanupret and catchret
// instructions with poison arguments, which wouldn't be valid.
if (isa<FuncletPadInst>(I)) {
- for (User *User : make_early_inc_range(I.users())) {
+ SmallPtrSet<BasicBlock *, 4> UniqueEHRetBlocksToDelete;
+ for (User *User : I.users()) {
Instruction *ReturnInstr = dyn_cast<Instruction>(User);
// If we have a cleanupret or catchret block, replace it with just an
// unreachable. The other alternative, that may use a catchpad is a
@@ -114,33 +115,12 @@ void llvm::detachDeadBlocks(ArrayRef<BasicBlock *> BBs,
if (isa<CatchReturnInst>(ReturnInstr) ||
isa<CleanupReturnInst>(ReturnInstr)) {
BasicBlock *ReturnInstrBB = ReturnInstr->getParent();
- // This catchret or catchpad basic block is detached now. Let the
- // successors know it.
- // This basic block also may have some predecessors too. For
- // example the following LLVM-IR is valid:
- //
- // [cleanuppad_block]
- // |
- // [regular_block]
- // |
- // [cleanupret_block]
- //
- // The IR after the cleanup will look like this:
- //
- // [cleanuppad_block]
- // |
- // [regular_block]
- // |
- // [unreachable]
- //
- // So regular_block will lead to an unreachable block, which is also
- // valid. There is no need to replace regular_block with unreachable
- // in this context now.
- // On the other hand, the cleanupret/catchret block's successors
- // need to know about the deletion of their predecessors.
- emptyAndDetachBlock(ReturnInstrBB, Updates, KeepOneInputPHIs);
+ UniqueEHRetBlocksToDelete.insert(ReturnInstrBB);
}
}
+ for (BasicBlock *EHRetBB :
+ make_early_inc_range(UniqueEHRetBlocksToDelete))
+ emptyAndDetachBlock(EHRetBB, Updates, KeepOneInputPHIs);
}
}Full diff: https://github.com/llvm/llvm-project/pull/159379.diff 2 Files Affected:
diff --git a/llvm/lib/Transforms/Utils/BasicBlockUtils.cpp b/llvm/lib/Transforms/Utils/BasicBlockUtils.cpp
index cad0b4c12b54e..1dd8cb4ee584c 100644
--- a/llvm/lib/Transforms/Utils/BasicBlockUtils.cpp
+++ b/llvm/lib/Transforms/Utils/BasicBlockUtils.cpp
@@ -58,37 +58,74 @@ static cl::opt<unsigned> MaxDeoptOrUnreachableSuccessorCheckDepth(
"is followed by a block that either has a terminating "
"deoptimizing call or is terminated with an unreachable"));
-void llvm::detachDeadBlocks(
- ArrayRef<BasicBlock *> BBs,
- SmallVectorImpl<DominatorTree::UpdateType> *Updates,
- bool KeepOneInputPHIs) {
+/// Zap all the instructions in the block and replace them with an unreachable
+/// instruction and notify the basic block's successors that one of their
+/// predecessors is going away.
+static void
+emptyAndDetachBlock(BasicBlock *BB,
+ SmallVectorImpl<DominatorTree::UpdateType> *Updates,
+ bool KeepOneInputPHIs) {
+ // 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();
+ // 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.");
+}
+
+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});
+ 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
+ // basic blocks than the pad instruction. If we would only delete the
+ // first block, the we would have possible cleanupret and catchret
+ // instructions with poison arguments, which wouldn't be valid.
+ if (isa<FuncletPadInst>(I)) {
+ SmallPtrSet<BasicBlock *, 4> UniqueEHRetBlocksToDelete;
+ for (User *User : I.users()) {
+ Instruction *ReturnInstr = dyn_cast<Instruction>(User);
+ // If we have a cleanupret or catchret block, replace it with just an
+ // unreachable. The other alternative, that may use a catchpad is a
+ // catchswitch. That does not need special handling for now.
+ if (isa<CatchReturnInst>(ReturnInstr) ||
+ isa<CleanupReturnInst>(ReturnInstr)) {
+ BasicBlock *ReturnInstrBB = ReturnInstr->getParent();
+ UniqueEHRetBlocksToDelete.insert(ReturnInstrBB);
+ }
+ }
+ for (BasicBlock *EHRetBB :
+ make_early_inc_range(UniqueEHRetBlocksToDelete))
+ emptyAndDetachBlock(EHRetBB, Updates, KeepOneInputPHIs);
+ }
}
- // Zap all the instructions in the block.
- 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.");
+ // Detaching and emptying the current basic block.
+ emptyAndDetachBlock(BB, Updates, KeepOneInputPHIs);
}
}
diff --git a/llvm/test/Transforms/SimplifyCFG/unreachable-multi-basic-block-funclet.ll b/llvm/test/Transforms/SimplifyCFG/unreachable-multi-basic-block-funclet.ll
new file mode 100644
index 0000000000000..0f0fc78ec7add
--- /dev/null
+++ b/llvm/test/Transforms/SimplifyCFG/unreachable-multi-basic-block-funclet.ll
@@ -0,0 +1,236 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5
+; RUN: opt -passes=simplifycfg -S < %s | FileCheck %s
+
+; cleanuppad/cleanupret
+
+define void @unreachable_cleanuppad_linear(i64 %shapes.1) personality ptr null {
+; CHECK-LABEL: define void @unreachable_cleanuppad_linear(
+; CHECK-SAME: i64 [[SHAPES_1:%.*]]) personality ptr null {
+; CHECK-NEXT: [[START:.*:]]
+; CHECK-NEXT: [[_7:%.*]] = icmp ult i64 0, [[SHAPES_1]]
+; CHECK-NEXT: ret void
+;
+start:
+ %_7 = icmp ult i64 0, %shapes.1
+ ret void
+
+funclet:
+ %cleanuppad = cleanuppad within none []
+ br label %funclet_end
+
+funclet_end:
+ cleanupret from %cleanuppad unwind to caller
+}
+
+define void @unreachable_cleanuppad_linear_middle_block(i64 %shapes.1) personality ptr null {
+; CHECK-LABEL: define void @unreachable_cleanuppad_linear_middle_block(
+; CHECK-SAME: i64 [[SHAPES_1:%.*]]) personality ptr null {
+; CHECK-NEXT: [[START:.*:]]
+; CHECK-NEXT: [[_7:%.*]] = icmp ult i64 0, [[SHAPES_1]]
+; CHECK-NEXT: ret void
+;
+start:
+ %_7 = icmp ult i64 0, %shapes.1
+ ret void
+
+funclet:
+ %cleanuppad = cleanuppad within none []
+ br label %middle_block
+
+middle_block:
+ %tmp1 = add i64 %shapes.1, 42
+ br label %funclet_end
+
+funclet_end:
+ cleanupret from %cleanuppad unwind to caller
+}
+
+define void @unreachable_cleanuppad_multiple_predecessors(i64 %shapes.1) personality ptr null {
+; CHECK-LABEL: define void @unreachable_cleanuppad_multiple_predecessors(
+; CHECK-SAME: i64 [[SHAPES_1:%.*]]) personality ptr null {
+; CHECK-NEXT: [[START:.*:]]
+; CHECK-NEXT: [[_7:%.*]] = icmp ult i64 0, [[SHAPES_1]]
+; CHECK-NEXT: ret void
+;
+start:
+ %_7 = icmp ult i64 0, %shapes.1
+ ret void
+
+funclet:
+ %cleanuppad = cleanuppad within none []
+ switch i64 %shapes.1, label %otherwise [ i64 0, label %one
+ i64 1, label %two
+ i64 42, label %three ]
+one:
+ br label %funclet_end
+
+two:
+ br label %funclet_end
+
+three:
+ br label %funclet_end
+
+otherwise:
+ br label %funclet_end
+
+funclet_end:
+ cleanupret from %cleanuppad unwind to caller
+}
+
+; catchpad/catchret
+
+define void @unreachable_catchpad_linear(i64 %shapes.1) personality ptr null {
+; CHECK-LABEL: define void @unreachable_catchpad_linear(
+; CHECK-SAME: i64 [[SHAPES_1:%.*]]) personality ptr null {
+; CHECK-NEXT: [[START:.*:]]
+; CHECK-NEXT: [[_7:%.*]] = icmp ult i64 0, [[SHAPES_1]]
+; CHECK-NEXT: ret void
+;
+start:
+ %_7 = icmp ult i64 0, %shapes.1
+ ret void
+
+dispatch:
+ %cs = catchswitch within none [label %funclet] unwind to caller
+
+funclet:
+ %cleanuppad = catchpad within %cs []
+ br label %funclet_end
+
+
+funclet_end:
+ catchret from %cleanuppad to label %unreachable
+
+unreachable:
+ unreachable
+}
+
+define void @unreachable_catchpad_multiple_predecessors(i64 %shapes.1) personality ptr null {
+; CHECK-LABEL: define void @unreachable_catchpad_multiple_predecessors(
+; CHECK-SAME: i64 [[SHAPES_1:%.*]]) personality ptr null {
+; CHECK-NEXT: [[START:.*:]]
+; CHECK-NEXT: [[_7:%.*]] = icmp ult i64 0, [[SHAPES_1]]
+; CHECK-NEXT: ret void
+;
+start:
+ %_7 = icmp ult i64 0, %shapes.1
+ ret void
+
+dispatch:
+ %cs = catchswitch within none [label %funclet] unwind to caller
+
+funclet:
+ %cleanuppad = catchpad within %cs []
+ switch i64 %shapes.1, label %otherwise [ i64 0, label %one
+ i64 1, label %two
+ i64 42, label %three ]
+one:
+ br label %funclet_end
+
+two:
+ br label %funclet_end
+
+three:
+ br label %funclet_end
+
+otherwise:
+ br label %funclet_end
+
+funclet_end:
+ catchret from %cleanuppad to label %unreachable
+
+unreachable:
+ unreachable
+}
+
+; Issue reproducer
+
+define void @gh148052(i64 %shapes.1) personality ptr null {
+; CHECK-LABEL: define void @gh148052(
+; CHECK-SAME: i64 [[SHAPES_1:%.*]]) personality ptr null {
+; CHECK-NEXT: [[START:.*:]]
+; CHECK-NEXT: [[_7:%.*]] = icmp ult i64 0, [[SHAPES_1]]
+; CHECK-NEXT: call void @llvm.assume(i1 [[_7]])
+; CHECK-NEXT: ret void
+;
+start:
+ %_7 = icmp ult i64 0, %shapes.1
+ br i1 %_7, label %bb1, label %panic
+
+bb1:
+ %_11 = icmp ult i64 0, %shapes.1
+ br i1 %_11, label %bb3, label %panic1
+
+panic:
+ unreachable
+
+bb3:
+ ret void
+
+panic1:
+ invoke void @func(i64 0, i64 0, ptr null)
+ to label %unreachable unwind label %funclet_bb14
+
+funclet_bb14:
+ %cleanuppad = cleanuppad within none []
+ br label %bb13
+
+unreachable:
+ unreachable
+
+bb10:
+ cleanupret from %cleanuppad5 unwind to caller
+
+funclet_bb10:
+ %cleanuppad5 = cleanuppad within none []
+ br label %bb10
+
+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 {
+; CHECK-LABEL: define x86_thiscallcc ptr @baz(
+; CHECK-SAME: ptr [[ARG:%.*]], ptr [[ARG1:%.*]], ptr [[ARG2:%.*]], i1 [[ARG3:%.*]], ptr [[ARG4:%.*]]) personality ptr null {
+; CHECK-NEXT: [[BB:.*:]]
+; CHECK-NEXT: [[ALLOCA:%.*]] = alloca [2 x %struct.foo], align 4
+; CHECK-NEXT: [[INVOKE:%.*]] = call x86_thiscallcc ptr @quux(ptr null, ptr null, i32 0) #[[ATTR1:[0-9]+]]
+; CHECK-NEXT: unreachable
+;
+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)
|
|
@rnk I am sorry, that my last PR didn't worked and sorry for wasting your time and requesting your review once again. I have done some testing with the Address Sanitizer on the tests that failed on the buildbot. They work now. I couldn't run |
rnk
approved these changes
Sep 17, 2025
Yeah, LLVM's build and test requirements continue to go up and to the right. It is a real barrier to contribution, as Nuno highlighted the other week. I think our default build and test configuration could use a bit more curation, like paring back the OCaml binding tests. Those are integration tests that are likely to fail out of the box in new environments, and they probably shouldn't be in |
…sors to an outer scope
|
Thank you @rnk .
Update: I have run the tests on all available targets with ASan and they are passing. |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/138/builds/19359 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/65/builds/22898 Here is the relevant piece of the build log for the reference |
SeongjaeP
pushed a commit
to SeongjaeP/llvm-project
that referenced
this pull request
Sep 23, 2025
…llvm#159379) Fixes llvm#148052 . Last PR did not account for the scenario, when more than one instruction used the `catchpad` label. In that case I have deleted uses, which were already "choosen to be iterated over" by the early increment iterator. This issue was not visible in normal release build on x86, but luckily later on the address sanitizer build it has found it on the buildbot. Here is the diff from the last version of this PR: llvm#158435 ```diff diff --git a/llvm/lib/Transforms/Utils/BasicBlockUtils.cpp b/llvm/lib/Transforms/Utils/BasicBlockUtils.cpp index 91e245e..1dd8cb4 100644 --- a/llvm/lib/Transforms/Utils/BasicBlockUtils.cpp +++ b/llvm/lib/Transforms/Utils/BasicBlockUtils.cpp @@ -106,7 +106,8 @@ void llvm::detachDeadBlocks(ArrayRef<BasicBlock *> BBs, // first block, the we would have possible cleanupret and catchret // instructions with poison arguments, which wouldn't be valid. if (isa<FuncletPadInst>(I)) { - for (User *User : make_early_inc_range(I.users())) { + SmallPtrSet<BasicBlock *, 4> UniqueEHRetBlocksToDelete; + for (User *User : I.users()) { Instruction *ReturnInstr = dyn_cast<Instruction>(User); // If we have a cleanupret or catchret block, replace it with just an // unreachable. The other alternative, that may use a catchpad is a @@ -114,33 +115,12 @@ void llvm::detachDeadBlocks(ArrayRef<BasicBlock *> BBs, if (isa<CatchReturnInst>(ReturnInstr) || isa<CleanupReturnInst>(ReturnInstr)) { BasicBlock *ReturnInstrBB = ReturnInstr->getParent(); - // This catchret or catchpad basic block is detached now. Let the - // successors know it. - // This basic block also may have some predecessors too. For - // example the following LLVM-IR is valid: - // - // [cleanuppad_block] - // | - // [regular_block] - // | - // [cleanupret_block] - // - // The IR after the cleanup will look like this: - // - // [cleanuppad_block] - // | - // [regular_block] - // | - // [unreachable] - // - // So regular_block will lead to an unreachable block, which is also - // valid. There is no need to replace regular_block with unreachable - // in this context now. - // On the other hand, the cleanupret/catchret block's successors - // need to know about the deletion of their predecessors. - emptyAndDetachBlock(ReturnInstrBB, Updates, KeepOneInputPHIs); + UniqueEHRetBlocksToDelete.insert(ReturnInstrBB); } } + for (BasicBlock *EHRetBB : + make_early_inc_range(UniqueEHRetBlocksToDelete)) + emptyAndDetachBlock(EHRetBB, Updates, KeepOneInputPHIs); } } ```
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 #148052 .
Last PR did not account for the scenario, when more than one instruction used the
catchpadlabel.In that case I have deleted uses, which were already "choosen to be iterated over" by the early increment iterator. This issue was not visible in normal release build on x86, but luckily later on the address sanitizer build it has found it on the buildbot.
Here is the diff from the last version of this PR: #158435