Skip to content

Commit

Permalink
Auto merge of #119044 - RalfJung:intern-without-types, r=oli-obk
Browse files Browse the repository at this point in the history
const-eval interning: get rid of type-driven traversal

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 rust-lang/rust#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:
```rust
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.
  • Loading branch information
bors committed Jan 23, 2024
2 parents 58b9062 + cdc7f9c commit 42c94f5
Show file tree
Hide file tree
Showing 2 changed files with 6 additions and 2 deletions.
4 changes: 3 additions & 1 deletion src/borrow_tracker/stacked_borrows/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -830,7 +830,9 @@ trait EvalContextPrivExt<'mir: 'ecx, 'tcx: 'mir, 'ecx>: crate::MiriInterpCxExt<'
let new_prov = this.sb_reborrow(place, size, new_perm, new_tag, info)?;

// Adjust place.
Ok(place.clone().map_provenance(|_| new_prov))
// (If the closure gets called, that means the old provenance was `Some`, and hence the new
// one must also be `Some`.)
Ok(place.clone().map_provenance(|_| new_prov.unwrap()))
}

/// Retags an individual pointer, returning the retagged version.
Expand Down
4 changes: 3 additions & 1 deletion src/borrow_tracker/tree_borrows/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -351,7 +351,9 @@ trait EvalContextPrivExt<'mir: 'ecx, 'tcx: 'mir, 'ecx>: crate::MiriInterpCxExt<'
let new_prov = this.tb_reborrow(place, reborrow_size, new_perm, new_tag)?;

// Adjust place.
Ok(place.clone().map_provenance(|_| new_prov))
// (If the closure gets called, that means the old provenance was `Some`, and hence the new
// one must also be `Some`.)
Ok(place.clone().map_provenance(|_| new_prov.unwrap()))
}

/// Retags an individual pointer, returning the retagged version.
Expand Down

0 comments on commit 42c94f5

Please sign in to comment.