From 49999e9b1d816e1ca7365887fd42422702bc46cb Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sat, 18 Aug 2018 13:46:52 +0200 Subject: [PATCH] optimize sanity check path printing During the sanity check, we keep track of the path we are below in a `Vec`. We avoid cloning that `Vec` unless we hit a pointer indirection. The `String` representation is only computed when validation actually fails. --- src/librustc_lint/builtin.rs | 6 +- src/librustc_mir/interpret/validity.rs | 179 ++++++++++-------- .../ui/consts/const-eval/double_check2.stderr | 2 +- src/test/ui/consts/const-eval/ub-enum-ptr.rs | 27 --- .../ui/consts/const-eval/ub-enum-ptr.stderr | 11 -- src/test/ui/consts/const-eval/ub-enum.rs | 49 +++++ src/test/ui/consts/const-eval/ub-enum.stderr | 27 +++ src/test/ui/union-ub-fat-ptr.stderr | 4 +- 8 files changed, 179 insertions(+), 126 deletions(-) delete mode 100644 src/test/ui/consts/const-eval/ub-enum-ptr.rs delete mode 100644 src/test/ui/consts/const-eval/ub-enum-ptr.stderr create mode 100644 src/test/ui/consts/const-eval/ub-enum.rs create mode 100644 src/test/ui/consts/const-eval/ub-enum.stderr diff --git a/src/librustc_lint/builtin.rs b/src/librustc_lint/builtin.rs index bcbf6d167df..681e9195805 100644 --- a/src/librustc_lint/builtin.rs +++ b/src/librustc_lint/builtin.rs @@ -1622,13 +1622,13 @@ fn validate_const<'a, 'tcx>( let layout = ecx.layout_of(constant.ty)?; let place = ecx.allocate_op(OpTy { op, layout })?.into(); - let mut todo = vec![(place, String::new())]; + let mut todo = vec![(place, Vec::new())]; let mut seen = FxHashSet(); seen.insert(place); - while let Some((place, path)) = todo.pop() { + while let Some((place, mut path)) = todo.pop() { ecx.validate_mplace( place, - path, + &mut path, &mut seen, &mut todo, )?; diff --git a/src/librustc_mir/interpret/validity.rs b/src/librustc_mir/interpret/validity.rs index 12ea3ed09ff..7e32f3a971f 100644 --- a/src/librustc_mir/interpret/validity.rs +++ b/src/librustc_mir/interpret/validity.rs @@ -1,5 +1,6 @@ use std::fmt::Write; +use syntax_pos::symbol::Symbol; use rustc::ty::layout::{self, Size, Primitive}; use rustc::ty::{self, Ty}; use rustc_data_structures::fx::FxHashSet; @@ -13,10 +14,11 @@ macro_rules! validation_failure{ ($what:expr, $where:expr, $details:expr) => {{ - let where_ = if $where.is_empty() { + let where_ = path_format($where); + let where_ = if where_.is_empty() { String::new() } else { - format!(" at {}", $where) + format!(" at {}", where_) }; err!(ValidationFailure(format!( "encountered {}{}, but expected {}", @@ -24,10 +26,11 @@ macro_rules! validation_failure{ ))) }}; ($what:expr, $where:expr) => {{ - let where_ = if $where.is_empty() { + let where_ = path_format($where); + let where_ = if where_.is_empty() { String::new() } else { - format!(" at {}", $where) + format!(" at {}", where_) }; err!(ValidationFailure(format!( "encountered {}{}", @@ -36,13 +39,59 @@ macro_rules! validation_failure{ }}; } +/// We want to show a nice path to the invalid field for diagnotsics, +/// but avoid string operations in the happy case where no error happens. +/// So we track a `Vec` where `PathElem` contains all the data we +/// need to later print something for the user. +#[derive(Copy, Clone, Debug)] +pub enum PathElem { + Field(Symbol), + ClosureVar(Symbol), + ArrayElem(usize), + TupleElem(usize), + Deref, + 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::*; + + 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(), + 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(), + } + } + out +} + impl<'a, 'mir, 'tcx, M: Machine<'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> { fn validate_scalar( &self, value: ScalarMaybeUndef, size: Size, scalar: &layout::Scalar, - path: &str, + path: &Vec, ty: Ty, ) -> EvalResult<'tcx> { trace!("validate scalar: {:#?}, {:#?}, {:#?}, {}", value, size, scalar, ty); @@ -93,7 +142,11 @@ fn validate_scalar( ty::TyChar => { debug_assert_eq!(size.bytes(), 4); if ::std::char::from_u32(bits as u32).is_none() { - return err!(InvalidChar(bits)); + return validation_failure!( + "character", + path, + "a valid unicode codepoint" + ); } } _ => {}, @@ -127,17 +180,21 @@ fn validate_scalar( /// This function checks the memory where `dest` points to. The place must be sized /// (i.e., dest.extra == PlaceExtra::None). /// 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( &self, dest: MPlaceTy<'tcx>, - path: String, + path: &mut Vec, seen: &mut FxHashSet<(MPlaceTy<'tcx>)>, - todo: &mut Vec<(MPlaceTy<'tcx>, String)>, + todo: &mut Vec<(MPlaceTy<'tcx>, Vec)>, ) -> EvalResult<'tcx> { self.memory.dump_alloc(dest.to_ptr()?.alloc_id); trace!("validate_mplace: {:?}, {:#?}", *dest, dest.layout); - // Find the right variant + // Find the right variant. We have to handle this as a prelude, not via + // proper recursion with the new inner layout, to be able to later nicely + // print the field names of the enum field that is being accessed. let (variant, dest) = match dest.layout.variants { layout::Variants::NicheFilling { niche: ref tag, .. } | layout::Variants::Tagged { ref tag, .. } => { @@ -145,28 +202,35 @@ pub fn validate_mplace( // 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 path = format!("{}.TAG", path); + 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 dest = self.mplace_downcast(dest, variant)?; + let inner_dest = self.mplace_downcast(dest, variant)?; + // Put the variant projection onto the path, as a field + path.push(PathElem::Field(dest.layout.ty.ty_adt_def().unwrap().variants[variant].name)); trace!("variant layout: {:#?}", dest.layout); - (variant, dest) + (variant, inner_dest) }, layout::Variants::Single { index } => { (index, dest) } }; + // Remember the length, in case we need to truncate + let path_len = path.len(); + // Validate all fields match dest.layout.fields { // primitives are unions with zero fields layout::FieldPlacement::Union(0) => { match dest.layout.abi { // nothing to do, whatever the pointer points to, it is never going to be read - layout::Abi::Uninhabited => validation_failure!("a value of an uninhabited type", path), + layout::Abi::Uninhabited => + return validation_failure!("a value of an uninhabited type", path), // check that the scalar is a valid pointer or that its bit range matches the // expectation. layout::Abi::Scalar(ref scalar_layout) => { @@ -179,8 +243,8 @@ pub fn validate_mplace( if let Scalar::Ptr(ptr) = scalar.not_undef()? { let alloc_kind = self.tcx.alloc_map.lock().get(ptr.alloc_id); if let Some(AllocType::Static(did)) = alloc_kind { - // statics from other crates are already checked - // extern statics should not be validated as they have no body + // statics from other crates are already checked. + // extern statics should not be validated as they have no body. if !did.is_local() || self.tcx.is_foreign_item(did) { return Ok(()); } @@ -190,12 +254,11 @@ pub fn validate_mplace( 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, format!("(*{})", path))) + todo.push((ptr_place, path_clone_and_deref(path))); } } } } - Ok(()) }, _ => bug!("bad abi for FieldPlacement::Union(0): {:#?}", dest.layout.abi), } @@ -204,16 +267,14 @@ pub fn validate_mplace( // We can't check 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::Array { .. } => { for (i, field) in self.mplace_array_fields(dest)?.enumerate() { let field = field?; - let mut path = path.clone(); - self.dump_field_name(&mut path, dest.layout.ty, i as usize, variant).unwrap(); + path.push(PathElem::ArrayElem(i)); self.validate_mplace(field, path, seen, todo)?; + path.truncate(path_len); } - Ok(()) }, layout::FieldPlacement::Arbitrary { ref offsets, .. } => { // fat pointers need special treatment @@ -232,9 +293,7 @@ pub fn validate_mplace( assert_eq!(ptr.extra, PlaceExtra::Length(len)); let unpacked_ptr = self.unpack_unsized_mplace(ptr)?; if seen.insert(unpacked_ptr) { - let mut path = path.clone(); - self.dump_field_name(&mut path, dest.layout.ty, 0, 0).unwrap(); - todo.push((unpacked_ptr, path)) + todo.push((unpacked_ptr, path_clone_and_deref(path))); } }, Some(ty::TyDynamic(..)) => { @@ -249,9 +308,7 @@ pub fn validate_mplace( assert_eq!(ptr.extra, PlaceExtra::Vtable(vtable)); let unpacked_ptr = self.unpack_unsized_mplace(ptr)?; if seen.insert(unpacked_ptr) { - let mut path = path.clone(); - self.dump_field_name(&mut path, dest.layout.ty, 0, 0).unwrap(); - todo.push((unpacked_ptr, path)) + todo.push((unpacked_ptr, path_clone_and_deref(path))); } // FIXME: More checks for the vtable... making sure it is exactly // the one one would expect for this type. @@ -261,84 +318,42 @@ pub fn validate_mplace( None => { // Not a pointer, perform regular aggregate handling below for i in 0..offsets.len() { - let mut path = path.clone(); - self.dump_field_name(&mut path, dest.layout.ty, i, variant).unwrap(); let field = self.mplace_field(dest, i as u64)?; + path.push(self.aggregate_field_path_elem(dest.layout.ty, variant, i)); self.validate_mplace(field, path, seen, todo)?; + path.truncate(path_len); } // FIXME: For a TyStr, check that this is valid UTF-8. }, } - - Ok(()) } } + Ok(()) } - fn dump_field_name(&self, s: &mut String, ty: Ty<'tcx>, i: usize, variant: usize) -> ::std::fmt::Result { + fn aggregate_field_path_elem(&self, ty: Ty<'tcx>, variant: usize, field: usize) -> PathElem { match ty.sty { - ty::TyBool | - ty::TyChar | - ty::TyInt(_) | - ty::TyUint(_) | - ty::TyFloat(_) | - ty::TyFnPtr(_) | - ty::TyNever | - ty::TyFnDef(..) | - ty::TyGeneratorWitness(..) | - ty::TyForeign(..) | - ty::TyDynamic(..) => { - bug!("field_name({:?}): not applicable", ty) - } - - // Potentially-fat pointers. - ty::TyRef(_, pointee, _) | - ty::TyRawPtr(ty::TypeAndMut { ty: pointee, .. }) => { - assert!(i < 2); - - // Reuse the fat *T type as its own thin pointer data field. - // This provides information about e.g. DST struct pointees - // (which may have no non-DST form), and will work as long - // as the `Abi` or `FieldPlacement` is checked by users. - if i == 0 { - return write!(s, ".data_ptr"); - } - - match self.tcx.struct_tail(pointee).sty { - ty::TySlice(_) | - ty::TyStr => write!(s, ".len"), - ty::TyDynamic(..) => write!(s, ".vtable_ptr"), - _ => bug!("field_name({:?}): not applicable", ty) - } - } - - // Arrays and slices. - ty::TyArray(_, _) | - ty::TySlice(_) | - ty::TyStr => write!(s, "[{}]", i), - // generators and closures. ty::TyClosure(def_id, _) | ty::TyGenerator(def_id, _, _) => { let node_id = self.tcx.hir.as_local_node_id(def_id).unwrap(); - let freevar = self.tcx.with_freevars(node_id, |fv| fv[i]); - write!(s, ".upvar({})", self.tcx.hir.name(freevar.var_id())) + let freevar = self.tcx.with_freevars(node_id, |fv| fv[field]); + PathElem::ClosureVar(self.tcx.hir.name(freevar.var_id())) } - ty::TyTuple(_) => write!(s, ".{}", i), + // tuples + ty::TyTuple(_) => PathElem::TupleElem(field), // enums ty::TyAdt(def, ..) if def.is_enum() => { let variant = &def.variants[variant]; - write!(s, ".{}::{}", variant.name, variant.fields[i].ident) + PathElem::Field(variant.fields[field].ident.name) } - // other ADTs. - ty::TyAdt(def, _) => write!(s, ".{}", def.non_enum_variant().fields[i].ident), + // other ADTs + ty::TyAdt(def, _) => PathElem::Field(def.non_enum_variant().fields[field].ident.name), - ty::TyProjection(_) | ty::TyAnon(..) | ty::TyParam(_) | - ty::TyInfer(_) | ty::TyError => { - bug!("dump_field_name: unexpected type `{}`", ty) - } + // nothing else has an aggregate layout + _ => bug!("aggregate_field_path_elem: got non-aggregate type {:?}", ty), } } } diff --git a/src/test/ui/consts/const-eval/double_check2.stderr b/src/test/ui/consts/const-eval/double_check2.stderr index 2a0a674e237..739af12d09c 100644 --- a/src/test/ui/consts/const-eval/double_check2.stderr +++ b/src/test/ui/consts/const-eval/double_check2.stderr @@ -5,7 +5,7 @@ LL | / static FOO: (&Foo, &Bar) = unsafe {( //~ undefined behavior LL | | Union { usize: &BAR }.foo, LL | | Union { usize: &BAR }.bar, LL | | )}; - | |___^ type validation failed: encountered 5 at (*.1).TAG, but expected something in the range 42..=99 + | |___^ type validation failed: encountered 5 at .1.., but expected something in the range 42..=99 | = 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/ub-enum-ptr.rs b/src/test/ui/consts/const-eval/ub-enum-ptr.rs deleted file mode 100644 index 8538dd14afe..00000000000 --- a/src/test/ui/consts/const-eval/ub-enum-ptr.rs +++ /dev/null @@ -1,27 +0,0 @@ -// Copyright 2018 The Rust Project Developers. See the COPYRIGHT -// file at the top-level directory of this distribution and at -// http://rust-lang.org/COPYRIGHT. -// -// Licensed under the Apache License, Version 2.0 or the MIT license -// , at your -// option. This file may not be copied, modified, or distributed -// except according to those terms. - -#[repr(usize)] -#[derive(Copy, Clone)] -enum Enum { - A = 0, -} - -union Foo { - a: &'static u8, - b: Enum, -} - -// A pointer is guaranteed non-null -const BAD_ENUM: Enum = unsafe { Foo { a: &1 }.b}; -//~^ ERROR this constant likely exhibits undefined behavior - -fn main() { -} diff --git a/src/test/ui/consts/const-eval/ub-enum-ptr.stderr b/src/test/ui/consts/const-eval/ub-enum-ptr.stderr deleted file mode 100644 index 4b7ccc25c6c..00000000000 --- a/src/test/ui/consts/const-eval/ub-enum-ptr.stderr +++ /dev/null @@ -1,11 +0,0 @@ -error[E0080]: this constant likely exhibits undefined behavior - --> $DIR/ub-enum-ptr.rs:23:1 - | -LL | const BAD_ENUM: Enum = unsafe { Foo { a: &1 }.b}; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered pointer at .TAG, but expected something in the range 0..=0 - | - = 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 - -error: aborting due to previous error - -For more information about this error, try `rustc --explain E0080`. diff --git a/src/test/ui/consts/const-eval/ub-enum.rs b/src/test/ui/consts/const-eval/ub-enum.rs new file mode 100644 index 00000000000..bcb71af54af --- /dev/null +++ b/src/test/ui/consts/const-eval/ub-enum.rs @@ -0,0 +1,49 @@ +// Copyright 2018 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +#[repr(usize)] +#[derive(Copy, Clone)] +enum Enum { + A = 0, +} +union TransmuteEnum { + a: &'static u8, + b: Enum, +} + +// A pointer is guaranteed non-null +const BAD_ENUM: Enum = unsafe { TransmuteEnum { a: &1 }.b }; +//~^ ERROR this constant likely exhibits undefined behavior + +// Invalid enum discriminant +#[repr(usize)] +#[derive(Copy, Clone)] +enum Enum2 { + A = 2, +} +union TransmuteEnum2 { + a: usize, + b: Enum2, +} +const BAD_ENUM2 : Enum2 = unsafe { TransmuteEnum2 { a: 0 }.b }; +//~^ ERROR this constant likely exhibits undefined behavior + +// Invalid enum field content (mostly to test printing of apths for enum tuple +// variants and tuples). +union TransmuteChar { + a: u32, + b: char, +} +// Need to create something which does not clash with enum layout optimizations. +const BAD_ENUM_CHAR : Option<(char, char)> = Some(('x', unsafe { TransmuteChar { a: !0 }.b })); +//~^ ERROR this constant likely exhibits undefined behavior + +fn main() { +} diff --git a/src/test/ui/consts/const-eval/ub-enum.stderr b/src/test/ui/consts/const-eval/ub-enum.stderr new file mode 100644 index 00000000000..98e9b598b54 --- /dev/null +++ b/src/test/ui/consts/const-eval/ub-enum.stderr @@ -0,0 +1,27 @@ +error[E0080]: this constant likely exhibits undefined behavior + --> $DIR/ub-enum.rs:22:1 + | +LL | const BAD_ENUM: Enum = unsafe { TransmuteEnum { a: &1 }.b }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered pointer at ., but expected something in the range 0..=0 + | + = 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 + +error[E0080]: this constant likely exhibits undefined behavior + --> $DIR/ub-enum.rs:35:1 + | +LL | const BAD_ENUM2 : Enum2 = unsafe { TransmuteEnum2 { a: 0 }.b }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered 0 at ., but expected something in the range 2..=2 + | + = 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 + +error[E0080]: this constant likely exhibits undefined behavior + --> $DIR/ub-enum.rs:45:1 + | +LL | const BAD_ENUM_CHAR : Option<(char, char)> = Some(('x', unsafe { TransmuteChar { a: !0 }.b })); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered character at .Some.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 rust compiler repository if you believe it should not be considered undefined behavior + +error: aborting due to 3 previous errors + +For more information about this error, try `rustc --explain E0080`. diff --git a/src/test/ui/union-ub-fat-ptr.stderr b/src/test/ui/union-ub-fat-ptr.stderr index addc013fc45..af85d1d39c4 100644 --- a/src/test/ui/union-ub-fat-ptr.stderr +++ b/src/test/ui/union-ub-fat-ptr.stderr @@ -58,7 +58,7 @@ error[E0080]: this constant likely exhibits undefined behavior --> $DIR/union-ub-fat-ptr.rs:102:1 | LL | const G: &Trait = &unsafe { BoolTransmute { val: 3 }.bl }; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered 3 at .data_ptr, 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 @@ -66,7 +66,7 @@ error[E0080]: this constant likely exhibits undefined behavior --> $DIR/union-ub-fat-ptr.rs:106:1 | LL | const H: &[bool] = &[unsafe { BoolTransmute { val: 3 }.bl }]; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered 3 at .data_ptr[0], but expected something in the range 0..=1 + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered 3 at .[0], 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 -- 2.44.0