const-eval interning: accept interior mutable pointers in final value #128543

RalfJung · 2024-08-02T13:46:13Z

…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:

pub enum JsValue {
    Undefined,
    Object(Cell<bool>),
}
impl Drop for JsValue {
    fn drop(&mut self) {}
}
// This does *not* get promoted since `JsValue` has a destructor.
// However, the outer scope rule applies, still giving this 'static lifetime.
const UNDEFINED: &JsValue = &JsValue::Undefined;

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 by const are immutable. Furthermore, we also accept the following even more questionable code without any lint today:

let x: &'static Option<Cell<i32>> = &None;

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 think UnsafeCell should "infect" surrounding enums, 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 surrounding const 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

rustbot · 2024-08-02T13:46:20Z

r? @nnethercote

rustbot has assigned @nnethercote.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

rustbot · 2024-08-02T13:46:22Z

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

nnethercote · 2024-08-04T23:26:36Z

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.)

saethlin · 2024-08-05T04:13:18Z

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)

nnethercote · 2024-08-05T05:27:57Z

Oli is away, so: r? @saethlin

Thanks!

bors · 2024-08-05T07:49:12Z

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

compiler/rustc_middle/src/mir/interpret/pointer.rs

tests/ui/consts/refs-to-cell-in-final.rs

fee1-dead

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.

…(but keep rejecting mutable references)

rfcbot · 2024-09-14T18:03:34Z

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.

RalfJung · 2024-09-14T21:04:48Z

Based on this
@bors r=fee1-dead

bors · 2024-09-14T21:04:51Z

📌 Commit 123757a has been approved by fee1-dead

It is now in the queue for this repository.

bors · 2024-09-14T21:11:08Z

⌛ Testing commit 123757a with merge 9b72238...

bors · 2024-09-14T23:46:52Z

☀️ Test successful - checks-actions
Approved by: fee1-dead
Pushing 9b72238 to master...

rust-timer · 2024-09-15T01:05:09Z

Finished benchmarking commit (9b72238): comparison URL.

Overall result: ❌ regressions - 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.3%	[0.3%, 0.3%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

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

Cycles

Results (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.

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

Binary size

Results (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.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.4%	[0.4%, 0.5%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Bootstrap: 760.71s -> 758.697s (-0.26%)
Artifact size: 341.13 MiB -> 340.54 MiB (-0.17%)

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

matthiaskrgr · 2024-09-15T13:21:18Z

compiler/rustc_const_eval/src/interpret/intern.rs

+                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"
+                    );
+                }


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 };

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

rustbot assigned nnethercote Aug 2, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 2, 2024

RalfJung force-pushed the const-interior-mut branch from 5371142 to d0e95a9 Compare August 2, 2024 13:47

RalfJung added the I-lang-nominated Nominated for discussion during a lang team meeting. label Aug 2, 2024

RalfJung mentioned this pull request Aug 2, 2024

regression: encountered mutable pointer in final value when "outer scope" rule applies in const/static with interior mutability #121610

Closed

This comment has been minimized.

Sign in to view

RalfJung force-pushed the const-interior-mut branch from d0e95a9 to bf4231a Compare August 2, 2024 14:13

RalfJung added T-lang Relevant to the language team, which will review and decide on the PR/issue. T-opsem Relevant to the opsem team labels Aug 2, 2024

This comment has been minimized.

Sign in to view

RalfJung force-pushed the const-interior-mut branch from bf4231a to 7b2cfd4 Compare August 2, 2024 16:29

traviscross changed the title ~~const-eval interning: accpt interior mutable pointers in final value~~ const-eval interning: accept interior mutable pointers in final value Aug 3, 2024

rustbot assigned saethlin and unassigned nnethercote Aug 5, 2024

RalfJung force-pushed the const-interior-mut branch from 7b2cfd4 to f1e47e2 Compare August 5, 2024 08:57

tarcieri mentioned this pull request Aug 16, 2024

Some simplifications of vartime division RustCrypto/crypto-bigint#646

Merged

RalfJung force-pushed the const-interior-mut branch 2 times, most recently from 9b3e7d8 to a2a238f Compare August 17, 2024 11:47

This was referenced Aug 17, 2024

Stabilize &mut (and *mut) as well as &Cell (and *const Cell) in const #129195

Merged

Tracking issue for &mut T in const contexts (const_mut_refs) #57349

Closed

fee1-dead requested changes Aug 17, 2024

View reviewed changes

compiler/rustc_middle/src/mir/interpret/pointer.rs Outdated Show resolved Hide resolved

tests/ui/consts/refs-to-cell-in-final.rs Outdated Show resolved Hide resolved

fee1-dead approved these changes Aug 17, 2024

View reviewed changes

fee1-dead self-assigned this Aug 17, 2024

rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Sep 4, 2024

traviscross removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). I-lang-nominated Nominated for discussion during a lang team meeting. labels Sep 4, 2024

