From f96208ca5bd34ba5e9106fe6527db43a9bdc3a8f Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 26 Aug 2018 14:22:59 +0200 Subject: [PATCH] address nits --- src/librustc/ich/impls_ty.rs | 2 +- src/librustc/mir/interpret/error.rs | 8 +- src/librustc/ty/structural_impls.rs | 2 +- src/librustc_mir/interpret/operand.rs | 2 +- src/librustc_mir/interpret/operator.rs | 251 +++++++++++------- src/librustc_mir/interpret/place.rs | 15 +- src/librustc_mir/interpret/terminator.rs | 4 +- src/librustc_mir/interpret/validity.rs | 14 +- src/librustc_mir/transform/const_prop.rs | 2 +- .../ui/consts/const-eval/double_check2.stderr | 2 +- src/test/ui/consts/const-eval/ub-enum.stderr | 4 +- 11 files changed, 178 insertions(+), 128 deletions(-) diff --git a/src/librustc/ich/impls_ty.rs b/src/librustc/ich/impls_ty.rs index 6ee0eb273d6..c598f99a2b5 100644 --- a/src/librustc/ich/impls_ty.rs +++ b/src/librustc/ich/impls_ty.rs @@ -517,7 +517,6 @@ fn hash_stable(&self, InvalidMemoryAccess | InvalidFunctionPointer | InvalidBool | - InvalidDiscriminant | InvalidNullPointerUsage | ReadPointerAsBytes | ReadBytesAsPointer | @@ -550,6 +549,7 @@ fn hash_stable(&self, GeneratorResumedAfterReturn | GeneratorResumedAfterPanic | InfiniteLoop => {} + InvalidDiscriminant(val) => val.hash_stable(hcx, hasher), Panic { ref msg, ref file, line, col } => { msg.hash_stable(hcx, hasher); file.hash_stable(hcx, hasher); diff --git a/src/librustc/mir/interpret/error.rs b/src/librustc/mir/interpret/error.rs index c62ed841866..ab38f8fa721 100644 --- a/src/librustc/mir/interpret/error.rs +++ b/src/librustc/mir/interpret/error.rs @@ -190,7 +190,7 @@ pub enum EvalErrorKind<'tcx, O> { InvalidMemoryAccess, InvalidFunctionPointer, InvalidBool, - InvalidDiscriminant, + InvalidDiscriminant(u128), PointerOutOfBounds { ptr: Pointer, access: bool, @@ -302,8 +302,8 @@ pub fn description(&self) -> &str { "tried to use a function pointer after offsetting it", InvalidBool => "invalid boolean value read", - InvalidDiscriminant => - "invalid enum discriminant value read or written", + InvalidDiscriminant(..) => + "invalid enum discriminant value read", PointerOutOfBounds { .. } => "pointer offset outside bounds of allocation", InvalidNullPointerUsage => @@ -488,6 +488,8 @@ fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { align {}", size.bytes(), align.abi(), size2.bytes(), align2.abi()), Panic { ref msg, line, col, ref file } => write!(f, "the evaluated program panicked at '{}', {}:{}:{}", msg, file, line, col), + InvalidDiscriminant(val) => + write!(f, "encountered invalid enum discriminant {}", val), _ => write!(f, "{}", self.description()), } } diff --git a/src/librustc/ty/structural_impls.rs b/src/librustc/ty/structural_impls.rs index 55a1eadb06f..1f3c3140504 100644 --- a/src/librustc/ty/structural_impls.rs +++ b/src/librustc/ty/structural_impls.rs @@ -498,7 +498,7 @@ fn lift_to_tcx<'b, 'gcx>(&self, tcx: TyCtxt<'b, 'gcx, 'tcx>) -> Option InvalidMemoryAccess, InvalidFunctionPointer => InvalidFunctionPointer, InvalidBool => InvalidBool, - InvalidDiscriminant => InvalidDiscriminant, + InvalidDiscriminant(val) => InvalidDiscriminant(val), PointerOutOfBounds { ptr, access, diff --git a/src/librustc_mir/interpret/operand.rs b/src/librustc_mir/interpret/operand.rs index 66b9861f02e..9aba7c78caf 100644 --- a/src/librustc_mir/interpret/operand.rs +++ b/src/librustc_mir/interpret/operand.rs @@ -585,7 +585,7 @@ pub fn read_discriminant( .expect("tagged layout for non adt") .discriminants(self.tcx.tcx) .position(|var| var.val == real_discr) - .ok_or_else(|| EvalErrorKind::InvalidDiscriminant)?; + .ok_or_else(|| EvalErrorKind::InvalidDiscriminant(real_discr))?; (real_discr, index) }, layout::Variants::NicheFilling { diff --git a/src/librustc_mir/interpret/operator.rs b/src/librustc_mir/interpret/operator.rs index b0c49efe8a2..34ca6613c1a 100644 --- a/src/librustc_mir/interpret/operator.rs +++ b/src/librustc_mir/interpret/operator.rs @@ -48,111 +48,100 @@ pub fn binop_ignore_overflow( } impl<'a, 'mir, 'tcx, M: Machine<'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> { - /// Returns the result of the specified operation and whether it overflowed. - pub fn binary_op( + fn binary_char_op( &self, bin_op: mir::BinOp, - ValTy { value: left, layout: left_layout }: ValTy<'tcx>, - ValTy { value: right, layout: right_layout }: ValTy<'tcx>, + l: char, + r: char, ) -> EvalResult<'tcx, (Scalar, bool)> { use rustc::mir::BinOp::*; - let left = left.to_scalar()?; - let right = right.to_scalar()?; + let res = match bin_op { + Eq => l == r, + Ne => l != r, + Lt => l < r, + Le => l <= r, + Gt => l > r, + Ge => l >= r, + _ => bug!("Invalid operation on char: {:?}", bin_op), + }; + return Ok((Scalar::from_bool(res), false)); + } - trace!("Running binary op {:?}: {:?} ({:?}), {:?} ({:?})", - bin_op, left, left_layout.ty.sty, right, right_layout.ty.sty); + fn binary_bool_op( + &self, + bin_op: mir::BinOp, + l: bool, + r: bool, + ) -> EvalResult<'tcx, (Scalar, bool)> { + use rustc::mir::BinOp::*; - // Handle non-integer operations - if let ty::Char = left_layout.ty.sty { - assert_eq!(right_layout.ty.sty, ty::Char); - let l = left.to_char()?; - let r = right.to_char()?; - let res = match bin_op { - Eq => l == r, - Ne => l != r, - Lt => l < r, - Le => l <= r, - Gt => l > r, - Ge => l >= r, - _ => bug!("Invalid operation on char: {:?}", bin_op), - }; - return Ok((Scalar::from_bool(res), false)); - } - if let ty::Bool = left_layout.ty.sty { - assert_eq!(right_layout.ty.sty, ty::Bool); - let l = left.to_bool()?; - let r = right.to_bool()?; - let res = match bin_op { - Eq => l == r, - Ne => l != r, - Lt => l < r, - Le => l <= r, - Gt => l > r, - Ge => l >= r, - BitAnd => l & r, - BitOr => l | r, - BitXor => l ^ r, - _ => bug!("Invalid operation on bool: {:?}", bin_op), - }; - return Ok((Scalar::from_bool(res), false)); - } - if let ty::Float(fty) = left_layout.ty.sty { - let l = left.to_bits(left_layout.size)?; - let r = right.to_bits(right_layout.size)?; - assert_eq!(right_layout.ty.sty, ty::Float(fty)); - macro_rules! float_math { - ($ty:path, $size:expr) => {{ - let l = <$ty>::from_bits(l); - let r = <$ty>::from_bits(r); - let bitify = |res: ::rustc_apfloat::StatusAnd<$ty>| Scalar::Bits { - bits: res.value.to_bits(), - size: $size, - }; - let val = match bin_op { - Eq => Scalar::from_bool(l == r), - Ne => Scalar::from_bool(l != r), - Lt => Scalar::from_bool(l < r), - Le => Scalar::from_bool(l <= r), - Gt => Scalar::from_bool(l > r), - Ge => Scalar::from_bool(l >= r), - Add => bitify(l + r), - Sub => bitify(l - r), - Mul => bitify(l * r), - Div => bitify(l / r), - Rem => bitify(l % r), - _ => bug!("invalid float op: `{:?}`", bin_op), - }; - return Ok((val, false)); - }}; - } - match fty { - FloatTy::F32 => float_math!(Single, 4), - FloatTy::F64 => float_math!(Double, 8), - } - } - // Only integers left - #[inline] - fn is_ptr<'tcx>(ty: ty::Ty<'tcx>) -> bool { - match ty.sty { - ty::RawPtr(..) | ty::Ref(..) | ty::FnPtr(..) => true, - _ => false, - } + let res = match bin_op { + Eq => l == r, + Ne => l != r, + Lt => l < r, + Le => l <= r, + Gt => l > r, + Ge => l >= r, + BitAnd => l & r, + BitOr => l | r, + BitXor => l ^ r, + _ => bug!("Invalid operation on bool: {:?}", bin_op), + }; + return Ok((Scalar::from_bool(res), false)); + } + + fn binary_float_op( + &self, + bin_op: mir::BinOp, + fty: FloatTy, + // passing in raw bits + l: u128, + r: u128, + ) -> EvalResult<'tcx, (Scalar, bool)> { + use rustc::mir::BinOp::*; + + macro_rules! float_math { + ($ty:path, $size:expr) => {{ + let l = <$ty>::from_bits(l); + let r = <$ty>::from_bits(r); + let bitify = |res: ::rustc_apfloat::StatusAnd<$ty>| Scalar::Bits { + bits: res.value.to_bits(), + size: $size, + }; + let val = match bin_op { + Eq => Scalar::from_bool(l == r), + Ne => Scalar::from_bool(l != r), + Lt => Scalar::from_bool(l < r), + Le => Scalar::from_bool(l <= r), + Gt => Scalar::from_bool(l > r), + Ge => Scalar::from_bool(l >= r), + Add => bitify(l + r), + Sub => bitify(l - r), + Mul => bitify(l * r), + Div => bitify(l / r), + Rem => bitify(l % r), + _ => bug!("invalid float op: `{:?}`", bin_op), + }; + return Ok((val, false)); + }}; } - assert!(left_layout.ty.is_integral() || is_ptr(left_layout.ty)); - assert!(right_layout.ty.is_integral() || is_ptr(right_layout.ty)); - - // Handle operations that support pointers - if let Some(handled) = - M::try_ptr_op(self, bin_op, left, left_layout, right, right_layout)? - { - return Ok(handled); + match fty { + FloatTy::F32 => float_math!(Single, 4), + FloatTy::F64 => float_math!(Double, 8), } + } - // From now on, everything must be bytes, no pointer values - // (this is independent of the type) - let l = left.to_bits(left_layout.size)?; - let r = right.to_bits(right_layout.size)?; + fn binary_int_op( + &self, + bin_op: mir::BinOp, + // passing in raw bits + l: u128, + left_layout: TyLayout<'tcx>, + r: u128, + right_layout: TyLayout<'tcx>, + ) -> EvalResult<'tcx, (Scalar, bool)> { + use rustc::mir::BinOp::*; // Shift ops can have an RHS with a different numeric type. if bin_op == Shl || bin_op == Shr { @@ -189,11 +178,11 @@ fn is_ptr<'tcx>(ty: ty::Ty<'tcx>) -> bool { // For the remaining ops, the types must be the same on both sides if left_layout.ty != right_layout.ty { let msg = format!( - "unimplemented binary op {:?}: {:?} ({:?}), {:?} ({:?})", + "unimplemented asymmetric binary op {:?}: {:?} ({:?}), {:?} ({:?})", bin_op, - left, + l, left_layout.ty, - right, + r, right_layout.ty ); return err!(Unimplemented(msg)); @@ -289,11 +278,10 @@ fn is_ptr<'tcx>(ty: ty::Ty<'tcx>) -> bool { _ => { let msg = format!( - "unimplemented binary op {:?}: {:?} ({:?}), {:?} ({:?})", + "unimplemented binary op {:?}: {:?}, {:?} (both {:?})", bin_op, - left, - left_layout.ty, - right, + l, + r, right_layout.ty, ); return err!(Unimplemented(msg)); @@ -303,6 +291,65 @@ fn is_ptr<'tcx>(ty: ty::Ty<'tcx>) -> bool { Ok((val, false)) } + /// Returns the result of the specified operation and whether it overflowed. + pub fn binary_op( + &self, + bin_op: mir::BinOp, + ValTy { value: left, layout: left_layout }: ValTy<'tcx>, + ValTy { value: right, layout: right_layout }: ValTy<'tcx>, + ) -> EvalResult<'tcx, (Scalar, bool)> { + let left = left.to_scalar()?; + let right = right.to_scalar()?; + + trace!("Running binary op {:?}: {:?} ({:?}), {:?} ({:?})", + bin_op, left, left_layout.ty.sty, right, right_layout.ty.sty); + + match left_layout.ty.sty { + ty::Char => { + assert_eq!(left_layout.ty, right_layout.ty); + let l = left.to_char()?; + let r = right.to_char()?; + self.binary_char_op(bin_op, l, r) + } + ty::Bool => { + assert_eq!(left_layout.ty, right_layout.ty); + let l = left.to_bool()?; + let r = right.to_bool()?; + self.binary_bool_op(bin_op, l, r) + } + ty::Float(fty) => { + assert_eq!(left_layout.ty, right_layout.ty); + let l = left.to_bits(left_layout.size)?; + let r = right.to_bits(right_layout.size)?; + self.binary_float_op(bin_op, fty, l, r) + } + _ => { + // Must be integer(-like) types + #[inline] + fn is_ptr<'tcx>(ty: ty::Ty<'tcx>) -> bool { + match ty.sty { + ty::RawPtr(..) | ty::Ref(..) | ty::FnPtr(..) => true, + _ => false, + } + } + assert!(left_layout.ty.is_integral() || is_ptr(left_layout.ty)); + assert!(right_layout.ty.is_integral() || is_ptr(right_layout.ty)); + + // Handle operations that support pointer values + if let Some(handled) = + M::try_ptr_op(self, bin_op, left, left_layout, right, right_layout)? + { + return Ok(handled); + } + + // Everything else only works with "proper" bits + let l = left.to_bits(left_layout.size)?; + let r = right.to_bits(right_layout.size)?; + self.binary_int_op(bin_op, l, left_layout, r, right_layout) + } + } + } + pub fn unary_op( &self, un_op: mir::UnOp, diff --git a/src/librustc_mir/interpret/place.rs b/src/librustc_mir/interpret/place.rs index f79c4d5721f..04112565202 100644 --- a/src/librustc_mir/interpret/place.rs +++ b/src/librustc_mir/interpret/place.rs @@ -660,7 +660,8 @@ pub fn force_allocation( // 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! + // that might e.g. be an inner field of a struct with `Scalar` layout, + // that has different alignment than the outer field. let local_layout = self.layout_of_local(frame, local)?; let ptr = self.allocate(local_layout, MemoryKind::Stack)?; self.write_value_to_mplace(value, ptr)?; @@ -695,15 +696,11 @@ pub fn write_discriminant_index( ) -> EvalResult<'tcx> { match dest.layout.variants { layout::Variants::Single { index } => { - if index != variant_index { - return err!(InvalidDiscriminant); - } + assert_eq!(index, variant_index); } layout::Variants::Tagged { ref tag, .. } => { let adt_def = dest.layout.ty.ty_adt_def().unwrap(); - if variant_index >= adt_def.variants.len() { - return err!(InvalidDiscriminant); - } + assert!(variant_index < adt_def.variants.len()); let discr_val = adt_def .discriminant_for_variant(*self.tcx, variant_index) .val; @@ -727,9 +724,7 @@ pub fn write_discriminant_index( niche_start, .. } => { - if variant_index >= dest.layout.ty.ty_adt_def().unwrap().variants.len() { - return err!(InvalidDiscriminant); - } + assert!(variant_index < dest.layout.ty.ty_adt_def().unwrap().variants.len()); if variant_index != dataful_variant { let niche_dest = self.place_field(dest, 0)?; diff --git a/src/librustc_mir/interpret/terminator.rs b/src/librustc_mir/interpret/terminator.rs index d2a91bd3ac2..11826e0ce0c 100644 --- a/src/librustc_mir/interpret/terminator.rs +++ b/src/librustc_mir/interpret/terminator.rs @@ -276,7 +276,7 @@ fn check_ty_compat<'tcx>(ty: Ty<'tcx>, real_ty: Ty<'tcx>) -> bool { } /// Call this function -- pushing the stack frame and initializing the arguments. - /// `sig` is ptional in case of FnPtr/FnDef -- but mandatory for closures! + /// `sig` is optional in case of FnPtr/FnDef -- but mandatory for closures! fn eval_fn_call( &mut self, instance: ty::Instance<'tcx>, @@ -462,7 +462,7 @@ fn drop_in_place( layout: self.layout_of(self.tcx.mk_mut_ptr(place.layout.ty))?, }; - let ty = self.tcx.mk_tup((&[] as &[ty::Ty<'tcx>]).iter()); // return type is () + let ty = self.tcx.mk_nil(); // return type is () let dest = PlaceTy::null(&self, self.layout_of(ty)?); self.eval_fn_call( diff --git a/src/librustc_mir/interpret/validity.rs b/src/librustc_mir/interpret/validity.rs index 47d5c88e5f5..d50fd6e13c1 100644 --- a/src/librustc_mir/interpret/validity.rs +++ b/src/librustc_mir/interpret/validity.rs @@ -208,10 +208,16 @@ pub fn validate_operand( layout::Variants::Tagged { .. } => { let variant = match self.read_discriminant(dest) { Ok(res) => res.1, - Err(_) => - return validation_failure!( - "invalid enum discriminant", path - ), + Err(err) => match err.kind { + EvalErrorKind::InvalidDiscriminant(val) => + return validation_failure!( + format!("invalid enum discriminant {}", val), path + ), + _ => + return validation_failure!( + format!("non-integer enum discriminant"), path + ), + } }; let inner_dest = self.operand_downcast(dest, variant)?; // Put the variant projection onto the path, as a field diff --git a/src/librustc_mir/transform/const_prop.rs b/src/librustc_mir/transform/const_prop.rs index ac0ed72d066..70179c86765 100644 --- a/src/librustc_mir/transform/const_prop.rs +++ b/src/librustc_mir/transform/const_prop.rs @@ -170,7 +170,7 @@ fn use_ecx( | DoubleFree | InvalidFunctionPointer | InvalidBool - | InvalidDiscriminant + | InvalidDiscriminant(..) | PointerOutOfBounds { .. } | InvalidNullPointerUsage | MemoryLockViolation { .. } diff --git a/src/test/ui/consts/const-eval/double_check2.stderr b/src/test/ui/consts/const-eval/double_check2.stderr index 6b33007ec09..21025877340 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 invalid enum discriminant at .1. + | |___^ type validation failed: encountered invalid enum discriminant 5 at .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/ub-enum.stderr b/src/test/ui/consts/const-eval/ub-enum.stderr index bb015db3e54..572d08ddfee 100644 --- a/src/test/ui/consts/const-eval/ub-enum.stderr +++ b/src/test/ui/consts/const-eval/ub-enum.stderr @@ -2,7 +2,7 @@ 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 invalid enum discriminant + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered non-integer enum discriminant | = 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 @@ -10,7 +10,7 @@ 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 invalid enum discriminant + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered invalid enum discriminant 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 -- GitLab