Skip to content

Commit

Permalink
Rollup merge of rust-lang#107271 - Zeegomo:drop-rmw, r=oli-obk
Browse files Browse the repository at this point in the history
Treat Drop as a rmw operation

Previously, a Drop terminator was considered a move in MIR. This commit changes the behavior to only treat Drop as a mutable access to the dropped place.

In order for this change to be correct, we need to guarantee that

1.  A dropped value won't be used again
   2.  Places that appear in a drop won't be used again before a
     subsequent initialization.

We can ensure this to be correct at MIR construction because Drop will only be emitted when a variable goes out of scope, thus having:
*   (1) as there is no way of reaching the old value. drop-elaboration
     will also remove any uninitialized drop.
 * (2) as the place can't be named following the end of the scope.

However, the initialization status, previously tracked by moves, should also be tied to the execution of a Drop, hence the additional logic in the dataflow analyses.

From discussion in [this thread](https://rust-lang.zulipchat.com/#narrow/stream/233931-t-compiler.2Fmajor-changes/topic/.60DROP.60.20to.20.60DROP_IF.60.20compiler-team.23558), originating from rust-lang/compiler-team#558.
See also rust-lang#104488 (comment)
  • Loading branch information
Dylan-DPC authored Feb 8, 2023
2 parents 339c152 + 68c1e2f commit 04607d6
Show file tree
Hide file tree
Showing 4 changed files with 46 additions and 9 deletions.
13 changes: 12 additions & 1 deletion compiler/rustc_mir_dataflow/src/drop_flag_effects.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use crate::elaborate_drops::DropFlagState;
use rustc_middle::mir::{self, Body, Location};
use rustc_middle::mir::{self, Body, Location, Terminator, TerminatorKind};
use rustc_middle::ty::{self, TyCtxt};
use rustc_target::abi::VariantIdx;

Expand Down Expand Up @@ -194,6 +194,17 @@ pub fn drop_flag_effects_for_location<'tcx, F>(
on_all_children_bits(tcx, body, move_data, path, |mpi| callback(mpi, DropFlagState::Absent))
}

// Drop does not count as a move but we should still consider the variable uninitialized.
if let Some(Terminator { kind: TerminatorKind::Drop { place, .. }, .. }) =
body.stmt_at(loc).right()
{
if let LookupResult::Exact(mpi) = move_data.rev_lookup.find(place.as_ref()) {
on_all_children_bits(tcx, body, move_data, mpi, |mpi| {
callback(mpi, DropFlagState::Absent)
})
}
}

debug!("drop_flag_effects: assignment for location({:?})", loc);

for_location_inits(tcx, body, move_data, loc, |mpi| callback(mpi, DropFlagState::Present));
Expand Down
7 changes: 2 additions & 5 deletions compiler/rustc_mir_dataflow/src/move_paths/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -376,7 +376,8 @@ impl<'b, 'a, 'tcx> Gatherer<'b, 'a, 'tcx> {
| TerminatorKind::Resume
| TerminatorKind::Abort
| TerminatorKind::GeneratorDrop
| TerminatorKind::Unreachable => {}
| TerminatorKind::Unreachable
| TerminatorKind::Drop { .. } => {}

TerminatorKind::Assert { ref cond, .. } => {
self.gather_operand(cond);
Expand All @@ -391,10 +392,6 @@ impl<'b, 'a, 'tcx> Gatherer<'b, 'a, 'tcx> {
self.create_move_path(place);
self.gather_init(place.as_ref(), InitKind::Deep);
}

TerminatorKind::Drop { place, target: _, unwind: _ } => {
self.gather_move(place);
}
TerminatorKind::DropAndReplace { place, ref value, .. } => {
self.create_move_path(place);
self.gather_operand(value);
Expand Down
6 changes: 3 additions & 3 deletions compiler/rustc_mir_dataflow/src/value_analysis.rs
Original file line number Diff line number Diff line change
Expand Up @@ -223,13 +223,13 @@ pub trait ValueAnalysis<'tcx> {
self.super_terminator(terminator, state)
}

fn super_terminator(&self, terminator: &Terminator<'tcx>, _state: &mut State<Self::Value>) {
fn super_terminator(&self, terminator: &Terminator<'tcx>, state: &mut State<Self::Value>) {
match &terminator.kind {
TerminatorKind::Call { .. } | TerminatorKind::InlineAsm { .. } => {
// Effect is applied by `handle_call_return`.
}
TerminatorKind::Drop { .. } => {
// We don't track dropped places.
TerminatorKind::Drop { place, .. } => {
state.flood_with(place.as_ref(), self.map(), Self::Value::bottom());
}
TerminatorKind::DropAndReplace { .. } | TerminatorKind::Yield { .. } => {
// They would have an effect, but are not allowed in this phase.
Expand Down
29 changes: 29 additions & 0 deletions compiler/rustc_mir_transform/src/elaborate_drops.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,35 @@ use rustc_span::Span;
use rustc_target::abi::VariantIdx;
use std::fmt;

/// During MIR building, Drop and DropAndReplace terminators are inserted in every place where a drop may occur.
/// However, in this phase, the presence of these terminators does not guarantee that a destructor will run,
/// as the target of the drop may be uninitialized.
/// In general, the compiler cannot determine at compile time whether a destructor will run or not.
///
/// At a high level, this pass refines Drop and DropAndReplace to only run the destructor if the
/// target is initialized. The way this is achievied is by inserting drop flags for every variable
/// that may be dropped, and then using those flags to determine whether a destructor should run.
/// This pass also removes DropAndReplace, replacing it with a Drop paired with an assign statement.
/// Once this is complete, Drop terminators in the MIR correspond to a call to the "drop glue" or
/// "drop shim" for the type of the dropped place.
///
/// This pass relies on dropped places having an associated move path, which is then used to determine
/// the initialization status of the place and its descendants.
/// It's worth noting that a MIR containing a Drop without an associated move path is probably ill formed,
/// as it would allow running a destructor on a place behind a reference:
///
/// ```text
// fn drop_term<T>(t: &mut T) {
// mir!(
// {
// Drop(*t, exit)
// }
// exit = {
// Return()
// }
// )
// }
/// ```
pub struct ElaborateDrops;

impl<'tcx> MirPass<'tcx> for ElaborateDrops {
Expand Down

0 comments on commit 04607d6

Please sign in to comment.