[WebAssembly] Guard EHPadStack against underflow in fixCallUnwindMismatches - #192968
[WebAssembly] Guard EHPadStack against underflow in fixCallUnwindMismatches#192968soilSpoon wants to merge 2 commits into
Conversation
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
|
@llvm/pr-subscribers-backend-webassembly Author: 이대희 (soilSpoon) ChangesFixes one of the subcases of #126916. SummaryWhen a single cleanup landing pad is reached as the unwind destination of
Three Triage noteThis is a workaround at the consumer side of the stack; a more Reproducer
Before this patch: ``` After: Test plan
Full diff: https://github.com/llvm/llvm-project/pull/192968.diff 4 Files Affected:
diff --git a/llvm/lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp b/llvm/lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp
index 540388cb17b68..f1cd618862440 100644
--- a/llvm/lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp
+++ b/llvm/lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp
@@ -1834,9 +1834,15 @@ bool WebAssemblyCFGStackify::fixCallUnwindMismatches(MachineFunction &MF) {
for (auto &MBB : reverse(MF)) {
bool SeenThrowableInstInBB = false;
for (auto &MI : reverse(MBB)) {
- if (WebAssembly::isTry(MI.getOpcode()))
- EHPadStack.pop_back();
- else if (MI.getOpcode() == WebAssembly::DELEGATE)
+ if (WebAssembly::isTry(MI.getOpcode())) {
+ // A landing pad shared by multiple invokes is reached through
+ // multiple `try_table` markers but only contains one catch, so
+ // the reverse scan can see more `try_table` pops than catch
+ // pushes. Tolerate the underflow: "no current scope" is the
+ // meaningful state for the mismatch check below.
+ if (!EHPadStack.empty())
+ EHPadStack.pop_back();
+ } else if (MI.getOpcode() == WebAssembly::DELEGATE)
EHPadStack.push_back(MI.getOperand(0).getMBB());
else if (WebAssembly::isCatch(MI.getOpcode()))
EHPadStack.push_back(MI.getParent());
@@ -1863,7 +1869,12 @@ bool WebAssemblyCFGStackify::fixCallUnwindMismatches(MachineFunction &MF) {
break;
}
}
- if (EHPadStack.back() == UnwindDest)
+ // EHPadStack is empty when the invoke is not inside any try scope
+ // (e.g. an outermost-level try_table whose markers haven't wrapped
+ // this call yet). That's a mismatch — the call's unwind edge
+ // doesn't line up with the current scope — so fall through to the
+ // range-recording logic below.
+ if (!EHPadStack.empty() && EHPadStack.back() == UnwindDest)
continue;
// Include EH_LABELs in the range before and after the invoke
@@ -1942,19 +1953,22 @@ bool WebAssemblyCFGStackify::fixCallUnwindMismatches(MachineFunction &MF) {
}
// Update EHPadStack.
- if (WebAssembly::isTry(MI.getOpcode()))
- EHPadStack.pop_back();
- else if (MI.getOpcode() == WebAssembly::DELEGATE)
+ if (WebAssembly::isTry(MI.getOpcode())) {
+ if (!EHPadStack.empty())
+ EHPadStack.pop_back();
+ } else if (MI.getOpcode() == WebAssembly::DELEGATE)
EHPadStack.push_back(MI.getOperand(0).getMBB());
else if (WebAssembly::isCatch(MI.getOpcode()))
EHPadStack.push_back(MI.getParent());
}
- if (RangeEnd)
+ if (RangeEnd && !EHPadStack.empty())
RecordCallerMismatchRange(EHPadStack.back());
}
- assert(EHPadStack.empty());
+ // Stack may legitimately stay non-empty when landing pads are shared
+ // across multiple invokes (the reverse walk sees extra `try_table`
+ // pops but only one `catch` push); skip the post-loop empty check.
// We don't have any unwind destination mismatches to resolve.
if (UnwindDestToTryRanges.empty())
diff --git a/llvm/lib/Target/WebAssembly/WebAssemblyRegStackify.cpp b/llvm/lib/Target/WebAssembly/WebAssemblyRegStackify.cpp
index 97f2ed0a828ba..9afc06848407f 100644
--- a/llvm/lib/Target/WebAssembly/WebAssemblyRegStackify.cpp
+++ b/llvm/lib/Target/WebAssembly/WebAssemblyRegStackify.cpp
@@ -539,10 +539,11 @@ static unsigned getTeeOpcode(const TargetRegisterClass *RC) {
// Shrink LI to its uses, cleaning up LI.
static void shrinkToUses(LiveInterval &LI, LiveIntervals &LIS) {
- if (LIS.shrinkToUses(&LI)) {
- SmallVector<LiveInterval *, 4> SplitLIs;
- LIS.splitSeparateComponents(LI, SplitLIs);
- }
+ LIS.shrinkToUses(&LI);
+ // In case the register's live interval now has multiple unconnected
+ // components, split them into multiple registers.
+ SmallVector<LiveInterval *, 4> SplitLIs;
+ LIS.splitSeparateComponents(LI, SplitLIs);
}
/// A single-use def in the same block with no intervening memory or register
diff --git a/llvm/test/CodeGen/WebAssembly/cfg-stackify-shared-cleanuppad.ll b/llvm/test/CodeGen/WebAssembly/cfg-stackify-shared-cleanuppad.ll
new file mode 100644
index 0000000000000..c81c1694397e2
--- /dev/null
+++ b/llvm/test/CodeGen/WebAssembly/cfg-stackify-shared-cleanuppad.ll
@@ -0,0 +1,74 @@
+; RUN: llc < %s -wasm-enable-eh -wasm-use-legacy-eh=false -exception-model=wasm -mattr=+exception-handling -verify-machineinstrs -filetype=obj -o /dev/null
+; RUN: llc < %s -mtriple=wasm64-unknown-emscripten -wasm-enable-eh -wasm-use-legacy-eh=false -exception-model=wasm -mattr=+atomics,+bulk-memory,+mutable-globals,+sign-ext,+nontrapping-fptoint,+multivalue,+exception-handling -verify-machineinstrs -filetype=obj -o /dev/null
+
+; Regression test for issue #126916 (one of its subcases): a single
+; `cleanuppad` reached as the unwind destination of multiple `invoke`s
+; (bb3 and bb7 both unwind to bb11 below). Under the standardized Wasm
+; EH codegen path, `placeTryTableMarker` emits two `TRY_TABLE` markers
+; for that pad but only one `catch`, so the reverse walk in
+; `fixCallUnwindMismatches` underflows `EHPadStack` when it tries to
+; match the pop_back against the push. Prior to the fix, this crashed
+; with `SmallVector::back()`/`pop_back()` assertions (or a segfault in
+; release builds).
+
+target triple = "wasm32-unknown-unknown"
+
+declare i32 @__gxx_wasm_personality_v0(...)
+declare ptr @llvm.wasm.get.exception(token)
+declare void @f1(ptr, ptr)
+declare ptr @f2(ptr, ptr)
+declare ptr @f3(ptr, ptr)
+declare void @f4(ptr)
+declare i32 @f5(ptr)
+
+define i32 @shared_cleanuppad(ptr %0, i1 %1) personality ptr @__gxx_wasm_personality_v0 {
+ br i1 %1, label %3, label %14
+
+3:
+ invoke void @f1(ptr null, ptr null)
+ to label %5 unwind label %11
+
+5:
+ %6 = invoke ptr @f2(ptr null, ptr null)
+ to label %7 unwind label %9
+
+7:
+ %8 = invoke ptr @f3(ptr null, ptr null)
+ to label %common.ret unwind label %11
+
+common.ret:
+ %common.ret.op = phi i32 [ 0, %23 ], [ 0, %9 ], [ 0, %18 ], [ 0, %26 ], [ 0, %7 ]
+ ret i32 %common.ret.op
+
+9:
+ %10 = cleanuppad within none []
+ br label %common.ret
+
+11:
+ %12 = cleanuppad within none []
+ cleanupret from %12 unwind to caller
+
+14:
+ invoke void @f4(ptr null)
+ to label %26 unwind label %16
+
+16:
+ %17 = catchswitch within none [label %18] unwind to caller
+
+18:
+ %19 = catchpad within %17 [ptr null]
+ %20 = tail call ptr @llvm.wasm.get.exception(token %19)
+ br label %common.ret
+
+21:
+ %22 = catchswitch within none [label %23] unwind to caller
+
+23:
+ %24 = catchpad within %22 [ptr null]
+ %25 = tail call ptr @llvm.wasm.get.exception(token %24)
+ br label %common.ret
+
+26:
+ %28 = invoke i32 @f5(ptr null)
+ to label %common.ret unwind label %21
+}
diff --git a/llvm/test/CodeGen/WebAssembly/tee-live-intervals.mir b/llvm/test/CodeGen/WebAssembly/tee-live-intervals.mir
new file mode 100644
index 0000000000000..b137e990932b0
--- /dev/null
+++ b/llvm/test/CodeGen/WebAssembly/tee-live-intervals.mir
@@ -0,0 +1,41 @@
+# RUN: llc -mtriple=wasm32-unknown-unknown -run-pass wasm-reg-stackify -verify-machineinstrs %s -o -
+
+# TEE generation in RegStackify can create virtual registers with LiveIntervals
+# with multiple disconnected segments, which is invalid in MachineVerifier. In
+# this test, '%0 = CALL @foo' will become a CALL and a TEE, which creates
+# unconnected split segments. This should be later split into multiple
+# registers. This test should not crash with -verify-machineinstrs, which checks
+# whether all segments within a register is connected. See ??? for the detailed
+# explanation.
+
+--- |
+ target triple = "wasm32-unknown-unknown"
+
+ declare ptr @foo(ptr returned)
+ define void @tee_live_intervals_test() {
+ ret void
+ }
+...
+---
+name: tee_live_intervals_test
+liveins:
+ - { reg: '$arguments' }
+tracksRegLiveness: true
+body: |
+ bb.0:
+ liveins: $arguments
+ successors: %bb.1, %bb.2
+ %0:i32 = ARGUMENT_i32 0, implicit $arguments
+ %1:i32 = CONST_I32 0, implicit-def dead $arguments
+ BR_IF %bb.2, %1:i32, implicit-def dead $arguments
+
+ bb.1:
+ ; predecessors: %bb.0
+ %0:i32 = CALL @foo, %0:i32, implicit-def dead $arguments, implicit $sp32, implicit $sp64
+ STORE8_I32_A32 0, 0, %0:i32, %1:i32, implicit-def dead $arguments
+ RETURN %0:i32, implicit-def dead $arguments
+
+ bb.2:
+ ; predecessors: %bb.0
+ %2:i32 = CONST_I32 0, implicit-def dead $arguments
+ RETURN %2:i32, implicit-def dead $arguments
|
0e2df21 to
0291171
Compare
…atches Issue llvm#126916 includes a subcase where a single cleanup landing pad is reached as the unwind destination of multiple invokes. Under the standardized Wasm EH codegen path (-wasm-use-legacy-eh=0), placeTryTableMarker emits a TRY_TABLE marker for each invoke but the pad only contains a single catch, so the reverse walk in fixCallUnwindMismatches sees more TRY_TABLE pops than catch pushes and underflows EHPadStack. In release this surfaces as a segfault a few passes later; with -verify-machineinstrs it asserts in SmallVector::back()/pop_back(). Guard the three EHPadStack.back()/pop_back() sites that did not already check for emptiness and drop the post-loop invariant assertion, since "no current scope" is now a valid state. Behaviour is unchanged when the pad is not shared (stack never underflows). Add a regression test that reduces from a VTK 9.6.1 translation unit (vtkFLUENTCFFReader::RequestData) to the minimal shape that reproduces the crash on tip LLVM and no longer crashes with the fix.
0291171 to
ac43dc2
Compare
|
Thanks for the reporting! I submitted #194184 to fix this. Can you test if #194184 compiles the full VTK 9.6.1? Also, I saw that you added some comments to #131561 and deleted them. Is that related to this issue? Or, do you need that fix for a separate bug? I wish I can land that too, but I somehow dropped it last year. |
|
Tested — see #194184 (comment) for the full A/B numbers. Short version: VTK 9.6.1 builds cleanly with #194184 (4618/4618 ninja steps, 153 libs); without it, 11 TUs in the standardized-EH path crash with the same backtrace. Yours is the right fix — please go ahead with #194184, this PR can be closed. About #131561: I commented there at first thinking it was the same bug, but on closer look it turned out to be a different issue, so I deleted those comments. No separate fix needed from my side. |
…194184) After #187484, `fixCallUnwindMismatches` now runs after `fixCatchUnwindMismatches`. But `fixCallUnwindMismatches` was not handling newly generated `try_table`s and trampoline BBs in `fixCatchUnwindMismatches` correctly. See the comments for details. This is mean to replace #192968 and adds its test case to `cfg-stackify-eh.ll` and `cfg-stackify-eh-legacy.ll`. (The bug only happens in the standard (exnref) EH so `cfg-stackify-eh.ll`, but I added the same test to `cfg-stackify-eh-legacy.ll` as well for more coverage.) `exception.ll`'s `inlined_cleanupret`'s results have changed because we were generating unnecessary `try_table`s, thinking there were call unwind mismatches when there weren't. (This wouldn't have resulted in incorrect execution; it just added more unnecessary instructions.) This also fixes some comments for `inlined_cleanupret`, because "throw_ref targets the top level cleanuppad" is confusing. (This comment was probably copy-pasted from "delegate targets ...".) Closes #192968.
…lvm#194184) After llvm#187484, `fixCallUnwindMismatches` now runs after `fixCatchUnwindMismatches`. But `fixCallUnwindMismatches` was not handling newly generated `try_table`s and trampoline BBs in `fixCatchUnwindMismatches` correctly. See the comments for details. This is mean to replace llvm#192968 and adds its test case to `cfg-stackify-eh.ll` and `cfg-stackify-eh-legacy.ll`. (The bug only happens in the standard (exnref) EH so `cfg-stackify-eh.ll`, but I added the same test to `cfg-stackify-eh-legacy.ll` as well for more coverage.) `exception.ll`'s `inlined_cleanupret`'s results have changed because we were generating unnecessary `try_table`s, thinking there were call unwind mismatches when there weren't. (This wouldn't have resulted in incorrect execution; it just added more unnecessary instructions.) This also fixes some comments for `inlined_cleanupret`, because "throw_ref targets the top level cleanuppad" is confusing. (This comment was probably copy-pasted from "delegate targets ...".) Closes llvm#192968.
3 participants
When a single cleanup landing pad is reached as the unwind destination of multiple
invokes, the reverse walk infixCallUnwindMismatches()underflowsEHPadStack: the pass hitsback()/pop_back()on an emptySmallVectorand asserts (or segfaults in release). Reproduces on currentmainwith-wasm-enable-eh -wasm-use-legacy-eh=false -exception-model=wasm -mattr=+exception-handling— see the new regression testcfg-stackify-shared-cleanuppad.ll(reduced from a VTK 9.6.1 translation unit withllvm-reduce).The fix guards the three
EHPadStack.back()/pop_back()sites infixCallUnwindMismatches()that did not already check for emptiness, matching the already-guardedEHPadStack.empty() || ...sites further down in the same function. The post-loopassert(EHPadStack.empty())is dropped since an empty stack is now a valid terminal state on the shared-pad path. Behaviour is unchanged on inputs where the stack never underflows.This is likely a workaround rather than a root fix — I haven't pinned down why marker placement leaves the reverse walk unbalanced on the reduced test, only that with these guards the function completes and
llc -filetype=objproduces valid output. A more principled fix may belong inplaceTryTableMarker/addNestedTryTableso the scan stays balanced; happy to pivot if a reviewer sees the right direction.A full VTK 9.6.1
-fwasm-exceptions -sWASM_LEGACY_EXCEPTIONS=0build (150 libraries) compiles with this patch; without it three translation units crash with the same backtrace.