From cabfe8f5dc6b481fa5c9422a2181958c2930a846 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Tue, 28 Nov 2023 09:59:09 +0100 Subject: [PATCH 1/2] move write_aggregate into step file, and also extract write_repeat into separate function --- .../rustc_const_eval/src/interpret/place.rs | 31 +----- .../rustc_const_eval/src/interpret/step.rs | 101 ++++++++++++------ 2 files changed, 72 insertions(+), 60 deletions(-) diff --git a/compiler/rustc_const_eval/src/interpret/place.rs b/compiler/rustc_const_eval/src/interpret/place.rs index 2ad0c9a873103..04f0cdb326e25 100644 --- a/compiler/rustc_const_eval/src/interpret/place.rs +++ b/compiler/rustc_const_eval/src/interpret/place.rs @@ -7,12 +7,11 @@ use std::assert_matches::assert_matches; use either::{Either, Left, Right}; use rustc_ast::Mutability; -use rustc_index::IndexSlice; use rustc_middle::mir; use rustc_middle::ty; use rustc_middle::ty::layout::{LayoutOf, TyAndLayout}; use rustc_middle::ty::Ty; -use rustc_target::abi::{Abi, Align, FieldIdx, HasDataLayout, Size, FIRST_VARIANT}; +use rustc_target::abi::{Abi, Align, HasDataLayout, Size}; use super::{ alloc_range, mir_assign_valid_types, AllocId, AllocRef, AllocRefMut, CheckAlignMsg, ImmTy, @@ -977,34 +976,6 @@ where Ok(self.ptr_with_meta_to_mplace(ptr.into(), MemPlaceMeta::Meta(meta), layout)) } - /// Writes the aggregate to the destination. - #[instrument(skip(self), level = "trace")] - pub fn write_aggregate( - &mut self, - kind: &mir::AggregateKind<'tcx>, - operands: &IndexSlice>, - dest: &PlaceTy<'tcx, M::Provenance>, - ) -> InterpResult<'tcx> { - self.write_uninit(dest)?; - let (variant_index, variant_dest, active_field_index) = match *kind { - mir::AggregateKind::Adt(_, variant_index, _, _, active_field_index) => { - let variant_dest = self.project_downcast(dest, variant_index)?; - (variant_index, variant_dest, active_field_index) - } - _ => (FIRST_VARIANT, dest.clone(), None), - }; - if active_field_index.is_some() { - assert_eq!(operands.len(), 1); - } - for (field_index, operand) in operands.iter_enumerated() { - let field_index = active_field_index.unwrap_or(field_index); - let field_dest = self.project_field(&variant_dest, field_index.as_usize())?; - let op = self.eval_operand(operand, Some(field_dest.layout))?; - self.copy_op(&op, &field_dest, /*allow_transmute*/ false)?; - } - self.write_discriminant(variant_index, dest) - } - pub fn raw_const_to_mplace( &self, raw: mir::ConstAlloc<'tcx>, diff --git a/compiler/rustc_const_eval/src/interpret/step.rs b/compiler/rustc_const_eval/src/interpret/step.rs index b6993d939c585..d48329b6c6946 100644 --- a/compiler/rustc_const_eval/src/interpret/step.rs +++ b/compiler/rustc_const_eval/src/interpret/step.rs @@ -4,11 +4,12 @@ use either::Either; +use rustc_index::IndexSlice; use rustc_middle::mir; -use rustc_middle::mir::interpret::{InterpResult, Scalar}; use rustc_middle::ty::layout::LayoutOf; +use rustc_target::abi::{FieldIdx, FIRST_VARIANT}; -use super::{ImmTy, InterpCx, Machine, Projectable}; +use super::{ImmTy, InterpCx, InterpResult, Machine, PlaceTy, Projectable, Scalar}; use crate::util; impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { @@ -187,34 +188,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { } Repeat(ref operand, _) => { - let src = self.eval_operand(operand, None)?; - assert!(src.layout.is_sized()); - let dest = self.force_allocation(&dest)?; - let length = dest.len(self)?; - - if length == 0 { - // Nothing to copy... but let's still make sure that `dest` as a place is valid. - self.get_place_alloc_mut(&dest)?; - } else { - // Write the src to the first element. - let first = self.project_index(&dest, 0)?; - self.copy_op(&src, &first, /*allow_transmute*/ false)?; - - // This is performance-sensitive code for big static/const arrays! So we - // avoid writing each operand individually and instead just make many copies - // of the first element. - let elem_size = first.layout.size; - let first_ptr = first.ptr(); - let rest_ptr = first_ptr.offset(elem_size, self)?; - // No alignment requirement since `copy_op` above already checked it. - self.mem_copy_repeatedly( - first_ptr, - rest_ptr, - elem_size, - length - 1, - /*nonoverlapping:*/ true, - )?; - } + self.write_repeat(operand, &dest)?; } Len(place) => { @@ -307,6 +281,73 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { Ok(()) } + /// Writes the aggregate to the destination. + #[instrument(skip(self), level = "trace")] + fn write_aggregate( + &mut self, + kind: &mir::AggregateKind<'tcx>, + operands: &IndexSlice>, + dest: &PlaceTy<'tcx, M::Provenance>, + ) -> InterpResult<'tcx> { + self.write_uninit(dest)?; // make sure all the padding ends up as uninit + let (variant_index, variant_dest, active_field_index) = match *kind { + mir::AggregateKind::Adt(_, variant_index, _, _, active_field_index) => { + let variant_dest = self.project_downcast(dest, variant_index)?; + (variant_index, variant_dest, active_field_index) + } + _ => (FIRST_VARIANT, dest.clone(), None), + }; + if active_field_index.is_some() { + assert_eq!(operands.len(), 1); + } + for (field_index, operand) in operands.iter_enumerated() { + let field_index = active_field_index.unwrap_or(field_index); + let field_dest = self.project_field(&variant_dest, field_index.as_usize())?; + let op = self.eval_operand(operand, Some(field_dest.layout))?; + self.copy_op(&op, &field_dest, /*allow_transmute*/ false)?; + } + self.write_discriminant(variant_index, dest) + } + + /// Repeats `operand` into the destination. `dest` must have array type, and that type + /// determines how often `operand` is repeated. + fn write_repeat( + &mut self, + operand: &mir::Operand<'tcx>, + dest: &PlaceTy<'tcx, M::Provenance>, + ) -> InterpResult<'tcx> { + let src = self.eval_operand(operand, None)?; + assert!(src.layout.is_sized()); + let dest = self.force_allocation(&dest)?; + let length = dest.len(self)?; + + if length == 0 { + // Nothing to copy... but let's still make sure that `dest` as a place is valid. + self.get_place_alloc_mut(&dest)?; + } else { + // Write the src to the first element. + let first = self.project_index(&dest, 0)?; + self.copy_op(&src, &first, /*allow_transmute*/ false)?; + + // This is performance-sensitive code for big static/const arrays! So we + // avoid writing each operand individually and instead just make many copies + // of the first element. + let elem_size = first.layout.size; + let first_ptr = first.ptr(); + let rest_ptr = first_ptr.offset(elem_size, self)?; + // No alignment requirement since `copy_op` above already checked it. + self.mem_copy_repeatedly( + first_ptr, + rest_ptr, + elem_size, + length - 1, + /*nonoverlapping:*/ true, + )?; + } + + Ok(()) + } + /// Evaluate the given terminator. Will also adjust the stack frame and statement position accordingly. fn terminator(&mut self, terminator: &mir::Terminator<'tcx>) -> InterpResult<'tcx> { info!("{:?}", terminator.kind); From 53eb91051519ce3c527c74a70f24e31a352238bc Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Tue, 28 Nov 2023 11:38:29 +0100 Subject: [PATCH 2/2] add test checking that aggregate assignments reset memory to uninit first --- .../fail/uninit-after-aggregate-assign.rs | 27 +++++++++++++++++++ .../fail/uninit-after-aggregate-assign.stderr | 15 +++++++++++ 2 files changed, 42 insertions(+) create mode 100644 src/tools/miri/tests/fail/uninit-after-aggregate-assign.rs create mode 100644 src/tools/miri/tests/fail/uninit-after-aggregate-assign.stderr diff --git a/src/tools/miri/tests/fail/uninit-after-aggregate-assign.rs b/src/tools/miri/tests/fail/uninit-after-aggregate-assign.rs new file mode 100644 index 0000000000000..98f9cc96fd0f9 --- /dev/null +++ b/src/tools/miri/tests/fail/uninit-after-aggregate-assign.rs @@ -0,0 +1,27 @@ +#![feature(core_intrinsics)] +#![feature(custom_mir)] + +use std::intrinsics::mir::*; +use std::ptr; + +#[repr(C)] +struct S(u8, u16); + +#[custom_mir(dialect = "runtime", phase = "optimized")] +fn main() { + mir! { + let s: S; + let sptr; + let sptr2; + let _val; + { + sptr = ptr::addr_of_mut!(s); + sptr2 = sptr as *mut [u8; 4]; + *sptr2 = [0; 4]; + *sptr = S(0, 0); // should reset the padding + _val = *sptr2; // should hence be UB + //~^ERROR: encountered uninitialized memory + Return() + } + } +} diff --git a/src/tools/miri/tests/fail/uninit-after-aggregate-assign.stderr b/src/tools/miri/tests/fail/uninit-after-aggregate-assign.stderr new file mode 100644 index 0000000000000..5f3d9bde1f244 --- /dev/null +++ b/src/tools/miri/tests/fail/uninit-after-aggregate-assign.stderr @@ -0,0 +1,15 @@ +error: Undefined Behavior: constructing invalid value at [1]: encountered uninitialized memory, but expected an integer + --> $DIR/uninit-after-aggregate-assign.rs:LL:CC + | +LL | _val = *sptr2; // should hence be UB + | ^^^^^^^^^^^^^ constructing invalid value at [1]: encountered uninitialized memory, but expected an integer + | + = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior + = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information + = note: BACKTRACE: + = note: inside `main` at $DIR/uninit-after-aggregate-assign.rs:LL:CC + +note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace + +error: aborting due to 1 previous error +