From 2131b153b33f7ba2dc0a7cd3ea4efd1bc4a03ba2 Mon Sep 17 00:00:00 2001 From: Matthew Jasper Date: Sun, 3 Mar 2019 19:04:26 +0000 Subject: [PATCH 1/3] Check which blocks are cleanup in mir-opt tests --- src/librustc_mir/util/pretty.rs | 5 ++--- src/test/mir-opt/basic_assignment.rs | 2 +- src/test/mir-opt/box_expr.rs | 8 ++++---- src/test/mir-opt/issue-38669.rs | 2 +- src/test/mir-opt/issue-49232.rs | 2 +- src/test/mir-opt/loop_test.rs | 2 +- src/test/mir-opt/match_false_edges.rs | 6 +++--- src/test/mir-opt/packed-struct-drop-aligned.rs | 4 ++-- src/test/mir-opt/remove_fake_borrows.rs | 4 ++-- src/test/mir-opt/unusual-item-types.rs | 10 +++++----- 10 files changed, 22 insertions(+), 23 deletions(-) diff --git a/src/librustc_mir/util/pretty.rs b/src/librustc_mir/util/pretty.rs index c3fbee3a2a6e5..5000eb55b09f6 100644 --- a/src/librustc_mir/util/pretty.rs +++ b/src/librustc_mir/util/pretty.rs @@ -317,9 +317,8 @@ where let data = &mir[block]; // Basic block label at the top. - let cleanup_text = if data.is_cleanup { " // cleanup" } else { "" }; - let lbl = format!("{}{:?}: {{", INDENT, block); - writeln!(w, "{0:1$}{2}", lbl, ALIGN, cleanup_text)?; + let cleanup_text = if data.is_cleanup { " (cleanup)" } else { "" }; + writeln!(w, "{}{:?}{}: {{", INDENT, block, cleanup_text)?; // List of statements in the middle. let mut current_location = Location { diff --git a/src/test/mir-opt/basic_assignment.rs b/src/test/mir-opt/basic_assignment.rs index 1bbbe67a12cb8..c6ca6e41c2da4 100644 --- a/src/test/mir-opt/basic_assignment.rs +++ b/src/test/mir-opt/basic_assignment.rs @@ -48,7 +48,7 @@ fn main() { // drop(_6) -> [return: bb6, unwind: bb4]; // } // ... -// bb5: { +// bb5 (cleanup): { // drop(_6) -> bb4; // } // END rustc.main.SimplifyCfg-initial.after.mir diff --git a/src/test/mir-opt/box_expr.rs b/src/test/mir-opt/box_expr.rs index ad5cf42029e9d..14d302f0eea72 100644 --- a/src/test/mir-opt/box_expr.rs +++ b/src/test/mir-opt/box_expr.rs @@ -38,7 +38,7 @@ impl Drop for S { // (*_2) = const S::new() -> [return: bb2, unwind: bb3]; // } // -// bb1: { +// bb1 (cleanup): { // resume; // } // @@ -47,7 +47,7 @@ impl Drop for S { // drop(_2) -> bb4; // } // -// bb3: { +// bb3 (cleanup): { // drop(_2) -> bb1; // } // @@ -62,11 +62,11 @@ impl Drop for S { // drop(_4) -> [return: bb8, unwind: bb6]; // } // -// bb6: { +// bb6 (cleanup): { // drop(_1) -> bb1; // } // -// bb7: { +// bb7 (cleanup): { // drop(_4) -> bb6; // } // diff --git a/src/test/mir-opt/issue-38669.rs b/src/test/mir-opt/issue-38669.rs index 618ee2f740746..047e623941b71 100644 --- a/src/test/mir-opt/issue-38669.rs +++ b/src/test/mir-opt/issue-38669.rs @@ -18,7 +18,7 @@ fn main() { // FakeRead(ForLet, _1); // goto -> bb2; // } -// bb1: { +// bb1 (cleanup): { // resume; // } // bb2: { diff --git a/src/test/mir-opt/issue-49232.rs b/src/test/mir-opt/issue-49232.rs index 0f0401a55eaca..5f4f4ab82af24 100644 --- a/src/test/mir-opt/issue-49232.rs +++ b/src/test/mir-opt/issue-49232.rs @@ -43,7 +43,7 @@ fn main() { // FakeRead(ForMatchedPlace, _3); // switchInt(_3) -> [false: bb9, otherwise: bb8]; // } -// bb4: { +// bb4 (cleanup): { // resume; // } // bb5: { diff --git a/src/test/mir-opt/loop_test.rs b/src/test/mir-opt/loop_test.rs index e44743aa4b780..34891ee70b5c6 100644 --- a/src/test/mir-opt/loop_test.rs +++ b/src/test/mir-opt/loop_test.rs @@ -18,7 +18,7 @@ fn main() { // END RUST SOURCE // START rustc.main.SimplifyCfg-qualify-consts.after.mir // ... -// bb1: { // The cleanup block +// bb1 (cleanup): { // resume; // } // ... diff --git a/src/test/mir-opt/match_false_edges.rs b/src/test/mir-opt/match_false_edges.rs index ab6de71d2894d..7d3abdd79bdda 100644 --- a/src/test/mir-opt/match_false_edges.rs +++ b/src/test/mir-opt/match_false_edges.rs @@ -47,7 +47,7 @@ fn main() { // _3 = discriminant(_2); // switchInt(move _3) -> [0isize: bb4, 1isize: bb2, otherwise: bb7]; // } -// bb1: { +// bb1 (cleanup): { // resume; // } // bb2: { @@ -116,7 +116,7 @@ fn main() { // _3 = discriminant(_2); // switchInt(move _3) -> [0isize: bb3, 1isize: bb2, otherwise: bb7]; // } -// bb1: { +// bb1 (cleanup): { // resume; // } // bb2: { @@ -185,7 +185,7 @@ fn main() { // _3 = discriminant(_2); // switchInt(move _3) -> [1isize: bb2, otherwise: bb3]; // } -// bb1: { +// bb1 (cleanup): { // resume; // } // bb2: { diff --git a/src/test/mir-opt/packed-struct-drop-aligned.rs b/src/test/mir-opt/packed-struct-drop-aligned.rs index 01402f261563c..167a6eb349eb2 100644 --- a/src/test/mir-opt/packed-struct-drop-aligned.rs +++ b/src/test/mir-opt/packed-struct-drop-aligned.rs @@ -38,14 +38,14 @@ impl Drop for Droppy { // _6 = move (_1.0: Aligned); // drop(_6) -> [return: bb4, unwind: bb3]; // } -// bb1: { +// bb1 (cleanup): { // resume; // } // bb2: { // StorageDead(_1); // return; // } -// bb3: { +// bb3 (cleanup): { // (_1.0: Aligned) = move _4; // drop(_1) -> bb1; // } diff --git a/src/test/mir-opt/remove_fake_borrows.rs b/src/test/mir-opt/remove_fake_borrows.rs index 48d1c991b623e..144348450a91b 100644 --- a/src/test/mir-opt/remove_fake_borrows.rs +++ b/src/test/mir-opt/remove_fake_borrows.rs @@ -63,7 +63,7 @@ fn main() { // StorageDead(_8); // return; // } -// bb10: { +// bb10 (cleanup): { // resume; // } // END rustc.match_guard.CleanupNonCodegenStatements.before.mir @@ -114,7 +114,7 @@ fn main() { // StorageDead(_8); // return; // } -// bb10: { +// bb10 (cleanup): { // resume; // } // END rustc.match_guard.CleanupNonCodegenStatements.after.mir diff --git a/src/test/mir-opt/unusual-item-types.rs b/src/test/mir-opt/unusual-item-types.rs index fe85baa048e39..c56334f2edd14 100644 --- a/src/test/mir-opt/unusual-item-types.rs +++ b/src/test/mir-opt/unusual-item-types.rs @@ -22,7 +22,7 @@ fn main() { // _0 = const 2i32; // return; // } -// bb1: { +// bb1 (cleanup): { // resume; // } // END rustc.{{impl}}-ASSOCIATED_CONSTANT.mir_map.0.mir @@ -32,7 +32,7 @@ fn main() { // _0 = const 5isize; // return; // } -// bb1: { +// bb1 (cleanup): { // resume; // } // END rustc.E-V-{{constant}}.mir_map.0.mir @@ -44,16 +44,16 @@ fn main() { // bb1: { // return; // } -// bb2: { +// bb2 (cleanup): { // resume; // } // bb3: { // goto -> bb1; // } -// bb4: { +// bb4 (cleanup): { // goto -> bb2; // } -// bb5: { +// bb5 (cleanup): { // drop(((*_1).0: alloc::raw_vec::RawVec)) -> bb4; // } // bb6: { From 8a7801908c6b8f142a0b31f0582e526fb7369833 Mon Sep 17 00:00:00 2001 From: Matthew Jasper Date: Sun, 3 Mar 2019 19:27:41 +0000 Subject: [PATCH 2/3] Don't incorrectly mark blocks in generator drop shims as cleanup This also ensure that dropping a generator won't leak upvars if dropping one of them panics --- src/librustc_mir/transform/generator.rs | 46 +++++++++++----------- src/test/mir-opt/generator-drop-cleanup.rs | 43 ++++++++++++++++++++ 2 files changed, 65 insertions(+), 24 deletions(-) create mode 100644 src/test/mir-opt/generator-drop-cleanup.rs diff --git a/src/librustc_mir/transform/generator.rs b/src/librustc_mir/transform/generator.rs index c455d38cebce8..9c212e36761c0 100644 --- a/src/librustc_mir/transform/generator.rs +++ b/src/librustc_mir/transform/generator.rs @@ -592,8 +592,15 @@ fn elaborate_generator_drops<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, let param_env = tcx.param_env(def_id); let gen = self_arg(); - for block in mir.basic_blocks().indices() { - let (target, unwind, source_info) = match mir.basic_blocks()[block].terminator() { + let mut elaborator = DropShimElaborator { + mir: mir, + patch: MirPatch::new(mir), + tcx, + param_env + }; + + for (block, block_data) in mir.basic_blocks().iter_enumerated() { + let (target, unwind, source_info) = match block_data.terminator() { &Terminator { source_info, kind: TerminatorKind::Drop { @@ -604,31 +611,22 @@ fn elaborate_generator_drops<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, } if local == gen => (target, unwind, source_info), _ => continue, }; - let unwind = if let Some(unwind) = unwind { - Unwind::To(unwind) - } else { + let unwind = if block_data.is_cleanup { Unwind::InCleanup + } else { + Unwind::To(unwind.unwrap_or_else(|| elaborator.patch.resume_block())) }; - let patch = { - let mut elaborator = DropShimElaborator { - mir: &mir, - patch: MirPatch::new(mir), - tcx, - param_env - }; - elaborate_drop( - &mut elaborator, - source_info, - &Place::Base(PlaceBase::Local(gen)), - (), - target, - unwind, - block - ); - elaborator.patch - }; - patch.apply(mir); + elaborate_drop( + &mut elaborator, + source_info, + &Place::Base(PlaceBase::Local(gen)), + (), + target, + unwind, + block, + ); } + elaborator.patch.apply(mir); } fn create_generator_drop_shim<'a, 'tcx>( diff --git a/src/test/mir-opt/generator-drop-cleanup.rs b/src/test/mir-opt/generator-drop-cleanup.rs new file mode 100644 index 0000000000000..48398691271ba --- /dev/null +++ b/src/test/mir-opt/generator-drop-cleanup.rs @@ -0,0 +1,43 @@ +#![feature(generators, generator_trait)] + +// Regression test for #58892, generator drop shims should not have blocks +// spuriously marked as cleanup + +fn main() { + let gen = || { + yield; + }; +} + +// END RUST SOURCE + +// START rustc.main-{{closure}}.generator_drop.0.mir +// bb0: { +// switchInt(((*_1).0: u32)) -> [0u32: bb4, 3u32: bb7, otherwise: bb8]; +// } +// bb1: { +// goto -> bb5; +// } +// bb2: { +// return; +// } +// bb3: { +// return; +// } +// bb4: { +// goto -> bb6; +// } +// bb5: { +// goto -> bb2; +// } +// bb6: { +// goto -> bb3; +// } +// bb7: { +// StorageLive(_3); +// goto -> bb1; +// } +// bb8: { +// return; +// } +// END rustc.main-{{closure}}.generator_drop.0.mir From 5e68c5708792a945b9e1dc5b2b0299fec629a509 Mon Sep 17 00:00:00 2001 From: Matthew Jasper Date: Sun, 3 Mar 2019 19:28:05 +0000 Subject: [PATCH 3/3] Use the correct state for poisoning a generator --- src/librustc_mir/transform/generator.rs | 33 +++++++++++-------- .../run-fail/generator-resume-after-panic.rs | 22 +++++++++++++ 2 files changed, 42 insertions(+), 13 deletions(-) create mode 100644 src/test/run-fail/generator-resume-after-panic.rs diff --git a/src/librustc_mir/transform/generator.rs b/src/librustc_mir/transform/generator.rs index 9c212e36761c0..38fc1243f24af 100644 --- a/src/librustc_mir/transform/generator.rs +++ b/src/librustc_mir/transform/generator.rs @@ -26,7 +26,7 @@ //! } //! //! This pass computes the meaning of the state field and the MIR locals which are live -//! across a suspension point. There are however two hardcoded generator states: +//! across a suspension point. There are however three hardcoded generator states: //! 0 - Generator have not been resumed yet //! 1 - Generator has returned / is completed //! 2 - Generator has been poisoned @@ -144,6 +144,13 @@ fn self_arg() -> Local { Local::new(1) } +/// Generator have not been resumed yet +const UNRESUMED: u32 = 0; +/// Generator has returned / is completed +const RETURNED: u32 = 1; +/// Generator has been poisoned +const POISONED: u32 = 2; + struct SuspensionPoint { state: u32, resume: BasicBlock, @@ -278,7 +285,7 @@ impl<'a, 'tcx> MutVisitor<'tcx> for TransformVisitor<'a, 'tcx> { state } else { // Return - 1 // state for returned + RETURNED // state for returned }; data.statements.push(self.set_state(state, source_info)); data.terminator.as_mut().unwrap().kind = TerminatorKind::Return; @@ -643,10 +650,10 @@ fn create_generator_drop_shim<'a, 'tcx>( let mut cases = create_cases(&mut mir, transform, |point| point.drop); - cases.insert(0, (0, drop_clean)); + cases.insert(0, (UNRESUMED, drop_clean)); - // The returned state (1) and the poisoned state (2) falls through to - // the default case which is just to return + // The returned state and the poisoned state fall through to the default + // case which is just to return insert_switch(tcx, &mut mir, cases, &transform, TerminatorKind::Return); @@ -762,7 +769,7 @@ fn create_generator_resume_function<'a, 'tcx>( for block in mir.basic_blocks_mut() { let source_info = block.terminator().source_info; if let &TerminatorKind::Resume = &block.terminator().kind { - block.statements.push(transform.set_state(1, source_info)); + block.statements.push(transform.set_state(POISONED, source_info)); } } @@ -773,12 +780,12 @@ fn create_generator_resume_function<'a, 'tcx>( GeneratorResumedAfterReturn, }; - // Jump to the entry point on the 0 state - cases.insert(0, (0, BasicBlock::new(0))); - // Panic when resumed on the returned (1) state - cases.insert(1, (1, insert_panic_block(tcx, mir, GeneratorResumedAfterReturn))); - // Panic when resumed on the poisoned (2) state - cases.insert(2, (2, insert_panic_block(tcx, mir, GeneratorResumedAfterPanic))); + // Jump to the entry point on the unresumed + cases.insert(0, (UNRESUMED, BasicBlock::new(0))); + // Panic when resumed on the returned state + cases.insert(1, (RETURNED, insert_panic_block(tcx, mir, GeneratorResumedAfterReturn))); + // Panic when resumed on the poisoned state + cases.insert(2, (POISONED, insert_panic_block(tcx, mir, GeneratorResumedAfterPanic))); insert_switch(tcx, mir, cases, &transform, TerminatorKind::Unreachable); @@ -942,7 +949,7 @@ impl MirPass for StateTransform { mir.generator_layout = Some(layout); // Insert `drop(generator_struct)` which is used to drop upvars for generators in - // the unresumed (0) state. + // the unresumed state. // This is expanded to a drop ladder in `elaborate_generator_drops`. let drop_clean = insert_clean_drop(mir); diff --git a/src/test/run-fail/generator-resume-after-panic.rs b/src/test/run-fail/generator-resume-after-panic.rs new file mode 100644 index 0000000000000..910b4903bf6a3 --- /dev/null +++ b/src/test/run-fail/generator-resume-after-panic.rs @@ -0,0 +1,22 @@ +// error-pattern:generator resumed after panicking + +// Test that we get the correct message for resuming a panicked generator. + +#![feature(generators, generator_trait)] + +use std::{ + ops::Generator, + pin::Pin, + panic, +}; + +fn main() { + let mut g = || { + panic!(); + yield; + }; + panic::catch_unwind(panic::AssertUnwindSafe(|| { + let x = Pin::new(&mut g).resume(); + })); + Pin::new(&mut g).resume(); +}