add const_make_global; err for const_allocate ptrs if didn't call

[fee1-dead]

Implements as discussed on Zulip: #t-compiler/const-eval > const heap

r? @rust-lang/wg-const-eval

cc #129233

[rustbot]

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri

Some changes occurred to the intrinsics. Make sure the CTFE / Miri interpreter
gets adapted for the changes, if necessary.

cc @rust-lang/miri, @RalfJung, @oli-obk, @lcnr

Some changes occurred to the CTFE machinery

cc @RalfJung, @oli-obk, @lcnr

[juntyr]

I'm really excited about this experiment and the movement on const alloc, so thanks for working on it <3

[RalfJung]

In terms of naming, I think we said that Box::leak would not be performing this operation, so "leak" seems like a suboptimal choice. Maybe something like "make_global" or "into_static" or so?

The original plan I proposed also makes the allocation immutable when it gets marked as global -- basically, this operation should indicate that you are done preparing this allocation and it is ready to be interned. (We actually intern it later as that's easier, but for the API this does not matter.)

[RalfJung]

This risks breaking key invariants like "tcx-managed allocations do not have any dangling pointers". So as nice as this is I think it won't work.

[fee1-dead]

I'm pretty sure both ptr_get_alloc_id and self.memory.alloc_map.remove(&alloc_id) calls above ensure that this allocation is valid, unless you mean that the allocation itself could contain values that are dangling pointers to other stuff?

[RalfJung]

Yes that's what I mean.

[oli-obk]

It's a bit easier in this situation than during interning, because we don't need to intern nested allocations, so you could just iterate over the relocation table and check if all of the relocations in it are already interned

[RalfJung]

I don't think we want that, as already mentioned it would make it impossible to use this on cyclic structures.

[fee1-dead]

I'm thinking of only marking it as immutable here and leaving it for the interning to actually make it into global.

[oli-obk]

Please also add a test for a cyclic globalification

[fee1-dead]

Please also add a test for a cyclic globalification

This turned out to be the hardest part that I haven't found enough time to come up with an adequate change to address. See the following snippet:

#![feature(core_intrinsics)]
#![feature(const_heap)]

use std::mem::{align_of, size_of};
use std::intrinsics;

struct SelfReferential {
    me: *const SelfReferential,
}

const Y: &'static SelfReferential = unsafe {
    let size = size_of::<SelfReferential>();
    let align = align_of::<SelfReferential>();
    let ptr_raw = intrinsics::const_allocate(size, align);
    let ptr = ptr_raw as *mut SelfReferential;
    let me_addr = &raw mut (*ptr).me;
    *me_addr = ptr;
    intrinsics::const_make_global(ptr_raw, size, align);
    &*ptr
};

fn main() {}

