const-eval interning: accept interior mutable pointers in final value #128543
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
r? @nnethercote rustbot has assigned @nnethercote. Use |
|
Some changes occurred to the CTFE / Miri engine cc @rust-lang/miri Some changes occurred to the CTFE / Miri engine cc @rust-lang/miri |
5371142 to
d0e95a9
Compare
This comment has been minimized.
This comment has been minimized.
d0e95a9 to
bf4231a
Compare
This comment has been minimized.
This comment has been minimized.
bf4231a to
7b2cfd4
Compare
|
I don't feel comfortable reviewing this. @saethlin, are you a suitable reviewer? (I ask based on your membership of the opsem group combined with your r+ privileges.) |
|
Me or @oli-obk (the implementation here is more interpreter internals, at a glance I don't think the opsem part of this is controversial) |
|
Oli is away, so: r? @saethlin Thanks! |
|
☔ The latest upstream changes (presumably #128672) made this pull request unmergeable. Please resolve the merge conflicts. |
7b2cfd4 to
f1e47e2
Compare
9b3e7d8 to
a2a238f
Compare
This was referenced Aug 17, 2024
fee1-dead
requested changes
Aug 17, 2024
fee1-dead
approved these changes
Aug 17, 2024
There was a problem hiding this comment.
Overall the compiler changes LGTM.
There is the ultimate question of "do we really want to do this". I don't personally like it, but you've probably sat on this for a while. r=me if there is official approval from T-lang.
Though question: is this needed for stabilizing mut refs in consts? The changes look tangential to me.
This comment was marked as resolved.
This comment was marked as resolved.
…(but keep rejecting mutable references)
9317d46 to
123757a
Compare
|
The final comment period, with a disposition to merge, as per the review above, is now complete. As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed. This will be merged soon. |
3 tasks
|
☀️ Test successful - checks-actions |
|
Finished benchmarking commit (9b72238): comparison URL. Overall result: ❌ regressions - 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)This benchmark run did not return any relevant results for this metric. CyclesResults (secondary -2.3%)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.
Binary sizeResults (secondary 0.4%)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.
Bootstrap: 760.71s -> 758.697s (-0.26%) |
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this pull request
Sep 15, 2024
Stabilize `&mut` (and `*mut`) as well as `&Cell` (and `*const Cell`) in const
This stabilizes `const_mut_refs` and `const_refs_to_cell`. That allows a bunch of new things in const contexts:
- Mentioning `&mut` types
- Creating `&mut` and `*mut` values
- Creating `&T` and `*const T` values where `T` contains interior mutability
- Dereferencing `&mut` and `*mut` values (both for reads and writes)
The same rules as at runtime apply: mutating immutable data is UB. This includes mutation through pointers derived from shared references; the following is diagnosed with a hard error:
```rust
#[allow(invalid_reference_casting)]
const _: () = {
let mut val = 15;
let ptr = &val as *const i32 as *mut i32;
unsafe { *ptr = 16; }
};
```
The main limitation that is enforced is that the final value of a const (or non-`mut` static) may not contain `&mut` values nor interior mutable `&` values. This is necessary because the memory those references point to becomes *read-only* when the constant is done computing, so (interior) mutable references to such memory would be pretty dangerous. We take a multi-layered approach here to ensuring no mutable references escape the initializer expression:
- A static analysis rejects (interior) mutable references when the referee looks like it may outlive the current MIR body.
- To be extra sure, this static check is complemented by a "safety net" of dynamic checks. ("Dynamic" in the sense of "running during/after const-evaluation, e.g. at runtime of this code" -- in contrast to "static" which works entirely by looking at the MIR without evaluating it.)
- After the final value is computed, we do a type-driven traversal of the entire value, and if we find any `&mut` or interior-mutable `&` we error out.
- However, the type-driven traversal cannot traverse `union` or raw pointers, so there is a second dynamic check where if the final value of the const contains any pointer that was not derived from a shared reference, we complain. This is currently a future-compat lint, but will become an ICE in rust-lang#128543. On the off-chance that it's actually possible to trigger this lint on stable, I'd prefer if we could make it an ICE before stabilizing const_mut_refs, but it's not a hard blocker. This part of the "safety net" is only active for mutable references since with shared references, it has false positives.
Altogether this should prevent people from leaking (interior) mutable references out of the const initializer.
While updating the tests I learned that surprisingly, this code gets rejected:
```rust
const _: Vec<i32> = {
let mut x = Vec::<i32>::new(); //~ ERROR destructor of `Vec<i32>` cannot be evaluated at compile-time
let r = &mut x;
let y = x;
y
};
```
The analysis that rejects destructors in `const` is very conservative when it sees an `&mut` being created to `x`, and then considers `x` to be always live. See [here](rust-lang#65394 (comment)) for a longer explanation. `const_precise_live_drops` will solve this, so I consider this problem to be tracked by rust-lang#73255.
Cc `@rust-lang/wg-const-eval` `@rust-lang/lang`
Cc rust-lang#57349
Cc rust-lang#80384
Comment on lines
+263
to
+268
| if !ecx.tcx.sess.opts.unstable_opts.unleash_the_miri_inside_of_you { | ||
| span_bug!( | ||
| ecx.tcx.span, | ||
| "the static const safety checks accepted mutable references they should not have accepted" | ||
| ); | ||
| } |
There was a problem hiding this comment.
this one is reachable as well but from what I have seen all the snippets involve intrinsics::const_allocate() abuse 🤷
#![feature(const_heap)]
const BAR: *mut i32 = unsafe { std::intrinsics::const_allocate(4, 4) as *mut i32 };There was a problem hiding this comment.
Yeah, I know -- I made the test that reaches this set "unleash miri" to keep it working.
const_heap has a bunch of unsolved open problems, this is just one of them. If this is at some point the last open problem, we can consider removing the check here, since it is anyway supposed to be just a safety net. But meanwhile having this ICE will help us ensure that just using mutable references, on cannot cause problems here.
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Sep 15, 2024
Rollup merge of rust-lang#129195 - RalfJung:const-mut-refs, r=fee1-dead Stabilize `&mut` (and `*mut`) as well as `&Cell` (and `*const Cell`) in const This stabilizes `const_mut_refs` and `const_refs_to_cell`. That allows a bunch of new things in const contexts: - Mentioning `&mut` types - Creating `&mut` and `*mut` values - Creating `&T` and `*const T` values where `T` contains interior mutability - Dereferencing `&mut` and `*mut` values (both for reads and writes) The same rules as at runtime apply: mutating immutable data is UB. This includes mutation through pointers derived from shared references; the following is diagnosed with a hard error: ```rust #[allow(invalid_reference_casting)] const _: () = { let mut val = 15; let ptr = &val as *const i32 as *mut i32; unsafe { *ptr = 16; } }; ``` The main limitation that is enforced is that the final value of a const (or non-`mut` static) may not contain `&mut` values nor interior mutable `&` values. This is necessary because the memory those references point to becomes *read-only* when the constant is done computing, so (interior) mutable references to such memory would be pretty dangerous. We take a multi-layered approach here to ensuring no mutable references escape the initializer expression: - A static analysis rejects (interior) mutable references when the referee looks like it may outlive the current MIR body. - To be extra sure, this static check is complemented by a "safety net" of dynamic checks. ("Dynamic" in the sense of "running during/after const-evaluation, e.g. at runtime of this code" -- in contrast to "static" which works entirely by looking at the MIR without evaluating it.) - After the final value is computed, we do a type-driven traversal of the entire value, and if we find any `&mut` or interior-mutable `&` we error out. - However, the type-driven traversal cannot traverse `union` or raw pointers, so there is a second dynamic check where if the final value of the const contains any pointer that was not derived from a shared reference, we complain. This is currently a future-compat lint, but will become an ICE in rust-lang#128543. On the off-chance that it's actually possible to trigger this lint on stable, I'd prefer if we could make it an ICE before stabilizing const_mut_refs, but it's not a hard blocker. This part of the "safety net" is only active for mutable references since with shared references, it has false positives. Altogether this should prevent people from leaking (interior) mutable references out of the const initializer. While updating the tests I learned that surprisingly, this code gets rejected: ```rust const _: Vec<i32> = { let mut x = Vec::<i32>::new(); //~ ERROR destructor of `Vec<i32>` cannot be evaluated at compile-time let r = &mut x; let y = x; y }; ``` The analysis that rejects destructors in `const` is very conservative when it sees an `&mut` being created to `x`, and then considers `x` to be always live. See [here](rust-lang#65394 (comment)) for a longer explanation. `const_precise_live_drops` will solve this, so I consider this problem to be tracked by rust-lang#73255. Cc `@rust-lang/wg-const-eval` `@rust-lang/lang` Cc rust-lang#57349 Cc rust-lang#80384
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
…but keep rejecting mutable references
This fixes #121610 by no longer firing the lint when there is a pointer with interior mutability in the final value of the constant. On stable, such pointers can be created with code like:
It's not great to accept such values since people might think that it is legal to mutate them with unsafe code. (This is related to how "infectious"
UnsafeCellis, which is a wide open question.) However, we explicitly document that things created byconstare immutable. Furthermore, we also accept the following even more questionable code without any lint today:This is even more questionable since it does not involve a
const, and yet still puts the data into immutable memory. We could view this as promotion potentially introducing UB. However, we've accepted this since ~forever and it's too late to reject this now; the pattern is just too useful.So basically, if you think that
UnsafeCellshould be tracked fully precisely, then you should want the lint we currently emit to be removed, which this PR does. If you thinkUnsafeCellshould "infect" surroundingenums, the big problem is really rust-lang/unsafe-code-guidelines#493 which does not trigger the lint -- the cases the lint triggers on are actually the "harmless" ones as there is an explicit surroundingconstexplaining why things end up being immutable.What all this goes to show is that the hard error added in #118324 (later turned into the future-compat lint that I am now suggesting we remove) was based on some wrong assumptions, at least insofar as it concerns shared references. Furthermore, that lint does not help at all for the most problematic case here where the potential UB is completely implicit. (In fact, the lint is actively in the way of my preferred long-term strategy for dealing with this UB.) So I think we should go back to square one and remove that error/lint for shared references. For mutable references, it does seem to work as intended, so we can keep it. Here it serves as a safety net in case the static checks that try to contain mutable references to the inside of a const initializer are not working as intended; I therefore made the check ICE to encourage users to tell us if that safety net is triggered.
Closes #122153 by removing the lint.
Cc @rust-lang/opsem @rust-lang/lang