-
Notifications
You must be signed in to change notification settings - Fork 14.1k
Fix vec iter zst alignment #149272
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
base: main
Are you sure you want to change the base?
Fix vec iter zst alignment #149272
Conversation
|
This PR modifies |
|
r? @fee1-dead rustbot has assigned @fee1-dead. Use |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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.
It looks like the file name needs to be changed.
https://rustc-dev-guide.rust-lang.org/tests/best-practices.html#test-naming
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.
Instead of using the issues directory, please refer to this and choose a more appropriate location.
tests/ui/issues/issue-148682.rs
Outdated
| @@ -0,0 +1,9 @@ | |||
| // check-pass | |||
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.
| // check-pass | |
| //@ check-pass |
This comment has been minimized.
This comment has been minimized.
|
r? libs |
|
@reddevilmidzy Thanks! Updated as requested. |
This comment has been minimized.
This comment has been minimized.
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.
and please squash
| // @ check-pass | ||
| //test Intolter::nth_back does not cause UB for ZSTs with high alignment |
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.
Please remove leading spaces from commands, and add a space before comments.
| // @ check-pass | |
| //test Intolter::nth_back does not cause UB for ZSTs with high alignment | |
| //@ check-pass | |
| // test Intolter::nth_back does not cause UB for ZSTs with high alignment |
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.
Thanks!It seems that all the CI tests are passed
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.
Instead of using the issues directory, please refer to this and choose a more appropriate location.
|
Could you please add |
ceb3c6f to
069f53f
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
This comment has been minimized.
This comment has been minimized.
069f53f to
d94bb76
Compare
|
@rustbot ready |
tests/ui/iterators/ZST-nthback.rs
Outdated
| //@ check-pass | ||
| // test Intolter::nth_back does not cause UB for ZSTs with high alignment | ||
|
|
||
| #[repr(align(8))] | ||
| struct Thing; | ||
|
|
||
| fn main() { | ||
| let v = vec![Thing, Thing]; | ||
| let _ = v.into_iter().nth_back(1); | ||
| } |
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.
The issue shows a reproduction using Miri, so I'm wondering how this test is supposed to reproduce that. IIUC this does the equivalent of "cargo check", no, while what is needed is a miri check.
580eef1 to
1a055d7
Compare
tests/ui/iterators/ZST-nthback.rs
Outdated
| @@ -0,0 +1,10 @@ | |||
| //@ run-pass | |||
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 still will not be a test we run under miri, I believe.
We do run the standard library's tests under miri, however.
This comment has been minimized.
This comment has been minimized.
fd0b327 to
8ef3419
Compare
|
The Miri subtree was changed cc @rust-lang/miri |
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.
Some comments for the Miri test. :)
I did not look at the libs part.
FWIW, we do run the standard library test suite in Miri. So maybe it would make more sense to add this there? We already have some tests in the standard library test suite that are primarily meant for Miri.
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.
Please use lower-case filenames.
| //@ compile-flags: -Zmiri-symbolic-alignment-check | ||
| //@ run-pass |
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.
| //@ compile-flags: -Zmiri-symbolic-alignment-check | |
| //@ run-pass | |
| //@compile-flags: -Zmiri-symbolic-alignment-check |
Please follow the conventions of surrounding files.
|
|
||
| // The main assertion is that the program runs to completion without Miri reporting UB. |
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.
| // The main assertion is that the program runs to completion without Miri reporting UB. |
This doesn't need to be explicitly stated.
| //@ compile-flags: -Zmiri-symbolic-alignment-check | ||
| //@ run-pass | ||
|
|
||
| use std::ptr; |
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.
What are you importing std::ptr for?
d7b95bc to
2096afe
Compare
|
Hello @RalfJung .Thanks for the guidance!But I'm still wondering about where I should place test file. Could you please confirm the exact directory path you'd like me to place the new test file ( |
|
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
library/alloctests/tests/vec.rs
Outdated
| // Regression test for Undefined Behavior (UB) caused by IntoIter::nth_back (#148682) | ||
| // when dealing with high-aligned Zero-Sized Types (ZSTs). | ||
| #[test] | ||
| #[cfg(miri)] |
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.
Why do you only enable this test in Miri?
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.
Sorry, I thought that the UB could only be detected by miri. I have modified that.
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, only Miri will detect UB. But the test should still pass without Miri, and in particular this enures it gets type-checked.
This comment has been minimized.
This comment has been minimized.
|
Also, please run |
e608b3a to
ef2ceec
Compare
This comment has been minimized.
This comment has been minimized.
This commit consolidates all changes, including the core logic fix for IntoIter::nth_back and the addition of the Miri regression test in `library/alloctests/tests/vec.rs`, to prevent Undefined Behavior (UB) when dealing with highly-aligned Zero-Sized Types.
ef2ceec to
89b4b30
Compare
|
Builds are green and I've addressed all feedback (moved test to library/alloc/tests/vec.rs). @rustbot ready |
Closes #148682