[PATCH] Revert "Use constant eval to do strict validity checks"
authorSimon Chopin <simon.chopin@canonical.com>
Wed, 18 Jan 2023 16:03:04 +0000 (17:03 +0100)
committerFabian Grünbichler <debian@fabian.gruenbichler.email>
Mon, 12 Jun 2023 16:36:56 +0000 (17:36 +0100)
This reverts commit 27412d1e3e128349bc515c16ce882860e20f037d.

This is likely a LLVM mis-optimization, but we're not really sure. It
leads to ICE on riscv64.

Bug: https://github.com/rust-lang/rust/issues/102155

Gbp-Pq: Name ubuntu-Revert-Use-constant-eval-to-do-strict-validity-check.patch

13 files changed:
Cargo.lock
compiler/rustc_codegen_cranelift/src/intrinsics/mod.rs
compiler/rustc_codegen_ssa/Cargo.toml
compiler/rustc_codegen_ssa/src/mir/block.rs
compiler/rustc_const_eval/src/const_eval/machine.rs
compiler/rustc_const_eval/src/interpret/intrinsics.rs
compiler/rustc_const_eval/src/lib.rs
compiler/rustc_const_eval/src/might_permit_raw_init.rs [deleted file]
compiler/rustc_middle/src/query/mod.rs
compiler/rustc_middle/src/ty/query.rs
compiler/rustc_query_impl/src/keys.rs
compiler/rustc_target/src/abi/mod.rs
src/test/ui/intrinsics/panic-uninitialized-zeroed.rs

index 2569b3e1976f378585ff5a6b5aec6831b0b9c5ac..b88158f9ff8f29d1f32aaa711d0d9dfdb207279e 100644 (file)
@@ -3731,7 +3731,6 @@ dependencies = [
  "rustc_arena",
  "rustc_ast",
  "rustc_attr",
- "rustc_const_eval",
  "rustc_data_structures",
  "rustc_errors",
  "rustc_fs_util",
index b2a83e1d4ebc96c57da441aff4e044123a5faeed..4f9ced001d4491bfe67da4c81121430f9b84965e 100644 (file)
@@ -22,6 +22,7 @@ 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;
@@ -693,7 +694,12 @@ fn codegen_regular_intrinsic_call<'tcx>(
                 return;
             }
 
-            if intrinsic == sym::assert_zero_valid && !fx.tcx.permits_zero_init(layout) {
+            if intrinsic == sym::assert_zero_valid
+                && !layout.might_permit_raw_init(
+                    fx,
+                    InitKind::Zero,
+                    fx.tcx.sess.opts.unstable_opts.strict_init_checks) {
+
                 with_no_trimmed_paths!({
                     crate::base::codegen_panic(
                         fx,
@@ -707,7 +713,12 @@ fn codegen_regular_intrinsic_call<'tcx>(
                 return;
             }
 
-            if intrinsic == sym::assert_uninit_valid && !fx.tcx.permits_uninit_init(layout) {
+            if intrinsic == sym::assert_uninit_valid
+                && !layout.might_permit_raw_init(
+                    fx,
+                    InitKind::Uninit,
+                    fx.tcx.sess.opts.unstable_opts.strict_init_checks) {
+
                 with_no_trimmed_paths!({
                     crate::base::codegen_panic(
                         fx,
index 46d6344dbb2ef9d273daa0a14d1937c527a5738c..e7ee424668b864af206d92baa63efdf89affea39 100644 (file)
@@ -40,7 +40,6 @@ 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"
index 3eee58d9d1c84aa916e7121497a1698df9a7d36e..a9eb4ec6439f9ddfd1b404912aa6f7d3ebce8305 100644 (file)
@@ -22,7 +22,7 @@ use rustc_span::source_map::Span;
 use rustc_span::{sym, Symbol};
 use rustc_symbol_mangling::typeid::typeid_for_fnabi;
 use rustc_target::abi::call::{ArgAbi, FnAbi, PassMode};
-use rustc_target::abi::{self, HasDataLayout, WrappingRange};
+use rustc_target::abi::{self, HasDataLayout, InitKind, WrappingRange};
 use rustc_target::spec::abi::Abi;
 
 /// Used by `FunctionCx::codegen_terminator` for emitting common patterns
@@ -528,6 +528,7 @@ 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 +547,12 @@ 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 => !bx.tcx().permits_zero_init(layout),
-                UninitValid => !bx.tcx().permits_uninit_init(layout),
+                ZeroValid => !layout.might_permit_raw_init(bx, InitKind::Zero, strict_validity),
+                UninitValid => !layout.might_permit_raw_init(bx, InitKind::Uninit, strict_validity),
             };
             if do_panic {
                 let msg_str = with_no_visible_paths!({
@@ -687,6 +687,7 @@ 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;
         }
index fc2e6652a3d72267811bf589cec5aa9fd18df404..ef6cff42ad9e5cbff388e724cba4f29d2180d70b 100644 (file)
@@ -104,7 +104,7 @@ pub struct CompileTimeInterpreter<'mir, 'tcx> {
 }
 
 impl<'mir, 'tcx> CompileTimeInterpreter<'mir, 'tcx> {
-    pub(crate) fn new(const_eval_limit: Limit, can_access_statics: bool) -> Self {
+    pub(super) fn new(const_eval_limit: Limit, can_access_statics: bool) -> Self {
         CompileTimeInterpreter {
             steps_remaining: const_eval_limit.0,
             stack: Vec::new(),
index 08209eb793216543453e86a23a3a15d08d3bec80..e0ce6d9acc834b296c06d8f386e90b19e2981eae 100644 (file)
@@ -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, Primitive, Size};
+use rustc_target::abi::{Abi, Align, InitKind, Primitive, Size};
 
 use super::{
     util::ensure_monomorphic_enough, CheckInAllocMsg, ImmTy, InterpCx, Machine, OpTy, PlaceTy,
@@ -435,33 +435,35 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
                         ),
                     )?;
                 }
-
-                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_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_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
-                            ),
-                        )?;
-                    }
+                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
+                        ),
+                    )?;
                 }
             }
             sym::simd_insert => {
index 72ac6af685dc4b247341c8a9fec847e1177cac83..d65d4f7eb720ec4e3cc1981d133759739c792453 100644 (file)
@@ -33,13 +33,11 @@ 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);
@@ -61,8 +59,4 @@ 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
deleted file mode 100644 (file)
index f971c22..0000000
+++ /dev/null
@@ -1,40 +0,0 @@
-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)
-    }
-}
index d8483e7e40914eb3d64053f5c39817e879d76007..e498015a4a570758edb9f6d1eabab8d77dd1bd2a 100644 (file)
@@ -2049,12 +2049,4 @@ 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 }
-    }
 }
