-
Notifications
You must be signed in to change notification settings - Fork 12.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
const-eval interning: get rid of type-driven traversal #119044
Conversation
r? @wesleywiser (rustbot has picked a reviewer for you, use r? to override) |
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 |
There are perf results here, and they look quite positive. :) |
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 (This is not promotion! Old internerThe 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 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 New internerThe new internet is a lot simpler. Our goal with This relies on #118324, where we now track at const-eval time, using provenance, whether a pointer is mutable or not. When evaluating an 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 Changes in behaviorThis 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 There might be things you can do with |
☔ The latest upstream changes (presumably #119146) made this pull request unmergeable. Please resolve the merge conflicts. |
5050583
to
a97ab84
Compare
// Special check for CTFE validation, preventing `UnsafeCell` inside unions in immutable memory. | ||
if self.ctfe_mode.is_some_and(|c| !c.allow_immutable_unsafe_cell()) { | ||
if !op.layout.is_zst() && !op.layout.ty.is_freeze(*self.ecx.tcx, self.ecx.param_env) { | ||
if !self.in_mutable_memory(op) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why this additional check? Won't this also allow more code to compile?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@rfcbot fcp merge |
Team member @tmandry has proposed to merge this. The next step is review by the rest of the tagged team members: 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. |
This comment has been minimized.
This comment has been minimized.
7cad54a
to
cb591db
Compare
@rfcbot reviewed |
🔔 This is now entering its final comment period, as per the review above. 🔔 |
One thing that seems important to me here is that we already have precedent for "value-based" restrictions, since e.g. I believe |
Yes absolutely, many things are value-based around consts.
|
Co-authored-by: Oli Scherer <github35764891676564198441@oli-obk.de>
42c4e69
to
2ab85e4
Compare
@bors r=oli-obk |
☀️ Test successful - checks-actions |
Finished benchmarking commit (6265a95): comparison URL. Overall result: ✅ improvements - no action needed@rustbot label: -perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis 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.
CyclesResultsThis 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.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 661.891s -> 662.447s (0.08%) |
…li-obk unstably allow constants to refer to statics and read from immutable statics I am not aware of any fundamental reason why we cannot allow constants to mention statics. What we really need is that constants do not *read from* statics that can change their value: - This would break the principle that "constants behave as-if their expression was inlined everywhere and executed at runtime". This is enforced by halting const-eval interpretation when a read from a mutable global occurs. - When building a valtree we want to be sure that the constant and everything it refers to is truly immutable. This is enforced by aborting valtree construction when a read from a mutable global occurs. r? `@oli-obk` -- if you are okay with experimenting with this feature, I will create a tracking issue. Based on and blocked on rust-lang#119044; only the last commit is new.
…alfJung various const interning cleanups After rust-lang#119044 I noticed that some things can be simplified and refactored. This is also a requirement for rust-lang#116564 as there we'll need to treat the base allocation differently from the others r? `@RalfJung`
…alfJung various const interning cleanups After rust-lang#119044 I noticed that some things can be simplified and refactored. This is also a requirement for rust-lang#116564 as there we'll need to treat the base allocation differently from the others r? ``@RalfJung``
…alfJung various const interning cleanups After rust-lang#119044 I noticed that some things can be simplified and refactored. This is also a requirement for rust-lang#116564 as there we'll need to treat the base allocation differently from the others r? ```@RalfJung```
…alfJung various const interning cleanups After rust-lang#119044 I noticed that some things can be simplified and refactored. This is also a requirement for rust-lang#116564 as there we'll need to treat the base allocation differently from the others r? ````@RalfJung````
Rollup merge of rust-lang#120302 - oli-obk:const_intern_cleanups, r=RalfJung various const interning cleanups After rust-lang#119044 I noticed that some things can be simplified and refactored. This is also a requirement for rust-lang#116564 as there we'll need to treat the base allocation differently from the others r? ````@RalfJung````
…-obk unstably allow constants to refer to statics and read from immutable statics I am not aware of any fundamental reason why we cannot allow constants to mention statics. What we really need is that constants do not *read from* statics that can change their value: - This would break the principle that "constants behave as-if their expression was inlined everywhere and executed at runtime". This is enforced by halting const-eval interpretation when a read from a mutable global occurs. - When building a valtree we want to be sure that the constant and everything it refers to is truly immutable. This is enforced by aborting valtree construction when a read from a mutable global occurs. r? `@oli-obk` -- if you are okay with experimenting with this feature, I will create a tracking issue. Based on and blocked on rust-lang#119044; only the last commit is new.
…-obk unstably allow constants to refer to statics and read from immutable statics I am not aware of any fundamental reason why we cannot allow constants to mention statics. What we really need is that constants do not *read from* statics that can change their value: - This would break the principle that "constants behave as-if their expression was inlined everywhere and executed at runtime". This is enforced by halting const-eval interpretation when a read from a mutable global occurs. - When building a valtree we want to be sure that the constant and everything it refers to is truly immutable. This is enforced by aborting valtree construction when a read from a mutable global occurs. r? `@oli-obk` -- if you are okay with experimenting with this feature, I will create a tracking issue. Based on and blocked on rust-lang#119044; only the last commit is new.
unstably allow constants to refer to statics and read from immutable statics I am not aware of any fundamental reason why we cannot allow constants to mention statics. What we really need is that constants do not *read from* statics that can change their value: - This would break the principle that "constants behave as-if their expression was inlined everywhere and executed at runtime". This is enforced by halting const-eval interpretation when a read from a mutable global occurs. - When building a valtree we want to be sure that the constant and everything it refers to is truly immutable. This is enforced by aborting valtree construction when a read from a mutable global occurs. r? `@oli-obk` -- if you are okay with experimenting with this feature, I will create a tracking issue. Based on and blocked on rust-lang/rust#119044; only the last commit is new.
Turns out this was actually an accidental breaking change: #121610. |
Pkgsrc changes: * Adapt checksums and patches. Upstream chnages: Version 1.77.0 (2024-03-21) ========================== - [Reveal opaque types within the defining body for exhaustiveness checking.] (rust-lang/rust#116821) - [Stabilize C-string literals.] (rust-lang/rust#117472) - [Stabilize THIR unsafeck.] (rust-lang/rust#117673) - [Add lint `static_mut_refs` to warn on references to mutable statics.] (rust-lang/rust#117556) - [Support async recursive calls (as long as they have indirection).] (rust-lang/rust#117703) - [Undeprecate lint `unstable_features` and make use of it in the compiler.] (rust-lang/rust#118639) - [Make inductive cycles in coherence ambiguous always.] (rust-lang/rust#118649) - [Get rid of type-driven traversal in const-eval interning] (rust-lang/rust#119044), only as a [future compatiblity lint] (rust-lang/rust#122204) for now. - [Deny braced macro invocations in let-else.] (rust-lang/rust#119062) Compiler -------- - [Include lint `soft_unstable` in future breakage reports.] (rust-lang/rust#116274) - [Make `i128` and `u128` 16-byte aligned on x86-based targets.] (rust-lang/rust#116672) - [Use `--verbose` in diagnostic output.] (rust-lang/rust#119129) - [Improve spacing between printed tokens.] (rust-lang/rust#120227) - [Merge the `unused_tuple_struct_fields` lint into `dead_code`.] (rust-lang/rust#118297) - [Error on incorrect implied bounds in well-formedness check] (rust-lang/rust#118553), with a temporary exception for Bevy. - [Fix coverage instrumentation/reports for non-ASCII source code.] (rust-lang/rust#119033) - [Fix `fn`/`const` items implied bounds and well-formedness check.] (rust-lang/rust#120019) - [Promote `riscv32{im|imafc}-unknown-none-elf` targets to tier 2.] (rust-lang/rust#118704) - Add several new tier 3 targets: - [`aarch64-unknown-illumos`] (rust-lang/rust#112936) - [`hexagon-unknown-none-elf`] (rust-lang/rust#117601) - [`riscv32imafc-esp-espidf`] (rust-lang/rust#119738) - [`riscv32im-risc0-zkvm-elf`] (rust-lang/rust#117958) Refer to Rust's [platform support page][platform-support-doc] for more information on Rust's tiered platform support. Libraries --------- - [Implement `From<&[T; N]>` for `Cow<[T]>`.] (rust-lang/rust#113489) - [Remove special-case handling of `vec.split_off (0)`.](rust-lang/rust#119917) Stabilized APIs --------------- - [`array::each_ref`] (https://doc.rust-lang.org/stable/std/primitive.array.html#method.each_ref) - [`array::each_mut`] (https://doc.rust-lang.org/stable/std/primitive.array.html#method.each_mut) - [`core::net`] (https://doc.rust-lang.org/stable/core/net/index.html) - [`f32::round_ties_even`] (https://doc.rust-lang.org/stable/std/primitive.f32.html#method.round_ties_even) - [`f64::round_ties_even`] (https://doc.rust-lang.org/stable/std/primitive.f64.html#method.round_ties_even) - [`mem::offset_of!`] (https://doc.rust-lang.org/stable/std/mem/macro.offset_of.html) - [`slice::first_chunk`] (https://doc.rust-lang.org/stable/std/primitive.slice.html#method.first_chunk) - [`slice::first_chunk_mut`] (https://doc.rust-lang.org/stable/std/primitive.slice.html#method.first_chunk_mut) - [`slice::split_first_chunk`] (https://doc.rust-lang.org/stable/std/primitive.slice.html#method.split_first_chunk) - [`slice::split_first_chunk_mut`] (https://doc.rust-lang.org/stable/std/primitive.slice.html#method.split_first_chunk_mut) - [`slice::last_chunk`] (https://doc.rust-lang.org/stable/std/primitive.slice.html#method.last_chunk) - [`slice::last_chunk_mut`] (https://doc.rust-lang.org/stable/std/primitive.slice.html#method.last_chunk_mut) - [`slice::split_last_chunk`] (https://doc.rust-lang.org/stable/std/primitive.slice.html#method.split_last_chunk) - [`slice::split_last_chunk_mut`] (https://doc.rust-lang.org/stable/std/primitive.slice.html#method.split_last_chunk_mut) - [`slice::chunk_by`] (https://doc.rust-lang.org/stable/std/primitive.slice.html#method.chunk_by) - [`slice::chunk_by_mut`] (https://doc.rust-lang.org/stable/std/primitive.slice.html#method.chunk_by_mut) - [`Bound::map`] (https://doc.rust-lang.org/stable/std/ops/enum.Bound.html#method.map) - [`File::create_new`] (https://doc.rust-lang.org/stable/std/fs/struct.File.html#method.create_new) - [`Mutex::clear_poison`] (https://doc.rust-lang.org/stable/std/sync/struct.Mutex.html#method.clear_poison) - [`RwLock::clear_poison`] (https://doc.rust-lang.org/stable/std/sync/struct.RwLock.html#method.clear_poison) Cargo ----- - [Extend the build directive syntax with `cargo::`.] (rust-lang/cargo#12201) - [Stabilize metadata `id` format as `PackageIDSpec`.] (rust-lang/cargo#12914) - [Pull out as `cargo-util-schemas` as a crate.] (rust-lang/cargo#13178) - [Strip all debuginfo when debuginfo is not requested.] (rust-lang/cargo#13257) - [Inherit jobserver from env for all kinds of runners.] (rust-lang/cargo#12776) - [Deprecate rustc plugin support in cargo.] (rust-lang/cargo#13248) Rustdoc ----- - [Allows links in markdown headings.] (rust-lang/rust#117662) - [Search for tuples and unit by type with `()`.] (rust-lang/rust#118194) - [Clean up the source sidebar's hide button.] (rust-lang/rust#119066) - [Prevent JS injection from `localStorage`.] (rust-lang/rust#120250) Misc ---- - [Recommend version-sorting for all sorting in style guide.] (rust-lang/rust#115046) Internal Changes ---------------- These changes do not affect any public interfaces of Rust, but they represent significant improvements to the performance or internals of rustc and related tools. - [Add more weirdness to `weird-exprs.rs`.] (rust-lang/rust#119028)
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 forstatic
andconst
, 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-mutablestatic
; all other allocations are interned as immutable forstatic
,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:const
andstatic
, 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 ifT
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 anUnsafeCell
, 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:
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.