From 68b7475dc04d4429d4bfb4837a902090915b6584 Mon Sep 17 00:00:00 2001 From: Ariel Ben-Yehuda Date: Mon, 15 May 2017 15:22:59 +0300 Subject: [PATCH] move "ADT master drop flag" logic to `open_drop_for_adt_contents` Fixes #41888. --- src/librustc_mir/util/elaborate_drops.rs | 273 ++++++++++++----------- src/test/mir-opt/issue-41888.rs | 186 +++++++++++++++ src/test/run-pass/dynamic-drop.rs | 25 +++ 3 files changed, 351 insertions(+), 133 deletions(-) create mode 100644 src/test/mir-opt/issue-41888.rs diff --git a/src/librustc_mir/util/elaborate_drops.rs b/src/librustc_mir/util/elaborate_drops.rs index afec04dafe314..3ec27db60c2bf 100644 --- a/src/librustc_mir/util/elaborate_drops.rs +++ b/src/librustc_mir/util/elaborate_drops.rs @@ -243,30 +243,37 @@ impl<'l, 'b, 'tcx, D> DropCtxt<'l, 'b, 'tcx, D> } /// Create one-half of the drop ladder for a list of fields, and return - /// the list of steps in it in reverse order. + /// the list of steps in it in reverse order, with the first step + /// dropping 0 fields and so on. /// /// `unwind_ladder` is such a list of steps in reverse order, /// which is called if the matching step of the drop glue panics. fn drop_halfladder(&mut self, unwind_ladder: &[Unwind], - succ: BasicBlock, + mut succ: BasicBlock, fields: &[(Lvalue<'tcx>, Option)]) -> Vec { - let goto = TerminatorKind::Goto { target: succ }; - let mut succ = self.new_block(unwind_ladder[0], goto); - - // Always clear the "master" drop flag at the bottom of the - // ladder. This is needed because the "master" drop flag - // protects the ADT's discriminant, which is invalidated - // after the ADT is dropped. - let succ_loc = Location { block: succ, statement_index: 0 }; - self.elaborator.clear_drop_flag(succ_loc, self.path, DropFlagMode::Shallow); - - fields.iter().rev().zip(unwind_ladder).map(|(&(ref lv, path), &unwind_succ)| { - succ = self.drop_subpath(lv, path, succ, unwind_succ); - succ - }).collect() + Some(succ).into_iter().chain( + fields.iter().rev().zip(unwind_ladder) + .map(|(&(ref lv, path), &unwind_succ)| { + succ = self.drop_subpath(lv, path, succ, unwind_succ); + succ + }) + ).collect() + } + + fn drop_ladder_bottom(&mut self) -> (BasicBlock, Unwind) { + // Clear the "master" drop flag at the end. This is needed + // because the "master" drop protects the ADT's discriminant, + // which is invalidated after the ADT is dropped. + let (succ, unwind) = (self.succ, self.unwind); // FIXME(#6393) + ( + self.drop_flag_reset_block(DropFlagMode::Shallow, succ, unwind), + unwind.map(|unwind| { + self.drop_flag_reset_block(DropFlagMode::Shallow, unwind, Unwind::InCleanup) + }) + ) } /// Create a full drop ladder, consisting of 2 connected half-drop-ladders @@ -283,8 +290,13 @@ impl<'l, 'b, 'tcx, D> DropCtxt<'l, 'b, 'tcx, D> /// ELAB(drop location.1 [target=.c2]) /// .c2: /// ELAB(drop location.2 [target=`self.unwind`]) + /// + /// NOTE: this does not clear the master drop flag, so you need + /// to point succ/unwind on a `drop_ladder_bottom`. fn drop_ladder<'a>(&mut self, - fields: Vec<(Lvalue<'tcx>, Option)>) + fields: Vec<(Lvalue<'tcx>, Option)>, + succ: BasicBlock, + unwind: Unwind) -> (BasicBlock, Unwind) { debug!("drop_ladder({:?}, {:?})", self, fields); @@ -297,20 +309,17 @@ impl<'l, 'b, 'tcx, D> DropCtxt<'l, 'b, 'tcx, D> debug!("drop_ladder - fields needing drop: {:?}", fields); let unwind_ladder = vec![Unwind::InCleanup; fields.len() + 1]; - let unwind_ladder: Vec<_> = if let Unwind::To(target) = self.unwind { + let unwind_ladder: Vec<_> = if let Unwind::To(target) = unwind { let halfladder = self.drop_halfladder(&unwind_ladder, target, &fields); - Some(self.unwind).into_iter().chain(halfladder.into_iter().map(Unwind::To)) - .collect() + halfladder.into_iter().map(Unwind::To).collect() } else { unwind_ladder }; - let succ = self.succ; // FIXME(#6393) let normal_ladder = self.drop_halfladder(&unwind_ladder, succ, &fields); - (normal_ladder.last().cloned().unwrap_or(succ), - unwind_ladder.last().cloned().unwrap_or(self.unwind)) + (*normal_ladder.last().unwrap(), *unwind_ladder.last().unwrap()) } fn open_drop_for_tuple<'a>(&mut self, tys: &[Ty<'tcx>]) @@ -323,7 +332,8 @@ impl<'l, 'b, 'tcx, D> DropCtxt<'l, 'b, 'tcx, D> self.elaborator.field_subpath(self.path, Field::new(i))) }).collect(); - self.drop_ladder(fields).0 + let (succ, unwind) = self.drop_ladder_bottom(); + self.drop_ladder(fields, succ, unwind).0 } fn open_drop_for_box<'a>(&mut self, ty: Ty<'tcx>) -> BasicBlock @@ -370,106 +380,100 @@ impl<'l, 'b, 'tcx, D> DropCtxt<'l, 'b, 'tcx, D> } } - fn open_drop_for_adt_contents<'a>(&mut self, adt: &'tcx ty::AdtDef, - substs: &'tcx Substs<'tcx>) - -> (BasicBlock, Unwind) { - match adt.variants.len() { - 1 => { - let fields = self.move_paths_for_fields( - self.lvalue, - self.path, - &adt.variants[0], - substs - ); - self.drop_ladder(fields) - } - _ => { - let succ = self.succ; - let unwind = self.unwind; // FIXME(#6393) + fn open_drop_for_adt_contents(&mut self, adt: &'tcx ty::AdtDef, + substs: &'tcx Substs<'tcx>) + -> (BasicBlock, Unwind) { + let (succ, unwind) = self.drop_ladder_bottom(); + if adt.variants.len() == 1 { + let fields = self.move_paths_for_fields( + self.lvalue, + self.path, + &adt.variants[0], + substs + ); + self.drop_ladder(fields, succ, unwind) + } else { + self.open_drop_for_multivariant(adt, substs, succ, unwind) + } + } + + fn open_drop_for_multivariant(&mut self, adt: &'tcx ty::AdtDef, + substs: &'tcx Substs<'tcx>, + succ: BasicBlock, + unwind: Unwind) + -> (BasicBlock, Unwind) { + let mut values = Vec::with_capacity(adt.variants.len()); + let mut normal_blocks = Vec::with_capacity(adt.variants.len()); + let mut unwind_blocks = if unwind.is_cleanup() { + None + } else { + Some(Vec::with_capacity(adt.variants.len())) + }; + + let mut have_otherwise = false; - let mut values = Vec::with_capacity(adt.variants.len()); - let mut normal_blocks = Vec::with_capacity(adt.variants.len()); - let mut unwind_blocks = if unwind.is_cleanup() { - None - } else { - Some(Vec::with_capacity(adt.variants.len())) - }; - let mut otherwise = None; - let mut unwind_otherwise = None; - for (variant_index, discr) in adt.discriminants(self.tcx()).enumerate() { - let subpath = self.elaborator.downcast_subpath( - self.path, variant_index); - if let Some(variant_path) = subpath { - let base_lv = self.lvalue.clone().elem( - ProjectionElem::Downcast(adt, variant_index) + for (variant_index, discr) in adt.discriminants(self.tcx()).enumerate() { + let subpath = self.elaborator.downcast_subpath( + self.path, variant_index); + if let Some(variant_path) = subpath { + let base_lv = self.lvalue.clone().elem( + ProjectionElem::Downcast(adt, variant_index) ); - let fields = self.move_paths_for_fields( - &base_lv, - variant_path, - &adt.variants[variant_index], - substs); - values.push(discr); - if let Unwind::To(unwind) = unwind { - // We can't use the half-ladder from the original - // drop ladder, because this breaks the - // "funclet can't have 2 successor funclets" - // requirement from MSVC: - // - // switch unwind-switch - // / \ / \ - // v1.0 v2.0 v2.0-unwind v1.0-unwind - // | | / | - // v1.1-unwind v2.1-unwind | - // ^ | - // \-------------------------------/ - // - // Create a duplicate half-ladder to avoid that. We - // could technically only do this on MSVC, but I - // I want to minimize the divergence between MSVC - // and non-MSVC. - - let unwind_blocks = unwind_blocks.as_mut().unwrap(); - let unwind_ladder = vec![Unwind::InCleanup; fields.len() + 1]; - let halfladder = - self.drop_halfladder(&unwind_ladder, unwind, &fields); - unwind_blocks.push(halfladder.last().cloned().unwrap_or(unwind)); - } - let (normal, _) = self.drop_ladder(fields); - normal_blocks.push(normal); - } else { - // variant not found - drop the entire enum - if let None = otherwise { - otherwise = Some(self.complete_drop( - Some(DropFlagMode::Shallow), - succ, - unwind)); - if let Unwind::To(unwind) = unwind { - unwind_otherwise = Some(self.complete_drop( - Some(DropFlagMode::Shallow), - unwind, - Unwind::InCleanup - )); - } - } - } - } - if let Some(block) = otherwise { - normal_blocks.push(block); - if let Some(ref mut unwind_blocks) = unwind_blocks { - unwind_blocks.push(unwind_otherwise.unwrap()); - } - } else { - values.pop(); + let fields = self.move_paths_for_fields( + &base_lv, + variant_path, + &adt.variants[variant_index], + substs); + values.push(discr); + if let Unwind::To(unwind) = unwind { + // We can't use the half-ladder from the original + // drop ladder, because this breaks the + // "funclet can't have 2 successor funclets" + // requirement from MSVC: + // + // switch unwind-switch + // / \ / \ + // v1.0 v2.0 v2.0-unwind v1.0-unwind + // | | / | + // v1.1-unwind v2.1-unwind | + // ^ | + // \-------------------------------/ + // + // Create a duplicate half-ladder to avoid that. We + // could technically only do this on MSVC, but I + // I want to minimize the divergence between MSVC + // and non-MSVC. + + let unwind_blocks = unwind_blocks.as_mut().unwrap(); + let unwind_ladder = vec![Unwind::InCleanup; fields.len() + 1]; + let halfladder = + self.drop_halfladder(&unwind_ladder, unwind, &fields); + unwind_blocks.push(halfladder.last().cloned().unwrap()); } + let (normal, _) = self.drop_ladder(fields, succ, unwind); + normal_blocks.push(normal); + } else { + have_otherwise = true; + } + } - (self.adt_switch_block(adt, normal_blocks, &values, succ, unwind), - unwind.map(|unwind| { - self.adt_switch_block( - adt, unwind_blocks.unwrap(), &values, unwind, Unwind::InCleanup - ) - })) + if have_otherwise { + normal_blocks.push(self.drop_block(succ, unwind)); + if let Unwind::To(unwind) = unwind { + unwind_blocks.as_mut().unwrap().push( + self.drop_block(unwind, Unwind::InCleanup) + ); } + } else { + values.pop(); } + + (self.adt_switch_block(adt, normal_blocks, &values, succ, unwind), + unwind.map(|unwind| { + self.adt_switch_block( + adt, unwind_blocks.unwrap(), &values, unwind, Unwind::InCleanup + ) + })) } fn adt_switch_block(&mut self, @@ -652,8 +656,8 @@ impl<'l, 'b, 'tcx, D> DropCtxt<'l, 'b, 'tcx, D> }); // FIXME(#34708): handle partially-dropped array/slice elements. - self.drop_flag_test_and_reset_block( - Some(DropFlagMode::Deep), drop_block, succ, unwind) + let reset_block = self.drop_flag_reset_block(DropFlagMode::Deep, drop_block, unwind); + self.drop_flag_test_block(reset_block, succ, unwind) } /// The slow-path - create an "open", elaborated drop for a type @@ -707,23 +711,26 @@ impl<'l, 'b, 'tcx, D> DropCtxt<'l, 'b, 'tcx, D> debug!("complete_drop({:?},{:?})", self, drop_mode); let drop_block = self.drop_block(succ, unwind); - self.drop_flag_test_and_reset_block(drop_mode, drop_block, succ, unwind) + let drop_block = if let Some(mode) = drop_mode { + self.drop_flag_reset_block(mode, drop_block, unwind) + } else { + drop_block + }; + + self.drop_flag_test_block(drop_block, succ, unwind) } - fn drop_flag_test_and_reset_block(&mut self, - drop_mode: Option, - drop_block: BasicBlock, - succ: BasicBlock, - unwind: Unwind) -> BasicBlock + fn drop_flag_reset_block(&mut self, + mode: DropFlagMode, + succ: BasicBlock, + unwind: Unwind) -> BasicBlock { - debug!("drop_flag_test_and_reset_block({:?},{:?})", self, drop_mode); - - if let Some(mode) = drop_mode { - let block_start = Location { block: drop_block, statement_index: 0 }; - self.elaborator.clear_drop_flag(block_start, self.path, mode); - } + debug!("drop_flag_reset_block({:?},{:?})", self, mode); - self.drop_flag_test_block(drop_block, succ, unwind) + let block = self.new_block(unwind, TerminatorKind::Goto { target: succ }); + let block_start = Location { block: block, statement_index: 0 }; + self.elaborator.clear_drop_flag(block_start, self.path, mode); + block } fn elaborated_drop_block<'a>(&mut self) -> BasicBlock { diff --git a/src/test/mir-opt/issue-41888.rs b/src/test/mir-opt/issue-41888.rs new file mode 100644 index 0000000000000..ea4d7d3165d06 --- /dev/null +++ b/src/test/mir-opt/issue-41888.rs @@ -0,0 +1,186 @@ +// Copyright 2017 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +// check that we clear the "ADT master drop flag" even when there are +// no fields to be dropped. + +fn main() { + let e; + if cond() { + e = E::F(K); + if let E::F(_k) = e { + // older versions of rustc used to not clear the + // drop flag for `e` in this path. + } + } +} + +fn cond() -> bool { false } + +struct K; + +enum E { + F(K), + G(Box) +} + +// END RUST SOURCE +// fn main() -> () { +// let mut _0: (); +// scope 1 { +// let _1: E; // `e` +// scope 2 { +// let _6: K; +// } +// } +// let mut _2: bool; +// let mut _3: (); +// let mut _4: E; +// let mut _5: K; +// let mut _7: isize; +// let mut _8: bool; // drop flag for `e` +// let mut _9: bool; +// let mut _10: bool; +// let mut _11: isize; +// let mut _12: isize; +// +// bb0: { +// _8 = const false; +// _10 = const false; +// _9 = const false; +// StorageLive(_1); +// StorageLive(_2); +// _2 = const cond() -> [return: bb3, unwind: bb2]; +// } +// +// bb1: { +// resume; +// } +// +// bb2: { +// goto -> bb1; +// } +// +// bb3: { +// switchInt(_2) -> [0u8: bb5, otherwise: bb4]; +// } +// +// bb4: { +// StorageLive(_4); +// StorageLive(_5); +// _5 = K::{{constructor}}; +// _4 = E::F(_5,); +// StorageDead(_5); +// goto -> bb15; +// } +// +// bb5: { +// _0 = (); +// goto -> bb12; +// } +// +// bb6: { +// goto -> bb2; +// } +// +// bb7: { +// goto -> bb8; +// } +// +// bb8: { +// StorageDead(_4); +// _7 = discriminant(_1); +// switchInt(_7) -> [0isize: bb10, otherwise: bb9]; +// } +// +// bb9: { +// _0 = (); +// goto -> bb11; +// } +// +// bb10: { +// StorageLive(_6); +// _10 = const false; +// _6 = ((_1 as F).0: K); +// _0 = (); +// goto -> bb11; +// } +// +// bb11: { +// StorageDead(_6); +// goto -> bb12; +// } +// +// bb12: { +// StorageDead(_2); +// goto -> bb22; +// } +// +// bb13: { +// StorageDead(_1); +// return; +// } +// +// bb14: { +// _8 = const true; +// _9 = const true; +// _10 = const true; +// _1 = _4; +// goto -> bb6; +// } +// +// bb15: { +// _8 = const true; +// _9 = const true; +// _10 = const true; +// _1 = _4; +// goto -> bb7; +// } +// +// bb16: { +// _8 = const false; // clear the drop flag - must always be reached +// goto -> bb13; +// } +// +// bb17: { +// _8 = const false; +// goto -> bb1; +// } +// +// bb18: { +// goto -> bb17; +// } +// +// bb19: { +// drop(_1) -> [return: bb16, unwind: bb17]; +// } +// +// bb20: { +// drop(_1) -> bb17; +// } +// +// bb21: { +// _11 = discriminant(_1); +// switchInt(_11) -> [0isize: bb16, otherwise: bb19]; +// } +// +// bb22: { +// switchInt(_8) -> [0u8: bb16, otherwise: bb21]; +// } +// +// bb23: { +// _12 = discriminant(_1); +// switchInt(_12) -> [0isize: bb18, otherwise: bb20]; +// } +// +// bb24: { +// switchInt(_8) -> [0u8: bb17, otherwise: bb23]; +// } +// } diff --git a/src/test/run-pass/dynamic-drop.rs b/src/test/run-pass/dynamic-drop.rs index 26e5fe987eedd..6725a0c547f1d 100644 --- a/src/test/run-pass/dynamic-drop.rs +++ b/src/test/run-pass/dynamic-drop.rs @@ -90,6 +90,22 @@ fn dynamic_drop(a: &Allocator, c: bool) { }; } +struct TwoPtrs<'a>(Ptr<'a>, Ptr<'a>); +fn struct_dynamic_drop(a: &Allocator, c0: bool, c1: bool, c: bool) { + for i in 0..2 { + let x; + let y; + if (c0 && i == 0) || (c1 && i == 1) { + x = (a.alloc(), a.alloc(), a.alloc()); + y = TwoPtrs(a.alloc(), a.alloc()); + if c { + drop(x.1); + drop(y.0); + } + } + } +} + fn assignment2(a: &Allocator, c0: bool, c1: bool) { let mut _v = a.alloc(); let mut _w = a.alloc(); @@ -182,5 +198,14 @@ fn main() { run_test(|a| array_simple(a)); run_test(|a| vec_simple(a)); + run_test(|a| struct_dynamic_drop(a, false, false, false)); + run_test(|a| struct_dynamic_drop(a, false, false, true)); + run_test(|a| struct_dynamic_drop(a, false, true, false)); + run_test(|a| struct_dynamic_drop(a, false, true, true)); + run_test(|a| struct_dynamic_drop(a, true, false, false)); + run_test(|a| struct_dynamic_drop(a, true, false, true)); + run_test(|a| struct_dynamic_drop(a, true, true, false)); + run_test(|a| struct_dynamic_drop(a, true, true, true)); + run_test_nopanic(|a| union1(a)); }