Skip to content
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.

Sign up for GitHub

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

Jump to bottom

unstably allow constants to refer to statics and read from immutable statics #119614

Merged
merged 5 commits into from
Feb 10, 2024
Merged

unstably allow constants to refer to statics and read from immutable statics #119614

merged 5 commits into from
Feb 10, 2024

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Jan 5, 2024

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 #119044; only the last commit is new.

@rustbot 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 Jan 5, 2024
@rustbot
Copy link
Collaborator

rustbot commented Jan 5, 2024

The Miri subtree was changed

cc @rust-lang/miri

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

@RalfJung RalfJung mentioned this pull request Jan 5, 2024
Open
oli-obk
oli-obk approved these changes Jan 5, 2024
View reviewed changes
Copy link
Contributor

@oli-obk oli-obk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fine by me, as long as the tracking issue mentions this new source of monomorphization time errors as an open question to put to T-lang

@RalfJung RalfJung mentioned this pull request Jan 5, 2024
Open
6 tasks
@RalfJung RalfJung force-pushed the const-refs-to-static branch 2 times, most recently from 768c020 to 7f21ace Compare January 5, 2024 14:34
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@RalfJung RalfJung force-pushed the const-refs-to-static branch 2 times, most recently from 6a0f1bc to 1ad4b85 Compare January 5, 2024 15:13
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rustbot
Copy link
Collaborator

rustbot commented Jan 6, 2024

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@RalfJung
Copy link
Member Author

RalfJung commented Jan 6, 2024

I realized I have to make validation descend into statics, or else valtree construction can encounter unvalidated data. This also makes the feature more conservative: consts that refer to mutable memory behind a shared reference are immediately rejected, not just when they are used as a pattern.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@oli-obk
Copy link
Contributor

oli-obk commented Jan 8, 2024

seems fishy to allow valtrees to read from immutable statics. That means we meaningfully lose the static's identity during rountdrip.

@RalfJung
Copy link
Member Author

RalfJung commented Jan 8, 2024

Valtrees are about the value. How would the static's identity matter?

@oli-obk
Copy link
Contributor

oli-obk commented Feb 10, 2024

@bors r- conflicts

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

@bors r=oli-obk

@bors
Copy link
Contributor

bors commented Feb 10, 2024

📌 Commit 4def373 has been approved by oli-obk

It is now in the queue for this repository.

@bors 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 Feb 10, 2024
@bors
Copy link
Contributor

bors commented Feb 10, 2024

⌛ Testing commit 4def373 with merge 6cc4843...

@bors
Copy link
Contributor

bors commented Feb 10, 2024

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 6cc4843 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 10, 2024
@bors bors merged commit 6cc4843 into rust-lang:master Feb 10, 2024
12 checks passed
@rustbot rustbot added this to the 1.78.0 milestone Feb 10, 2024
@bors bors mentioned this pull request Feb 10, 2024
Merged
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (6cc4843): 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.2% [0.2%, 0.2%] 1
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results

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)		 3.3% [2.3%, 4.4%] 2
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) - - 0

Cycles

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

Binary size

Results

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.1% [0.1%, 0.1%] 1
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) - - 0

Bootstrap: 666.587s -> 667.259s (0.10%)
Artifact size: 308.05 MiB -> 308.02 MiB (-0.01%)

@RalfJung RalfJung deleted the const-refs-to-static branch February 11, 2024 20:51
@RalfJung RalfJung mentioned this pull request Feb 11, 2024
Open
@DaniPopes DaniPopes mentioned this pull request Feb 11, 2024
Closed
@RalfJung RalfJung mentioned this pull request Feb 12, 2024
Merged
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 12, 2024
@matthiaskrgr
Rollup merge of rust-lang#120970 - RalfJung:static-promoted-test, r=o…
338a844 
…li-obk

add another test for promoteds-in-static

rust-lang#119614 led to validation of promoteds recursing into statics. These statics can point to `static mut` and interior mutable `static` and do other things we don't allow in `const`, but promoteds are validated as `const`, so we get strange errors (saying "in `const`" when there is no const) and surprising validation failures.

rust-lang#120960 fixes that; this here adds another test.

r? `@oli-obk`
Fixes rust-lang#120968
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 12, 2024
@matthiaskrgr
Rollup merge of rust-lang#120970 - RalfJung:static-promoted-test, r=o…
f759c23 
…li-obk

add another test for promoteds-in-static

rust-lang#119614 led to validation of promoteds recursing into statics. These statics can point to `static mut` and interior mutable `static` and do other things we don't allow in `const`, but promoteds are validated as `const`, so we get strange errors (saying "in `const`" when there is no const) and surprising validation failures.

rust-lang#120960 fixes that; this here adds another test.

r? ``@oli-obk``
Fixes rust-lang#120968
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Feb 13, 2024
@rust-timer
Unrolled build for rust-lang#120970
19f2847 
Rollup merge of rust-lang#120970 - RalfJung:static-promoted-test, r=oli-obk

add another test for promoteds-in-static

rust-lang#119614 led to validation of promoteds recursing into statics. These statics can point to `static mut` and interior mutable `static` and do other things we don't allow in `const`, but promoteds are validated as `const`, so we get strange errors (saying "in `const`" when there is no const) and surprising validation failures.

rust-lang#120960 fixes that; this here adds another test.

r? ``@oli-obk``
Fixes rust-lang#120968
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Feb 13, 2024
@matthiaskrgr
Rollup merge of #120970 - RalfJung:static-promoted-test, r=oli-obk
4567c19 
add another test for promoteds-in-static

rust-lang/rust#119614 led to validation of promoteds recursing into statics. These statics can point to `static mut` and interior mutable `static` and do other things we don't allow in `const`, but promoteds are validated as `const`, so we get strange errors (saying "in `const`" when there is no const) and surprising validation failures.

rust-lang/rust#120960 fixes that; this here adds another test.

r? ``@oli-obk``
Fixes rust-lang/rust#120968
@ojeda ojeda mentioned this pull request Feb 17, 2024
Open
83 tasks
@fmease fmease added the F-const_refs_to_static `#![feature(const_refs_to_static)]` label Feb 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fmease fmease fmease left review comments

@oli-obk oli-obk oli-obk approved these changes

Assignees

@oli-obk oli-obk

Labels
F-const_refs_to_static `#![feature(const_refs_to_static)]` merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.78.0
Development

Successfully merging this pull request may close these issues.
10 participants
@RalfJung @rustbot @rust-log-analyzer @oli-obk @bors @Darksonn @BoxyUwU @matthiaskrgr @rust-timer @fmease