Skip to content

stabilize const_swap #134757

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

Merged
merged 5 commits into from
Dec 31, 2024
Merged

stabilize const_swap #134757

merged 5 commits into from
Dec 31, 2024

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Dec 25, 2024

libs-api FCP passed in #83163.

However, I only just realized that this actually involves an intrinsic. The intrinsic could be implemented entirely with existing stable const functionality, but we choose to make it a primitive to be able to detect more UB. So nominating for @rust-lang/lang to make sure they are aware; I leave it up to them whether they want to FCP this.

While at it I also renamed the intrinsic to make the "nonoverlapping" constraint more clear.

Fixes #83163

@RalfJung RalfJung added I-lang-nominated Nominated for discussion during a lang team meeting. I-lang-easy-decision Issue: The decision needed by the team is conjectured to be easy; this does not imply nomination labels Dec 25, 2024
@rustbot
Copy link
Collaborator

rustbot commented Dec 25, 2024

r? @scottmcm

rustbot has assigned @scottmcm.
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 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. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Dec 25, 2024
@rustbot
Copy link
Collaborator

rustbot commented Dec 25, 2024

Some changes occurred to the intrinsics. Make sure the CTFE / Miri interpreter
gets adapted for the changes, if necessary.

cc @rust-lang/miri, @rust-lang/wg-const-eval

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri

The Miri subtree was changed

cc @rust-lang/miri

Some changes occurred to the CTFE machinery

cc @rust-lang/wg-const-eval

@traviscross traviscross added T-lang Relevant to the language team, which will review and decide on the PR/issue. and removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Dec 25, 2024
Co-authored-by: scottmcm <[email protected]>
Comment on lines -887 to -894
// The initializer of constants and statics will get validated separately
// after the constant has been fully evaluated. While we could fall back to the default
// code path, that will cause -Zenforce-validity to cycle on static initializers.
// Reading from a static's memory is not allowed during its evaluation, and will always
// trigger a cycle error. Validation must read from the memory of the current item.
// For Miri this means we do not validate the root frame return value,
// but Miri anyway calls `read_target_isize` on that so separate validation
// is not needed.
Copy link
Member Author

@RalfJung RalfJung Dec 30, 2024

Choose a reason for hiding this comment

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

I think this comment is outdated ever since we changed how statics get interned and when exactly validation runs... but if there's still a risk of an ICE here I am sure matthiaskrgr will find it in no time and then we'll have a testcase. ;)

@scottmcm
Copy link
Member

I have zero concerns with stabilizing this intrinsic. I added it in #122582 with no specific intention to check things better in const, just a want to be able to detect layouts in a way that doesn't work in libcore source code. If I were remaking that PR today it might just have been #[miri::intrinsic_fallback_is_spec], but AFAICT that didn't exist at the time.

So r=me once CI finishes, unless you really want broader lang approval on it.

@RalfJung
Copy link
Member Author

If I were remaking that PR today it might just have been #[miri::intrinsic_fallback_is_spec],

I would object to that, given that the intrinsic docs clearly say that it is a typed swap, and the fallback implementation uses an untyped swap.

@RalfJung
Copy link
Member Author

@bors r=scottmcm

@bors
Copy link
Collaborator

bors commented Dec 30, 2024

📌 Commit 3c0c138 has been approved by scottmcm

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-review Status: Awaiting review from the assignee but also interested parties. labels Dec 30, 2024
@bors
Copy link
Collaborator

bors commented Dec 30, 2024

⌛ Testing commit 3c0c138 with merge 4e5fec2...

@bors
Copy link
Collaborator

bors commented Dec 31, 2024

☀️ Test successful - checks-actions
Approved by: scottmcm
Pushing 4e5fec2 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 31, 2024
@bors bors merged commit 4e5fec2 into rust-lang:master Dec 31, 2024
7 checks passed
@rustbot rustbot added this to the 1.85.0 milestone Dec 31, 2024
@scottmcm
Copy link
Member

I would object to that, given that the intrinsic docs clearly say that it is a typed swap

I agree -- would have needed to be a different phrasing to make that reasonable, unsure exactly how.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (4e5fec2): comparison URL.

Overall result: ❌✅ regressions and improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

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

Max RSS (memory usage)

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

Cycles

Results (secondary 3.2%)

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

Binary size

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

Bootstrap: 758.809s -> 761.022s (0.29%)
Artifact size: 325.55 MiB -> 325.49 MiB (-0.02%)

@RalfJung RalfJung deleted the const_swap branch December 31, 2024 11:07
@apiraino apiraino removed the I-lang-nominated Nominated for discussion during a lang team meeting. label Apr 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I-lang-easy-decision Issue: The decision needed by the team is conjectured to be easy; this does not imply nomination 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-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tracking Issue for const_swap
7 participants