Skip to content
Permalink
Browse files

Make validity checking use `MPlaceTy` instead of `OpTy`

  • Loading branch information...
oli-obk committed Feb 15, 2019
1 parent bee3c67 commit 525983a2a4ac3029dd9979d924ef444f48a4d7b3
Showing with 17 additions and 20 deletions.
  1. +5 −7 src/librustc_mir/const_eval.rs
  2. +1 −1 src/librustc_mir/interpret/place.rs
  3. +11 −12 src/librustc_mir/interpret/validity.rs
@@ -523,13 +523,11 @@ fn validate_and_turn_into_const<'a, 'tcx>(
let cid = key.value;
let ecx = mk_eval_cx(tcx, tcx.def_span(key.value.instance.def_id()), key.param_env);
let val = (|| {
let op = ecx.raw_const_to_mplace(constant)?.into();
// FIXME: Once the visitor infrastructure landed, change validation to
// work directly on `MPlaceTy`.

This comment has been minimized.

@RalfJung

RalfJung Feb 16, 2019

Member

It's still validate_operand (as opposed to validate_mplace), so the FIXME should stay.

This comment has been minimized.

@oli-obk

oli-obk Feb 16, 2019

Author Contributor

What action would we take to resolve this FIXME? I see nothing that we can do except start forcing allocations if Machine::VALIDATE is true, because we are validating locals which might just have Immedidate values

This comment has been minimized.

@RalfJung

RalfJung Feb 16, 2019

Member

Oh, good point. I forgot we are using this code during evaluation as well.

Never mind then.

let mut ref_tracking = RefTracking::new(op);
while let Some((op, path)) = ref_tracking.todo.pop() {
let mplace = ecx.raw_const_to_mplace(constant)?;
let mut ref_tracking = RefTracking::new(mplace);
while let Some((mplace, path)) = ref_tracking.todo.pop() {
ecx.validate_operand(
op,
mplace.into(),
path,
Some(&mut ref_tracking),
true, // const mode
@@ -538,7 +536,7 @@ fn validate_and_turn_into_const<'a, 'tcx>(
// Now that we validated, turn this into a proper constant.
let def_id = cid.instance.def.def_id();
let normalize = tcx.is_static(def_id).is_none() && cid.promoted.is_none();
op_to_const(&ecx, op, normalize)
op_to_const(&ecx, mplace.into(), normalize)
})();

val.map_err(|error| {
@@ -58,7 +58,7 @@ impl<'tcx, Tag> ::std::ops::Deref for PlaceTy<'tcx, Tag> {
}

/// A MemPlace with its layout. Constructing it is only possible in this module.
#[derive(Copy, Clone, Debug)]
#[derive(Copy, Clone, Debug, Hash, Eq, PartialEq)]
pub struct MPlaceTy<'tcx, Tag=()> {
mplace: MemPlace<Tag>,
pub layout: TyLayout<'tcx>,
@@ -11,7 +11,7 @@ use rustc::mir::interpret::{
};

use super::{
OpTy, Machine, EvalContext, ValueVisitor,
OpTy, Machine, EvalContext, ValueVisitor, MPlaceTy,
};

macro_rules! validation_failure {
@@ -74,13 +74,13 @@ pub enum PathElem {
}

/// State for tracking recursive validation of references
pub struct RefTracking<'tcx, Tag> {
pub seen: FxHashSet<(OpTy<'tcx, Tag>)>,
pub todo: Vec<(OpTy<'tcx, Tag>, Vec<PathElem>)>,
pub struct RefTracking<T> {
pub seen: FxHashSet<T>,
pub todo: Vec<(T, Vec<PathElem>)>,
}

impl<'tcx, Tag: Copy+Eq+Hash> RefTracking<'tcx, Tag> {
pub fn new(op: OpTy<'tcx, Tag>) -> Self {
impl<'tcx, T: Copy + Eq + Hash> RefTracking<T> {
pub fn new(op: T) -> Self {
let mut ref_tracking = RefTracking {
seen: FxHashSet::default(),
todo: vec![(op, Vec::new())],
@@ -151,7 +151,7 @@ struct ValidityVisitor<'rt, 'a: 'rt, 'mir: 'rt, 'tcx: 'a+'rt+'mir, M: Machine<'a
/// starts must not be changed! `visit_fields` and `visit_array` rely on
/// this stack discipline.
path: Vec<PathElem>,
ref_tracking: Option<&'rt mut RefTracking<'tcx, M::PointerTag>>,
ref_tracking: Option<&'rt mut RefTracking<MPlaceTy<'tcx, M::PointerTag>>>,
const_mode: bool,
ecx: &'rt EvalContext<'a, 'mir, 'tcx, M>,
}
@@ -399,16 +399,15 @@ impl<'rt, 'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>>
// before. Proceed recursively even for integer pointers, no
// reason to skip them! They are (recursively) valid for some ZST,
// but not for others (e.g., `!` is a ZST).
let op = place.into();
if ref_tracking.seen.insert(op) {
trace!("Recursing below ptr {:#?}", *op);
if ref_tracking.seen.insert(place) {
trace!("Recursing below ptr {:#?}", *place);
// We need to clone the path anyway, make sure it gets created
// with enough space for the additional `Deref`.
let mut new_path = Vec::with_capacity(self.path.len()+1);
new_path.clone_from(&self.path);
new_path.push(PathElem::Deref);
// Remember to come back to this later.
ref_tracking.todo.push((op, new_path));
ref_tracking.todo.push((place, new_path));
}
}
}
@@ -598,7 +597,7 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M>
&self,
op: OpTy<'tcx, M::PointerTag>,
path: Vec<PathElem>,
ref_tracking: Option<&mut RefTracking<'tcx, M::PointerTag>>,
ref_tracking: Option<&mut RefTracking<MPlaceTy<'tcx, M::PointerTag>>>,
const_mode: bool,
) -> EvalResult<'tcx> {
trace!("validate_operand: {:?}, {:?}", *op, op.layout.ty);

0 comments on commit 525983a

Please sign in to comment.
You can’t perform that action at this time.