From 12054dce9ac82ec63aedb91082e3ea9cdc8c2e5c Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Wed, 26 Feb 2020 12:00:33 +0100 Subject: [PATCH] miri: validity visitor comments and path printing improvements --- src/librustc_mir/interpret/validity.rs | 67 +++++++++++++------ src/test/ui/consts/const-eval/ub-enum.stderr | 2 +- .../ui/consts/const-eval/ub-upvars.stderr | 2 +- 3 files changed, 47 insertions(+), 24 deletions(-) diff --git a/src/librustc_mir/interpret/validity.rs b/src/librustc_mir/interpret/validity.rs index f2c956c80f7..263883d5639 100644 --- a/src/librustc_mir/interpret/validity.rs +++ b/src/librustc_mir/interpret/validity.rs @@ -67,11 +67,12 @@ pub enum PathElem { Field(Symbol), Variant(Symbol), GeneratorState(VariantIdx), - ClosureVar(Symbol), + CapturedVar(Symbol), ArrayElem(usize), TupleElem(usize), Deref, - Tag, + EnumTag, + GeneratorTag, DynDowncast, } @@ -109,9 +110,11 @@ fn write_path(out: &mut String, path: &Vec) { for elem in path.iter() { match elem { Field(name) => write!(out, ".{}", name), - Variant(name) => write!(out, ".", name), + EnumTag => write!(out, "."), + Variant(name) => write!(out, ".", name), + GeneratorTag => write!(out, "."), GeneratorState(idx) => write!(out, ".", idx.index()), - ClosureVar(name) => write!(out, ".", name), + CapturedVar(name) => write!(out, ".", name), TupleElem(idx) => write!(out, ".{}", idx), ArrayElem(idx) => write!(out, "[{}]", idx), // `.` does not match Rust syntax, but it is more readable for long paths -- and @@ -119,7 +122,6 @@ fn write_path(out: &mut String, path: &Vec) { // even use the usual syntax because we are just showing the projections, // not the root. Deref => write!(out, "."), - Tag => write!(out, "."), DynDowncast => write!(out, "."), } .unwrap() @@ -170,6 +172,21 @@ struct ValidityVisitor<'rt, 'mir, 'tcx, M: Machine<'mir, 'tcx>> { impl<'rt, 'mir, 'tcx, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, 'tcx, M> { fn aggregate_field_path_elem(&mut self, layout: TyLayout<'tcx>, field: usize) -> PathElem { + // First, check if we are projecting to a variant. + match layout.variants { + layout::Variants::Multiple { discr_index, .. } => { + if discr_index == field { + return match layout.ty.kind { + ty::Adt(def, ..) if def.is_enum() => PathElem::EnumTag, + ty::Generator(..) => PathElem::GeneratorTag, + _ => bug!("non-variant type {:?}", layout.ty), + }; + } + } + layout::Variants::Single { .. } => {} + } + + // Now we know we are projecting to a field, so figure out which one. match layout.ty.kind { // generators and closures. ty::Closure(def_id, _) | ty::Generator(def_id, _, _) => { @@ -190,7 +207,7 @@ impl<'rt, 'mir, 'tcx, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, 'tcx, M } } - PathElem::ClosureVar(name.unwrap_or_else(|| { + PathElem::CapturedVar(name.unwrap_or_else(|| { // Fall back to showing the field index. sym::integer(field) })) @@ -207,10 +224,7 @@ impl<'rt, 'mir, 'tcx, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, 'tcx, M // Inside a variant PathElem::Field(def.variants[index].fields[field].ident.name) } - layout::Variants::Multiple { discr_index, .. } => { - assert_eq!(discr_index, field); - PathElem::Tag - } + layout::Variants::Multiple { .. } => bug!("we handled variants above"), } } @@ -441,11 +455,12 @@ impl<'rt, 'mir, 'tcx, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, 'tcx, M fn visit_scalar( &mut self, op: OpTy<'tcx, M::PointerTag>, - layout: &layout::Scalar, + scalar_layout: &layout::Scalar, ) -> InterpResult<'tcx> { let value = self.ecx.read_scalar(op)?; + let valid_range = &scalar_layout.valid_range; + let (lo, hi) = valid_range.clone().into_inner(); // Determine the allowed range - let (lo, hi) = layout.valid_range.clone().into_inner(); // `max_hi` is as big as the size fits let max_hi = u128::max_value() >> (128 - op.layout.size.bits()); assert!(hi <= max_hi); @@ -459,7 +474,7 @@ impl<'rt, 'mir, 'tcx, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, 'tcx, M value.not_undef(), value, self.path, - format_args!("something {}", wrapping_range_format(&layout.valid_range, max_hi),) + format_args!("something {}", wrapping_range_format(valid_range, max_hi),) ); let bits = match value.to_bits_or_ptr(op.layout.size, self.ecx) { Err(ptr) => { @@ -471,7 +486,7 @@ impl<'rt, 'mir, 'tcx, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, 'tcx, M self.path, format_args!( "something that cannot possibly fail to be {}", - wrapping_range_format(&layout.valid_range, max_hi) + wrapping_range_format(valid_range, max_hi) ) ) } @@ -484,7 +499,7 @@ impl<'rt, 'mir, 'tcx, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, 'tcx, M self.path, format_args!( "something that cannot possibly fail to be {}", - wrapping_range_format(&layout.valid_range, max_hi) + wrapping_range_format(valid_range, max_hi) ) ) } @@ -492,13 +507,13 @@ impl<'rt, 'mir, 'tcx, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, 'tcx, M Ok(data) => data, }; // Now compare. This is slightly subtle because this is a special "wrap-around" range. - if wrapping_range_contains(&layout.valid_range, bits) { + if wrapping_range_contains(&valid_range, bits) { Ok(()) } else { throw_validation_failure!( bits, self.path, - format_args!("something {}", wrapping_range_format(&layout.valid_range, max_hi)) + format_args!("something {}", wrapping_range_format(valid_range, max_hi)) ) } } @@ -594,7 +609,8 @@ impl<'rt, 'mir, 'tcx, M: Machine<'mir, 'tcx>> ValueVisitor<'mir, 'tcx, M> // *After* all of this, check the ABI. We need to check the ABI to handle // types like `NonNull` where the `Scalar` info is more restrictive than what - // the fields say. But in most cases, this will just propagate what the fields say, + // the fields say (`rustc_layout_scalar_valid_range_start`). + // But in most cases, this will just propagate what the fields say, // and then we want the error to point at the field -- so, first recurse, // then check ABI. // @@ -603,11 +619,18 @@ impl<'rt, 'mir, 'tcx, M: Machine<'mir, 'tcx>> ValueVisitor<'mir, 'tcx, M> // MyNewtype and then the scalar in there). match op.layout.abi { layout::Abi::Uninhabited => unreachable!(), // checked above - layout::Abi::Scalar(ref layout) => { - self.visit_scalar(op, layout)?; + layout::Abi::Scalar(ref scalar_layout) => { + self.visit_scalar(op, scalar_layout)?; + } + layout::Abi::ScalarPair { .. } | layout::Abi::Vector { .. } => { + // These have fields that we already visited above, so we already checked + // all their scalar-level restrictions. + // There is also no equivalent to `rustc_layout_scalar_valid_range_start` + // that would make skipping them here an issue. + } + layout::Abi::Aggregate { .. } => { + // Nothing to do. } - // FIXME: Should we do something for ScalarPair? Vector? - _ => {} } Ok(()) diff --git a/src/test/ui/consts/const-eval/ub-enum.stderr b/src/test/ui/consts/const-eval/ub-enum.stderr index 3680f4f2ac2..8c47d68e968 100644 --- a/src/test/ui/consts/const-eval/ub-enum.stderr +++ b/src/test/ui/consts/const-eval/ub-enum.stderr @@ -66,7 +66,7 @@ error[E0080]: it is undefined behavior to use this value --> $DIR/ub-enum.rs:71:1 | LL | const BAD_OPTION_CHAR: Option<(char, char)> = Some(('x', unsafe { TransmuteChar { a: !0 }.b })); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered 4294967295 at ..0.1, but expected a valid unicode codepoint + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered 4294967295 at ..0.1, but expected a valid unicode codepoint | = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rustc repository if you believe it should not be considered undefined behavior. diff --git a/src/test/ui/consts/const-eval/ub-upvars.stderr b/src/test/ui/consts/const-eval/ub-upvars.stderr index 7e4633b0908..972c9eb38c8 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 a NULL reference at ... + | |__^ type validation failed: encountered a NULL reference at ... | = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rustc repository if you believe it should not be considered undefined behavior.