From d95a7768a1c5497c6ccb0c2ded30f5968552513c Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Wed, 18 Sep 2019 14:13:36 -0400 Subject: [PATCH 1/8] don't record all intermediate adjustment types That's way more than is needed, and winds up recording types that will never appear in MIR. --- .../check/generator_interior.rs | 33 +++++++++++++++---- src/test/ui/async-await/issues/issue-64477.rs | 20 +++++++++++ 2 files changed, 47 insertions(+), 6 deletions(-) create mode 100644 src/test/ui/async-await/issues/issue-64477.rs diff --git a/src/librustc_typeck/check/generator_interior.rs b/src/librustc_typeck/check/generator_interior.rs index 5cee3d63ffc04..a7c307fdf894f 100644 --- a/src/librustc_typeck/check/generator_interior.rs +++ b/src/librustc_typeck/check/generator_interior.rs @@ -181,13 +181,34 @@ impl<'a, 'tcx> Visitor<'tcx> for InteriorVisitor<'a, 'tcx> { let scope = self.region_scope_tree.temporary_scope(expr.hir_id.local_id); - // Record the unadjusted type + // If there are adjustments, then record the final type -- + // this is the actual value that is being produced. + if let Some(adjusted_ty) = self.fcx.tables.borrow().expr_ty_adjusted_opt(expr) { + self.record(adjusted_ty, scope, Some(expr), expr.span); + } + + // Also record the unadjusted type (which is the only type if + // there are no adjustments). The reason for this is that the + // unadjusted value is sometimes a "temporary" that would wind + // up in a MIR temporary. + // + // As an example, consider an expression like `vec![].push()`. + // Here, the `vec![]` would wind up MIR stored into a + // temporary variable `t` which we can borrow to invoke + // `>::push(&mut t)`. + // + // Note that an expression can have many adjustments, and we + // are just ignoring those intermediate types. This is because + // those intermediate values are always linearly "consumed" by + // the other adjustments, and hence would never be directly + // captured in the MIR. + // + // (Note that this partly relies on the fact that the `Deref` + // traits always return references, which means their content + // can be reborrowed without needing to spill to a temporary. + // If this were not the case, then we could conceivably have + // to create intermediate temporaries.) let ty = self.fcx.tables.borrow().expr_ty(expr); self.record(ty, scope, Some(expr), expr.span); - - // Also include the adjusted types, since these can result in MIR locals - for adjustment in self.fcx.tables.borrow().expr_adjustments(expr) { - self.record(adjustment.target, scope, Some(expr), expr.span); - } } } diff --git a/src/test/ui/async-await/issues/issue-64477.rs b/src/test/ui/async-await/issues/issue-64477.rs new file mode 100644 index 0000000000000..5bd52d44a5827 --- /dev/null +++ b/src/test/ui/async-await/issues/issue-64477.rs @@ -0,0 +1,20 @@ +// Regression test for #64477. +// +// We were incorrectly claiming that the `f(x).await` future captured +// a value of type `T`, and hence that `T: Send` would have to hold. +// +// check-pass +// edition:2018 + +use std::future::Future; +use std::pin::Pin; + +fn f(_: &T) -> Pin + Send>> { + unimplemented!() +} + +pub fn g(x: &'static T) -> impl Future + Send { + async move { f(x).await } +} + +fn main() { } From 9a447a232e3d17d7dea7eeeda51cbc9a28d497dc Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Thu, 19 Sep 2019 10:07:50 -0400 Subject: [PATCH 2/8] replace boolean with lazy iteration Should probably check how this performs, but it's always nicer to track just a bit less mutable state. --- src/librustc_mir/build/scope.rs | 39 +++++++++++++++++---------------- 1 file changed, 20 insertions(+), 19 deletions(-) diff --git a/src/librustc_mir/build/scope.rs b/src/librustc_mir/build/scope.rs index ee6d42de388d9..3c735c8cb09d7 100644 --- a/src/librustc_mir/build/scope.rs +++ b/src/librustc_mir/build/scope.rs @@ -104,19 +104,6 @@ struct Scope { /// the span of that region_scope region_scope_span: Span, - /// Whether there's anything to do for the cleanup path, that is, - /// when unwinding through this scope. This includes destructors, - /// but not StorageDead statements, which don't get emitted at all - /// for unwinding, for several reasons: - /// * clang doesn't emit llvm.lifetime.end for C++ unwinding - /// * LLVM's memory dependency analysis can't handle it atm - /// * polluting the cleanup MIR with StorageDead creates - /// landing pads even though there's no actual destructors - /// * freeing up stack space has no effect during unwinding - /// Note that for generators we do emit StorageDeads, for the - /// use of optimizations in the MIR generator transform. - needs_cleanup: bool, - /// set of places to drop when exiting this scope. This starts /// out empty but grows as variables are declared during the /// building process. This is a stack, so we always drop from the @@ -261,6 +248,25 @@ impl Scope { scope: self.source_scope } } + + + /// Whether there's anything to do for the cleanup path, that is, + /// when unwinding through this scope. This includes destructors, + /// but not StorageDead statements, which don't get emitted at all + /// for unwinding, for several reasons: + /// * clang doesn't emit llvm.lifetime.end for C++ unwinding + /// * LLVM's memory dependency analysis can't handle it atm + /// * polluting the cleanup MIR with StorageDead creates + /// landing pads even though there's no actual destructors + /// * freeing up stack space has no effect during unwinding + /// Note that for generators we do emit StorageDeads, for the + /// use of optimizations in the MIR generator transform. + fn needs_cleanup(&self) -> bool { + self.drops.iter().any(|drop| match drop.kind { + DropKind::Value => true, + DropKind::Storage => false, + }) + } } impl<'tcx> Scopes<'tcx> { @@ -274,7 +280,6 @@ impl<'tcx> Scopes<'tcx> { source_scope: vis_scope, region_scope: region_scope.0, region_scope_span: region_scope.1.span, - needs_cleanup: false, drops: vec![], cached_generator_drop: None, cached_exits: Default::default(), @@ -295,7 +300,7 @@ impl<'tcx> Scopes<'tcx> { fn may_panic(&self, scope_count: usize) -> bool { let len = self.len(); - self.scopes[(len - scope_count)..].iter().any(|s| s.needs_cleanup) + self.scopes[(len - scope_count)..].iter().any(|s| s.needs_cleanup()) } /// Finds the breakable scope for a given label. This is used for @@ -801,10 +806,6 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { // cache of outer scope stays intact. scope.invalidate_cache(!needs_drop, self.is_generator, this_scope); if this_scope { - if let DropKind::Value = drop_kind { - scope.needs_cleanup = true; - } - let region_scope_span = region_scope.span(self.hir.tcx(), &self.hir.region_scope_tree); // Attribute scope exit drops to scope's closing brace. From e35698371d36da83d3aca8322ad812fa5512b90f Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Thu, 19 Sep 2019 10:30:46 -0400 Subject: [PATCH 3/8] make clear that `invalidate` just replaces with `default` --- src/librustc_mir/build/scope.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/librustc_mir/build/scope.rs b/src/librustc_mir/build/scope.rs index 3c735c8cb09d7..9c1acb3faae0f 100644 --- a/src/librustc_mir/build/scope.rs +++ b/src/librustc_mir/build/scope.rs @@ -189,8 +189,7 @@ pub enum BreakableTarget { impl CachedBlock { fn invalidate(&mut self) { - self.generator_drop = None; - self.unwind = None; + *self = CachedBlock::default(); } fn get(&self, generator_drop: bool) -> Option { From b2c51c24c96f8827eefc6f0de29636cc4cb6ff2c Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Thu, 19 Sep 2019 11:41:10 -0400 Subject: [PATCH 4/8] avoid generating drops for moved operands of calls Currently, after a CALL terminator is created in MIR, we insert DROP statements for all of its operands -- even though they were just moved shortly before! These spurious drops are later removed, but not before causing borrow check errors. This PR series modifies the drop code to track operands that were moved and avoid creating drops for them. Right now, I'm only using this mechanism for calls, but it seems likely it could be used in more places. --- src/librustc/mir/mod.rs | 9 ++ src/librustc_mir/build/expr/into.rs | 3 + src/librustc_mir/build/scope.rs | 93 ++++++++++++++++++- .../ui/async-await/issues/issue-64391-2.rs | 17 ++++ src/test/ui/async-await/issues/issue-64433.rs | 29 ++++++ 5 files changed, 147 insertions(+), 4 deletions(-) create mode 100644 src/test/ui/async-await/issues/issue-64391-2.rs create mode 100644 src/test/ui/async-await/issues/issue-64433.rs diff --git a/src/librustc/mir/mod.rs b/src/librustc/mir/mod.rs index 0ebc70750a6bb..92efcf44dea36 100644 --- a/src/librustc/mir/mod.rs +++ b/src/librustc/mir/mod.rs @@ -1917,6 +1917,15 @@ impl<'tcx> Place<'tcx> { } } + /// If this place represents a local variable like `_X` with no + /// projections, return `Some(_X)`. + pub fn as_local(&self) -> Option { + match self { + Place { projection: box [], base: PlaceBase::Local(l) } => Some(*l), + _ => None, + } + } + pub fn as_ref(&self) -> PlaceRef<'_, 'tcx> { PlaceRef { base: &self.base, diff --git a/src/librustc_mir/build/expr/into.rs b/src/librustc_mir/build/expr/into.rs index 45f4a16853606..30d53502b11ff 100644 --- a/src/librustc_mir/build/expr/into.rs +++ b/src/librustc_mir/build/expr/into.rs @@ -244,6 +244,9 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { let success = this.cfg.start_new_block(); let cleanup = this.diverge_cleanup(); + + this.record_operands_moved(&args); + this.cfg.terminate( block, source_info, diff --git a/src/librustc_mir/build/scope.rs b/src/librustc_mir/build/scope.rs index 9c1acb3faae0f..13d94262b6c53 100644 --- a/src/librustc_mir/build/scope.rs +++ b/src/librustc_mir/build/scope.rs @@ -110,6 +110,8 @@ struct Scope { /// end of the vector (top of the stack) first. drops: Vec, + moved_locals: Vec, + /// The cache for drop chain on “normal” exit into a particular BasicBlock. cached_exits: FxHashMap<(BasicBlock, region::Scope), BasicBlock>, @@ -159,7 +161,7 @@ struct CachedBlock { generator_drop: Option, } -#[derive(Debug)] +#[derive(Debug, PartialEq, Eq)] pub(crate) enum DropKind { Value, Storage, @@ -280,6 +282,7 @@ impl<'tcx> Scopes<'tcx> { region_scope: region_scope.0, region_scope_span: region_scope.1.span, drops: vec![], + moved_locals: vec![], cached_generator_drop: None, cached_exits: Default::default(), cached_unwind: CachedBlock::default(), @@ -484,7 +487,8 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { block, unwind_to, self.arg_count, - false, + false, // not generator + false, // not unwind path )); block.unit() @@ -576,7 +580,8 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { block, unwind_to, self.arg_count, - false, + false, // not generator + false, // not unwind path )); scope = next_scope; @@ -626,7 +631,8 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { block, unwind_to, self.arg_count, - true, + true, // is generator + true, // is cached path )); } @@ -822,6 +828,75 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { span_bug!(span, "region scope {:?} not in scope to drop {:?}", region_scope, local); } + /// Indicates that the "local operand" stored in `local` is + /// *moved* at some point during execution (see `local_scope` for + /// more information about what a "local operand" is -- in short, + /// it's an intermediate operand created as part of preparing some + /// MIR instruction). We use this information to suppress + /// redundant drops on the non-unwind paths. This results in less + /// MIR, but also avoids spurious borrow check errors + /// (c.f. #64391). + /// + /// Example: when compiling the call to `foo` here: + /// + /// ```rust + /// foo(bar(), ...) + /// ``` + /// + /// we would evaluate `bar()` to an operand `_X`. We would also + /// schedule `_X` to be dropped when the expression scope for + /// `foo(bar())` is exited. This is relevant, for example, if the + /// later arguments should unwind (it would ensure that `_X` gets + /// dropped). However, if no unwind occurs, then `_X` will be + /// unconditionally consumed by the `call`: + /// + /// ``` + /// bb { + /// ... + /// _R = CALL(foo, _X, ...) + /// } + /// ``` + /// + /// However, `_X` is still registered to be dropped, and so if we + /// do nothing else, we would generate a `DROP(_X)` that occurs + /// after the call. This will later be optimized out by the + /// drop-elaboation code, but in the meantime it can lead to + /// spurious borrow-check errors -- the problem, ironically, is + /// not the `DROP(_X)` itself, but the (spurious) unwind pathways + /// that it creates. See #64391 for an example. + pub fn record_operands_moved( + &mut self, + operands: &[Operand<'tcx>], + ) { + let scope = match self.local_scope() { + None => { + // if there is no local scope, operands won't be dropped anyway + return; + } + + Some(local_scope) => { + self.scopes.iter_mut().find(|scope| scope.region_scope == local_scope) + .unwrap_or_else(|| bug!("scope {:?} not found in scope list!", local_scope)) + } + }; + + // look for moves of a local variable, like `MOVE(_X)` + let locals_moved = operands.iter().flat_map(|operand| match operand { + Operand::Copy(_) | Operand::Constant(_) => None, + Operand::Move(place) => place.as_local(), + }); + + for local in locals_moved { + // check if we have a Drop for this operand and -- if so + // -- add it to the list of moved operands. Note that this + // local might not have been an operand created for this + // call, it could come from other places too. + if scope.drops.iter().any(|drop| drop.local == local && drop.kind == DropKind::Value) { + scope.moved_locals.push(local); + } + } + } + // Other // ===== /// Branch based on a boolean condition. @@ -1020,6 +1095,7 @@ fn build_scope_drops<'tcx>( last_unwind_to: BasicBlock, arg_count: usize, generator_drop: bool, + is_cached_path: bool, ) -> BlockAnd<()> { debug!("build_scope_drops({:?} -> {:?})", block, scope); @@ -1046,6 +1122,15 @@ fn build_scope_drops<'tcx>( let drop_data = &scope.drops[drop_idx]; let source_info = scope.source_info(drop_data.span); let local = drop_data.local; + + // If the operand has been moved, and we are not on an unwind + // path, then don't generate the drop. (We only take this into + // account for non-unwind paths so as not to disturb the + // caching mechanism.) + if !is_cached_path && scope.moved_locals.iter().any(|&o| o == local) { + continue; + } + match drop_data.kind { DropKind::Value => { let unwind_to = get_unwind_to(scope, is_generator, drop_idx, generator_drop) diff --git a/src/test/ui/async-await/issues/issue-64391-2.rs b/src/test/ui/async-await/issues/issue-64391-2.rs new file mode 100644 index 0000000000000..528704c0e45c3 --- /dev/null +++ b/src/test/ui/async-await/issues/issue-64391-2.rs @@ -0,0 +1,17 @@ +// Regression test for #64391 +// +// As described on the issue, the (spurious) `DROP` inserted for the +// `"".to_string()` value was causing a (spurious) unwind path that +// led us to believe that the future might be dropped after `config` +// had been dropped. This cannot, in fact, happen. + +async fn connect() { + let config = 666; + connect2(&config, "".to_string()).await +} + +async fn connect2(_config: &u32, _tls: String) { + unimplemented!() +} + +fn main() { } diff --git a/src/test/ui/async-await/issues/issue-64433.rs b/src/test/ui/async-await/issues/issue-64433.rs new file mode 100644 index 0000000000000..ca819e78fb8cb --- /dev/null +++ b/src/test/ui/async-await/issues/issue-64433.rs @@ -0,0 +1,29 @@ +// Regression test for issue #64433. +// +// See issue-64391-2.rs for more details, as that was fixed by the +// same PR. +// +// check-pass + +#[derive(Debug)] +struct A<'a> { + inner: Vec<&'a str>, +} + +struct B {} + +impl B { + async fn something_with_a(&mut self, a: A<'_>) -> Result<(), String> { + println!("{:?}", a); + Ok(()) + } +} + +async fn can_error(some_string: &str) -> Result<(), String> { + let a = A { inner: vec![some_string, "foo"] }; + let mut b = B {}; + Ok(b.something_with_a(a).await.map(|_| ())?) +} + +fn main() { +} From 265566334549bbe38e775af092f9b991e91076e1 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Thu, 19 Sep 2019 13:15:54 -0400 Subject: [PATCH 5/8] fix tests for 2018 --- src/test/ui/async-await/issues/issue-64391-2.rs | 3 +++ src/test/ui/async-await/issues/issue-64433.rs | 1 + 2 files changed, 4 insertions(+) diff --git a/src/test/ui/async-await/issues/issue-64391-2.rs b/src/test/ui/async-await/issues/issue-64391-2.rs index 528704c0e45c3..eef2c1fb20ab4 100644 --- a/src/test/ui/async-await/issues/issue-64391-2.rs +++ b/src/test/ui/async-await/issues/issue-64391-2.rs @@ -4,6 +4,9 @@ // `"".to_string()` value was causing a (spurious) unwind path that // led us to believe that the future might be dropped after `config` // had been dropped. This cannot, in fact, happen. +// +// check-pass +// edition:2018 async fn connect() { let config = 666; diff --git a/src/test/ui/async-await/issues/issue-64433.rs b/src/test/ui/async-await/issues/issue-64433.rs index ca819e78fb8cb..802e09c8a1f00 100644 --- a/src/test/ui/async-await/issues/issue-64433.rs +++ b/src/test/ui/async-await/issues/issue-64433.rs @@ -4,6 +4,7 @@ // same PR. // // check-pass +// edition:2018 #[derive(Debug)] struct A<'a> { From c78a00991b57bdeec0f55452d14b9e4a0ccc97c4 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Thu, 19 Sep 2019 13:42:46 -0400 Subject: [PATCH 6/8] whoops, only trigger this path for avoiding DROP, not StorageDead --- src/librustc_mir/build/scope.rs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/librustc_mir/build/scope.rs b/src/librustc_mir/build/scope.rs index 13d94262b6c53..a26ec72584bda 100644 --- a/src/librustc_mir/build/scope.rs +++ b/src/librustc_mir/build/scope.rs @@ -1123,16 +1123,16 @@ fn build_scope_drops<'tcx>( let source_info = scope.source_info(drop_data.span); let local = drop_data.local; - // If the operand has been moved, and we are not on an unwind - // path, then don't generate the drop. (We only take this into - // account for non-unwind paths so as not to disturb the - // caching mechanism.) - if !is_cached_path && scope.moved_locals.iter().any(|&o| o == local) { - continue; - } - match drop_data.kind { DropKind::Value => { + // If the operand has been moved, and we are not on an unwind + // path, then don't generate the drop. (We only take this into + // account for non-unwind paths so as not to disturb the + // caching mechanism.) + if !is_cached_path && scope.moved_locals.iter().any(|&o| o == local) { + continue; + } + let unwind_to = get_unwind_to(scope, is_generator, drop_idx, generator_drop) .unwrap_or(last_unwind_to); From 1962d75c10363e1754df5526ed8fa01d22e42f24 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Thu, 19 Sep 2019 14:30:05 -0400 Subject: [PATCH 7/8] fix mir-opt tests --- src/test/mir-opt/box_expr.rs | 15 +++-------- .../mir-opt/generator-storage-dead-unwind.rs | 26 +++++++++---------- 2 files changed, 17 insertions(+), 24 deletions(-) diff --git a/src/test/mir-opt/box_expr.rs b/src/test/mir-opt/box_expr.rs index d9fa3d3d4736d..8dc6b73edf6d4 100644 --- a/src/test/mir-opt/box_expr.rs +++ b/src/test/mir-opt/box_expr.rs @@ -57,25 +57,18 @@ impl Drop for S { // } // // bb5: { -// drop(_4) -> [return: bb8, unwind: bb6]; +// StorageDead(_4); +// StorageDead(_3); +// _0 = (); +// drop(_1) -> bb8; // } -// // bb6 (cleanup): { // drop(_1) -> bb1; // } -// // bb7 (cleanup): { // drop(_4) -> bb6; // } -// // bb8: { -// StorageDead(_4); -// StorageDead(_3); -// _0 = (); -// drop(_1) -> bb9; -// } -// -// bb9: { // StorageDead(_1); // return; // } diff --git a/src/test/mir-opt/generator-storage-dead-unwind.rs b/src/test/mir-opt/generator-storage-dead-unwind.rs index bcdb937542716..109304d6d22cc 100644 --- a/src/test/mir-opt/generator-storage-dead-unwind.rs +++ b/src/test/mir-opt/generator-storage-dead-unwind.rs @@ -57,7 +57,7 @@ fn main() { // StorageLive(_6); // StorageLive(_7); // _7 = move _2; -// _6 = const take::(move _7) -> [return: bb9, unwind: bb8]; +// _6 = const take::(move _7) -> [return: bb7, unwind: bb9]; // } // bb3 (cleanup): { // StorageDead(_2); @@ -75,17 +75,7 @@ fn main() { // bb6: { // generator_drop; // } -// bb7 (cleanup): { -// StorageDead(_3); -// StorageDead(_2); -// drop(_1) -> bb1; -// } -// bb8 (cleanup): { -// StorageDead(_7); -// StorageDead(_6); -// goto -> bb7; -// } -// bb9: { +// bb7: { // StorageDead(_7); // StorageDead(_6); // StorageLive(_8); @@ -93,6 +83,16 @@ fn main() { // _9 = move _3; // _8 = const take::(move _9) -> [return: bb10, unwind: bb11]; // } +// bb8 (cleanup): { +// StorageDead(_3); +// StorageDead(_2); +// drop(_1) -> bb1; +// } +// bb9 (cleanup): { +// StorageDead(_7); +// StorageDead(_6); +// goto -> bb8; +// } // bb10: { // StorageDead(_9); // StorageDead(_8); @@ -104,7 +104,7 @@ fn main() { // bb11 (cleanup): { // StorageDead(_9); // StorageDead(_8); -// goto -> bb7; +// goto -> bb8; // } // bb12: { // return; From 77fd0a76491afb4c93359f6ceb33a9f79c4f4228 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Thu, 19 Sep 2019 16:12:48 -0400 Subject: [PATCH 8/8] add a mir-opt test that we don't add the spurious drop --- .../mir-opt/no-spurious-drop-after-call.rs | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) create mode 100644 src/test/mir-opt/no-spurious-drop-after-call.rs diff --git a/src/test/mir-opt/no-spurious-drop-after-call.rs b/src/test/mir-opt/no-spurious-drop-after-call.rs new file mode 100644 index 0000000000000..782bc31186ca5 --- /dev/null +++ b/src/test/mir-opt/no-spurious-drop-after-call.rs @@ -0,0 +1,24 @@ +// ignore-wasm32-bare compiled with panic=abort by default + +// Test that after the call to `std::mem::drop` we do not generate a +// MIR drop of the argument. (We used to have a `DROP(_2)` in the code +// below, as part of bb3.) + +fn main() { + std::mem::drop("".to_string()); +} + +// END RUST SOURCE +// START rustc.main.ElaborateDrops.before.mir +// bb2: { +// StorageDead(_3); +// _1 = const std::mem::drop::(move _2) -> [return: bb3, unwind: bb4]; +// } +// bb3: { +// StorageDead(_2); +// StorageDead(_4); +// StorageDead(_1); +// _0 = (); +// return; +// } +// END rustc.main.ElaborateDrops.before.mir