Rollup merge of rust-lang#99033 - 5225225:interpreter-validity-checks, r=oli-obk

Use constant eval to do strict mem::uninit/zeroed validity checks

I'm not sure about the code organisation here, I just dumped the check in rustc_const_eval at the root. Not hard to move it elsewhere, in any case.

Also, this means cranelift codegen intrinsics lose the strict checks, since they don't seem to depend on rustc_const_eval, and I didn't see a point in keeping around two copies.

I also left comments in the is_zero_valid methods about "uhhh help how do i do this", those apply to both methods equally.

Also rustc_codegen_ssa now depends on rustc_const_eval... is this okay?

Pinging `@RalfJung` since you were the one who mentioned this to me, so I'm assuming you're interested.

Haven't had a chance to run full tests on this since it's really warm, and it's 1AM, I'll check out any failures/comments in the morning :)

Author: Dylan-DPC

Cargo.lock

@@ -3664,6 +3664,7 @@ dependencies = [
  "rustc_arena",
  "rustc_ast",
  "rustc_attr",
+ "rustc_const_eval",
  "rustc_data_structures",
  "rustc_errors",
  "rustc_fs_util",

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,10 +674,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) {
+                && !might_permit_raw_init(fx.tcx, layout, InitKind::Zero) {
 
                 with_no_trimmed_paths!({
                     crate::base::codegen_panic(
@@ -689,10 +687,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) {
+                && !might_permit_raw_init(fx.tcx, layout, InitKind::Uninit) {
 
                 with_no_trimmed_paths!({
                     crate::base::codegen_panic(

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;

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"

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<mir::BasicBlock>,
         cleanup: Option<mir::BasicBlock>,
-        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
@@ -546,13 +545,15 @@ 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 => !layout.might_permit_raw_init(bx, InitKind::Zero, strict_validity),
-                UninitValid => !layout.might_permit_raw_init(bx, InitKind::Uninit, strict_validity),
+                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!({
@@ -687,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;
         }

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(),

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;
 
@@ -413,35 +415,33 @@ 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 = !might_permit_raw_init(*self.tcx, layout, 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 = !might_permit_raw_init(*self.tcx, layout, InitKind::Uninit);
+
+                    if should_panic {
+                        M::abort(
+                            self,
+                            format!(
+                                "aborted execution: attempted to leave type `{}` uninitialized, which is invalid",
+                                ty
+                            ),
+                        )?;
+                    }
                 }
             }
             sym::simd_insert => {

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;
 

compiler/rustc_const_eval/src/might_permit_raw_init.rs

@@ -0,0 +1,44 @@
+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_target::abi::InitKind;
+
+pub fn might_permit_raw_init<'tcx>(
+    tcx: TyCtxt<'tcx>,
+    ty: TyAndLayout<'tcx>,
+    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, 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.
+        // 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)
+    }
+}

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,
@@ -1487,14 +1487,18 @@ 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 <https://github.com/rust-lang/rust/issues/66151>.
     ///
-    /// 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<C>(self, cx: &C, init_kind: InitKind, strict: bool) -> bool
+    /// 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<C>(self, cx: &C, init_kind: InitKind) -> bool
     where
         Self: Copy,
         Ty: TyAbiInterface<'a, C>,
@@ -1507,13 +1511,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 +1533,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;
                     }