[BasicBlockUtils] Handle funclets when detaching EH pad blocks #157363

spaits · 2025-09-07T20:54:44Z

The issue reproducer: https://godbolt.org/z/njT5ET3dj

The problem in the issue was that, SimplifyCFG deemed the basic block (bb1) unnecessary, that would invoke the excpetion (under basic block panic1). I think SimplifyCFG is right to deem it unnecessary.

This makes a few basic blocks unreachable including the one containing the cleanuppad instruction. The "result" of the cleanuppad is replaced with poison, it is used for the cleanupret. I don't think it is valid to use a poison value for cleanupret. See https://llvm.org/docs/LangRef.html#cleanupret-instruction :

The ‘cleanupret’ instruction requires one argument, which indicates which cleanuppad it exits, and must be a cleanuppad.

This PR disables the deletion of cleanuppad basic blocks, if the cleanupret basic block is in another block.

I am open to implement any idea if you have a more generic solution in mind.

llvmbot · 2025-09-07T20:55:14Z

@llvm/pr-subscribers-llvm-ir

@llvm/pr-subscribers-llvm-transforms

Author: Gábor Spaits (spaits)

Changes

Fixes #148052 .

The issue reproducer: https://godbolt.org/z/njT5ET3dj

The problem in the issue was that, SimplifyCFG deemed the basic block (bb1) unnecessary, that would invoke the excpetion (under basic block panic1). I think SimplifyCFG is right to deem it unnecessary.

This makes a few basic blocks unreachable including the one containing the cleanuppad instruction. The "result" of the cleanuppad is replaced with poison, it is used for the cleanupret. I don't think it is valid to use a poison value for cleanupret. See https://llvm.org/docs/LangRef.html#cleanupret-instruction :

The ‘cleanupret’ instruction requires one argument, which indicates which cleanuppad it exits, and must be a cleanuppad.

This PR disables the deletion of cleanuppad basic blocks, if the cleanupret basic block is in another block.

Full diff: https://github.com/llvm/llvm-project/pull/157363.diff

4 Files Affected:

(modified) llvm/include/llvm/IR/BasicBlock.h (+1)
(modified) llvm/lib/IR/BasicBlock.cpp (+4)
(modified) llvm/lib/Transforms/Utils/SimplifyCFG.cpp (+3-2)
(added) llvm/test/Transforms/SimplifyCFG/keep-non-linear-cleanuppad.ll (+48)

diff --git a/llvm/include/llvm/IR/BasicBlock.h b/llvm/include/llvm/IR/BasicBlock.h
index 533808e0666d5..8c831d8380bbb 100644
--- a/llvm/include/llvm/IR/BasicBlock.h
+++ b/llvm/include/llvm/IR/BasicBlock.h
@@ -705,6 +705,7 @@ class BasicBlock final : public Value, // Basic blocks are data objects also
 
   /// Return true if this basic block is an exception handling block.
   bool isEHPad() const { return getFirstNonPHIIt()->isEHPad(); }
+  bool isEHPadWithReturn() const;
 
   /// Return true if this basic block is a landing pad.
   ///
diff --git a/llvm/lib/IR/BasicBlock.cpp b/llvm/lib/IR/BasicBlock.cpp
index 3642e935397cb..51b303d36e862 100644
--- a/llvm/lib/IR/BasicBlock.cpp
+++ b/llvm/lib/IR/BasicBlock.cpp
@@ -658,6 +658,10 @@ void BasicBlock::replaceSuccessorsPhiUsesWith(BasicBlock *New) {
   this->replaceSuccessorsPhiUsesWith(this, New);
 }
 
+bool BasicBlock::isEHPadWithReturn() const {
+  return isEHPad() && isa<CleanupReturnInst>(getTerminator());
+}
+
 bool BasicBlock::isLandingPad() const {
   return isa<LandingPadInst>(getFirstNonPHIIt());
 }
