From 06eb5a6645ff9faf8fbe553e73efaae00c70090a Mon Sep 17 00:00:00 2001 From: Ariel Ben-Yehuda Date: Tue, 3 Oct 2017 16:01:01 +0200 Subject: [PATCH] fix codegen of drops of fields of packed structs --- src/librustc_mir/shim.rs | 5 +- .../transform/add_moves_for_packed_drops.rs | 141 ++++++++++++++++++ src/librustc_mir/transform/check_unsafety.rs | 29 +--- src/librustc_mir/transform/mod.rs | 6 + src/librustc_mir/util/alignment.rs | 74 +++++++++ src/librustc_mir/util/mod.rs | 2 + .../mir-opt/packed-struct-drop-aligned.rs | 68 +++++++++ .../run-pass/packed-struct-drop-aligned.rs | 42 ++++++ 8 files changed, 341 insertions(+), 26 deletions(-) create mode 100644 src/librustc_mir/transform/add_moves_for_packed_drops.rs create mode 100644 src/librustc_mir/util/alignment.rs create mode 100644 src/test/mir-opt/packed-struct-drop-aligned.rs create mode 100644 src/test/run-pass/packed-struct-drop-aligned.rs diff --git a/src/librustc_mir/shim.rs b/src/librustc_mir/shim.rs index 15c68954230..0fa47d80999 100644 --- a/src/librustc_mir/shim.rs +++ b/src/librustc_mir/shim.rs @@ -27,7 +27,8 @@ use std::fmt; use std::iter; -use transform::{add_call_guards, no_landing_pads, simplify}; +use transform::{add_moves_for_packed_drops, add_call_guards}; +use transform::{no_landing_pads, simplify}; use util::elaborate_drops::{self, DropElaborator, DropStyle, DropFlagMode}; use util::patch::MirPatch; @@ -114,6 +115,8 @@ fn make_shim<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, } }; debug!("make_shim({:?}) = untransformed {:?}", instance, result); + add_moves_for_packed_drops::add_moves_for_packed_drops( + tcx, &mut result, instance.def_id()); no_landing_pads::no_landing_pads(tcx, &mut result); simplify::simplify_cfg(&mut result); add_call_guards::CriticalCallEdges.add_call_guards(&mut result); diff --git a/src/librustc_mir/transform/add_moves_for_packed_drops.rs b/src/librustc_mir/transform/add_moves_for_packed_drops.rs new file mode 100644 index 00000000000..297bc76d472 --- /dev/null +++ b/src/librustc_mir/transform/add_moves_for_packed_drops.rs @@ -0,0 +1,141 @@ +// 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. + +use rustc::hir::def_id::DefId; +use rustc::mir::*; +use rustc::ty::TyCtxt; + +use transform::{MirPass, MirSource}; +use util::patch::MirPatch; +use util; + +// This pass moves values being dropped that are within a packed +// struct to a separate local before dropping them, to ensure that +// they are dropped from an aligned address. +// +// For example, if we have something like +// ```Rust +// #[repr(packed)] +// struct Foo { +// dealign: u8, +// data: Vec +// } +// +// let foo = ...; +// ``` +// +// We want to call `drop_in_place::>` on `data` from an aligned +// address. This means we can't simply drop `foo.data` directly, because +// its address is not aligned. +// +// Instead, we move `foo.data` to a local and drop that: +// ``` +// storage.live(drop_temp) +// drop_temp = foo.data; +// drop(drop_temp) -> next +// next: +// storage.dead(drop_temp) +// ``` +// +// The storage instructions are required to avoid stack space +// blowup. + +pub struct AddMovesForPackedDrops; + +impl MirPass for AddMovesForPackedDrops { + fn run_pass<'a, 'tcx>(&self, + tcx: TyCtxt<'a, 'tcx, 'tcx>, + src: MirSource, + mir: &mut Mir<'tcx>) + { + debug!("add_moves_for_packed_drops({:?} @ {:?})", src, mir.span); + add_moves_for_packed_drops(tcx, mir, src.def_id); + } +} + +pub fn add_moves_for_packed_drops<'a, 'tcx>( + tcx: TyCtxt<'a, 'tcx, 'tcx>, + mir: &mut Mir<'tcx>, + def_id: DefId) +{ + let patch = add_moves_for_packed_drops_patch(tcx, mir, def_id); + patch.apply(mir); +} + +fn add_moves_for_packed_drops_patch<'a, 'tcx>( + tcx: TyCtxt<'a, 'tcx, 'tcx>, + mir: &Mir<'tcx>, + def_id: DefId) + -> MirPatch<'tcx> +{ + let mut patch = MirPatch::new(mir); + let param_env = tcx.param_env(def_id); + + for (bb, data) in mir.basic_blocks().iter_enumerated() { + let loc = Location { block: bb, statement_index: data.statements.len() }; + let terminator = data.terminator(); + + match terminator.kind { + TerminatorKind::Drop { ref location, .. } + if util::is_disaligned(tcx, mir, param_env, location) => + { + add_move_for_packed_drop(tcx, mir, &mut patch, terminator, + loc, data.is_cleanup); + } + TerminatorKind::DropAndReplace { .. } => { + span_bug!(terminator.source_info.span, + "replace in AddMovesForPackedDrops"); + } + _ => {} + } + } + + patch +} + +fn add_move_for_packed_drop<'a, 'tcx>( + tcx: TyCtxt<'a, 'tcx, 'tcx>, + mir: &Mir<'tcx>, + patch: &mut MirPatch<'tcx>, + terminator: &Terminator<'tcx>, + loc: Location, + is_cleanup: bool) +{ + debug!("add_move_for_packed_drop({:?} @ {:?})", terminator, loc); + let (location, target, unwind) = match terminator.kind { + TerminatorKind::Drop { ref location, target, unwind } => + (location, target, unwind), + _ => unreachable!() + }; + + let source_info = terminator.source_info; + let ty = location.ty(mir, tcx).to_ty(tcx); + let temp = patch.new_temp(ty, terminator.source_info.span); + + let storage_dead_block = patch.new_block(BasicBlockData { + statements: vec![Statement { + source_info, kind: StatementKind::StorageDead(temp) + }], + terminator: Some(Terminator { + source_info, kind: TerminatorKind::Goto { target } + }), + is_cleanup + }); + + patch.add_statement( + loc, StatementKind::StorageLive(temp)); + patch.add_assign(loc, Lvalue::Local(temp), + Rvalue::Use(Operand::Consume(location.clone()))); + patch.patch_terminator(loc.block, TerminatorKind::Drop { + location: Lvalue::Local(temp), + target: storage_dead_block, + unwind + }); +} diff --git a/src/librustc_mir/transform/check_unsafety.rs b/src/librustc_mir/transform/check_unsafety.rs index 8620fba973d..9cadc04dc0a 100644 --- a/src/librustc_mir/transform/check_unsafety.rs +++ b/src/librustc_mir/transform/check_unsafety.rs @@ -12,7 +12,7 @@ use rustc_data_structures::indexed_vec::IndexVec; use rustc::ty::maps::Providers; -use rustc::ty::{self, Ty, TyCtxt}; +use rustc::ty::{self, TyCtxt}; use rustc::hir; use rustc::hir::def_id::DefId; use rustc::lint::builtin::{SAFE_EXTERN_STATICS, UNUSED_UNSAFE}; @@ -22,16 +22,13 @@ use syntax::ast; use std::rc::Rc; - +use util; pub struct UnsafetyChecker<'a, 'tcx: 'a> { mir: &'a Mir<'tcx>, visibility_scope_info: &'a IndexVec, violations: Vec, source_info: SourceInfo, - // true if an a part of this *memory block* of this expression - // is being borrowed, used for repr(packed) checking. - need_check_packed: bool, tcx: TyCtxt<'a, 'tcx, 'tcx>, param_env: ty::ParamEnv<'tcx>, used_unsafe: FxHashSet, @@ -53,7 +50,6 @@ fn new(mir: &'a Mir<'tcx>, }, tcx, param_env, - need_check_packed: false, used_unsafe: FxHashSet(), inherited_blocks: vec![], } @@ -142,11 +138,9 @@ fn visit_lvalue(&mut self, lvalue: &Lvalue<'tcx>, context: LvalueContext<'tcx>, location: Location) { - let old_need_check_packed = self.need_check_packed; if let LvalueContext::Borrow { .. } = context { - let ty = lvalue.ty(self.mir, self.tcx).to_ty(self.tcx); - if !self.has_align_1(ty) { - self.need_check_packed = true; + if util::is_disaligned(self.tcx, self.mir, self.param_env, lvalue) { + self.require_unsafe("borrow of packed field") } } @@ -163,9 +157,6 @@ fn visit_lvalue(&mut self, self.source_info = self.mir.local_decls[local].source_info; } } - if let &ProjectionElem::Deref = elem { - self.need_check_packed = false; - } let base_ty = base.ty(self.mir, self.tcx).to_ty(self.tcx); match base_ty.sty { ty::TyRawPtr(..) => { @@ -194,9 +185,6 @@ fn visit_lvalue(&mut self, self.require_unsafe("access to union field") } } - if adt.repr.packed() && self.need_check_packed { - self.require_unsafe("borrow of packed field") - } } _ => {} } @@ -221,19 +209,10 @@ fn visit_lvalue(&mut self, } }; self.super_lvalue(lvalue, context, location); - self.need_check_packed = old_need_check_packed; } } impl<'a, 'tcx> UnsafetyChecker<'a, 'tcx> { - - fn has_align_1(&self, ty: Ty<'tcx>) -> bool { - self.tcx.at(self.source_info.span) - .layout_raw(self.param_env.and(ty)) - .map(|layout| layout.align.abi() == 1) - .unwrap_or(false) - } - fn require_unsafe(&mut self, description: &'static str) { diff --git a/src/librustc_mir/transform/mod.rs b/src/librustc_mir/transform/mod.rs index 6987cfa79be..418d3d22058 100644 --- a/src/librustc_mir/transform/mod.rs +++ b/src/librustc_mir/transform/mod.rs @@ -23,6 +23,7 @@ use syntax_pos::Span; pub mod add_validation; +pub mod add_moves_for_packed_drops; pub mod clean_end_regions; pub mod check_unsafety; pub mod simplify_branches; @@ -236,7 +237,12 @@ fn optimized_mir<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, def_id: DefId) -> &'tcx // an AllCallEdges pass right before it. add_call_guards::AllCallEdges, add_validation::AddValidation, + // AddMovesForPackedDrops needs to run after drop + // elaboration. + add_moves_for_packed_drops::AddMovesForPackedDrops, + simplify::SimplifyCfg::new("elaborate-drops"), + // No lifetime analysis based on borrowing can be done from here on out. // From here on out, regions are gone. diff --git a/src/librustc_mir/util/alignment.rs b/src/librustc_mir/util/alignment.rs new file mode 100644 index 00000000000..afda6e4031c --- /dev/null +++ b/src/librustc_mir/util/alignment.rs @@ -0,0 +1,74 @@ +// 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. + + +use rustc::ty::{self, TyCtxt}; +use rustc::mir::*; + +/// Return `true` if this lvalue is allowed to be less aligned +/// than its containing struct (because it is within a packed +/// struct). +pub fn is_disaligned<'a, 'tcx, L>(tcx: TyCtxt<'a, 'tcx, 'tcx>, + local_decls: &L, + param_env: ty::ParamEnv<'tcx>, + lvalue: &Lvalue<'tcx>) + -> bool + where L: HasLocalDecls<'tcx> +{ + debug!("is_disaligned({:?})", lvalue); + if !is_within_packed(tcx, local_decls, lvalue) { + debug!("is_disaligned({:?}) - not within packed", lvalue); + return false + } + + let ty = lvalue.ty(local_decls, tcx).to_ty(tcx); + match tcx.layout_raw(param_env.and(ty)) { + Ok(layout) if layout.align.abi() == 1 => { + // if the alignment is 1, the type can't be further + // disaligned. + debug!("is_disaligned({:?}) - align = 1", lvalue); + false + } + _ => { + debug!("is_disaligned({:?}) - true", lvalue); + true + } + } +} + +fn is_within_packed<'a, 'tcx, L>(tcx: TyCtxt<'a, 'tcx, 'tcx>, + local_decls: &L, + lvalue: &Lvalue<'tcx>) + -> bool + where L: HasLocalDecls<'tcx> +{ + let mut lvalue = lvalue; + while let &Lvalue::Projection(box Projection { + ref base, ref elem + }) = lvalue { + match *elem { + // encountered a Deref, which is ABI-aligned + ProjectionElem::Deref => break, + ProjectionElem::Field(..) => { + let ty = base.ty(local_decls, tcx).to_ty(tcx); + match ty.sty { + ty::TyAdt(def, _) if def.repr.packed() => { + return true + } + _ => {} + } + } + _ => {} + } + lvalue = base; + } + + false +} diff --git a/src/librustc_mir/util/mod.rs b/src/librustc_mir/util/mod.rs index 13c14f8920f..feea0e28809 100644 --- a/src/librustc_mir/util/mod.rs +++ b/src/librustc_mir/util/mod.rs @@ -13,10 +13,12 @@ pub mod def_use; pub mod patch; +mod alignment; mod graphviz; mod pretty; pub mod liveness; +pub use self::alignment::is_disaligned; pub use self::pretty::{dump_enabled, dump_mir, write_mir_pretty, PassWhere}; pub use self::graphviz::{write_mir_graphviz}; pub use self::graphviz::write_node_label as write_graphviz_node_label; diff --git a/src/test/mir-opt/packed-struct-drop-aligned.rs b/src/test/mir-opt/packed-struct-drop-aligned.rs new file mode 100644 index 00000000000..07a943976c3 --- /dev/null +++ b/src/test/mir-opt/packed-struct-drop-aligned.rs @@ -0,0 +1,68 @@ +// 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. + +fn main() { + let mut x = Packed(Aligned(Droppy(0))); + x.0 = Aligned(Droppy(0)); +} + +struct Aligned(Droppy); +#[repr(packed)] +struct Packed(Aligned); + +struct Droppy(usize); +impl Drop for Droppy { + fn drop(&mut self) {} +} + +// END RUST SOURCE +// START rustc.main.EraseRegions.before.mir +// fn main() -> () { +// let mut _0: (); +// scope 1 { +// let mut _1: Packed; +// } +// scope 2 { +// } +// let mut _2: Aligned; +// let mut _3: Droppy; +// let mut _4: Aligned; +// let mut _5: Droppy; +// let mut _6: Aligned; +// +// bb0: { +// StorageLive(_1); +// ... +// _1 = Packed::{{constructor}}(_2,); +// ... +// StorageLive(_6); +// _6 = (_1.0: Aligned); +// drop(_6) -> [return: bb4, unwind: bb3]; +// } +// bb1: { +// resume; +// } +// bb2: { +// StorageDead(_1); +// return; +// } +// bb3: { +// (_1.0: Aligned) = _4; +// drop(_1) -> bb1; +// } +// bb4: { +// StorageDead(_6); +// (_1.0: Aligned) = _4; +// StorageDead(_4); +// _0 = (); +// drop(_1) -> bb2; +// } +// } +// END rustc.main.EraseRegions.before.mir diff --git a/src/test/run-pass/packed-struct-drop-aligned.rs b/src/test/run-pass/packed-struct-drop-aligned.rs new file mode 100644 index 00000000000..bbe31a65e86 --- /dev/null +++ b/src/test/run-pass/packed-struct-drop-aligned.rs @@ -0,0 +1,42 @@ +// 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. + +use std::cell::Cell; +use std::mem; + +struct Aligned<'a> { + drop_count: &'a Cell +} + +#[inline(never)] +fn check_align(ptr: *const Aligned) { + assert_eq!(ptr as usize % mem::align_of::(), + 0); +} + +impl<'a> Drop for Aligned<'a> { + fn drop(&mut self) { + check_align(self); + self.drop_count.set(self.drop_count.get() + 1); + } +} + +#[repr(packed)] +struct Packed<'a>(u8, Aligned<'a>); + +fn main() { + let drop_count = &Cell::new(0); + { + let mut p = Packed(0, Aligned { drop_count }); + p.1 = Aligned { drop_count }; + assert_eq!(drop_count.get(), 1); + } + assert_eq!(drop_count.get(), 2); +} -- 2.44.0