fix codegen of drops of fields of packed structs

3 files changed, 151 insertions, 25 deletions

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 <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
+    });
+}
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::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)
     {
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::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.