Compiling ndarray: alignment mismatch between ABI and layout #99836
Comments
cuviper
added
I-ICE
|
Here's a smaller reproducer: fn main() {
enum Error { A, B }
type Foo = Result<[usize; 0], Error>;
dbg!(std::mem::size_of::<Foo>());
dbg!(std::mem::align_of::<Foo>());
}Output
Note that |
|
This can make safe misaligned references: playground fn main() {
#[derive(Debug)]
#[allow(unused)]
enum Error { A, B }
type Foo = Result<[usize; 0], Error>;
dbg!(std::mem::size_of::<Foo>());
dbg!(std::mem::align_of::<Foo>());
let x: [Foo; 2] = [Ok([]), Ok([])];
let r0: &[usize] = x[0].as_ref().unwrap();
let r1: &[usize] = x[1].as_ref().unwrap();
eprintln!("{r0:p} {r1:p}");
} |
cuviper
added
the
I-unsound
rustbot
added
the
I-prioritize
|
WG-prioritization assigning priority (Zulip discussion). @rustbot label -I-prioritize +P-critical |
rustbot
added
P-critical
|
Looks like the assertions found a bug in layout computation? The assertions are right I think, and that layout seems very bogus. Layout was wrong already before my PR, we just didn't catch the problem.
|
|
Yeah, now that I've explored further, I agree the assertions caught a real layout bug. AIUI, the size should always be a multiple of the alignment, since we don't have separate notions of size and stride. For testing older fn main() {
type Foo = Result<[usize; 0], bool>;
println!(
"size:{} align:{}",
std::mem::size_of::<Foo>(),
std::mem::align_of::<Foo>()
);
}Up to Rust 1.23, size and align were 8, and then 1.24 dropped to size 1. That corresponds to the time that niche layout was implemented, #45225 (and dropped from 1.23 in #46925), although I didn't bisect that specifically. cc @eddyb |
|
Ahh, zero-sized aligned types, the bane of layout algorithms... Likely needs an audit for everywhere we special-case ZSTs and maybe just make them require align == 1 for now. In this specific case though, we could keep the niche optimization, just need to increase align of the whole type to the max Then it will look like an |
I don't understand how you could require that -- |
|
Yes, and for that reason |
|
Oh I see, the places looking for ZSTs should be looking for "1-ZST", where they want no effect on layout. |
|
Sorry for the confusion - when I wrote:
I should've been more specific instead of "them", and phrased it more like:
(or used the 1-ZST naming, but I completely forgot we had that) (click to open old notes, ended up tracking down the bug and was wrong)Those special cases, FWIW, are almost always ignoring ZSTs in a larger aggregate (which is also something I should've been clearer about). For example, the one you're hitting for
But I wanted to be sure I was accurate, so I checked:
Oh, whoops, misunderstood the bug: alignment is (This playground is what I used to inspect the layout, pretty useful - also, This is what tagged union rust/compiler/rustc_middle/src/ty/layout.rs Lines 1355 to 1356 in 5dda74a To fix the niche-filling codepath, you need to duplicate the above, just after let size = st[i].size();
However, be careful around The fact that |
|
Here's what I've got so far: diff --git a/compiler/rustc_middle/src/ty/layout.rs b/compiler/rustc_middle/src/ty/layout.rs
index dde55dd96554..833edd228050 100644
--- a/compiler/rustc_middle/src/ty/layout.rs
+++ b/compiler/rustc_middle/src/ty/layout.rs
@@ -1231,11 +1231,15 @@ fn layout_of_uncached(&self, ty: Ty<'tcx>) -> Result<Layout<'tcx>, LayoutError<'
.collect::<Result<IndexVec<VariantIdx, _>, _>>()?;
let offset = st[i].fields().offset(field_index) + niche.offset;
- let size = st[i].size();
+
+ // Align the total size to the largest alignment.
+ let size = st[i].size().align_to(align.abi);
let abi = if st.iter().all(|v| v.abi().is_uninhabited()) {
Abi::Uninhabited
- } else {
+ } else if align == st[i].align() && size == st[i].size() {
+ // When the total alignment and size match, we can use the
+ // same ABI as the scalar variant with the reserved niche.
match st[i].abi() {
Abi::Scalar(_) => Abi::Scalar(niche_scalar),
Abi::ScalarPair(first, second) => {
@@ -1249,6 +1253,8 @@ fn layout_of_uncached(&self, ty: Ty<'tcx>) -> Result<Layout<'tcx>, LayoutError<'
}
_ => Abi::Aggregate { sized: true },
}
+ } else {
+ Abi::Aggregate { sized: true }
};
let largest_niche = Niche::from_scalar(dl, offset, niche_scalar);For the example of The original |
Ah, does it hit the check between niche-filling and tag, that IIRC might prefer tag (if the sizes are equal) because it likely results in better codegen/more niches in many cases? Looks like it: rust/compiler/rustc_middle/src/ty/layout.rs Lines 1555 to 1560 in 5dda74a In that case, it should get the same layout as s/bool/u8/ (since it ends up not using the niche). We might still want a test where the tagged form doesn't happen. I was able to emulate the fix described above by making the enum FixedResult<T, E> {
Ok(T, [E; 0]),
Err(E, [T; 0]),
}Such a definition shouldn't be notably different from With that, I was able to come up with an extended example on playground: // Tagged repr is clever enough to grow tags to fill any padding, e.g.:
// 1. `T_FF` (one byte of Tag, one byte of padding, two bytes of align=2 Field)
// -> `TTFF` (Tag has expanded to two bytes, i.e. like `#[repr(u16)]`)
// 2. `TFF` (one byte of Tag, two bytes of align=1 Field)
// -> Tag has no room to expand!
// (this outcome can be forced onto 1. by wrapping Field in `Packed<...>`)
#[repr(packed)]
struct Packed<T>(T);
#[rustc_layout(debug)]
type NicheLosesToTagged = FixedResult<[u64; 0], Packed<std::num::NonZeroU16>>;
#[repr(u16)]
enum U16IsZero { _Zero = 0 }
#[rustc_layout(debug)]
type NicheWinsOverTagged = FixedResult<[u64; 0], Packed<U16IsZero>>;Relevant parts of the output (cleaned up a bit): layout_of(FixedResult<[u64; 0], Packed<std::num::NonZeroU16>>) = Layout {
// ...
variants: Multiple {
tag: Initialized {
value: Int(I8, false),
valid_range: 0..=1,
},
tag_encoding: Direct,
// ...
},
// ...
align: AbiAndPrefAlign {
abi: Align(8 bytes),
pref: Align(8 bytes),
},
size: Size(8 bytes),
}
layout_of(FixedResult<[u64; 0], Packed<U16IsZero>>) = Layout {
// ...
variants: Multiple {
tag: Initialized {
value: Int(I16, false),
valid_range: 0..=1,
},
tag_encoding: Niche {
dataful_variant: 1,
niche_variants: 0..=0,
niche_start: 1,
},
// ..
},
// ...
align: AbiAndPrefAlign {
abi: Align(8 bytes),
pref: Align(8 bytes),
},
size: Size(8 bytes),
}To be clear, you should be able to get those exact results with |
|
That's a nice test, thanks! And indeed, nightly currently makes those both niche with |
Code
ndarray master as of
ddef4d280fb5abc82325287e68ecacfc81882a1e.Meta
rustc --version --verbose:That's commit 2643b16 built with this
config.toml:Error output
Note that this assertion is under
if cfg!(debug_assertions):rust/compiler/rustc_middle/src/ty/layout.rs
Lines 240 to 244 in 2643b16
Full output
The text was updated successfully, but these errors were encountered: