From bea1bde8c7e5616a9dc9d501622bdeb5772501b9 Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Wed, 10 Nov 2021 16:21:37 -0800 Subject: [PATCH 1/4] Mark mutably borrowed places as maybe initialized --- compiler/rustc_mir_dataflow/src/impls/mod.rs | 73 ++++++++++++++++++-- 1 file changed, 67 insertions(+), 6 deletions(-) diff --git a/compiler/rustc_mir_dataflow/src/impls/mod.rs b/compiler/rustc_mir_dataflow/src/impls/mod.rs index 91dddc6cd55..2585701f60c 100644 --- a/compiler/rustc_mir_dataflow/src/impls/mod.rs +++ b/compiler/rustc_mir_dataflow/src/impls/mod.rs @@ -4,17 +4,18 @@ use rustc_index::bit_set::BitSet; use rustc_index::vec::Idx; +use rustc_middle::mir::visit::{MirVisitable, Visitor}; use rustc_middle::mir::{self, Body, Location}; use rustc_middle::ty::{self, TyCtxt}; -use crate::drop_flag_effects; use crate::drop_flag_effects_for_function_entry; use crate::drop_flag_effects_for_location; use crate::elaborate_drops::DropFlagState; use crate::framework::SwitchIntEdgeEffects; -use crate::move_paths::{HasMoveData, InitIndex, InitKind, MoveData, MovePathIndex}; +use crate::move_paths::{HasMoveData, InitIndex, InitKind, LookupResult, MoveData, MovePathIndex}; use crate::on_lookup_result_bits; use crate::MoveDataParamEnv; +use crate::{drop_flag_effects, on_all_children_bits}; use crate::{lattice, AnalysisDomain, GenKill, GenKillAnalysis}; mod borrowed_locals; @@ -307,22 +308,45 @@ impl<'tcx> GenKillAnalysis<'tcx> for MaybeInitializedPlaces<'_, 'tcx> { fn statement_effect( &self, trans: &mut impl GenKill, - _statement: &mir::Statement<'tcx>, + statement: &mir::Statement<'tcx>, location: Location, ) { drop_flag_effects_for_location(self.tcx, self.body, self.mdpe, location, |path, s| { Self::update_bits(trans, path, s) + }); + + if !self.tcx.sess.opts.debugging_opts.precise_enum_drop_elaboration { + return; + } + + // Mark all places as "maybe init" if they are mutably borrowed. See #90752. + for_each_mut_borrow(statement, location, |place| { + let LookupResult::Exact(mpi) = self.move_data().rev_lookup.find(place.as_ref()) else { return }; + on_all_children_bits(self.tcx, self.body, self.move_data(), mpi, |child| { + trans.gen(child); + }) }) } fn terminator_effect( &self, trans: &mut impl GenKill, - _terminator: &mir::Terminator<'tcx>, + terminator: &mir::Terminator<'tcx>, location: Location, ) { drop_flag_effects_for_location(self.tcx, self.body, self.mdpe, location, |path, s| { Self::update_bits(trans, path, s) + }); + + if !self.tcx.sess.opts.debugging_opts.precise_enum_drop_elaboration { + return; + } + + for_each_mut_borrow(terminator, location, |place| { + let LookupResult::Exact(mpi) = self.move_data().rev_lookup.find(place.as_ref()) else { return }; + on_all_children_bits(self.tcx, self.body, self.move_data(), mpi, |child| { + trans.gen(child); + }) }) } @@ -427,7 +451,10 @@ impl<'tcx> GenKillAnalysis<'tcx> for MaybeUninitializedPlaces<'_, 'tcx> { ) { drop_flag_effects_for_location(self.tcx, self.body, self.mdpe, location, |path, s| { Self::update_bits(trans, path, s) - }) + }); + + // Unlike in `MaybeInitializedPlaces` above, we don't need to change the state when a + // mutable borrow occurs. Places cannot become uninitialized through a mutable reference. } fn terminator_effect( @@ -438,7 +465,7 @@ impl<'tcx> GenKillAnalysis<'tcx> for MaybeUninitializedPlaces<'_, 'tcx> { ) { drop_flag_effects_for_location(self.tcx, self.body, self.mdpe, location, |path, s| { Self::update_bits(trans, path, s) - }) + }); } fn call_return_effect( @@ -704,3 +731,37 @@ fn switch_on_enum_discriminant( _ => None, } } + +struct OnMutBorrow(F); + +impl Visitor<'_> for OnMutBorrow +where + F: FnMut(&mir::Place<'_>), +{ + fn visit_rvalue(&mut self, rvalue: &mir::Rvalue<'_>, location: Location) { + // FIXME: Does `&raw const foo` allow mutation? See #90413. + match rvalue { + mir::Rvalue::Ref(_, mir::BorrowKind::Mut { .. }, place) + | mir::Rvalue::AddressOf(_, place) => (self.0)(place), + + _ => {} + } + + self.super_rvalue(rvalue, location) + } +} + +/// Calls `f` for each mutable borrow or raw reference in the program. +/// +/// This DOES NOT call `f` for a shared borrow of a type with interior mutability. That's okay for +/// initializedness, because we cannot move from an `UnsafeCell` (outside of `core::cell`), but +/// other analyses will likely need to check for `!Freeze`. +fn for_each_mut_borrow<'tcx>( + mir: &impl MirVisitable<'tcx>, + location: Location, + f: impl FnMut(&mir::Place<'_>), +) { + let mut vis = OnMutBorrow(f); + + mir.apply(location, &mut vis); +} From d846fe052294be6bd763bfb08479c9b93cda90fd Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Wed, 10 Nov 2021 16:22:16 -0800 Subject: [PATCH 2/4] Add regression test for #90752 --- src/test/ui/drop/issue-90752.rs | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) create mode 100644 src/test/ui/drop/issue-90752.rs diff --git a/src/test/ui/drop/issue-90752.rs b/src/test/ui/drop/issue-90752.rs new file mode 100644 index 00000000000..4395e45e773 --- /dev/null +++ b/src/test/ui/drop/issue-90752.rs @@ -0,0 +1,32 @@ +// run-pass + +use std::cell::RefCell; + +struct S<'a>(i32, &'a RefCell>); + +impl<'a> Drop for S<'a> { + fn drop(&mut self) { + self.1.borrow_mut().push(self.0); + } +} + +fn test(drops: &RefCell>) { + let mut foo = None; + match foo { + None => (), + _ => return, + } + + *(&mut foo) = Some((S(0, drops), S(1, drops))); // Both S(0) and S(1) should be dropped + + match foo { + Some((_x, _)) => {} + _ => {} + } +} + +fn main() { + let drops = RefCell::new(Vec::new()); + test(&drops); + assert_eq!(*drops.borrow(), &[0, 1]); +} From ece0e6ae6d29aa8c8b1fea06986840a4609f43b8 Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Thu, 11 Nov 2021 14:55:08 -0800 Subject: [PATCH 3/4] Add raw pointer variant of #90752 with incorrect behavior --- .../drop/issue-90752-raw-ptr-shenanigans.rs | 41 +++++++++++++++++++ 1 file changed, 41 insertions(+) create mode 100644 src/test/ui/drop/issue-90752-raw-ptr-shenanigans.rs diff --git a/src/test/ui/drop/issue-90752-raw-ptr-shenanigans.rs b/src/test/ui/drop/issue-90752-raw-ptr-shenanigans.rs new file mode 100644 index 00000000000..4e67b35949e --- /dev/null +++ b/src/test/ui/drop/issue-90752-raw-ptr-shenanigans.rs @@ -0,0 +1,41 @@ +// run-pass + +use std::cell::RefCell; + +struct S<'a>(i32, &'a RefCell>); + +impl<'a> Drop for S<'a> { + fn drop(&mut self) { + self.1.borrow_mut().push(self.0); + } +} + +fn test(drops: &RefCell>) { + let mut foo = None; + let pfoo: *mut _ = &mut foo; + + match foo { + None => (), + _ => return, + } + + // Both S(0) and S(1) should be dropped, but aren't. + unsafe { *pfoo = Some((S(0, drops), S(1, drops))); } + + match foo { + Some((_x, _)) => {} + _ => {} + } +} + +fn main() { + let drops = RefCell::new(Vec::new()); + test(&drops); + + // Ideally, we want this... + //assert_eq!(*drops.borrow(), &[0, 1]); + + // But the delayed access through the raw pointer confuses drop elaboration, + // causing S(1) to be leaked. + assert_eq!(*drops.borrow(), &[0]); +} From 22d937ddfc64bdf1f8724a27a782f2da2ae72be0 Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Thu, 11 Nov 2021 14:55:58 -0800 Subject: [PATCH 4/4] Sanity check for move from an uninit variable whose address is taken --- src/test/ui/moves/move-of-addr-of-mut.rs | 12 ++++++++++++ src/test/ui/moves/move-of-addr-of-mut.stderr | 11 +++++++++++ 2 files changed, 23 insertions(+) create mode 100644 src/test/ui/moves/move-of-addr-of-mut.rs create mode 100644 src/test/ui/moves/move-of-addr-of-mut.stderr diff --git a/src/test/ui/moves/move-of-addr-of-mut.rs b/src/test/ui/moves/move-of-addr-of-mut.rs new file mode 100644 index 00000000000..f2f64e43cd2 --- /dev/null +++ b/src/test/ui/moves/move-of-addr-of-mut.rs @@ -0,0 +1,12 @@ +// Ensure that taking a mutable raw ptr to an uninitialized variable does not change its +// initializedness. + +struct S; + +fn main() { + let mut x: S; + std::ptr::addr_of_mut!(x); //~ borrow of + + let y = x; // Should error here if `addr_of_mut` is ever allowed on uninitialized variables + drop(y); +} diff --git a/src/test/ui/moves/move-of-addr-of-mut.stderr b/src/test/ui/moves/move-of-addr-of-mut.stderr new file mode 100644 index 00000000000..ce8fb028316 --- /dev/null +++ b/src/test/ui/moves/move-of-addr-of-mut.stderr @@ -0,0 +1,11 @@ +error[E0381]: borrow of possibly-uninitialized variable: `x` + --> $DIR/move-of-addr-of-mut.rs:8:5 + | +LL | std::ptr::addr_of_mut!(x); + | ^^^^^^^^^^^^^^^^^^^^^^^^^ use of possibly-uninitialized `x` + | + = note: this error originates in the macro `std::ptr::addr_of_mut` (in Nightly builds, run with -Z macro-backtrace for more info) + +error: aborting due to previous error + +For more information about this error, try `rustc --explain E0381`.