const-eval: make misalignment a hard error by RalfJung · Pull Request #115524 ·...
source link: https://github.com/rust-lang/rust/pull/115524
Go to the source link to view the article. You can view the picture content, updated content and better typesetting reading experience. If the link is broken, please click the button below to view the snapshot at that time.
const-eval: make misalignment a hard error #115524
Merged
Conversation
Member
It's been a future-incompat error (showing up in cargo's reports) since #104616, Rust 1.68, released in March. That should be long enough.
The question for the lang team is simply -- should we move ahead with this, making const-eval alignment failures a hard error? (It turns out some of them accidentally already were hard errors since #104616. But not all so this is still a breaking change. Crater found no regression.)
Collaborator
|
r? @wesleywiser (rustbot has picked a reviewer for you, use r? to override) |
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
Collaborator
|
Some changes occurred to the CTFE / Miri engine cc @rust-lang/miri Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt The Miri subtree was changed cc @rust-lang/miri |
let ptr = mem.as_ptr().byte_add(1); |
||
let _val = *ptr; //~ERROR: evaluation of constant value failed |
||
//~^NOTE: accessing memory with alignment 1, but alignment 4 is required |
||
}; |
Member
Author
Ah, here's why:
check_ptr_access_align will always check alignment, so this entirely circumvented the future-incompat machinery.
Contributor
|
Should we crater it again just for impact analysis? |
added T-lang Relevant to the language team, which will review and decide on the PR/issue. needs-fcp This change is insta-stable, so needs a completed FCP to proceed.
and removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
labels
Member
Author
|
I don't even know of an example where this is a breaking change... So crater seems unnecessary to me.
|
added the I-lang-nominated Indicates that an issue has been nominated for discussion during a lang team meeting. label
Member
Author
|
This is a kind of funny FCP since it turns out the breaking change accidentally happened in #104616, we're now just removing some dead code.^^ |
Contributor
|
r=me on the impl. Nominating for T-lang team discussion. Don't know if you want to FCP this, as it's already in line with the const UB RFC (if your code exhibits UB at compile-time, but passes, we are allowed to break it later) |
removed the needs-fcp This change is insta-stable, so needs a completed FCP to proceed. label
Member
Author
|
I'd say let's nominate, and then T-lang can decide if they want an FCP or not. |
Member
Author
|
Ah, the lint docs actually have an example of code that did trigger the lint. So this is a breaking change after all. |
Member
Author
|
@bors try |
Contributor
|
⌛ Trying commit 70db9968a8d128fde5d2fb656c7b4a62cf3c92be with merge bbf68c7b2a40f9417a5515b471f1ff6e35782bca... |
Contributor
|
💔 Test failed - checks-actions |
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-review Status: Awaiting review from the assignee but also interested parties.
labels
Member
Author
|
All jobs are failing with |
This comment has been minimized.
r=me when FCP completes 🙂
Member
Author
|
@joshtriplett @pnkfelix @scottmcm friendly reminder that there's a checkbox waiting for you here. :) |
added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised.
and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off.
labels
|
🔔 This is now entering its final comment period, as per the review above. 🔔 |
removed the I-lang-nominated Indicates that an issue has been nominated for discussion during a lang team meeting. label
Contributor
|
@rustbot labels -S-waiting-on-team |
removed the S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). label
added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting
and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised.
labels
|
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. |
Member
Author
|
@bors r=wesleywiser |
added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label
Contributor
|
☀️ Test successful - checks-actions |
Collaborator
|
Finished benchmarking commit (75a5dd0): 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)Results CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 627.259s -> 628.136s (0.14%) |
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
None yet
Successfully merging this pull request may close these issues.
None yet
Recommend
About Joyk
Aggregate valuable and interesting links.
Joyk means Joy of geeK