Skip to content

Try to find cyclic data dependencies during const-checking #71526

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.

Sign up for GitHub

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

Closed
wants to merge 6 commits into from
Closed

Try to find cyclic data dependencies during const-checking #71526

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a75c792
Make type alias private
ecstatic-morse Apr 19, 2020
274c85f
Don't cache result of `in_any_value_of_ty` for locals
ecstatic-morse Apr 19, 2020
1343797
Lazily run dataflow for const qualification
ecstatic-morse Apr 19, 2020
15f95b1
Cycle errors now occur during const-eval, not checking
ecstatic-morse Apr 19, 2020
918dcf2
Find potential const-eval cycles during const-checking
ecstatic-morse Apr 23, 2020
3608535
Cycle errors are caught during const-checking again
ecstatic-morse Apr 24, 2020
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
182 changes: 105 additions & 77 deletions src/librustc_mir/transform/check_consts/validation.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
use rustc_errors::struct_span_err;
use rustc_hir::lang_items;
use rustc_hir::{def_id::DefId, HirId};
use rustc_index::bit_set::BitSet;
use rustc_infer::infer::TyCtxtInferExt;
use rustc_middle::mir::visit::{MutatingUseContext, NonMutatingUseContext, PlaceContext, Visitor};
use rustc_middle::mir::*;
Expand All @@ -28,70 +27,100 @@ use crate::dataflow::{self, Analysis};
// We are using `MaybeMutBorrowedLocals` as a proxy for whether an item may have been mutated
// through a pointer prior to the given point. This is okay even though `MaybeMutBorrowedLocals`
// kills locals upon `StorageDead` because a local will never be used after a `StorageDead`.
pub type IndirectlyMutableResults<'mir, 'tcx> =
type IndirectlyMutableResults<'mir, 'tcx> =
dataflow::ResultsCursor<'mir, 'tcx, MaybeMutBorrowedLocals<'mir, 'tcx>>;

struct QualifCursor<'a, 'mir, 'tcx, Q: Qualif> {
cursor: dataflow::ResultsCursor<'mir, 'tcx, FlowSensitiveAnalysis<'a, 'mir, 'tcx, Q>>,
in_any_value_of_ty: BitSet<Local>,
}

impl<Q: Qualif> QualifCursor<'a, 'mir, 'tcx, Q> {
pub fn new(q: Q, ccx: &'a ConstCx<'mir, 'tcx>) -> Self {
let cursor = FlowSensitiveAnalysis::new(q, ccx)
.into_engine(ccx.tcx, ccx.body, ccx.def_id)
.iterate_to_fixpoint()
.into_results_cursor(ccx.body);

let mut in_any_value_of_ty = BitSet::new_empty(ccx.body.local_decls.len());
for (local, decl) in ccx.body.local_decls.iter_enumerated() {
if Q::in_any_value_of_ty(ccx, decl.ty) {
in_any_value_of_ty.insert(local);
}
}

QualifCursor { cursor, in_any_value_of_ty }
}
}
type QualifResults<'mir, 'tcx, Q> =
dataflow::ResultsCursor<'mir, 'tcx, FlowSensitiveAnalysis<'mir, 'mir, 'tcx, Q>>;

pub struct Qualifs<'a, 'mir, 'tcx> {
has_mut_interior: QualifCursor<'a, 'mir, 'tcx, HasMutInterior>,
needs_drop: QualifCursor<'a, 'mir, 'tcx, NeedsDrop>,
indirectly_mutable: IndirectlyMutableResults<'mir, 'tcx>,
#[derive(Default)]
pub struct Qualifs<'mir, 'tcx> {
has_mut_interior: Option<QualifResults<'mir, 'tcx, HasMutInterior>>,
needs_drop: Option<QualifResults<'mir, 'tcx, NeedsDrop>>,
indirectly_mutable: Option<IndirectlyMutableResults<'mir, 'tcx>>,
}

