switch validation to use operand, not mplace

this means we can get rid of the public allocate_op, and make OpTy only
constructible in librustc_mir

Author: RalfJung

src/librustc_lint/builtin.rs

@@ -1614,21 +1614,15 @@ fn validate_const<'a, 'tcx>(
     gid: ::rustc::mir::interpret::GlobalId<'tcx>,
     what: &str,
 ) {
-    let mut ecx = ::rustc_mir::interpret::mk_eval_cx(tcx, gid.instance, param_env).unwrap();
+    let ecx = ::rustc_mir::interpret::mk_eval_cx(tcx, gid.instance, param_env).unwrap();
     let result = (|| {
-        use rustc_target::abi::LayoutOf;
-        use rustc_mir::interpret::OpTy;
-
-        let op = ecx.const_value_to_op(constant.val)?;
-        let layout = ecx.layout_of(constant.ty)?;
-        let place = ecx.allocate_op(OpTy { op, layout })?.into();
-
-        let mut todo = vec![(place, Vec::new())];
+        let op = ecx.const_to_op(constant)?;
+        let mut todo = vec![(op, Vec::new())];
         let mut seen = FxHashSet();
-        seen.insert(place);
-        while let Some((place, mut path)) = todo.pop() {
-            ecx.validate_mplace(
-                place,
+        seen.insert(op);
+        while let Some((op, mut path)) = todo.pop() {
+            ecx.validate_operand(
+                op,
                 &mut path,
                 &mut seen,
                 &mut todo,

src/librustc_mir/interpret/const_eval.rs

@@ -120,13 +120,6 @@ pub fn op_to_const<'tcx>(
     };
     Ok(ty::Const::from_const_value(ecx.tcx.tcx, val, op.layout.ty))
 }
-pub fn const_to_op<'tcx>(
-    ecx: &mut EvalContext<'_, '_, 'tcx, CompileTimeEvaluator>,
-    cnst: &'tcx ty::Const<'tcx>,
-) -> EvalResult<'tcx, OpTy<'tcx>> {
-    let op = ecx.const_value_to_op(cnst.val)?;
-    Ok(OpTy { op, layout: ecx.layout_of(cnst.ty)? })
-}
 
 fn eval_body_and_ecx<'a, 'mir, 'tcx>(
     tcx: TyCtxt<'a, 'tcx, 'tcx>,
@@ -351,10 +344,10 @@ pub fn const_field<'a, 'tcx>(
     value: &'tcx ty::Const<'tcx>,
 ) -> ::rustc::mir::interpret::ConstEvalResult<'tcx> {
     trace!("const_field: {:?}, {:?}, {:?}", instance, field, value);
-    let mut ecx = mk_eval_cx(tcx, instance, param_env).unwrap();
+    let ecx = mk_eval_cx(tcx, instance, param_env).unwrap();
     let result = (|| {
         // get the operand again
-        let op = const_to_op(&mut ecx, value)?;
+        let op = ecx.const_to_op(value)?;
         // downcast
         let down = match variant {
             None => op,
@@ -383,8 +376,8 @@ pub fn const_variant_index<'a, 'tcx>(
     val: &'tcx ty::Const<'tcx>,
 ) -> EvalResult<'tcx, usize> {
     trace!("const_variant_index: {:?}, {:?}", instance, val);
-    let mut ecx = mk_eval_cx(tcx, instance, param_env).unwrap();
-    let op = const_to_op(&mut ecx, val)?;
+    let ecx = mk_eval_cx(tcx, instance, param_env).unwrap();
+    let op = ecx.const_to_op(val)?;
     ecx.read_discriminant_as_variant_index(op)
 }
 

src/librustc_mir/interpret/operand.rs

@@ -11,9 +11,10 @@
 //! Functions concerning immediate values and operands, and reading from operands.
 //! All high-level functions to read from memory work on operands as sources.
 
+use std::hash::{Hash, Hasher};
 use std::convert::TryInto;
 
-use rustc::mir;
+use rustc::{mir, ty};
 use rustc::ty::layout::{self, Size, Align, LayoutOf, TyLayout, HasDataLayout, IntegerExt};
 use rustc_data_structures::indexed_vec::Idx;
 
@@ -150,7 +151,7 @@ impl Operand {
 
 #[derive(Copy, Clone, Debug)]
 pub struct OpTy<'tcx> {
-    pub op: Operand,
+    crate op: Operand, // ideally we'd make this private, but we are not there yet
     pub layout: TyLayout<'tcx>,
 }
 
@@ -182,6 +183,20 @@ impl<'tcx> From<ValTy<'tcx>> for OpTy<'tcx> {
     }
 }
 
+// Validation needs to hash OpTy, but we cannot hash Layout -- so we just hash the type
+impl<'tcx> Hash for OpTy<'tcx> {
+    fn hash<H: Hasher>(&self, state: &mut H) {
+        self.op.hash(state);
+        self.layout.ty.hash(state);
+    }
+}
+impl<'tcx> PartialEq for OpTy<'tcx> {
+    fn eq(&self, other: &Self) -> bool {
+        self.op == other.op && self.layout.ty == other.layout.ty
+    }
+}
+impl<'tcx> Eq for OpTy<'tcx> {}
+
 impl<'tcx> OpTy<'tcx> {
     #[inline]
     pub fn from_ptr(ptr: Pointer, align: Align, layout: TyLayout<'tcx>) -> Self {
@@ -492,7 +507,7 @@ impl<'a, 'mir, 'tcx, M: Machine<'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> {
     }
 
     // Also used e.g. when miri runs into a constant.
-    pub fn const_value_to_op(
+    pub(super) fn const_value_to_op(
         &self,
         val: ConstValue<'tcx>,
     ) -> EvalResult<'tcx, Operand> {
@@ -516,6 +531,13 @@ impl<'a, 'mir, 'tcx, M: Machine<'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> {
                 Ok(Operand::Immediate(Value::Scalar(x.into()))),
         }
     }
+    pub fn const_to_op(
+        &self,
+        cnst: &ty::Const<'tcx>,
+    ) -> EvalResult<'tcx, OpTy<'tcx>> {
+        let op = self.const_value_to_op(cnst.val)?;
+        Ok(OpTy { op, layout: self.layout_of(cnst.ty)? })
+    }
 
     pub(super) fn global_to_op(&self, gid: GlobalId<'tcx>) -> EvalResult<'tcx, Operand> {
         let cv = self.const_eval(gid)?;

src/librustc_mir/interpret/place.rs

@@ -12,7 +12,6 @@
 //! into a place.
 //! All high-level functions to write to memory work on places as destinations.
 
-use std::hash::{Hash, Hasher};
 use std::convert::TryFrom;
 
 use rustc::mir;
@@ -159,20 +158,6 @@ impl<'tcx> MPlaceTy<'tcx> {
     }
 }
 
-// Validation needs to hash MPlaceTy, but we cannot hash Layout -- so we just hash the type
-impl<'tcx> Hash for MPlaceTy<'tcx> {
-    fn hash<H: Hasher>(&self, state: &mut H) {
-        self.mplace.hash(state);
-        self.layout.ty.hash(state);
-    }
-}
-impl<'tcx> PartialEq for MPlaceTy<'tcx> {
-    fn eq(&self, other: &Self) -> bool {
-        self.mplace == other.mplace && self.layout.ty == other.layout.ty
-    }
-}
-impl<'tcx> Eq for MPlaceTy<'tcx> {}
-
 impl<'tcx> OpTy<'tcx> {
     #[inline(always)]
     pub fn try_as_mplace(self) -> Result<MPlaceTy<'tcx>, Value> {
@@ -681,20 +666,25 @@ impl<'a, 'mir, 'tcx, M: Machine<'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> {
     ) -> EvalResult<'tcx, MPlaceTy<'tcx>> {
         let mplace = match place.place {
             Place::Local { frame, local } => {
-                // FIXME: Consider not doing anything for a ZST, and just returning
-                // a fake pointer?
-
-                // We need the layout of the local.  We can NOT use the layout we got,
-                // that might e.g. be a downcast variant!
-                let local_layout = self.layout_of_local(frame, local)?;
-                // Make sure it has a place
-                let rval = *self.stack[frame].locals[local].access()?;
-                let mplace = self.allocate_op(OpTy { op: rval, layout: local_layout })?.mplace;
-                // This might have allocated the flag
-                *self.stack[frame].locals[local].access_mut()? =
-                    Operand::Indirect(mplace);
-                // done
-                mplace
+                match *self.stack[frame].locals[local].access()? {
+                    Operand::Indirect(mplace) => mplace,
+                    Operand::Immediate(value) => {
+                        // We need to make an allocation.
+                        // FIXME: Consider not doing anything for a ZST, and just returning
+                        // a fake pointer?  Are we even called for ZST?
+
+                        // We need the layout of the local.  We can NOT use the layout we got,
+                        // that might e.g. be a downcast variant!
+                        let local_layout = self.layout_of_local(frame, local)?;
+                        let ptr = self.allocate(local_layout, MemoryKind::Stack)?;
+                        self.write_value_to_mplace(value, ptr)?;
+                        let mplace = ptr.mplace;
+                        // Update the local
+                        *self.stack[frame].locals[local].access_mut()? =
+                            Operand::Indirect(mplace);
+                        mplace
+                    }
+                }
             }
             Place::Ptr(mplace) => mplace
         };
@@ -712,23 +702,6 @@ impl<'a, 'mir, 'tcx, M: Machine<'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> {
         Ok(MPlaceTy::from_aligned_ptr(ptr, layout))
     }
 
-    /// Make a place for an operand, allocating if needed
-    pub fn allocate_op(
-        &mut self,
-        OpTy { op, layout }: OpTy<'tcx>,
-    ) -> EvalResult<'tcx, MPlaceTy<'tcx>> {
-        trace!("allocate_op: {:?}", op);
-        Ok(match op {
-            Operand::Indirect(mplace) => MPlaceTy { mplace, layout },
-            Operand::Immediate(value) => {
-                // FIXME: Is stack always right here?
-                let ptr = self.allocate(layout, MemoryKind::Stack)?;
-                self.write_value_to_mplace(value, ptr)?;
-                ptr
-            },
-        })
-    }
-
     pub fn write_discriminant_value(
         &mut self,
         variant_index: usize,

src/librustc_mir/interpret/validity.rs

@@ -19,7 +19,7 @@ use rustc::mir::interpret::{
 };
 
 use super::{
-    MPlaceTy, Machine, EvalContext
+    OpTy, Machine, EvalContext
 };
 
 macro_rules! validation_failure{
@@ -187,19 +187,17 @@ impl<'a, 'mir, 'tcx, M: Machine<'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> {
         }
     }
 
-    /// This function checks the memory where `dest` points to.  The place must be sized
-    /// (i.e., dest.extra == PlaceExtra::None).
+    /// This function checks the data at `op`.  The operand must be sized.
     /// It will error if the bits at the destination do not match the ones described by the layout.
     /// The `path` may be pushed to, but the part that is present when the function
     /// starts must not be changed!
-    pub fn validate_mplace(
+    pub fn validate_operand(
         &self,
-        dest: MPlaceTy<'tcx>,
+        dest: OpTy<'tcx>,
         path: &mut Vec<PathElem>,
-        seen: &mut FxHashSet<(MPlaceTy<'tcx>)>,
-        todo: &mut Vec<(MPlaceTy<'tcx>, Vec<PathElem>)>,
+        seen: &mut FxHashSet<(OpTy<'tcx>)>,
+        todo: &mut Vec<(OpTy<'tcx>, Vec<PathElem>)>,
     ) -> EvalResult<'tcx> {
-        self.memory.dump_alloc(dest.to_ptr()?.alloc_id);
         trace!("validate_mplace: {:?}, {:#?}", *dest, dest.layout);
 
         // Find the right variant.  We have to handle this as a prelude, not via
@@ -210,16 +208,16 @@ impl<'a, 'mir, 'tcx, M: Machine<'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> {
             layout::Variants::Tagged { ref tag, .. } => {
                 let size = tag.value.size(self);
                 // we first read the tag value as scalar, to be able to validate it
-                let tag_mplace = self.mplace_field(dest, 0)?;
-                let tag_value = self.read_scalar(tag_mplace.into())?;
+                let tag_mplace = self.operand_field(dest, 0)?;
+                let tag_value = self.read_scalar(tag_mplace)?;
                 path.push(PathElem::Tag);
                 self.validate_scalar(
                     tag_value, size, tag, &path, tag_mplace.layout.ty
                 )?;
                 path.pop(); // remove the element again
                 // then we read it again to get the index, to continue
                 let variant = self.read_discriminant_as_variant_index(dest.into())?;
-                let inner_dest = self.mplace_downcast(dest, variant)?;
+                let inner_dest = self.operand_downcast(dest, variant)?;
                 // Put the variant projection onto the path, as a field
                 path.push(PathElem::Field(dest.layout.ty
                                           .ty_adt_def()
@@ -251,7 +249,7 @@ impl<'a, 'mir, 'tcx, M: Machine<'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> {
                     // expectation.
                     layout::Abi::Scalar(ref scalar_layout) => {
                         let size = scalar_layout.value.size(self);
-                        let value = self.read_value(dest.into())?;
+                        let value = self.read_value(dest)?;
                         let scalar = value.to_scalar_or_undef();
                         self.validate_scalar(scalar, size, scalar_layout, &path, dest.layout.ty)?;
                         if scalar_layout.value == Primitive::Pointer {
@@ -267,11 +265,11 @@ impl<'a, 'mir, 'tcx, M: Machine<'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> {
                                 }
                                 if value.layout.ty.builtin_deref(false).is_some() {
                                     trace!("Recursing below ptr {:#?}", value);
-                                    let ptr_place = self.ref_to_mplace(value)?;
-                                    // we have not encountered this pointer+layout
-                                    // combination before
-                                    if seen.insert(ptr_place) {
-                                        todo.push((ptr_place, path_clone_and_deref(path)));
+                                    let ptr_op = self.ref_to_mplace(value)?.into();
+                                    // we have not encountered this pointer+layout combination
+                                    // before.
+                                    if seen.insert(ptr_op) {
+                                        todo.push((ptr_op, path_clone_and_deref(path)));
                                     }
                                 }
                             }
@@ -286,11 +284,15 @@ impl<'a, 'mir, 'tcx, M: Machine<'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> {
                 // See https://github.com/rust-lang/rust/issues/32836#issuecomment-406875389
             },
             layout::FieldPlacement::Array { .. } => {
-                for (i, field) in self.mplace_array_fields(dest)?.enumerate() {
-                    let field = field?;
-                    path.push(PathElem::ArrayElem(i));
-                    self.validate_mplace(field, path, seen, todo)?;
-                    path.truncate(path_len);
+                // Skips for ZSTs; we could have an empty array as an immediate
+                if !dest.layout.is_zst() {
+                    let dest = dest.to_mem_place(); // arrays cannot be immediate
+                    for (i, field) in self.mplace_array_fields(dest)?.enumerate() {
+                        let field = field?;
+                        path.push(PathElem::ArrayElem(i));
+                        self.validate_operand(field.into(), path, seen, todo)?;
+                        path.truncate(path_len);
+                    }
                 }
             },
             layout::FieldPlacement::Arbitrary { ref offsets, .. } => {
@@ -311,7 +313,7 @@ impl<'a, 'mir, 'tcx, M: Machine<'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> {
                             _ => return Err(err),
                         }
                     };
-                    let unpacked_ptr = self.unpack_unsized_mplace(ptr)?;
+                    let unpacked_ptr = self.unpack_unsized_mplace(ptr)?.into();
                     // for safe ptrs, recursively check it
                     if !dest.layout.ty.is_unsafe_ptr() {
                         trace!("Recursing below fat ptr {:?} (unpacked: {:?})", ptr, unpacked_ptr);
@@ -322,9 +324,9 @@ impl<'a, 'mir, 'tcx, M: Machine<'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> {
                 } else {
                     // Not a pointer, perform regular aggregate handling below
                     for i in 0..offsets.len() {
-                        let field = self.mplace_field(dest, i as u64)?;
+                        let field = self.operand_field(dest, i as u64)?;
                         path.push(self.aggregate_field_path_elem(dest.layout.ty, variant, i));
-                        self.validate_mplace(field, path, seen, todo)?;
+                        self.validate_operand(field, path, seen, todo)?;
                         path.truncate(path_len);
                     }
                     // FIXME: For a TyStr, check that this is valid UTF-8.

src/librustc_mir/transform/const_prop.rs

@@ -257,10 +257,9 @@ impl<'b, 'a, 'tcx:'b> ConstPropagator<'b, 'a, 'tcx> {
         source_info: SourceInfo,
     ) -> Option<Const<'tcx>> {
         self.ecx.tcx.span = source_info.span;
-        match self.ecx.const_value_to_op(c.literal.val) {
+        match self.ecx.const_to_op(c.literal) {
             Ok(op) => {
-                let layout = self.tcx.layout_of(self.param_env.and(c.literal.ty)).ok()?;
-                Some((OpTy { op, layout }, c.span))
+                Some((op, c.span))
             },
             Err(error) => {
                 let (stacktrace, span) = self.ecx.generate_stacktrace(None);