From b36163893625cd634246d34919b7dfd6cb04e572 Mon Sep 17 00:00:00 2001 From: 5225225 <5225225@mailbox.org> Date: Fri, 8 Jul 2022 00:45:37 +0100 Subject: [PATCH 1/6] Use constant eval to do strict validity checks --- Cargo.lock | 1 + .../src/intrinsics/mod.rs | 6 +- compiler/rustc_codegen_ssa/Cargo.toml | 1 + compiler/rustc_codegen_ssa/src/mir/block.rs | 14 ++-- .../src/const_eval/machine.rs | 2 +- .../src/interpret/intrinsics.rs | 62 ++++++++++-------- compiler/rustc_const_eval/src/lib.rs | 38 +++++++++++ compiler/rustc_target/src/abi/mod.rs | 22 ++----- .../intrinsics/panic-uninitialized-zeroed.rs | 65 ++++++++++++------- 9 files changed, 135 insertions(+), 76 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 35fefd2fb247e..694f422dc04a2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3680,6 +3680,7 @@ dependencies = [ "rustc_arena", "rustc_ast", "rustc_attr", + "rustc_const_eval", "rustc_data_structures", "rustc_errors", "rustc_fs_util", diff --git a/compiler/rustc_codegen_cranelift/src/intrinsics/mod.rs b/compiler/rustc_codegen_cranelift/src/intrinsics/mod.rs index 6937e658ed5ee..dfba824c04bae 100644 --- a/compiler/rustc_codegen_cranelift/src/intrinsics/mod.rs +++ b/compiler/rustc_codegen_cranelift/src/intrinsics/mod.rs @@ -675,8 +675,7 @@ fn codegen_regular_intrinsic_call<'tcx>( if intrinsic == sym::assert_zero_valid && !layout.might_permit_raw_init( fx, - InitKind::Zero, - fx.tcx.sess.opts.debugging_opts.strict_init_checks) { + InitKind::Zero) { with_no_trimmed_paths!({ crate::base::codegen_panic( @@ -691,8 +690,7 @@ fn codegen_regular_intrinsic_call<'tcx>( if intrinsic == sym::assert_uninit_valid && !layout.might_permit_raw_init( fx, - InitKind::Uninit, - fx.tcx.sess.opts.debugging_opts.strict_init_checks) { + InitKind::Uninit) { with_no_trimmed_paths!({ crate::base::codegen_panic( diff --git a/compiler/rustc_codegen_ssa/Cargo.toml b/compiler/rustc_codegen_ssa/Cargo.toml index faabea92f5a6c..81c8b9ceb136e 100644 --- a/compiler/rustc_codegen_ssa/Cargo.toml +++ b/compiler/rustc_codegen_ssa/Cargo.toml @@ -40,6 +40,7 @@ rustc_metadata = { path = "../rustc_metadata" } rustc_query_system = { path = "../rustc_query_system" } rustc_target = { path = "../rustc_target" } rustc_session = { path = "../rustc_session" } +rustc_const_eval = { path = "../rustc_const_eval" } [dependencies.object] version = "0.29.0" diff --git a/compiler/rustc_codegen_ssa/src/mir/block.rs b/compiler/rustc_codegen_ssa/src/mir/block.rs index b8e3cb32ef633..ac0a6c29c6b36 100644 --- a/compiler/rustc_codegen_ssa/src/mir/block.rs +++ b/compiler/rustc_codegen_ssa/src/mir/block.rs @@ -549,10 +549,16 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> { use AssertIntrinsic::*; let ty = instance.unwrap().substs.type_at(0); let layout = bx.layout_of(ty); - let do_panic = match intrinsic { - Inhabited => layout.abi.is_uninhabited(), - ZeroValid => !layout.might_permit_raw_init(bx, InitKind::Zero, strict_validity), - UninitValid => !layout.might_permit_raw_init(bx, InitKind::Uninit, strict_validity), + let do_panic = match (&intrinsic, strict_validity) { + (Inhabited, _) => layout.abi.is_uninhabited(), + (ZeroValid, true) => { + !rustc_const_eval::is_zero_valid(bx.tcx(), source_info.span, layout) + } + (UninitValid, true) => { + !rustc_const_eval::is_uninit_valid(bx.tcx(), source_info.span, layout) + } + (ZeroValid, false) => !layout.might_permit_raw_init(bx, InitKind::Zero), + (UninitValid, false) => !layout.might_permit_raw_init(bx, InitKind::Uninit), }; if do_panic { let msg_str = with_no_visible_paths!({ diff --git a/compiler/rustc_const_eval/src/const_eval/machine.rs b/compiler/rustc_const_eval/src/const_eval/machine.rs index 29ab1d187719c..e00e667fb71e2 100644 --- a/compiler/rustc_const_eval/src/const_eval/machine.rs +++ b/compiler/rustc_const_eval/src/const_eval/machine.rs @@ -104,7 +104,7 @@ pub struct CompileTimeInterpreter<'mir, 'tcx> { } impl<'mir, 'tcx> CompileTimeInterpreter<'mir, 'tcx> { - pub(super) fn new(const_eval_limit: Limit, can_access_statics: bool) -> Self { + pub(crate) fn new(const_eval_limit: Limit, can_access_statics: bool) -> Self { CompileTimeInterpreter { steps_remaining: const_eval_limit.0, stack: Vec::new(), diff --git a/compiler/rustc_const_eval/src/interpret/intrinsics.rs b/compiler/rustc_const_eval/src/interpret/intrinsics.rs index 93b64d9d37a49..308d63f83197e 100644 --- a/compiler/rustc_const_eval/src/interpret/intrinsics.rs +++ b/compiler/rustc_const_eval/src/interpret/intrinsics.rs @@ -413,35 +413,41 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { ), )?; } - if intrinsic_name == sym::assert_zero_valid - && !layout.might_permit_raw_init( - self, - InitKind::Zero, - self.tcx.sess.opts.debugging_opts.strict_init_checks, - ) - { - M::abort( - self, - format!( - "aborted execution: attempted to zero-initialize type `{}`, which is invalid", - ty - ), - )?; + + if intrinsic_name == sym::assert_zero_valid { + let should_panic = if self.tcx.sess.opts.debugging_opts.strict_init_checks { + !crate::is_zero_valid(*self.tcx, self.cur_span(), layout) + } else { + !layout.might_permit_raw_init(self, InitKind::Zero) + }; + + if should_panic { + M::abort( + self, + format!( + "aborted execution: attempted to zero-initialize type `{}`, which is invalid", + ty + ), + )?; + } } - if intrinsic_name == sym::assert_uninit_valid - && !layout.might_permit_raw_init( - self, - InitKind::Uninit, - self.tcx.sess.opts.debugging_opts.strict_init_checks, - ) - { - M::abort( - self, - format!( - "aborted execution: attempted to leave type `{}` uninitialized, which is invalid", - ty - ), - )?; + + if intrinsic_name == sym::assert_uninit_valid { + let should_panic = if self.tcx.sess.opts.debugging_opts.strict_init_checks { + !crate::is_uninit_valid(*self.tcx, self.cur_span(), layout) + } else { + !layout.might_permit_raw_init(self, InitKind::Uninit) + }; + + if should_panic { + M::abort( + self, + format!( + "aborted execution: attempted to leave type `{}` uninitialized, which is invalid", + ty + ), + )?; + } } } sym::simd_insert => { diff --git a/compiler/rustc_const_eval/src/lib.rs b/compiler/rustc_const_eval/src/lib.rs index d65d4f7eb720e..4eacc7b614507 100644 --- a/compiler/rustc_const_eval/src/lib.rs +++ b/compiler/rustc_const_eval/src/lib.rs @@ -60,3 +60,41 @@ pub fn provide(providers: &mut Providers) { const_eval::deref_mir_constant(tcx, param_env, value) }; } + +use crate::const_eval::CompileTimeInterpreter; +use crate::interpret::{InterpCx, MemoryKind, OpTy}; +use rustc_middle::ty::{layout::TyAndLayout, ParamEnv, TyCtxt}; +use rustc_session::Limit; +use rustc_span::Span; + +pub fn is_uninit_valid<'tcx>(tcx: TyCtxt<'tcx>, root_span: Span, ty: TyAndLayout<'tcx>) -> bool { + let machine = CompileTimeInterpreter::new(Limit::new(0), false); + let mut cx = InterpCx::new(tcx, root_span, ParamEnv::reveal_all(), machine); + let allocated = cx + .allocate(ty, MemoryKind::Machine(const_eval::MemoryKind::Heap)) + .expect("failed to allocate for uninit check"); + let ot: OpTy<'_, _> = allocated.into(); + cx.validate_operand(&ot).is_ok() +} + +pub fn is_zero_valid<'tcx>(tcx: TyCtxt<'tcx>, root_span: Span, ty: TyAndLayout<'tcx>) -> bool { + let machine = CompileTimeInterpreter::new(Limit::new(0), false); + + let mut cx = InterpCx::new(tcx, root_span, ParamEnv::reveal_all(), machine); + + // We could panic here... Or we could just return "yeah it's valid whatever". Or let + // codegen_panic_intrinsic return an error that halts compilation. + // I'm not exactly sure *when* this can fail. OOM? + let allocated = cx + .allocate(ty, MemoryKind::Machine(const_eval::MemoryKind::Heap)) + .expect("failed to allocate for uninit check"); + + // Again, unclear what to do here if it fails. + cx.write_bytes_ptr(allocated.ptr, std::iter::repeat(0_u8).take(ty.layout.size().bytes_usize())) + .expect("failed to write bytes for zero valid check"); + + let ot: OpTy<'_, _> = allocated.into(); + + // Assume that if it failed, it's a validation failure. + cx.validate_operand(&ot).is_ok() +} diff --git a/compiler/rustc_target/src/abi/mod.rs b/compiler/rustc_target/src/abi/mod.rs index d1eafd6ac5fb8..b41354db53903 100644 --- a/compiler/rustc_target/src/abi/mod.rs +++ b/compiler/rustc_target/src/abi/mod.rs @@ -1494,7 +1494,7 @@ impl<'a, Ty> TyAndLayout<'a, Ty> { /// FIXME: Once we removed all the conservatism, we could alternatively /// create an all-0/all-undef constant and run the const value validator to see if /// this is a valid value for the given type. - pub fn might_permit_raw_init(self, cx: &C, init_kind: InitKind, strict: bool) -> bool + pub fn might_permit_raw_init(self, cx: &C, init_kind: InitKind) -> bool where Self: Copy, Ty: TyAbiInterface<'a, C>, @@ -1507,13 +1507,8 @@ impl<'a, Ty> TyAndLayout<'a, Ty> { s.valid_range(cx).contains(0) } InitKind::Uninit => { - if strict { - // The type must be allowed to be uninit (which means "is a union"). - s.is_uninit_valid() - } else { - // The range must include all values. - s.is_always_valid(cx) - } + // The range must include all values. + s.is_always_valid(cx) } } }; @@ -1534,19 +1529,12 @@ impl<'a, Ty> TyAndLayout<'a, Ty> { // If we have not found an error yet, we need to recursively descend into fields. match &self.fields { FieldsShape::Primitive | FieldsShape::Union { .. } => {} - FieldsShape::Array { count, .. } => { + FieldsShape::Array { .. } => { // FIXME(#66151): For now, we are conservative and do not check arrays by default. - if strict - && *count > 0 - && !self.field(cx, 0).might_permit_raw_init(cx, init_kind, strict) - { - // Found non empty array with a type that is unhappy about this kind of initialization - return false; - } } FieldsShape::Arbitrary { offsets, .. } => { for idx in 0..offsets.len() { - if !self.field(cx, idx).might_permit_raw_init(cx, init_kind, strict) { + if !self.field(cx, idx).might_permit_raw_init(cx, init_kind) { // We found a field that is unhappy with this kind of initialization. return false; } diff --git a/src/test/ui/intrinsics/panic-uninitialized-zeroed.rs b/src/test/ui/intrinsics/panic-uninitialized-zeroed.rs index 3ffd35ecdb8da..9f020e9152954 100644 --- a/src/test/ui/intrinsics/panic-uninitialized-zeroed.rs +++ b/src/test/ui/intrinsics/panic-uninitialized-zeroed.rs @@ -57,6 +57,13 @@ enum LR_NonZero { struct ZeroSized; +#[allow(dead_code)] +#[repr(i32)] +enum ZeroIsValid { + Zero(u8) = 0, + One(NonNull<()>) = 1, +} + fn test_panic_msg(op: impl (FnOnce() -> T) + panic::UnwindSafe, msg: &str) { let err = panic::catch_unwind(op).err(); assert_eq!( @@ -152,33 +159,12 @@ fn main() { "attempted to zero-initialize type `*const dyn core::marker::Send`, which is invalid" ); - /* FIXME(#66151) we conservatively do not error here yet. - test_panic_msg( - || mem::uninitialized::(), - "attempted to leave type `LR_NonZero` uninitialized, which is invalid" - ); - test_panic_msg( - || mem::zeroed::(), - "attempted to zero-initialize type `LR_NonZero`, which is invalid" - ); - - test_panic_msg( - || mem::uninitialized::>(), - "attempted to leave type `std::mem::ManuallyDrop` uninitialized, \ - which is invalid" - ); - test_panic_msg( - || mem::zeroed::>(), - "attempted to zero-initialize type `std::mem::ManuallyDrop`, \ - which is invalid" - ); - */ - test_panic_msg( || mem::uninitialized::<(NonNull, u32, u32)>(), "attempted to leave type `(core::ptr::non_null::NonNull, u32, u32)` uninitialized, \ which is invalid" ); + test_panic_msg( || mem::zeroed::<(NonNull, u32, u32)>(), "attempted to zero-initialize type `(core::ptr::non_null::NonNull, u32, u32)`, \ @@ -196,11 +182,23 @@ fn main() { which is invalid" ); + test_panic_msg( + || mem::uninitialized::(), + "attempted to leave type `LR_NonZero` uninitialized, which is invalid" + ); + + test_panic_msg( + || mem::uninitialized::>(), + "attempted to leave type `core::mem::manually_drop::ManuallyDrop` uninitialized, \ + which is invalid" + ); + test_panic_msg( || mem::uninitialized::(), "attempted to leave type `NoNullVariant` uninitialized, \ which is invalid" ); + test_panic_msg( || mem::zeroed::(), "attempted to zero-initialize type `NoNullVariant`, \ @@ -212,10 +210,12 @@ fn main() { || mem::uninitialized::(), "attempted to leave type `bool` uninitialized, which is invalid" ); + test_panic_msg( || mem::uninitialized::(), "attempted to leave type `LR` uninitialized, which is invalid" ); + test_panic_msg( || mem::uninitialized::>(), "attempted to leave type `core::mem::manually_drop::ManuallyDrop` uninitialized, which is invalid" @@ -229,6 +229,7 @@ fn main() { let _val = mem::zeroed::>(); let _val = mem::zeroed::>>(); let _val = mem::zeroed::<[!; 0]>(); + let _val = mem::zeroed::(); let _val = mem::uninitialized::>(); let _val = mem::uninitialized::<[!; 0]>(); let _val = mem::uninitialized::<()>(); @@ -259,12 +260,32 @@ fn main() { || mem::zeroed::<[NonNull<()>; 1]>(), "attempted to zero-initialize type `[core::ptr::non_null::NonNull<()>; 1]`, which is invalid" ); + + // FIXME(#66151) we conservatively do not error here yet (by default). + test_panic_msg( + || mem::zeroed::(), + "attempted to zero-initialize type `LR_NonZero`, which is invalid" + ); + + test_panic_msg( + || mem::zeroed::>(), + "attempted to zero-initialize type `core::mem::manually_drop::ManuallyDrop`, \ + which is invalid" + ); } else { // These are UB because they have not been officially blessed, but we await the resolution // of before doing // anything about that. let _val = mem::uninitialized::(); let _val = mem::uninitialized::<*const ()>(); + + // These are UB, but best to test them to ensure we don't become unintentionally + // stricter. + + // It's currently unchecked to create invalid enums and values inside arrays. + let _val = mem::zeroed::(); + let _val = mem::zeroed::<[LR_NonZero; 1]>(); + let _val = mem::zeroed::<[NonNull<()>; 1]>(); } } } From c84cfa4649dbc927ca9d0f4e5ef315b1fb843039 Mon Sep 17 00:00:00 2001 From: 5225225 <5225225@mailbox.org> Date: Sat, 9 Jul 2022 12:03:47 +0100 Subject: [PATCH 2/6] All init checks go through rustc_const_eval, cleanup --- .../src/intrinsics/mod.rs | 14 +++-- compiler/rustc_codegen_cranelift/src/lib.rs | 1 + compiler/rustc_codegen_ssa/src/mir/block.rs | 26 +++++---- .../src/interpret/intrinsics.rs | 24 ++++---- compiler/rustc_const_eval/src/lib.rs | 58 +++++++++++-------- compiler/rustc_target/src/abi/mod.rs | 2 +- 6 files changed, 75 insertions(+), 50 deletions(-) diff --git a/compiler/rustc_codegen_cranelift/src/intrinsics/mod.rs b/compiler/rustc_codegen_cranelift/src/intrinsics/mod.rs index dfba824c04bae..8604ff2f8b3bb 100644 --- a/compiler/rustc_codegen_cranelift/src/intrinsics/mod.rs +++ b/compiler/rustc_codegen_cranelift/src/intrinsics/mod.rs @@ -673,8 +673,11 @@ fn codegen_regular_intrinsic_call<'tcx>( } if intrinsic == sym::assert_zero_valid - && !layout.might_permit_raw_init( - fx, + && !rustc_const_eval::might_permit_raw_init( + fx.tcx, + source_info.span, + layout, + fx.tcx.sess.opts.debugging_opts.strict_init_checks, InitKind::Zero) { with_no_trimmed_paths!({ @@ -688,8 +691,11 @@ fn codegen_regular_intrinsic_call<'tcx>( } if intrinsic == sym::assert_uninit_valid - && !layout.might_permit_raw_init( - fx, + && !rustc_const_eval::might_permit_raw_init( + fx.tcx, + source_info.span, + layout, + fx.tcx.sess.opts.debugging_opts.strict_init_checks, InitKind::Uninit) { with_no_trimmed_paths!({ diff --git a/compiler/rustc_codegen_cranelift/src/lib.rs b/compiler/rustc_codegen_cranelift/src/lib.rs index be2d3108c5fa9..23724538194a4 100644 --- a/compiler/rustc_codegen_cranelift/src/lib.rs +++ b/compiler/rustc_codegen_cranelift/src/lib.rs @@ -8,6 +8,7 @@ extern crate rustc_middle; extern crate rustc_ast; extern crate rustc_codegen_ssa; +extern crate rustc_const_eval; extern crate rustc_data_structures; extern crate rustc_errors; extern crate rustc_fs_util; diff --git a/compiler/rustc_codegen_ssa/src/mir/block.rs b/compiler/rustc_codegen_ssa/src/mir/block.rs index ac0a6c29c6b36..b37ae9abbd83d 100644 --- a/compiler/rustc_codegen_ssa/src/mir/block.rs +++ b/compiler/rustc_codegen_ssa/src/mir/block.rs @@ -549,16 +549,22 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> { use AssertIntrinsic::*; let ty = instance.unwrap().substs.type_at(0); let layout = bx.layout_of(ty); - let do_panic = match (&intrinsic, strict_validity) { - (Inhabited, _) => layout.abi.is_uninhabited(), - (ZeroValid, true) => { - !rustc_const_eval::is_zero_valid(bx.tcx(), source_info.span, layout) - } - (UninitValid, true) => { - !rustc_const_eval::is_uninit_valid(bx.tcx(), source_info.span, layout) - } - (ZeroValid, false) => !layout.might_permit_raw_init(bx, InitKind::Zero), - (UninitValid, false) => !layout.might_permit_raw_init(bx, InitKind::Uninit), + let do_panic = match &intrinsic { + Inhabited => layout.abi.is_uninhabited(), + ZeroValid => !rustc_const_eval::might_permit_raw_init( + bx.tcx(), + source_info.span, + layout, + strict_validity, + InitKind::Zero, + ), + UninitValid => !rustc_const_eval::might_permit_raw_init( + bx.tcx(), + source_info.span, + layout, + strict_validity, + InitKind::Uninit, + ), }; if do_panic { let msg_str = with_no_visible_paths!({ diff --git a/compiler/rustc_const_eval/src/interpret/intrinsics.rs b/compiler/rustc_const_eval/src/interpret/intrinsics.rs index 308d63f83197e..bcc07fbfaaa35 100644 --- a/compiler/rustc_const_eval/src/interpret/intrinsics.rs +++ b/compiler/rustc_const_eval/src/interpret/intrinsics.rs @@ -415,11 +415,13 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { } if intrinsic_name == sym::assert_zero_valid { - let should_panic = if self.tcx.sess.opts.debugging_opts.strict_init_checks { - !crate::is_zero_valid(*self.tcx, self.cur_span(), layout) - } else { - !layout.might_permit_raw_init(self, InitKind::Zero) - }; + let should_panic = !crate::might_permit_raw_init( + *self.tcx, + self.cur_span(), + layout, + self.tcx.sess.opts.debugging_opts.strict_init_checks, + InitKind::Zero, + ); if should_panic { M::abort( @@ -433,11 +435,13 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { } if intrinsic_name == sym::assert_uninit_valid { - let should_panic = if self.tcx.sess.opts.debugging_opts.strict_init_checks { - !crate::is_uninit_valid(*self.tcx, self.cur_span(), layout) - } else { - !layout.might_permit_raw_init(self, InitKind::Uninit) - }; + let should_panic = !crate::might_permit_raw_init( + *self.tcx, + self.cur_span(), + layout, + self.tcx.sess.opts.debugging_opts.strict_init_checks, + InitKind::Uninit, + ); if should_panic { M::abort( diff --git a/compiler/rustc_const_eval/src/lib.rs b/compiler/rustc_const_eval/src/lib.rs index 4eacc7b614507..5da76b8e7408d 100644 --- a/compiler/rustc_const_eval/src/lib.rs +++ b/compiler/rustc_const_eval/src/lib.rs @@ -63,38 +63,46 @@ pub fn provide(providers: &mut Providers) { use crate::const_eval::CompileTimeInterpreter; use crate::interpret::{InterpCx, MemoryKind, OpTy}; +use rustc_middle::ty::layout::LayoutCx; use rustc_middle::ty::{layout::TyAndLayout, ParamEnv, TyCtxt}; use rustc_session::Limit; use rustc_span::Span; +use rustc_target::abi::InitKind; -pub fn is_uninit_valid<'tcx>(tcx: TyCtxt<'tcx>, root_span: Span, ty: TyAndLayout<'tcx>) -> bool { - let machine = CompileTimeInterpreter::new(Limit::new(0), false); - let mut cx = InterpCx::new(tcx, root_span, ParamEnv::reveal_all(), machine); - let allocated = cx - .allocate(ty, MemoryKind::Machine(const_eval::MemoryKind::Heap)) - .expect("failed to allocate for uninit check"); - let ot: OpTy<'_, _> = allocated.into(); - cx.validate_operand(&ot).is_ok() -} - -pub fn is_zero_valid<'tcx>(tcx: TyCtxt<'tcx>, root_span: Span, ty: TyAndLayout<'tcx>) -> bool { - let machine = CompileTimeInterpreter::new(Limit::new(0), false); +pub fn might_permit_raw_init<'tcx>( + tcx: TyCtxt<'tcx>, + root_span: Span, + ty: TyAndLayout<'tcx>, + strict: bool, + kind: InitKind, +) -> bool { + if strict { + let machine = CompileTimeInterpreter::new(Limit::new(0), false); - let mut cx = InterpCx::new(tcx, root_span, ParamEnv::reveal_all(), machine); + let mut cx = InterpCx::new(tcx, root_span, ParamEnv::reveal_all(), machine); - // We could panic here... Or we could just return "yeah it's valid whatever". Or let - // codegen_panic_intrinsic return an error that halts compilation. - // I'm not exactly sure *when* this can fail. OOM? - let allocated = cx - .allocate(ty, MemoryKind::Machine(const_eval::MemoryKind::Heap)) - .expect("failed to allocate for uninit check"); + // We could panic here... Or we could just return "yeah it's valid whatever". Or let + // codegen_panic_intrinsic return an error that halts compilation. + // I'm not exactly sure *when* this can fail. OOM? + let allocated = cx + .allocate(ty, MemoryKind::Machine(const_eval::MemoryKind::Heap)) + .expect("failed to allocate for uninit check"); - // Again, unclear what to do here if it fails. - cx.write_bytes_ptr(allocated.ptr, std::iter::repeat(0_u8).take(ty.layout.size().bytes_usize())) - .expect("failed to write bytes for zero valid check"); + if kind == InitKind::Zero { + // Again, unclear what to do here if it fails. + cx.write_bytes_ptr( + allocated.ptr, + std::iter::repeat(0_u8).take(ty.layout.size().bytes_usize()), + ) + .expect("failed to write bytes for zero valid check"); + } - let ot: OpTy<'_, _> = allocated.into(); + let ot: OpTy<'_, _> = allocated.into(); - // Assume that if it failed, it's a validation failure. - cx.validate_operand(&ot).is_ok() + // Assume that if it failed, it's a validation failure. + cx.validate_operand(&ot).is_ok() + } else { + let layout_cx = LayoutCx { tcx, param_env: ParamEnv::reveal_all() }; + !ty.might_permit_raw_init(&layout_cx, kind) + } } diff --git a/compiler/rustc_target/src/abi/mod.rs b/compiler/rustc_target/src/abi/mod.rs index b41354db53903..69f49570f598a 100644 --- a/compiler/rustc_target/src/abi/mod.rs +++ b/compiler/rustc_target/src/abi/mod.rs @@ -1372,7 +1372,7 @@ pub struct PointeeInfo { /// Used in `might_permit_raw_init` to indicate the kind of initialisation /// that is checked to be valid -#[derive(Copy, Clone, Debug)] +#[derive(Copy, Clone, Debug, PartialEq, Eq)] pub enum InitKind { Zero, Uninit, From ab4a80ee2322efef111ec0e03a241159dab6757b Mon Sep 17 00:00:00 2001 From: 5225225 <5225225@mailbox.org> Date: Sat, 9 Jul 2022 12:17:29 +0100 Subject: [PATCH 3/6] Adjust docs on old might_permit_raw_init, cleanup --- compiler/rustc_codegen_ssa/src/mir/block.rs | 2 +- compiler/rustc_target/src/abi/mod.rs | 14 +++++++++----- 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/compiler/rustc_codegen_ssa/src/mir/block.rs b/compiler/rustc_codegen_ssa/src/mir/block.rs index b37ae9abbd83d..4c7eb29cfabfa 100644 --- a/compiler/rustc_codegen_ssa/src/mir/block.rs +++ b/compiler/rustc_codegen_ssa/src/mir/block.rs @@ -549,7 +549,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> { use AssertIntrinsic::*; let ty = instance.unwrap().substs.type_at(0); let layout = bx.layout_of(ty); - let do_panic = match &intrinsic { + let do_panic = match intrinsic { Inhabited => layout.abi.is_uninhabited(), ZeroValid => !rustc_const_eval::might_permit_raw_init( bx.tcx(), diff --git a/compiler/rustc_target/src/abi/mod.rs b/compiler/rustc_target/src/abi/mod.rs index 69f49570f598a..2c3340ba4448b 100644 --- a/compiler/rustc_target/src/abi/mod.rs +++ b/compiler/rustc_target/src/abi/mod.rs @@ -1487,13 +1487,17 @@ impl<'a, Ty> TyAndLayout<'a, Ty> { /// /// `init_kind` indicates if the memory is zero-initialized or left uninitialized. /// - /// `strict` is an opt-in debugging flag added in #97323 that enables more checks. + /// This code is intentionally conservative, and will not detect + /// * zero init of an enum whose 0 variant does not allow zero initialization + /// * making uninitialized types who have a full valid range (ints, floats, raw pointers) + /// * Any form of invalid value being made inside an array (unless the value is uninhabited) /// - /// This is conservative: in doubt, it will answer `true`. + /// A strict form of these checks that uses const evaluation exists in + /// [`rustc_const_eval::might_permit_raw_init`], and a tracking issue for making these checks + /// stricter is . /// - /// FIXME: Once we removed all the conservatism, we could alternatively - /// create an all-0/all-undef constant and run the const value validator to see if - /// this is a valid value for the given type. + /// FIXME: Once all the conservatism is removed from here, and the checks are ran by default, + /// we can use the const evaluation checks always instead. pub fn might_permit_raw_init(self, cx: &C, init_kind: InitKind) -> bool where Self: Copy, From b398fa5180d7de6d68ff3f944002a5556e72e63b Mon Sep 17 00:00:00 2001 From: 5225225 <5225225@mailbox.org> Date: Sat, 9 Jul 2022 14:27:49 +0100 Subject: [PATCH 4/6] Cleanup, move might_permit_raw_init to own file --- .../src/intrinsics/mod.rs | 4 +- compiler/rustc_codegen_ssa/src/mir/block.rs | 6 ++- .../src/interpret/intrinsics.rs | 6 ++- compiler/rustc_const_eval/src/lib.rs | 47 +------------------ .../src/might_permit_raw_init.rs | 45 ++++++++++++++++++ 5 files changed, 56 insertions(+), 52 deletions(-) create mode 100644 compiler/rustc_const_eval/src/might_permit_raw_init.rs diff --git a/compiler/rustc_codegen_cranelift/src/intrinsics/mod.rs b/compiler/rustc_codegen_cranelift/src/intrinsics/mod.rs index 8604ff2f8b3bb..6360c14ff52bd 100644 --- a/compiler/rustc_codegen_cranelift/src/intrinsics/mod.rs +++ b/compiler/rustc_codegen_cranelift/src/intrinsics/mod.rs @@ -673,7 +673,7 @@ fn codegen_regular_intrinsic_call<'tcx>( } if intrinsic == sym::assert_zero_valid - && !rustc_const_eval::might_permit_raw_init( + && !rustc_const_eval::might_permit_raw_init::might_permit_raw_init( fx.tcx, source_info.span, layout, @@ -691,7 +691,7 @@ fn codegen_regular_intrinsic_call<'tcx>( } if intrinsic == sym::assert_uninit_valid - && !rustc_const_eval::might_permit_raw_init( + && !rustc_const_eval::might_permit_raw_init::might_permit_raw_init( fx.tcx, source_info.span, layout, diff --git a/compiler/rustc_codegen_ssa/src/mir/block.rs b/compiler/rustc_codegen_ssa/src/mir/block.rs index 4c7eb29cfabfa..1778c4b8538c9 100644 --- a/compiler/rustc_codegen_ssa/src/mir/block.rs +++ b/compiler/rustc_codegen_ssa/src/mir/block.rs @@ -546,19 +546,21 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> { _ => None, }); if let Some(intrinsic) = panic_intrinsic { + use rustc_const_eval::might_permit_raw_init::might_permit_raw_init; use AssertIntrinsic::*; + let ty = instance.unwrap().substs.type_at(0); let layout = bx.layout_of(ty); let do_panic = match intrinsic { Inhabited => layout.abi.is_uninhabited(), - ZeroValid => !rustc_const_eval::might_permit_raw_init( + ZeroValid => !might_permit_raw_init( bx.tcx(), source_info.span, layout, strict_validity, InitKind::Zero, ), - UninitValid => !rustc_const_eval::might_permit_raw_init( + UninitValid => !might_permit_raw_init( bx.tcx(), source_info.span, layout, diff --git a/compiler/rustc_const_eval/src/interpret/intrinsics.rs b/compiler/rustc_const_eval/src/interpret/intrinsics.rs index bcc07fbfaaa35..20705be359beb 100644 --- a/compiler/rustc_const_eval/src/interpret/intrinsics.rs +++ b/compiler/rustc_const_eval/src/interpret/intrinsics.rs @@ -22,6 +22,8 @@ use super::{ Pointer, }; +use crate::might_permit_raw_init::might_permit_raw_init; + mod caller_location; mod type_name; @@ -415,7 +417,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { } if intrinsic_name == sym::assert_zero_valid { - let should_panic = !crate::might_permit_raw_init( + let should_panic = !might_permit_raw_init( *self.tcx, self.cur_span(), layout, @@ -435,7 +437,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { } if intrinsic_name == sym::assert_uninit_valid { - let should_panic = !crate::might_permit_raw_init( + let should_panic = !might_permit_raw_init( *self.tcx, self.cur_span(), layout, diff --git a/compiler/rustc_const_eval/src/lib.rs b/compiler/rustc_const_eval/src/lib.rs index 5da76b8e7408d..70bfd109c437d 100644 --- a/compiler/rustc_const_eval/src/lib.rs +++ b/compiler/rustc_const_eval/src/lib.rs @@ -33,6 +33,7 @@ extern crate rustc_middle; pub mod const_eval; mod errors; pub mod interpret; +pub mod might_permit_raw_init; pub mod transform; pub mod util; @@ -60,49 +61,3 @@ pub fn provide(providers: &mut Providers) { const_eval::deref_mir_constant(tcx, param_env, value) }; } - -use crate::const_eval::CompileTimeInterpreter; -use crate::interpret::{InterpCx, MemoryKind, OpTy}; -use rustc_middle::ty::layout::LayoutCx; -use rustc_middle::ty::{layout::TyAndLayout, ParamEnv, TyCtxt}; -use rustc_session::Limit; -use rustc_span::Span; -use rustc_target::abi::InitKind; - -pub fn might_permit_raw_init<'tcx>( - tcx: TyCtxt<'tcx>, - root_span: Span, - ty: TyAndLayout<'tcx>, - strict: bool, - kind: InitKind, -) -> bool { - if strict { - let machine = CompileTimeInterpreter::new(Limit::new(0), false); - - let mut cx = InterpCx::new(tcx, root_span, ParamEnv::reveal_all(), machine); - - // We could panic here... Or we could just return "yeah it's valid whatever". Or let - // codegen_panic_intrinsic return an error that halts compilation. - // I'm not exactly sure *when* this can fail. OOM? - let allocated = cx - .allocate(ty, MemoryKind::Machine(const_eval::MemoryKind::Heap)) - .expect("failed to allocate for uninit check"); - - if kind == InitKind::Zero { - // Again, unclear what to do here if it fails. - cx.write_bytes_ptr( - allocated.ptr, - std::iter::repeat(0_u8).take(ty.layout.size().bytes_usize()), - ) - .expect("failed to write bytes for zero valid check"); - } - - let ot: OpTy<'_, _> = allocated.into(); - - // Assume that if it failed, it's a validation failure. - cx.validate_operand(&ot).is_ok() - } else { - let layout_cx = LayoutCx { tcx, param_env: ParamEnv::reveal_all() }; - !ty.might_permit_raw_init(&layout_cx, kind) - } -} diff --git a/compiler/rustc_const_eval/src/might_permit_raw_init.rs b/compiler/rustc_const_eval/src/might_permit_raw_init.rs new file mode 100644 index 0000000000000..cd900957bb3af --- /dev/null +++ b/compiler/rustc_const_eval/src/might_permit_raw_init.rs @@ -0,0 +1,45 @@ +use crate::const_eval::CompileTimeInterpreter; +use crate::interpret::{InterpCx, MemoryKind, OpTy}; +use rustc_middle::ty::layout::LayoutCx; +use rustc_middle::ty::{layout::TyAndLayout, ParamEnv, TyCtxt}; +use rustc_session::Limit; +use rustc_span::Span; +use rustc_target::abi::InitKind; + +pub fn might_permit_raw_init<'tcx>( + tcx: TyCtxt<'tcx>, + root_span: Span, + ty: TyAndLayout<'tcx>, + strict: bool, + kind: InitKind, +) -> bool { + if strict { + let machine = CompileTimeInterpreter::new(Limit::new(0), false); + + let mut cx = InterpCx::new(tcx, root_span, ParamEnv::reveal_all(), machine); + + // We could panic here... Or we could just return "yeah it's valid whatever". Or let + // codegen_panic_intrinsic return an error that halts compilation. + // I'm not exactly sure *when* this can fail. OOM? + let allocated = cx + .allocate(ty, MemoryKind::Machine(crate::const_eval::MemoryKind::Heap)) + .expect("failed to allocate for uninit check"); + + if kind == InitKind::Zero { + // Again, unclear what to do here if it fails. + cx.write_bytes_ptr( + allocated.ptr, + std::iter::repeat(0_u8).take(ty.layout.size().bytes_usize()), + ) + .expect("failed to write bytes for zero valid check"); + } + + let ot: OpTy<'_, _> = allocated.into(); + + // Assume that if it failed, it's a validation failure. + cx.validate_operand(&ot).is_ok() + } else { + let layout_cx = LayoutCx { tcx, param_env: ParamEnv::reveal_all() }; + ty.might_permit_raw_init(&layout_cx, kind) + } +} From b7c18687b3807655c54a2b7d4ae473e5975a7772 Mon Sep 17 00:00:00 2001 From: 5225225 <5225225@mailbox.org> Date: Mon, 11 Jul 2022 12:57:35 +0100 Subject: [PATCH 5/6] Cleanup not needed arguments --- .../src/intrinsics/mod.rs | 15 +++------------ compiler/rustc_codegen_ssa/src/mir/block.rs | 18 ++---------------- .../src/interpret/intrinsics.rs | 16 ++-------------- .../src/might_permit_raw_init.rs | 7 +++---- 4 files changed, 10 insertions(+), 46 deletions(-) diff --git a/compiler/rustc_codegen_cranelift/src/intrinsics/mod.rs b/compiler/rustc_codegen_cranelift/src/intrinsics/mod.rs index 6360c14ff52bd..9201bb035eadc 100644 --- a/compiler/rustc_codegen_cranelift/src/intrinsics/mod.rs +++ b/compiler/rustc_codegen_cranelift/src/intrinsics/mod.rs @@ -55,6 +55,7 @@ mod simd; pub(crate) use cpuid::codegen_cpuid_call; pub(crate) use llvm::codegen_llvm_intrinsic_call; +use rustc_const_eval::might_permit_raw_init::might_permit_raw_init; use rustc_middle::ty::print::with_no_trimmed_paths; use rustc_middle::ty::subst::SubstsRef; use rustc_span::symbol::{kw, sym, Symbol}; @@ -673,12 +674,7 @@ fn codegen_regular_intrinsic_call<'tcx>( } if intrinsic == sym::assert_zero_valid - && !rustc_const_eval::might_permit_raw_init::might_permit_raw_init( - fx.tcx, - source_info.span, - layout, - fx.tcx.sess.opts.debugging_opts.strict_init_checks, - InitKind::Zero) { + && !might_permit_raw_init(fx.tcx, layout, InitKind::Zero) { with_no_trimmed_paths!({ crate::base::codegen_panic( @@ -691,12 +687,7 @@ fn codegen_regular_intrinsic_call<'tcx>( } if intrinsic == sym::assert_uninit_valid - && !rustc_const_eval::might_permit_raw_init::might_permit_raw_init( - fx.tcx, - source_info.span, - layout, - fx.tcx.sess.opts.debugging_opts.strict_init_checks, - InitKind::Uninit) { + && !might_permit_raw_init(fx.tcx, layout, InitKind::Uninit) { with_no_trimmed_paths!({ crate::base::codegen_panic( diff --git a/compiler/rustc_codegen_ssa/src/mir/block.rs b/compiler/rustc_codegen_ssa/src/mir/block.rs index 1778c4b8538c9..a0a9065180a65 100644 --- a/compiler/rustc_codegen_ssa/src/mir/block.rs +++ b/compiler/rustc_codegen_ssa/src/mir/block.rs @@ -528,7 +528,6 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> { source_info: mir::SourceInfo, target: Option, cleanup: Option, - strict_validity: bool, ) -> bool { // Emit a panic or a no-op for `assert_*` intrinsics. // These are intrinsics that compile to panics so that we can get a message @@ -553,20 +552,8 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> { let layout = bx.layout_of(ty); let do_panic = match intrinsic { Inhabited => layout.abi.is_uninhabited(), - ZeroValid => !might_permit_raw_init( - bx.tcx(), - source_info.span, - layout, - strict_validity, - InitKind::Zero, - ), - UninitValid => !might_permit_raw_init( - bx.tcx(), - source_info.span, - layout, - strict_validity, - InitKind::Uninit, - ), + ZeroValid => !might_permit_raw_init(bx.tcx(), layout, InitKind::Zero), + UninitValid => !might_permit_raw_init(bx.tcx(), layout, InitKind::Uninit), }; if do_panic { let msg_str = with_no_visible_paths!({ @@ -701,7 +688,6 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> { source_info, target, cleanup, - self.cx.tcx().sess.opts.debugging_opts.strict_init_checks, ) { return; } diff --git a/compiler/rustc_const_eval/src/interpret/intrinsics.rs b/compiler/rustc_const_eval/src/interpret/intrinsics.rs index 20705be359beb..0503c93beccf5 100644 --- a/compiler/rustc_const_eval/src/interpret/intrinsics.rs +++ b/compiler/rustc_const_eval/src/interpret/intrinsics.rs @@ -417,13 +417,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { } if intrinsic_name == sym::assert_zero_valid { - let should_panic = !might_permit_raw_init( - *self.tcx, - self.cur_span(), - layout, - self.tcx.sess.opts.debugging_opts.strict_init_checks, - InitKind::Zero, - ); + let should_panic = !might_permit_raw_init(*self.tcx, layout, InitKind::Zero); if should_panic { M::abort( @@ -437,13 +431,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { } if intrinsic_name == sym::assert_uninit_valid { - let should_panic = !might_permit_raw_init( - *self.tcx, - self.cur_span(), - layout, - self.tcx.sess.opts.debugging_opts.strict_init_checks, - InitKind::Uninit, - ); + let should_panic = !might_permit_raw_init(*self.tcx, layout, InitKind::Uninit); if should_panic { M::abort( diff --git a/compiler/rustc_const_eval/src/might_permit_raw_init.rs b/compiler/rustc_const_eval/src/might_permit_raw_init.rs index cd900957bb3af..7a3741c541a1f 100644 --- a/compiler/rustc_const_eval/src/might_permit_raw_init.rs +++ b/compiler/rustc_const_eval/src/might_permit_raw_init.rs @@ -3,20 +3,19 @@ use crate::interpret::{InterpCx, MemoryKind, OpTy}; use rustc_middle::ty::layout::LayoutCx; use rustc_middle::ty::{layout::TyAndLayout, ParamEnv, TyCtxt}; use rustc_session::Limit; -use rustc_span::Span; use rustc_target::abi::InitKind; pub fn might_permit_raw_init<'tcx>( tcx: TyCtxt<'tcx>, - root_span: Span, ty: TyAndLayout<'tcx>, - strict: bool, kind: InitKind, ) -> bool { + let strict = tcx.sess.opts.debugging_opts.strict_init_checks; + if strict { let machine = CompileTimeInterpreter::new(Limit::new(0), false); - let mut cx = InterpCx::new(tcx, root_span, ParamEnv::reveal_all(), machine); + let mut cx = InterpCx::new(tcx, rustc_span::DUMMY_SP, ParamEnv::reveal_all(), machine); // We could panic here... Or we could just return "yeah it's valid whatever". Or let // codegen_panic_intrinsic return an error that halts compilation. From 236c7c0abe80d516c05399ea035dcd6cdb979d77 Mon Sep 17 00:00:00 2001 From: 5225225 <5225225@mailbox.org> Date: Mon, 11 Jul 2022 13:00:46 +0100 Subject: [PATCH 6/6] Add test for uninit value with validity invariant in array --- src/test/ui/intrinsics/panic-uninitialized-zeroed.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/test/ui/intrinsics/panic-uninitialized-zeroed.rs b/src/test/ui/intrinsics/panic-uninitialized-zeroed.rs index 9f020e9152954..255151a96032c 100644 --- a/src/test/ui/intrinsics/panic-uninitialized-zeroed.rs +++ b/src/test/ui/intrinsics/panic-uninitialized-zeroed.rs @@ -286,6 +286,7 @@ fn main() { let _val = mem::zeroed::(); let _val = mem::zeroed::<[LR_NonZero; 1]>(); let _val = mem::zeroed::<[NonNull<()>; 1]>(); + let _val = mem::uninitialized::<[NonNull<()>; 1]>(); } } }