• me's provenance says it wasn't derived from an immutable reference. We can't really change that because it can't be mutated after we call const_make_global.
• The fact above hits "accepted a mutable pointer that should not have accepted" at

  rust/compiler/rustc_const_eval/src/interpret/intern.rs

  Lines 247 to 265 in 040e2f8

  	// Ensure that this is derived from a shared reference. Crucially, we check this *before*
  	// checking whether the `alloc_id` has already been interned. The point of this check is to
  	// ensure that when there are multiple pointers to the same allocation, they are *all*
  	// derived from a shared reference. Therefore it would be bad if we only checked the first
  	// pointer to any given allocation.
  	// (It is likely not possible to actually have multiple pointers to the same allocation,
  	// so alternatively we could also check that and ICE if there are multiple such pointers.)
  	// See <https://github.com/rust-lang/rust/pull/128543> for why we are checking for "shared
  	// reference" and not "immutable", i.e., for why we are allowing interior-mutable shared
  	// references: they can actually be created in safe code while pointing to apparently
  	// "immutable" values, via promotion or tail expression lifetime extension of
  	// `&None::<Cell<T>>`.
  	// We also exclude promoteds from this as `&mut []` can be promoted, which is a mutable
  	// reference pointing to an immutable (zero-sized) allocation. We rely on the promotion
  	// analysis not screwing up to ensure that it is sound to intern promoteds as immutable.
  	if intern_kind != InternKind::Promoted
  	&& inner_mutability == Mutability::Not
  	&& !prov.shared_ref()
  	{

  .
• We can check if alloc_id has kind Heap { was_made_immut: true } and skip the error that way.
• intern_shallow will keep returning alloc2 adding to our todo list being weird left and right. I added a .filter in

  rust/compiler/rustc_const_eval/src/interpret/intern.rs

  Lines 311 to 312 in 040e2f8

  	match intern_shallow(ecx, alloc_id, inner_mutability, Some(&mut disambiguator)) {
  	Ok(nested) => todo.extend(nested),

  to skip provs that are in just_interned.
• Apparently that wasn't enough and rustc hits an infinite loop at some point. The stack trace isn't helpful enough and I didn't have enough time to find a way to debug the stack overflow.

[RalfJung]

Hm, we will probably have to relax that "derived from shared ref" check for heap allocations... which should be fine since those are all explicitly marked as immutable via make_global so nobody can complain if it is UB to mutate them.

[RalfJung]

Maybe we leave the cyclic part to a next PR, seems easier to review this one that way. At least the API is done in a way that cyclic globalification should be possible.

[oli-obk]

In terms of naming, I think we said that Box::leak would not be performing this operation, so "leak" seems like a suboptimal choice. Maybe something like "make_global" or "into_static" or so?

The original plan I proposed also makes the allocation immutable when it gets marked as global -- basically, this operation should indicate that you are done preparing this allocation and it is ready to be interned. (We actually intern it later as that's easier, but for the API this does not matter.)

We already have a name for this: static promotion. What do you think about reusing that terminology here?

[oli-obk]

It's a bit easier in this situation than during interning, because we don't need to intern nested allocations, so you could just iterate over the relocation table and check if all of the relocations in it are already interned

[RalfJung]

We already have a name for this: static promotion. What do you think about reusing that terminology here?

So what would the API look like then, Box::promote? I think that's not very informative, it's unclear what it gets promoted to. Box::promote_to_static would clarify that but is very long; Box::into_global seems equally clear but shorter.

I think of static promotion as a MIR transformation. In that sense, it has very little to do with this PR, which is about achieving a similar result but via very different means.

[RalfJung]

FWIW PR name, description, and commit message also still need updating for the new term.

[rustbot]

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri, @RalfJung, @oli-obk, @lcnr

[fee1-dead]

Note that this actually doesn't fix #129233, and the crash doesn't go away, because the ICE code happens before intern_shallow is called, the latter gives us the actually good error.

On the other hand, a followup to indeed fix the issue (an escape hatch from interning yelling about const_allocate allocations) should test that we do require const_make_global to be called before storing that *mut i32. It'd obviously be UB for any runtime code to actually mutate through that *mut, but that's the best we can do as we can't really prevent anyone from storing pointers derived from the original *mut u8.

[RalfJung]

That seems very confusing to communicate different errors via entirely different channels. Can it be avoided?

[fee1-dead]

It takes a lot of extra work when I try to avoid it. If I return an error immediately then it won't get interned - so we'd have to store it in the dead memory map - this leads to additional errors of dangling pointers if someone stores a non-made-global pointer in the final value (we'd detect the dead allocation).

If we don't return an error immediately then proceed with a shallow intern, then we won't be able to intern recursively (only the Ok variant gives nested things to intern) without some more complex machinery or design.

[RalfJung]

Hm... I need find some time to take a closer look at the code then to understand why you'd add this to the dead alloc map etc.

[fee1-dead]

It ICEs in get_alloc_info because the alloc gets removed during interning - maybe I should try re-inserting it into the alloc_map? (wouldn't that hit another check because we're then not interning everything in the alloc_map..?)

[RalfJung]

I think ideally it is never removed from the alloc_map if it can't get interned.

However we have to worry about hitting the same allocation multiple times then, we should avoid duplicate errors.

[RalfJung]

Thinking about it more, maybe the current logic is not so bad. It means we show the error only once for each allocation, but we do show it for all affected allocations. I still plan to take a closer look at the code.

What's not great is the resulting error -- ideally we'd delay showing these until after validation, so validation can show a better error if it finds the same allocation. Right now it can be quite hard to figure out where exactly there is a not-properly-global'd pointer. But we can leave better errors to a future PR.

[fee1-dead]

r? RalfJung since you looked at this extensively, lemme know if further changes are needed :)

[RalfJung]

I have some more suggestions that I wanted to push to your branch, which usually is permitted in PRs, but it seems you disabled that, so please take this diff instead:

diff --git a/compiler/rustc_const_eval/src/const_eval/machine.rs b/compiler/rustc_const_eval/src/const_eval/machine.rs
index 39791b7085d..ffb579aaab5 100644
--- a/compiler/rustc_const_eval/src/const_eval/machine.rs
+++ b/compiler/rustc_const_eval/src/const_eval/machine.rs
@@ -169,7 +169,11 @@ fn get_mut_or<E>(&mut self, k: K, vacant: impl FnOnce() -> Result<V, E>) -> Resu
 
 #[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 {
diff --git a/compiler/rustc_const_eval/src/interpret/intern.rs b/compiler/rustc_const_eval/src/interpret/intern.rs
index 85524949053..b8fcdc9b7ef 100644
--- a/compiler/rustc_const_eval/src/interpret/intern.rs
+++ b/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 });
             }