index 2452bcf6a61b8f3aa456f9c4d65344185502f0d4..3d662ed5de4baa1e42feccd224b9b98c93cabbb0 100644 (file)
@@ -28,7 +28,6 @@ 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;
index 49175e97f41711407d452e8f6928d63e26438ad4..4d6bb02c38ef60d5b3e80d9b23d96016fe4b6047 100644 (file)
@@ -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, layout::TyAndLayout, Ty, TyCtxt};
+use rustc_middle::ty::{self, Ty, TyCtxt};
 use rustc_span::symbol::{Ident, Symbol};
 use rustc_span::{Span, DUMMY_SP};
 
@@ -395,16 +395,6 @@ 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 {
index 92ce4d91d84d1baeaf9288d034066107c2b908cb..d103a06060deb76bd125904feabf1c9a80026574 100644 (file)
@@ -1378,7 +1378,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, PartialEq, Eq)]
+#[derive(Copy, Clone, Debug)]
 pub enum InitKind {
     Zero,
     Uninit,
@@ -1493,18 +1493,14 @@ impl<'a, Ty> TyAndLayout<'a, Ty> {
     ///
     /// `init_kind` indicates if the memory is zero-initialized or left uninitialized.
     ///
-    /// 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)
+    /// `strict` is an opt-in debugging flag added in #97323 that enables more checks.
     ///
-    /// 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>.
+    /// This is conservative: in doubt, it will answer `true`.
     ///
-    /// 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
+    /// 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
     where
         Self: Copy,
         Ty: TyAbiInterface<'a, C>,
@@ -1517,8 +1513,13 @@ impl<'a, Ty> TyAndLayout<'a, Ty> {
                     s.valid_range(cx).contains(0)
                 }
                 InitKind::Uninit => {
-                    // The range must include all values.
-                    s.is_always_valid(cx)
+                    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)
+                    }
                 }
             }
         };
