Skip to content
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.

Sign up for GitHub

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

Make sure we handle backwards_incompatible_lint drops appropriately in drop elaboration #134486

Merged
merged 4 commits into from
Dec 19, 2024
Merged

Make sure we handle backwards_incompatible_lint drops appropriately in drop elaboration #134486

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2e57394
Add a failing test
compiler-errors Dec 18, 2024
5e07901
Separate DropKind::ForLint
compiler-errors Dec 18, 2024
b535061
explain how `build_scope_drops` works
Dec 19, 2024
6564403
pacify merciless fmt
Dec 19, 2024
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
108 changes: 69 additions & 39 deletions compiler/rustc_mir_build/src/builder/scope.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,15 +151,13 @@ struct DropData {

/// Whether this is a value Drop or a StorageDead.
kind: DropKind,

/// Whether this is a backwards-incompatible drop lint
backwards_incompatible_lint: bool,
}

#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
pub(crate) enum DropKind {
Value,
Storage,
ForLint,
}

#[derive(Debug)]
Expand Down Expand Up @@ -248,7 +246,7 @@ impl Scope {
/// use of optimizations in the MIR coroutine transform.
fn needs_cleanup(&self) -> bool {
self.drops.iter().any(|drop| match drop.kind {
DropKind::Value => true,
DropKind::Value | DropKind::ForLint => true,
DropKind::Storage => false,
})
}
Expand Down Expand Up @@ -277,12 +275,8 @@ impl DropTree {
// represents the block in the tree that should be jumped to once all
// of the required drops have been performed.
let fake_source_info = SourceInfo::outermost(DUMMY_SP);
let fake_data = DropData {
source_info: fake_source_info,
local: Local::MAX,
kind: DropKind::Storage,
backwards_incompatible_lint: false,
};
let fake_data =
DropData { source_info: fake_source_info, local: Local::MAX, kind: DropKind::Storage };
let drops = IndexVec::from_raw(vec![DropNode { data: fake_data, next: DropIdx::MAX }]);
Self { drops, entry_points: Vec::new(), existing_drops_map: FxHashMap::default() }
}
Expand Down Expand Up @@ -411,6 +405,27 @@ impl DropTree {
};
cfg.terminate(block, drop_node.data.source_info, terminator);
}
DropKind::ForLint => {
Copy link
Member Author

Choose a reason for hiding this comment

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

I ended up adjusting this to act like DropKind::Storage, but inserting a BackwardIncompatibleDropHint instead of a StorageDead. Previously we'd insert a drop terminator here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense.

let stmt = Statement {
source_info: drop_node.data.source_info,
kind: StatementKind::BackwardIncompatibleDropHint {
place: Box::new(drop_node.data.local.into()),
reason: BackwardIncompatibleDropReason::Edition2024,
},
};
cfg.push(block, stmt);
let target = blocks[drop_node.next].unwrap();
if target != block {
// Diagnostics don't use this `Span` but debuginfo
// might. Since we don't want breakpoints to be placed
// here, especially when this is on an unwind path, we
// use `DUMMY_SP`.
let source_info =
SourceInfo { span: DUMMY_SP, ..drop_node.data.source_info };
let terminator = TerminatorKind::Goto { target };
cfg.terminate(block, source_info, terminator);
}
}
// Root nodes don't correspond to a drop.
DropKind::Storage if drop_idx == ROOT_NODE => {}
DropKind::Storage => {
Expand Down Expand Up @@ -770,12 +785,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
let local =
place.as_local().unwrap_or_else(|| bug!("projection in tail call args"));

Some(DropData {
source_info,
local,
kind: DropKind::Value,
backwards_incompatible_lint: false,
})
Some(DropData { source_info, local, kind: DropKind::Value })
}
Operand::Constant(_) => None,
})
Expand Down Expand Up @@ -822,6 +832,15 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
});
block = next;
}
DropKind::ForLint => {
Copy link
Member Author

Choose a reason for hiding this comment

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

Previously we were inserting a drop terminator for tail calls. Not certain if this is ever reachable, but it's more correct.

self.cfg.push(block, Statement {
source_info,
kind: StatementKind::BackwardIncompatibleDropHint {
place: Box::new(local.into()),
reason: BackwardIncompatibleDropReason::Edition2024,
},
});
}
DropKind::Storage => {
// Only temps and vars need their storage dead.
assert!(local.index() > self.arg_count);
Expand Down Expand Up @@ -1021,7 +1040,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
drop_kind: DropKind,
) {
let needs_drop = match drop_kind {
DropKind::Value => {
DropKind::Value | DropKind::ForLint => {
if !self.local_decls[local].ty.needs_drop(self.tcx, self.typing_env()) {
return;
}
Expand Down Expand Up @@ -1101,7 +1120,6 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
source_info: SourceInfo { span: scope_end, scope: scope.source_scope },
local,
kind: drop_kind,
backwards_incompatible_lint: false,
});

return;
Expand Down Expand Up @@ -1135,8 +1153,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
scope.drops.push(DropData {
source_info: SourceInfo { span: scope_end, scope: scope.source_scope },
local,
kind: DropKind::Value,
backwards_incompatible_lint: true,
kind: DropKind::ForLint,
});

return;
Expand Down Expand Up @@ -1430,25 +1447,38 @@ fn build_scope_drops<'tcx>(
continue;
}

if drop_data.backwards_incompatible_lint {
cfg.push(block, Statement {
source_info,
kind: StatementKind::BackwardIncompatibleDropHint {
place: Box::new(local.into()),
reason: BackwardIncompatibleDropReason::Edition2024,
},
});
} else {
unwind_drops.add_entry_point(block, unwind_to);
let next = cfg.start_new_block();
cfg.terminate(block, source_info, TerminatorKind::Drop {
place: local.into(),
target: next,
unwind: UnwindAction::Continue,
replace: false,
});
block = next;
unwind_drops.add_entry_point(block, unwind_to);
let next = cfg.start_new_block();
cfg.terminate(block, source_info, TerminatorKind::Drop {
place: local.into(),
target: next,
unwind: UnwindAction::Continue,
replace: false,
});
block = next;
}
DropKind::ForLint => {
// 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 scope.moved_locals.iter().any(|&o| o == local) {
continue;
}

if storage_dead_on_unwind {
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not certain if we need to handle this unwind block here.

debug_assert_eq!(unwind_drops.drops[unwind_to].data.local, drop_data.local);
debug_assert_eq!(unwind_drops.drops[unwind_to].data.kind, drop_data.kind);
unwind_to = unwind_drops.drops[unwind_to].next;
}

cfg.push(block, Statement {
source_info,
kind: StatementKind::BackwardIncompatibleDropHint {
place: Box::new(local.into()),
reason: BackwardIncompatibleDropReason::Edition2024,
},
});
}
DropKind::Storage => {
if storage_dead_on_unwind {
Expand Down Expand Up @@ -1500,7 +1530,7 @@ impl<'a, 'tcx: 'a> Builder<'a, 'tcx> {
unwind_indices.push(unwind_indices[drop_node.next]);
}
}
DropKind::Value => {
DropKind::Value | DropKind::ForLint => {
let unwind_drop = self
.scopes
.unwind_drops
Expand Down
74 changes: 25 additions & 49 deletions tests/mir-opt/tail_expr_drop_order_unwind.method_1.ElaborateDrops.after.panic-abort.mir
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,9 @@ fn method_1(_1: Guard) -> () {
let _8: OtherDrop;
let _9: ();
let mut _10: bool;
let mut _11: bool;
let mut _11: isize;
let mut _12: isize;
let mut _13: isize;
let mut _14: isize;
scope 1 {
debug other_drop => _8;
}
Expand All @@ -24,23 +23,21 @@ fn method_1(_1: Guard) -> () {
}

bb0: {
_11 = const false;
_10 = const false;
StorageLive(_2);
StorageLive(_3);
StorageLive(_4);
StorageLive(_5);
StorageLive(_6);
_6 = &_1;
_5 = <Guard as Clone>::clone(move _6) -> [return: bb1, unwind: bb16];
_5 = <Guard as Clone>::clone(move _6) -> [return: bb1, unwind: bb13];
}

bb1: {
_11 = const true;
StorageDead(_6);
_4 = &_5;
_3 = &(*_4);
_2 = method_2(move _3) -> [return: bb2, unwind: bb14];
_2 = method_2(move _3) -> [return: bb2, unwind: bb12];
}

bb2: {
Expand Down Expand Up @@ -78,106 +75,85 @@ fn method_1(_1: Guard) -> () {
bb7: {
backward incompatible drop(_2);
backward incompatible drop(_5);
goto -> bb24;
goto -> bb21;
}

bb8: {
drop(_5) -> [return: bb9, unwind: bb16];
drop(_5) -> [return: bb9, unwind: bb13];
}

bb9: {
_11 = const false;
StorageDead(_5);
StorageDead(_4);
_10 = const false;
StorageDead(_2);
drop(_1) -> [return: bb10, unwind: bb17];
drop(_1) -> [return: bb10, unwind: bb14];
}

bb10: {
return;
}

bb11 (cleanup): {
goto -> bb28;
goto -> bb25;
}

bb12 (cleanup): {
drop(_5) -> [return: bb13, unwind terminate(cleanup)];
}

bb13 (cleanup): {
goto -> bb15;
drop(_1) -> [return: bb14, unwind terminate(cleanup)];
}

bb14 (cleanup): {
drop(_5) -> [return: bb15, unwind terminate(cleanup)];
resume;
}

bb15 (cleanup): {
goto -> bb30;
bb15: {
goto -> bb8;
}

bb16 (cleanup): {
drop(_1) -> [return: bb17, unwind terminate(cleanup)];
goto -> bb12;
}

bb17 (cleanup): {
resume;
goto -> bb12;
}

bb18: {
goto -> bb8;
goto -> bb15;
}

bb19 (cleanup): {
bb19: {
goto -> bb15;
}

bb20 (cleanup): {
goto -> bb15;
goto -> bb12;
}

bb21: {
goto -> bb18;
_11 = discriminant(_2);
switchInt(move _11) -> [0: bb18, otherwise: bb19];
}

bb22: {
goto -> bb18;
}

bb23 (cleanup): {
goto -> bb15;
}

bb24: {
bb22 (cleanup): {
_12 = discriminant(_2);
switchInt(move _12) -> [0: bb21, otherwise: bb22];
switchInt(move _12) -> [0: bb16, otherwise: bb20];
}

bb25 (cleanup): {
_13 = discriminant(_2);
switchInt(move _13) -> [0: bb19, otherwise: bb23];
}

bb26 (cleanup): {
bb23 (cleanup): {
goto -> bb12;
}

bb27 (cleanup): {
bb24 (cleanup): {
goto -> bb12;
}

bb28 (cleanup): {
_14 = discriminant(_2);
switchInt(move _14) -> [0: bb26, otherwise: bb27];
}

bb29 (cleanup): {
drop(_5) -> [return: bb16, unwind terminate(cleanup)];
}

bb30 (cleanup): {
switchInt(copy _11) -> [0: bb16, otherwise: bb29];
bb25 (cleanup): {
_13 = discriminant(_2);
switchInt(move _13) -> [0: bb23, otherwise: bb24];
}
}
Loading
Loading