const-eval interning: get rid of type-driven traversal

[RalfJung]

This entirely replaces our const-eval interner, i.e. the code that takes the final result of a constant evaluation from the local memory of the const-eval machine to the global tcx memory. The main goal of this change is to ensure that we can detect mutable references that sneak into this final value -- this is something we want to reject for static and const, and while const-checking performs some static analysis to ensure this, I would be much more comfortable stabilizing const_mut_refs if we had a dynamic check that sanitizes the final value. (This is generally the approach we have been using on const-eval: do a static check to give nice errors upfront, and then do a dynamic check to be really sure that the properties we need for soundness, actually hold.)

We can do this now that #118324 landed and each pointer comes with a bit (completely independent of its type) storing whether mutation is permitted through this pointer or not.

The new interner is a lot simpler than the old one: previously we did a complete type-driven traversal to determine the mutability of all memory we see, and then a second pass to intern any leftover raw pointers. The new interner simply recursively traverses the allocation holding the final result, and all allocations reachable from it (which can be determined from the raw bytes of the result, without knowing anything about types), and ensures they all get interned. The initial allocation is interned as immutable for const and pomoted and non-interior-mutable static; all other allocations are interned as immutable for static, const, and promoted. The main subtlety is justifying that those inner allocations may indeed be interned immutably, i.e., that mutating them later would anyway already be UB:

• for promoteds, we rely on the analysis that does promotion to ensure that this is sound.
• for const and static, we check that all pointers in the final result that point to things that are new (i.e., part of this const evaluation) are immutable, i.e., were created via &<expr> at a non-interior-mutable type. Mutation through immutable pointers is UB so we are free to intern that memory as immutable.

Interning raises an error if it encounters a dangling pointer or a mutable pointer that violates the above rules.

I also extended our type-driven const validity checks to ensure that &mut T in the final value of a const points to mutable memory, at least if T is not zero-sized. This catches cases of people turning &i32 into &mut i32 (which would still be considered a read-only pointer). Similarly, when these checks encounter an UnsafeCell, they are checking that it lives in mutable memory. (Both of these only traverse the newly created values; if those point to other consts/promoteds, the check stops there. But that's okay, we don't have to catch all the UB.) I co-developed this with the stricter interner changes but I can split it out into a separate PR if you prefer.

This PR does have the immediate effect of allowing some new code on stable, for instance:

const CONST_RAW: *const Vec<i32> = &Vec::new() as *const _;

Previously that code got rejected since the type-based interner didn't know what to do with that pointer. It's a raw pointer, we cannot trust its type. The new interner does not care about types so it sees no issue with this code; there's an immutable pointer pointing to some read-only memory (storing a Vec<i32>), all is good. Accepting this code pretty much commits us to non-type-based interning, but I think that's the better strategy anyway.

This PR also leads to slightly worse error messages when the final value of a const contains a dangling reference. Previously we would complete interning and then the type-based validation would detect this dangling reference and show a nice error saying where in the value (i.e., in which field) the dangling reference is located. However, the new interner cannot distinguish dangling references from dangling raw pointers, so it must throw an error when it encounters either of them. It doesn't have an understanding of the value structure so all it can say is "somewhere in this constant there's a dangling pointer". (Later parts of the compiler don't like dangling pointers/references so we have to reject them either during interning or during validation.) This could potentially be improved by doing validation before interning, but that's a larger change that I have not attempted yet. (It's also subtle since we do want validation to use the final mutability bits of all involved allocations, and currently it is interning that marks a bunch of allocations as immutable -- that would have to still happen before validation.)

@rust-lang/wg-const-eval I hope you are okay with this plan. :)
@rust-lang/lang paging you in since this accepts new code on stable as explained above. Please let me know if you think FCP is necessary.

This is a successor to #116745.

[rustbot]

r? @wesleywiser

(rustbot has picked a reviewer for you, use r? to override)

[rustbot]

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

The Miri subtree was changed

cc @rust-lang/miri

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

[RalfJung]

There are perf results here, and they look quite positive. :)

[tmandry]

We discussed this in the lang team meeting but didn't entirely grok what it does from the description.

@RalfJung Can you give an algorithmic summary of the check we used to do and the check this PR implements, along with an example of something that wouldn't compile under this PR?

[traviscross]

@RalfJung: In terms of the algorithmic summary mentioned by @tmandry, it may help if you could step us carefully through why...

const CONST_RAW: *const Vec<i32> = &Vec::new() as *const _;

...this example does not compile today and the theory and algorithm of why it would under this PR.

[RalfJung]

To give some context, the only non-trivial job the interner has is to decide with which mutability the allocations found in the just-evaluated constant should be interned. Allocations generally start out mutable (after all, they are uninitialized at first and then have their initial value written into them during const-eval, which requires them to be mutable), but we want to put them in read-only memory in the final binary if possible.

Usually there is only a single allocation to be interned: the one that holds the final value of the constant. However, there is some code we accept where there is more than one allocation to intern. For instance:

const CONST_VEC: &Vec<i32> = &Vec::new();

There are two allocations to intern here, the one that stores the Vec and the one that stores the reference to the Vec. We sometimes call these secondary allocations that also need interning "inner" allocations.

