Skip to content

Commit

Permalink
Rollup merge of #71824 - ecstatic-morse:const-check-post-drop-elab, r…
Browse files Browse the repository at this point in the history
…=oli-obk

Check for live drops in constants after drop elaboration

Resolves #66753.

This PR splits the MIR "optimization" pass series in two and introduces a query–`mir_drops_elaborated_and_const_checked`–that holds the result of the `post_borrowck_cleanup` analyses and checks for live drops. This query is invoked in `rustc_interface` for all items requiring const-checking, which means we now do `post_borrowck_cleanup` for items even if they are unused in the crate.

As a result, we are now more precise about when drops are live. This is because drop elaboration can e.g. eliminate drops of a local when all its fields are moved from. This does not mean we are doing value-based analysis on move paths, however; Storing a `Some(CustomDropImpl)` into a field of a local will still set the qualifs for that entire local.

r? @oli-obk
  • Loading branch information
RalfJung authored Jun 15, 2020
2 parents 4fb54ed + 2dcf7db commit 5c61a8d
Show file tree
Hide file tree
Showing 16 changed files with 264 additions and 51 deletions.
12 changes: 6 additions & 6 deletions src/librustc_error_codes/error_codes/E0493.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
A type with a `Drop` implementation was destructured when trying to initialize
a static item.
A value with a custom `Drop` implementation may be dropped during const-eval.

Erroneous code example:

