Skip to content

Treat Drop as a rmw operation #107271

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Feb 8, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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 @@ -375,7 +375,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 @@ -390,10 +391,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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it suffice to just do the first part here (create_move_path) instead of having drop_flag_effects_for_location to redo this work?

Copy link
Contributor Author

@zeegomo zeegomo Jan 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I understand what you mean. This builder basically records moves and inits, and Drop is neither of them.
drop_flag_effects_for_location (which may be renamed to initialization_effects) now complements this information with Drops.
What would creating a move path gain us?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, I managed to confuse myself a bit. You're right. It's just a bit scary how far apart the initialization tracking and move checking are.

So the next change would be to change DropAndReplace to stop moving out of the dropped value. The only operation we need to do in the move path builder is to gather the operand with which we are replacing the existing value. There's not even a need to handle this case in initialization tracking logic, as it will be initialized right after. Or do you plan to just remove DropAndReplace in favor of a Drop terminator followed by a regular assignment?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or do you plan to just remove DropAndReplace in favor of a Drop terminator followed by a regular assignment?

yes, that's what I would like to have.

So the next change would be to change DropAndReplace to stop moving out of the dropped value

AFAIK DropAndReplace does not move out of the dropped place.
Indeed, in the move path builder DropAndReplace already does the exact same thing as an Assign (barring the Operand / Rvalue difference)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DropAndReplace already does the exact same thing as an Assign

Yes, as I understand it, DropAndReplace is a hack that was added to work around the following issue: If you treat a drop as being a move, then in this code, x is unnecessarily captured by value.

let mut x = String::new();

|| {
    x = "foo".to_string();
}

}
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 @@ -222,13 +222,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