bjoernager/rust
bjoernager/rust
1
Fork 0
Code Activity

Rollup merge of #96714 - RalfJung:scalar-pair-debug, r=oli-obk

Browse source 
interpret/validity: debug-check ScalarPair layout information

This would have caught https://github.com/rust-lang/rust/issues/96158.
I ran the Miri test suite and it still passes.

r? `@oli-obk`
This commit is contained in:
Matthias Krüger 2022-05-05 15:43:07 +02:00 • committed by GitHub
commit 68048199c9
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
5 changed files with 99 additions and 53 deletions
Download patch file Download diff file Expand all files Collapse all files

84
compiler/rustc_const_eval/src/interpret/operand.rs
View file

@ -84,14 +84,18 @@ impl<'tcx, Tag: Provenance> Immediate<Tag> {
} }
#[inline] #[inline]
pub fn to_scalar_pair(self) -> InterpResult<'tcx, (Scalar<Tag>, Scalar<Tag>)> { pub fn to_scalar_or_uninit_pair(self) -> (ScalarMaybeUninit<Tag>, ScalarMaybeUninit<Tag>) {
match self { match self {
Immediate::ScalarPair(val1, val2) => Ok((val1.check_init()?, val2.check_init()?)), Immediate::ScalarPair(val1, val2) => (val1, val2),
Immediate::Scalar(..) => { Immediate::Scalar(..) => bug!("Got a scalar where a scalar pair was expected"),
bug!("Got a scalar where a scalar pair was expected")
}
} }
} }
#[inline]
pub fn to_scalar_pair(self) -> InterpResult<'tcx, (Scalar<Tag>, Scalar<Tag>)> {
let (val1, val2) = self.to_scalar_or_uninit_pair();
Ok((val1.check_init()?, val2.check_init()?))
}
} }
// ScalarPair needs a type to interpret, so we often have an immediate and a type together // ScalarPair needs a type to interpret, so we often have an immediate and a type together
@ -248,9 +252,12 @@ impl<'tcx, Tag: Provenance> ImmTy<'tcx, Tag> {
impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
/// Try reading an immediate in memory; this is interesting particularly for `ScalarPair`. /// Try reading an immediate in memory; this is interesting particularly for `ScalarPair`.
/// Returns `None` if the layout does not permit loading this as a value. /// Returns `None` if the layout does not permit loading this as a value.
fn try_read_immediate_from_mplace( ///
/// This is an internal function; call `read_immediate` instead.
fn read_immediate_from_mplace_raw(
&self, &self,
mplace: &MPlaceTy<'tcx, M::PointerTag>, mplace: &MPlaceTy<'tcx, M::PointerTag>,
force: bool,
) -> InterpResult<'tcx, Option<ImmTy<'tcx, M::PointerTag>>> { ) -> InterpResult<'tcx, Option<ImmTy<'tcx, M::PointerTag>>> {
if mplace.layout.is_unsized() { if mplace.layout.is_unsized() {
// Don't touch unsized // Don't touch unsized
@ -271,42 +278,61 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
// case where some of the bytes are initialized and others are not. So, we need an extra // case where some of the bytes are initialized and others are not. So, we need an extra
// check that walks over the type of `mplace` to make sure it is truly correct to treat this // check that walks over the type of `mplace` to make sure it is truly correct to treat this
// like a `Scalar` (or `ScalarPair`). // like a `Scalar` (or `ScalarPair`).
match mplace.layout.abi { let scalar_layout = match mplace.layout.abi {
Abi::Scalar(abi::Scalar::Initialized { .. }) => { // `if` does not work nested inside patterns, making this a bit awkward to express.
let scalar = alloc.read_scalar(alloc_range(Size::ZERO, mplace.layout.size))?; Abi::Scalar(abi::Scalar::Initialized { value: s, .. }) => Some(s),
Ok(Some(ImmTy { imm: scalar.into(), layout: mplace.layout })) Abi::Scalar(s) if force => Some(s.primitive()),
} _ => None,
};
if let Some(_) = scalar_layout {
let scalar = alloc.read_scalar(alloc_range(Size::ZERO, mplace.layout.size))?;
return Ok(Some(ImmTy { imm: scalar.into(), layout: mplace.layout }));
}
let scalar_pair_layout = match mplace.layout.abi {
Abi::ScalarPair( Abi::ScalarPair(
abi::Scalar::Initialized { value: a, .. }, abi::Scalar::Initialized { value: a, .. },
abi::Scalar::Initialized { value: b, .. }, abi::Scalar::Initialized { value: b, .. },
) => { ) => Some((a, b)),
// We checked `ptr_align` above, so all fields will have the alignment they need. Abi::ScalarPair(a, b) if force => Some((a.primitive(), b.primitive())),
// We would anyway check against `ptr_align.restrict_for_offset(b_offset)`, _ => None,
// which `ptr.offset(b_offset)` cannot possibly fail to satisfy. };
let (a_size, b_size) = (a.size(self), b.size(self)); if let Some((a, b)) = scalar_pair_layout {
let b_offset = a_size.align_to(b.align(self).abi); // We checked `ptr_align` above, so all fields will have the alignment they need.
assert!(b_offset.bytes() > 0); // we later use the offset to tell apart the fields // We would anyway check against `ptr_align.restrict_for_offset(b_offset)`,
let a_val = alloc.read_scalar(alloc_range(Size::ZERO, a_size))?; // which `ptr.offset(b_offset)` cannot possibly fail to satisfy.
let b_val = alloc.read_scalar(alloc_range(b_offset, b_size))?; let (a_size, b_size) = (a.size(self), b.size(self));
Ok(Some(ImmTy { imm: Immediate::ScalarPair(a_val, b_val), layout: mplace.layout })) let b_offset = a_size.align_to(b.align(self).abi);
} assert!(b_offset.bytes() > 0); // we later use the offset to tell apart the fields
_ => Ok(None), let a_val = alloc.read_scalar(alloc_range(Size::ZERO, a_size))?;
let b_val = alloc.read_scalar(alloc_range(b_offset, b_size))?;
return Ok(Some(ImmTy {
imm: Immediate::ScalarPair(a_val, b_val),
layout: mplace.layout,
}));
} }
// Neither a scalar nor scalar pair.
return Ok(None);
} }
/// Try returning an immediate for the operand. /// Try returning an immediate for the operand. If the layout does not permit loading this as an
/// If the layout does not permit loading this as an immediate, return where in memory /// immediate, return where in memory we can find the data.
/// we can find the data.
/// Note that for a given layout, this operation will either always fail or always /// Note that for a given layout, this operation will either always fail or always
/// succeed! Whether it succeeds depends on whether the layout can be represented /// succeed! Whether it succeeds depends on whether the layout can be represented
/// in an `Immediate`, not on which data is stored there currently. /// in an `Immediate`, not on which data is stored there currently.
pub fn try_read_immediate( ///
/// If `force` is `true`, then even scalars with fields that can be ununit will be
/// read. This means the load is lossy and should not be written back!
/// This flag exists only for validity checking.
///
/// This is an internal function that should not usually be used; call `read_immediate` instead.
pub fn read_immediate_raw(
&self, &self,
src: &OpTy<'tcx, M::PointerTag>, src: &OpTy<'tcx, M::PointerTag>,
force: bool,
) -> InterpResult<'tcx, Result<ImmTy<'tcx, M::PointerTag>, MPlaceTy<'tcx, M::PointerTag>>> { ) -> InterpResult<'tcx, Result<ImmTy<'tcx, M::PointerTag>, MPlaceTy<'tcx, M::PointerTag>>> {
Ok(match src.try_as_mplace() { Ok(match src.try_as_mplace() {
Ok(ref mplace) => { Ok(ref mplace) => {
if let Some(val) = self.try_read_immediate_from_mplace(mplace)? { if let Some(val) = self.read_immediate_from_mplace_raw(mplace, force)? {
Ok(val) Ok(val)
} else { } else {
Err(*mplace) Err(*mplace)
@ -322,7 +348,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
&self, &self,
op: &OpTy<'tcx, M::PointerTag>, op: &OpTy<'tcx, M::PointerTag>,
) -> InterpResult<'tcx, ImmTy<'tcx, M::PointerTag>> { ) -> InterpResult<'tcx, ImmTy<'tcx, M::PointerTag>> {
if let Ok(imm) = self.try_read_immediate(op)? { if let Ok(imm) = self.read_immediate_raw(op, /*force*/ false)? {
Ok(imm) Ok(imm)
} else { } else {
span_bug!(self.cur_span(), "primitive read failed for type: {:?}", op.layout.ty); span_bug!(self.cur_span(), "primitive read failed for type: {:?}", op.layout.ty);

4
compiler/rustc_const_eval/src/interpret/place.rs
View file

@ -720,7 +720,7 @@ where
} }
trace!("write_immediate: {:?} <- {:?}: {}", *dest, src, dest.layout.ty); trace!("write_immediate: {:?} <- {:?}: {}", *dest, src, dest.layout.ty);
// See if we can avoid an allocation. This is the counterpart to `try_read_immediate`, // See if we can avoid an allocation. This is the counterpart to `read_immediate_raw`,
// but not factored as a separate function. // but not factored as a separate function.
let mplace = match dest.place { let mplace = match dest.place {
Place::Local { frame, local } => { Place::Local { frame, local } => {
@ -879,7 +879,7 @@ where
} }
// Let us see if the layout is simple so we take a shortcut, avoid force_allocation. // Let us see if the layout is simple so we take a shortcut, avoid force_allocation.
let src = match self.try_read_immediate(src)? { let src = match self.read_immediate_raw(src, /*force*/ false)? {
Ok(src_val) => { Ok(src_val) => {
assert!(!src.layout.is_unsized(), "cannot have unsized immediates"); assert!(!src.layout.is_unsized(), "cannot have unsized immediates");
// Yay, we got a value that we can write directly. // Yay, we got a value that we can write directly.

56
compiler/rustc_const_eval/src/interpret/validity.rs
View file

@ -20,8 +20,8 @@ use rustc_target::abi::{Abi, Scalar as ScalarAbi, Size, VariantIdx, Variants, Wr
use std::hash::Hash; use std::hash::Hash;
use super::{ use super::{
alloc_range, CheckInAllocMsg, GlobalAlloc, InterpCx, InterpResult, MPlaceTy, Machine, alloc_range, CheckInAllocMsg, GlobalAlloc, Immediate, InterpCx, InterpResult, MPlaceTy,
MemPlaceMeta, OpTy, Scalar, ScalarMaybeUninit, ValueVisitor, Machine, MemPlaceMeta, OpTy, Scalar, ScalarMaybeUninit, ValueVisitor,
}; };
macro_rules! throw_validation_failure { macro_rules! throw_validation_failure {
@ -487,6 +487,17 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, '
)) ))
} }
fn read_immediate_forced(
&self,
op: &OpTy<'tcx, M::PointerTag>,
) -> InterpResult<'tcx, Immediate<M::PointerTag>> {
Ok(*try_validation!(
self.ecx.read_immediate_raw(op, /*force*/ true),
self.path,
err_unsup!(ReadPointerAsBytes) => { "(potentially part of) a pointer" } expected { "plain (non-pointer) bytes" },
).unwrap())
}
/// Check if this is a value of primitive type, and if yes check the validity of the value /// Check if this is a value of primitive type, and if yes check the validity of the value
/// at that type. Return `true` if the type is indeed primitive. /// at that type. Return `true` if the type is indeed primitive.
fn try_visit_primitive( fn try_visit_primitive(
@ -626,18 +637,19 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, '
fn visit_scalar( fn visit_scalar(
&mut self, &mut self,
op: &OpTy<'tcx, M::PointerTag>, scalar: ScalarMaybeUninit<M::PointerTag>,
scalar_layout: ScalarAbi, scalar_layout: ScalarAbi,
) -> InterpResult<'tcx> { ) -> InterpResult<'tcx> {
// We check `is_full_range` in a slightly complicated way because *if* we are checking // We check `is_full_range` in a slightly complicated way because *if* we are checking
// number validity, then we want to ensure that `Scalar::Initialized` is indeed initialized, // number validity, then we want to ensure that `Scalar::Initialized` is indeed initialized,
// i.e. that we go over the `check_init` below. // i.e. that we go over the `check_init` below.
let size = scalar_layout.size(self.ecx);
let is_full_range = match scalar_layout { let is_full_range = match scalar_layout {
ScalarAbi::Initialized { valid_range, .. } => { ScalarAbi::Initialized { valid_range, .. } => {
if M::enforce_number_validity(self.ecx) { if M::enforce_number_validity(self.ecx) {
false // not "full" since uninit is not accepted false // not "full" since uninit is not accepted
} else { } else {
valid_range.is_full_for(op.layout.size) valid_range.is_full_for(size)
} }
} }
ScalarAbi::Union { .. } => true, ScalarAbi::Union { .. } => true,
@ -646,21 +658,19 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, '
// Nothing to check // Nothing to check
return Ok(()); return Ok(());
} }
// We have something to check. // We have something to check: it must at least be initialized.
let valid_range = scalar_layout.valid_range(self.ecx); let valid_range = scalar_layout.valid_range(self.ecx);
let WrappingRange { start, end } = valid_range; let WrappingRange { start, end } = valid_range;
let max_value = op.layout.size.unsigned_int_max(); let max_value = size.unsigned_int_max();
assert!(end <= max_value); assert!(end <= max_value);
// Determine the allowed range
let value = self.read_scalar(op)?;
let value = try_validation!( let value = try_validation!(
value.check_init(), scalar.check_init(),
self.path, self.path,
err_ub!(InvalidUninitBytes(None)) => { "{:x}", value } err_ub!(InvalidUninitBytes(None)) => { "{:x}", scalar }
expected { "something {}", wrapping_range_format(valid_range, max_value) }, expected { "something {}", wrapping_range_format(valid_range, max_value) },
); );
let bits = match value.try_to_int() { let bits = match value.try_to_int() {
Ok(int) => int.assert_bits(op.layout.size), Ok(int) => int.assert_bits(size),
Err(_) => { Err(_) => {
// So this is a pointer then, and casting to an int failed. // So this is a pointer then, and casting to an int failed.
// Can only happen during CTFE. // Can only happen during CTFE.
@ -678,7 +688,7 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, '
} else { } else {
return Ok(()); return Ok(());
} }
} else if scalar_layout.valid_range(self.ecx).is_full_for(op.layout.size) { } else if scalar_layout.valid_range(self.ecx).is_full_for(size) {
// Easy. (This is reachable if `enforce_number_validity` is set.) // Easy. (This is reachable if `enforce_number_validity` is set.)
return Ok(()); return Ok(());
} else { } else {
@ -817,13 +827,23 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValueVisitor<'mir, 'tcx, M>
); );
} }
Abi::Scalar(scalar_layout) => { Abi::Scalar(scalar_layout) => {
self.visit_scalar(op, scalar_layout)?; let scalar = self.read_immediate_forced(op)?.to_scalar_or_uninit();
self.visit_scalar(scalar, scalar_layout)?;
} }
Abi::ScalarPair { .. } | Abi::Vector { .. } => { Abi::ScalarPair(a_layout, b_layout) => {
// These have fields that we already visited above, so we already checked // We would validate these things as we descend into the fields,
// all their scalar-level restrictions. // but that can miss bugs in layout computation. Layout computation
// There is also no equivalent to `rustc_layout_scalar_valid_range_start` // is subtle due to enums having ScalarPair layout, where one field
// that would make skipping them here an issue. // is the discriminant.
if cfg!(debug_assertions) {
let (a, b) = self.read_immediate_forced(op)?.to_scalar_or_uninit_pair();
self.visit_scalar(a, a_layout)?;
self.visit_scalar(b, b_layout)?;
}
}
Abi::Vector { .. } => {
// No checks here, we assume layout computation gets this right.
// (This is harder to check since Miri does not represent these as `Immediate`.)
} }
Abi::Aggregate { .. } => { Abi::Aggregate { .. } => {
// Nothing to do. // Nothing to do.

6
compiler/rustc_mir_transform/src/const_prop.rs
View file

@ -415,7 +415,7 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
// Try to read the local as an immediate so that if it is representable as a scalar, we can // Try to read the local as an immediate so that if it is representable as a scalar, we can
// handle it as such, but otherwise, just return the value as is. // handle it as such, but otherwise, just return the value as is.
Some(match self.ecx.try_read_immediate(&op) { Some(match self.ecx.read_immediate_raw(&op, /*force*/ false) {
Ok(Ok(imm)) => imm.into(), Ok(Ok(imm)) => imm.into(),
_ => op, _ => op,
}) })
@ -709,8 +709,8 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
return; return;
} }
// FIXME> figure out what to do when try_read_immediate fails // FIXME> figure out what to do when read_immediate_raw fails
let imm = self.use_ecx(|this| this.ecx.try_read_immediate(value)); let imm = self.use_ecx(|this| this.ecx.read_immediate_raw(value, /*force*/ false));
if let Some(Ok(imm)) = imm { if let Some(Ok(imm)) = imm {
match *imm { match *imm {

2
compiler/rustc_mir_transform/src/const_prop_lint.rs
View file

@ -412,7 +412,7 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
// Try to read the local as an immediate so that if it is representable as a scalar, we can // Try to read the local as an immediate so that if it is representable as a scalar, we can
// handle it as such, but otherwise, just return the value as is. // handle it as such, but otherwise, just return the value as is.
Some(match self.ecx.try_read_immediate(&op) { Some(match self.ecx.read_immediate_raw(&op, /*force*/ false) {
Ok(Ok(imm)) => imm.into(), Ok(Ok(imm)) => imm.into(),
_ => op, _ => op,
}) })