diff --git a/llvm/lib/Transforms/Utils/SimplifyCFG.cpp b/llvm/lib/Transforms/Utils/SimplifyCFG.cpp
index 055e8cadaab76..ae55cee02ecb6 100644
--- a/llvm/lib/Transforms/Utils/SimplifyCFG.cpp
+++ b/llvm/lib/Transforms/Utils/SimplifyCFG.cpp
@@ -8340,8 +8340,9 @@ bool SimplifyCFGOpt::simplifyOnce(BasicBlock *BB) {
 
   // Remove basic blocks that have no predecessors (except the entry block)...
   // or that just have themself as a predecessor.  These are unreachable.
-  if ((pred_empty(BB) && BB != &BB->getParent()->getEntryBlock()) ||
-      BB->getSinglePredecessor() == BB) {
+  if (((pred_empty(BB) && BB != &BB->getParent()->getEntryBlock()) ||
+       BB->getSinglePredecessor() == BB) &&
+      (!BB->isEHPad() || BB->isEHPadWithReturn())) {
     LLVM_DEBUG(dbgs() << "Removing BB: \n" << *BB);
     DeleteDeadBlock(BB, DTU);
     return true;
diff --git a/llvm/test/Transforms/SimplifyCFG/keep-non-linear-cleanuppad.ll b/llvm/test/Transforms/SimplifyCFG/keep-non-linear-cleanuppad.ll
new file mode 100644
index 0000000000000..e91df1b903037
--- /dev/null
+++ b/llvm/test/Transforms/SimplifyCFG/keep-non-linear-cleanuppad.ll
@@ -0,0 +1,48 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5
+; RUN: opt -passes=simplifycfg -S < %s | FileCheck %s
+
+define void @_RINvCs3LQYlzOoal9_5repro13repro_genericNtB2_10ReproShapeEB2_(i64 %shapes.1) personality ptr null {
+; CHECK-LABEL: define void @_RINvCs3LQYlzOoal9_5repro13repro_genericNtB2_10ReproShapeEB2_(
+; 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 @_RNvNtCshEuXqvZjEXJ_4core9panicking18panic_bounds_check(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
+}
+
+declare void @_RNvNtCshEuXqvZjEXJ_4core9panicking18panic_bounds_check(i64, i64, ptr)

llvm/lib/Transforms/Utils/SimplifyCFG.cpp

llvm/include/llvm/IR/Instruction.h

…ble instruction

spaits · 2025-09-09T20:11:52Z

I have checked the failing tests in the CI and I have found something pretty interesting.

It is not caused by the PR, it is rather "found" by it.
These tests have failed, when I have deleted instructions, that used catchpads and weren't catchrets.

CodeGen/WebAssembly/cfg-stackify-eh-legacy.ll
CodeGen/WebAssembly/cfg-stackify-eh.ll
Transforms/Coroutines/coro-catchswitch-cleanuppad.ll

For example in CodeGen/WebAssembly/cfg-stackify-eh-legacy.ll the IR is in a state, where we have an instruction like bellow:

%h1 = catchpad within poison [ptr null, i32 64, ptr null]

So it may happen, that the catchpad's within argument is a poison due to a deleted catchswitch block. According to the language reference it shouldn't be that (https://llvm.org/docs/LangRef.html#id355).

The IR never leaves any pass in that state.

It doesn't seem cause any problems for now. Addressing catchswitch would have some additional costs. We would have to iterate over the handlers, and then process each catchpad/catchret pairs like we do now in the current state of the PR.

So summarized, it would be more pragmatic, but more expensive, while the current version also works, but the IR may be in inconsistent state (not caused by this PR, it was already in that state). This state has never caused any problem and will not cause any problem after this ptach.

llvm/lib/Transforms/Utils/BasicBlockUtils.cpp

github-actions · 2025-09-09T20:59:58Z

✅ With the latest revision this PR passed the C/C++ code formatter.

spaits · 2025-09-09T21:26:00Z

I have added another commit to the PR that contains the code, that makes the dead basic block deletion fully consistent for exception handling.

I will check the CI.

llvm/lib/Transforms/Utils/BasicBlockUtils.cpp

spaits · 2025-09-10T21:26:44Z

I think the current version works almost 100% according to the language refrenece. There is only one thing I did not "fix". That is, the language reference says that catchswitch can only receive the funclet it is within as its within argument or none. Actually sometimes it recieves poison too (if the funclet's eh pad block is deleted). I think that is fine. The IR has never leaved SimplifyCFG in that state. This patch does not change that.
(This case is tested by llvm/test/Transforms/Coroutines/coro-catchswitch-cleanuppad.ll)

rnk

Looks good with the recommended suggestion

llvm/lib/Transforms/Utils/BasicBlockUtils.cpp

spaits · 2025-09-11T12:03:13Z

Can I please ask you @dtcxzyw to run your pre commit checks for this PR? I think they would be relevant here. Thank you.

dtcxzyw · 2025-09-11T12:51:57Z

Can I please ask you @dtcxzyw to run your pre commit checks for this PR? I think they would be relevant here. Thank you.

No new crashes.

spaits · 2025-09-11T13:04:24Z

Thank you @dtcxzyw .

spaits · 2025-09-11T13:05:49Z

Thank you @rnk @efriedma-quic @nikic for reviewing. I will wait some time, if there is any additional comment and then merge the PR.

aeubanks · 2025-09-12T20:48:51Z

hi, we're seeing crashes after this patch:

$ cat /tmp/a.ll
target datalayout = "e-m:x-p:32:32-p270:32:32-p271:32:32-p272:64:64-i64:64-i128:128-f80:128-n8:16:32-a:0:32-S32"
target triple = "i386-pc-windows-msvc19.34.0"

%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
}
$ opt -p simplifycfg /tmp/a.ll
opt: ../../llvm/include/llvm/Support/Casting.h:662: decltype(auto) llvm::dyn_cast(From *) [To = llvm::BranchInst, From = llvm::Instruction]: Assertion `detail::isPresent(Val) && "dyn_cast on a non-existent value"' failed.

#157363)" This reverts commit 43561ad.

spaits · 2025-09-12T20:52:52Z

Hi @aeubanks . I will check it out.

…s" (#158364) Reverts #157363 Causes crashes, see #157363 (comment)

…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`.

…H pad blocks" (#158364) Reverts llvm/llvm-project#157363 Causes crashes, see llvm/llvm-project#157363 (comment)

…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`.

spaits requested a review from jmorse September 7, 2025 20:54

llvmbot added llvm:ir llvm:transforms labels Sep 7, 2025

spaits commented Sep 7, 2025

View reviewed changes

llvm/lib/Transforms/Utils/SimplifyCFG.cpp Outdated Show resolved Hide resolved

nikic requested review from efriedma-quic and rnk September 7, 2025 21:21

spaits force-pushed the gh-148052 branch from f221b6d to f2837b8 Compare September 9, 2025 12:45

nikic reviewed Sep 9, 2025

View reviewed changes

llvm/include/llvm/IR/Instruction.h Outdated Show resolved Hide resolved

spaits force-pushed the gh-148052 branch 3 times, most recently from 02c5309 to 644e0d7 Compare September 9, 2025 15:17

Replace unreachable cleanupret and catchret instruction with unreacha…

efade81

…ble instruction

spaits force-pushed the gh-148052 branch from 644e0d7 to efade81 Compare September 9, 2025 15:18

spaits commented Sep 9, 2025

View reviewed changes

llvm/lib/Transforms/Utils/BasicBlockUtils.cpp Outdated Show resolved Hide resolved

rnk reviewed Sep 9, 2025

View reviewed changes

llvm/lib/Transforms/Utils/BasicBlockUtils.cpp Outdated Show resolved Hide resolved

Address the case when catchswitch leaves poisoned pads

063fccf

spaits force-pushed the gh-148052 branch from 142716b to 063fccf Compare September 10, 2025 07:47

spaits changed the title ~~[SimplifyCFG] Don't delete basic block if it is a partial cleanuppad~~ [BasicBlockUtils] Handle funclets when detaching EH pad blocks Sep 10, 2025

spaits added 2 commits September 10, 2025 13:01

Clean up and add assertion

0376e58

Handle catchswitch's unwind

0893d42

spaits requested review from nikic and rnk September 10, 2025 21:26

rnk approved these changes Sep 10, 2025

View reviewed changes

llvm/lib/Transforms/Utils/BasicBlockUtils.cpp Outdated Show resolved Hide resolved

llvm/lib/Transforms/Utils/BasicBlockUtils.cpp Show resolved Hide resolved

Formating and typo

09b5007

spaits force-pushed the gh-148052 branch from 5e2fc00 to 09b5007 Compare September 11, 2025 08:36

dtcxzyw mentioned this pull request Sep 11, 2025

Task submission dtcxzyw/llvm-opt-benchmark#1312

Open

zyw-bot mentioned this pull request Sep 11, 2025

pre-commit: PR157363 dtcxzyw/llvm-opt-benchmark#2797

Closed

spaits merged commit 43561ad into llvm:main Sep 11, 2025
9 checks passed

aeubanks added a commit that referenced this pull request Sep 12, 2025

Revert "[BasicBlockUtils] Handle funclets when detaching EH pad blocks (

a00dbed

#157363)" This reverts commit 43561ad.

aeubanks mentioned this pull request Sep 12, 2025

Revert "[BasicBlockUtils] Handle funclets when detaching EH pad blocks" #158364

Merged

spaits mentioned this pull request Sep 13, 2025

Reland "[BasicBlockUtils] Handle funclets when detaching EH pad blocks" #158435

Merged

aeubanks added a commit that referenced this pull request Sep 14, 2025

Revert "[BasicBlockUtils] Handle funclets when detaching EH pad block…

b30c29c

…s" (#158364) Reverts #157363 Causes crashes, see #157363 (comment)

llvm-sync bot pushed a commit to arm/arm-toolchain that referenced this pull request Sep 16, 2025

Automerge: Revert "[BasicBlockUtils] Handle funclets when detaching E…

28bbb24

…H pad blocks" (#158364) Reverts llvm/llvm-project#157363 Causes crashes, see llvm/llvm-project#157363 (comment)

[BasicBlockUtils] Handle funclets when detaching EH pad blocks #157363

[BasicBlockUtils] Handle funclets when detaching EH pad blocks #157363

Uh oh!

Conversation

spaits commented Sep 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Sep 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

spaits commented Sep 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Sep 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

spaits commented Sep 9, 2025

Uh oh!

Uh oh!

spaits commented Sep 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rnk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

spaits commented Sep 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dtcxzyw commented Sep 11, 2025

Uh oh!

spaits commented Sep 11, 2025

Uh oh!

spaits commented Sep 11, 2025

Uh oh!

Uh oh!

aeubanks commented Sep 12, 2025

Uh oh!

spaits commented Sep 12, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

spaits commented Sep 7, 2025 •

edited

Loading

llvmbot commented Sep 7, 2025 •

edited

Loading

spaits commented Sep 9, 2025 •

edited

Loading

github-actions bot commented Sep 9, 2025 •

edited

Loading

spaits commented Sep 10, 2025 •

edited

Loading

spaits commented Sep 11, 2025 •

edited

Loading