Skip to content

Commit

Permalink
Auto merge of #78373 - matthewjasper:drop-on-into, r=pnkfelix
Browse files Browse the repository at this point in the history
Don't leak return value after panic in drop

Closes #47949
  • Loading branch information
bors committed Dec 5, 2020
2 parents 551a2c6 + 4fef391 commit 9122b76
Show file tree
Hide file tree
Showing 32 changed files with 11,088 additions and 10,909 deletions.
10 changes: 7 additions & 3 deletions compiler/rustc_mir_build/src/build/block.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ use crate::build::ForGuard::OutsideGuard;
use crate::build::{BlockAnd, BlockAndExtension, BlockFrame, Builder};
use crate::thir::*;
use rustc_hir as hir;
use rustc_middle::middle::region;
use rustc_middle::mir::*;
use rustc_session::lint::builtin::UNSAFE_OP_IN_UNSAFE_FN;
use rustc_session::lint::Level;
Expand All @@ -12,6 +13,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
crate fn ast_block(
&mut self,
destination: Place<'tcx>,
scope: Option<region::Scope>,
block: BasicBlock,
ast_block: &'tcx hir::Block<'tcx>,
source_info: SourceInfo,
Expand All @@ -28,9 +30,10 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
self.in_opt_scope(opt_destruction_scope.map(|de| (de, source_info)), move |this| {
this.in_scope((region_scope, source_info), LintLevel::Inherited, move |this| {
if targeted_by_break {
this.in_breakable_scope(None, destination, span, |this| {
this.in_breakable_scope(None, destination, scope, span, |this| {
Some(this.ast_block_stmts(
destination,
scope,
block,
span,
stmts,
Expand All @@ -39,7 +42,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
))
})
} else {
this.ast_block_stmts(destination, block, span, stmts, expr, safety_mode)
this.ast_block_stmts(destination, scope, block, span, stmts, expr, safety_mode)
}
})
})
Expand All @@ -48,6 +51,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
fn ast_block_stmts(
&mut self,
destination: Place<'tcx>,
scope: Option<region::Scope>,
mut block: BasicBlock,
span: Span,
stmts: Vec<StmtRef<'tcx>>,
Expand Down Expand Up @@ -182,7 +186,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
};
this.block_context.push(BlockFrame::TailExpr { tail_result_is_ignored, span });

unpack!(block = this.into(destination, block, expr));
unpack!(block = this.into(destination, scope, block, expr));
let popped = this.block_context.pop();

assert!(popped.map_or(false, |bf| bf.is_tail_expr()));
Expand Down
20 changes: 16 additions & 4 deletions compiler/rustc_mir_build/src/build/expr/as_rvalue.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ use rustc_middle::mir::*;
use rustc_middle::ty::{self, Ty, UpvarSubsts};
use rustc_span::Span;

use std::slice;

impl<'a, 'tcx> Builder<'a, 'tcx> {
/// Returns an rvalue suitable for use until the end of the current
/// scope expression.
Expand Down Expand Up @@ -112,12 +114,19 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
let box_ = Rvalue::NullaryOp(NullOp::Box, value.ty);
this.cfg.push_assign(block, source_info, Place::from(result), box_);

// initialize the box contents:
// Initialize the box contents. No scope is needed since the
// `Box` is already scheduled to be dropped.
unpack!(
block =
this.into(this.hir.tcx().mk_place_deref(Place::from(result)), block, value)
block = this.into(
this.hir.tcx().mk_place_deref(Place::from(result)),
None,
block,
value,
)
);
block.and(Rvalue::Use(Operand::Move(Place::from(result))))
let result_operand = Operand::Move(Place::from(result));
this.record_operands_moved(slice::from_ref(&result_operand));
block.and(Rvalue::Use(result_operand))
}
ExprKind::Cast { source } => {
let source = unpack!(block = this.as_operand(block, scope, source));
Expand Down Expand Up @@ -161,6 +170,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
.map(|f| unpack!(block = this.as_operand(block, scope, f)))
.collect();

this.record_operands_moved(&fields);
block.and(Rvalue::Aggregate(box AggregateKind::Array(el_ty), fields))
}
ExprKind::Tuple { fields } => {
Expand All @@ -171,6 +181,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
.map(|f| unpack!(block = this.as_operand(block, scope, f)))
.collect();

this.record_operands_moved(&fields);
block.and(Rvalue::Aggregate(box AggregateKind::Tuple, fields))
}
ExprKind::Closure { closure_id, substs, upvars, movability } => {
Expand Down Expand Up @@ -222,6 +233,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
}
UpvarSubsts::Closure(substs) => box AggregateKind::Closure(closure_id, substs),
};
this.record_operands_moved(&operands);
block.and(Rvalue::Aggregate(result, operands))
}
ExprKind::Assign { .. } | ExprKind::AssignOp { .. } => {
Expand Down
6 changes: 1 addition & 5 deletions compiler/rustc_mir_build/src/build/expr/as_temp.rs
Original file line number Diff line number Diff line change
Expand Up @@ -114,11 +114,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
}
}

