fix codegen of drops of fields of packed structs

src/librustc_mir/shim.rs

@@ -27,7 +27,8 @@ use syntax_pos::Span;
 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);

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 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, 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<u8>
+//     }
+//
+//     let foo = ...;
+// ```
+//
+// We want to call `drop_in_place::<Vec<u8>>` 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
+    });
+}

src/librustc_mir/transform/check_unsafety.rs

@@ -12,7 +12,7 @@ use rustc_data_structures::fx::FxHashSet;
 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 rustc::mir::visit::{LvalueContext, Visitor};
 use syntax::ast;
 
 use std::rc::Rc;
-
+use util;
 
 pub struct UnsafetyChecker<'a, 'tcx: 'a> {
     mir: &'a Mir<'tcx>,
     visibility_scope_info: &'a IndexVec<VisibilityScope, VisibilityScopeInfo>,
     violations: Vec<UnsafetyViolation>,
     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<ast::NodeId>,
@@ -53,7 +50,6 @@ impl<'a, 'gcx, 'tcx> UnsafetyChecker<'a, 'tcx> {
             },
             tcx,
             param_env,
-            need_check_packed: false,
             used_unsafe: FxHashSet(),
             inherited_blocks: vec![],
         }
@@ -142,11 +138,9 @@ impl<'a, 'tcx> Visitor<'tcx> for UnsafetyChecker<'a, 'tcx> {
                     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 @@ impl<'a, 'tcx> Visitor<'tcx> for UnsafetyChecker<'a, 'tcx> {
                         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 @@ impl<'a, 'tcx> Visitor<'tcx> for UnsafetyChecker<'a, 'tcx> {
                                 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 @@ impl<'a, 'tcx> Visitor<'tcx> for UnsafetyChecker<'a, 'tcx> {
             }
         };
         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)
     {

src/librustc_mir/transform/mod.rs

@@ -23,6 +23,7 @@ use syntax::ast;
 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.

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 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, 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
+}

src/librustc_mir/util/mod.rs

@@ -13,10 +13,12 @@ pub mod elaborate_drops;
 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;

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 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, 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

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 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, 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<usize>
+}
+
+#[inline(never)]
+fn check_align(ptr: *const Aligned) {
+    assert_eq!(ptr as usize % mem::align_of::<Aligned>(),
+               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);
+}