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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 42 additions & 20 deletions compiler/rustc_abi/src/layout.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

let mut biggest_zst_align = align;
let mut biggest_non_zst_align = align;
Comment on lines +905 to +906
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.

let index = V::new(0);
for field in &variants[index] {
assert!(field.is_sized());
align = align.max(field.align);
assert!(!field.is_unsized());

// If all non-ZST fields have the same ABI, forward this ABI
if optimize && !field.is_zst() {
// Discard valid range information and allow undef
let field_abi = match field.abi {
Abi::Scalar(x) => Abi::Scalar(x.to_union()),
Abi::ScalarPair(x, y) => Abi::ScalarPair(x.to_union(), y.to_union()),
Abi::Vector { element: x, count } => {
Abi::Vector { element: x.to_union(), count }
}
Abi::Uninhabited | Abi::Aggregate { .. } => Abi::Aggregate { sized: true },
};
if optimize {
// If all non-ZST fields have the same ABI, forward this ABI
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.

// Discard valid range information and allow undef
let field_abi = match field.abi {
Abi::Scalar(x) => Abi::Scalar(x.to_union()),
Abi::ScalarPair(x, y) => Abi::ScalarPair(x.to_union(), y.to_union()),
Abi::Vector { element: x, count } => {
Abi::Vector { element: x.to_union(), count }
}
Abi::Uninhabited | Abi::Aggregate { .. } => Abi::Aggregate { sized: true },
};

if size == Size::ZERO {
// first non ZST: initialize 'abi'
abi = field_abi;
} else if abi != field_abi {
// different fields have different ABI: reset to Aggregate
abi = Abi::Aggregate { sized: true };
if let Some(abi) = &mut abi {
if *abi != field_abi {
// different fields have different ABI: reset to Aggregate
*abi = Abi::Aggregate { sized: true };
}
} else {
abi = Some(field_abi);
}
}
}

align = align.max(field.align);
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.

if biggest_zst_align.abi > biggest_non_zst_align.abi {
// 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.
Comment on lines +946 to +948
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).

Abi::Aggregate { sized: true }
} else {
non_zst_abi
}
}
};

if let Some(pack) = repr.pack {
align = align.min(AbiAndPrefAlign::new(pack));
}
Expand Down
21 changes: 21 additions & 0 deletions src/test/ui/layout/debug.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,27 @@ type Test = Result<i32, i32>; //~ ERROR: layout_of
#[rustc_layout(debug)]
type T = impl std::fmt::Debug; //~ ERROR: layout_of

#[rustc_layout(debug)]
pub union V { //~ ERROR: layout_of
a: [u16; 0],
b: u8,
}
oli-obk marked this conversation as resolved.
Show resolved Hide resolved

#[rustc_layout(debug)]
pub union W { //~ ERROR: layout_of
b: u8,
a: [u16; 0],
}

#[rustc_layout(debug)]
pub union Y { //~ ERROR: layout_of
b: [u8; 0],
a: [u16; 0],
}

#[rustc_layout(debug)]
type X = std::mem::MaybeUninit<u8>; //~ ERROR: layout_of

fn f() -> T {
0i32
}
95 changes: 94 additions & 1 deletion src/test/ui/layout/debug.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -307,5 +307,98 @@ error: layout_of(i32) = Layout {
LL | type T = impl std::fmt::Debug;
| ^^^^^^

error: aborting due to 5 previous errors
error: layout_of(V) = Layout {
size: Size(2 bytes),
align: AbiAndPrefAlign {
abi: Align(2 bytes),
pref: $PREF_ALIGN,
},
abi: Aggregate {
sized: true,
},
fields: Union(
2,
),
largest_niche: None,
variants: Single {
index: 0,
},
}
--> $DIR/debug.rs:21:1
|
LL | pub union V {
| ^^^^^^^^^^^

error: layout_of(W) = Layout {
size: Size(2 bytes),
align: AbiAndPrefAlign {
abi: Align(2 bytes),
pref: $PREF_ALIGN,
},
abi: Aggregate {
sized: true,
},
fields: Union(
2,
),
largest_niche: None,
variants: Single {
index: 0,
},
}
--> $DIR/debug.rs:27:1
|
LL | pub union W {
| ^^^^^^^^^^^

error: layout_of(Y) = Layout {
size: Size(0 bytes),
align: AbiAndPrefAlign {
abi: Align(2 bytes),
pref: $PREF_ALIGN,
},
abi: Aggregate {
sized: true,
},
fields: Union(
2,
),
largest_niche: None,
variants: Single {
index: 0,
},
}
--> $DIR/debug.rs:33:1
|
LL | pub union Y {
| ^^^^^^^^^^^

error: layout_of(std::mem::MaybeUninit<u8>) = Layout {
size: Size(1 bytes),
align: AbiAndPrefAlign {
abi: Align(1 bytes),
pref: $PREF_ALIGN,
},
abi: Scalar(
Union {
value: Int(
I8,
false,
),
},
),
fields: Union(
2,
),
largest_niche: None,
variants: Single {
index: 0,
},
}
--> $DIR/debug.rs:39:1
|
LL | type X = std::mem::MaybeUninit<u8>;
| ^^^^^^

error: aborting due to 9 previous errors