From acbfd79acf662be04745dc0675558e75e0d30d87 Mon Sep 17 00:00:00 2001 From: Hood Chatham Date: Mon, 26 Jan 2026 16:58:31 -0800 Subject: [PATCH] Fix: On wasm targets, call `panic_in_cleanup` if panic occurs in cleanup Previously this was not correctly implemented. Each funclet may need its own terminate block, so this changes the `terminate_block` into a `terminate_blocks` `IndexVec` which can have a terminate_block for each funclet. We key on the first basic block of the funclet -- in particular, this is the start block for the old case of the top level terminate function. Rather than using a catchswitch/catchpad pair, I used a cleanuppad. The reason for the pair is to avoid catching foreign exceptions on MSVC. On wasm, it seems that the catchswitch/catchpad pair is optimized back into a single cleanuppad and a catch_all instruction is emitted which will catch foreign exceptions. Because the new logic is only used on wasm, it seemed better to take the simpler approach seeing as they do the same thing. --- compiler/rustc_codegen_gcc/src/builder.rs | 4 + compiler/rustc_codegen_llvm/src/builder.rs | 4 + compiler/rustc_codegen_ssa/src/mir/block.rs | 96 +++++++++++++++---- compiler/rustc_codegen_ssa/src/mir/mod.rs | 9 +- .../rustc_codegen_ssa/src/traits/builder.rs | 5 +- tests/codegen-llvm/double_panic_wasm.rs | 34 +++++++ tests/codegen-llvm/terminating-catchpad.rs | 10 ++ 7 files changed, 138 insertions(+), 24 deletions(-) create mode 100644 tests/codegen-llvm/double_panic_wasm.rs diff --git a/compiler/rustc_codegen_gcc/src/builder.rs b/compiler/rustc_codegen_gcc/src/builder.rs index 3def9a5c015cc..1b0a4e0b67959 100644 --- a/compiler/rustc_codegen_gcc/src/builder.rs +++ b/compiler/rustc_codegen_gcc/src/builder.rs @@ -1653,6 +1653,10 @@ impl<'a, 'gcc, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'gcc, 'tcx> { unimplemented!(); } + fn get_funclet_cleanuppad(&self, _funclet: &Funclet) -> RValue<'gcc> { + unimplemented!(); + } + // Atomic Operations fn atomic_cmpxchg( &mut self, diff --git a/compiler/rustc_codegen_llvm/src/builder.rs b/compiler/rustc_codegen_llvm/src/builder.rs index ca4805a93e017..c3e15eadfced1 100644 --- a/compiler/rustc_codegen_llvm/src/builder.rs +++ b/compiler/rustc_codegen_llvm/src/builder.rs @@ -1309,6 +1309,10 @@ impl<'a, 'll, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'll, 'tcx> { ret } + fn get_funclet_cleanuppad(&self, funclet: &Funclet<'ll>) -> &'ll Value { + funclet.cleanuppad() + } + // Atomic Operations fn atomic_cmpxchg( &mut self, diff --git a/compiler/rustc_codegen_ssa/src/mir/block.rs b/compiler/rustc_codegen_ssa/src/mir/block.rs index 35de8b5e1486b..4859380daa662 100644 --- a/compiler/rustc_codegen_ssa/src/mir/block.rs +++ b/compiler/rustc_codegen_ssa/src/mir/block.rs @@ -215,19 +215,18 @@ impl<'a, 'tcx> TerminatorCodegenHelper<'tcx> { mir::UnwindAction::Continue => None, mir::UnwindAction::Unreachable => None, mir::UnwindAction::Terminate(reason) => { - if fx.mir[self.bb].is_cleanup && base::wants_new_eh_instructions(fx.cx.tcx().sess) { + if fx.mir[self.bb].is_cleanup && base::wants_wasm_eh(fx.cx.tcx().sess) { + // For wasm, we need to generate a nested `cleanuppad within %outer_pad` + // to catch exceptions during cleanup and call `panic_in_cleanup`. + Some(fx.terminate_block(reason, Some(self.bb))) + } else if fx.mir[self.bb].is_cleanup + && base::wants_new_eh_instructions(fx.cx.tcx().sess) + { // MSVC SEH will abort automatically if an exception tries to // propagate out from cleanup. - - // FIXME(@mirkootter): For wasm, we currently do not support terminate during - // cleanup, because this requires a few more changes: The current code - // caches the `terminate_block` for each function; funclet based code - however - - // requires a different terminate_block for each funclet - // Until this is implemented, we just do not unwind inside cleanup blocks - None } else { - Some(fx.terminate_block(reason)) + Some(fx.terminate_block(reason, None)) } } }; @@ -239,7 +238,7 @@ impl<'a, 'tcx> TerminatorCodegenHelper<'tcx> { if let Some(unwind_block) = unwind_block { let ret_llbb = if let Some((_, target)) = destination { - fx.llbb(target) + self.llbb_with_cleanup(fx, target) } else { fx.unreachable_block() }; @@ -310,7 +309,7 @@ impl<'a, 'tcx> TerminatorCodegenHelper<'tcx> { ) -> MergingSucc { let unwind_target = match unwind { mir::UnwindAction::Cleanup(cleanup) => Some(self.llbb_with_cleanup(fx, cleanup)), - mir::UnwindAction::Terminate(reason) => Some(fx.terminate_block(reason)), + mir::UnwindAction::Terminate(reason) => Some(fx.terminate_block(reason, None)), mir::UnwindAction::Continue => None, mir::UnwindAction::Unreachable => None, }; @@ -318,7 +317,7 @@ impl<'a, 'tcx> TerminatorCodegenHelper<'tcx> { if operands.iter().any(|x| matches!(x, InlineAsmOperandRef::Label { .. })) { assert!(unwind_target.is_none()); let ret_llbb = if let Some(target) = destination { - fx.llbb(target) + self.llbb_with_cleanup(fx, target) } else { fx.unreachable_block() }; @@ -335,7 +334,7 @@ impl<'a, 'tcx> TerminatorCodegenHelper<'tcx> { MergingSucc::False } else if let Some(cleanup) = unwind_target { let ret_llbb = if let Some(target) = destination { - fx.llbb(target) + self.llbb_with_cleanup(fx, target) } else { fx.unreachable_block() }; @@ -1830,8 +1829,39 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> { }) } - fn terminate_block(&mut self, reason: UnwindTerminateReason) -> Bx::BasicBlock { - if let Some((cached_bb, cached_reason)) = self.terminate_block + fn terminate_block( + &mut self, + reason: UnwindTerminateReason, + outer_catchpad_bb: Option, + ) -> Bx::BasicBlock { + // mb_funclet_bb should be present if and only if the target is wasm and + // we're terminating because of an unwind in a cleanup block. In that + // case we have nested funclets and the inner catch_switch needs to know + // what outer catch_pad it is contained in. + debug_assert!( + outer_catchpad_bb.is_some() + == (base::wants_wasm_eh(self.cx.tcx().sess) + && reason == UnwindTerminateReason::InCleanup) + ); + + // When we aren't in a wasm InCleanup block, there's only one terminate + // block needed so we cache at START_BLOCK index. + let mut cache_bb = mir::START_BLOCK; + // In wasm eh InCleanup, use the outer funclet's cleanup BB as the cache + // key. + if let Some(outer_bb) = outer_catchpad_bb { + let cleanup_kinds = + self.cleanup_kinds.as_ref().expect("cleanup_kinds required for funclets"); + cache_bb = cleanup_kinds[outer_bb] + .funclet_bb(outer_bb) + .expect("funclet_bb should be in a funclet"); + + // Ensure the outer funclet is created first + if self.funclets[cache_bb].is_none() { + self.landing_pad_for(cache_bb); + } + } + if let Some((cached_bb, cached_reason)) = self.terminate_blocks[cache_bb] && reason == cached_reason { return cached_bb; @@ -1869,12 +1899,35 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> { // cp_terminate: // %cp = catchpad within %cs [null, i32 64, null] // ... + // + // By contrast, on WebAssembly targets, we specifically _do_ want to + // catch foreign exceptions. The situation with MSVC is a + // regrettable hack which we don't want to extend to other targets + // unless necessary. For WebAssembly, to generate catch(...) and + // catch only C++ exception instead of generating a catch_all, we + // need to call the intrinsics @llvm.wasm.get.exception and + // @llvm.wasm.get.ehselector in the catch pad. Since we don't do + // this, we generate a catch_all. We originally got this behavior + // by accident but it luckily matches our intention. llbb = Bx::append_block(self.cx, self.llfn, "cs_terminate"); - let cp_llbb = Bx::append_block(self.cx, self.llfn, "cp_terminate"); let mut cs_bx = Bx::build(self.cx, llbb); - let cs = cs_bx.catch_switch(None, None, &[cp_llbb]); + + // For wasm InCleanup blocks, our catch_switch is nested within the + // outer catchpad, so we need to provide it as the parent value to + // catch_switch. + let mut outer_cleanuppad = None; + if outer_catchpad_bb.is_some() { + // Get the outer funclet's catchpad + let outer_funclet = self.funclets[cache_bb] + .as_ref() + .expect("landing_pad_for didn't create funclet"); + outer_cleanuppad = Some(cs_bx.get_funclet_cleanuppad(outer_funclet)); + } + let cp_llbb = Bx::append_block(self.cx, self.llfn, "cp_terminate"); + let cs = cs_bx.catch_switch(outer_cleanuppad, None, &[cp_llbb]); + drop(cs_bx); bx = Bx::build(self.cx, cp_llbb); let null = @@ -1895,13 +1948,18 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> { } else { // Specifying more arguments than necessary usually doesn't // hurt, but the `WasmEHPrepare` LLVM pass does not recognize - // anything other than a single `null` as a `catch (...)` block, + // anything other than a single `null` as a `catch_all` block, // leading to problems down the line during instruction // selection. &[null] as &[_] }; funclet = Some(bx.catch_pad(cs, args)); + // On wasm, if we wanted to generate a catch(...) and only catch C++ + // exceptions, we'd call @llvm.wasm.get.exception and + // @llvm.wasm.get.ehselector selectors here. We want a catch_all so + // we leave them out. This is intentionally diverging from the MSVC + // behavior. } else { llbb = Bx::append_block(self.cx, self.llfn, "terminate"); bx = Bx::build(self.cx, llbb); @@ -1927,7 +1985,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> { bx.unreachable(); - self.terminate_block = Some((llbb, reason)); + self.terminate_blocks[cache_bb] = Some((llbb, reason)); llbb } diff --git a/compiler/rustc_codegen_ssa/src/mir/mod.rs b/compiler/rustc_codegen_ssa/src/mir/mod.rs index 819abb9ae644d..1a0f66d31cca4 100644 --- a/compiler/rustc_codegen_ssa/src/mir/mod.rs +++ b/compiler/rustc_codegen_ssa/src/mir/mod.rs @@ -90,8 +90,11 @@ pub struct FunctionCx<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> { /// Cached unreachable block unreachable_block: Option, - /// Cached terminate upon unwinding block and its reason - terminate_block: Option<(Bx::BasicBlock, UnwindTerminateReason)>, + /// Cached terminate upon unwinding block and its reason. For non-wasm + /// targets, there is at most one such block per function, stored at index + /// `START_BLOCK`. For wasm targets, each funclet needs its own terminate + /// block, indexed by the cleanup block that is the funclet's head. + terminate_blocks: IndexVec>, /// A bool flag for each basic block indicating whether it is a cold block. /// A cold block is a block that is unlikely to be executed at runtime. @@ -227,7 +230,7 @@ pub fn codegen_mir<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>( personality_slot: None, cached_llbbs, unreachable_block: None, - terminate_block: None, + terminate_blocks: IndexVec::from_elem(None, &mir.basic_blocks), cleanup_kinds, landing_pads: IndexVec::from_elem(None, &mir.basic_blocks), funclets: IndexVec::from_fn_n(|_| None, mir.basic_blocks.len()), diff --git a/compiler/rustc_codegen_ssa/src/traits/builder.rs b/compiler/rustc_codegen_ssa/src/traits/builder.rs index 3486bd140eceb..edabd8188a3b6 100644 --- a/compiler/rustc_codegen_ssa/src/traits/builder.rs +++ b/compiler/rustc_codegen_ssa/src/traits/builder.rs @@ -552,12 +552,12 @@ pub trait BuilderMethods<'a, 'tcx>: fn set_personality_fn(&mut self, personality: Self::Function); - // These are used by everyone except msvc + // These are used by everyone except msvc and wasm EH fn cleanup_landing_pad(&mut self, pers_fn: Self::Function) -> (Self::Value, Self::Value); fn filter_landing_pad(&mut self, pers_fn: Self::Function); fn resume(&mut self, exn0: Self::Value, exn1: Self::Value); - // These are used only by msvc + // These are used by msvc and wasm EH fn cleanup_pad(&mut self, parent: Option, args: &[Self::Value]) -> Self::Funclet; fn cleanup_ret(&mut self, funclet: &Self::Funclet, unwind: Option); fn catch_pad(&mut self, parent: Self::Value, args: &[Self::Value]) -> Self::Funclet; @@ -567,6 +567,7 @@ pub trait BuilderMethods<'a, 'tcx>: unwind: Option, handlers: &[Self::BasicBlock], ) -> Self::Value; + fn get_funclet_cleanuppad(&self, funclet: &Self::Funclet) -> Self::Value; fn atomic_cmpxchg( &mut self, diff --git a/tests/codegen-llvm/double_panic_wasm.rs b/tests/codegen-llvm/double_panic_wasm.rs new file mode 100644 index 0000000000000..1eafe60503809 --- /dev/null +++ b/tests/codegen-llvm/double_panic_wasm.rs @@ -0,0 +1,34 @@ +//@ compile-flags: -C panic=unwind -Copt-level=0 +//@ needs-unwind +//@ only-wasm32 + +#![crate_type = "lib"] + +// Test that `panic_in_cleanup` is called on webassembly targets when a panic +// occurs in a destructor during unwinding. + +extern "Rust" { + fn may_panic(); +} + +struct PanicOnDrop; + +impl Drop for PanicOnDrop { + fn drop(&mut self) { + unsafe { may_panic() } + } +} + +// CHECK-LABEL: @double_panic +// CHECK: invoke void @may_panic() +// CHECK: invoke void @{{.+}}drop_in_place{{.+}} +// CHECK: unwind label %[[TERMINATE:.*]] +// +// CHECK: [[TERMINATE]]: +// CHECK: call void @{{.*panic_in_cleanup}} +// CHECK: unreachable +#[no_mangle] +pub fn double_panic() { + let _guard = PanicOnDrop; + unsafe { may_panic() } +} diff --git a/tests/codegen-llvm/terminating-catchpad.rs b/tests/codegen-llvm/terminating-catchpad.rs index a2ec19871d1fc..7c98ea94fdc13 100644 --- a/tests/codegen-llvm/terminating-catchpad.rs +++ b/tests/codegen-llvm/terminating-catchpad.rs @@ -9,6 +9,10 @@ // Ensure a catch-all generates: // - `catchpad ... [ptr null]` on Wasm (otherwise LLVM gets confused) // - `catchpad ... [ptr null, i32 64, ptr null]` on Windows (otherwise we catch SEH exceptions) +// +// Unlike on windows, on Wasm, we specifically do want to catch foreign +// exceptions. To catch only C++ exceptions we'd need to call +// @llvm.wasm.get.exception and @llvm.wasm.get.ehselector in the catchpad. #![feature(no_core, lang_items, rustc_attrs)] #![crate_type = "lib"] @@ -36,8 +40,14 @@ fn panic_cannot_unwind() -> ! { #[no_mangle] #[rustc_nounwind] pub fn doesnt_unwind() { + // CHECK: catchswitch within none [label %{{.*}}] unwind to caller // emscripten: %catchpad = catchpad within %catchswitch [ptr null] // wasi: %catchpad = catchpad within %catchswitch [ptr null] // seh: %catchpad = catchpad within %catchswitch [ptr null, i32 64, ptr null] + // + // We don't call these intrinsics on wasm targets so we generate a catch_all + // instruction which also picks up foreign exceptions + // NOT: @llvm.wasm.get.exception + // NOT: @llvm.wasm.get.ehselector unwinds(); }