From 6c55fb8227423c9d22585711efc7666675a898f2 Mon Sep 17 00:00:00 2001 From: Eduard-Mihai Burtescu Date: Wed, 21 Aug 2019 23:31:58 +0300 Subject: [PATCH 1/2] rustc_mir: double-check const-promotion candidates for sanity. --- src/librustc_mir/transform/promote_consts.rs | 610 ++++++++++++++++++- src/librustc_mir/transform/qualify_consts.rs | 117 +++- 2 files changed, 694 insertions(+), 33 deletions(-) diff --git a/src/librustc_mir/transform/promote_consts.rs b/src/librustc_mir/transform/promote_consts.rs index 7a9c489fa79..4d411ad4f53 100644 --- a/src/librustc_mir/transform/promote_consts.rs +++ b/src/librustc_mir/transform/promote_consts.rs @@ -12,15 +12,22 @@ //! initialization and can otherwise silence errors, if //! move analysis runs after promotion on broken MIR. +use rustc::hir; use rustc::hir::def_id::DefId; use rustc::mir::*; +use rustc::mir::interpret::ConstValue; use rustc::mir::visit::{PlaceContext, MutatingUseContext, MutVisitor, Visitor}; use rustc::mir::traversal::ReversePostorder; +use rustc::ty::{self, List, TyCtxt}; use rustc::ty::subst::InternalSubsts; -use rustc::ty::{List, TyCtxt}; -use syntax_pos::Span; +use rustc::ty::cast::CastTy; +use syntax::ast::LitKind; +use syntax::symbol::sym; +use syntax_pos::{Span, DUMMY_SP}; +use rustc_index::bit_set::BitSet; use rustc_index::vec::{IndexVec, Idx}; +use rustc_target::spec::abi::Abi; use std::{iter, mem, usize}; @@ -57,7 +64,7 @@ impl TempState { /// A "root candidate" for promotion, which will become the /// returned value in a promoted MIR, unless it's a subset /// of a larger candidate. -#[derive(Debug)] +#[derive(Copy, Clone, PartialEq, Eq, Debug)] pub enum Candidate { /// Borrow of a constant temporary. Ref(Location), @@ -73,13 +80,28 @@ pub enum Candidate { Argument { bb: BasicBlock, index: usize }, } -struct TempCollector<'tcx> { - temps: IndexVec, - span: Span, - body: &'tcx Body<'tcx>, +fn args_required_const(tcx: TyCtxt<'_>, def_id: DefId) -> Option> { + let attrs = tcx.get_attrs(def_id); + let attr = attrs.iter().find(|a| a.check_name(sym::rustc_args_required_const))?; + let mut ret = vec![]; + for meta in attr.meta_item_list()? { + match meta.literal()?.kind { + LitKind::Int(a, _) => { ret.push(a as usize); } + _ => return None, + } + } + Some(ret) } -impl<'tcx> Visitor<'tcx> for TempCollector<'tcx> { +struct Collector<'a, 'tcx> { + tcx: TyCtxt<'tcx>, + body: &'a Body<'tcx>, + temps: IndexVec, + candidates: Vec, + span: Span, +} + +impl<'tcx> Visitor<'tcx> for Collector<'_, 'tcx> { fn visit_local(&mut self, &index: &Local, context: PlaceContext, @@ -134,22 +156,576 @@ impl<'tcx> Visitor<'tcx> for TempCollector<'tcx> { *temp = TempState::Unpromotable; } + fn visit_rvalue(&mut self, rvalue: &Rvalue<'tcx>, location: Location) { + self.super_rvalue(rvalue, location); + + match *rvalue { + Rvalue::Ref(..) => { + self.candidates.push(Candidate::Ref(location)); + } + Rvalue::Repeat(..) if self.tcx.features().const_in_array_repeat_expressions => { + // FIXME(#49147) only promote the element when it isn't `Copy` + // (so that code that can copy it at runtime is unaffected). + self.candidates.push(Candidate::Repeat(location)); + } + _ => {} + } + } + + fn visit_terminator_kind(&mut self, + kind: &TerminatorKind<'tcx>, + location: Location) { + self.super_terminator_kind(kind, location); + + if let TerminatorKind::Call { ref func, .. } = *kind { + if let ty::FnDef(def_id, _) = func.ty(self.body, self.tcx).kind { + let fn_sig = self.tcx.fn_sig(def_id); + if let Abi::RustIntrinsic | Abi::PlatformIntrinsic = fn_sig.abi() { + let name = self.tcx.item_name(def_id); + // FIXME(eddyb) use `#[rustc_args_required_const(2)]` for shuffles. + if name.as_str().starts_with("simd_shuffle") { + self.candidates.push(Candidate::Argument { + bb: location.block, + index: 2, + }); + } + } + + if let Some(constant_args) = args_required_const(self.tcx, def_id) { + for index in constant_args { + self.candidates.push(Candidate::Argument { bb: location.block, index }); + } + } + } + } + } + fn visit_source_info(&mut self, source_info: &SourceInfo) { self.span = source_info.span; } } -pub fn collect_temps(body: &Body<'_>, - rpo: &mut ReversePostorder<'_, '_>) -> IndexVec { - let mut collector = TempCollector { - temps: IndexVec::from_elem(TempState::Undefined, &body.local_decls), - span: body.span, +pub fn collect_temps_and_candidates( + tcx: TyCtxt<'tcx>, + body: &Body<'tcx>, + rpo: &mut ReversePostorder<'_, 'tcx>, +) -> (IndexVec, Vec) { + let mut collector = Collector { + tcx, body, + temps: IndexVec::from_elem(TempState::Undefined, &body.local_decls), + candidates: vec![], + span: body.span, }; for (bb, data) in rpo { collector.visit_basic_block_data(bb, data); } - collector.temps + (collector.temps, collector.candidates) +} + +struct Validator<'a, 'tcx> { + tcx: TyCtxt<'tcx>, + param_env: ty::ParamEnv<'tcx>, + body: &'a Body<'tcx>, + is_static: bool, + is_static_mut: bool, + is_non_const_fn: bool, + temps: &'a IndexVec, + // FIXME(eddyb) compute these 2 on the fly. + has_mut_interior: &'a BitSet, + needs_drop: &'a BitSet, + + /// Explicit promotion happens e.g. for constant arguments declared via + /// `rustc_args_required_const`. + /// Implicit promotion has almost the same rules, except that disallows `const fn` + /// except for those marked `#[rustc_promotable]`. This is to avoid changing + /// a legitimate run-time operation into a failing compile-time operation + /// e.g. due to addresses being compared inside the function. + explicit: bool, +} + +struct Unpromotable; + +impl<'tcx> Validator<'_, 'tcx> { + fn is_const_panic_fn(&self, def_id: DefId) -> bool { + Some(def_id) == self.tcx.lang_items().panic_fn() || + Some(def_id) == self.tcx.lang_items().begin_panic_fn() + } + + fn validate_candidate(&self, candidate: Candidate) -> Result<(), Unpromotable> { + match candidate { + Candidate::Ref(loc) => { + assert!(!self.explicit); + + let statement = &self.body[loc.block].statements[loc.statement_index]; + match &statement.kind { + StatementKind::Assign(box(_, Rvalue::Ref(_, kind, place))) => { + match kind { + BorrowKind::Shared | BorrowKind::Mut { .. } => {} + + // FIXME(eddyb) these aren't promoted here but *could* + // be promoted as part of a larger value because + // `validate_rvalue` doesn't check them, need to + // figure out what is the intended behavior. + BorrowKind::Shallow | BorrowKind::Unique => return Err(Unpromotable), + } + + // We can only promote interior borrows of promotable temps (non-temps + // don't get promoted anyway). + let base = match place.base { + PlaceBase::Local(local) => local, + _ => return Err(Unpromotable), + }; + if place.projection.contains(&ProjectionElem::Deref) { + return Err(Unpromotable); + } + + // FIXME(eddyb) compute this on the fly. + let mut has_mut_interior = self.has_mut_interior.contains(base); + // HACK(eddyb) this should compute the same thing as + // `::in_projection` from + // `qualify_consts` but without recursion. + if has_mut_interior { + // This allows borrowing fields which don't have + // `HasMutInterior`, from a type that does, e.g.: + // `let _: &'static _ = &(Cell::new(1), 2).1;` + let mut place_projection = &place.projection[..]; + // FIXME(eddyb) use a forward loop instead of a reverse one. + while let [proj_base @ .., elem] = place_projection { + // FIXME(eddyb) this is probably excessive, with + // the exception of `union` member accesses. + let ty = + Place::ty_from(&place.base, proj_base, self.body, self.tcx) + .projection_ty(self.tcx, elem) + .ty; + if ty.is_freeze(self.tcx, self.param_env, DUMMY_SP) { + has_mut_interior = false; + break; + } + + place_projection = proj_base; + } + } + + // FIXME(eddyb) this duplicates part of `validate_rvalue`. + if has_mut_interior { + return Err(Unpromotable); + } + // FIXME(eddyb) compute this on the fly. + if self.needs_drop.contains(base) { + return Err(Unpromotable); + } + if let BorrowKind::Mut { .. } = kind { + let ty = place.ty(self.body, self.tcx).ty; + + // In theory, any zero-sized value could be borrowed + // mutably without consequences. However, only &mut [] + // is allowed right now, and only in functions. + if self.is_static_mut { + // Inside a `static mut`, &mut [...] is also allowed. + match ty.kind { + ty::Array(..) | ty::Slice(_) => {} + _ => return Err(Unpromotable), + } + } else if let ty::Array(_, len) = ty.kind { + // FIXME(eddyb) the `self.is_non_const_fn` condition + // seems unnecessary, given that this is merely a ZST. + match len.try_eval_usize(self.tcx, self.param_env) { + Some(0) if self.is_non_const_fn => {}, + _ => return Err(Unpromotable), + } + } else { + return Err(Unpromotable); + } + } + + self.validate_local(base) + } + _ => bug!() + } + } + Candidate::Repeat(loc) => { + assert!(!self.explicit); + + let statement = &self.body[loc.block].statements[loc.statement_index]; + match &statement.kind { + StatementKind::Assign(box(_, Rvalue::Repeat(ref operand, _))) => { + if !self.tcx.features().const_in_array_repeat_expressions { + return Err(Unpromotable); + } + + self.validate_operand(operand) + } + _ => bug!() + } + }, + Candidate::Argument { bb, index } => { + assert!(self.explicit); + + let terminator = self.body[bb].terminator(); + match &terminator.kind { + TerminatorKind::Call { args, .. } => { + self.validate_operand(&args[index]) + } + _ => bug!() + } + } + } + } + + // FIXME(eddyb) maybe cache this? + fn validate_local(&self, local: Local) -> Result<(), Unpromotable> { + if let TempState::Defined { location: loc, .. } = self.temps[local] { + let num_stmts = self.body[loc.block].statements.len(); + + if loc.statement_index < num_stmts { + let statement = &self.body[loc.block].statements[loc.statement_index]; + match &statement.kind { + StatementKind::Assign(box(_, rhs)) => self.validate_rvalue(rhs), + _ => { + span_bug!(statement.source_info.span, "{:?} is not an assignment", + statement); + } + } + } else { + let terminator = self.body[loc.block].terminator(); + match &terminator.kind { + TerminatorKind::Call { func, args, .. } => self.validate_call(func, args), + kind => { + span_bug!(terminator.source_info.span, "{:?} not promotable", kind); + } + } + } + } else { + Err(Unpromotable) + } + } + + fn validate_place(&self, place: PlaceRef<'_, 'tcx>) -> Result<(), Unpromotable> { + match place { + PlaceRef { + base: PlaceBase::Local(local), + projection: [], + } => self.validate_local(*local), + PlaceRef { + base: PlaceBase::Static(box Static { + kind: StaticKind::Promoted { .. }, + .. + }), + projection: [], + } => bug!("qualifying already promoted MIR"), + PlaceRef { + base: PlaceBase::Static(box Static { + kind: StaticKind::Static, + def_id, + .. + }), + projection: [], + } => { + // Only allow statics (not consts) to refer to other statics. + // FIXME(eddyb) does this matter at all for promotion? + let allowed = self.is_static || self.is_static_mut; + if !allowed { + return Err(Unpromotable); + } + + let is_thread_local = self.tcx.has_attr(*def_id, sym::thread_local); + if is_thread_local { + return Err(Unpromotable); + } + + Ok(()) + } + PlaceRef { + base: _, + projection: [proj_base @ .., elem], + } => { + match *elem { + ProjectionElem::Deref | + ProjectionElem::Downcast(..) => return Err(Unpromotable), + + ProjectionElem::ConstantIndex {..} | + ProjectionElem::Subslice {..} => {} + + ProjectionElem::Index(local) => { + self.validate_local(local)?; + } + + ProjectionElem::Field(..) => { + if self.is_non_const_fn { + let base_ty = + Place::ty_from(place.base, proj_base, self.body, self.tcx).ty; + if let Some(def) = base_ty.ty_adt_def() { + // No promotion of union field accesses. + if def.is_union() { + return Err(Unpromotable); + } + } + } + } + } + + self.validate_place(PlaceRef { + base: place.base, + projection: proj_base, + }) + } + } + } + + fn validate_operand(&self, operand: &Operand<'tcx>) -> Result<(), Unpromotable> { + match operand { + Operand::Copy(place) | + Operand::Move(place) => self.validate_place(place.as_ref()), + + Operand::Constant(constant) => { + if let ConstValue::Unevaluated(def_id, _) = constant.literal.val { + if self.tcx.trait_of_item(def_id).is_some() { + // Don't peek inside trait associated constants. + // (see below what we do for other consts, for now) + } else { + // HACK(eddyb) ensure that errors propagate correctly. + // FIXME(eddyb) remove this once the old promotion logic + // is gone - we can always promote constants even if they + // fail to pass const-checking, as compilation would've + // errored independently and promotion can't change that. + let (bits, _) = self.tcx.at(constant.span).mir_const_qualif(def_id); + if bits == super::qualify_consts::QUALIF_ERROR_BIT { + self.tcx.sess.delay_span_bug( + constant.span, + "promote_consts: MIR had errors", + ); + return Err(Unpromotable); + } + } + } + + Ok(()) + } + } + } + + fn validate_rvalue(&self, rvalue: &Rvalue<'tcx>) -> Result<(), Unpromotable> { + match *rvalue { + Rvalue::Cast(CastKind::Misc, ref operand, cast_ty) if self.is_non_const_fn => { + let operand_ty = operand.ty(self.body, self.tcx); + let cast_in = CastTy::from_ty(operand_ty).expect("bad input type for cast"); + let cast_out = CastTy::from_ty(cast_ty).expect("bad output type for cast"); + match (cast_in, cast_out) { + (CastTy::Ptr(_), CastTy::Int(_)) | + (CastTy::FnPtr, CastTy::Int(_)) => { + // in normal functions, mark such casts as not promotable + return Err(Unpromotable); + } + _ => {} + } + } + + Rvalue::BinaryOp(op, ref lhs, _) if self.is_non_const_fn => { + if let ty::RawPtr(_) | ty::FnPtr(..) = lhs.ty(self.body, self.tcx).kind { + assert!(op == BinOp::Eq || op == BinOp::Ne || + op == BinOp::Le || op == BinOp::Lt || + op == BinOp::Ge || op == BinOp::Gt || + op == BinOp::Offset); + + // raw pointer operations are not allowed inside promoteds + return Err(Unpromotable); + } + } + + Rvalue::NullaryOp(NullOp::Box, _) => return Err(Unpromotable), + + _ => {} + } + + match rvalue { + Rvalue::NullaryOp(..) => Ok(()), + + Rvalue::Discriminant(place) | + Rvalue::Len(place) => self.validate_place(place.as_ref()), + + Rvalue::Use(operand) | + Rvalue::Repeat(operand, _) | + Rvalue::UnaryOp(_, operand) | + Rvalue::Cast(_, operand, _) => self.validate_operand(operand), + + Rvalue::BinaryOp(_, lhs, rhs) | + Rvalue::CheckedBinaryOp(_, lhs, rhs) => { + self.validate_operand(lhs)?; + self.validate_operand(rhs) + } + + Rvalue::Ref(_, kind, place) => { + if let BorrowKind::Mut { .. } = kind { + let ty = place.ty(self.body, self.tcx).ty; + + // In theory, any zero-sized value could be borrowed + // mutably without consequences. However, only &mut [] + // is allowed right now, and only in functions. + if self.is_static_mut { + // Inside a `static mut`, &mut [...] is also allowed. + match ty.kind { + ty::Array(..) | ty::Slice(_) => {} + _ => return Err(Unpromotable), + } + } else if let ty::Array(_, len) = ty.kind { + // FIXME(eddyb) the `self.is_non_const_fn` condition + // seems unnecessary, given that this is merely a ZST. + match len.try_eval_usize(self.tcx, self.param_env) { + Some(0) if self.is_non_const_fn => {}, + _ => return Err(Unpromotable), + } + } else { + return Err(Unpromotable); + } + } + + // Special-case reborrows to be more like a copy of the reference. + let mut place = place.as_ref(); + if let [proj_base @ .., ProjectionElem::Deref] = &place.projection { + let base_ty = + Place::ty_from(&place.base, proj_base, self.body, self.tcx).ty; + if let ty::Ref(..) = base_ty.kind { + place = PlaceRef { + base: &place.base, + projection: proj_base, + }; + } + } + + // HACK(eddyb) this should compute the same thing as + // `::in_projection` from + // `qualify_consts` but without recursion. + let mut has_mut_interior = match place.base { + PlaceBase::Local(local) => { + // FIXME(eddyb) compute this on the fly. + self.has_mut_interior.contains(*local) + } + PlaceBase::Static(_) => false, + }; + if has_mut_interior { + let mut place_projection = place.projection; + // FIXME(eddyb) use a forward loop instead of a reverse one. + while let [proj_base @ .., elem] = place_projection { + // FIXME(eddyb) this is probably excessive, with + // the exception of `union` member accesses. + let ty = Place::ty_from(place.base, proj_base, self.body, self.tcx) + .projection_ty(self.tcx, elem) + .ty; + if ty.is_freeze(self.tcx, self.param_env, DUMMY_SP) { + has_mut_interior = false; + break; + } + + place_projection = proj_base; + } + } + if has_mut_interior { + return Err(Unpromotable); + } + + self.validate_place(place) + } + + Rvalue::Aggregate(_, ref operands) => { + for o in operands { + self.validate_operand(o)?; + } + + Ok(()) + } + } + } + + fn validate_call( + &self, + callee: &Operand<'tcx>, + args: &[Operand<'tcx>], + ) -> Result<(), Unpromotable> { + let fn_ty = callee.ty(self.body, self.tcx); + + if !self.explicit && self.is_non_const_fn { + if let ty::FnDef(def_id, _) = fn_ty.kind { + // Never promote runtime `const fn` calls of + // functions without `#[rustc_promotable]`. + if !self.tcx.is_promotable_const_fn(def_id) { + return Err(Unpromotable); + } + } + } + + let is_const_fn = match fn_ty.kind { + ty::FnDef(def_id, _) => { + self.tcx.is_const_fn(def_id) || + self.tcx.is_unstable_const_fn(def_id).is_some() || + self.is_const_panic_fn(def_id) + } + _ => false, + }; + if !is_const_fn { + return Err(Unpromotable); + } + + self.validate_operand(callee)?; + for arg in args { + self.validate_operand(arg)?; + } + + Ok(()) + } +} + +pub fn validate_candidates( + tcx: TyCtxt<'tcx>, + body: &Body<'tcx>, + def_id: DefId, + temps: &IndexVec, + // FIXME(eddyb) compute these 2 on the fly. + has_mut_interior: &BitSet, + needs_drop: &BitSet, + candidates: &[Candidate], +) -> Vec { + let mut validator = Validator { + tcx, + param_env: tcx.param_env(def_id), + body, + is_static: false, + is_static_mut: false, + is_non_const_fn: false, + temps, + // FIXME(eddyb) compute these 2 on the fly. + has_mut_interior, + needs_drop, + + explicit: false, + }; + + // FIXME(eddyb) remove the distinctions that make this necessary. + let id = tcx.hir().as_local_hir_id(def_id).unwrap(); + match tcx.hir().body_owner_kind(id) { + hir::BodyOwnerKind::Closure => validator.is_non_const_fn = true, + hir::BodyOwnerKind::Fn => { + if !tcx.is_const_fn(def_id) { + validator.is_non_const_fn = true; + } + }, + hir::BodyOwnerKind::Static(hir::MutImmutable) => validator.is_static = true, + hir::BodyOwnerKind::Static(hir::MutMutable) => validator.is_static_mut = true, + _ => {} + } + + candidates.iter().copied().filter(|&candidate| { + validator.explicit = match candidate { + Candidate::Ref(_) | + Candidate::Repeat(_) => false, + Candidate::Argument { .. } => true, + }; + + // FIXME(eddyb) also emit the errors for shuffle indices + // and `#[rustc_args_required_const]` arguments here. + + validator.validate_candidate(candidate).is_ok() + }).collect() } struct Promoter<'a, 'tcx> { @@ -215,17 +791,17 @@ impl<'a, 'tcx> Promoter<'a, 'tcx> { self.temps[temp] = TempState::PromotedOut; } - let no_stmts = self.source[loc.block].statements.len(); + let num_stmts = self.source[loc.block].statements.len(); let new_temp = self.promoted.local_decls.push( LocalDecl::new_temp(self.source.local_decls[temp].ty, self.source.local_decls[temp].source_info.span)); debug!("promote({:?} @ {:?}/{:?}, {:?})", - temp, loc, no_stmts, self.keep_original); + temp, loc, num_stmts, self.keep_original); // First, take the Rvalue or Call out of the source MIR, // or duplicate it, depending on keep_original. - if loc.statement_index < no_stmts { + if loc.statement_index < num_stmts { let (mut rvalue, source_info) = { let statement = &mut self.source[loc.block].statements[loc.statement_index]; let rhs = match statement.kind { diff --git a/src/librustc_mir/transform/qualify_consts.rs b/src/librustc_mir/transform/qualify_consts.rs index 6aba91f4162..1c5c84c6923 100644 --- a/src/librustc_mir/transform/qualify_consts.rs +++ b/src/librustc_mir/transform/qualify_consts.rs @@ -206,6 +206,9 @@ trait Qualif { ProjectionElem::ConstantIndex { .. } | ProjectionElem::Downcast(..) => qualif, + // FIXME(eddyb) shouldn't this be masked *after* including the + // index local? Then again, it's `usize` which is neither + // `HasMutInterior` nor `NeedsDrop`. ProjectionElem::Index(local) => qualif || Self::in_local(cx, *local), } } else { @@ -442,6 +445,7 @@ impl Qualif for IsNotPromotable { StaticKind::Promoted(_, _) => unreachable!(), StaticKind::Static => { // Only allow statics (not consts) to refer to other statics. + // FIXME(eddyb) does this matter at all for promotion? let allowed = cx.mode == Mode::Static || cx.mode == Mode::StaticMut; !allowed || @@ -674,6 +678,7 @@ struct Checker<'a, 'tcx> { temp_promotion_state: IndexVec, promotion_candidates: Vec, + unchecked_promotion_candidates: Vec, /// If `true`, do not emit errors to the user, merely collect them in `errors`. suppress_errors: bool, @@ -703,7 +708,8 @@ impl<'a, 'tcx> Checker<'a, 'tcx> { fn new(tcx: TyCtxt<'tcx>, def_id: DefId, body: &'a Body<'tcx>, mode: Mode) -> Self { assert!(def_id.is_local()); let mut rpo = traversal::reverse_postorder(body); - let temps = promote_consts::collect_temps(body, &mut rpo); + let (temps, unchecked_promotion_candidates) = + promote_consts::collect_temps_and_candidates(tcx, body, &mut rpo); rpo.reset(); let param_env = tcx.param_env(def_id); @@ -742,6 +748,7 @@ impl<'a, 'tcx> Checker<'a, 'tcx> { rpo, temp_promotion_state: temps, promotion_candidates: vec![], + unchecked_promotion_candidates, errors: vec![], suppress_errors: false, } @@ -828,6 +835,10 @@ impl<'a, 'tcx> Checker<'a, 'tcx> { } else if let BorrowKind::Mut { .. } | BorrowKind::Shared = kind { // Don't promote BorrowKind::Shallow borrows, as they don't // reach codegen. + // FIXME(eddyb) the two other kinds of borrow (`Shallow` and `Unique`) + // aren't promoted here but *could* be promoted as part of a larger + // value because `IsNotPromotable` isn't being set for them, + // need to figure out what is the intended behavior. // We might have a candidate for promotion. let candidate = Candidate::Ref(location); @@ -967,6 +978,7 @@ impl<'a, 'tcx> Checker<'a, 'tcx> { let mut seen_blocks = BitSet::new_empty(body.basic_blocks().len()); let mut bb = START_BLOCK; + let mut has_controlflow_error = false; loop { seen_blocks.insert(bb.index()); @@ -1007,6 +1019,7 @@ impl<'a, 'tcx> Checker<'a, 'tcx> { bb = target; } _ => { + has_controlflow_error = true; self.not_const(ops::Loop); validator.check_op(ops::Loop); break; @@ -1036,9 +1049,28 @@ impl<'a, 'tcx> Checker<'a, 'tcx> { // Collect all the temps we need to promote. let mut promoted_temps = BitSet::new_empty(self.temp_promotion_state.len()); - debug!("qualify_const: promotion_candidates={:?}", self.promotion_candidates); - for candidate in &self.promotion_candidates { - match *candidate { + // HACK(eddyb) don't try to validate promotion candidates if any + // parts of the control-flow graph were skipped due to an error. + let promotion_candidates = if has_controlflow_error { + let unleash_miri = self + .tcx + .sess + .opts + .debugging_opts + .unleash_the_miri_inside_of_you; + if !unleash_miri { + self.tcx.sess.delay_span_bug( + body.span, + "check_const: expected control-flow error(s)", + ); + } + self.promotion_candidates.clone() + } else { + self.valid_promotion_candidates() + }; + debug!("qualify_const: promotion_candidates={:?}", promotion_candidates); + for candidate in promotion_candidates { + match candidate { Candidate::Repeat(Location { block: bb, statement_index: stmt_idx }) => { if let StatementKind::Assign(box(_, Rvalue::Repeat( Operand::Move(place), @@ -1075,6 +1107,51 @@ impl<'a, 'tcx> Checker<'a, 'tcx> { (qualifs.encode_to_bits(), self.tcx.arena.alloc(promoted_temps)) } + + /// Get the subset of `unchecked_promotion_candidates` that are eligible + /// for promotion. + // FIXME(eddyb) replace the old candidate gathering with this. + fn valid_promotion_candidates(&self) -> Vec { + // Sanity-check the promotion candidates. + let candidates = promote_consts::validate_candidates( + self.tcx, + self.body, + self.def_id, + &self.temp_promotion_state, + &self.per_local.0[HasMutInterior::IDX], + &self.per_local.0[NeedsDrop::IDX], + &self.unchecked_promotion_candidates, + ); + + if candidates != self.promotion_candidates { + let report = |msg, candidate| { + let span = match candidate { + Candidate::Ref(loc) | + Candidate::Repeat(loc) => self.body.source_info(loc).span, + Candidate::Argument { bb, .. } => { + self.body[bb].terminator().source_info.span + } + }; + self.tcx.sess.span_err(span, &format!("{}: {:?}", msg, candidate)); + }; + + for &c in &self.promotion_candidates { + if !candidates.contains(&c) { + report("invalidated old candidate", c); + } + } + + for &c in &candidates { + if !self.promotion_candidates.contains(&c) { + report("extra new candidate", c); + } + } + + bug!("promotion candidate validation mismatches (see above)"); + } + + candidates + } } impl<'a, 'tcx> Visitor<'tcx> for Checker<'a, 'tcx> { @@ -1644,6 +1721,10 @@ pub fn provide(providers: &mut Providers<'_>) { }; } +// FIXME(eddyb) this is only left around for the validation logic +// in `promote_consts`, see the comment in `validate_operand`. +pub(super) const QUALIF_ERROR_BIT: u8 = 1 << IsNotPromotable::IDX; + fn mir_const_qualif(tcx: TyCtxt<'_>, def_id: DefId) -> (u8, &BitSet) { // N.B., this `borrow()` is guaranteed to be valid (i.e., the value // cannot yet be stolen), because `mir_validated()`, which steals @@ -1653,7 +1734,7 @@ fn mir_const_qualif(tcx: TyCtxt<'_>, def_id: DefId) -> (u8, &BitSet) { if body.return_ty().references_error() { tcx.sess.delay_span_bug(body.span, "mir_const_qualif: MIR had errors"); - return (1 << IsNotPromotable::IDX, tcx.arena.alloc(BitSet::new_empty(0))); + return (QUALIF_ERROR_BIT, tcx.arena.alloc(BitSet::new_empty(0))); } Checker::new(tcx, def_id, body, Mode::Const).check_const() @@ -1695,29 +1776,33 @@ impl<'tcx> MirPass<'tcx> for QualifyAndPromoteConstants<'tcx> { let (temps, candidates) = { let mut checker = Checker::new(tcx, def_id, body, mode); if let Mode::ConstFn = mode { - if tcx.sess.opts.debugging_opts.unleash_the_miri_inside_of_you { - checker.check_const(); - } else if tcx.is_min_const_fn(def_id) { + let use_min_const_fn_checks = + !tcx.sess.opts.debugging_opts.unleash_the_miri_inside_of_you && + tcx.is_min_const_fn(def_id); + if use_min_const_fn_checks { // Enforce `min_const_fn` for stable `const fn`s. use super::qualify_min_const_fn::is_min_const_fn; if let Err((span, err)) = is_min_const_fn(tcx, def_id, body) { error_min_const_fn_violation(tcx, span, err); - } else { - // this should not produce any errors, but better safe than sorry - // FIXME(#53819) - checker.check_const(); + return; } - } else { - // Enforce a constant-like CFG for `const fn`. - checker.check_const(); + + // `check_const` should not produce any errors, but better safe than sorry + // FIXME(#53819) + // NOTE(eddyb) `check_const` is actually needed for promotion inside + // `min_const_fn` functions. } + + // Enforce a constant-like CFG for `const fn`. + checker.check_const(); } else { while let Some((bb, data)) = checker.rpo.next() { checker.visit_basic_block_data(bb, data); } } - (checker.temp_promotion_state, checker.promotion_candidates) + let promotion_candidates = checker.valid_promotion_candidates(); + (checker.temp_promotion_state, promotion_candidates) }; // Do the actual promotion, now that we know what's viable. From f2c8628920b73f546b6d1a64babd7e8a481a9b9f Mon Sep 17 00:00:00 2001 From: Eduard-Mihai Burtescu Date: Mon, 21 Oct 2019 21:18:12 +0300 Subject: [PATCH 2/2] rustc_mir: use the new validator's Qualif in promotion. --- .../transform/check_consts/mod.rs | 25 ++++++- .../transform/check_consts/qualifs.rs | 46 +++++++----- .../transform/check_consts/resolver.rs | 12 ++- .../transform/check_consts/validation.rs | 9 +-- src/librustc_mir/transform/promote_consts.rs | 75 ++++++++++++++----- src/librustc_mir/transform/qualify_consts.rs | 2 - 6 files changed, 120 insertions(+), 49 deletions(-) diff --git a/src/librustc_mir/transform/check_consts/mod.rs b/src/librustc_mir/transform/check_consts/mod.rs index 3a959a86edd..0c643f46243 100644 --- a/src/librustc_mir/transform/check_consts/mod.rs +++ b/src/librustc_mir/transform/check_consts/mod.rs @@ -11,7 +11,7 @@ use rustc::ty::{self, TyCtxt}; pub use self::qualifs::Qualif; pub mod ops; -mod qualifs; +pub mod qualifs; mod resolver; pub mod validation; @@ -23,6 +23,7 @@ pub struct Item<'mir, 'tcx> { def_id: DefId, param_env: ty::ParamEnv<'tcx>, mode: validation::Mode, + for_promotion: bool, } impl Item<'mir, 'tcx> { @@ -41,6 +42,28 @@ impl Item<'mir, 'tcx> { def_id, param_env, mode, + for_promotion: false, + } + } + + // HACK(eddyb) this is to get around the panic for a runtime fn from `Item::new`. + // Also, it allows promoting `&mut []`. + pub fn for_promotion( + tcx: TyCtxt<'tcx>, + def_id: DefId, + body: &'mir mir::Body<'tcx>, + ) -> Self { + let param_env = tcx.param_env(def_id); + let mode = validation::Mode::for_item(tcx, def_id) + .unwrap_or(validation::Mode::ConstFn); + + Item { + body, + tcx, + def_id, + param_env, + mode, + for_promotion: true, } } } diff --git a/src/librustc_mir/transform/check_consts/qualifs.rs b/src/librustc_mir/transform/check_consts/qualifs.rs index c8605e22e10..e666dd9571f 100644 --- a/src/librustc_mir/transform/check_consts/qualifs.rs +++ b/src/librustc_mir/transform/check_consts/qualifs.rs @@ -3,7 +3,6 @@ use rustc::mir::*; use rustc::mir::interpret::ConstValue; use rustc::ty::{self, Ty}; -use rustc_index::bit_set::BitSet; use syntax_pos::DUMMY_SP; use super::Item as ConstCx; @@ -44,7 +43,7 @@ pub trait Qualif { fn in_projection_structurally( cx: &ConstCx<'_, 'tcx>, - per_local: &BitSet, + per_local: &impl Fn(Local) -> bool, place: PlaceRef<'_, 'tcx>, ) -> bool { if let [proj_base @ .., elem] = place.projection { @@ -65,7 +64,7 @@ pub trait Qualif { ProjectionElem::ConstantIndex { .. } | ProjectionElem::Downcast(..) => qualif, - ProjectionElem::Index(local) => qualif || per_local.contains(*local), + ProjectionElem::Index(local) => qualif || per_local(*local), } } else { bug!("This should be called if projection is not empty"); @@ -74,7 +73,7 @@ pub trait Qualif { fn in_projection( cx: &ConstCx<'_, 'tcx>, - per_local: &BitSet, + per_local: &impl Fn(Local) -> bool, place: PlaceRef<'_, 'tcx>, ) -> bool { Self::in_projection_structurally(cx, per_local, place) @@ -82,14 +81,14 @@ pub trait Qualif { fn in_place( cx: &ConstCx<'_, 'tcx>, - per_local: &BitSet, + per_local: &impl Fn(Local) -> bool, place: PlaceRef<'_, 'tcx>, ) -> bool { match place { PlaceRef { base: PlaceBase::Local(local), projection: [], - } => per_local.contains(*local), + } => per_local(*local), PlaceRef { base: PlaceBase::Static(box Static { kind: StaticKind::Promoted(..), @@ -112,7 +111,7 @@ pub trait Qualif { fn in_operand( cx: &ConstCx<'_, 'tcx>, - per_local: &BitSet, + per_local: &impl Fn(Local) -> bool, operand: &Operand<'tcx>, ) -> bool { match *operand { @@ -143,7 +142,7 @@ pub trait Qualif { fn in_rvalue_structurally( cx: &ConstCx<'_, 'tcx>, - per_local: &BitSet, + per_local: &impl Fn(Local) -> bool, rvalue: &Rvalue<'tcx>, ) -> bool { match *rvalue { @@ -185,13 +184,17 @@ pub trait Qualif { } } - fn in_rvalue(cx: &ConstCx<'_, 'tcx>, per_local: &BitSet, rvalue: &Rvalue<'tcx>) -> bool { + fn in_rvalue( + cx: &ConstCx<'_, 'tcx>, + per_local: &impl Fn(Local) -> bool, + rvalue: &Rvalue<'tcx>, + ) -> bool { Self::in_rvalue_structurally(cx, per_local, rvalue) } fn in_call( cx: &ConstCx<'_, 'tcx>, - _per_local: &BitSet, + _per_local: &impl Fn(Local) -> bool, _callee: &Operand<'tcx>, _args: &[Operand<'tcx>], return_ty: Ty<'tcx>, @@ -216,7 +219,11 @@ impl Qualif for HasMutInterior { !ty.is_freeze(cx.tcx, cx.param_env, DUMMY_SP) } - fn in_rvalue(cx: &ConstCx<'_, 'tcx>, per_local: &BitSet, rvalue: &Rvalue<'tcx>) -> bool { + fn in_rvalue( + cx: &ConstCx<'_, 'tcx>, + per_local: &impl Fn(Local) -> bool, + rvalue: &Rvalue<'tcx>, + ) -> bool { match *rvalue { // Returning `true` for `Rvalue::Ref` indicates the borrow isn't // allowed in constants (and the `Checker` will error), and/or it @@ -231,12 +238,11 @@ impl Qualif for HasMutInterior { // Inside a `static mut`, &mut [...] is also allowed. ty::Array(..) | ty::Slice(_) if cx.mode == Mode::StaticMut => {}, - // FIXME(ecstaticmorse): uncomment the following match arm to stop marking - // `&mut []` as `HasMutInterior`. - /* - ty::Array(_, len) if len.try_eval_usize(cx.tcx, cx.param_env) == Some(0) - => {}, - */ + // FIXME(eddyb) the `cx.for_promotion` condition + // seems unnecessary, given that this is merely a ZST. + ty::Array(_, len) + if len.try_eval_usize(cx.tcx, cx.param_env) == Some(0) + && cx.for_promotion => {}, _ => return true, } @@ -275,7 +281,11 @@ impl Qualif for NeedsDrop { ty.needs_drop(cx.tcx, cx.param_env) } - fn in_rvalue(cx: &ConstCx<'_, 'tcx>, per_local: &BitSet, rvalue: &Rvalue<'tcx>) -> bool { + fn in_rvalue( + cx: &ConstCx<'_, 'tcx>, + per_local: &impl Fn(Local) -> bool, + rvalue: &Rvalue<'tcx>, + ) -> bool { if let Rvalue::Aggregate(ref kind, _) = *rvalue { if let AggregateKind::Adt(def, ..) = **kind { if def.has_dtor(cx.tcx) { diff --git a/src/librustc_mir/transform/check_consts/resolver.rs b/src/librustc_mir/transform/check_consts/resolver.rs index fc9290d6380..d3f1c760724 100644 --- a/src/librustc_mir/transform/check_consts/resolver.rs +++ b/src/librustc_mir/transform/check_consts/resolver.rs @@ -81,7 +81,13 @@ where return_place: &mir::Place<'tcx>, ) { let return_ty = return_place.ty(self.item.body, self.item.tcx).ty; - let qualif = Q::in_call(self.item, &mut self.qualifs_per_local, func, args, return_ty); + let qualif = Q::in_call( + self.item, + &|l| self.qualifs_per_local.contains(l), + func, + args, + return_ty, + ); if !return_place.is_indirect() { self.assign_qualif_direct(return_place, qualif); } @@ -114,7 +120,7 @@ where rvalue: &mir::Rvalue<'tcx>, location: Location, ) { - let qualif = Q::in_rvalue(self.item, self.qualifs_per_local, rvalue); + let qualif = Q::in_rvalue(self.item, &|l| self.qualifs_per_local.contains(l), rvalue); if !place.is_indirect() { self.assign_qualif_direct(place, qualif); } @@ -129,7 +135,7 @@ where // here; that occurs in `apply_call_return_effect`. if let mir::TerminatorKind::DropAndReplace { value, location: dest, .. } = kind { - let qualif = Q::in_operand(self.item, self.qualifs_per_local, value); + let qualif = Q::in_operand(self.item, &|l| self.qualifs_per_local.contains(l), value); if !dest.is_indirect() { self.assign_qualif_direct(dest, qualif); } diff --git a/src/librustc_mir/transform/check_consts/validation.rs b/src/librustc_mir/transform/check_consts/validation.rs index 76a73adf038..fa746639736 100644 --- a/src/librustc_mir/transform/check_consts/validation.rs +++ b/src/librustc_mir/transform/check_consts/validation.rs @@ -369,11 +369,10 @@ impl Visitor<'tcx> for Validator<'_, 'mir, 'tcx> { // it depends on `HasMutInterior` being set for mutable borrows as well as values with // interior mutability. if let Rvalue::Ref(_, kind, ref borrowed_place) = *rvalue { - let rvalue_has_mut_interior = HasMutInterior::in_rvalue( - &self.item, - self.qualifs.has_mut_interior.get(), - rvalue, - ); + let rvalue_has_mut_interior = { + let has_mut_interior = self.qualifs.has_mut_interior.get(); + HasMutInterior::in_rvalue(&self.item, &|l| has_mut_interior.contains(l), rvalue) + }; if rvalue_has_mut_interior { let is_derived_from_illegal_borrow = match borrowed_place.as_local() { diff --git a/src/librustc_mir/transform/promote_consts.rs b/src/librustc_mir/transform/promote_consts.rs index 4d411ad4f53..7aaff5735f6 100644 --- a/src/librustc_mir/transform/promote_consts.rs +++ b/src/librustc_mir/transform/promote_consts.rs @@ -25,12 +25,13 @@ use syntax::ast::LitKind; use syntax::symbol::sym; use syntax_pos::{Span, DUMMY_SP}; -use rustc_index::bit_set::BitSet; use rustc_index::vec::{IndexVec, Idx}; use rustc_target::spec::abi::Abi; use std::{iter, mem, usize}; +use crate::transform::check_consts::{qualifs, Item as ConstCx}; + /// State of a temporary during collection and promotion. #[derive(Copy, Clone, PartialEq, Eq, Debug)] pub enum TempState { @@ -231,9 +232,9 @@ struct Validator<'a, 'tcx> { is_static_mut: bool, is_non_const_fn: bool, temps: &'a IndexVec, - // FIXME(eddyb) compute these 2 on the fly. - has_mut_interior: &'a BitSet, - needs_drop: &'a BitSet, + + // FIXME(eddyb) deduplicate the data in this vs other fields. + const_cx: ConstCx<'a, 'tcx>, /// Explicit promotion happens e.g. for constant arguments declared via /// `rustc_args_required_const`. @@ -276,15 +277,17 @@ impl<'tcx> Validator<'_, 'tcx> { PlaceBase::Local(local) => local, _ => return Err(Unpromotable), }; + self.validate_local(base)?; + if place.projection.contains(&ProjectionElem::Deref) { return Err(Unpromotable); } - // FIXME(eddyb) compute this on the fly. - let mut has_mut_interior = self.has_mut_interior.contains(base); + let mut has_mut_interior = + self.qualif_local::(base); // HACK(eddyb) this should compute the same thing as // `::in_projection` from - // `qualify_consts` but without recursion. + // `check_consts::qualifs` but without recursion. if has_mut_interior { // This allows borrowing fields which don't have // `HasMutInterior`, from a type that does, e.g.: @@ -311,8 +314,7 @@ impl<'tcx> Validator<'_, 'tcx> { if has_mut_interior { return Err(Unpromotable); } - // FIXME(eddyb) compute this on the fly. - if self.needs_drop.contains(base) { + if self.qualif_local::(base) { return Err(Unpromotable); } if let BorrowKind::Mut { .. } = kind { @@ -339,7 +341,7 @@ impl<'tcx> Validator<'_, 'tcx> { } } - self.validate_local(base) + Ok(()) } _ => bug!() } @@ -373,6 +375,42 @@ impl<'tcx> Validator<'_, 'tcx> { } } + // FIXME(eddyb) maybe cache this? + fn qualif_local(&self, local: Local) -> bool { + let per_local = &|l| self.qualif_local::(l); + + if let TempState::Defined { location: loc, .. } = self.temps[local] { + let num_stmts = self.body[loc.block].statements.len(); + + if loc.statement_index < num_stmts { + let statement = &self.body[loc.block].statements[loc.statement_index]; + match &statement.kind { + StatementKind::Assign(box(_, rhs)) => { + Q::in_rvalue(&self.const_cx, per_local, rhs) + } + _ => { + span_bug!(statement.source_info.span, "{:?} is not an assignment", + statement); + } + } + } else { + let terminator = self.body[loc.block].terminator(); + match &terminator.kind { + TerminatorKind::Call { func, args, .. } => { + let return_ty = self.body.local_decls[local].ty; + Q::in_call(&self.const_cx, per_local, func, args, return_ty) + } + kind => { + span_bug!(terminator.source_info.span, "{:?} not promotable", kind); + } + } + } + } else { + let span = self.body.local_decls[local].source_info.span; + span_bug!(span, "{:?} not promotable, qualif_local shouldn't have been called", local); + } + } + // FIXME(eddyb) maybe cache this? fn validate_local(&self, local: Local) -> Result<(), Unpromotable> { if let TempState::Defined { location: loc, .. } = self.temps[local] { @@ -593,13 +631,14 @@ impl<'tcx> Validator<'_, 'tcx> { } } + self.validate_place(place)?; + // HACK(eddyb) this should compute the same thing as // `::in_projection` from - // `qualify_consts` but without recursion. + // `check_consts::qualifs` but without recursion. let mut has_mut_interior = match place.base { PlaceBase::Local(local) => { - // FIXME(eddyb) compute this on the fly. - self.has_mut_interior.contains(*local) + self.qualif_local::(*local) } PlaceBase::Static(_) => false, }; @@ -624,7 +663,7 @@ impl<'tcx> Validator<'_, 'tcx> { return Err(Unpromotable); } - self.validate_place(place) + Ok(()) } Rvalue::Aggregate(_, ref operands) => { @@ -680,9 +719,6 @@ pub fn validate_candidates( body: &Body<'tcx>, def_id: DefId, temps: &IndexVec, - // FIXME(eddyb) compute these 2 on the fly. - has_mut_interior: &BitSet, - needs_drop: &BitSet, candidates: &[Candidate], ) -> Vec { let mut validator = Validator { @@ -693,9 +729,8 @@ pub fn validate_candidates( is_static_mut: false, is_non_const_fn: false, temps, - // FIXME(eddyb) compute these 2 on the fly. - has_mut_interior, - needs_drop, + + const_cx: ConstCx::for_promotion(tcx, def_id, body), explicit: false, }; diff --git a/src/librustc_mir/transform/qualify_consts.rs b/src/librustc_mir/transform/qualify_consts.rs index 1c5c84c6923..21feeb1fad6 100644 --- a/src/librustc_mir/transform/qualify_consts.rs +++ b/src/librustc_mir/transform/qualify_consts.rs @@ -1118,8 +1118,6 @@ impl<'a, 'tcx> Checker<'a, 'tcx> { self.body, self.def_id, &self.temp_promotion_state, - &self.per_local.0[HasMutInterior::IDX], - &self.per_local.0[NeedsDrop::IDX], &self.unchecked_promotion_candidates, );