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

bors · bors · commit 263edd43c525 · 2022-07-17T19:28:01.000Z
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 :)
diff --git a/Cargo.lock b/Cargo.lock
@@ -3752,6 +3752,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
@@ -58,7 +58,6 @@ pub(crate) use llvm::codegen_llvm_intrinsic_call;
 use rustc_middle::ty::print::with_no_trimmed_paths;
 use rustc_middle::ty::subst::SubstsRef;
 use rustc_span::symbol::{kw, sym, Symbol};
-use rustc_target::abi::InitKind;
 
 use crate::prelude::*;
 use cranelift_codegen::ir::AtomicRmwOp;
@@ -672,12 +671,7 @@ fn codegen_regular_intrinsic_call<'tcx>(
                 return;
             }
 
-            if intrinsic == sym::assert_zero_valid
-                && !layout.might_permit_raw_init(
-                    fx,
-                    InitKind::Zero,
-                    fx.tcx.sess.opts.unstable_opts.strict_init_checks) {
-
+            if intrinsic == sym::assert_zero_valid && !fx.tcx.permits_zero_init(layout) {
                 with_no_trimmed_paths!({
                     crate::base::codegen_panic(
                         fx,
@@ -688,12 +682,7 @@ fn codegen_regular_intrinsic_call<'tcx>(
                 return;
             }
 
-            if intrinsic == sym::assert_uninit_valid
-                && !layout.might_permit_raw_init(
-                    fx,
-                    InitKind::Uninit,
-                    fx.tcx.sess.opts.unstable_opts.strict_init_checks) {
-
+            if intrinsic == sym::assert_uninit_valid && !fx.tcx.permits_uninit_init(layout) {
                 with_no_trimmed_paths!({
                     crate::base::codegen_panic(
                         fx,
diff --git 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
@@ -22,7 +22,7 @@ use rustc_span::source_map::Span;
 use rustc_span::{sym, Symbol};
 use rustc_symbol_mangling::typeid_for_fnabi;
 use rustc_target::abi::call::{ArgAbi, FnAbi, PassMode};
-use rustc_target::abi::{self, HasDataLayout, InitKind, WrappingRange};
+use rustc_target::abi::{self, HasDataLayout, WrappingRange};
 use rustc_target::spec::abi::Abi;
 
 /// Used by `FunctionCx::codegen_terminator` for emitting common patterns
@@ -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
@@ -547,12 +546,13 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
         });
         if let Some(intrinsic) = panic_intrinsic {
             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 => !bx.tcx().permits_zero_init(layout),
+                UninitValid => !bx.tcx().permits_uninit_init(layout),
             };
             if do_panic {
                 let msg_str = with_no_visible_paths!({
@@ -687,7 +687,6 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
             source_info,
             target,
             cleanup,
-            self.cx.tcx().sess.opts.unstable_opts.strict_init_checks,
         ) {
             return;
         }
diff --git 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
@@ -15,7 +15,7 @@ use rustc_middle::ty::layout::LayoutOf as _;
 use rustc_middle::ty::subst::SubstsRef;
 use rustc_middle::ty::{Ty, TyCtxt};
 use rustc_span::symbol::{sym, Symbol};
-use rustc_target::abi::{Abi, Align, InitKind, Primitive, Size};
+use rustc_target::abi::{Abi, Align, Primitive, Size};
 
 use super::{
     util::ensure_monomorphic_enough, CheckInAllocMsg, ImmTy, InterpCx, Machine, OpTy, PlaceTy,
@@ -411,35 +411,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.unstable_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 = !self.tcx.permits_zero_init(layout);
+
+                    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.unstable_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 = !self.tcx.permits_uninit_init(layout);
+
+                    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
@@ -33,11 +33,13 @@ extern crate rustc_middle;
 pub mod const_eval;
 mod errors;
 pub mod interpret;
+mod might_permit_raw_init;
 pub mod transform;
 pub mod util;
 
 use rustc_middle::ty;
 use rustc_middle::ty::query::Providers;
+use rustc_target::abi::InitKind;
 
 pub fn provide(providers: &mut Providers) {
     const_eval::provide(providers);
@@ -59,4 +61,8 @@ pub fn provide(providers: &mut Providers) {
         let (param_env, value) = param_env_and_value.into_parts();
         const_eval::deref_mir_constant(tcx, param_env, value)
     };
+    providers.permits_uninit_init =
+        |tcx, ty| might_permit_raw_init::might_permit_raw_init(tcx, ty, InitKind::Uninit);
+    providers.permits_zero_init =
+        |tcx, ty| might_permit_raw_init::might_permit_raw_init(tcx, ty, InitKind::Zero);
 }
diff --git a/compiler/rustc_const_eval/src/might_permit_raw_init.rs b/compiler/rustc_const_eval/src/might_permit_raw_init.rs
@@ -0,0 +1,40 @@
+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.unstable_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);
+
+        let allocated = cx
+            .allocate(ty, MemoryKind::Machine(crate::const_eval::MemoryKind::Heap))
+            .expect("OOM: failed to allocate for uninit check");
+
+        if kind == InitKind::Zero {
+            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_middle/src/query/mod.rs b/compiler/rustc_middle/src/query/mod.rs
@@ -2053,4 +2053,12 @@ rustc_queries! {
         desc { |tcx| "looking up generator diagnostic data of `{}`", tcx.def_path_str(key) }
         separate_provide_extern
     }
+
+    query permits_uninit_init(key: TyAndLayout<'tcx>) -> bool {
+        desc { "checking to see if {:?} permits being left uninit", key.ty }
+    }
+
+    query permits_zero_init(key: TyAndLayout<'tcx>) -> bool {
+        desc { "checking to see if {:?} permits being left zeroed", key.ty }
+    }
 }
diff --git a/compiler/rustc_middle/src/ty/query.rs b/compiler/rustc_middle/src/ty/query.rs
@@ -28,6 +28,7 @@ use crate::traits::query::{
 use crate::traits::specialization_graph;
 use crate::traits::{self, ImplSource};
 use crate::ty::fast_reject::SimplifiedType;
+use crate::ty::layout::TyAndLayout;
 use crate::ty::subst::{GenericArg, SubstsRef};
 use crate::ty::util::AlwaysRequiresDrop;
 use crate::ty::GeneratorDiagnosticData;
diff --git a/compiler/rustc_query_impl/src/keys.rs b/compiler/rustc_query_impl/src/keys.rs
@@ -6,7 +6,7 @@ use rustc_middle::mir;
 use rustc_middle::traits;
 use rustc_middle::ty::fast_reject::SimplifiedType;
 use rustc_middle::ty::subst::{GenericArg, SubstsRef};
-use rustc_middle::ty::{self, Ty, TyCtxt};
+use rustc_middle::ty::{self, layout::TyAndLayout, Ty, TyCtxt};
 use rustc_span::symbol::{Ident, Symbol};
 use rustc_span::{Span, DUMMY_SP};
 
@@ -385,6 +385,16 @@ impl<'tcx> Key for Ty<'tcx> {
     }
 }
 
+impl<'tcx> Key for TyAndLayout<'tcx> {
+    #[inline(always)]
+    fn query_crate_is_local(&self) -> bool {
+        true
+    }
+    fn default_span(&self, _: TyCtxt<'_>) -> Span {
+        DUMMY_SP
+    }
+}
+
 impl<'tcx> Key for (Ty<'tcx>, Ty<'tcx>) {
     #[inline(always)]
     fn query_crate_is_local(&self) -> bool {
diff --git 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,
@@ -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;
                     }
diff --git a/src/test/ui/intrinsics/panic-uninitialized-zeroed.rs b/src/test/ui/intrinsics/panic-uninitialized-zeroed.rs

Original file line number	Diff line number	Diff line change
`@@ -104,7 +104,7 @@ pub struct CompileTimeInterpreter<'mir, 'tcx> {`
`104`	`104`	`}`
`105`	`105`
`106`	`106`	`impl<'mir, 'tcx> CompileTimeInterpreter<'mir, 'tcx> {`
`107`		`- pub(super) fn new(const_eval_limit: Limit, can_access_statics: bool) -> Self {`
	`107`	`+ pub(crate) fn new(const_eval_limit: Limit, can_access_statics: bool) -> Self {`
`108`	`108`	`CompileTimeInterpreter {`
`109`	`109`	`steps_remaining: const_eval_limit.0,`
`110`	`110`	`stack: Vec::new(),`
Original file line number	Diff line number	Diff line change
`@@ -2053,4 +2053,12 @@ rustc_queries! {`
`2053`	`2053`	desc { \|tcx\| "looking up generator diagnostic data of `{}`", tcx.def_path_str(key) }
`2054`	`2054`	`separate_provide_extern`
`2055`	`2055`	`}`
	`2056`	`+`
	`2057`	`+ query permits_uninit_init(key: TyAndLayout<'tcx>) -> bool {`
	`2058`	`+ desc { "checking to see if {:?} permits being left uninit", key.ty }`
	`2059`	`+ }`
	`2060`	`+`
	`2061`	`+ query permits_zero_init(key: TyAndLayout<'tcx>) -> bool {`
	`2062`	`+ desc { "checking to see if {:?} permits being left zeroed", key.ty }`
	`2063`	`+ }`
`2056`	`2064`	`}`