diff --git a/src/librustc_mir/interpret/validity.rs b/src/librustc_mir/interpret/validity.rs index 8ac3f992cfd..7caf81788dd 100644 --- a/src/librustc_mir/interpret/validity.rs +++ b/src/librustc_mir/interpret/validity.rs @@ -78,6 +78,7 @@ pub enum PathElem { TupleElem(usize), Deref, Tag, + DynDowncast, } /// State for tracking recursive validation of references @@ -97,15 +98,6 @@ impl<'tcx, Tag: Copy+Eq+Hash> RefTracking<'tcx, Tag> { } } -// Adding a Deref and making a copy of the path to be put into the queue -// always go together. This one does it with only new allocation. -fn path_clone_and_deref(path: &Vec) -> Vec { - let mut new_path = Vec::with_capacity(path.len()+1); - new_path.clone_from(path); - new_path.push(PathElem::Deref); - new_path -} - /// Format a path fn path_format(path: &Vec) -> String { use self::PathElem::*; @@ -113,59 +105,23 @@ fn path_format(path: &Vec) -> String { let mut out = String::new(); for elem in path.iter() { match elem { - Field(name) => write!(out, ".{}", name).unwrap(), - ClosureVar(name) => write!(out, ".", name).unwrap(), - TupleElem(idx) => write!(out, ".{}", idx).unwrap(), - ArrayElem(idx) => write!(out, "[{}]", idx).unwrap(), + Field(name) => write!(out, ".{}", name), + ClosureVar(name) => write!(out, ".", name), + TupleElem(idx) => write!(out, ".{}", idx), + ArrayElem(idx) => write!(out, "[{}]", idx), Deref => // This does not match Rust syntax, but it is more readable for long paths -- and // some of the other items here also are not Rust syntax. Actually we can't // even use the usual syntax because we are just showing the projections, // not the root. - write!(out, ".").unwrap(), - Tag => write!(out, ".").unwrap(), - } + write!(out, "."), + Tag => write!(out, "."), + DynDowncast => write!(out, "."), + }.unwrap() } out } -fn aggregate_field_path_elem<'a, 'tcx>( - layout: TyLayout<'tcx>, - field: usize, - tcx: TyCtxt<'a, 'tcx, 'tcx>, -) -> PathElem { - match layout.ty.sty { - // generators and closures. - ty::Closure(def_id, _) | ty::Generator(def_id, _, _) => { - if let Some(upvar) = tcx.optimized_mir(def_id).upvar_decls.get(field) { - PathElem::ClosureVar(upvar.debug_name) - } else { - // Sometimes the index is beyond the number of freevars (seen - // for a generator). - PathElem::ClosureVar(Symbol::intern(&field.to_string())) - } - } - - // tuples - ty::Tuple(_) => PathElem::TupleElem(field), - - // enums - ty::Adt(def, ..) if def.is_enum() => { - let variant = match layout.variants { - layout::Variants::Single { index } => &def.variants[index], - _ => bug!("aggregate_field_path_elem: got enum but not in a specific variant"), - }; - PathElem::Field(variant.fields[field].ident.name) - } - - // other ADTs - ty::Adt(def, _) => PathElem::Field(def.non_enum_variant().fields[field].ident.name), - - // nothing else has an aggregate layout - _ => bug!("aggregate_field_path_elem: got non-aggregate type {:?}", layout.ty), - } -} - fn scalar_format(value: ScalarMaybeUndef) -> String { match value { ScalarMaybeUndef::Undef => @@ -177,7 +133,7 @@ fn scalar_format(value: ScalarMaybeUndef) -> String { } } -struct ValidityVisitor<'rt, 'tcx, Tag> { +struct ValidityVisitor<'rt, 'a, 'tcx, Tag> { op: OpTy<'tcx, Tag>, /// The `path` may be pushed to, but the part that is present when a function /// starts must not be changed! `visit_fields` and `visit_array` rely on @@ -185,20 +141,89 @@ struct ValidityVisitor<'rt, 'tcx, Tag> { path: Vec, ref_tracking: Option<&'rt mut RefTracking<'tcx, Tag>>, const_mode: bool, + tcx: TyCtxt<'a, 'tcx, 'tcx>, } -impl fmt::Debug for ValidityVisitor<'_, '_, Tag> { +impl fmt::Debug for ValidityVisitor<'_, '_, '_, Tag> { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - write!(f, "{:?} ({:?})", *self.op, self.op.layout.ty) + write!(f, "{:?}, {:?}", *self.op, self.op.layout.ty) + } +} + +impl<'rt, 'a, 'tcx, Tag> ValidityVisitor<'rt, 'a, 'tcx, Tag> { + fn push_aggregate_field_path_elem( + &mut self, + layout: TyLayout<'tcx>, + field: usize, + ) { + let elem = match layout.ty.sty { + // generators and closures. + ty::Closure(def_id, _) | ty::Generator(def_id, _, _) => { + if let Some(upvar) = self.tcx.optimized_mir(def_id).upvar_decls.get(field) { + PathElem::ClosureVar(upvar.debug_name) + } else { + // Sometimes the index is beyond the number of freevars (seen + // for a generator). + PathElem::ClosureVar(Symbol::intern(&field.to_string())) + } + } + + // tuples + ty::Tuple(_) => PathElem::TupleElem(field), + + // enums + ty::Adt(def, ..) if def.is_enum() => { + let variant = match layout.variants { + layout::Variants::Single { index } => &def.variants[index], + _ => bug!("aggregate_field_path_elem: got enum but not in a specific variant"), + }; + PathElem::Field(variant.fields[field].ident.name) + } + + // other ADTs + ty::Adt(def, _) => PathElem::Field(def.non_enum_variant().fields[field].ident.name), + + // arrays/slices + ty::Array(..) | ty::Slice(..) => PathElem::ArrayElem(field), + + // dyn traits + ty::Dynamic(..) => PathElem::DynDowncast, + + // nothing else has an aggregate layout + _ => bug!("aggregate_field_path_elem: got non-aggregate type {:?}", layout.ty), + }; + self.path.push(elem); } } impl<'rt, 'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> - ValueVisitor<'a, 'mir, 'tcx, M> for ValidityVisitor<'rt, 'tcx, M::PointerTag> + ValueVisitor<'a, 'mir, 'tcx, M> for ValidityVisitor<'rt, 'a, 'tcx, M::PointerTag> { + type V = OpTy<'tcx, M::PointerTag>; + #[inline(always)] - fn layout(&self) -> TyLayout<'tcx> { - self.op.layout + fn value(&self) -> &OpTy<'tcx, M::PointerTag> { + &self.op + } + + #[inline] + fn with_field( + &mut self, + val: Self::V, + field: usize, + f: impl FnOnce(&mut Self) -> EvalResult<'tcx>, + ) -> EvalResult<'tcx> { + // Remember the old state + let path_len = self.path.len(); + let op = self.op; + // Perform operation + self.push_aggregate_field_path_elem(op.layout, field); + self.op = val; + f(self)?; + // Undo changes + self.path.truncate(path_len); + self.op = op; + Ok(()) } fn downcast_enum(&mut self, ectx: &EvalContext<'a, 'mir, 'tcx, M>) @@ -227,15 +252,6 @@ impl<'rt, 'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> Ok(()) } - fn downcast_dyn_trait(&mut self, ectx: &EvalContext<'a, 'mir, 'tcx, M>) - -> EvalResult<'tcx> - { - // FIXME: Should we reflect this in `self.path`? - let dest = self.op.to_mem_place(); // immediate trait objects are not a thing - self.op = ectx.unpack_dyn_trait(dest)?.1.into(); - Ok(()) - } - fn visit_primitive(&mut self, ectx: &mut EvalContext<'a, 'mir, 'tcx, M>) -> EvalResult<'tcx> { @@ -365,7 +381,13 @@ impl<'rt, 'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> let op = place.into(); if ref_tracking.seen.insert(op) { trace!("Recursing below ptr {:#?}", *op); - ref_tracking.todo.push((op, path_clone_and_deref(&self.path))); + // 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)); } } } @@ -378,12 +400,17 @@ impl<'rt, 'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> // FIXME: Check if the signature matches } // This should be all the primitive types - ty::Never => bug!("Uninhabited type should have been caught earlier"), _ => bug!("Unexpected primitive type {}", value.layout.ty) } Ok(()) } + fn visit_uninhabited(&mut self, _ectx: &mut EvalContext<'a, 'mir, 'tcx, M>) + -> EvalResult<'tcx> + { + validation_failure!("a value of an uninhabited type", self.path) + } + fn visit_scalar(&mut self, ectx: &mut EvalContext<'a, 'mir, 'tcx, M>, layout: &layout::Scalar) -> EvalResult<'tcx> { @@ -468,47 +495,16 @@ impl<'rt, 'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> } } - fn visit_fields(&mut self, ectx: &mut EvalContext<'a, 'mir, 'tcx, M>, num_fields: usize) - -> EvalResult<'tcx> + fn handle_array(&mut self, ectx: &EvalContext<'a, 'mir, 'tcx, M>) + -> EvalResult<'tcx, bool> { - // Remember some stuff that will change for the recursive calls - let op = self.op; - let path_len = self.path.len(); - // Go look at all the fields - for i in 0..num_fields { - // Adapt our state - self.op = ectx.operand_field(op, i as u64)?; - self.path.push(aggregate_field_path_elem(op.layout, i, *ectx.tcx)); - // Recursive visit - ectx.visit_value(self)?; - // Restore original state - self.op = op; - self.path.truncate(path_len); - } - Ok(()) - } - - fn visit_str(&mut self, ectx: &mut EvalContext<'a, 'mir, 'tcx, M>) - -> EvalResult<'tcx> - { - let mplace = self.op.to_mem_place(); // strings are never immediate - try_validation!(ectx.read_str(mplace), - "uninitialized or non-UTF-8 data in str", self.path); - Ok(()) - } - - fn visit_array(&mut self, ectx: &mut EvalContext<'a, 'mir, 'tcx, M>) -> EvalResult<'tcx> - { - let mplace = if self.op.layout.is_zst() { - // it's a ZST, the memory content cannot matter - MPlaceTy::dangling(self.op.layout, ectx) - } else { - // non-ZST array/slice/str cannot be immediate - self.op.to_mem_place() - }; - match self.op.layout.ty.sty { - ty::Str => bug!("Strings should be handled separately"), - // Special handling for arrays/slices of builtin integer types + Ok(match self.op.layout.ty.sty { + ty::Str => { + let mplace = self.op.to_mem_place(); // strings are never immediate + try_validation!(ectx.read_str(mplace), + "uninitialized or non-UTF-8 data in str", self.path); + true + } ty::Array(tys, ..) | ty::Slice(tys) if { // This optimization applies only for integer and floating point types // (i.e., types that can hold arbitrary bytes). @@ -517,6 +513,13 @@ impl<'rt, 'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> _ => false, } } => { + let mplace = if self.op.layout.is_zst() { + // it's a ZST, the memory content cannot matter + MPlaceTy::dangling(self.op.layout, ectx) + } else { + // non-ZST array/slice/str cannot be immediate + self.op.to_mem_place() + }; // This is the length of the array/slice. let len = mplace.len(ectx)?; // This is the element type size. @@ -539,7 +542,7 @@ impl<'rt, 'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> /*allow_ptr_and_undef*/!self.const_mode, ) { // In the happy case, we needn't check anything else. - Ok(()) => {}, + Ok(()) => true, // handled these arrays // Some error happened, try to provide a more detailed description. Err(err) => { // For some errors we might be able to provide extra information @@ -560,26 +563,9 @@ impl<'rt, 'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> } } } - }, - _ => { - // Remember some stuff that will change for the recursive calls - let op = self.op; - let path_len = self.path.len(); - // This handles the unsized case correctly as well, as well as - // SIMD and all sorts of other array-like types. - for (i, field) in ectx.mplace_array_fields(mplace)?.enumerate() { - // Adapt our state - self.op = field?.into(); - self.path.push(PathElem::ArrayElem(i)); - // Recursive visit - ectx.visit_value(self)?; - // Restore original state - self.op = op; - self.path.truncate(path_len); - } } - } - Ok(()) + _ => false, // not handled + }) } } @@ -605,7 +591,8 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> op, path, ref_tracking, - const_mode + const_mode, + tcx: *self.tcx, }; // Run it diff --git a/src/librustc_mir/interpret/visitor.rs b/src/librustc_mir/interpret/visitor.rs index 892eaceff1b..7d6029d6424 100644 --- a/src/librustc_mir/interpret/visitor.rs +++ b/src/librustc_mir/interpret/visitor.rs @@ -10,34 +10,103 @@ use rustc::mir::interpret::{ }; use super::{ - Machine, EvalContext, + Machine, EvalContext, MPlaceTy, OpTy, }; -// How to traverse a value and what to do when we are at the leaves. -// In the future, we might want to turn this into two traits, but so far the -// only implementations we have couldn't share any code anyway. -pub trait ValueVisitor<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>>: fmt::Debug { +// A thing that we can project into, and that has a layout. +// This wouldn't have to depend on `Machine` but with the current type inference, +// that's just more convenient to work with (avoids repeading all the `Machine` bounds). +pub trait Value<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>>: Copy +{ // Get this value's layout. fn layout(&self) -> TyLayout<'tcx>; - // Downcast functions. These change the value as a side-effect. + // Get the underlying `MPlaceTy`, or panic if there is no such thing. + fn to_mem_place(self) -> MPlaceTy<'tcx, M::PointerTag>; + + // Create this from an `MPlaceTy` + fn from_mem_place(MPlaceTy<'tcx, M::PointerTag>) -> Self; + + // Project to the n-th field + fn project_field( + self, + ectx: &mut EvalContext<'a, 'mir, 'tcx, M>, + field: u64, + ) -> EvalResult<'tcx, Self>; +} + +// Operands and places are both values +impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> Value<'a, 'mir, 'tcx, M> + for OpTy<'tcx, M::PointerTag> +{ + #[inline(always)] + fn layout(&self) -> TyLayout<'tcx> { + self.layout + } + + #[inline(always)] + fn to_mem_place(self) -> MPlaceTy<'tcx, M::PointerTag> { + self.to_mem_place() + } + + #[inline(always)] + fn from_mem_place(mplace: MPlaceTy<'tcx, M::PointerTag>) -> Self { + mplace.into() + } + + #[inline(always)] + fn project_field( + self, + ectx: &mut EvalContext<'a, 'mir, 'tcx, M>, + field: u64, + ) -> EvalResult<'tcx, Self> { + ectx.operand_field(self, field) + } +} + +// How to traverse a value and what to do when we are at the leaves. +pub trait ValueVisitor<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>>: fmt::Debug { + type V: Value<'a, 'mir, 'tcx, M>; + + // There's a value in here. + fn value(&self) -> &Self::V; + + // The value's layout (not meant to be overwritten). + #[inline(always)] + fn layout(&self) -> TyLayout<'tcx> { + self.value().layout() + } + + // Replace the value by `val`, which must be the `field`th field of `self`, + // then call `f` and then un-do everything that might have happened to the visitor state. + // The point of this is that some visitors keep a stack of fields that we projected below, + // and this lets us avoid copying that stack; instead they will pop the stack after + // executing `f`. + fn with_field( + &mut self, + val: Self::V, + field: usize, + f: impl FnOnce(&mut Self) -> EvalResult<'tcx>, + ) -> EvalResult<'tcx>; + + // This is an enum, downcast it to whatever the current variant is. + // (We do this here and not in `Value` to keep error handling + // under control of th visitor.) fn downcast_enum(&mut self, ectx: &EvalContext<'a, 'mir, 'tcx, M>) -> EvalResult<'tcx>; - fn downcast_dyn_trait(&mut self, ectx: &EvalContext<'a, 'mir, 'tcx, M>) - -> EvalResult<'tcx>; - // Visit all fields of a compound. - // Just call `visit_value` if you want to go on recursively. - fn visit_fields(&mut self, ectx: &mut EvalContext<'a, 'mir, 'tcx, M>, num_fields: usize) - -> EvalResult<'tcx>; - // Optimized handling for arrays -- avoid computing the layout for every field. - // Also it is the value's responsibility to figure out the length. - fn visit_array(&mut self, ectx: &mut EvalContext<'a, 'mir, 'tcx, M>) -> EvalResult<'tcx>; - // Special handling for strings. - fn visit_str(&mut self, ectx: &mut EvalContext<'a, 'mir, 'tcx, M>) - -> EvalResult<'tcx>; + // A chance for the visitor to do special (different or more efficient) handling for some + // array types. Return `true` if the value was handled and we should return. + #[inline] + fn handle_array(&mut self, _ectx: &EvalContext<'a, 'mir, 'tcx, M>) + -> EvalResult<'tcx, bool> + { + Ok(false) + } // Actions on the leaves. + fn visit_uninhabited(&mut self, ectx: &mut EvalContext<'a, 'mir, 'tcx, M>) + -> EvalResult<'tcx>; fn visit_scalar(&mut self, ectx: &mut EvalContext<'a, 'mir, 'tcx, M>, layout: &layout::Scalar) -> EvalResult<'tcx>; fn visit_primitive(&mut self, ectx: &mut EvalContext<'a, 'mir, 'tcx, M>) @@ -45,7 +114,10 @@ pub trait ValueVisitor<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>>: fmt::Debug { } impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> { - pub fn visit_value>(&mut self, v: &mut V) -> EvalResult<'tcx> { + pub fn visit_value>( + &mut self, + v: &mut V, + ) -> EvalResult<'tcx> { trace!("visit_value: {:?}", v); // If this is a multi-variant layout, we have find the right one and proceed with that. @@ -63,7 +135,10 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> // If it is a trait object, switch to the actual type that was used to create it. match v.layout().ty.sty { ty::Dynamic(..) => { - v.downcast_dyn_trait(self)?; + let dest = v.value().to_mem_place(); // immediate trait objects are not a thing + let inner = self.unpack_dyn_trait(dest)?.1; + // recurse with the inner type + return v.with_field(Value::from_mem_place(inner), 0, |v| self.visit_value(v)); }, _ => {}, }; @@ -76,6 +151,9 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> // scalars, we do the same check on every "level" (e.g. first we check // MyNewtype and then the scalar in there). match v.layout().abi { + layout::Abi::Uninhabited => { + v.visit_uninhabited(self)?; + } layout::Abi::Scalar(ref layout) => { v.visit_scalar(self, layout)?; } @@ -107,19 +185,31 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> // We can't traverse unions, their bits are allowed to be anything. // The fields don't need to correspond to any bit pattern of the union's fields. // See https://github.com/rust-lang/rust/issues/32836#issuecomment-406875389 - Ok(()) }, layout::FieldPlacement::Arbitrary { ref offsets, .. } => { - v.visit_fields(self, offsets.len()) + for i in 0..offsets.len() { + let val = v.value().project_field(self, i as u64)?; + v.with_field(val, i, |v| self.visit_value(v))?; + } }, layout::FieldPlacement::Array { .. } => { - match v.layout().ty.sty { - // Strings have properties that cannot be expressed pointwise. - ty::Str => v.visit_str(self), - // General case -- might also be SIMD vector or so - _ => v.visit_array(self), + if !v.handle_array(self)? { + // We still have to work! + // Let's get an mplace first. + let mplace = if v.layout().is_zst() { + // it's a ZST, the memory content cannot matter + MPlaceTy::dangling(v.layout(), self) + } else { + // non-ZST array/slice/str cannot be immediate + v.value().to_mem_place() + }; + // Now iterate over it. + for (i, field) in self.mplace_array_fields(mplace)?.enumerate() { + v.with_field(Value::from_mem_place(field?), i, |v| self.visit_value(v))?; + } } } } + Ok(()) } } diff --git a/src/test/ui/consts/const-eval/ub-upvars.stderr b/src/test/ui/consts/const-eval/ub-upvars.stderr index 178f80f88e8..947af20b889 100644 --- a/src/test/ui/consts/const-eval/ub-upvars.stderr +++ b/src/test/ui/consts/const-eval/ub-upvars.stderr @@ -6,7 +6,7 @@ LL | | let bad_ref: &'static u16 = unsafe { mem::transmute(0usize) }; LL | | let another_var = 13; LL | | move || { let _ = bad_ref; let _ = another_var; } LL | | }; - | |__^ type validation failed: encountered 0 at .., but expected something greater or equal to 1 + | |__^ type validation failed: encountered 0 at ..., but expected something greater or equal to 1 | = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior diff --git a/src/test/ui/consts/const-eval/union-ub-fat-ptr.stderr b/src/test/ui/consts/const-eval/union-ub-fat-ptr.stderr index b61ea9ca6f9..13683ead0d5 100644 --- a/src/test/ui/consts/const-eval/union-ub-fat-ptr.stderr +++ b/src/test/ui/consts/const-eval/union-ub-fat-ptr.stderr @@ -66,7 +66,7 @@ error[E0080]: it is undefined behavior to use this value --> $DIR/union-ub-fat-ptr.rs:116:1 | LL | const G: &Trait = &unsafe { BoolTransmute { val: 3 }.bl }; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered 3 at ., but expected something in the range 0..=1 + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered 3 at .., but expected something in the range 0..=1 | = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior