From f77d1076ab63dc68c1bce2b8cc08e05b0ea885b0 Mon Sep 17 00:00:00 2001 From: Jonas Schievink Date: Sun, 16 Feb 2020 21:12:46 +0100 Subject: [PATCH 1/7] Match MIR statements exhaustively --- src/librustc_mir/dataflow/impls/storage_liveness.rs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/librustc_mir/dataflow/impls/storage_liveness.rs b/src/librustc_mir/dataflow/impls/storage_liveness.rs index 7508d71945e..2a7c6c2ffc1 100644 --- a/src/librustc_mir/dataflow/impls/storage_liveness.rs +++ b/src/librustc_mir/dataflow/impls/storage_liveness.rs @@ -129,7 +129,14 @@ impl<'mir, 'tcx> BitDenotation<'tcx> for RequiresStorage<'mir, 'tcx> { sets.gen(place.local); } } - _ => (), + + // Nothing to do for these. Match exhaustively so this fails to compile when new + // variants are added. + StatementKind::AscribeUserType(..) + | StatementKind::FakeRead(..) + | StatementKind::Nop + | StatementKind::Retag(..) + | StatementKind::StorageLive(..) => {} } } From 3723fc1e45381ed8957b60967c8f7af5ab93e796 Mon Sep 17 00:00:00 2001 From: Jonas Schievink Date: Sun, 16 Feb 2020 21:36:50 +0100 Subject: [PATCH 2/7] Use match ergonomics to simplify match --- src/librustc_mir/dataflow/impls/storage_liveness.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/librustc_mir/dataflow/impls/storage_liveness.rs b/src/librustc_mir/dataflow/impls/storage_liveness.rs index 2a7c6c2ffc1..5781b3982b1 100644 --- a/src/librustc_mir/dataflow/impls/storage_liveness.rs +++ b/src/librustc_mir/dataflow/impls/storage_liveness.rs @@ -118,13 +118,13 @@ impl<'mir, 'tcx> BitDenotation<'tcx> for RequiresStorage<'mir, 'tcx> { self.borrowed_locals.borrow().analysis().statement_effect(sets, stmt, loc); // If a place is assigned to in a statement, it needs storage for that statement. - match stmt.kind { - StatementKind::StorageDead(l) => sets.kill(l), - StatementKind::Assign(box (ref place, _)) - | StatementKind::SetDiscriminant { box ref place, .. } => { + match &stmt.kind { + StatementKind::StorageDead(l) => sets.kill(*l), + StatementKind::Assign(box (place, _)) + | StatementKind::SetDiscriminant { box place, .. } => { sets.gen(place.local); } - StatementKind::InlineAsm(box InlineAsm { ref outputs, .. }) => { + StatementKind::InlineAsm(box InlineAsm { outputs, .. }) => { for place in &**outputs { sets.gen(place.local); } From d4c6dfe6d6028dacc0dc59f509a823aad2d2cdf6 Mon Sep 17 00:00:00 2001 From: Jonas Schievink Date: Sun, 16 Feb 2020 22:18:26 +0100 Subject: [PATCH 3/7] Handle resume args in `RequiresStorage` analysis --- .../dataflow/impls/storage_liveness.rs | 57 +++++++++++++++---- 1 file changed, 46 insertions(+), 11 deletions(-) diff --git a/src/librustc_mir/dataflow/impls/storage_liveness.rs b/src/librustc_mir/dataflow/impls/storage_liveness.rs index 5781b3982b1..659b66823c2 100644 --- a/src/librustc_mir/dataflow/impls/storage_liveness.rs +++ b/src/librustc_mir/dataflow/impls/storage_liveness.rs @@ -152,23 +152,58 @@ impl<'mir, 'tcx> BitDenotation<'tcx> for RequiresStorage<'mir, 'tcx> { // If a place is borrowed in a terminator, it needs storage for that terminator. self.borrowed_locals.borrow().analysis().terminator_effect(sets, terminator, loc); - if let TerminatorKind::Call { destination: Some((place, _)), .. } = terminator.kind { - sets.gen(place.local); + match &terminator.kind { + TerminatorKind::Call { destination: Some((Place { local, .. }, _)), .. } + | TerminatorKind::Yield { resume_arg: Place { local, .. }, .. } => { + sets.gen(*local); + } + + // Nothing to do for these. Match exhaustively so this fails to compile when new + // variants are added. + TerminatorKind::Call { destination: None, .. } + | TerminatorKind::Abort + | TerminatorKind::Assert { .. } + | TerminatorKind::Drop { .. } + | TerminatorKind::DropAndReplace { .. } + | TerminatorKind::FalseEdges { .. } + | TerminatorKind::FalseUnwind { .. } + | TerminatorKind::GeneratorDrop + | TerminatorKind::Goto { .. } + | TerminatorKind::Resume + | TerminatorKind::Return + | TerminatorKind::SwitchInt { .. } + | TerminatorKind::Unreachable => {} } } fn terminator_effect(&self, sets: &mut GenKillSet, loc: Location) { - // For call terminators the destination requires storage for the call - // and after the call returns successfully, but not after a panic. - // Since `propagate_call_unwind` doesn't exist, we have to kill the - // destination here, and then gen it again in `propagate_call_return`. - if let TerminatorKind::Call { destination: Some((ref place, _)), .. } = - self.body[loc.block].terminator().kind - { - if let Some(local) = place.as_local() { - sets.kill(local); + match &self.body[loc.block].terminator().kind { + // For call terminators the destination requires storage for the call + // and after the call returns successfully, but not after a panic. + // Since `propagate_call_unwind` doesn't exist, we have to kill the + // destination here, and then gen it again in `propagate_call_return`. + TerminatorKind::Call { destination: Some((Place { local, .. }, _)), .. } => { + sets.kill(*local); } + + // Nothing to do for these. Match exhaustively so this fails to compile when new + // variants are added. + TerminatorKind::Call { destination: None, .. } + | TerminatorKind::Yield { .. } + | TerminatorKind::Abort + | TerminatorKind::Assert { .. } + | TerminatorKind::Drop { .. } + | TerminatorKind::DropAndReplace { .. } + | TerminatorKind::FalseEdges { .. } + | TerminatorKind::FalseUnwind { .. } + | TerminatorKind::GeneratorDrop + | TerminatorKind::Goto { .. } + | TerminatorKind::Resume + | TerminatorKind::Return + | TerminatorKind::SwitchInt { .. } + | TerminatorKind::Unreachable => {} } + self.check_for_move(sets, loc); } From 71c4f76153590fc1744c7f73b94d2d39ae2e2f23 Mon Sep 17 00:00:00 2001 From: Jonas Schievink Date: Mon, 17 Feb 2020 21:20:48 +0100 Subject: [PATCH 4/7] Reorder yield visitation order to match call --- src/librustc/mir/visit.rs | 2 +- src/librustc_mir/dataflow/move_paths/builder.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/librustc/mir/visit.rs b/src/librustc/mir/visit.rs index 8330bbe0834..409c981801b 100644 --- a/src/librustc/mir/visit.rs +++ b/src/librustc/mir/visit.rs @@ -519,12 +519,12 @@ macro_rules! make_mir_visitor { resume_arg, drop: _, } => { + self.visit_operand(value, source_location); self.visit_place( resume_arg, PlaceContext::MutatingUse(MutatingUseContext::Store), source_location, ); - self.visit_operand(value, source_location); } } diff --git a/src/librustc_mir/dataflow/move_paths/builder.rs b/src/librustc_mir/dataflow/move_paths/builder.rs index 6f8caca5e21..57aa5de7f7a 100644 --- a/src/librustc_mir/dataflow/move_paths/builder.rs +++ b/src/librustc_mir/dataflow/move_paths/builder.rs @@ -381,9 +381,9 @@ impl<'b, 'a, 'tcx> Gatherer<'b, 'a, 'tcx> { } TerminatorKind::Yield { ref value, resume_arg: ref place, .. } => { + self.gather_operand(value); self.create_move_path(place); self.gather_init(place.as_ref(), InitKind::Deep); - self.gather_operand(value); } TerminatorKind::Drop { ref location, target: _, unwind: _ } => { From 689615724535d00c093c6c5dff0accaf6e38d9bc Mon Sep 17 00:00:00 2001 From: Jonas Schievink Date: Wed, 19 Feb 2020 23:38:31 +0100 Subject: [PATCH 5/7] Remap the resume place if necessary --- src/librustc_mir/transform/generator.rs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/librustc_mir/transform/generator.rs b/src/librustc_mir/transform/generator.rs index b3dc87d1a16..a94f03301f3 100644 --- a/src/librustc_mir/transform/generator.rs +++ b/src/librustc_mir/transform/generator.rs @@ -325,6 +325,15 @@ impl MutVisitor<'tcx> for TransformVisitor<'tcx> { // Yield let state = 3 + self.suspension_points.len(); + // The resume arg target location might itself be remapped if its base local is + // live across a yield. + let resume_arg = + if let Some(&(ty, variant, idx)) = self.remap.get(&resume_arg.local) { + self.make_field(variant, idx, ty) + } else { + resume_arg + }; + self.suspension_points.push(SuspensionPoint { state, resume, From 66b1ae40606ae0c07645ebc8f97e537d8d30ef73 Mon Sep 17 00:00:00 2001 From: Jonas Schievink Date: Wed, 19 Feb 2020 23:39:00 +0100 Subject: [PATCH 6/7] Add more comments to `SuspensionPoint` --- src/librustc_mir/transform/generator.rs | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/librustc_mir/transform/generator.rs b/src/librustc_mir/transform/generator.rs index a94f03301f3..3621ca63209 100644 --- a/src/librustc_mir/transform/generator.rs +++ b/src/librustc_mir/transform/generator.rs @@ -186,18 +186,24 @@ fn self_arg() -> Local { Local::new(1) } -/// Generator have not been resumed yet +/// Generator has not been resumed yet. const UNRESUMED: usize = GeneratorSubsts::UNRESUMED; -/// Generator has returned / is completed +/// Generator has returned / is completed. const RETURNED: usize = GeneratorSubsts::RETURNED; -/// Generator has been poisoned +/// Generator has panicked and is poisoned. const POISONED: usize = GeneratorSubsts::POISONED; +/// A `yield` point in the generator. struct SuspensionPoint<'tcx> { + /// State discriminant used when suspending or resuming at this point. state: usize, + /// The block to jump to after resumption. resume: BasicBlock, + /// Where to move the resume argument after resumption. resume_arg: Place<'tcx>, + /// Which block to jump to if the generator is dropped in this state. drop: Option, + /// Set of locals that have live storage while at this suspension point. storage_liveness: liveness::LiveVarSet, } From fc2702c96c0db560f55683e4cd33075c054ed062 Mon Sep 17 00:00:00 2001 From: Jonas Schievink Date: Wed, 19 Feb 2020 23:44:53 +0100 Subject: [PATCH 7/7] Add regression test --- src/test/ui/generator/issue-69039.rs | 30 ++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) create mode 100644 src/test/ui/generator/issue-69039.rs diff --git a/src/test/ui/generator/issue-69039.rs b/src/test/ui/generator/issue-69039.rs new file mode 100644 index 00000000000..60004f3b0ae --- /dev/null +++ b/src/test/ui/generator/issue-69039.rs @@ -0,0 +1,30 @@ +// run-pass + +#![feature(generators, generator_trait)] + +use std::ops::{Generator, GeneratorState}; + +fn my_scenario() -> impl Generator { + |_arg: String| { + let my_name = yield "What is your name?"; + let my_mood = yield "How are you feeling?"; + format!("{} is {}", my_name.trim(), my_mood.trim()) + } +} + +fn main() { + let mut my_session = Box::pin(my_scenario()); + + assert_eq!( + my_session.as_mut().resume("_arg".to_string()), + GeneratorState::Yielded("What is your name?") + ); + assert_eq!( + my_session.as_mut().resume("Your Name".to_string()), + GeneratorState::Yielded("How are you feeling?") + ); + assert_eq!( + my_session.as_mut().resume("Sensory Organs".to_string()), + GeneratorState::Complete("Your Name is Sensory Organs".to_string()) + ); +}