From 035c69f6589c1bf77191157fc24b45db51066ca9 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Fri, 24 Aug 2018 15:27:05 +0200 Subject: [PATCH] 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 --- src/librustc_lint/builtin.rs | 20 +++----- src/librustc_mir/interpret/const_eval.rs | 15 ++---- src/librustc_mir/interpret/operand.rs | 28 +++++++++-- src/librustc_mir/interpret/place.rs | 63 +++++++----------------- src/librustc_mir/interpret/validity.rs | 52 +++++++++---------- src/librustc_mir/transform/const_prop.rs | 5 +- 6 files changed, 83 insertions(+), 100 deletions(-) diff --git a/src/librustc_lint/builtin.rs b/src/librustc_lint/builtin.rs index 92a2ea2bf2d7..ed9d6ca440c0 100644 --- a/src/librustc_lint/builtin.rs +++ b/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, diff --git a/src/librustc_mir/interpret/const_eval.rs b/src/librustc_mir/interpret/const_eval.rs index ec3e46f7067e..5ed8bc875c2a 100644 --- a/src/librustc_mir/interpret/const_eval.rs +++ b/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) } diff --git a/src/librustc_mir/interpret/operand.rs b/src/librustc_mir/interpret/operand.rs index 1e45215238c3..8c0cb0b7a415 100644 --- a/src/librustc_mir/interpret/operand.rs +++ b/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> 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(&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)?; diff --git a/src/librustc_mir/interpret/place.rs b/src/librustc_mir/interpret/place.rs index de458b570f0d..6d8d1a1e8ff4 100644 --- a/src/librustc_mir/interpret/place.rs +++ b/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(&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, 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? + 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)?; - // 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 + // 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, diff --git a/src/librustc_mir/interpret/validity.rs b/src/librustc_mir/interpret/validity.rs index b0dfceb25974..af191ce3adcc 100644 --- a/src/librustc_mir/interpret/validity.rs +++ b/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, - seen: &mut FxHashSet<(MPlaceTy<'tcx>)>, - todo: &mut Vec<(MPlaceTy<'tcx>, Vec)>, + seen: &mut FxHashSet<(OpTy<'tcx>)>, + todo: &mut Vec<(OpTy<'tcx>, Vec)>, ) -> 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,8 +208,8 @@ 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 @@ -219,7 +217,7 @@ impl<'a, 'mir, 'tcx, M: Machine<'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> { 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. diff --git a/src/librustc_mir/transform/const_prop.rs b/src/librustc_mir/transform/const_prop.rs index fa11e6f0719b..ac0ed72d0663 100644 --- a/src/librustc_mir/transform/const_prop.rs +++ b/src/librustc_mir/transform/const_prop.rs @@ -257,10 +257,9 @@ impl<'b, 'a, 'tcx:'b> ConstPropagator<'b, 'a, 'tcx> { source_info: SourceInfo, ) -> Option> { 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);