From cfbf1bf7cd300ddbfd2826488469c0921738685a Mon Sep 17 00:00:00 2001 From: Gary Guo Date: Wed, 28 Jun 2023 15:16:11 +0100 Subject: [PATCH 1/6] Do not create new resume block if there isn't one already --- .../src/remove_noop_landing_pads.rs | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/compiler/rustc_mir_transform/src/remove_noop_landing_pads.rs b/compiler/rustc_mir_transform/src/remove_noop_landing_pads.rs index 4941c9edce305..4e85c76fbc066 100644 --- a/compiler/rustc_mir_transform/src/remove_noop_landing_pads.rs +++ b/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(); From cec8e09edf55fbf474c4414e3fec26d85c7869ff Mon Sep 17 00:00:00 2001 From: Gary Guo Date: Wed, 28 Jun 2023 16:05:17 +0100 Subject: [PATCH 2/6] Run `AbortUnwindingCalls` after generator transform --- compiler/rustc_mir_transform/src/generator.rs | 16 +++++++++++++ ...await.a-{closure#0}.generator_resume.0.mir | 2 +- ...await.b-{closure#0}.generator_resume.0.mir | 2 +- tests/run-make/panic-abort-eh_frame/Makefile | 2 +- tests/run-make/panic-abort-eh_frame/foo.rs | 24 +++++++++++++++++++ 5 files changed, 43 insertions(+), 3 deletions(-) diff --git a/compiler/rustc_mir_transform/src/generator.rs b/compiler/rustc_mir_transform/src/generator.rs index 797a1a86846f6..2bd759ab1e5dc 100644 --- a/compiler/rustc_mir_transform/src/generator.rs +++ b/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,6 +1150,17 @@ 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. + let drop_in_place = tcx.require_lang_item(LangItem::DropInPlace, None); + body.source.instance = InstanceDef::DropGlue(drop_in_place, Some(gen_ty)); + + pm::run_passes_no_validate( + tcx, + &mut body, + &[&abort_unwinding_calls::AbortUnwindingCalls], + None, + ); + dump_mir(tcx, false, "generator_drop", &0, &body, |_, _| Ok(())); body @@ -1317,6 +1331,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(())); } diff --git a/tests/mir-opt/building/async_await.a-{closure#0}.generator_resume.0.mir b/tests/mir-opt/building/async_await.a-{closure#0}.generator_resume.0.mir index 074ebddf78bb4..9be5b8509c728 100644 --- a/tests/mir-opt/building/async_await.a-{closure#0}.generator_resume.0.mir +++ b/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: { diff --git a/tests/mir-opt/building/async_await.b-{closure#0}.generator_resume.0.mir b/tests/mir-opt/building/async_await.b-{closure#0}.generator_resume.0.mir index f774f32eb23e8..80ac92d59f3c2 100644 --- a/tests/mir-opt/building/async_await.b-{closure#0}.generator_resume.0.mir +++ b/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: { diff --git a/tests/run-make/panic-abort-eh_frame/Makefile b/tests/run-make/panic-abort-eh_frame/Makefile index 1cb7bf575cbdf..7020455b742d0 100644 --- a/tests/run-make/panic-abort-eh_frame/Makefile +++ b/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' diff --git a/tests/run-make/panic-abort-eh_frame/foo.rs b/tests/run-make/panic-abort-eh_frame/foo.rs index e185352945538..e2274d469e73c 100644 --- a/tests/run-make/panic-abort-eh_frame/foo.rs +++ b/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)); +} From 56b933763e9799f9deef86153d60988571188b49 Mon Sep 17 00:00:00 2001 From: Gary Guo Date: Wed, 28 Jun 2023 15:15:34 +0100 Subject: [PATCH 3/6] Add MIR validation for unwind out from nounwind functions --- .../src/transform/validate.rs | 48 ++++++++++++++++--- 1 file changed, 41 insertions(+), 7 deletions(-) diff --git a/compiler/rustc_const_eval/src/transform/validate.rs b/compiler/rustc_const_eval/src/transform/validate.rs index 33e73ad665359..50ae30711a659 100644 --- a/compiler/rustc_const_eval/src/transform/validate.rs +++ b/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>, value_cache: FxHashSet, + // 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 => { From 907e431f9318eb83dd5aaab6dee67a88a97df787 Mon Sep 17 00:00:00 2001 From: Gary Guo Date: Wed, 28 Jun 2023 15:29:16 +0100 Subject: [PATCH 4/6] Perform MIR validation on drop glue of generator --- compiler/rustc_mir_transform/src/shim.rs | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/compiler/rustc_mir_transform/src/shim.rs b/compiler/rustc_mir_transform/src/shim.rs index 5e8ba4f544c97..223dc59c68d17 100644 --- a/compiler/rustc_mir_transform/src/shim.rs +++ b/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; } From eb4d6d9ff72d09eee908340d8d8726b5644ae86e Mon Sep 17 00:00:00 2001 From: Gary Guo Date: Fri, 18 Aug 2023 16:19:26 +0100 Subject: [PATCH 5/6] Add missing instantiation of generator ty in validator --- compiler/rustc_const_eval/src/transform/validate.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/rustc_const_eval/src/transform/validate.rs b/compiler/rustc_const_eval/src/transform/validate.rs index 50ae30711a659..783b52d004020 100644 --- a/compiler/rustc_const_eval/src/transform/validate.rs +++ b/compiler/rustc_const_eval/src/transform/validate.rs @@ -699,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 { From 0a7202d4769fe417988b7e74333e3014a51ad462 Mon Sep 17 00:00:00 2001 From: Gary Guo Date: Fri, 18 Aug 2023 16:36:22 +0100 Subject: [PATCH 6/6] Change generator_drop's instance to that of generator for dump_mir Otherwise the file name generated for generator_drop will become core.ptr-drop_in_place.[generator@_].generator_drop.0.mir instead of main-{closure#0}.generator_drop.0.mir which breaks a mir-opt test. --- compiler/rustc_mir_transform/src/generator.rs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/compiler/rustc_mir_transform/src/generator.rs b/compiler/rustc_mir_transform/src/generator.rs index 2bd759ab1e5dc..ff4822f333f08 100644 --- a/compiler/rustc_mir_transform/src/generator.rs +++ b/compiler/rustc_mir_transform/src/generator.rs @@ -1151,8 +1151,11 @@ fn create_generator_drop_shim<'tcx>( 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); - body.source.instance = InstanceDef::DropGlue(drop_in_place, Some(gen_ty)); + let drop_instance = InstanceDef::DropGlue(drop_in_place, Some(gen_ty)); + body.source.instance = drop_instance; pm::run_passes_no_validate( tcx, @@ -1161,7 +1164,11 @@ fn create_generator_drop_shim<'tcx>( 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 }