(This is not promotion! Vec does not get promoted since it has a destructor. Instead this is the "outer scope rule" that gives certain expressions the lifetime of the "outer" scope. For const/static, the "outer" scope is 'static, i.e., this implicitly creates another global. This seems to be somewhat accidental, or at least it was never properly specified or discussed to my knowledge. It also causes some subtle bugs since we do not have a system in place to manage the identity of these globals.)

Old interner

The old interner works as follows: we do a type-based traversal of the final value of the const. That means we recursively go through fields of structs, tuples, enums, ... and when we encounter a reference we also descend into that.

During this descend, we keep track of the current mutability. We start out immutable (except for static mut where we start mutable). When we go into an UnsafeCell we switch to mutable. When we encounter a shared reference we switch to immutable. When we hit a reference to a not-yet-interned allocation, we use the current mutability plus a Freeze check to determine whether to intern that as mutable or immutable.

We also keep track of any other pointer values we find that are not references (raw pointers, things in union fields, and in theory there could also be pointers stored in padding between fields and similar nasty things). These are the "leftover" pointers. In static, we intern everything from that list as mutable; in const, we raise an error if there are any leftover pointers.

New interner

The new internet is a lot simpler. Our goal with static and const is to intern everything as immutable, except possibly the original allocation storing the value itself: a static A: AtomicUisze must be put in mutable memory, of course. We just have to verify that this is sound. To this end we recursively traverse all pointers that we find (this doesn't require type-based traversal, pointers are special magic values and we can easily find them all -- we just don't know their type). All pointers to newly interned "inner" allocations must be immutable, i.e., they must be derived from a shared reference without interior mutability.

This relies on #118324, where we now track at const-eval time, using provenance, whether a pointer is mutable or not. When evaluating an &<place> expression, if the pointee is Freeze, we set that pointer as immutable, and all pointers derived from this inherit that bit.

This allows us to reject, for instance

const NOT_ACTUALLY_CONST_VEC: *const Vec<i32> = &mut Vec::new() as *mut _ as *const _;

We have to reject that code since if we accepted it, arguably we should say that NOT_ACTUALLY_CONST_VEC.cast_mut().write(...) is allowed -- there's nothing that would cause UB here, the pointer we are writing to was created via &mut after all. But we don't want const to implicitly create global mutable state.

Changes in behavior

This example used to be rejected due to leftover pointers:

const CONST_RAW: *const Vec<i32> = &Vec::new() as *const _;

The concept of a leftover pointer no longer exists, so we no longer reject this.

Note that this is still rejected:

const CONST_RAW: *const Vec<i32> = ptr::addr_of!(Vec::new());

Raw pointers to temporaries are not allowed; only references. (This is not even a const-check I think, but a more general rule.)

I am not aware of any code that we currently accept, that would get rejected after this change. We already refuse all code that would cause inner allocations with interior mutability to be created. However, the checks for that are a bit scattered and indirect; the new interner serves as a second line of defense to ensure that we really do not ever accept such code. We have "miri unleashed" tests to ensure this. (These tests disable many of the usual const checks, accepting basically anything in a const fn that the interpreter is able to execute.)

There might be things you can do with const_mut_refs or const_refs_to_cell that would be rejected now, but those are unstable features and we want to restrict them to ensure that nothing mutable leaks into the final value. Anything we currently accept there would be a bug. Closing any holes that we might currently have in these unstable features is the main motivation for this PR.

[bors]

☔ The latest upstream changes (presumably #119146) made this pull request unmergeable. Please resolve the merge conflicts.

[oli-obk]

why this additional check? Won't this also allow more code to compile?

[RalfJung]

How can this allow more code to compile? It rejects !Freeze unions in immutable memory. This is basically the union version of what we are checking here.

We could debate whether we should have that check; such UnsafeCell are actually fine if you don't write to them. They are certainly suspicious though.

[tmandry]

@rfcbot fcp merge

[rfcbot]

Team member @tmandry has proposed to merge this. The next step is review by the rest of the tagged team members:

• @joshtriplett
• @nikomatsakis
• @pnkfelix
• @scottmcm
• @tmandry

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

[nikomatsakis]

@rfcbot reviewed

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[nikomatsakis]

One thing that seems important to me here is that we already have precedent for "value-based" restrictions, since e.g. I believe const X: Option<Foo> = None works for any Foo.

[RalfJung]

Yes absolutely, many things are value-based around consts.

[RalfJung]

@bors r=oli-obk

[bors]

📌 Commit 2ab85e4 has been approved by oli-obk

It is now in the queue for this repository.

[bors]

⌛ Testing commit 2ab85e4 with merge 6265a95...

[bors]

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 6265a95 to master...

[rust-timer]

Finished benchmarking commit (6265a95): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.9%	[-1.1%, -0.8%]	3
Improvements ✅ (secondary)	-3.8%	[-8.0%, -0.6%]	14
All ❌✅ (primary)	-0.9%	[-1.1%, -0.8%]	3

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	3.0%	[3.0%, 3.0%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-6.0%	[-8.8%, -3.5%]	9
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 661.891s -> 662.447s (0.08%)
Artifact size: 308.33 MiB -> 308.28 MiB (-0.01%)

[RalfJung]

Turns out this was actually an accidental breaking change: #121610.