diff --git a/compiler/rustc_const_eval/src/interpret/memory.rs b/compiler/rustc_const_eval/src/interpret/memory.rs
index f52dc06f173..d855d5980ab 100644
--- a/compiler/rustc_const_eval/src/interpret/memory.rs
+++ b/compiler/rustc_const_eval/src/interpret/memory.rs
@@ -312,7 +312,7 @@ pub fn reallocate_ptr(
         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 @@ pub fn make_const_heap_ptr_global(
             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 @@ pub fn make_const_heap_ptr_global(
             }
         }
 
+        // Prevent further mutation, this is now an immutable global.
+        alloc.mutability = Mutability::Not;
+
         interp_ok(())
     }
 
diff --git a/compiler/rustc_const_eval/src/util/check_validity_requirement.rs b/compiler/rustc_const_eval/src/util/check_validity_requirement.rs
index 1c2167a50fd..33478aa3a99 100644
--- a/compiler/rustc_const_eval/src/util/check_validity_requirement.rs
+++ b/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();
 

[RalfJung]

Should calling it twice really be an error? Seems innocent enough to allow it.

[fee1-dead]

My thinking in making this an error is that the input to const_make_global requires a mutable pointer and thus it must point to a mutable const_heap allocation. It's not an error if we allowed it but it might be indicative of errors in safe wrappers on unsafe code.

We could discuss this more and make a followup on this if this should be changed IMO.

[RalfJung]

Suggested change

	#![feature(core_intrinsics)]
	//! Ensure that once an allocation is "made global", we can no longer mutate it.
	#![feature(core_intrinsics)]

Some of the other tests could also use similar comments explaining their purpose.

[RalfJung]

Does this test still have any point? The comment at the top is already outdated, and ptr_not_made_global checks the same thing more explicitly, doesn't it?

[fee1-dead]

Yes, because if we don't unleash miri here it ICEs and needs fixing once #129233 is fixed.

ptr_not_made_global is different because that has immutable pointers in the final value (unlike this file)

[RalfJung]

@rustbot author

[RalfJung]

Also, could you file an issue about the problem with recursive globalification? We should look at that before stabilization, in particular if it ICEs. ;)

[fee1-dead]

@rustbot ready