Do not use scalar layout if there are ZSTs with alignment > 1#103926
Do not use scalar layout if there are ZSTs with alignment > 1#103926oli-obk wants to merge 1 commit intorust-lang:masterfrom
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @eddyb (or someone else) soon. Please see the contribution instructions for more information. |
rustbot
added
S-waiting-on-review
eddyb
left a comment
eddyb
left a comment
There was a problem hiding this comment.
Likely wrong as-is (check the suggested testcase), ideally it should be less stateful.
|
☔ The latest upstream changes (presumably #104370) made this pull request unmergeable. Please resolve the merge conflicts. |
eddyb
left a comment
eddyb
left a comment
There was a problem hiding this comment.
Sorry, still not too happy with the exact approach, I think we should do the Abi::inherent_align (or fixed_align or forced_align or any other name you like) for Abi::{Scalar,ScalarPair,Vector} (which last I checked all mandated exact alignment)
| let optimize = !repr.inhibit_union_abi_opt(); | ||
| let mut size = Size::ZERO; | ||
| let mut abi = Abi::Aggregate { sized: true }; | ||
| let mut abi = None; |
There was a problem hiding this comment.
Can you rename this to common_non_zst_abi? Feels very misleading as-is.
| let mut biggest_zst_align = align; | ||
| let mut biggest_non_zst_align = align; |
There was a problem hiding this comment.
This can't be correct, you need the exact field alignment, not some combination.
| if field.is_zst() { | ||
| biggest_zst_align = biggest_zst_align.max(field.align); | ||
| } else { | ||
| biggest_non_zst_align = biggest_non_zst_align.max(field.align); |
There was a problem hiding this comment.
This doesn't seem right. Thing is, you cannot have the same Abi::{Scalar,ScalarPair,Vector} and different alignment.
What you want is common_non_zst_abi to actually be common_non_zst_abi_and_align, I think?
And then if the Abi matches between fields, and it's not Aggregate (which is being abused as an Err(Mismatch) state!), you can assert that the tracked alignment is the same.
|
|
||
| let abi = match abi { | ||
| None => Abi::Aggregate { sized: true }, | ||
| Some(non_zst_abi) => { |
There was a problem hiding this comment.
What if you move the optimize check here? That should make it clear that no decisions are made above, just data collection (speaking of, I had to leave more comments, because the data collection doesn't seem right.
| // If a zst has a bigger alignment than the non-zst fields, | ||
| // we cannot use scalar layout, because scalar(pair)s can't be | ||
| // more aligned than their primitive. |
There was a problem hiding this comment.
This comment is misleading. Abi::{Scalar,ScalarPair,Vector} imply fixed alignment based on the their contents (and the TargetDataLayout). There's zero wiggle room (unless things changed again ofc).
(Also, you don't need biggest_zst_align at all either way - you can replace it with align, if biggest_zst_align is smaller than align would naturally be equal to biggest_non_zst_align)
Honestly there should be an e.g. fn inherent_align(&self, cx: &C) -> Option<AbiAndPrefAlign> method on Abi to query this properly for those 3 variants, it's getting a bit silly at this point...
(I believe @RalfJung's layout sanity checking code could also use that? I forget if it ended up duplicating some logic or not)
There was a problem hiding this comment.
For ScalarPair, my sanity check currently enforces layout.layout.align().abi >= cmp::max(align1, align2), i.e. it leaves some wiggle room between the alignment of the 2 fields and the alignment of the type -- the type may have higher alignment than what the fields say. I am pretty sure I tried == and that did not pass the ui test suite. Would that be a bug?
There was a problem hiding this comment.
Maybe! I'm pretty sure we assume the exact alignment at least... somewhere?
Also, it's always an optimization, unlike the other two Abis, so us using it less is not going to break anything correctness-wise (yes I have some PRs from this summer I need to get back to.... life caught up with me sigh).
There was a problem hiding this comment.
If #105006 passes, the sanity check could also use such a inherent_align function, yeah.
There was a problem hiding this comment.
Oh wait actually, we currently also allow strengthening the alignment in vectors (i.e., the layout can have stricter alignment than the scalar type would require). And again probably I had a reason to allow that.
There was a problem hiding this comment.
#105006 landed, so looks like you are right -- size and alignment of Scalar/ScalarPair/Vector types are entirely determined by their Abi (and target information).
|
@oli-obk It would be nice if this PR could be repurposes to never mention ZSTs explicitly, do the "inherent alignment check" and also fix #104802. EDIT: also see #104872 (comment), particularly:
|
This comment was marked as outdated.
This comment was marked as outdated.
…eddyb stricter alignment enforcement for ScalarPair `@eddyb` [indicated](rust-lang#103926 (comment)) that alignment violating this check might be a bug. So let's see what the test suite says. (Only the 2nd commit actually changes behavior... but I couldn't not do that other cleanup.^^) Does the PR CI runner even enable debug assertions though...?
stricter alignment enforcement for ScalarPair `@eddyb` [indicated](rust-lang/rust#103926 (comment)) that alignment violating this check might be a bug. So let's see what the test suite says. (Only the 2nd commit actually changes behavior... but I couldn't not do that other cleanup.^^) Does the PR CI runner even enable debug assertions though...?
Dylan-DPC
added
S-waiting-on-author
|
closing in favour of #104872 |
7 participants
fixes #103634
r? @eddyb