Validate constants during const_eval_raw

Author: oli-obk

compiler/rustc_mir/src/const_eval/eval_queries.rs

@@ -193,21 +193,7 @@ fn validate_and_turn_into_const<'tcx>(
     let ecx = mk_eval_cx(tcx, tcx.def_span(key.value.instance.def_id()), key.param_env, is_static);
     let val = (|| {
         let mplace = ecx.raw_const_to_mplace(constant)?;
-
-        // FIXME do not validate promoteds until a decision on
-        // https://github.com/rust-lang/rust/issues/67465 is made
-        if cid.promoted.is_none() {
-            let mut ref_tracking = RefTracking::new(mplace);
-            while let Some((mplace, path)) = ref_tracking.todo.pop() {
-                ecx.const_validate_operand(
-                    mplace.into(),
-                    path,
-                    &mut ref_tracking,
-                    /*may_ref_to_static*/ ecx.memory.extra.can_access_statics,
-                )?;
-            }
-        }
-        // Now that we validated, turn this into a proper constant.
+        // Turn this into a proper constant.
         // Statics/promoteds are always `ByRef`, for the rest `op_to_const` decides
         // whether they become immediates.
         if is_static || cid.promoted.is_some() {
@@ -221,6 +207,7 @@ fn validate_and_turn_into_const<'tcx>(
         }
     })();
 
+    // FIXME: Can this ever be an error and not be a compiler bug or can we just ICE here?
     val.map_err(|error| {
         let err = ConstEvalErr::new(&ecx, error, None);
         err.struct_error(ecx.tcx, "it is undefined behavior to use this value", |mut diag| {
@@ -319,7 +306,6 @@ pub fn const_eval_raw_provider<'tcx>(
 
     let res = ecx.load_mir(cid.instance.def, cid.promoted);
     res.and_then(|body| eval_body_using_ecx(&mut ecx, cid, &body))
-        .map(|place| RawConst { alloc_id: place.ptr.assert_ptr().alloc_id, ty: place.layout.ty })
         .map_err(|error| {
             let err = ConstEvalErr::new(&ecx, error, None);
             // errors in statics are always emitted as fatal errors
@@ -397,4 +383,37 @@ pub fn const_eval_raw_provider<'tcx>(
                 err.report_as_error(ecx.tcx.at(ecx.cur_span()), "could not evaluate constant")
             }
         })
+        .and_then(|mplace| {
+            // Since evaluation had no errors, valiate the resulting constant:
+            let validation = try {
+                // FIXME do not validate promoteds until a decision on
+                // https://github.com/rust-lang/rust/issues/67465 is made
+                if cid.promoted.is_none() {
+                    let mut ref_tracking = RefTracking::new(mplace);
+                    while let Some((mplace, path)) = ref_tracking.todo.pop() {
+                        ecx.const_validate_operand(
+                            mplace.into(),
+                            path,
+                            &mut ref_tracking,
+                            /*may_ref_to_static*/ ecx.memory.extra.can_access_statics,
+                        )?;
+                    }
+                }
+            };
+            if let Err(error) = validation {
+                // Validation failed, report an error
+                let err = ConstEvalErr::new(&ecx, error, None);
+                Err(err.struct_error(
+                    ecx.tcx,
+                    "it is undefined behavior to use this value",
+                    |mut diag| {
+                        diag.note(note_on_undefined_behavior_error());
+                        diag.emit();
+                    },
+                ))
+            } else {
+                // Convert to raw constant
+                Ok(RawConst { alloc_id: mplace.ptr.assert_ptr().alloc_id, ty: mplace.layout.ty })
+            }
+        })
 }

compiler/rustc_mir/src/interpret/validity.rs

@@ -425,26 +425,28 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, '
                 let alloc_kind = self.ecx.tcx.get_global_alloc(ptr.alloc_id);
                 if let Some(GlobalAlloc::Static(did)) = alloc_kind {
                     assert!(!self.ecx.tcx.is_thread_local_static(did));
+                    assert!(self.ecx.tcx.is_static(did));
                     // See const_eval::machine::MemoryExtra::can_access_statics for why
                     // this check is so important.
                     // This check is reachable when the const just referenced the static,
                     // but never read it (so we never entered `before_access_global`).
                     // We also need to do it here instead of going on to avoid running
                     // into the `before_access_global` check during validation.
-                    if !self.may_ref_to_static && self.ecx.tcx.is_static(did) {
+                    if !self.may_ref_to_static {
                         throw_validation_failure!(self.path,
                             { "a {} pointing to a static variable", kind }
                         );
                     }
-                    // `extern static` cannot be validated as they have no body.
-                    // FIXME: Statics from other crates are also skipped.
-                    // They might be checked at a different type, but for now we
-                    // want to avoid recursing too deeply.  We might miss const-invalid data,
+                    // We skip checking other statics. These statics must be sound by themselves,
+                    // and the only way to get broken statics here is by using unsafe code.
+                    // The reasons we don't check other statics is twofold. For one, in all sound
+                    // cases, the static was already validated on its own, and second, we trigger
+                    // cycle errors if we try to compute the value of the other static and that
+                    // static refers back to us.
+                    // We might miss const-invalid data,
                     // but things are still sound otherwise (in particular re: consts
                     // referring to statics).
-                    if !did.is_local() || self.ecx.tcx.is_foreign_item(did) {
-                        return Ok(());
-                    }
+                    return Ok(());
                 }
             }
             // Proceed recursively even for ZST, no reason to skip them!

src/test/ui/consts/const-eval/double_check2.rs

@@ -1,3 +1,11 @@
+// check-pass
+
+// This test exhibits undefined behavior, but it is impossible to prevent generally during
+// const eval, even if possible to prevent in the cases here. The reason it's impossible in general
+// is that we run into query cycles even *without* UB, just because we're checking for UB.
+// We do not detect it if you create references to statics
+// in ways that are UB.
+
 enum Foo {
     A = 5,
     B = 42,
@@ -13,11 +21,14 @@ union Union {
     u8: &'static u8,
 }
 static BAR: u8 = 5;
-static FOO: (&Foo, &Bar) = unsafe {( //~ undefined behavior
-    Union { u8: &BAR }.foo,
-    Union { u8: &BAR }.bar,
-)};
-static FOO2: (&Foo, &Bar) = unsafe {(std::mem::transmute(&BAR), std::mem::transmute(&BAR))};
-//~^ undefined behavior
+static FOO: (&Foo, &Bar) = unsafe {
+    (
+        // undefined behavior
+        Union { u8: &BAR }.foo,
+        Union { u8: &BAR }.bar,
+    )
+};
+static FOO2: (&Foo, &Bar) = unsafe { (std::mem::transmute(&BAR), std::mem::transmute(&BAR)) };
+//^ undefined behavior
 
 fn main() {}