From f9644c16f3156025d83d3dc1262ac35d9ae89588 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Fri, 28 Feb 2020 09:21:25 +0100 Subject: [PATCH 1/5] add comment to check_data --- src/librustc/mir/interpret/value.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/librustc/mir/interpret/value.rs b/src/librustc/mir/interpret/value.rs index 2c146b5d7b426..0e14089eebc3e 100644 --- a/src/librustc/mir/interpret/value.rs +++ b/src/librustc/mir/interpret/value.rs @@ -170,6 +170,10 @@ impl From for Scalar { } impl Scalar<()> { + /// Make sure the `data` fits in `size`. + /// This is guaranteed by all constructors here, but since the enum variants are public, + /// it could still be violated (even though no code outside this file should + /// construct `Scalar`s). #[inline(always)] fn check_data(data: u128, size: u8) { debug_assert_eq!( From e9ab099d6f30328b03ec2d5515bd11e4852d1eca Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Fri, 28 Feb 2020 09:27:21 +0100 Subject: [PATCH 2/5] get rid of to_ptr --- src/librustc/mir/interpret/value.rs | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/src/librustc/mir/interpret/value.rs b/src/librustc/mir/interpret/value.rs index 0e14089eebc3e..44a6fc5fb842c 100644 --- a/src/librustc/mir/interpret/value.rs +++ b/src/librustc/mir/interpret/value.rs @@ -406,22 +406,14 @@ impl<'tcx, Tag> Scalar { self.to_bits(target_size).expect("expected Raw bits but got a Pointer") } - /// Do not call this method! Use either `assert_ptr` or `force_ptr`. - /// This method is intentionally private, do not make it public. #[inline] - fn to_ptr(self) -> InterpResult<'tcx, Pointer> { + pub fn assert_ptr(self) -> Pointer { match self { - Scalar::Raw { data: 0, .. } => throw_unsup!(InvalidNullPointerUsage), - Scalar::Raw { .. } => throw_unsup!(ReadBytesAsPointer), - Scalar::Ptr(p) => Ok(p), + Scalar::Ptr(p) => p, + Scalar::Raw { .. } => bug!("expected a Pointer but got Raw bits") } } - #[inline(always)] - pub fn assert_ptr(self) -> Pointer { - self.to_ptr().expect("expected a Pointer but got Raw bits") - } - /// Do not call this method! Dispatch based on the type instead. #[inline] pub fn is_bits(self) -> bool { From 1fec68277a16e39389085fba462a05cd71797423 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Fri, 28 Feb 2020 09:15:04 +0100 Subject: [PATCH 3/5] remove check_raw after reducing it to one use only --- src/librustc/mir/interpret/value.rs | 11 ++------ src/librustc_mir_build/hair/pattern/_match.rs | 28 +++++++++---------- 2 files changed, 16 insertions(+), 23 deletions(-) diff --git a/src/librustc/mir/interpret/value.rs b/src/librustc/mir/interpret/value.rs index 44a6fc5fb842c..2eb5f7ca87823 100644 --- a/src/librustc/mir/interpret/value.rs +++ b/src/librustc/mir/interpret/value.rs @@ -382,19 +382,14 @@ impl<'tcx, Tag> Scalar { } } - #[inline(always)] - pub fn check_raw(data: u128, size: u8, target_size: Size) { - assert_eq!(target_size.bytes(), size as u64); - assert_ne!(size, 0, "you should never look at the bits of a ZST"); - Scalar::check_data(data, size); - } - /// Do not call this method! Use either `assert_bits` or `force_bits`. #[inline] pub fn to_bits(self, target_size: Size) -> InterpResult<'tcx, u128> { match self { Scalar::Raw { data, size } => { - Self::check_raw(data, size, target_size); + assert_eq!(target_size.bytes(), size as u64); + assert_ne!(size, 0, "you should never look at the bits of a ZST"); + Scalar::check_data(data, size); Ok(data) } Scalar::Ptr(_) => throw_unsup!(ReadPointerAsBytes), diff --git a/src/librustc_mir_build/hair/pattern/_match.rs b/src/librustc_mir_build/hair/pattern/_match.rs index 85f03629b646a..a4a122a737fe0 100644 --- a/src/librustc_mir_build/hair/pattern/_match.rs +++ b/src/librustc_mir_build/hair/pattern/_match.rs @@ -1396,21 +1396,19 @@ impl<'tcx> IntRange<'tcx> { ) -> Option> { if let Some((target_size, bias)) = Self::integral_size_and_signed_bias(tcx, value.ty) { let ty = value.ty; - let val = if let ty::ConstKind::Value(ConstValue::Scalar(Scalar::Raw { data, size })) = - value.val - { - // For this specific pattern we can skip a lot of effort and go - // straight to the result, after doing a bit of checking. (We - // could remove this branch and just use the next branch, which - // is more general but much slower.) - Scalar::<()>::check_raw(data, size, target_size); - data - } else if let Some(val) = value.try_eval_bits(tcx, param_env, ty) { - // This is a more general form of the previous branch. - val - } else { - return None; - }; + let val = (|| { + if let ty::ConstKind::Value(ConstValue::Scalar(scalar)) = value.val { + // For this specific pattern we can skip a lot of effort and go + // straight to the result, after doing a bit of checking. (We + // could remove this branch and just fall through, which + // is more general but much slower.) + if let Ok(bits) = scalar.to_bits_or_ptr(target_size, &tcx) { + return Some(bits); + } + } + // This is a more general form of the previous case. + value.try_eval_bits(tcx, param_env, ty) + })()?; let val = val ^ bias; Some(IntRange { range: val..=val, ty, span }) } else { From 63c77f1bd0beee9c3ee75cc41c507e31d3a72a5d Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Fri, 28 Feb 2020 11:04:12 +0100 Subject: [PATCH 4/5] remove ScalarMaybeUndef::to_bits and make Scalar::to_bits private --- src/librustc/mir/interpret/value.rs | 13 ++--- src/librustc/ty/sty.rs | 2 +- src/librustc_mir/interpret/intrinsics.rs | 2 +- src/librustc_mir/interpret/operand.rs | 63 +++++++++++------------- src/librustc_mir/interpret/validity.rs | 15 +++--- src/librustc_mir/transform/const_prop.rs | 4 +- 6 files changed, 46 insertions(+), 53 deletions(-) diff --git a/src/librustc/mir/interpret/value.rs b/src/librustc/mir/interpret/value.rs index 2eb5f7ca87823..c931fcf71e39d 100644 --- a/src/librustc/mir/interpret/value.rs +++ b/src/librustc/mir/interpret/value.rs @@ -382,9 +382,10 @@ impl<'tcx, Tag> Scalar { } } - /// Do not call this method! Use either `assert_bits` or `force_bits`. + /// This method is intentionally private! + /// It is just a helper for other methods in this file. #[inline] - pub fn to_bits(self, target_size: Size) -> InterpResult<'tcx, u128> { + fn to_bits(self, target_size: Size) -> InterpResult<'tcx, u128> { match self { Scalar::Raw { data, size } => { assert_eq!(target_size.bytes(), size as u64); @@ -405,7 +406,7 @@ impl<'tcx, Tag> Scalar { pub fn assert_ptr(self) -> Pointer { match self { Scalar::Ptr(p) => p, - Scalar::Raw { .. } => bug!("expected a Pointer but got Raw bits") + Scalar::Raw { .. } => bug!("expected a Pointer but got Raw bits"), } } @@ -586,12 +587,6 @@ impl<'tcx, Tag> ScalarMaybeUndef { } } - /// Do not call this method! Use either `assert_bits` or `force_bits`. - #[inline(always)] - pub fn to_bits(self, target_size: Size) -> InterpResult<'tcx, u128> { - self.not_undef()?.to_bits(target_size) - } - #[inline(always)] pub fn to_bool(self) -> InterpResult<'tcx, bool> { self.not_undef()?.to_bool() diff --git a/src/librustc/ty/sty.rs b/src/librustc/ty/sty.rs index c3698f402a9d1..241781bb93d54 100644 --- a/src/librustc/ty/sty.rs +++ b/src/librustc/ty/sty.rs @@ -2576,7 +2576,7 @@ impl<'tcx> ConstKind<'tcx> { #[inline] pub fn try_to_bits(&self, size: ty::layout::Size) -> Option { - self.try_to_scalar()?.to_bits(size).ok() + if let ConstKind::Value(val) = self { val.try_to_bits(size) } else { None } } } diff --git a/src/librustc_mir/interpret/intrinsics.rs b/src/librustc_mir/interpret/intrinsics.rs index cd06cf01bfa81..d63abdc356234 100644 --- a/src/librustc_mir/interpret/intrinsics.rs +++ b/src/librustc_mir/interpret/intrinsics.rs @@ -384,7 +384,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { // `x % y != 0` or `y == 0` or `x == T::min_value() && y == -1`. // First, check x % y != 0 (or if that computation overflows). let (res, overflow, _ty) = self.overflowing_binary_op(BinOp::Rem, a, b)?; - if overflow || res.to_bits(a.layout.size)? != 0 { + if overflow || res.assert_bits(a.layout.size) != 0 { // Then, check if `b` is -1, which is the "min_value / -1" case. let minus1 = Scalar::from_int(-1, dest.layout.size); let b_scalar = b.to_scalar().unwrap(); diff --git a/src/librustc_mir/interpret/operand.rs b/src/librustc_mir/interpret/operand.rs index 14b8a341e26a0..20efe7dfc0367 100644 --- a/src/librustc_mir/interpret/operand.rs +++ b/src/librustc_mir/interpret/operand.rs @@ -96,40 +96,40 @@ pub struct ImmTy<'tcx, Tag = ()> { impl std::fmt::Display for ImmTy<'tcx, Tag> { fn fmt(&self, fmt: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match &self.imm { - Immediate::Scalar(ScalarMaybeUndef::Scalar(s)) => match s.to_bits(self.layout.size) { - Ok(s) => { - match self.layout.ty.kind { - ty::Int(_) => { - return write!( - fmt, - "{}", - super::sign_extend(s, self.layout.size) as i128, - ); - } - ty::Uint(_) => return write!(fmt, "{}", s), - ty::Bool if s == 0 => return fmt.write_str("false"), - ty::Bool if s == 1 => return fmt.write_str("true"), - ty::Char => { - if let Some(c) = u32::try_from(s).ok().and_then(std::char::from_u32) { - return write!(fmt, "{}", c); - } + // We cannot use `to_bits_or_ptr` as we do not have a `tcx`. + // So we use `is_bits` and circumvent a bunch of sanity checking -- but + // this is anyway only for printing. + Immediate::Scalar(ScalarMaybeUndef::Scalar(s)) if s.is_ptr() => { + fmt.write_str("{pointer}") + } + Immediate::Scalar(ScalarMaybeUndef::Scalar(s)) => { + let s = s.assert_bits(self.layout.size); + match self.layout.ty.kind { + ty::Int(_) => { + return write!(fmt, "{}", super::sign_extend(s, self.layout.size) as i128,); + } + ty::Uint(_) => return write!(fmt, "{}", s), + ty::Bool if s == 0 => return fmt.write_str("false"), + ty::Bool if s == 1 => return fmt.write_str("true"), + ty::Char => { + if let Some(c) = u32::try_from(s).ok().and_then(std::char::from_u32) { + return write!(fmt, "{}", c); } - ty::Float(ast::FloatTy::F32) => { - if let Ok(u) = u32::try_from(s) { - return write!(fmt, "{}", f32::from_bits(u)); - } + } + ty::Float(ast::FloatTy::F32) => { + if let Ok(u) = u32::try_from(s) { + return write!(fmt, "{}", f32::from_bits(u)); } - ty::Float(ast::FloatTy::F64) => { - if let Ok(u) = u64::try_from(s) { - return write!(fmt, "{}", f64::from_bits(u)); - } + } + ty::Float(ast::FloatTy::F64) => { + if let Ok(u) = u64::try_from(s) { + return write!(fmt, "{}", f64::from_bits(u)); } - _ => {} } - write!(fmt, "{:x}", s) + _ => {} } - Err(_) => fmt.write_str("{pointer}"), - }, + write!(fmt, "{:x}", s) + } Immediate::Scalar(ScalarMaybeUndef::Undef) => fmt.write_str("{undef}"), Immediate::ScalarPair(..) => fmt.write_str("{wide pointer or tuple}"), } @@ -205,11 +205,6 @@ impl<'tcx, Tag: Copy> ImmTy<'tcx, Tag> { pub fn from_int(i: impl Into, layout: TyLayout<'tcx>) -> Self { Self::from_scalar(Scalar::from_int(i, layout.size), layout) } - - #[inline] - pub fn to_bits(self) -> InterpResult<'tcx, u128> { - self.to_scalar()?.to_bits(self.layout.size) - } } // Use the existing layout if given (but sanity check in debug mode), diff --git a/src/librustc_mir/interpret/validity.rs b/src/librustc_mir/interpret/validity.rs index aa2b3040a716f..48c0e7930e97c 100644 --- a/src/librustc_mir/interpret/validity.rs +++ b/src/librustc_mir/interpret/validity.rs @@ -361,16 +361,17 @@ impl<'rt, 'mir, 'tcx, M: Machine<'mir, 'tcx>> ValueVisitor<'mir, 'tcx, M> ty::Float(_) | ty::Int(_) | ty::Uint(_) => { // NOTE: Keep this in sync with the array optimization for int/float // types below! - let size = value.layout.size; let value = value.to_scalar_or_undef(); if self.ref_tracking_for_consts.is_some() { // Integers/floats in CTFE: Must be scalar bits, pointers are dangerous - try_validation!( - value.to_bits(size), - value, - self.path, - "initialized plain (non-pointer) bytes" - ); + let is_bits = value.not_undef().map_or(false, |v| v.is_bits()); + if !is_bits { + throw_validation_failure!( + value, + self.path, + "initialized plain (non-pointer) bytes" + ) + } } else { // At run-time, for now, we accept *anything* for these types, including // undef. We should fix that, but let's start low. diff --git a/src/librustc_mir/transform/const_prop.rs b/src/librustc_mir/transform/const_prop.rs index 9e05133132e05..fe736321df745 100644 --- a/src/librustc_mir/transform/const_prop.rs +++ b/src/librustc_mir/transform/const_prop.rs @@ -557,7 +557,9 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { let left_ty = left.ty(&self.local_decls, self.tcx); let left_size_bits = self.ecx.layout_of(left_ty).ok()?.size.bits(); let right_size = r.layout.size; - let r_bits = r.to_scalar().and_then(|r| r.to_bits(right_size)); + let r_bits = r.to_scalar().ok(); + // This is basically `force_bits`. + let r_bits = r_bits.and_then(|r| r.to_bits_or_ptr(right_size, &self.tcx).ok()); if r_bits.map_or(false, |b| b >= left_size_bits as u128) { self.report_assert_as_lint( lint::builtin::ARITHMETIC_OVERFLOW, From 0e157380ae2a7af0c457aea1c8be22161899ea1d Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Fri, 28 Feb 2020 14:08:48 +0100 Subject: [PATCH 5/5] move ZST assertion up, for better errors --- src/librustc/mir/interpret/value.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/librustc/mir/interpret/value.rs b/src/librustc/mir/interpret/value.rs index c931fcf71e39d..2be60d35af330 100644 --- a/src/librustc/mir/interpret/value.rs +++ b/src/librustc/mir/interpret/value.rs @@ -368,10 +368,10 @@ impl<'tcx, Tag> Scalar { target_size: Size, cx: &impl HasDataLayout, ) -> Result> { + assert_ne!(target_size.bytes(), 0, "you should never look at the bits of a ZST"); match self { Scalar::Raw { data, size } => { assert_eq!(target_size.bytes(), size as u64); - assert_ne!(size, 0, "you should never look at the bits of a ZST"); Scalar::check_data(data, size); Ok(data) } @@ -386,10 +386,10 @@ impl<'tcx, Tag> Scalar { /// It is just a helper for other methods in this file. #[inline] fn to_bits(self, target_size: Size) -> InterpResult<'tcx, u128> { + assert_ne!(target_size.bytes(), 0, "you should never look at the bits of a ZST"); match self { Scalar::Raw { data, size } => { assert_eq!(target_size.bytes(), size as u64); - assert_ne!(size, 0, "you should never look at the bits of a ZST"); Scalar::check_data(data, size); Ok(data) }