Comment more code and make tests clearer

Co-Authored-By: Ralf Jung <[email protected]>

Author: fee1-dead

compiler/rustc_const_eval/src/const_eval/machine.rs

@@ -169,7 +169,11 @@ pub type CompileTimeInterpCx<'tcx> = InterpCx<'tcx, CompileTimeMachine<'tcx>>;
 
 #[derive(Debug, PartialEq, Eq, Copy, Clone)]
 pub enum MemoryKind {
-    Heap { was_made_global: bool },
+    Heap {
+        /// Indicates whether `make_global` was called on this allocation.
+        /// If this is `true`, the allocation must be immutable.
+        was_made_global: bool,
+    },
 }
 
 impl fmt::Display for MemoryKind {

compiler/rustc_const_eval/src/interpret/intern.rs

@@ -106,13 +106,12 @@ fn intern_shallow<'tcx, T: CanIntern, M: CompileTimeMachine<'tcx, T>>(
 
     match kind {
         MemoryKind::Machine(x) if let Some(reason) = x.disallows_intern() => match reason {
-            // attempting to intern a `const_allocate`d pointer that was not made global via
-            // `const_make_global`. We emit an error here but don't return an `Err`. The `Err`
-            // is for pointers that we can't intern at all (i.e. dangling pointers). We still
-            // (recursively) intern this pointer because we don't have to worry about the
-            // additional paperwork involved with _not_ interning it, such as storing it in
-            // the dead memory map and having to deal with additional "dangling pointer"
-            // messages if someone tries to store the non-made-global ptr in the final value.
+            // Attempting to intern a `const_allocate`d pointer that was not made global via
+            // `const_make_global`. This is an error, but if we return `Err` now, things
+            // become inconsistent because we already removed the allocation from `alloc_map`.
+            // So instead we just emit an error and then continue interning as usual.
+            // We *could* do this check before removing the allocation from `alloc_map`, but then
+            // it would be much harder to ensure that we only error once for each allocation.
             DisallowInternReason::ConstHeap => {
                 ecx.tcx.dcx().emit_err(ConstHeapPtrInFinal { span: ecx.tcx.span });
             }

compiler/rustc_const_eval/src/interpret/memory.rs

@@ -312,7 +312,7 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
         interp_ok(new_ptr)
     }
 
-    /// mark the `const_allocate`d pointer immutable so we can intern it.
+    /// Mark the `const_allocate`d allocation `ptr` points to as immutable so we can intern it.
     pub fn make_const_heap_ptr_global(
         &mut self,
         ptr: Pointer<Option<CtfeProvenance>>,
@@ -325,20 +325,19 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
             return Err(ConstEvalErrKind::ConstMakeGlobalWithOffset(ptr)).into();
         }
 
-        let not_local_heap =
-            matches!(self.tcx.try_get_global_alloc(alloc_id), Some(GlobalAlloc::Memory(_)));
-
-        if not_local_heap {
+        if matches!(self.tcx.try_get_global_alloc(alloc_id), Some(_)) {
+            // This points to something outside the current interpreter.
             return Err(ConstEvalErrKind::ConstMakeGlobalPtrIsNonHeap(ptr)).into();
         }
 
+        // If we can't find it in `alloc_map` it must be dangling (because we don't use
+        // `extra_fn_ptr_map` in const-eval).
         let (kind, alloc) = self
             .memory
             .alloc_map
             .get_mut_or(alloc_id, || Err(ConstEvalErrKind::ConstMakeGlobalWithDanglingPtr(ptr)))?;
 
-        alloc.mutability = Mutability::Not;
-
+        // Ensure this is actually a *heap* allocation, and record it as made-global.
         match kind {
             MemoryKind::Stack | MemoryKind::CallerLocation => {
                 return Err(ConstEvalErrKind::ConstMakeGlobalPtrIsNonHeap(ptr)).into();
@@ -352,6 +351,9 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
             }
         }
 
+        // Prevent further mutation, this is now an immutable global.
+        alloc.mutability = Mutability::Not;
+
         interp_ok(())
     }
 

compiler/rustc_const_eval/src/util/check_validity_requirement.rs

@@ -52,6 +52,7 @@ fn check_validity_requirement_strict<'tcx>(
 
     let mut cx = InterpCx::new(cx.tcx(), DUMMY_SP, cx.typing_env, machine);
 
+    // It doesn't really matter which `MemoryKind` we use here, `Stack` is the least wrong.
     let allocated =
         cx.allocate(ty, MemoryKind::Stack).expect("OOM: failed to allocate for uninit check");
 
@@ -183,12 +184,10 @@ pub(crate) fn validate_scalar_in_layout<'tcx>(
     let Ok(layout) = cx.layout_of(ty) else {
         bug!("could not compute layout of {scalar:?}:{ty:?}")
     };
-    let allocated = cx
-        .allocate(
-            layout,
-            MemoryKind::Machine(crate::const_eval::MemoryKind::Heap { was_made_global: false }),
-        )
-        .expect("OOM: failed to allocate for uninit check");
+
+    // It doesn't really matter which `MemoryKind` we use here, `Stack` is the least wrong.
+    let allocated =
+        cx.allocate(layout, MemoryKind::Stack).expect("OOM: failed to allocate for uninit check");
 
     cx.write_scalar(scalar, &allocated).unwrap();
 

tests/ui/consts/const-eval/heap/alloc_intrinsic_nontransient.rs

@@ -12,7 +12,7 @@ const fn foo() -> &'static i32 {
         *i = 20;
         i
     };
-    unsafe { &*(intrinsics::const_make_global(t as *mut u8) as *mut i32) }
+    unsafe { &*(intrinsics::const_make_global(t as *mut u8) as *const i32) }
 }
 fn main() {
     assert_eq!(*FOO, 20);

tests/ui/consts/const-eval/heap/alloc_intrinsic_uninit.rs

@@ -5,6 +5,7 @@
 use std::intrinsics;
 
 const BAR: &i32 = unsafe { //~ ERROR: uninitialized memory
-    &*(intrinsics::const_make_global(intrinsics::const_allocate(4, 4)) as *mut i32)
+    // Make the pointer immutable to avoid errors related to mutable pointers in constants.
+    &*(intrinsics::const_make_global(intrinsics::const_allocate(4, 4)) as *const i32)
 };
 fn main() {}

tests/ui/consts/const-eval/heap/make-global-dangling.rs

@@ -1,3 +1,5 @@
+// Ensure that we can't call `const_make_global` on dangling pointers.
+
 #![feature(core_intrinsics)]
 #![feature(const_heap)]
 

tests/ui/consts/const-eval/heap/make-global-dangling.stderr

@@ -1,11 +1,11 @@
 error[E0080]: pointer not dereferenceable: pointer must point to some allocation, but got null pointer
-  --> $DIR/make-global-dangling.rs:7:8
+  --> $DIR/make-global-dangling.rs:9:8
    |
 LL |     &*(intrinsics::const_make_global(std::ptr::null_mut()) as *const u32)
    |        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ evaluation of `Y` failed here
 
 error[E0080]: pointer not dereferenceable: pointer must point to some allocation, but got 0x1[noalloc] which is a dangling pointer (it has no provenance)
-  --> $DIR/make-global-dangling.rs:12:8
+  --> $DIR/make-global-dangling.rs:14:8
    |
 LL |     &*(intrinsics::const_make_global(std::ptr::dangling_mut()) as *const u32)
    |        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ evaluation of `Z` failed here

tests/ui/consts/const-eval/heap/make-global-other.rs

@@ -1,3 +1,5 @@
+// Ensure that we can't call `const_make_global` on pointers not in the current interpreter.
+
 #![feature(core_intrinsics)]
 #![feature(const_heap)]
 

tests/ui/consts/const-eval/heap/make-global-other.stderr

@@ -1,5 +1,5 @@
 error[E0080]: pointer passed to `const_make_global` does not point to a heap allocation: ALLOC0<imm>
-  --> $DIR/make-global-other.rs:9:8
+  --> $DIR/make-global-other.rs:11:8
    |
 LL |     &*(intrinsics::const_make_global(X as *const i32 as *mut u8) as *const i32)
    |        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ evaluation of `Y` failed here