Skip to content
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

Revert 78373 ("dont leak return value after panic in drop") #81257

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
10 changes: 3 additions & 7 deletions compiler/rustc_mir_build/src/build/block.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ 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 @@ -13,7 +12,6 @@ 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 @@ -30,10 +28,9 @@ 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, scope, span, |this| {
this.in_breakable_scope(None, destination, span, |this| {
Some(this.ast_block_stmts(
destination,
scope,
block,
span,
stmts,
Expand All @@ -42,7 +39,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
))
})
} else {
this.ast_block_stmts(destination, scope, block, span, stmts, expr, safety_mode)
this.ast_block_stmts(destination, block, span, stmts, expr, safety_mode)
}
})
})
Expand All @@ -51,7 +48,6 @@ 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 @@ -186,7 +182,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
};
this.block_context.push(BlockFrame::TailExpr { tail_result_is_ignored, span });

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

assert!(popped.map_or(false, |bf| bf.is_tail_expr()));
Expand Down
20 changes: 4 additions & 16 deletions compiler/rustc_mir_build/src/build/expr/as_rvalue.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,6 @@ 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 @@ -115,19 +113,12 @@ 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. No scope is needed since the
// `Box` is already scheduled to be dropped.
// initialize the box contents:
unpack!(
block = this.into(
this.hir.tcx().mk_place_deref(Place::from(result)),
None,
block,
value,
)
block =
this.into(this.hir.tcx().mk_place_deref(Place::from(result)), block, value)
);
let result_operand = Operand::Move(Place::from(result));
this.record_operands_moved(slice::from_ref(&result_operand));
block.and(Rvalue::Use(result_operand))
block.and(Rvalue::Use(Operand::Move(Place::from(result))))
}
ExprKind::Cast { source } => {
let source = unpack!(block = this.as_operand(block, scope, source));
Expand Down Expand Up @@ -171,7 +162,6 @@ 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 @@ -182,7 +172,6 @@ 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 @@ -234,7 +223,6 @@ 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: 5 additions & 1 deletion compiler/rustc_mir_build/src/build/expr/as_temp.rs
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,11 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
}
}

unpack!(block = this.into(temp_place, temp_lifetime, block, expr));
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);
}

block.and(temp)
}
Expand Down
90 changes: 27 additions & 63 deletions compiler/rustc_mir_build/src/build/expr/into.rs
Original file line number Diff line number Diff line change
@@ -1,35 +1,25 @@
//! 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::CanonicalUserTypeAnnotation;

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={:?}, scope={:?}, block={:?}, expr={:?})",
destination, scope, block, expr
);
debug!("into_expr(destination={:?}, block={:?}, expr={:?})", destination, block, expr);

// since we frequently have to reference `self` from within a
// closure, where `self` would be shadowed, it's easier to
Expand All @@ -41,14 +31,6 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
let expr_is_block_or_scope =
matches!(expr.kind, ExprKind::Block { .. } | ExprKind::Scope { .. });

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 @@ -58,15 +40,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, scope, block, value)
this.into(destination, block, value)
})
})
}
ExprKind::Block { body: ast_block } => {
this.ast_block(destination, scope, block, ast_block, source_info)
this.ast_block(destination, block, ast_block, source_info)
}
ExprKind::Match { scrutinee, arms } => {
this.match_expr(destination, scope, expr_span, block, scrutinee, arms)
this.match_expr(destination, expr_span, block, scrutinee, arms)
}
ExprKind::If { cond, then, else_opt } => {
let place = unpack!(
Expand All @@ -79,9 +61,9 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
let term = TerminatorKind::if_(this.hir.tcx(), operand, then_block, else_block);
this.cfg.terminate(block, source_info, term);

unpack!(then_block = this.into(destination, scope, then_block, then));
unpack!(then_block = this.into(destination, then_block, then));
else_block = if let Some(else_opt) = else_opt {
unpack!(this.into(destination, None, else_block, else_opt))
unpack!(this.into(destination, else_block, else_opt))
} else {
// Body of the `if` expression without an `else` clause must return `()`, thus
// we implicitly generate a `else {}` if it is not specified.
Expand Down Expand Up @@ -117,7 +99,6 @@ 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 @@ -193,35 +174,26 @@ 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,
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
},
)
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
})
}
ExprKind::Call { ty: _, fun, args, from_hir_call, fn_span } => {
let fun = unpack!(block = this.as_local_operand(block, fun));
Expand Down Expand Up @@ -256,10 +228,9 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
},
);
this.diverge_from(block);
schedule_drop(this);
success.unit()
}
ExprKind::Use { source } => this.into(destination, scope, block, source),
ExprKind::Use { source } => this.into(destination, 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 @@ -341,14 +312,12 @@ 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 @@ -445,7 +414,6 @@ 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 @@ -463,22 +431,19 @@ 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, Some(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 @@ -510,7 +475,6 @@ 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: 0 additions & 2 deletions compiler/rustc_mir_build/src/build/expr/stmt.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ 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 @@ -47,7 +46,6 @@ 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