From 2e57394d8004b155c6f74ca4e2a1106dedfcccc4 Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Wed, 18 Dec 2024 21:54:50 +0000 Subject: [PATCH 1/4] Add a failing test --- ...hod_1.ElaborateDrops.after.panic-abort.mir | 183 ++++++++++++++++++ ...od_1.ElaborateDrops.after.panic-unwind.mir | 183 ++++++++++++++++++ tests/mir-opt/tail_expr_drop_order_unwind.rs | 36 ++++ 3 files changed, 402 insertions(+) create mode 100644 tests/mir-opt/tail_expr_drop_order_unwind.method_1.ElaborateDrops.after.panic-abort.mir create mode 100644 tests/mir-opt/tail_expr_drop_order_unwind.method_1.ElaborateDrops.after.panic-unwind.mir create mode 100644 tests/mir-opt/tail_expr_drop_order_unwind.rs diff --git a/tests/mir-opt/tail_expr_drop_order_unwind.method_1.ElaborateDrops.after.panic-abort.mir b/tests/mir-opt/tail_expr_drop_order_unwind.method_1.ElaborateDrops.after.panic-abort.mir new file mode 100644 index 0000000000000..54bedfdc0af1e --- /dev/null +++ b/tests/mir-opt/tail_expr_drop_order_unwind.method_1.ElaborateDrops.after.panic-abort.mir @@ -0,0 +1,183 @@ +// MIR for `method_1` after ElaborateDrops + +fn method_1(_1: Guard) -> () { + debug g => _1; + let mut _0: (); + let mut _2: std::result::Result; + let mut _3: &Guard; + let _4: &Guard; + let _5: Guard; + let mut _6: &Guard; + let mut _7: isize; + let _8: OtherDrop; + let _9: (); + let mut _10: bool; + let mut _11: bool; + let mut _12: isize; + let mut _13: isize; + let mut _14: isize; + scope 1 { + debug other_drop => _8; + } + scope 2 { + debug err => _9; + } + + bb0: { + _11 = const false; + _10 = const false; + StorageLive(_2); + StorageLive(_3); + StorageLive(_4); + StorageLive(_5); + StorageLive(_6); + _6 = &_1; + _5 = ::clone(move _6) -> [return: bb1, unwind: bb16]; + } + + bb1: { + _11 = const true; + StorageDead(_6); + _4 = &_5; + _3 = &(*_4); + _2 = method_2(move _3) -> [return: bb2, unwind: bb14]; + } + + bb2: { + _10 = const true; + StorageDead(_3); + PlaceMention(_2); + _7 = discriminant(_2); + switchInt(move _7) -> [0: bb5, 1: bb4, otherwise: bb3]; + } + + bb3: { + unreachable; + } + + bb4: { + StorageLive(_9); + _9 = copy ((_2 as Err).0: ()); + _0 = const (); + StorageDead(_9); + goto -> bb7; + } + + bb5: { + StorageLive(_8); + _8 = move ((_2 as Ok).0: OtherDrop); + _0 = const (); + drop(_8) -> [return: bb6, unwind: bb11]; + } + + bb6: { + StorageDead(_8); + goto -> bb7; + } + + bb7: { + backward incompatible drop(_2); + backward incompatible drop(_5); + goto -> bb24; + } + + bb8: { + drop(_5) -> [return: bb9, unwind: bb16]; + } + + bb9: { + _11 = const false; + StorageDead(_5); + StorageDead(_4); + _10 = const false; + StorageDead(_2); + drop(_1) -> [return: bb10, unwind: bb17]; + } + + bb10: { + return; + } + + bb11 (cleanup): { + goto -> bb28; + } + + bb12 (cleanup): { + drop(_5) -> [return: bb13, unwind terminate(cleanup)]; + } + + bb13 (cleanup): { + goto -> bb15; + } + + bb14 (cleanup): { + drop(_5) -> [return: bb15, unwind terminate(cleanup)]; + } + + bb15 (cleanup): { + goto -> bb30; + } + + bb16 (cleanup): { + drop(_1) -> [return: bb17, unwind terminate(cleanup)]; + } + + bb17 (cleanup): { + resume; + } + + bb18: { + goto -> bb8; + } + + bb19 (cleanup): { + goto -> bb15; + } + + bb20 (cleanup): { + goto -> bb15; + } + + bb21: { + goto -> bb18; + } + + bb22: { + goto -> bb18; + } + + bb23 (cleanup): { + goto -> bb15; + } + + bb24: { + _12 = discriminant(_2); + switchInt(move _12) -> [0: bb21, otherwise: bb22]; + } + + bb25 (cleanup): { + _13 = discriminant(_2); + switchInt(move _13) -> [0: bb19, otherwise: bb23]; + } + + bb26 (cleanup): { + goto -> bb12; + } + + bb27 (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]; + } +} diff --git a/tests/mir-opt/tail_expr_drop_order_unwind.method_1.ElaborateDrops.after.panic-unwind.mir b/tests/mir-opt/tail_expr_drop_order_unwind.method_1.ElaborateDrops.after.panic-unwind.mir new file mode 100644 index 0000000000000..54bedfdc0af1e --- /dev/null +++ b/tests/mir-opt/tail_expr_drop_order_unwind.method_1.ElaborateDrops.after.panic-unwind.mir @@ -0,0 +1,183 @@ +// MIR for `method_1` after ElaborateDrops + +fn method_1(_1: Guard) -> () { + debug g => _1; + let mut _0: (); + let mut _2: std::result::Result; + let mut _3: &Guard; + let _4: &Guard; + let _5: Guard; + let mut _6: &Guard; + let mut _7: isize; + let _8: OtherDrop; + let _9: (); + let mut _10: bool; + let mut _11: bool; + let mut _12: isize; + let mut _13: isize; + let mut _14: isize; + scope 1 { + debug other_drop => _8; + } + scope 2 { + debug err => _9; + } + + bb0: { + _11 = const false; + _10 = const false; + StorageLive(_2); + StorageLive(_3); + StorageLive(_4); + StorageLive(_5); + StorageLive(_6); + _6 = &_1; + _5 = ::clone(move _6) -> [return: bb1, unwind: bb16]; + } + + bb1: { + _11 = const true; + StorageDead(_6); + _4 = &_5; + _3 = &(*_4); + _2 = method_2(move _3) -> [return: bb2, unwind: bb14]; + } + + bb2: { + _10 = const true; + StorageDead(_3); + PlaceMention(_2); + _7 = discriminant(_2); + switchInt(move _7) -> [0: bb5, 1: bb4, otherwise: bb3]; + } + + bb3: { + unreachable; + } + + bb4: { + StorageLive(_9); + _9 = copy ((_2 as Err).0: ()); + _0 = const (); + StorageDead(_9); + goto -> bb7; + } + + bb5: { + StorageLive(_8); + _8 = move ((_2 as Ok).0: OtherDrop); + _0 = const (); + drop(_8) -> [return: bb6, unwind: bb11]; + } + + bb6: { + StorageDead(_8); + goto -> bb7; + } + + bb7: { + backward incompatible drop(_2); + backward incompatible drop(_5); + goto -> bb24; + } + + bb8: { + drop(_5) -> [return: bb9, unwind: bb16]; + } + + bb9: { + _11 = const false; + StorageDead(_5); + StorageDead(_4); + _10 = const false; + StorageDead(_2); + drop(_1) -> [return: bb10, unwind: bb17]; + } + + bb10: { + return; + } + + bb11 (cleanup): { + goto -> bb28; + } + + bb12 (cleanup): { + drop(_5) -> [return: bb13, unwind terminate(cleanup)]; + } + + bb13 (cleanup): { + goto -> bb15; + } + + bb14 (cleanup): { + drop(_5) -> [return: bb15, unwind terminate(cleanup)]; + } + + bb15 (cleanup): { + goto -> bb30; + } + + bb16 (cleanup): { + drop(_1) -> [return: bb17, unwind terminate(cleanup)]; + } + + bb17 (cleanup): { + resume; + } + + bb18: { + goto -> bb8; + } + + bb19 (cleanup): { + goto -> bb15; + } + + bb20 (cleanup): { + goto -> bb15; + } + + bb21: { + goto -> bb18; + } + + bb22: { + goto -> bb18; + } + + bb23 (cleanup): { + goto -> bb15; + } + + bb24: { + _12 = discriminant(_2); + switchInt(move _12) -> [0: bb21, otherwise: bb22]; + } + + bb25 (cleanup): { + _13 = discriminant(_2); + switchInt(move _13) -> [0: bb19, otherwise: bb23]; + } + + bb26 (cleanup): { + goto -> bb12; + } + + bb27 (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]; + } +} diff --git a/tests/mir-opt/tail_expr_drop_order_unwind.rs b/tests/mir-opt/tail_expr_drop_order_unwind.rs new file mode 100644 index 0000000000000..b67b358087502 --- /dev/null +++ b/tests/mir-opt/tail_expr_drop_order_unwind.rs @@ -0,0 +1,36 @@ +// skip-filecheck +// EMIT_MIR_FOR_EACH_PANIC_STRATEGY +// EMIT_MIR tail_expr_drop_order_unwind.method_1.ElaborateDrops.after.mir + +#![deny(tail_expr_drop_order)] + +use std::backtrace::Backtrace; + +#[derive(Clone)] +struct Guard; +impl Drop for Guard { + fn drop(&mut self) { + println!("Drop!"); + } +} + +#[derive(Clone)] +struct OtherDrop; +impl Drop for OtherDrop { + fn drop(&mut self) { + println!("Drop!"); + } +} + +fn method_1(g: Guard) { + match method_2(&g.clone()) { + Ok(other_drop) => { + // repro needs something else being dropped too. + }, + Err(err) => {}, + } +} + +fn method_2(_: &Guard) -> Result { + panic!("Method 2 panics!"); +} From 5e079011eafbb1d5fc779c14c7a29d4a620574f9 Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Wed, 18 Dec 2024 21:57:20 +0000 Subject: [PATCH 2/4] Separate DropKind::ForLint --- compiler/rustc_mir_build/src/builder/scope.rs | 108 +++++++++++------- ...hod_1.ElaborateDrops.after.panic-abort.mir | 74 ++++-------- ...od_1.ElaborateDrops.after.panic-unwind.mir | 74 ++++-------- tests/mir-opt/tail_expr_drop_order_unwind.rs | 4 +- 4 files changed, 121 insertions(+), 139 deletions(-) diff --git a/compiler/rustc_mir_build/src/builder/scope.rs b/compiler/rustc_mir_build/src/builder/scope.rs index 882e29de46d34..19ab7a44e0dc7 100644 --- a/compiler/rustc_mir_build/src/builder/scope.rs +++ b/compiler/rustc_mir_build/src/builder/scope.rs @@ -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)] @@ -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, }) } @@ -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() } } @@ -411,6 +405,27 @@ impl DropTree { }; cfg.terminate(block, drop_node.data.source_info, terminator); } + DropKind::ForLint => { + 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 => { @@ -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, }) @@ -822,6 +832,15 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { }); block = next; } + DropKind::ForLint => { + 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); @@ -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; } @@ -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; @@ -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; @@ -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 { + 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 { @@ -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 diff --git a/tests/mir-opt/tail_expr_drop_order_unwind.method_1.ElaborateDrops.after.panic-abort.mir b/tests/mir-opt/tail_expr_drop_order_unwind.method_1.ElaborateDrops.after.panic-abort.mir index 54bedfdc0af1e..e9bbe30bd774f 100644 --- a/tests/mir-opt/tail_expr_drop_order_unwind.method_1.ElaborateDrops.after.panic-abort.mir +++ b/tests/mir-opt/tail_expr_drop_order_unwind.method_1.ElaborateDrops.after.panic-abort.mir @@ -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; } @@ -24,7 +23,6 @@ fn method_1(_1: Guard) -> () { } bb0: { - _11 = const false; _10 = const false; StorageLive(_2); StorageLive(_3); @@ -32,15 +30,14 @@ fn method_1(_1: Guard) -> () { StorageLive(_5); StorageLive(_6); _6 = &_1; - _5 = ::clone(move _6) -> [return: bb1, unwind: bb16]; + _5 = ::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: { @@ -78,20 +75,19 @@ 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: { @@ -99,7 +95,7 @@ fn method_1(_1: Guard) -> () { } bb11 (cleanup): { - goto -> bb28; + goto -> bb25; } bb12 (cleanup): { @@ -107,77 +103,57 @@ fn method_1(_1: Guard) -> () { } 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]; } } diff --git a/tests/mir-opt/tail_expr_drop_order_unwind.method_1.ElaborateDrops.after.panic-unwind.mir b/tests/mir-opt/tail_expr_drop_order_unwind.method_1.ElaborateDrops.after.panic-unwind.mir index 54bedfdc0af1e..e9bbe30bd774f 100644 --- a/tests/mir-opt/tail_expr_drop_order_unwind.method_1.ElaborateDrops.after.panic-unwind.mir +++ b/tests/mir-opt/tail_expr_drop_order_unwind.method_1.ElaborateDrops.after.panic-unwind.mir @@ -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; } @@ -24,7 +23,6 @@ fn method_1(_1: Guard) -> () { } bb0: { - _11 = const false; _10 = const false; StorageLive(_2); StorageLive(_3); @@ -32,15 +30,14 @@ fn method_1(_1: Guard) -> () { StorageLive(_5); StorageLive(_6); _6 = &_1; - _5 = ::clone(move _6) -> [return: bb1, unwind: bb16]; + _5 = ::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: { @@ -78,20 +75,19 @@ 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: { @@ -99,7 +95,7 @@ fn method_1(_1: Guard) -> () { } bb11 (cleanup): { - goto -> bb28; + goto -> bb25; } bb12 (cleanup): { @@ -107,77 +103,57 @@ fn method_1(_1: Guard) -> () { } 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]; } } diff --git a/tests/mir-opt/tail_expr_drop_order_unwind.rs b/tests/mir-opt/tail_expr_drop_order_unwind.rs index b67b358087502..065e08c340963 100644 --- a/tests/mir-opt/tail_expr_drop_order_unwind.rs +++ b/tests/mir-opt/tail_expr_drop_order_unwind.rs @@ -26,8 +26,8 @@ fn method_1(g: Guard) { match method_2(&g.clone()) { Ok(other_drop) => { // repro needs something else being dropped too. - }, - Err(err) => {}, + } + Err(err) => {} } } From b5350610608b724a3f152ccd889a78f82860ed69 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Thu, 19 Dec 2024 13:53:35 +0000 Subject: [PATCH 3/4] explain how `build_scope_drops` works --- compiler/rustc_mir_build/src/builder/scope.rs | 40 ++++++++++++++++++- 1 file changed, 38 insertions(+), 2 deletions(-) diff --git a/compiler/rustc_mir_build/src/builder/scope.rs b/compiler/rustc_mir_build/src/builder/scope.rs index 19ab7a44e0dc7..c5463e514c3ba 100644 --- a/compiler/rustc_mir_build/src/builder/scope.rs +++ b/compiler/rustc_mir_build/src/builder/scope.rs @@ -1396,12 +1396,23 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { } /// Builds drops for `pop_scope` and `leave_top_scope`. +/// +/// # Parameters +/// +/// * `unwind_drops`, the drop tree data structure storing what needs to be cleaned up if unwind occurs +/// * `scope`, describes the drops that will occur on exiting the scope in regular execution +/// * `block`, the block to branch to once drops are complete (assuming no unwind occurs) +/// * `unwind_to`, describes the drops that would occur at this point in the code if a +/// panic occurred (a subset of the drops in `scope`, since we sometimes elide StorageDead and other +/// instructions on unwinding) +/// * `storage_dead_on_unwind`, if true, then we should emit `StorageDead` even when unwinding +/// * `arg_count`, number of MIR local variables corresponding to fn arguments (used to assert that we don't drop those) fn build_scope_drops<'tcx>( cfg: &mut CFG<'tcx>, unwind_drops: &mut DropTree, scope: &Scope, - mut block: BasicBlock, - mut unwind_to: DropIdx, + block: BasicBlock, + unwind_to: DropIdx, storage_dead_on_unwind: bool, arg_count: usize, ) -> BlockAnd<()> { @@ -1425,6 +1436,18 @@ fn build_scope_drops<'tcx>( // statement. For other functions we don't worry about StorageDead. The // drops for the unwind path should have already been generated by // `diverge_cleanup_gen`. + + // `unwind_to` indicates what needs to be dropped should unwinding occur. + // This is a subset of what needs to be dropped when exiting the scope. + // As we unwind the scope, we will also move `unwind_to` backwards to match, + // so that we can use it should a destructor panic. + let mut unwind_to = unwind_to; + + // The block that we should jump to after drops complete. We start by building the final drop (`drops[n]` + // in the diagram above) and then build the drops (e.g., `drop[1]`, `drop[0]`) that come before it. + // block begins as the successor of `drops[n]` and then becomes `drops[n]` so that `drops[n-1]` + // will branch to `drops[n]`. + let mut block = block; for drop_data in scope.drops.iter().rev() { let source_info = drop_data.source_info; @@ -1435,6 +1458,9 @@ fn build_scope_drops<'tcx>( // `unwind_to` should drop the value that we're about to // schedule. If dropping this value panics, then we continue // with the *next* value on the unwind path. + // + // We adjust this BEFORE we create the drop (e.g., `drops[n]`) + // because `drops[n]` should unwind to `drops[n-1]`. 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; @@ -1466,6 +1492,11 @@ fn build_scope_drops<'tcx>( continue; } + // As in the `DropKind::Storage` case below: + // normally lint-related drops are not emitted for unwind, + // so we can just leave `unwind_to` unmodified, but in some + // cases we emit things ALSO on the unwind path, so we need to adjust + // `unwind_to` in that case. if storage_dead_on_unwind { 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); @@ -1481,6 +1512,11 @@ fn build_scope_drops<'tcx>( }); } DropKind::Storage => { + // Ordinarily, storage-dead nodes are not emitted on unwind, so we don't + // need to adjust `unwind_to` on this path. However, in some specific cases + // we *do* emit storage-dead nodes on the unwind path, and in that case now that + // the storage-dead has completed, we need to adjust the `unwind_to` pointer + // so that any future drops we emit will not register storage-dead. if storage_dead_on_unwind { 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); From 6564403641afde8bf445914ec2996fe7219289ab Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Thu, 19 Dec 2024 14:32:25 +0000 Subject: [PATCH 4/4] pacify merciless fmt --- compiler/rustc_mir_build/src/builder/scope.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/compiler/rustc_mir_build/src/builder/scope.rs b/compiler/rustc_mir_build/src/builder/scope.rs index c5463e514c3ba..a51fe4c52fa3c 100644 --- a/compiler/rustc_mir_build/src/builder/scope.rs +++ b/compiler/rustc_mir_build/src/builder/scope.rs @@ -1396,9 +1396,9 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { } /// Builds drops for `pop_scope` and `leave_top_scope`. -/// +/// /// # Parameters -/// +/// /// * `unwind_drops`, the drop tree data structure storing what needs to be cleaned up if unwind occurs /// * `scope`, describes the drops that will occur on exiting the scope in regular execution /// * `block`, the block to branch to once drops are complete (assuming no unwind occurs) @@ -1436,14 +1436,14 @@ fn build_scope_drops<'tcx>( // statement. For other functions we don't worry about StorageDead. The // drops for the unwind path should have already been generated by // `diverge_cleanup_gen`. - + // `unwind_to` indicates what needs to be dropped should unwinding occur. // This is a subset of what needs to be dropped when exiting the scope. // As we unwind the scope, we will also move `unwind_to` backwards to match, // so that we can use it should a destructor panic. let mut unwind_to = unwind_to; - // The block that we should jump to after drops complete. We start by building the final drop (`drops[n]` + // The block that we should jump to after drops complete. We start by building the final drop (`drops[n]` // in the diagram above) and then build the drops (e.g., `drop[1]`, `drop[0]`) that come before it. // block begins as the successor of `drops[n]` and then becomes `drops[n]` so that `drops[n-1]` // will branch to `drops[n]`. @@ -1492,7 +1492,7 @@ fn build_scope_drops<'tcx>( continue; } - // As in the `DropKind::Storage` case below: + // As in the `DropKind::Storage` case below: // normally lint-related drops are not emitted for unwind, // so we can just leave `unwind_to` unmodified, but in some // cases we emit things ALSO on the unwind path, so we need to adjust