Rollup merge of rust-lang#151771 - hoodmane:wasm-double-panic, r=bjorn3

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.

I also fixed the `terminate` handler to not be invoked when a foreign exception is raised, mimicking the behavior from msvc. On wasm, in order to avoid generating a `catch_all` we need to call `llvm.wasm.get.exception` and `llvm.wasm.get.ehselector`.

Author: JonathanBrouwer

compiler/rustc_codegen_gcc/src/builder.rs

@@ -1655,6 +1655,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,

compiler/rustc_codegen_llvm/src/builder.rs

@@ -1296,6 +1296,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,

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,15 +309,15 @@ 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,
         };
 
         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<mir::BasicBlock>,
+    ) -> 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
     }
 

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<Bx::BasicBlock>,
 
-    /// 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<mir::BasicBlock, Option<(Bx::BasicBlock, UnwindTerminateReason)>>,
 
     /// 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()),

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<Self::Value>, args: &[Self::Value]) -> Self::Funclet;
     fn cleanup_ret(&mut self, funclet: &Self::Funclet, unwind: Option<Self::BasicBlock>);
     fn catch_pad(&mut self, parent: Self::Value, args: &[Self::Value]) -> Self::Funclet;
@@ -567,6 +567,7 @@ pub trait BuilderMethods<'a, 'tcx>:
         unwind: Option<Self::BasicBlock>,
         handlers: &[Self::BasicBlock],
     ) -> Self::Value;
+    fn get_funclet_cleanuppad(&self, funclet: &Self::Funclet) -> Self::Value;
 
     fn atomic_cmpxchg(
         &mut self,

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() }
+}

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();
 }