-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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: accept interior mutable pointers in final value #128543
Conversation
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
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.
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. |
☀️ 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%) |
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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
…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"
UnsafeCell
is, which is a wide open question.) However, we explicitly document that things created byconst
are 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
UnsafeCell
should be tracked fully precisely, then you should want the lint we currently emit to be removed, which this PR does. If you thinkUnsafeCell
should "infect" surroundingenum
s, 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 surroundingconst
explaining 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