From 965b0bfefe27b0a487b7243cb1a0a6f36a2be70b Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Fri, 8 Jan 2016 20:40:52 +0100 Subject: [PATCH] Issue 30530: initialize allocas for `Datum::to_lvalue_datum_in_scope`. In particular, bring back the `zero` flag for `lvalue_scratch_datum`, which controls whether the alloca's created immediately at function start are uninitialized at that point or have their embedded drop-flags initialized to "dropped". Then made `to_lvalue_datum_in_scope` pass "dropped" as `zero` flag. --- src/librustc_trans/trans/adt.rs | 2 ++ src/librustc_trans/trans/base.rs | 59 +++++++++++++++++++++++++++++-- src/librustc_trans/trans/datum.rs | 15 ++++++-- 3 files changed, 70 insertions(+), 6 deletions(-) diff --git a/src/librustc_trans/trans/adt.rs b/src/librustc_trans/trans/adt.rs index c744ef321278..e152e11a6a65 100644 --- a/src/librustc_trans/trans/adt.rs +++ b/src/librustc_trans/trans/adt.rs @@ -55,6 +55,7 @@ use syntax::ast; use syntax::attr; use syntax::attr::IntType; use trans::_match; +use trans::base::InitAlloca; use trans::build::*; use trans::cleanup; use trans::cleanup::CleanupMethods; @@ -1279,6 +1280,7 @@ pub fn trans_drop_flag_ptr<'blk, 'tcx>(mut bcx: Block<'blk, 'tcx>, let custom_cleanup_scope = fcx.push_custom_cleanup_scope(); let scratch = unpack_datum!(bcx, datum::lvalue_scratch_datum( bcx, tcx.dtor_type(), "drop_flag", + InitAlloca::Uninit("drop flag itself has no dtor"), cleanup::CustomScope(custom_cleanup_scope), (), |_, bcx, _| bcx )); bcx = fold_variants(bcx, r, val, |variant_cx, st, value| { diff --git a/src/librustc_trans/trans/base.rs b/src/librustc_trans/trans/base.rs index d694374acc92..b2bc7c2af933 100644 --- a/src/librustc_trans/trans/base.rs +++ b/src/librustc_trans/trans/base.rs @@ -1285,12 +1285,62 @@ fn memfill<'a, 'tcx>(b: &Builder<'a, 'tcx>, llptr: ValueRef, ty: Ty<'tcx>, byte: None); } -pub fn alloc_ty<'blk, 'tcx>(bcx: Block<'blk, 'tcx>, t: Ty<'tcx>, name: &str) -> ValueRef { +/// In general, when we create an scratch value in an alloca, the +/// creator may not know if the block (that initializes the scratch +/// with the desired value) actually dominates the cleanup associated +/// with the scratch value. +/// +/// To deal with this, when we do an alloca (at the *start* of whole +/// function body), we optionally can also set the associated +/// dropped-flag state of the alloca to "dropped." +#[derive(Copy, Clone, Debug)] +pub enum InitAlloca { + /// Indicates that the state should have its associated drop flag + /// set to "dropped" at the point of allocation. + Dropped, + /// Indicates the value of the associated drop flag is irrelevant. + /// The embedded string literal is a programmer provided argument + /// for why. This is a safeguard forcing compiler devs to + /// document; it might be a good idea to also emit this as a + /// comment with the alloca itself when emitting LLVM output.ll. + Uninit(&'static str), +} + + +pub fn alloc_ty<'blk, 'tcx>(bcx: Block<'blk, 'tcx>, + t: Ty<'tcx>, + name: &str) -> ValueRef { + // pnkfelix: I do not know why alloc_ty meets the assumptions for + // passing Uninit, but it was never needed (even back when we had + // the original boolean `zero` flag on `lvalue_scratch_datum`). + alloc_ty_init(bcx, t, InitAlloca::Uninit("all alloc_ty are uninit"), name) +} + +pub fn alloc_ty_init<'blk, 'tcx>(bcx: Block<'blk, 'tcx>, + t: Ty<'tcx>, + init: InitAlloca, + name: &str) -> ValueRef { let _icx = push_ctxt("alloc_ty"); let ccx = bcx.ccx(); let ty = type_of::type_of(ccx, t); assert!(!t.has_param_types()); - alloca(bcx, ty, name) + match init { + InitAlloca::Dropped => alloca_dropped(bcx, t, name), + InitAlloca::Uninit(_) => alloca(bcx, ty, name), + } +} + +pub fn alloca_dropped<'blk, 'tcx>(cx: Block<'blk, 'tcx>, ty: Ty<'tcx>, name: &str) -> ValueRef { + let _icx = push_ctxt("alloca_dropped"); + let llty = type_of::type_of(cx.ccx(), ty); + if cx.unreachable.get() { + unsafe { return llvm::LLVMGetUndef(llty.ptr_to().to_ref()); } + } + let p = alloca(cx, llty, name); + let b = cx.fcx.ccx.builder(); + b.position_before(cx.fcx.alloca_insert_pt.get().unwrap()); + memfill(&b, p, ty, adt::DTOR_DONE); + p } pub fn alloca(cx: Block, ty: Type, name: &str) -> ValueRef { @@ -1650,6 +1700,7 @@ pub fn create_datums_for_fn_args<'a, 'tcx>(mut bcx: Block<'a, 'tcx>, // This alloca should be optimized away by LLVM's mem-to-reg pass in // the event it's not truly needed. let mut idx = fcx.arg_offset() as c_uint; + let uninit_reason = InitAlloca::Uninit("fn_arg populate dominates dtor"); for (i, &arg_ty) in arg_tys.iter().enumerate() { let arg_datum = if !has_tupled_arg || i < arg_tys.len() - 1 { if type_of::arg_is_indirect(bcx.ccx(), arg_ty) && @@ -1669,7 +1720,7 @@ pub fn create_datums_for_fn_args<'a, 'tcx>(mut bcx: Block<'a, 'tcx>, let data = get_param(fcx.llfn, idx); let extra = get_param(fcx.llfn, idx + 1); idx += 2; - unpack_datum!(bcx, datum::lvalue_scratch_datum(bcx, arg_ty, "", + unpack_datum!(bcx, datum::lvalue_scratch_datum(bcx, arg_ty, "", uninit_reason, arg_scope_id, (data, extra), |(data, extra), bcx, dst| { Store(bcx, data, expr::get_dataptr(bcx, dst)); @@ -1684,6 +1735,7 @@ pub fn create_datums_for_fn_args<'a, 'tcx>(mut bcx: Block<'a, 'tcx>, datum::lvalue_scratch_datum(bcx, arg_ty, "", + uninit_reason, arg_scope_id, tmp, |tmp, bcx, dst| tmp.store_to(bcx, dst))) @@ -1696,6 +1748,7 @@ pub fn create_datums_for_fn_args<'a, 'tcx>(mut bcx: Block<'a, 'tcx>, datum::lvalue_scratch_datum(bcx, arg_ty, "tupled_args", + uninit_reason, arg_scope_id, (), |(), diff --git a/src/librustc_trans/trans/datum.rs b/src/librustc_trans/trans/datum.rs index 418ff4c8337e..339b3f0f920d 100644 --- a/src/librustc_trans/trans/datum.rs +++ b/src/librustc_trans/trans/datum.rs @@ -288,20 +288,29 @@ pub fn immediate_rvalue_bcx<'blk, 'tcx>(bcx: Block<'blk, 'tcx>, return DatumBlock::new(bcx, immediate_rvalue(val, ty)) } - /// Allocates temporary space on the stack using alloca() and returns a by-ref Datum pointing to /// it. The memory will be dropped upon exit from `scope`. The callback `populate` should /// initialize the memory. +/// +/// The flag `zero` indicates how the temporary space itself should be +/// initialized at the outset of the function; the only time that +/// `InitAlloca::Uninit` is a valid value for `zero` is when the +/// caller can prove that either (1.) the code injected by `populate` +/// onto `bcx` always dominates the end of `scope`, or (2.) the data +/// being allocated has no associated destructor. pub fn lvalue_scratch_datum<'blk, 'tcx, A, F>(bcx: Block<'blk, 'tcx>, ty: Ty<'tcx>, name: &str, + zero: InitAlloca, scope: cleanup::ScopeId, arg: A, populate: F) -> DatumBlock<'blk, 'tcx, Lvalue> where F: FnOnce(A, Block<'blk, 'tcx>, ValueRef) -> Block<'blk, 'tcx>, { - let scratch = alloc_ty(bcx, ty, name); + // Very subtle: potentially initialize the scratch memory at point where it is alloca'ed. + // (See discussion at Issue 30530.) + let scratch = alloc_ty_init(bcx, ty, zero, name); // Subtle. Populate the scratch memory *before* scheduling cleanup. let bcx = populate(arg, bcx, scratch); @@ -496,7 +505,7 @@ impl<'tcx> Datum<'tcx, Rvalue> { ByValue => { lvalue_scratch_datum( - bcx, self.ty, name, scope, self, + bcx, self.ty, name, InitAlloca::Dropped, scope, self, |this, bcx, llval| { call_lifetime_start(bcx, llval); let bcx = this.store_to(bcx, llval);