Rollup merge of rust-lang#22818 - pnkfelix:fsk-issue-22536, r=nikomatsakis

 Ensure we do not zero when \"moving\" types that are Copy.

Uses more precise `type_needs_drop` for deciding about emitting cleanup code.  Added notes about the weaknesses regarding `ty::type_contents` here.

Fix rust-lang#22536

Author: Manishearth

src/libcore/intrinsics.rs

@@ -241,7 +241,12 @@ extern "rust-intrinsic" {
     /// will trigger a compiler error.
     pub fn return_address() -> *const u8;
 
-    /// Returns `true` if a type requires drop glue.
+    /// Returns `true` if the actual type given as `T` requires drop
+    /// glue; returns `false` if the actual type provided for `T`
+    /// implements `Copy`.
+    ///
+    /// If the actual type neither requires drop glue nor implements
+    /// `Copy`, then may return `true` or `false`.
     pub fn needs_drop<T>() -> bool;
 
     /// Returns `true` if a type is managed (will be allocated on the local heap)

src/librustc_trans/trans/_match.rs

@@ -1499,6 +1499,7 @@ pub fn store_local<'blk, 'tcx>(bcx: Block<'blk, 'tcx>,
     fn create_dummy_locals<'blk, 'tcx>(mut bcx: Block<'blk, 'tcx>,
                                        pat: &ast::Pat)
                                        -> Block<'blk, 'tcx> {
+        let _icx = push_ctxt("create_dummy_locals");
         // create dummy memory for the variables if we have no
         // value to store into them immediately
         let tcx = bcx.tcx();

src/librustc_trans/trans/callee.rs

@@ -734,7 +734,7 @@ pub fn trans_call_inner<'a, 'blk, 'tcx, F>(bcx: Block<'blk, 'tcx>,
             };
             if !is_rust_fn ||
               type_of::return_uses_outptr(ccx, ret_ty) ||
-              common::type_needs_drop(bcx.tcx(), ret_ty) {
+              bcx.fcx.type_needs_drop(ret_ty) {
                 // Push the out-pointer if we use an out-pointer for this
                 // return type, otherwise push "undef".
                 if common::type_is_zero_size(ccx, ret_ty) {

src/librustc_trans/trans/cleanup.rs

@@ -386,7 +386,7 @@ impl<'blk, 'tcx> CleanupMethods<'blk, 'tcx> for FunctionContext<'blk, 'tcx> {
                          cleanup_scope: ScopeId,
                          val: ValueRef,
                          ty: Ty<'tcx>) {
-        if !common::type_needs_drop(self.ccx.tcx(), ty) { return; }
+        if !self.type_needs_drop(ty) { return; }
         let drop = box DropValue {
             is_immediate: false,
             must_unwind: common::type_needs_unwind_cleanup(self.ccx, ty),
@@ -408,7 +408,8 @@ impl<'blk, 'tcx> CleanupMethods<'blk, 'tcx> for FunctionContext<'blk, 'tcx> {
                                   cleanup_scope: ScopeId,
                                   val: ValueRef,
                                   ty: Ty<'tcx>) {
-        if !common::type_needs_drop(self.ccx.tcx(), ty) { return; }
+        if !self.type_needs_drop(ty) { return; }
+
         let drop = box DropValue {
             is_immediate: false,
             must_unwind: common::type_needs_unwind_cleanup(self.ccx, ty),
@@ -432,7 +433,7 @@ impl<'blk, 'tcx> CleanupMethods<'blk, 'tcx> for FunctionContext<'blk, 'tcx> {
                                val: ValueRef,
                                ty: Ty<'tcx>) {
 
-        if !common::type_needs_drop(self.ccx.tcx(), ty) { return; }
+        if !self.type_needs_drop(ty) { return; }
         let drop = box DropValue {
             is_immediate: true,
             must_unwind: common::type_needs_unwind_cleanup(self.ccx, ty),
@@ -1007,6 +1008,7 @@ impl<'tcx> Cleanup<'tcx> for DropValue<'tcx> {
                    bcx: Block<'blk, 'tcx>,
                    debug_loc: DebugLoc)
                    -> Block<'blk, 'tcx> {
+        let _icx = base::push_ctxt("<DropValue as Cleanup>::trans");
         let bcx = if self.is_immediate {
             glue::drop_ty_immediate(bcx, self.val, self.ty, debug_loc)
         } else {

src/librustc_trans/trans/common.rs

@@ -213,8 +213,43 @@ pub fn type_needs_unwind_cleanup<'a, 'tcx>(ccx: &CrateContext<'a, 'tcx>, ty: Ty<
     }
 }
 
+/// If `type_needs_drop` returns true, then `ty` is definitely
+/// non-copy and *might* have a destructor attached; if it returns
+/// false, then `ty` definitely has no destructor (i.e. no drop glue).
+///
+/// (Note that this implies that if `ty` has a destructor attached,
+/// then `type_needs_drop` will definitely return `true` for `ty`.)
 pub fn type_needs_drop<'tcx>(cx: &ty::ctxt<'tcx>, ty: Ty<'tcx>) -> bool {
-    ty::type_contents(cx, ty).needs_drop(cx)
+    type_needs_drop_given_env(cx, ty, &ty::empty_parameter_environment(cx))
+}
+
+/// Core implementation of type_needs_drop, potentially making use of
+/// and/or updating caches held in the `param_env`.
+fn type_needs_drop_given_env<'a,'tcx>(cx: &ty::ctxt<'tcx>,
+                                      ty: Ty<'tcx>,
+                                      param_env: &ty::ParameterEnvironment<'a,'tcx>) -> bool {
+    // Issue #22536: We first query type_moves_by_default.  It sees a
+    // normalized version of the type, and therefore will definitely
+    // know whether the type implements Copy (and thus needs no
+    // cleanup/drop/zeroing) ...
+    let implements_copy = !ty::type_moves_by_default(&param_env, DUMMY_SP, ty);
+
+    if implements_copy { return false; }
+
+    // ... (issue #22536 continued) but as an optimization, still use
+    // prior logic of asking if the `needs_drop` bit is set; we need
+    // not zero non-Copy types if they have no destructor.
+
+    // FIXME(#22815): Note that calling `ty::type_contents` is a
+    // conservative heuristic; it may report that `needs_drop` is set
+    // when actual type does not actually have a destructor associated
+    // with it. But since `ty` absolutely did not have the `Copy`
+    // bound attached (see above), it is sound to treat it as having a
+    // destructor (e.g. zero its memory on move).
+
+    let contents = ty::type_contents(cx, ty);
+    debug!("type_needs_drop ty={} contents={:?}", ty.repr(cx), contents);
+    contents.needs_drop(cx)
 }
 
 fn type_is_newtype_immediate<'a, 'tcx>(ccx: &CrateContext<'a, 'tcx>, ty: Ty<'tcx>) -> bool {
@@ -534,6 +569,12 @@ impl<'a, 'tcx> FunctionContext<'a, 'tcx> {
                                          self.param_substs,
                                          value)
     }
+
+    /// This is the same as `common::type_needs_drop`, except that it
+    /// may use or update caches within this `FunctionContext`.
+    pub fn type_needs_drop(&self, ty: Ty<'tcx>) -> bool {
+        type_needs_drop_given_env(self.ccx.tcx(), ty, &self.param_env)
+    }
 }
 
 // Basic block context.  We create a block context for each basic block

src/librustc_trans/trans/controlflow.rs

@@ -77,7 +77,7 @@ pub fn trans_stmt_semi<'blk, 'tcx>(cx: Block<'blk, 'tcx>, e: &ast::Expr)
                                    -> Block<'blk, 'tcx> {
     let _icx = push_ctxt("trans_stmt_semi");
     let ty = expr_ty(cx, e);
-    if type_needs_drop(cx.tcx(), ty) {
+    if cx.fcx.type_needs_drop(ty) {
         expr::trans_to_lvalue(cx, e, "stmt").bcx
     } else {
         expr::trans_into(cx, e, expr::Ignore)

src/librustc_trans/trans/datum.rs

@@ -311,7 +311,8 @@ impl KindOps for Lvalue {
                               val: ValueRef,
                               ty: Ty<'tcx>)
                               -> Block<'blk, 'tcx> {
-        if type_needs_drop(bcx.tcx(), ty) {
+        let _icx = push_ctxt("<Lvalue as KindOps>::post_store");
+        if bcx.fcx.type_needs_drop(ty) {
             // cancel cleanup of affine values by zeroing out
             let () = zero_mem(bcx, val, ty);
             bcx
@@ -656,7 +657,7 @@ impl<'tcx, K: KindOps + fmt::Debug> Datum<'tcx, K> {
     /// scalar-ish (like an int or a pointer) which (1) does not require drop glue and (2) is
     /// naturally passed around by value, and not by reference.
     pub fn to_llscalarish<'blk>(self, bcx: Block<'blk, 'tcx>) -> ValueRef {
-        assert!(!type_needs_drop(bcx.tcx(), self.ty));
+        assert!(!bcx.fcx.type_needs_drop(self.ty));
         assert!(self.appropriate_rvalue_mode(bcx.ccx()) == ByValue);
         if self.kind.is_by_ref() {
             load_ty(bcx, self.val, self.ty)

src/librustc_trans/trans/expr.rs

@@ -974,7 +974,7 @@ fn trans_rvalue_stmt_unadjusted<'blk, 'tcx>(bcx: Block<'blk, 'tcx>,
             let src_datum = unpack_datum!(bcx, trans(bcx, &**src));
             let dst_datum = unpack_datum!(bcx, trans_to_lvalue(bcx, &**dst, "assign"));
 
-            if type_needs_drop(bcx.tcx(), dst_datum.ty) {
+            if bcx.fcx.type_needs_drop(dst_datum.ty) {
                 // If there are destructors involved, make sure we
                 // are copying from an rvalue, since that cannot possible
                 // alias an lvalue. We are concerned about code like:
@@ -1498,7 +1498,7 @@ pub fn trans_adt<'a, 'blk, 'tcx>(mut bcx: Block<'blk, 'tcx>,
         assert_eq!(discr, 0);
 
         match ty::expr_kind(bcx.tcx(), &*base.expr) {
-            ty::RvalueDpsExpr | ty::RvalueDatumExpr if !type_needs_drop(bcx.tcx(), ty) => {
+            ty::RvalueDpsExpr | ty::RvalueDatumExpr if !bcx.fcx.type_needs_drop(ty) => {
                 bcx = trans_into(bcx, &*base.expr, SaveIn(addr));
             },
             ty::RvalueStmtExpr => bcx.tcx().sess.bug("unexpected expr kind for struct base expr"),
@@ -2116,7 +2116,7 @@ fn trans_assign_op<'blk, 'tcx>(bcx: Block<'blk, 'tcx>,
 
     // Evaluate LHS (destination), which should be an lvalue
     let dst_datum = unpack_datum!(bcx, trans_to_lvalue(bcx, dst, "assign_op"));
-    assert!(!type_needs_drop(bcx.tcx(), dst_datum.ty));
+    assert!(!bcx.fcx.type_needs_drop(dst_datum.ty));
     let dst_ty = dst_datum.ty;
     let dst = load_ty(bcx, dst_datum.val, dst_datum.ty);
 

src/librustc_trans/trans/glue.rs

@@ -99,6 +99,16 @@ pub fn get_drop_glue_type<'a, 'tcx>(ccx: &CrateContext<'a, 'tcx>,
     if !type_is_sized(tcx, t) {
         return t
     }
+
+    // FIXME (#22815): note that type_needs_drop conservatively
+    // approximates in some cases and may say a type expression
+    // requires drop glue when it actually does not.
+    //
+    // (In this case it is not clear whether any harm is done, i.e.
+    // erroneously returning `t` in some cases where we could have
+    // returned `tcx.types.i8` does not appear unsound. The impact on
+    // code quality is unknown at this time.)
+
     if !type_needs_drop(tcx, t) {
         return tcx.types.i8;
     }
@@ -125,7 +135,7 @@ pub fn drop_ty<'blk, 'tcx>(bcx: Block<'blk, 'tcx>,
     // NB: v is an *alias* of type t here, not a direct value.
     debug!("drop_ty(t={})", t.repr(bcx.tcx()));
     let _icx = push_ctxt("drop_ty");
-    if type_needs_drop(bcx.tcx(), t) {
+    if bcx.fcx.type_needs_drop(t) {
         let ccx = bcx.ccx();
         let glue = get_drop_glue(ccx, t);
         let glue_type = get_drop_glue_type(ccx, t);
@@ -480,7 +490,7 @@ fn make_drop_glue<'blk, 'tcx>(bcx: Block<'blk, 'tcx>, v0: ValueRef, t: Ty<'tcx>)
         },
         _ => {
             assert!(type_is_sized(bcx.tcx(), t));
-            if type_needs_drop(bcx.tcx(), t) && ty::type_is_structural(t) {
+            if bcx.fcx.type_needs_drop(t) && ty::type_is_structural(t) {
                 iter_structural_ty(bcx,
                                    v0,
                                    t,

src/librustc_trans/trans/intrinsic.rs

@@ -156,6 +156,8 @@ pub fn trans_intrinsic_call<'a, 'blk, 'tcx>(mut bcx: Block<'blk, 'tcx>,
     let ccx = fcx.ccx;
     let tcx = bcx.tcx();
 
+    let _icx = push_ctxt("trans_intrinsic_call");
+
     let ret_ty = match callee_ty.sty {
         ty::ty_bare_fn(_, ref f) => {
             ty::erase_late_bound_regions(bcx.tcx(), &f.sig.output())
@@ -376,7 +378,8 @@ pub fn trans_intrinsic_call<'a, 'blk, 'tcx>(mut bcx: Block<'blk, 'tcx>,
         }
         (_, "needs_drop") => {
             let tp_ty = *substs.types.get(FnSpace, 0);
-            C_bool(ccx, type_needs_drop(ccx.tcx(), tp_ty))
+
+            C_bool(ccx, bcx.fcx.type_needs_drop(tp_ty))
         }
         (_, "owns_managed") => {
             let tp_ty = *substs.types.get(FnSpace, 0);

src/librustc_trans/trans/meth.rs

@@ -454,7 +454,7 @@ fn trans_trait_callee<'blk, 'tcx>(bcx: Block<'blk, 'tcx>,
     let self_datum = unpack_datum!(
         bcx, expr::trans(bcx, self_expr));
 
-    let llval = if type_needs_drop(bcx.tcx(), self_datum.ty) {
+    let llval = if bcx.fcx.type_needs_drop(self_datum.ty) {
         let self_datum = unpack_datum!(
             bcx, self_datum.to_rvalue_datum(bcx, "trait_callee"));
 

src/librustc_trans/trans/tvec.rs

@@ -53,11 +53,10 @@ pub fn make_drop_glue_unboxed<'blk, 'tcx>(bcx: Block<'blk, 'tcx>,
     let not_null = IsNotNull(bcx, vptr);
     with_cond(bcx, not_null, |bcx| {
         let ccx = bcx.ccx();
-        let tcx = bcx.tcx();
         let _icx = push_ctxt("tvec::make_drop_glue_unboxed");
 
         let dataptr = get_dataptr(bcx, vptr);
-        let bcx = if type_needs_drop(tcx, unit_ty) {
+        let bcx = if bcx.fcx.type_needs_drop(unit_ty) {
             let len = get_len(bcx, vptr);
             iter_vec_raw(bcx,
                          dataptr,

src/test/run-pass/issue-22536-copy-mustnt-zero.rs

@@ -0,0 +1,34 @@
+// Copyright 2015 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.
+
+// Regression test for Issue #22536: If a type implements Copy, then
+// moving it must not zero the original memory.
+
+trait Resources {
+    type Buffer: Copy;
+    fn foo(&self) {}
+}
+
+struct BufferHandle<R: Resources> {
+    raw: <R as Resources>::Buffer,
+}
+impl<R: Resources> Copy for BufferHandle<R> {}
+
+enum Res {}
+impl Resources for Res {
+    type Buffer = u32;
+}
+impl Copy for Res { }
+
+fn main() {
+    let b: BufferHandle<Res> = BufferHandle { raw: 1 };
+    let c = b;
+    assert_eq!(c.raw, b.raw)
+}