Skip to content

Avoid adding drops for types w/ no dtor in MIR construction #30492

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jan 6, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 34 additions & 1 deletion src/librustc/middle/ty/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ use std::vec::IntoIter;
use std::collections::{HashMap, HashSet};
use syntax::ast::{self, CrateNum, Name, NodeId};
use syntax::attr::{self, AttrMetaMethods};
use syntax::codemap::Span;
use syntax::codemap::{DUMMY_SP, Span};
use syntax::parse::token::{InternedString, special_idents};

use rustc_front::hir;
Expand Down Expand Up @@ -2394,6 +2394,39 @@ impl<'tcx> ctxt<'tcx> {
|| self.sess.cstore.item_super_predicates(self, did))
}

/// 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_given_env<'a>(&self,
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same note that @nagisa wrote on fn type_needs_drop applies here, namely, that the assumptions that hold in the context of trans may not hold here. (When I wrote "normalized" in the original comment, I probably should have written "monomorphized.")

// know whether the type implements Copy (and thus needs no
// cleanup/drop/zeroing) ...
let implements_copy = !ty.moves_by_default(param_env, DUMMY_SP);

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(self);
debug!("type_needs_drop ty={:?} contents={:?}", ty, contents);
contents.needs_drop(self)
}

/// Get the attributes of a definition.
pub fn get_attrs(&self, did: DefId) -> Cow<'tcx, [ast::Attribute]> {
if let Some(id) = self.map.as_local_node_id(did) {
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_mir/build/scope.rs
Original file line number Diff line number Diff line change
Expand Up @@ -248,7 +248,7 @@ impl<'a,'tcx> Builder<'a,'tcx> {
kind: DropKind,
lvalue: &Lvalue<'tcx>,
lvalue_ty: Ty<'tcx>) {
if self.hir.needs_drop(lvalue_ty, span) {
if self.hir.needs_drop(lvalue_ty) {
match self.scopes.iter_mut().rev().find(|s| s.extent == extent) {
Some(scope) => {
scope.drops.push((kind, span, lvalue.clone()));
Expand Down
13 changes: 2 additions & 11 deletions src/librustc_mir/hair/cx/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -98,17 +98,8 @@ impl<'a,'tcx:'a> Cx<'a, 'tcx> {
.collect()
}

pub fn needs_drop(&mut self, ty: Ty<'tcx>, span: Span) -> bool {
if self.infcx.type_moves_by_default(ty, span) {
// FIXME(#21859) we should do an add'l check here to determine if
// any dtor will execute, but the relevant fn
// (`type_needs_drop`) is currently factored into
// `librustc_trans`, so we can't easily do so.
true
} else {
// if type implements Copy, cannot require drop
false
}
pub fn needs_drop(&mut self, ty: Ty<'tcx>) -> bool {
self.tcx.type_needs_drop_given_env(ty, &self.infcx.parameter_environment)
}

pub fn span_bug(&mut self, span: Span, message: &str) -> ! {
Expand Down
41 changes: 1 addition & 40 deletions src/librustc_trans/trans/common.rs
Original file line number Diff line number Diff line change
Expand Up @@ -73,45 +73,6 @@ pub fn type_is_fat_ptr<'tcx>(cx: &ty::ctxt<'tcx>, ty: Ty<'tcx>) -> bool {
}
}

/// 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 {
type_needs_drop_given_env(cx, ty, &cx.empty_parameter_environment())
}

/// 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.moves_by_default(param_env, DUMMY_SP);

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);
debug!("type_needs_drop ty={:?} contents={:?}", ty, contents);
contents.needs_drop(cx)
}

fn type_is_newtype_immediate<'a, 'tcx>(ccx: &CrateContext<'a, 'tcx>, ty: Ty<'tcx>) -> bool {
match ty.sty {
ty::TyStruct(def, substs) => {
Expand Down Expand Up @@ -518,7 +479,7 @@ impl<'a, 'tcx> FunctionContext<'a, 'tcx> {
/// 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)
self.ccx.tcx().type_needs_drop_given_env(ty, &self.param_env)
}

pub fn eh_personality(&self) -> ValueRef {
Expand Down
8 changes: 6 additions & 2 deletions src/librustc_trans/trans/glue.rs
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,10 @@ pub fn trans_exchange_free_ty<'blk, 'tcx>(bcx: Block<'blk, 'tcx>,
}
}

fn type_needs_drop<'tcx>(tcx: &ty::ctxt<'tcx>, ty: Ty<'tcx>) -> bool {
tcx.type_needs_drop_given_env(ty, &tcx.empty_parameter_environment())
}

pub fn get_drop_glue_type<'a, 'tcx>(ccx: &CrateContext<'a, 'tcx>,
t: Ty<'tcx>) -> Ty<'tcx> {
let tcx = ccx.tcx();
Expand All @@ -106,11 +110,11 @@ pub fn get_drop_glue_type<'a, 'tcx>(ccx: &CrateContext<'a, 'tcx>,
// returned `tcx.types.i8` does not appear unsound. The impact on
// code quality is unknown at this time.)

if !type_needs_drop(tcx, t) {
if !type_needs_drop(&tcx, t) {
return tcx.types.i8;
}
match t.sty {
ty::TyBox(typ) if !type_needs_drop(tcx, typ)
ty::TyBox(typ) if !type_needs_drop(&tcx, typ)
&& type_is_sized(tcx, typ) => {
let llty = sizing_type_of(ccx, typ);
// `Box<ZeroSizeType>` does not allocate.
Expand Down