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

Do not use scalar layout if there are ZSTs with alignment > 1 #103926

Closed
wants to merge 1 commit into from
Closed

Do not use scalar layout if there are ZSTs with alignment > 1 #103926

wants to merge 1 commit into from

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Nov 3, 2022

fixes #103634

r? @eddyb

@rustbot
Copy link
Collaborator

rustbot commented Nov 3, 2022

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 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 Nov 3, 2022
eddyb
eddyb suggested changes Nov 3, 2022
View reviewed changes
Copy link
Member

@eddyb eddyb left a comment

Choose a reason for hiding this comment

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

Likely wrong as-is (check the suggested testcase), ideally it should be less stateful.

compiler/rustc_ty_utils/src/layout.rs Outdated Show resolved Hide resolved
src/test/ui/layout/debug.rs Show resolved Hide resolved
@bors
Copy link
Contributor

bors commented Nov 13, 2022

☔ The latest upstream changes (presumably #104370) made this pull request unmergeable. Please resolve the merge conflicts.

eddyb
eddyb suggested changes Nov 25, 2022
View reviewed changes
Copy link
Member

@eddyb eddyb left a comment

Choose a reason for hiding this comment

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

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)

compiler/rustc_abi/src/layout.rs
@@ -901,36 +901,58 @@ pub trait LayoutCalculator {

let optimize = !repr.inhibit_union_abi_opt();
let mut size = Size::ZERO;
let mut abi = Abi::Aggregate { sized: true };
let mut abi = None;
Copy link
Member

Choose a reason for hiding this comment

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

Can you rename this to common_non_zst_abi? Feels very misleading as-is.

compiler/rustc_abi/src/layout.rs
Comment on lines +905 to +906
let mut biggest_zst_align = align;
let mut biggest_non_zst_align = align;
Copy link
Member

Choose a reason for hiding this comment

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

This can't be correct, you need the exact field alignment, not some combination.

compiler/rustc_abi/src/layout.rs
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);
Copy link
Member

Choose a reason for hiding this comment

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

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.

compiler/rustc_abi/src/layout.rs
size = cmp::max(size, field.size);
}

let abi = match abi {
None => Abi::Aggregate { sized: true },
Some(non_zst_abi) => {
Copy link
Member

Choose a reason for hiding this comment

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

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.

compiler/rustc_abi/src/layout.rs
Comment on lines +946 to +948
// 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.
Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Member

@RalfJung RalfJung Nov 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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).

Copy link
Member

Choose a reason for hiding this comment

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

If #105006 passes, the sanity check could also use such a inherent_align function, yeah.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

#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).

@eddyb
Copy link
Member

eddyb commented Nov 28, 2022 •
edited
Loading

@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:

Also, we need a test for #[repr(packed)] union Foo { zst: [u16; 0], byte: u8 }, which should, by virtue of #[repr(packed)] reducing alignment back, be able to be Abi::Scalar again!

@eddyb eddyb mentioned this pull request Nov 28, 2022
Merged
@jruderman

This comment was marked as outdated.

Sign in to view
@RalfJung RalfJung mentioned this pull request Nov 28, 2022
Merged
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Nov 28, 2022
@matthiaskrgr
Rollup merge of rust-lang#105006 - RalfJung:scalar-pair-alignment, r=…
3dfb6ca 
…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...?
RalfJung pushed a commit to RalfJung/miri that referenced this pull request Dec 3, 2022
@matthiaskrgr
Rollup merge of #105006 - RalfJung:scalar-pair-alignment, r=eddyb
5d3ede2 
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 Dylan-DPC 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 Dec 15, 2022
@oli-obk
Copy link
Contributor Author

oli-obk commented Dec 16, 2022

closing in favour of #104872

@oli-obk oli-obk closed this Dec 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RalfJung RalfJung RalfJung left review comments

@eddyb eddyb eddyb requested changes

Assignees

@eddyb eddyb

Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Layout sanity failed check: "size mismatch between ABI and layout in TyAndLayout". Zero size array in union.
7 participants
@oli-obk @rustbot @bors @eddyb @jruderman @RalfJung @Dylan-DPC