Auto merge of #113124 - nbdd0121:eh_frame, r=cjgillot

Add MIR validation for unwind out from nounwind functions + fixes to make validation pass

`@Nilstrieb`  This is the MIR validation you asked in #112403 (comment).

Two passes need to be fixed to get the validation to pass:
* `RemoveNoopLandingPads` currently unconditionally introduce a resume block (even there is none to begin with!), changed to not do that
* Generator state transform introduces a `assert` which may unwind, and its drop elaboration also introduces many new `UnwindAction`s, so in this case run the AbortUnwindingCalls after the transformation.

I believe this PR should also fix Rust-for-Linux/linux#1016, cc `@ojeda`

r? `@Nilstrieb`

compiler/rustc_const_eval/src/transform/validate.rs

@@ -18,6 +18,7 @@ use rustc_mir_dataflow::impls::MaybeStorageLive;
 use rustc_mir_dataflow::storage::always_storage_live_locals;
 use rustc_mir_dataflow::{Analysis, ResultsCursor};
 use rustc_target::abi::{Size, FIRST_VARIANT};
+use rustc_target::spec::abi::Abi;
 
 #[derive(Copy, Clone, Debug, PartialEq, Eq)]
 enum EdgeKind {
@@ -58,6 +59,25 @@ impl<'tcx> MirPass<'tcx> for Validator {
             .iterate_to_fixpoint()
             .into_results_cursor(body);
 
+        let can_unwind = if mir_phase <= MirPhase::Runtime(RuntimePhase::Initial) {
+            // In this case `AbortUnwindingCalls` haven't yet been executed.
+            true
+        } else if !tcx.def_kind(def_id).is_fn_like() {
+            true
+        } else {
+            let body_ty = tcx.type_of(def_id).skip_binder();
+            let body_abi = match body_ty.kind() {
+                ty::FnDef(..) => body_ty.fn_sig(tcx).abi(),
+                ty::Closure(..) => Abi::RustCall,
+                ty::Generator(..) => Abi::Rust,
+                _ => {
+                    span_bug!(body.span, "unexpected body ty: {:?} phase {:?}", body_ty, mir_phase)
+                }
+            };
+
+            ty::layout::fn_can_unwind(tcx, Some(def_id), body_abi)
+        };
+
         let mut cfg_checker = CfgChecker {
             when: &self.when,
             body,
@@ -68,6 +88,7 @@ impl<'tcx> MirPass<'tcx> for Validator {
             storage_liveness,
             place_cache: FxHashSet::default(),
             value_cache: FxHashSet::default(),
+            can_unwind,
         };
         cfg_checker.visit_body(body);
         cfg_checker.check_cleanup_control_flow();
@@ -99,6 +120,9 @@ struct CfgChecker<'a, 'tcx> {
     storage_liveness: ResultsCursor<'a, 'tcx, MaybeStorageLive<'static>>,
     place_cache: FxHashSet<PlaceRef<'tcx>>,
     value_cache: FxHashSet<u128>,
+    // If `false`, then the MIR must not contain `UnwindAction::Continue` or
+    // `TerminatorKind::Resume`.
+    can_unwind: bool,
 }
 
 impl<'a, 'tcx> CfgChecker<'a, 'tcx> {
@@ -237,13 +261,17 @@ impl<'a, 'tcx> CfgChecker<'a, 'tcx> {
         match unwind {
             UnwindAction::Cleanup(unwind) => {
                 if is_cleanup {
-                    self.fail(location, "unwind on cleanup block");
+                    self.fail(location, "`UnwindAction::Cleanup` in cleanup block");
                 }
                 self.check_edge(location, unwind, EdgeKind::Unwind);
             }
             UnwindAction::Continue => {
                 if is_cleanup {
-                    self.fail(location, "unwind on cleanup block");
+                    self.fail(location, "`UnwindAction::Continue` in cleanup block");
+                }
+
+                if !self.can_unwind {
+                    self.fail(location, "`UnwindAction::Continue` in no-unwind function");
                 }
             }
             UnwindAction::Unreachable | UnwindAction::Terminate => (),
@@ -464,13 +492,19 @@ impl<'a, 'tcx> Visitor<'tcx> for CfgChecker<'a, 'tcx> {
                     );
                 }
             }
-            TerminatorKind::Resume | TerminatorKind::Terminate => {
+            TerminatorKind::Resume => {
                 let bb = location.block;
                 if !self.body.basic_blocks[bb].is_cleanup {
-                    self.fail(
-                        location,
-                        "Cannot `Resume` or `Terminate` from non-cleanup basic block",
-                    )
+                    self.fail(location, "Cannot `Resume` from non-cleanup basic block")
+                }
+                if !self.can_unwind {
+                    self.fail(location, "Cannot `Resume` in a function that cannot unwind")
+                }
+            }
+            TerminatorKind::Terminate => {
+                let bb = location.block;
+                if !self.body.basic_blocks[bb].is_cleanup {
+                    self.fail(location, "Cannot `Terminate` from non-cleanup basic block")
                 }
             }
             TerminatorKind::Return => {
@@ -665,7 +699,7 @@ impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> {
                                 return;
                             };
 
-                            f_ty.ty
+                            ty::EarlyBinder::bind(f_ty.ty).instantiate(self.tcx, args)
                         } else {
                             let Some(&f_ty) = args.as_generator().prefix_tys().get(f.index())
                             else {

compiler/rustc_mir_transform/src/generator.rs

@@ -50,8 +50,10 @@
 //! For generators with state 1 (returned) and state 2 (poisoned) it does nothing.
 //! Otherwise it drops all the values in scope at the last suspension point.
 
+use crate::abort_unwinding_calls;
 use crate::deref_separator::deref_finder;
 use crate::errors;
+use crate::pass_manager as pm;
 use crate::simplify;
 use crate::MirPass;
 use rustc_data_structures::fx::{FxHashMap, FxHashSet};
@@ -64,6 +66,7 @@ use rustc_index::{Idx, IndexVec};
 use rustc_middle::mir::dump_mir;
 use rustc_middle::mir::visit::{MutVisitor, PlaceContext, Visitor};
 use rustc_middle::mir::*;
+use rustc_middle::ty::InstanceDef;
 use rustc_middle::ty::{self, AdtDef, Ty, TyCtxt};
 use rustc_middle::ty::{GeneratorArgs, GenericArgsRef};
 use rustc_mir_dataflow::impls::{
@@ -1147,7 +1150,25 @@ fn create_generator_drop_shim<'tcx>(
     // unrelated code from the resume part of the function
     simplify::remove_dead_blocks(tcx, &mut body);
 
+    // Update the body's def to become the drop glue.
+    // This needs to be updated before the AbortUnwindingCalls pass.
+    let gen_instance = body.source.instance;
+    let drop_in_place = tcx.require_lang_item(LangItem::DropInPlace, None);
+    let drop_instance = InstanceDef::DropGlue(drop_in_place, Some(gen_ty));
+    body.source.instance = drop_instance;
+
+    pm::run_passes_no_validate(
+        tcx,
+        &mut body,
+        &[&abort_unwinding_calls::AbortUnwindingCalls],
+        None,
+    );
+
+    // Temporary change MirSource to generator's instance so that dump_mir produces more sensible
+    // filename.
+    body.source.instance = gen_instance;
     dump_mir(tcx, false, "generator_drop", &0, &body, |_, _| Ok(()));
+    body.source.instance = drop_instance;
 
     body
 }
@@ -1317,6 +1338,8 @@ fn create_generator_resume_function<'tcx>(
     // unrelated code from the drop part of the function
     simplify::remove_dead_blocks(tcx, body);
 
+    pm::run_passes_no_validate(tcx, body, &[&abort_unwinding_calls::AbortUnwindingCalls], None);
+
     dump_mir(tcx, false, "generator_resume", &0, body, |_, _| Ok(()));
 }
 

compiler/rustc_mir_transform/src/remove_noop_landing_pads.rs

@@ -6,8 +6,8 @@ use rustc_middle::ty::TyCtxt;
 use rustc_target::spec::PanicStrategy;
 
 /// A pass that removes noop landing pads and replaces jumps to them with
-/// `None`. This is important because otherwise LLVM generates terrible
-/// code for these.
+/// `UnwindAction::Continue`. This is important because otherwise LLVM generates
+/// terrible code for these.
 pub struct RemoveNoopLandingPads;
 
 impl<'tcx> MirPass<'tcx> for RemoveNoopLandingPads {
@@ -84,7 +84,17 @@ impl RemoveNoopLandingPads {
     fn remove_nop_landing_pads(&self, body: &mut Body<'_>) {
         debug!("body: {:#?}", body);
 
-        // make sure there's a resume block
+        // Skip the pass if there are no blocks with a resume terminator.
+        let has_resume = body
+            .basic_blocks
+            .iter_enumerated()
+            .any(|(_bb, block)| matches!(block.terminator().kind, TerminatorKind::Resume));
+        if !has_resume {
+            debug!("remove_noop_landing_pads: no resume block in MIR");
+            return;
+        }
+
+        // make sure there's a resume block without any statements
         let resume_block = {
             let mut patch = MirPatch::new(body);
             let resume_block = patch.resume_block();

compiler/rustc_mir_transform/src/shim.rs

@@ -71,8 +71,17 @@ fn make_shim<'tcx>(tcx: TyCtxt<'tcx>, instance: ty::InstanceDef<'tcx>) -> Body<'
             // of this function. Is this intentional?
             if let Some(ty::Generator(gen_def_id, args, _)) = ty.map(Ty::kind) {
                 let body = tcx.optimized_mir(*gen_def_id).generator_drop().unwrap();
-                let body = EarlyBinder::bind(body.clone()).instantiate(tcx, args);
+                let mut body = EarlyBinder::bind(body.clone()).instantiate(tcx, args);
                 debug!("make_shim({:?}) = {:?}", instance, body);
+
+                // Run empty passes to mark phase change and perform validation.
+                pm::run_passes(
+                    tcx,
+                    &mut body,
+                    &[],
+                    Some(MirPhase::Runtime(RuntimePhase::Optimized)),
+                );
+
                 return body;
             }
 

tests/mir-opt/building/async_await.a-{closure#0}.generator_resume.0.mir

@@ -30,7 +30,7 @@ fn a::{closure#0}(_1: Pin<&mut [async fn body@$DIR/async_await.rs:11:14: 11:16]>
     }
 
     bb2: {
-        assert(const false, "`async fn` resumed after completion") -> [success: bb2, unwind continue];
+        assert(const false, "`async fn` resumed after completion") -> [success: bb2, unwind unreachable];
     }
 
     bb3: {

tests/mir-opt/building/async_await.b-{closure#0}.generator_resume.0.mir

@@ -310,7 +310,7 @@ fn b::{closure#0}(_1: Pin<&mut [async fn body@$DIR/async_await.rs:14:18: 17:2]>,
     }
 
     bb28: {
-        assert(const false, "`async fn` resumed after completion") -> [success: bb28, unwind continue];
+        assert(const false, "`async fn` resumed after completion") -> [success: bb28, unwind unreachable];
     }
 
     bb29: {

tests/run-make/panic-abort-eh_frame/Makefile

@@ -6,5 +6,5 @@
 include ../tools.mk
 
 all:
-	$(RUSTC) foo.rs --crate-type=lib --emit=obj=$(TMPDIR)/foo.o -Cpanic=abort
+	$(RUSTC) foo.rs --crate-type=lib --emit=obj=$(TMPDIR)/foo.o -Cpanic=abort --edition 2021 -Z validate-mir
 	objdump --dwarf=frames $(TMPDIR)/foo.o | $(CGREP) -v 'DW_CFA'

tests/run-make/panic-abort-eh_frame/foo.rs

@@ -1,5 +1,13 @@
 #![no_std]
 
+use core::future::Future;
+
+pub struct NeedsDrop;
+
+impl Drop for NeedsDrop {
+    fn drop(&mut self) {}
+}
+
 #[panic_handler]
 fn handler(_: &core::panic::PanicInfo<'_>) -> ! {
     loop {}
@@ -8,3 +16,19 @@ fn handler(_: &core::panic::PanicInfo<'_>) -> ! {
 pub unsafe fn oops(x: *const u32) -> u32 {
     *x
 }
+
+pub async fn foo(_: NeedsDrop) {
+    async fn bar() {}
+    bar().await;
+}
+
+pub fn poll_foo(x: &mut core::task::Context<'_>) {
+    let _g = NeedsDrop;
+    let mut p = core::pin::pin!(foo(NeedsDrop));
+    let _ = p.as_mut().poll(x);
+    let _ = p.as_mut().poll(x);
+}
+
+pub fn drop_foo() {
+    drop(foo(NeedsDrop));
+}