From 351dfa2f4611eb849bdc24cc6a0dce9c65bc1348 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sat, 26 Feb 2022 11:04:10 -0500 Subject: [PATCH 1/2] Miri/CTFE engine: do not treat unions as scalar values --- .../src/const_eval/eval_queries.rs | 3 +- .../rustc_const_eval/src/interpret/mod.rs | 1 + .../rustc_const_eval/src/interpret/operand.rs | 63 ++++++++++++++++++- 3 files changed, 63 insertions(+), 4 deletions(-) diff --git a/compiler/rustc_const_eval/src/const_eval/eval_queries.rs b/compiler/rustc_const_eval/src/const_eval/eval_queries.rs index dad572741049b..78a50def0e934 100644 --- a/compiler/rustc_const_eval/src/const_eval/eval_queries.rs +++ b/compiler/rustc_const_eval/src/const_eval/eval_queries.rs @@ -118,8 +118,9 @@ pub(super) fn op_to_const<'tcx>( // instead allow `ConstValue::Scalar` to store `ScalarMaybeUninit`, but that would affect all // the usual cases of extracting e.g. a `usize`, without there being a real use case for the // `Undef` situation. + // FIXME: This is all a horrible hack and should go away once we are properly using valtrees. let try_as_immediate = match op.layout.abi { - Abi::Scalar(..) => true, + Abi::Scalar(..) if crate::interpret::type_is_scalar(ecx.tcx.tcx, op.layout.ty) => true, Abi::ScalarPair(..) => match op.layout.ty.kind() { ty::Ref(_, inner, _) => match *inner.kind() { ty::Slice(elem) => elem == ecx.tcx.types.u8, diff --git a/compiler/rustc_const_eval/src/interpret/mod.rs b/compiler/rustc_const_eval/src/interpret/mod.rs index 2b9fe56599715..6003fb7e8bd4d 100644 --- a/compiler/rustc_const_eval/src/interpret/mod.rs +++ b/compiler/rustc_const_eval/src/interpret/mod.rs @@ -30,4 +30,5 @@ pub use self::validity::{CtfeValidationMode, RefTracking}; pub use self::visitor::{MutValueVisitor, ValueVisitor}; crate use self::intrinsics::eval_nullary_intrinsic; +crate use self::operand::type_is_scalar; use eval_context::{from_known_layout, mir_assign_valid_types}; diff --git a/compiler/rustc_const_eval/src/interpret/operand.rs b/compiler/rustc_const_eval/src/interpret/operand.rs index 9b317e8e0abfb..9fd1fbe8bd519 100644 --- a/compiler/rustc_const_eval/src/interpret/operand.rs +++ b/compiler/rustc_const_eval/src/interpret/operand.rs @@ -9,7 +9,7 @@ use rustc_hir::def::Namespace; use rustc_macros::HashStable; use rustc_middle::ty::layout::{LayoutOf, PrimitiveExt, TyAndLayout}; use rustc_middle::ty::print::{FmtPrinter, PrettyPrinter, Printer}; -use rustc_middle::ty::{ConstInt, Ty}; +use rustc_middle::ty::{ConstInt, Ty, TyCtxt}; use rustc_middle::{mir, ty}; use rustc_target::abi::{Abi, HasDataLayout, Size, TagEncoding}; use rustc_target::abi::{VariantIdx, Variants}; @@ -246,6 +246,57 @@ impl<'tcx, Tag: Provenance> ImmTy<'tcx, Tag> { } } +/// See `try_read_immediate_from_mplace` for what this function is about. +/// Exposed to the rest of the crate since some hacky code in `eval_queries.rs` (that shouldn't +/// exist) also needs it. +pub(crate) fn type_is_scalar<'tcx>(tcx: TyCtxt<'tcx>, ty: Ty<'tcx>) -> bool { + match ty.kind() { + // Primitive types. We *have to* handle these since the primitive operations that + // act on them require it. + ty::Bool + | ty::Char + | ty::Float(_) + | ty::Int(_) + | ty::Uint(_) + | ty::RawPtr(..) + | ty::Ref(..) + | ty::FnPtr(..) + | ty::FnDef(..) + | ty::Never => true, + // Compound types. We have to exclude unions here. + ty::Adt(adt_def, substs) => { + if adt_def.is_union() { + // Unions are explicitly allowed to be partially initialized, so we have to + // exclude them here. + false + } else { + // Check all fields of all variants, to make sure there is no union hiding here. + adt_def.all_fields().all(|field| type_is_scalar(tcx, field.ty(tcx, substs))) + } + } + ty::Tuple(..) => { + // If all fields are scalar, we are good. + ty.tuple_fields().iter().all(|field| type_is_scalar(tcx, field)) + } + // FIXME: can these ever have Scalar ABI? + ty::Closure(..) | ty::Generator(..) => false, + // Types that don't have scalar layout to begin with. + ty::Array(..) | ty::Slice(..) | ty::Str | ty::Dynamic(..) | ty::Foreign(..) => { + false + } + // Types we should not uusally see here, but when called from CTFE op_to_const these can + // actually happen. + ty::Error(_) + | ty::Infer(..) + | ty::Placeholder(..) + | ty::Bound(..) + | ty::Param(..) + | ty::Opaque(..) + | ty::Projection(..) + | ty::GeneratorWitness(..) => false, + } +} + impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { /// 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. @@ -266,12 +317,18 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { })); }; + // It may seem like all types with `Scalar` or `ScalarPair` ABI are fair game at this point. + // However, `MaybeUninit` is considered a `Scalar` as far as its layout is concerned -- + // and yet cannot be represented by an interpreter `Scalar`, since we have to handle the + // 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 + // like a `Scalar` (or `ScalarPair`). match mplace.layout.abi { - Abi::Scalar(..) => { + Abi::Scalar(..) if type_is_scalar(self.tcx.tcx, mplace.layout.ty) => { let scalar = alloc.read_scalar(alloc_range(Size::ZERO, mplace.layout.size))?; Ok(Some(ImmTy { imm: scalar.into(), layout: mplace.layout })) } - Abi::ScalarPair(a, b) => { + Abi::ScalarPair(a, b) if type_is_scalar(self.tcx.tcx, mplace.layout.ty) => { // We checked `ptr_align` above, so all fields will have the alignment they need. // We would anyway check against `ptr_align.restrict_for_offset(b_offset)`, // which `ptr.offset(b_offset)` cannot possibly fail to satisfy. From ed7ca445c74909a20f3d0529255d2eadd62aa321 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sat, 26 Feb 2022 17:44:31 -0500 Subject: [PATCH 2/2] add tests --- .../rustc_const_eval/src/interpret/operand.rs | 4 +-- src/test/ui/consts/issue-69488.rs | 33 +++++++++++++++++++ src/test/ui/consts/issue-94371.rs | 16 +++++++++ 3 files changed, 50 insertions(+), 3 deletions(-) create mode 100644 src/test/ui/consts/issue-69488.rs create mode 100644 src/test/ui/consts/issue-94371.rs diff --git a/compiler/rustc_const_eval/src/interpret/operand.rs b/compiler/rustc_const_eval/src/interpret/operand.rs index 9fd1fbe8bd519..524d3c20631ab 100644 --- a/compiler/rustc_const_eval/src/interpret/operand.rs +++ b/compiler/rustc_const_eval/src/interpret/operand.rs @@ -281,9 +281,7 @@ pub(crate) fn type_is_scalar<'tcx>(tcx: TyCtxt<'tcx>, ty: Ty<'tcx>) -> bool { // FIXME: can these ever have Scalar ABI? ty::Closure(..) | ty::Generator(..) => false, // Types that don't have scalar layout to begin with. - ty::Array(..) | ty::Slice(..) | ty::Str | ty::Dynamic(..) | ty::Foreign(..) => { - false - } + ty::Array(..) | ty::Slice(..) | ty::Str | ty::Dynamic(..) | ty::Foreign(..) => false, // Types we should not uusally see here, but when called from CTFE op_to_const these can // actually happen. ty::Error(_) diff --git a/src/test/ui/consts/issue-69488.rs b/src/test/ui/consts/issue-69488.rs new file mode 100644 index 0000000000000..600774d51385d --- /dev/null +++ b/src/test/ui/consts/issue-69488.rs @@ -0,0 +1,33 @@ +// run-pass + +#![feature(const_ptr_write)] +#![feature(const_fn_trait_bound)] +#![feature(const_mut_refs)] + +// Or, equivalently: `MaybeUninit`. +pub union BagOfBits { + uninit: (), + _storage: T, +} + +pub const fn make_1u8_bag() -> BagOfBits { + assert!(core::mem::size_of::() >= 1); + let mut bag = BagOfBits { uninit: () }; + unsafe { (&mut bag as *mut _ as *mut u8).write(1); }; + bag +} + +pub fn check_bag(bag: &BagOfBits) { + let val = unsafe { (bag as *const _ as *const u8).read() }; + assert_eq!(val, 1); +} + +fn main() { + check_bag(&make_1u8_bag::<[usize; 1]>()); // Fine + check_bag(&make_1u8_bag::()); // Fine + + const CONST_ARRAY_BAG: BagOfBits<[usize; 1]> = make_1u8_bag(); + check_bag(&CONST_ARRAY_BAG); // Fine. + const CONST_USIZE_BAG: BagOfBits = make_1u8_bag(); + check_bag(&CONST_USIZE_BAG); // Panics +} diff --git a/src/test/ui/consts/issue-94371.rs b/src/test/ui/consts/issue-94371.rs new file mode 100644 index 0000000000000..de9ff730b66f3 --- /dev/null +++ b/src/test/ui/consts/issue-94371.rs @@ -0,0 +1,16 @@ +// check-pass + +#![feature(const_swap)] +#![feature(const_mut_refs)] + +#[repr(C)] +struct Demo(u64, bool, u64, u32, u64, u64, u64); + +const C: (Demo, Demo) = { + let mut x = Demo(1, true, 3, 4, 5, 6, 7); + let mut y = Demo(10, false, 12, 13, 14, 15, 16); + std::mem::swap(&mut x, &mut y); + (x, y) +}; + +fn main() {}