unpack!(block = this.into(temp_place, block, expr));

if let Some(temp_lifetime) = temp_lifetime {
this.schedule_drop(expr_span, temp_lifetime, temp, DropKind::Value);
}
unpack!(block = this.into(temp_place, temp_lifetime, block, expr));

block.and(temp)
}
Expand Down
99 changes: 70 additions & 29 deletions compiler/rustc_mir_build/src/build/expr/into.rs
Original file line number Diff line number Diff line change
@@ -1,28 +1,37 @@
//! See docs in build/expr/mod.rs

use crate::build::expr::category::{Category, RvalueFunc};
use crate::build::scope::DropKind;
use crate::build::{BlockAnd, BlockAndExtension, BlockFrame, Builder};
use crate::thir::*;
use rustc_ast::InlineAsmOptions;
use rustc_data_structures::fx::FxHashMap;
use rustc_data_structures::stack::ensure_sufficient_stack;
use rustc_hir as hir;
use rustc_middle::middle::region;
use rustc_middle::mir::*;
use rustc_middle::ty::{self, CanonicalUserTypeAnnotation};
use rustc_span::symbol::sym;

use rustc_target::spec::abi::Abi;

use std::slice;

impl<'a, 'tcx> Builder<'a, 'tcx> {
/// Compile `expr`, storing the result into `destination`, which
/// is assumed to be uninitialized.
/// If a `drop_scope` is provided, `destination` is scheduled to be dropped
/// in `scope` once it has been initialized.
crate fn into_expr(
&mut self,
destination: Place<'tcx>,
scope: Option<region::Scope>,
mut block: BasicBlock,
expr: Expr<'tcx>,
) -> BlockAnd<()> {
debug!("into_expr(destination={:?}, block={:?}, expr={:?})", destination, block, expr);
debug!(
"into_expr(destination={:?}, scope={:?}, block={:?}, expr={:?})",
destination, scope, block, expr
);

// since we frequently have to reference `self` from within a
// closure, where `self` would be shadowed, it's easier to
Expand All @@ -37,6 +46,14 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
_ => false,
};

let schedule_drop = move |this: &mut Self| {
if let Some(drop_scope) = scope {
let local =
destination.as_local().expect("cannot schedule drop of non-Local place");
this.schedule_drop(expr_span, drop_scope, local, DropKind::Value);
}
};

if !expr_is_block_or_scope {
this.block_context.push(BlockFrame::SubExpr);
}
Expand All @@ -46,15 +63,15 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
let region_scope = (region_scope, source_info);
ensure_sufficient_stack(|| {
this.in_scope(region_scope, lint_level, |this| {
this.into(destination, block, value)
this.into(destination, scope, block, value)
})
})
}
ExprKind::Block { body: ast_block } => {
this.ast_block(destination, block, ast_block, source_info)
this.ast_block(destination, scope, block, ast_block, source_info)
}
ExprKind::Match { scrutinee, arms } => {
this.match_expr(destination, expr_span, block, scrutinee, arms)
this.match_expr(destination, scope, expr_span, block, scrutinee, arms)
}
ExprKind::NeverToAny { source } => {
let source = this.hir.mirror(source);
Expand All @@ -66,6 +83,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {

// This is an optimization. If the expression was a call then we already have an
// unreachable block. Don't bother to terminate it and create a new one.
schedule_drop(this);
if is_call {
block.unit()
} else {
Expand Down Expand Up @@ -141,26 +159,35 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
// Start the loop.
this.cfg.goto(block, source_info, loop_block);

this.in_breakable_scope(Some(loop_block), destination, expr_span, move |this| {
// conduct the test, if necessary
let body_block = this.cfg.start_new_block();
this.cfg.terminate(
loop_block,
source_info,
TerminatorKind::FalseUnwind { real_target: body_block, unwind: None },
);
this.diverge_from(loop_block);

// The “return” value of the loop body must always be an unit. We therefore
// introduce a unit temporary as the destination for the loop body.
let tmp = this.get_unit_temp();
// Execute the body, branching back to the test.
let body_block_end = unpack!(this.into(tmp, body_block, body));
this.cfg.goto(body_block_end, source_info, loop_block);

// Loops are only exited by `break` expressions.
None
})
this.in_breakable_scope(
Some(loop_block),
destination,
scope,
expr_span,
move |this| {
// conduct the test, if necessary
let body_block = this.cfg.start_new_block();
this.cfg.terminate(
loop_block,
source_info,
TerminatorKind::FalseUnwind { real_target: body_block, unwind: None },
);
this.diverge_from(loop_block);

// The “return” value of the loop body must always be an unit. We therefore
// introduce a unit temporary as the destination for the loop body.
let tmp = this.get_unit_temp();
// Execute the body, branching back to the test.
// We don't need to provide a drop scope because `tmp`
// has type `()`.
let body_block_end = unpack!(this.into(tmp, None, body_block, body));
this.cfg.goto(body_block_end, source_info, loop_block);
schedule_drop(this);

// Loops are only exited by `break` expressions.
None
},
)
}
ExprKind::Call { ty, fun, args, from_hir_call, fn_span } => {
let intrinsic = match *ty.kind() {
Expand Down Expand Up @@ -192,8 +219,14 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
.local_decls
.push(LocalDecl::with_source_info(ptr_ty, source_info).internal());
let ptr_temp = Place::from(ptr_temp);
let block = unpack!(this.into(ptr_temp, block, ptr));
this.into(this.hir.tcx().mk_place_deref(ptr_temp), block, val)
// No need for a scope, ptr_temp doesn't need drop
let block = unpack!(this.into(ptr_temp, None, block, ptr));
// Maybe we should provide a scope here so that
// `move_val_init` wouldn't leak on panic even with an
// arbitrary `val` expression, but `schedule_drop`,
// borrowck and drop elaboration all prevent us from
// dropping `ptr_temp.deref()`.
this.into(this.hir.tcx().mk_place_deref(ptr_temp), None, block, val)
} else {
let args: Vec<_> = args
.into_iter()
Expand Down Expand Up @@ -226,10 +259,11 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
},
);
this.diverge_from(block);
schedule_drop(this);
success.unit()
}
}
ExprKind::Use { source } => this.into(destination, block, source),
ExprKind::Use { source } => this.into(destination, scope, block, source),
ExprKind::Borrow { arg, borrow_kind } => {
// We don't do this in `as_rvalue` because we use `as_place`
// for borrow expressions, so we cannot create an `RValue` that
Expand Down Expand Up @@ -271,7 +305,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {

let field_names = this.hir.all_fields(adt_def, variant_index);

let fields = if let Some(FruInfo { base, field_types }) = base {
let fields: Vec<_> = if let Some(FruInfo { base, field_types }) = base {
let base = unpack!(block = this.as_place(block, base));

// MIR does not natively support FRU, so for each
Expand Down Expand Up @@ -306,12 +340,14 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
user_ty,
active_field_index,
);
this.record_operands_moved(&fields);
this.cfg.push_assign(
block,
source_info,
destination,
Rvalue::Aggregate(adt, fields),
);
schedule_drop(this);
block.unit()
}
ExprKind::InlineAsm { template, operands, options, line_spans } => {
Expand Down Expand Up @@ -408,6 +444,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
let place = unpack!(block = this.as_place(block, expr));
let rvalue = Rvalue::Use(this.consume_by_copy_or_move(place));
this.cfg.push_assign(block, source_info, destination, rvalue);
schedule_drop(this);
block.unit()
}
ExprKind::Index { .. } | ExprKind::Deref { .. } | ExprKind::Field { .. } => {
Expand All @@ -425,19 +462,22 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
let place = unpack!(block = this.as_place(block, expr));
let rvalue = Rvalue::Use(this.consume_by_copy_or_move(place));
this.cfg.push_assign(block, source_info, destination, rvalue);
schedule_drop(this);
block.unit()
}

ExprKind::Yield { value } => {
let scope = this.local_scope();
let value = unpack!(block = this.as_operand(block, scope, value));
let resume = this.cfg.start_new_block();
this.record_operands_moved(slice::from_ref(&value));
this.cfg.terminate(
block,
source_info,
TerminatorKind::Yield { value, resume, resume_arg: destination, drop: None },
);
this.generator_drop_cleanup(block);
schedule_drop(this);
resume.unit()
}

Expand Down Expand Up @@ -469,6 +509,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {

let rvalue = unpack!(block = this.as_local_rvalue(block, expr));
this.cfg.push_assign(block, source_info, destination, rvalue);
schedule_drop(this);
block.unit()
}
};
Expand Down
2 changes: 2 additions & 0 deletions compiler/rustc_mir_build/src/build/expr/stmt.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ use crate::build::{BlockAnd, BlockAndExtension, BlockFrame, Builder};
use crate::thir::*;
use rustc_middle::middle::region;
use rustc_middle::mir::*;
use std::slice;

impl<'a, 'tcx> Builder<'a, 'tcx> {
/// Builds a block of MIR statements to evaluate the THIR `expr`.
Expand Down Expand Up @@ -46,6 +47,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
if this.hir.needs_drop(lhs.ty) {
let rhs = unpack!(block = this.as_local_operand(block, rhs));
let lhs = unpack!(block = this.as_place(block, lhs));
this.record_operands_moved(slice::from_ref(&rhs));
unpack!(block = this.build_drop_and_replace(block, lhs_span, lhs, rhs));
} else {
let rhs = unpack!(block = this.as_local_rvalue(block, rhs));
Expand Down
Loading

0 comments on commit 9122b76

Please sign in to comment.