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 1/7] 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 c744ef321278d..e152e11a6a657 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 d694374acc927..b2bc7c2af933b 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 418ff4c8337e7..339b3f0f920d7 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); From cec7280bf367be9da472e02eba59b5440b5336c9 Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Fri, 8 Jan 2016 20:40:13 +0100 Subject: [PATCH 2/7] debug instrumentation (can remove) --- src/librustc_trans/trans/_match.rs | 3 +++ src/librustc_trans/trans/adt.rs | 6 +++++- src/librustc_trans/trans/base.rs | 17 +++++++++++++++-- src/librustc_trans/trans/datum.rs | 6 ++++++ src/librustc_trans/trans/expr.rs | 2 ++ 5 files changed, 31 insertions(+), 3 deletions(-) diff --git a/src/librustc_trans/trans/_match.rs b/src/librustc_trans/trans/_match.rs index 0ad780fb0e4d3..f46a7ea67b5f2 100644 --- a/src/librustc_trans/trans/_match.rs +++ b/src/librustc_trans/trans/_match.rs @@ -1760,6 +1760,9 @@ fn mk_binding_alloca<'blk, 'tcx, A, F>(bcx: Block<'blk, 'tcx>, let lvalue = Lvalue::new_with_hint(caller_name, bcx, p_id, HintKind::DontZeroJustUse); let datum = Datum::new(llval, var_ty, lvalue); + debug!("mk_binding_alloca cleanup_scope={:?} llval={} var_ty={:?}", + cleanup_scope, bcx.ccx().tn().val_to_string(llval), var_ty); + // Subtle: be sure that we *populate* the memory *before* // we schedule the cleanup. call_lifetime_start(bcx, llval); diff --git a/src/librustc_trans/trans/adt.rs b/src/librustc_trans/trans/adt.rs index e152e11a6a657..d22d619b96227 100644 --- a/src/librustc_trans/trans/adt.rs +++ b/src/librustc_trans/trans/adt.rs @@ -1281,7 +1281,11 @@ pub fn trans_drop_flag_ptr<'blk, 'tcx>(mut bcx: Block<'blk, 'tcx>, 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 + cleanup::CustomScope(custom_cleanup_scope), (), |_, bcx, _| { + debug!("no-op populate call for trans_drop_flag_ptr on dtor_type={:?}", + tcx.dtor_type()); + bcx + } )); bcx = fold_variants(bcx, r, val, |variant_cx, st, value| { let ptr = struct_field_ptr(variant_cx, st, MaybeSizedValue::sized(value), diff --git a/src/librustc_trans/trans/base.rs b/src/librustc_trans/trans/base.rs index b2bc7c2af933b..1e5c60609b0b8 100644 --- a/src/librustc_trans/trans/base.rs +++ b/src/librustc_trans/trans/base.rs @@ -1689,6 +1689,8 @@ pub fn create_datums_for_fn_args<'a, 'tcx>(mut bcx: Block<'a, 'tcx>, let fcx = bcx.fcx; let arg_scope_id = cleanup::CustomScope(arg_scope); + debug!("create_datums_for_fn_args"); + // Return an array wrapping the ValueRefs that we get from `get_param` for // each argument into datums. // @@ -1723,6 +1725,9 @@ pub fn create_datums_for_fn_args<'a, 'tcx>(mut bcx: Block<'a, 'tcx>, unpack_datum!(bcx, datum::lvalue_scratch_datum(bcx, arg_ty, "", uninit_reason, arg_scope_id, (data, extra), |(data, extra), bcx, dst| { + debug!("populate call for create_datum_for_fn_args \ + early fat arg, on arg[{}] ty={:?}", i, arg_ty); + Store(bcx, data, expr::get_dataptr(bcx, dst)); Store(bcx, extra, expr::get_meta(bcx, dst)); bcx @@ -1738,7 +1743,13 @@ pub fn create_datums_for_fn_args<'a, 'tcx>(mut bcx: Block<'a, 'tcx>, uninit_reason, arg_scope_id, tmp, - |tmp, bcx, dst| tmp.store_to(bcx, dst))) + |tmp, bcx, dst| { + + debug!("populate call for create_datum_for_fn_args \ + early thin arg, on arg[{}] ty={:?}", i, arg_ty); + + tmp.store_to(bcx, dst) + })) } } else { // FIXME(pcwalton): Reduce the amount of code bloat this is responsible for. @@ -1753,7 +1764,9 @@ pub fn create_datums_for_fn_args<'a, 'tcx>(mut bcx: Block<'a, 'tcx>, (), |(), mut bcx, - llval| { + llval| { + debug!("populate call for create_datum_for_fn_args \ + tupled_args, on arg[{}] ty={:?}", i, arg_ty); for (j, &tupled_arg_ty) in tupled_arg_tys.iter().enumerate() { let lldest = StructGEP(bcx, llval, j); diff --git a/src/librustc_trans/trans/datum.rs b/src/librustc_trans/trans/datum.rs index 339b3f0f920d7..6a033602adaba 100644 --- a/src/librustc_trans/trans/datum.rs +++ b/src/librustc_trans/trans/datum.rs @@ -311,6 +311,8 @@ pub fn lvalue_scratch_datum<'blk, 'tcx, A, F>(bcx: Block<'blk, 'tcx>, // 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); + debug!("lvalue_scratch_datum scope={:?} scratch={} ty={:?}", + scope, bcx.ccx().tn().val_to_string(scratch), ty); // Subtle. Populate the scratch memory *before* scheduling cleanup. let bcx = populate(arg, bcx, scratch); @@ -349,6 +351,8 @@ fn add_rvalue_clean<'a, 'tcx>(mode: RvalueMode, scope: cleanup::ScopeId, val: ValueRef, ty: Ty<'tcx>) { + debug!("add_rvalue_clean scope={:?} val={} ty={:?}", + scope, fcx.ccx.tn().val_to_string(val), ty); match mode { ByValue => { fcx.schedule_drop_immediate(scope, val, ty); } ByRef => { @@ -507,6 +511,8 @@ impl<'tcx> Datum<'tcx, Rvalue> { lvalue_scratch_datum( bcx, self.ty, name, InitAlloca::Dropped, scope, self, |this, bcx, llval| { + debug!("populate call for Datum::to_lvalue_datum_in_scope \ + self.ty={:?}", this.ty); call_lifetime_start(bcx, llval); let bcx = this.store_to(bcx, llval); bcx.fcx.schedule_lifetime_end(scope, llval); diff --git a/src/librustc_trans/trans/expr.rs b/src/librustc_trans/trans/expr.rs index fb6f2190207ee..57afd0b580f17 100644 --- a/src/librustc_trans/trans/expr.rs +++ b/src/librustc_trans/trans/expr.rs @@ -1487,6 +1487,8 @@ pub fn trans_adt<'a, 'blk, 'tcx>(mut bcx: Block<'blk, 'tcx>, } }; + debug!("trans_adt"); + // This scope holds intermediates that must be cleaned should // panic occur before the ADT as a whole is ready. let custom_cleanup_scope = fcx.push_custom_cleanup_scope(); From f251ff4081dc89b984000547ec0903e845f14201 Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Mon, 11 Jan 2016 18:23:22 +0100 Subject: [PATCH 3/7] bug fixes for issues 30018 and 30822. includes bugfixes pointed out during review: * Only `call_lifetime_start` for an alloca if the function entry does not itself initialize it to "dropped." * Remove `schedule_lifetime_end` after writing an *element* into a borrowed slice. (As explained by [dotdash][irc], "the lifetime end that is being removed was for an element in the slice, which is not an alloca of its own and has no lifetime start of its own") [irc]: https://botbot.me/mozilla/rust-internals/2016-01-13/?msg=57844504&page=3 --- src/librustc_trans/trans/tvec.rs | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/src/librustc_trans/trans/tvec.rs b/src/librustc_trans/trans/tvec.rs index c7e1af5853d11..3a1568a70c992 100644 --- a/src/librustc_trans/trans/tvec.rs +++ b/src/librustc_trans/trans/tvec.rs @@ -111,8 +111,15 @@ pub fn trans_slice_vec<'blk, 'tcx>(bcx: Block<'blk, 'tcx>, // Always create an alloca even if zero-sized, to preserve // the non-null invariant of the inner slice ptr - let llfixed = base::alloc_ty(bcx, fixed_ty, ""); - call_lifetime_start(bcx, llfixed); + let llfixed; + // Issue 30018: ensure state is initialized as dropped if necessary. + if fcx.type_needs_drop(vt.unit_ty) { + llfixed = base::alloc_ty_init(bcx, fixed_ty, InitAlloca::Dropped, ""); + } else { + let uninit = InitAlloca::Uninit("fcx says vt.unit_ty is non-drop"); + llfixed = base::alloc_ty_init(bcx, fixed_ty, uninit, ""); + call_lifetime_start(bcx, llfixed); + }; if count > 0 { // Arrange for the backing array to be cleaned up. @@ -212,8 +219,8 @@ fn write_content<'blk, 'tcx>(bcx: Block<'blk, 'tcx>, bcx = expr::trans_into(bcx, &**element, SaveIn(lleltptr)); let scope = cleanup::CustomScope(temp_scope); - fcx.schedule_lifetime_end(scope, lleltptr); - fcx.schedule_drop_mem(scope, lleltptr, vt.unit_ty, None); + // Issue #30822: mark memory as dropped after running destructor + fcx.schedule_drop_and_fill_mem(scope, lleltptr, vt.unit_ty, None); } fcx.pop_custom_cleanup_scope(temp_scope); } From df1283cd1a6902bad5c869b124519814cd482064 Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Mon, 11 Jan 2016 18:32:28 +0100 Subject: [PATCH 4/7] Unit/regression tests for issues #29092, #30018, #30530, #30822. Note that the test for #30822 is folded into the test for #30530 (but the file name mentions only 30530). --- src/test/run-pass/issue-29092.rs | 35 +++++++ src/test/run-pass/issue-30018-nopanic.rs | 111 +++++++++++++++++++++++ src/test/run-pass/issue-30018-panic.rs | 32 +++++++ src/test/run-pass/issue-30530.rs | 35 +++++++ 4 files changed, 213 insertions(+) create mode 100644 src/test/run-pass/issue-29092.rs create mode 100644 src/test/run-pass/issue-30018-nopanic.rs create mode 100644 src/test/run-pass/issue-30018-panic.rs create mode 100644 src/test/run-pass/issue-30530.rs diff --git a/src/test/run-pass/issue-29092.rs b/src/test/run-pass/issue-29092.rs new file mode 100644 index 0000000000000..c55cc91cc928f --- /dev/null +++ b/src/test/run-pass/issue-29092.rs @@ -0,0 +1,35 @@ +// Copyright 2012-2016 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. + +// Regression test for Issue #29092. +// +// (Possibly redundant with regression test run-pass/issue-30530.rs) + +use self::Term::*; + +#[derive(Clone)] +pub enum Term { + Dummy, + A(Box), + B(Box), +} + +// a small-step evaluator +pub fn small_eval(v: Term) -> Term { + match v { + A(t) => *t.clone(), + B(t) => *t.clone(), + _ => Dummy, + } +} + +fn main() { + small_eval(Dummy); +} diff --git a/src/test/run-pass/issue-30018-nopanic.rs b/src/test/run-pass/issue-30018-nopanic.rs new file mode 100644 index 0000000000000..25eff9def9dfc --- /dev/null +++ b/src/test/run-pass/issue-30018-nopanic.rs @@ -0,0 +1,111 @@ +// Copyright 2012-2016 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. + +// More thorough regression test for Issues #30018 and #30822. This +// attempts to explore different ways that array element construction +// (for both scratch arrays and non-scratch ones) interacts with +// breaks in the control-flow, in terms of the order of evaluation of +// the destructors (which may change; see RFC Issue 744) and the +// number of times that the destructor evaluates for each value (which +// should never exceed 1; this latter case is what #30822 is about). + +use std::cell::RefCell; + +struct D<'a>(&'a RefCell>, i32); + +impl<'a> Drop for D<'a> { + fn drop(&mut self) { + println!("Dropping D({})", self.1); + (self.0).borrow_mut().push(self.1); + } +} + +fn main() { + println!("Start"); + break_during_elem(); + break_after_whole(); + println!("Finis"); +} + +fn break_during_elem() { + let log = &RefCell::new(Vec::new()); + + // CASE 1: Fixed-size array itself is stored in _r slot. + loop { + let _r = [D(log, 10), + D(log, 11), + { D(log, 12); break; }, + D(log, 13)]; + } + assert_eq!(&log.borrow()[..], &[12, 11, 10]); + log.borrow_mut().clear(); + + // CASE 2: Slice (borrow of array) is stored in _r slot. + // This is the case that is actually being reported in #30018. + loop { + let _r = &[D(log, 20), + D(log, 21), + { D(log, 22); break; }, + D(log, 23)]; + } + assert_eq!(&log.borrow()[..], &[22, 21, 20]); + log.borrow_mut().clear(); + + // CASE 3: (Borrow of) slice-index of array is stored in _r slot. + loop { + let _r = &[D(log, 30), + D(log, 31), + { D(log, 32); break; }, + D(log, 33)][..]; + } + assert_eq!(&log.borrow()[..], &[32, 31, 30]); + log.borrow_mut().clear(); +} + +// The purpose of these functions is to test what happens when we +// panic after an array has been constructed in its entirety. +// +// It is meant to act as proof that we still need to continue +// scheduling the destruction of an array even after we've scheduling +// drop for its elements during construction; the latter is tested by +// `fn break_during_elem()`. +fn break_after_whole() { + let log = &RefCell::new(Vec::new()); + + // CASE 1: Fixed-size array itself is stored in _r slot. + loop { + let _r = [D(log, 10), + D(log, 11), + D(log, 12)]; + break; + } + assert_eq!(&log.borrow()[..], &[10, 11, 12]); + log.borrow_mut().clear(); + + // CASE 2: Slice (borrow of array) is stored in _r slot. + loop { + let _r = &[D(log, 20), + D(log, 21), + D(log, 22)]; + break; + } + assert_eq!(&log.borrow()[..], &[20, 21, 22]); + log.borrow_mut().clear(); + + // CASE 3: (Borrow of) slice-index of array is stored in _r slot. + loop { + let _r = &[D(log, 30), + D(log, 31), + D(log, 32)][..]; + break; + } + assert_eq!(&log.borrow()[..], &[30, 31, 32]); + log.borrow_mut().clear(); +} diff --git a/src/test/run-pass/issue-30018-panic.rs b/src/test/run-pass/issue-30018-panic.rs new file mode 100644 index 0000000000000..da4d5f19d4a2e --- /dev/null +++ b/src/test/run-pass/issue-30018-panic.rs @@ -0,0 +1,32 @@ +// Copyright 2012-2016 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. + +// Regression test for Issue #30018. This is very similar to the +// original reported test, except that the panic is wrapped in a +// spawned thread to isolate the expected error result from the +// SIGTRAP injected by the drop-flag consistency checking. + +struct Foo; + +impl Drop for Foo { + fn drop(&mut self) {} +} + +fn foo() -> Foo { + panic!(); +} + +fn main() { + use std::thread; + let handle = thread::spawn(|| { + let _ = &[foo()]; + }); + let _ = handle.join(); +} diff --git a/src/test/run-pass/issue-30530.rs b/src/test/run-pass/issue-30530.rs new file mode 100644 index 0000000000000..d5139c908bdac --- /dev/null +++ b/src/test/run-pass/issue-30530.rs @@ -0,0 +1,35 @@ +// Copyright 2012-2016 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. + +// Regression test for Issue #30530: alloca's created for storing +// intermediate scratch values during brace-less match arms need to be +// initialized with their drop-flag set to "dropped" (or else we end +// up running the destructors on garbage data at the end of the +// function). + +pub enum Handler { + Default, + #[allow(dead_code)] + Custom(*mut Box), +} + +fn main() { + take(Handler::Default, Box::new(main)); +} + +#[inline(never)] +pub fn take(h: Handler, f: Box) -> Box { + unsafe { + match h { + Handler::Custom(ptr) => *Box::from_raw(ptr), + Handler::Default => f, + } + } +} From 8168765d43937d1def40f13bcfe9be5748a180b3 Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Tue, 12 Jan 2016 17:17:50 +0100 Subject: [PATCH 5/7] Factored out private routine for emitting LLVM lifetime intrinsic calls. (The reason this is not factored as far as possible because a subsequent commit is going to need to do construction without having access to a `cx`.) --- src/librustc_trans/trans/base.rs | 75 +++++++++++++++++++------------- 1 file changed, 45 insertions(+), 30 deletions(-) diff --git a/src/librustc_trans/trans/base.rs b/src/librustc_trans/trans/base.rs index 1e5c60609b0b8..636db8fecdf7f 100644 --- a/src/librustc_trans/trans/base.rs +++ b/src/librustc_trans/trans/base.rs @@ -1147,48 +1147,63 @@ pub fn with_cond<'blk, 'tcx, F>(bcx: Block<'blk, 'tcx>, val: ValueRef, f: F) -> next_cx } -pub fn call_lifetime_start(cx: Block, ptr: ValueRef) { - if cx.sess().opts.optimize == config::No { +enum Lifetime { Start, End } + +// If LLVM lifetime intrinsic support is enabled (i.e. optimizations +// on), and `ptr` is nonzero-sized, then extracts the size of `ptr` +// and the intrinsic for `lt` and passes them to `emit`, which is in +// charge of generating code to call the passed intrinsic on whatever +// block of generated code is targetted for the intrinsic. +// +// If LLVM lifetime intrinsic support is disabled (i.e. optimizations +// off) or `ptr` is zero-sized, then no-op (does not call `emit`). +fn core_lifetime_emit<'blk, 'tcx, F>(ccx: &'blk CrateContext<'blk, 'tcx>, + ptr: ValueRef, + lt: Lifetime, + emit: F) + where F: FnOnce(&'blk CrateContext<'blk, 'tcx>, machine::llsize, ValueRef) +{ + if ccx.sess().opts.optimize == config::No { return; } - let _icx = push_ctxt("lifetime_start"); - let ccx = cx.ccx(); + let _icx = push_ctxt(match lt { + Lifetime::Start => "lifetime_start", + Lifetime::End => "lifetime_end" + }); let size = machine::llsize_of_alloc(ccx, val_ty(ptr).element_type()); if size == 0 { return; } - let ptr = PointerCast(cx, ptr, Type::i8p(ccx)); - let lifetime_start = ccx.get_intrinsic(&"llvm.lifetime.start"); - Call(cx, - lifetime_start, - &[C_u64(ccx, size), ptr], - None, - DebugLoc::None); + let lifetime_intrinsic = ccx.get_intrinsic(match lt { + Lifetime::Start => "llvm.lifetime.start", + Lifetime::End => "llvm.lifetime.end" + }); + emit(ccx, size, lifetime_intrinsic) } -pub fn call_lifetime_end(cx: Block, ptr: ValueRef) { - if cx.sess().opts.optimize == config::No { - return; - } - - let _icx = push_ctxt("lifetime_end"); - let ccx = cx.ccx(); - - let size = machine::llsize_of_alloc(ccx, val_ty(ptr).element_type()); - if size == 0 { - return; - } +pub fn call_lifetime_start(cx: Block, ptr: ValueRef) { + core_lifetime_emit(cx.ccx(), ptr, Lifetime::Start, |ccx, size, lifetime_start| { + let ptr = PointerCast(cx, ptr, Type::i8p(ccx)); + Call(cx, + lifetime_start, + &[C_u64(ccx, size), ptr], + None, + DebugLoc::None); + }) +} - let ptr = PointerCast(cx, ptr, Type::i8p(ccx)); - let lifetime_end = ccx.get_intrinsic(&"llvm.lifetime.end"); - Call(cx, - lifetime_end, - &[C_u64(ccx, size), ptr], - None, - DebugLoc::None); +pub fn call_lifetime_end(cx: Block, ptr: ValueRef) { + core_lifetime_emit(cx.ccx(), ptr, Lifetime::End, |ccx, size, lifetime_end| { + let ptr = PointerCast(cx, ptr, Type::i8p(ccx)); + Call(cx, + lifetime_end, + &[C_u64(ccx, size), ptr], + None, + DebugLoc::None); + }) } // Generates code for resumption of unwind at the end of a landing pad. From 7706e2333eacbbdf943164cec583a51b216bbde1 Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Tue, 12 Jan 2016 17:21:11 +0100 Subject: [PATCH 6/7] revise lifetime handling for alloca's that are initialized as "dropped." (This can/should be revisited when drop flags are stored out of band.) --- src/librustc_trans/trans/base.rs | 7 +++++++ src/librustc_trans/trans/datum.rs | 4 +++- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/src/librustc_trans/trans/base.rs b/src/librustc_trans/trans/base.rs index 636db8fecdf7f..375b3e3f0d2d5 100644 --- a/src/librustc_trans/trans/base.rs +++ b/src/librustc_trans/trans/base.rs @@ -1354,6 +1354,13 @@ pub fn alloca_dropped<'blk, 'tcx>(cx: Block<'blk, 'tcx>, ty: Ty<'tcx>, name: &st let p = alloca(cx, llty, name); let b = cx.fcx.ccx.builder(); b.position_before(cx.fcx.alloca_insert_pt.get().unwrap()); + + // This is just like `call_lifetime_start` (but latter expects a + // Block, which we do not have for `alloca_insert_pt`). + core_lifetime_emit(cx.ccx(), p, Lifetime::Start, |ccx, size, lifetime_start| { + let ptr = b.pointercast(p, Type::i8p(ccx)); + b.call(lifetime_start, &[C_u64(ccx, size), ptr], None); + }); memfill(&b, p, ty, adt::DTOR_DONE); p } diff --git a/src/librustc_trans/trans/datum.rs b/src/librustc_trans/trans/datum.rs index 6a033602adaba..32f263746d31e 100644 --- a/src/librustc_trans/trans/datum.rs +++ b/src/librustc_trans/trans/datum.rs @@ -513,7 +513,9 @@ impl<'tcx> Datum<'tcx, Rvalue> { |this, bcx, llval| { debug!("populate call for Datum::to_lvalue_datum_in_scope \ self.ty={:?}", this.ty); - call_lifetime_start(bcx, llval); + // do not call_lifetime_start here; the + // `InitAlloc::Dropped` will start scratch + // value's lifetime at open of function body. let bcx = this.store_to(bcx, llval); bcx.fcx.schedule_lifetime_end(scope, llval); bcx From decc2867577c65682c6a73c89c550d34b5b270ad Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Tue, 12 Jan 2016 17:24:00 +0100 Subject: [PATCH 7/7] add doc for new `fn alloc_ty_init`. (Note that it might be a good idea to replace *all* calls of `alloc_ty` with calls to `alloc_ty_init`, to encourage programmers to consider the appropriate value for the `init` flag when creating temporary values.) --- src/librustc_trans/trans/base.rs | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/librustc_trans/trans/base.rs b/src/librustc_trans/trans/base.rs index 375b3e3f0d2d5..5f2fe98727fa1 100644 --- a/src/librustc_trans/trans/base.rs +++ b/src/librustc_trans/trans/base.rs @@ -1331,6 +1331,18 @@ pub fn alloc_ty<'blk, 'tcx>(bcx: Block<'blk, 'tcx>, alloc_ty_init(bcx, t, InitAlloca::Uninit("all alloc_ty are uninit"), name) } +/// This variant of `fn alloc_ty` does not necessarily assume that the +/// alloca should be created with no initial value. Instead the caller +/// controls that assumption via the `init` flag. +/// +/// Note that if the alloca *is* initialized via `init`, then we will +/// also inject an `llvm.lifetime.start` before that initialization +/// occurs, and thus callers should not call_lifetime_start +/// themselves. But if `init` says "uninitialized", then callers are +/// in charge of choosing where to call_lifetime_start and +/// subsequently populate the alloca. +/// +/// (See related discussion on PR #30823.) pub fn alloc_ty_init<'blk, 'tcx>(bcx: Block<'blk, 'tcx>, t: Ty<'tcx>, init: InitAlloca,