Expand All @@ -16,13 +15,14 @@ struct Foo {
field1: DropType,
}
static FOO: Foo = Foo { ..Foo { field1: DropType::A } }; // error!
static FOO: Foo = Foo { field1: (DropType::A, DropType::A).1 }; // error!
```

The problem here is that if the given type or one of its fields implements the
`Drop` trait, this `Drop` implementation cannot be called during the static
type initialization which might cause a memory leak. To prevent this issue,
you need to instantiate all the static type's fields by hand.
`Drop` trait, this `Drop` implementation cannot be called within a const
context since it may run arbitrary, non-const-checked code. To prevent this
issue, ensure all values with custom a custom `Drop` implementation escape the
initializer.

```
enum DropType {
Expand Down
3 changes: 3 additions & 0 deletions src/librustc_feature/active.rs
Original file line number Diff line number Diff line change
Expand Up @@ -577,6 +577,9 @@ declare_features! (
/// Allows `extern "avr-interrupt" fn()` and `extern "avr-non-blocking-interrupt" fn()`.
(active, abi_avr_interrupt, "1.45.0", Some(69664), None),

/// Be more precise when looking for live drops in a const context.
(active, const_precise_live_drops, "1.46.0", Some(73255), None),

// -------------------------------------------------------------------------
// feature-group-end: actual feature gates
// -------------------------------------------------------------------------
Expand Down
6 changes: 5 additions & 1 deletion src/librustc_interface/passes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -847,7 +847,11 @@ fn analysis(tcx: TyCtxt<'_>, cnum: CrateNum) -> Result<()> {

sess.time("MIR_effect_checking", || {
for def_id in tcx.body_owners() {
mir::transform::check_unsafety::check_unsafety(tcx, def_id)
mir::transform::check_unsafety::check_unsafety(tcx, def_id);

if tcx.hir().body_const_context(def_id).is_some() {
tcx.ensure().mir_drops_elaborated_and_const_checked(def_id);
}
}
});

Expand Down
3 changes: 2 additions & 1 deletion src/librustc_middle/mir/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,8 @@ pub enum MirPhase {
Build = 0,
Const = 1,
Validated = 2,
Optimized = 3,
DropElab = 3,
Optimized = 4,
}

impl MirPhase {
Expand Down
6 changes: 6 additions & 0 deletions src/librustc_middle/query/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,12 @@ rustc_queries! {
no_hash
}

query mir_drops_elaborated_and_const_checked(key: LocalDefId) -> Steal<mir::Body<'tcx>> {
storage(ArenaCacheSelector<'tcx>)
no_hash
desc { |tcx| "elaborating drops for `{}`", tcx.def_path_str(key.to_def_id()) }
}

query mir_validated(key: LocalDefId) ->
(
Steal<mir::Body<'tcx>>,
Expand Down
1 change: 1 addition & 0 deletions src/librustc_mir/transform/check_consts/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ use rustc_middle::ty::{self, TyCtxt};
pub use self::qualifs::Qualif;

mod ops;
pub mod post_drop_elaboration;
pub mod qualifs;
mod resolver;
pub mod validation;
Expand Down
16 changes: 16 additions & 0 deletions src/librustc_mir/transform/check_consts/ops.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,22 @@ use rustc_span::{Span, Symbol};

use super::ConstCx;

/// Emits an error if `op` is not allowed in the given const context.
pub fn non_const<O: NonConstOp>(ccx: &ConstCx<'_, '_>, op: O, span: Span) {
debug!("illegal_op: op={:?}", op);

if op.is_allowed_in_item(ccx) {
return;
}

if ccx.tcx.sess.opts.debugging_opts.unleash_the_miri_inside_of_you {
ccx.tcx.sess.miri_unleashed_feature(span, O::feature_gate());
return;
}

op.emit_error(ccx, span);
}

/// An operation that is not *always* allowed in a const context.
pub trait NonConstOp: std::fmt::Debug {
/// Returns the `Symbol` corresponding to the feature gate that would enable this operation,
Expand Down
119 changes: 119 additions & 0 deletions src/librustc_mir/transform/check_consts/post_drop_elaboration.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,119 @@
use rustc_hir::def_id::LocalDefId;
use rustc_middle::mir::visit::Visitor;
use rustc_middle::mir::{self, BasicBlock, Location};
use rustc_middle::ty::TyCtxt;
use rustc_span::Span;

use super::ops;
use super::qualifs::{NeedsDrop, Qualif};
use super::validation::Qualifs;
use super::ConstCx;

/// Returns `true` if we should use the more precise live drop checker that runs after drop
/// elaboration.
pub fn checking_enabled(tcx: TyCtxt<'tcx>) -> bool {
tcx.features().const_precise_live_drops
}

/// Look for live drops in a const context.
///
/// This is separate from the rest of the const checking logic because it must run after drop
/// elaboration.
pub fn check_live_drops(tcx: TyCtxt<'tcx>, def_id: LocalDefId, body: &mir::Body<'tcx>) {
let const_kind = tcx.hir().body_const_context(def_id);
if const_kind.is_none() {
return;
}

if !checking_enabled(tcx) {
return;
}

let ccx = ConstCx {
body,
tcx,
def_id: def_id.to_def_id(),
const_kind,
param_env: tcx.param_env(def_id),
};

let mut visitor = CheckLiveDrops { ccx: &ccx, qualifs: Qualifs::default() };

visitor.visit_body(body);
}

struct CheckLiveDrops<'mir, 'tcx> {
ccx: &'mir ConstCx<'mir, 'tcx>,
qualifs: Qualifs<'mir, 'tcx>,
}

// So we can access `body` and `tcx`.
impl std::ops::Deref for CheckLiveDrops<'mir, 'tcx> {
type Target = ConstCx<'mir, 'tcx>;

fn deref(&self) -> &Self::Target {
&self.ccx
}
}

impl CheckLiveDrops<'mir, 'tcx> {
fn check_live_drop(&self, span: Span) {
ops::non_const(self.ccx, ops::LiveDrop, span);
}
}

impl Visitor<'tcx> for CheckLiveDrops<'mir, 'tcx> {
fn visit_basic_block_data(&mut self, bb: BasicBlock, block: &mir::BasicBlockData<'tcx>) {
trace!("visit_basic_block_data: bb={:?} is_cleanup={:?}", bb, block.is_cleanup);

// Ignore drop terminators in cleanup blocks.
if block.is_cleanup {
return;
}

self.super_basic_block_data(bb, block);
}

fn visit_terminator(&mut self, terminator: &mir::Terminator<'tcx>, location: Location) {
trace!("visit_terminator: terminator={:?} location={:?}", terminator, location);

match &terminator.kind {
mir::TerminatorKind::Drop { location: dropped_place, .. } => {
let dropped_ty = dropped_place.ty(self.body, self.tcx).ty;
if !NeedsDrop::in_any_value_of_ty(self.ccx, dropped_ty) {
return;
}

if dropped_place.is_indirect() {
self.check_live_drop(terminator.source_info.span);
return;
}

if self.qualifs.needs_drop(self.ccx, dropped_place.local, location) {
// Use the span where the dropped local was declared for the error.
let span = self.body.local_decls[dropped_place.local].source_info.span;
self.check_live_drop(span);
}
}

mir::TerminatorKind::DropAndReplace { .. } => span_bug!(
terminator.source_info.span,
"`DropAndReplace` should be removed by drop elaboration",
),

mir::TerminatorKind::Abort
| mir::TerminatorKind::Call { .. }
| mir::TerminatorKind::Assert { .. }
| mir::TerminatorKind::FalseEdge { .. }
| mir::TerminatorKind::FalseUnwind { .. }
| mir::TerminatorKind::GeneratorDrop
| mir::TerminatorKind::Goto { .. }
| mir::TerminatorKind::InlineAsm { .. }
| mir::TerminatorKind::Resume
| mir::TerminatorKind::Return
| mir::TerminatorKind::SwitchInt { .. }
| mir::TerminatorKind::Unreachable
| mir::TerminatorKind::Yield { .. } => {}
}
}
}
41 changes: 16 additions & 25 deletions src/librustc_mir/transform/check_consts/validation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ pub struct Qualifs<'mir, 'tcx> {
}

impl Qualifs<'mir, 'tcx> {
fn indirectly_mutable(
pub fn indirectly_mutable(
&mut self,
ccx: &'mir ConstCx<'mir, 'tcx>,
local: Local,
Expand Down Expand Up @@ -68,7 +68,7 @@ impl Qualifs<'mir, 'tcx> {
/// Returns `true` if `local` is `NeedsDrop` at the given `Location`.
///
/// Only updates the cursor if absolutely necessary
fn needs_drop(
pub fn needs_drop(
&mut self,
ccx: &'mir ConstCx<'mir, 'tcx>,
local: Local,
Expand All @@ -95,7 +95,7 @@ impl Qualifs<'mir, 'tcx> {
/// Returns `true` if `local` is `HasMutInterior` at the given `Location`.
///
/// Only updates the cursor if absolutely necessary.
fn has_mut_interior(
pub fn has_mut_interior(
&mut self,
ccx: &'mir ConstCx<'mir, 'tcx>,
local: Local,
Expand Down Expand Up @@ -232,30 +232,15 @@ impl Validator<'mir, 'tcx> {
self.qualifs.in_return_place(self.ccx)
}

/// Emits an error at the given `span` if an expression cannot be evaluated in the current
/// context.
pub fn check_op_spanned<O>(&mut self, op: O, span: Span)
where
O: NonConstOp,
{
debug!("check_op: op={:?}", op);

if op.is_allowed_in_item(self) {
return;
}

if self.tcx.sess.opts.debugging_opts.unleash_the_miri_inside_of_you {
self.tcx.sess.miri_unleashed_feature(span, O::feature_gate());
return;
}

op.emit_error(self, span);
}

/// Emits an error if an expression cannot be evaluated in the current context.
pub fn check_op(&mut self, op: impl NonConstOp) {
let span = self.span;
self.check_op_spanned(op, span)
ops::non_const(self.ccx, op, self.span);
}

/// Emits an error at the given `span` if an expression cannot be evaluated in the current
/// context.
pub fn check_op_spanned(&mut self, op: impl NonConstOp, span: Span) {
ops::non_const(self.ccx, op, span);
}

fn check_static(&mut self, def_id: DefId, span: Span) {
Expand Down Expand Up @@ -577,6 +562,12 @@ impl Visitor<'tcx> for Validator<'mir, 'tcx> {
// projections that cannot be `NeedsDrop`.
TerminatorKind::Drop { location: dropped_place, .. }
| TerminatorKind::DropAndReplace { location: dropped_place, .. } => {
// If we are checking live drops after drop-elaboration, don't emit duplicate
// errors here.
if super::post_drop_elaboration::checking_enabled(self.tcx) {
return;
}

let mut err_span = self.span;

// Check to see if the type of this place can ever have a drop impl. If not, this
Expand Down
Loading

0 comments on commit 5c61a8d

Please sign in to comment.