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 ee6d42de388d9..a26ec72584bda 100644 --- a/src/librustc_mir/build/scope.rs +++ b/src/librustc_mir/build/scope.rs @@ -104,25 +104,14 @@ 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 /// 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>, @@ -172,7 +161,7 @@ struct CachedBlock { generator_drop: Option, } -#[derive(Debug)] +#[derive(Debug, PartialEq, Eq)] pub(crate) enum DropKind { Value, Storage, @@ -202,8 +191,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 { @@ -261,6 +249,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,8 +281,8 @@ 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![], + moved_locals: vec![], cached_generator_drop: None, cached_exits: Default::default(), cached_unwind: CachedBlock::default(), @@ -295,7 +302,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 @@ -480,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() @@ -572,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; @@ -622,7 +631,8 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { block, unwind_to, self.arg_count, - true, + true, // is generator + true, // is cached path )); } @@ -801,10 +811,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. @@ -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,8 +1122,17 @@ 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; + 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); 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/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; 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 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..eef2c1fb20ab4 --- /dev/null +++ b/src/test/ui/async-await/issues/issue-64391-2.rs @@ -0,0 +1,20 @@ +// 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. +// +// check-pass +// edition:2018 + +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..802e09c8a1f00 --- /dev/null +++ b/src/test/ui/async-await/issues/issue-64433.rs @@ -0,0 +1,30 @@ +// Regression test for issue #64433. +// +// See issue-64391-2.rs for more details, as that was fixed by the +// same PR. +// +// check-pass +// edition:2018 + +#[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() { +} 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() { }