From 425e7e5596c0ab6555fa75292d38863280d4a3d7 Mon Sep 17 00:00:00 2001 From: Jonas Schievink Date: Sat, 7 Mar 2020 21:29:09 +0100 Subject: [PATCH 1/7] Don't insert panic when generator can not return --- src/librustc_mir/transform/generator.rs | 31 +++++++++++++++++++++++-- 1 file changed, 29 insertions(+), 2 deletions(-) diff --git a/src/librustc_mir/transform/generator.rs b/src/librustc_mir/transform/generator.rs index d060a0eab3db0..cc8d2807b472b 100644 --- a/src/librustc_mir/transform/generator.rs +++ b/src/librustc_mir/transform/generator.rs @@ -991,12 +991,30 @@ fn insert_panic_block<'tcx>( assert_block } +fn can_return<'tcx>(tcx: TyCtxt<'tcx>, body: &Body<'tcx>) -> bool { + // Returning from a function with an uninhabited return type is undefined behavior. + if body.return_ty().conservative_is_privately_uninhabited(tcx) { + return false; + } + + // If there's no return terminator the function also won't return. + for block in body.basic_blocks() { + if let TerminatorKind::Return = block.terminator().kind { + return true; + } + } + + // Otherwise we assume that the function may return. + false +} + fn create_generator_resume_function<'tcx>( tcx: TyCtxt<'tcx>, transform: TransformVisitor<'tcx>, def_id: DefId, source: MirSource<'tcx>, body: &mut BodyAndCache<'tcx>, + can_return: bool, ) { // Poison the generator when it unwinds for block in body.basic_blocks_mut() { @@ -1015,7 +1033,14 @@ fn create_generator_resume_function<'tcx>( // Panic when resumed on the returned or poisoned state let generator_kind = body.generator_kind.unwrap(); - cases.insert(1, (RETURNED, insert_panic_block(tcx, body, ResumedAfterReturn(generator_kind)))); + + if can_return { + cases.insert( + 1, + (RETURNED, insert_panic_block(tcx, body, ResumedAfterReturn(generator_kind))), + ); + } + cases.insert(2, (POISONED, insert_panic_block(tcx, body, ResumedAfterPanic(generator_kind)))); insert_switch(body, cases, &transform, TerminatorKind::Unreachable); @@ -1200,6 +1225,8 @@ impl<'tcx> MirPass<'tcx> for StateTransform { let (remap, layout, storage_liveness) = compute_layout(tcx, source, &upvars, interior, movable, body); + let can_return = can_return(tcx, body); + // Run the transformation which converts Places from Local to generator struct // accesses for locals in `remap`. // It also rewrites `return x` and `yield y` as writing a new generator state and returning @@ -1243,6 +1270,6 @@ impl<'tcx> MirPass<'tcx> for StateTransform { body.generator_drop = Some(box drop_shim); // Create the Generator::resume function - create_generator_resume_function(tcx, transform, def_id, source, body); + create_generator_resume_function(tcx, transform, def_id, source, body, can_return); } } From 5ac41a1a8d28493c7aa927ae49c664b9b9dce476 Mon Sep 17 00:00:00 2001 From: Jonas Schievink Date: Sat, 7 Mar 2020 21:51:34 +0100 Subject: [PATCH 2/7] Don't insert panic when generator can not unwind --- src/librustc_mir/transform/generator.rs | 46 ++++++++++++++++++++++++- 1 file changed, 45 insertions(+), 1 deletion(-) diff --git a/src/librustc_mir/transform/generator.rs b/src/librustc_mir/transform/generator.rs index cc8d2807b472b..0502e57533d02 100644 --- a/src/librustc_mir/transform/generator.rs +++ b/src/librustc_mir/transform/generator.rs @@ -1008,6 +1008,45 @@ fn can_return<'tcx>(tcx: TyCtxt<'tcx>, body: &Body<'tcx>) -> bool { false } +fn can_unwind<'tcx>(tcx: TyCtxt<'tcx>, body: &Body<'tcx>) -> bool { + // Nothing can unwind when landing pads are off. + if tcx.sess.no_landing_pads() { + return false; + } + + // Unwinds can only start at certain terminators. + for block in body.basic_blocks() { + match block.terminator().kind { + // These never unwind. + TerminatorKind::Goto { .. } + | TerminatorKind::SwitchInt { .. } + | TerminatorKind::Abort + | TerminatorKind::Return + | TerminatorKind::Unreachable + | TerminatorKind::GeneratorDrop + | TerminatorKind::FalseEdges { .. } + | TerminatorKind::FalseUnwind { .. } => {} + + // Resume will *continue* unwinding, but if there's no other unwinding terminator it + // will never be reached. + TerminatorKind::Resume => {} + + TerminatorKind::Yield { .. } => { + unreachable!("`can_unwind` called before generator transform") + } + + // These may unwind. + TerminatorKind::Drop { .. } + | TerminatorKind::DropAndReplace { .. } + | TerminatorKind::Call { .. } + | TerminatorKind::Assert { .. } => return true, + } + } + + // If we didn't find an unwinding terminator, the function cannot unwind. + false +} + fn create_generator_resume_function<'tcx>( tcx: TyCtxt<'tcx>, transform: TransformVisitor<'tcx>, @@ -1041,7 +1080,12 @@ fn create_generator_resume_function<'tcx>( ); } - cases.insert(2, (POISONED, insert_panic_block(tcx, body, ResumedAfterPanic(generator_kind)))); + if can_unwind(tcx, body) { + cases.insert( + 2, + (POISONED, insert_panic_block(tcx, body, ResumedAfterPanic(generator_kind))), + ); + } insert_switch(body, cases, &transform, TerminatorKind::Unreachable); From 46aeef6e655a85087f82bb0d1f94a97e83009218 Mon Sep 17 00:00:00 2001 From: Jonas Schievink Date: Sun, 8 Mar 2020 00:55:29 +0100 Subject: [PATCH 3/7] Poison generators when any terminator unwinds This didn't cause issues before since generator types were always considered to "need drop", leading to unwind paths (including a `Resume` block) always getting generated. --- src/librustc_mir/transform/generator.rs | 36 +++++++++++++++++++++---- 1 file changed, 31 insertions(+), 5 deletions(-) diff --git a/src/librustc_mir/transform/generator.rs b/src/librustc_mir/transform/generator.rs index 0502e57533d02..ffbf4ae939334 100644 --- a/src/librustc_mir/transform/generator.rs +++ b/src/librustc_mir/transform/generator.rs @@ -1055,11 +1055,37 @@ fn create_generator_resume_function<'tcx>( body: &mut BodyAndCache<'tcx>, can_return: bool, ) { + let can_unwind = can_unwind(tcx, body); + // Poison the generator when it unwinds - for block in body.basic_blocks_mut() { - let source_info = block.terminator().source_info; - if let &TerminatorKind::Resume = &block.terminator().kind { - block.statements.push(transform.set_discr(VariantIdx::new(POISONED), source_info)); + if can_unwind { + let poison_block = BasicBlock::new(body.basic_blocks().len()); + let source_info = source_info(body); + body.basic_blocks_mut().push(BasicBlockData { + statements: vec![transform.set_discr(VariantIdx::new(POISONED), source_info)], + terminator: Some(Terminator { source_info, kind: TerminatorKind::Resume }), + is_cleanup: true, + }); + + for (idx, block) in body.basic_blocks_mut().iter_enumerated_mut() { + let source_info = block.terminator().source_info; + + if let TerminatorKind::Resume = block.terminator().kind { + // An existing `Resume` terminator is redirected to jump to our dedicated + // "poisoning block" above. + if idx != poison_block { + *block.terminator_mut() = Terminator { + source_info, + kind: TerminatorKind::Goto { target: poison_block }, + }; + } + } else if !block.is_cleanup { + // Any terminators that *can* unwind but don't have an unwind target set are also + // pointed at our poisoning block (unless they're part of the cleanup path). + if let Some(unwind @ None) = block.terminator_mut().unwind_mut() { + *unwind = Some(poison_block); + } + } } } @@ -1080,7 +1106,7 @@ fn create_generator_resume_function<'tcx>( ); } - if can_unwind(tcx, body) { + if can_unwind { cases.insert( 2, (POISONED, insert_panic_block(tcx, body, ResumedAfterPanic(generator_kind))), From 8d9f633f4d78777487010ae119454dc9674e42ee Mon Sep 17 00:00:00 2001 From: Jonas Schievink Date: Sun, 8 Mar 2020 01:23:00 +0100 Subject: [PATCH 4/7] Change index for SwitchInt case The indices do not matter here, and this fixes an index out of bounds panic when compiling a generator that can unwind but not return. --- src/librustc_mir/transform/generator.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/librustc_mir/transform/generator.rs b/src/librustc_mir/transform/generator.rs index ffbf4ae939334..841d18268b13a 100644 --- a/src/librustc_mir/transform/generator.rs +++ b/src/librustc_mir/transform/generator.rs @@ -1108,7 +1108,7 @@ fn create_generator_resume_function<'tcx>( if can_unwind { cases.insert( - 2, + 1, (POISONED, insert_panic_block(tcx, body, ResumedAfterPanic(generator_kind))), ); } From 2cbccced0856260c6acafabecb3030465dff28c2 Mon Sep 17 00:00:00 2001 From: Jonas Schievink Date: Sun, 8 Mar 2020 01:47:54 +0100 Subject: [PATCH 5/7] Add test for unnecessary panic branches --- src/test/mir-opt/generator-tiny.rs | 34 ++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) create mode 100644 src/test/mir-opt/generator-tiny.rs diff --git a/src/test/mir-opt/generator-tiny.rs b/src/test/mir-opt/generator-tiny.rs new file mode 100644 index 0000000000000..09e943bd962e6 --- /dev/null +++ b/src/test/mir-opt/generator-tiny.rs @@ -0,0 +1,34 @@ +//! Tests that generators that cannot return or unwind don't have unnecessary +//! panic branches. + +// compile-flags: -Zno-landing-pads + +#![feature(generators, generator_trait)] + +struct HasDrop; + +impl Drop for HasDrop { + fn drop(&mut self) {} +} + +fn callee() {} + +fn main() { + let _gen = |_x: u8| { + let _d = HasDrop; + loop { + yield; + callee(); + } + }; +} + +// END RUST SOURCE + +// START rustc.main-{{closure}}.generator_resume.0.mir +// bb0: { +// ... +// switchInt(move _11) -> [0u32: bb1, 3u32: bb5, otherwise: bb6]; +// } +// ... +// END rustc.main-{{closure}}.generator_resume.0.mir From cb58de81654d92074a4abdbf90400f35f6a0062f Mon Sep 17 00:00:00 2001 From: Jonas Schievink Date: Sun, 8 Mar 2020 13:51:26 +0100 Subject: [PATCH 6/7] Apply suggestions from code review Co-Authored-By: bjorn3 --- src/librustc_mir/transform/generator.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/librustc_mir/transform/generator.rs b/src/librustc_mir/transform/generator.rs index 841d18268b13a..b386582947f1a 100644 --- a/src/librustc_mir/transform/generator.rs +++ b/src/librustc_mir/transform/generator.rs @@ -997,14 +997,14 @@ fn can_return<'tcx>(tcx: TyCtxt<'tcx>, body: &Body<'tcx>) -> bool { return false; } - // If there's no return terminator the function also won't return. + // If there's a return terminator the function may return. for block in body.basic_blocks() { if let TerminatorKind::Return = block.terminator().kind { return true; } } - // Otherwise we assume that the function may return. + // Otherwise the function can't return. false } From 38fa3783ce635814b7e3814ab26d52b653fc0ab0 Mon Sep 17 00:00:00 2001 From: Jonas Schievink Date: Sat, 14 Mar 2020 15:46:57 +0100 Subject: [PATCH 7/7] Swap inserts to keep the original ordering --- src/librustc_mir/transform/generator.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/librustc_mir/transform/generator.rs b/src/librustc_mir/transform/generator.rs index b386582947f1a..0001bb0536365 100644 --- a/src/librustc_mir/transform/generator.rs +++ b/src/librustc_mir/transform/generator.rs @@ -1099,17 +1099,17 @@ fn create_generator_resume_function<'tcx>( // Panic when resumed on the returned or poisoned state let generator_kind = body.generator_kind.unwrap(); - if can_return { + if can_unwind { cases.insert( 1, - (RETURNED, insert_panic_block(tcx, body, ResumedAfterReturn(generator_kind))), + (POISONED, insert_panic_block(tcx, body, ResumedAfterPanic(generator_kind))), ); } - if can_unwind { + if can_return { cases.insert( 1, - (POISONED, insert_panic_block(tcx, body, ResumedAfterPanic(generator_kind))), + (RETURNED, insert_panic_block(tcx, body, ResumedAfterReturn(generator_kind))), ); }