impl Qualifs<'a, 'mir, 'tcx> {
fn indirectly_mutable(&mut self, local: Local, location: Location) -> bool {
self.indirectly_mutable.seek_before(location);
self.indirectly_mutable.get().contains(local)
impl Qualifs<'mir, 'tcx> {
fn indirectly_mutable(
&mut self,
ccx: &'mir ConstCx<'mir, 'tcx>,
local: Local,
location: Location,
) -> bool {
let indirectly_mutable = self.indirectly_mutable.get_or_insert_with(|| {
let ConstCx { tcx, body, def_id, param_env, .. } = *ccx;

// We can use `unsound_ignore_borrow_on_drop` here because custom drop impls are not
// allowed in a const.
//
// FIXME(ecstaticmorse): Someday we want to allow custom drop impls. How do we do this
// without breaking stable code?
MaybeMutBorrowedLocals::mut_borrows_only(tcx, &body, param_env)
.unsound_ignore_borrow_on_drop()
.into_engine(tcx, &body, def_id)
.iterate_to_fixpoint()
.into_results_cursor(&body)
});

indirectly_mutable.seek_before(location);
indirectly_mutable.get().contains(local)
}

/// Returns `true` if `local` is `NeedsDrop` at the given `Location`.
///
/// Only updates the cursor if absolutely necessary
fn needs_drop(&mut self, local: Local, location: Location) -> bool {
if !self.needs_drop.in_any_value_of_ty.contains(local) {
fn needs_drop(
&mut self,
ccx: &'mir ConstCx<'mir, 'tcx>,
local: Local,
location: Location,
) -> bool {
let ty = ccx.body.local_decls[local].ty;
if !NeedsDrop::in_any_value_of_ty(ccx, ty) {
return false;
}

self.needs_drop.cursor.seek_before(location);
self.needs_drop.cursor.get().contains(local) || self.indirectly_mutable(local, location)
let needs_drop = self.needs_drop.get_or_insert_with(|| {
let ConstCx { tcx, body, def_id, .. } = *ccx;

FlowSensitiveAnalysis::new(NeedsDrop, ccx)
.into_engine(tcx, &body, def_id)
.iterate_to_fixpoint()
.into_results_cursor(&body)
});

needs_drop.seek_before(location);
needs_drop.get().contains(local) || self.indirectly_mutable(ccx, local, location)
}

/// Returns `true` if `local` is `HasMutInterior` at the given `Location`.
///
/// Only updates the cursor if absolutely necessary.
fn has_mut_interior(&mut self, local: Local, location: Location) -> bool {
if !self.has_mut_interior.in_any_value_of_ty.contains(local) {
fn has_mut_interior(
&mut self,
ccx: &'mir ConstCx<'mir, 'tcx>,
local: Local,
location: Location,
) -> bool {
let ty = ccx.body.local_decls[local].ty;
if !HasMutInterior::in_any_value_of_ty(ccx, ty) {
return false;
}

self.has_mut_interior.cursor.seek_before(location);
self.has_mut_interior.cursor.get().contains(local)
|| self.indirectly_mutable(local, location)
let has_mut_interior = self.has_mut_interior.get_or_insert_with(|| {
let ConstCx { tcx, body, def_id, .. } = *ccx;

FlowSensitiveAnalysis::new(HasMutInterior, ccx)
.into_engine(tcx, &body, def_id)
.iterate_to_fixpoint()
.into_results_cursor(&body)
});

has_mut_interior.seek_before(location);
has_mut_interior.get().contains(local) || self.indirectly_mutable(ccx, local, location)
}

fn in_return_place(&mut self, ccx: &ConstCx<'_, 'tcx>) -> ConstQualifs {
fn in_return_place(&mut self, ccx: &'mir ConstCx<'mir, 'tcx>) -> ConstQualifs {
// Find the `Return` terminator if one exists.
//
// If no `Return` terminator exists, this MIR is divergent. Just return the conservative
Expand All @@ -114,49 +143,31 @@ impl Qualifs<'a, 'mir, 'tcx> {
let return_loc = ccx.body.terminator_loc(return_block);

ConstQualifs {
needs_drop: self.needs_drop(RETURN_PLACE, return_loc),
has_mut_interior: self.has_mut_interior(RETURN_PLACE, return_loc),
needs_drop: self.needs_drop(ccx, RETURN_PLACE, return_loc),
has_mut_interior: self.has_mut_interior(ccx, RETURN_PLACE, return_loc),
}
}
}

pub struct Validator<'a, 'mir, 'tcx> {
ccx: &'a ConstCx<'mir, 'tcx>,
qualifs: Qualifs<'a, 'mir, 'tcx>,
pub struct Validator<'mir, 'tcx> {
ccx: &'mir ConstCx<'mir, 'tcx>,
qualifs: Qualifs<'mir, 'tcx>,

/// The span of the current statement.
span: Span,
}

impl Deref for Validator<'_, 'mir, 'tcx> {
impl Deref for Validator<'mir, 'tcx> {
type Target = ConstCx<'mir, 'tcx>;

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

impl Validator<'a, 'mir, 'tcx> {
pub fn new(ccx: &'a ConstCx<'mir, 'tcx>) -> Self {
let ConstCx { tcx, body, def_id, param_env, .. } = *ccx;

let needs_drop = QualifCursor::new(NeedsDrop, ccx);
let has_mut_interior = QualifCursor::new(HasMutInterior, ccx);

// We can use `unsound_ignore_borrow_on_drop` here because custom drop impls are not
// allowed in a const.
//
// FIXME(ecstaticmorse): Someday we want to allow custom drop impls. How do we do this
// without breaking stable code?
let indirectly_mutable = MaybeMutBorrowedLocals::mut_borrows_only(tcx, body, param_env)
.unsound_ignore_borrow_on_drop()
.into_engine(tcx, body, def_id)
.iterate_to_fixpoint()
.into_results_cursor(body);

let qualifs = Qualifs { needs_drop, has_mut_interior, indirectly_mutable };

Validator { span: ccx.body.span, ccx, qualifs }
impl Validator<'mir, 'tcx> {
pub fn new(ccx: &'mir ConstCx<'mir, 'tcx>) -> Self {
Validator { span: ccx.body.span, ccx, qualifs: Default::default() }
}

pub fn check_body(&mut self) {
Expand Down Expand Up @@ -239,7 +250,7 @@ impl Validator<'a, 'mir, 'tcx> {
}
}

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

Expand Down Expand Up @@ -345,7 +356,7 @@ impl Visitor<'tcx> for Validator<'_, 'mir, 'tcx> {
| Rvalue::AddressOf(Mutability::Not, ref place) => {
let borrowed_place_has_mut_interior = qualifs::in_place::<HasMutInterior, _>(
&self.ccx,
&mut |local| self.qualifs.has_mut_interior(local, location),
&mut |local| self.qualifs.has_mut_interior(self.ccx, local, location),
place.as_ref(),
);

Expand Down Expand Up @@ -395,14 +406,31 @@ impl Visitor<'tcx> for Validator<'_, 'mir, 'tcx> {
);
}

fn visit_operand(&mut self, op: &Operand<'tcx>, location: Location) {
self.super_operand(op, location);
if let Operand::Constant(c) = op {
if let Some(def_id) = c.check_static_ptr(self.tcx) {
self.check_static(def_id, self.span);
fn visit_constant(&mut self, constant: &Constant<'tcx>, location: Location) {
self.super_constant(constant, location);

if let ty::ConstKind::Unevaluated(def_id, _, promoted) = constant.literal.val {
assert!(promoted.is_none(), "Const-checking should run before promotion");

// If a cyclic data dependency exists within a const initializer, try to find
// it during const-checking. This is important because MIR optimizations could
// eliminate a cycle before const-eval runs. See #71078 for an example of this.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

#71078 isn't actually an example of this, as Ralf notes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Huh? It isn't? Or are you talking about a comment by Ralf from elsewhere but this PR?

//
// FIXME: This means we don't look for cycles involving associated constants, but we
// should handle fully monomorphized ones here at least.
if self.tcx.trait_of_item(def_id).is_none() {
Copy link
Contributor

Choose a reason for hiding this comment

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

You can invoke Instance::resolve here, which will either bail out or give you the correct assoc constant if it can be resolved with the information at hand.

let _ = self.tcx.at(self.span).mir_const_qualif(def_id);
}
}

if let Some(def_id) = constant.check_static_ptr(self.tcx) {
self.check_static(def_id, self.span);

// NOTE: Because we are allowed to refer to the address of a static within its
// initializer, we don't try to trigger cycle errors every time we see a static.
}
}

fn visit_projection_elem(
&mut self,
place_local: Local,
Expand Down Expand Up @@ -571,7 +599,7 @@ impl Visitor<'tcx> for Validator<'_, 'mir, 'tcx> {
let needs_drop = if let Some(local) = dropped_place.as_local() {
// Use the span where the local was declared as the span of the drop error.
err_span = self.body.local_decls[local].source_info.span;
self.qualifs.needs_drop(local, location)
self.qualifs.needs_drop(self.ccx, local, location)
} else {
true
};
Expand Down