@@ -1539,12 +1540,19 @@ 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 { .. } => {
+            FieldsShape::Array { count, .. } => {
                 // 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) {
+                    if !self.field(cx, idx).might_permit_raw_init(cx, init_kind, strict) {
                         // We found a field that is unhappy with this kind of initialization.
                         return false;
                     }
index 255151a96032c016eccb4305c614925306e1b79b..3ffd35ecdb8da7b8543b82313ecd61ed5bc24455 100644 (file)
@@ -57,13 +57,6 @@ enum LR_NonZero {
 
 struct ZeroSized;
 
-#[allow(dead_code)]
-#[repr(i32)]
-enum ZeroIsValid {
-    Zero(u8) = 0,
-    One(NonNull<()>) = 1,
-}
-
 fn test_panic_msg<T>(op: impl (FnOnce() -> T) + panic::UnwindSafe, msg: &str) {
     let err = panic::catch_unwind(op).err();
     assert_eq!(
@@ -159,12 +152,33 @@ 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::<LR_NonZero>(),
+            "attempted to leave type `LR_NonZero` uninitialized, which is invalid"
+        );
+        test_panic_msg(
+            || mem::zeroed::<LR_NonZero>(),
+            "attempted to zero-initialize type `LR_NonZero`, which is invalid"
+        );
+
+        test_panic_msg(
+            || mem::uninitialized::<ManuallyDrop<LR_NonZero>>(),
+            "attempted to leave type `std::mem::ManuallyDrop<LR_NonZero>` uninitialized, \
+             which is invalid"
+        );
+        test_panic_msg(
+            || mem::zeroed::<ManuallyDrop<LR_NonZero>>(),
+            "attempted to zero-initialize type `std::mem::ManuallyDrop<LR_NonZero>`, \
+             which is invalid"
+        );
+        */
+
         test_panic_msg(
             || mem::uninitialized::<(NonNull<u32>, u32, u32)>(),
             "attempted to leave type `(core::ptr::non_null::NonNull<u32>, u32, u32)` uninitialized, \
                 which is invalid"
         );
-
         test_panic_msg(
             || mem::zeroed::<(NonNull<u32>, u32, u32)>(),
             "attempted to zero-initialize type `(core::ptr::non_null::NonNull<u32>, u32, u32)`, \
@@ -182,23 +196,11 @@ fn main() {
                 which is invalid"
         );
 
-        test_panic_msg(
-            || mem::uninitialized::<LR_NonZero>(),
-            "attempted to leave type `LR_NonZero` uninitialized, which is invalid"
-        );
-
-        test_panic_msg(
-            || mem::uninitialized::<ManuallyDrop<LR_NonZero>>(),
-            "attempted to leave type `core::mem::manually_drop::ManuallyDrop<LR_NonZero>` uninitialized, \
-             which is invalid"
-        );
-
         test_panic_msg(
             || mem::uninitialized::<NoNullVariant>(),
             "attempted to leave type `NoNullVariant` uninitialized, \
                 which is invalid"
         );
-
         test_panic_msg(
             || mem::zeroed::<NoNullVariant>(),
             "attempted to zero-initialize type `NoNullVariant`, \
@@ -210,12 +212,10 @@ fn main() {
             || mem::uninitialized::<bool>(),
             "attempted to leave type `bool` uninitialized, which is invalid"
         );
-
         test_panic_msg(
             || mem::uninitialized::<LR>(),
             "attempted to leave type `LR` uninitialized, which is invalid"
         );
-
         test_panic_msg(
             || mem::uninitialized::<ManuallyDrop<LR>>(),
             "attempted to leave type `core::mem::manually_drop::ManuallyDrop<LR>` uninitialized, which is invalid"
@@ -229,7 +229,6 @@ fn main() {
         let _val = mem::zeroed::<Option<&'static i32>>();
         let _val = mem::zeroed::<MaybeUninit<NonNull<u32>>>();
         let _val = mem::zeroed::<[!; 0]>();
-        let _val = mem::zeroed::<ZeroIsValid>();
         let _val = mem::uninitialized::<MaybeUninit<bool>>();
         let _val = mem::uninitialized::<[!; 0]>();
         let _val = mem::uninitialized::<()>();
@@ -260,33 +259,12 @@ 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::<LR_NonZero>(),
-                "attempted to zero-initialize type `LR_NonZero`, which is invalid"
-            );
-
-            test_panic_msg(
-                || mem::zeroed::<ManuallyDrop<LR_NonZero>>(),
-                "attempted to zero-initialize type `core::mem::manually_drop::ManuallyDrop<LR_NonZero>`, \
-                 which is invalid"
-            );
         } else {
             // These are UB because they have not been officially blessed, but we await the resolution
             // of <https://github.com/rust-lang/unsafe-code-guidelines/issues/71> before doing
             // anything about that.
             let _val = mem::uninitialized::<i32>();
             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::<LR_NonZero>();
-            let _val = mem::zeroed::<[LR_NonZero; 1]>();
-            let _val = mem::zeroed::<[NonNull<()>; 1]>();
-            let _val = mem::uninitialized::<[NonNull<()>; 1]>();
         }
     }
 }