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

Fixes #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