traviscross added this to the 1.83.0 milestone Sep 4, 2024

fee1-dead added the S-waiting-on-fcp Status: PR is in FCP and is awaiting for FCP to complete. label Sep 5, 2024

This comment was marked as resolved.

Sign in to view

bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Sep 10, 2024

RalfJung added 2 commits September 10, 2024 10:26

const-eval interning: accpt interior mutable pointers in final value …

f76f128

…(but keep rejecting mutable references)

turn errors that should be impossible due to our static checks into ICEs

123757a

RalfJung force-pushed the const-interior-mut branch from 9317d46 to 123757a Compare September 10, 2024 08:28

rustbot mentioned this pull request Sep 14, 2024

Tracking issue for release notes of #128543: const-eval interning: accept interior mutable pointers in final value #130363

Open

3 tasks

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 14, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 14, 2024

bors merged commit 9b72238 into rust-lang:master Sep 14, 2024
7 checks passed

bors mentioned this pull request Sep 15, 2024

make unsupported_calling_conventions a hard error #129935

Open

RalfJung deleted the const-interior-mut branch September 15, 2024 07:09

matthiaskrgr mentioned this pull request Sep 15, 2024

ICE: the static const safety checks accepted mutable references they should not have accepted #130392

Closed

matthiaskrgr reviewed Sep 15, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

const-eval interning: accept interior mutable pointers in final value #128543

const-eval interning: accept interior mutable pointers in final value #128543

RalfJung commented Aug 2, 2024 •

edited

Loading

rustbot commented Aug 2, 2024

rustbot commented Aug 2, 2024

This comment has been minimized.

This comment has been minimized.

nnethercote commented Aug 4, 2024

saethlin commented Aug 5, 2024 •

edited

Loading

nnethercote commented Aug 5, 2024

bors commented Aug 5, 2024

fee1-dead left a comment •

edited

Loading

This comment was marked as resolved.

rfcbot commented Sep 14, 2024

RalfJung commented Sep 14, 2024

bors commented Sep 14, 2024

bors commented Sep 14, 2024

bors commented Sep 14, 2024

rust-timer commented Sep 15, 2024

matthiaskrgr Sep 15, 2024

RalfJung Sep 15, 2024

const-eval interning: accept interior mutable pointers in final value #128543

const-eval interning: accept interior mutable pointers in final value #128543

Conversation

RalfJung commented Aug 2, 2024 • edited Loading

rustbot commented Aug 2, 2024

rustbot commented Aug 2, 2024

This comment has been minimized.

This comment has been minimized.

nnethercote commented Aug 4, 2024

saethlin commented Aug 5, 2024 • edited Loading

nnethercote commented Aug 5, 2024

bors commented Aug 5, 2024

fee1-dead left a comment • edited Loading

Choose a reason for hiding this comment

This comment was marked as resolved.

rfcbot commented Sep 14, 2024

RalfJung commented Sep 14, 2024

bors commented Sep 14, 2024

bors commented Sep 14, 2024

bors commented Sep 14, 2024

rust-timer commented Sep 15, 2024

Overall result: ❌ regressions - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

matthiaskrgr Sep 15, 2024

Choose a reason for hiding this comment

RalfJung Sep 15, 2024

Choose a reason for hiding this comment

RalfJung commented Aug 2, 2024 •

edited

Loading

saethlin commented Aug 5, 2024 •

edited

Loading

fee1-dead left a comment •

edited

Loading