cuviper
commented
2 years ago cc @RalfJung since you re-enabled these assertions in #98968.
Closed cuviper closed 2 years ago
cuviper
commented
2 years ago cc @RalfJung since you re-enabled these assertions in #98968.
cuviper
commented
2 years ago 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>());
}
```
warning: variants `A` and `B` are never constructed
--> align.rs:2:18
|
2 | enum Error { A, B }
| ----- ^ ^
| |
| variants in this enum
|
= note: `#[warn(dead_code)]` on by default
thread 'rustc' panicked at 'assertion failed: `(left == right)`
left: `Align(8 bytes)`,
right: `Align(1 bytes)`: alignment mismatch between ABI and layout in Layout {
fields: Arbitrary {
offsets: [
Size(0 bytes),
],
memory_index: [
0,
],
},
variants: Multiple {
tag: Initialized {
value: Int(
I8,
false,
),
valid_range: 0..=2,
},
tag_encoding: Niche {
dataful_variant: 1,
niche_variants: 0..=0,
niche_start: 2,
},
tag_field: 0,
variants: [
Layout {
fields: Arbitrary {
offsets: [
Size(0 bytes),
],
memory_index: [
0,
],
},
variants: Single {
index: 0,
},
abi: Aggregate {
sized: true,
},
largest_niche: None,
align: AbiAndPrefAlign {
abi: Align(8 bytes),
pref: Align(8 bytes),
},
size: Size(0 bytes),
},
Layout {
fields: Arbitrary {
offsets: [
Size(0 bytes),
],
memory_index: [
0,
],
},
variants: Single {
index: 1,
},
abi: Scalar(
Initialized {
value: Int(
I8,
false,
),
valid_range: 0..=1,
},
),
largest_niche: Some(
Niche {
offset: Size(0 bytes),
value: Int(
I8,
false,
),
valid_range: 0..=1,
},
),
align: AbiAndPrefAlign {
abi: Align(1 bytes),
pref: Align(8 bytes),
},
size: Size(1 bytes),
},
],
},
abi: Scalar(
Initialized {
value: Int(
I8,
false,
),
valid_range: 0..=2,
},
),
largest_niche: Some(
Niche {
offset: Size(0 bytes),
value: Int(
I8,
false,
),
valid_range: 0..=2,
},
),
align: AbiAndPrefAlign {
abi: Align(8 bytes),
pref: Align(8 bytes),
},
size: Size(1 bytes),
}', compiler/rustc_middle/src/ty/layout.rs:240:21
stack backtrace:
0: 0x7ff8704d57b4 -
Note that Foo has size 1 and align 8, which seems really bad...
cuviper
commented
2 years ago 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}");
}[src/main.rs:8] std::mem::size_of::<Foo>() = 1
[src/main.rs:9] std::mem::align_of::<Foo>() = 8
0x7ffe2c4ba120 0x7ffe2c4ba121
apiraino
commented
2 years ago WG-prioritization assigning priority (Zulip discussion).
@rustbot label -I-prioritize +P-critical
RalfJung
commented
2 years ago 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.
cuviper
commented
2 years ago 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 rustc, I simplified it to this:
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
eddyb
commented
2 years ago 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 variants[v].fields[f].align (across all variants v and their fields f).
Then it will look like an Option<([usize; 0], bool)>, i.e. one byte for the either-bool-or-2u8 plus 7 padding bytes.
cuviper
commented
2 years ago Likely needs an audit for everywhere we special-case ZSTs and maybe just make them require align == 1 for now.
I don't understand how you could require that -- [T; 0] must still be aligned like T, right?.
RalfJung
commented
2 years ago Yes, and for that reason [T; 0] is not a ZST for many T. In the WG-UCG we started using the term "1-ZST" to refer to types with size 0 and alignment 1; that's really what we usually mean when we say "ZST".
cuviper
commented
2 years ago Oh I see, the places looking for ZSTs should be looking for "1-ZST", where they want no effect on layout.
eddyb
commented
2 years ago Sorry for the confusion - when I wrote:
Likely needs an audit for everywhere we special-case ZSTs and maybe just make them require align == 1 for now.
I should've been more specific instead of "them", and phrased it more like:
Likely needs an audit for everywhere we special-case ZSTs and change those special cases to also check align == 1 for now.
(or used the 1-ZST naming, but I completely forgot we had that)
Oh, whoops, misunderstood the bug: alignment is 8 so we do in fact consider [usize; 0] for alignment, but we never adjust the size to be a multiple of alignment.
(This playground is what I used to inspect the layout, pretty useful - also, Error could be replaced by bool in the minimal repro, just noticed)
This is what tagged union enum layout does, after it merges variants' align/size: https://github.com/rust-lang/rust/blob/5dda74a48cd50de10539478c1e0b6699bfdab665/compiler/rustc_middle/src/ty/layout.rs#L1355-L1356
To fix the niche-filling codepath, you need to duplicate the above, just after let size = st[i].size();
However, be careful around abi: the match st[i].abi() { cases for Abi::{Scalar,ScalarPair} will need to also have an if align == st[i].align() && size == st[i].size() guard (as Abi::{Scalar,ScalarPair} have expected align/sizes determined entirely by the Abi value and the target spec).
The fact that align == st[i].align() isn't checked today for Abi::{Scalar,ScalarPair} is technically wrong AFAICT, but I can't think of a way to actually exploit layout.align being larger than what is required for a specific layout.abi.
(AFAICT the LLVM backend always makes alignments explicit where possible, and always inserts explicit padding where there are any gaps between fields, the only way any of that could go wrong is if Rust alignment was less than the LLVM alignment for the same type, but only the opposite can be true as LLVM has less information and we also force packed LLVM types at the slightest hint of underaligned fields)
cuviper
commented
2 years ago 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 Result<[usize; 0], bool>, it ends up using a direct tag instead, still size == align == 8.
The original ndarray build also passes all assertions with this change.
eddyb
commented
2 years ago For the example of
Result<[usize; 0], bool>, it ends up using a direct tag instead, stillsize == align == 8.
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: https://github.com/rust-lang/rust/blob/5dda74a48cd50de10539478c1e0b6699bfdab665/compiler/rustc_middle/src/ty/layout.rs#L1555-L1560
In that case, it should get the same layout as s/bool/u8/ (since it ends up not using the niche).
With some quick testing, I was able to confirm that [usize; 0] in such a tagged union enum gets placed at the very end (i.e. offset 8 in a size 8 type), which matches the respective handwritten #[repr(C)] struct, so I'm happy to say that all looks good.
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 "cross-pollinate" alignments:
enum FixedResult<T, E> {
Ok(T, [E; 0]),
Err(E, [T; 0]),
}Such a definition shouldn't be notably different from Result<T, E> (modulo bugs like this very one).
(note that you can't just use Result<(T, [E; 0]), (E, [T; 0])> to emulate the fix, as that will put padding inside variant fields, instead of in between them, which can pessimize tagged layouts)
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 Result instead of FixedResult, on your branch.
cuviper
commented
2 years ago That's a nice test, thanks! And indeed, nightly currently makes those both niche with align==8 && size==2, while my branch makes them direct and niche respectively with align == size == 8.
Code
ndarray master as of
ddef4d280fb5abc82325287e68ecacfc81882a1e.Meta
rustc --version --verbose:That's commit 2643b16468fda787470340890212591d8bc832b7 built with this
config.toml:Error output
Note that this assertion is under
if cfg!(debug_assertions): https://github.com/rust-lang/rust/blob/2643b16468fda787470340890212591d8bc832b7/compiler/rustc_middle/src/ty/layout.rs#L240-L244Full output
``` thread 'rustc' panicked at 'assertion failed: `(left == right)` left: `Align(8 bytes)`, right: `Align(1 bytes)`: alignment mismatch between ABI and layout in Layout { fields: Arbitrary { offsets: [ Size(0 bytes), ], memory_index: [ 0, ], }, variants: Multiple { tag: Initialized { value: Int( I8, false, ), valid_range: 0..=6, }, tag_encoding: Niche { dataful_variant: 1, niche_variants: 0..=0, niche_start: 0, }, tag_field: 0, variants: [ Layout { fields: Arbitrary { offsets: [ Size(0 bytes), ], memory_index: [ 0, ], }, variants: Single { index: 0, }, abi: Aggregate { sized: true, }, largest_niche: None, align: AbiAndPrefAlign { abi: Align(8 bytes), pref: Align(8 bytes), }, size: Size(0 bytes), }, Layout { fields: Arbitrary { offsets: [ Size(0 bytes), ], memory_index: [ 0, ], }, variants: Single { index: 1, }, abi: Scalar( Initialized { value: Int( I8, false, ), valid_range: 1..=6, }, ), largest_niche: Some( Niche { offset: Size(0 bytes), value: Int( I8, false, ), valid_range: 1..=6, }, ), align: AbiAndPrefAlign { abi: Align(1 bytes), pref: Align(8 bytes), }, size: Size(1 bytes), }, ], }, abi: Scalar( Initialized { value: Int( I8, false, ), valid_range: 0..=6, }, ), largest_niche: Some( Niche { offset: Size(0 bytes), value: Int( I8, false, ), valid_range: 0..=6, }, ), align: AbiAndPrefAlign { abi: Align(8 bytes), pref: Align(8 bytes), }, size: Size(1 bytes), }', compiler/rustc_middle/src/ty/layout.rs:240:21 stack backtrace: 0: rust_begin_unwind 1: core::panicking::panic_fmt 2: core::panicking::assert_failed_inner 3: core::panicking::assert_failed::
4: rustc_middle::ty::layout::sanity_check_layout::check_layout_abi
5: rustc_middle::ty::context::tls::with_context::, rustc_middle::ty::layout::LayoutError>>::{closure#0}, core::result::Result, rustc_middle::ty::layout::LayoutError>>::{closure#0}
6: rustc_middle::ty::layout::layout_of
7: ::with_deps::<>::with_task_impl, core::result::Result, rustc_middle::ty::layout::LayoutError>>::{closure#0}, core::result::Result, rustc_middle::ty::layout::LayoutError>>
8: >::with_task::, core::result::Result, rustc_middle::ty::layout::LayoutError>>
9: rustc_query_system::query::plumbing::get_query::
10: ::layout_of
11: ::check
12: ::run_lint
13: rustc_mir_transform::pass_manager::run_passes
14: rustc_mir_transform::run_post_borrowck_cleanup_passes
15: rustc_mir_transform::mir_drops_elaborated_and_const_checked
16: ::with_deps::<>::with_task_impl, &rustc_data_structures::steal::Steal>::{closure#0}, &rustc_data_structures::steal::Steal>
17: >::with_task::, &rustc_data_structures::steal::Steal>
18: rustc_query_system::query::plumbing::try_execute_query::, &rustc_data_structures::steal::Steal>>
19: rustc_query_system::query::plumbing::get_query::
20: ::mir_drops_elaborated_and_const_checked
21: rustc_mir_transform::optimized_mir
22: ::with_deps::<>::with_task_impl::{closure#0}, &rustc_middle::mir::Body>
23: >::with_task::
24: rustc_query_system::query::plumbing::try_execute_query::>
25: rustc_query_system::query::plumbing::get_query::
26: ::optimized_mir
27: ::encode_crate_root
28: rustc_metadata::rmeta::encoder::encode_metadata_impl
29: rustc_data_structures::sync::join::
30: rustc_metadata::rmeta::encoder::encode_metadata
31: rustc_metadata::fs::encode_and_write_metadata
32: rustc_interface::passes::start_codegen
33: ::enter::<::ongoing_codegen::{closure#0}::{closure#0}, core::result::Result, rustc_errors::ErrorGuaranteed>>
34: >>::compute::<::ongoing_codegen::{closure#0}>
35: ::enter::, rustc_errors::ErrorGuaranteed>>
36: rustc_span::with_source_map::, rustc_interface::interface::create_compiler_and_run, rustc_driver::run_compiler::{closure#1}>::{closure#1}>
37: rustc_interface::interface::create_compiler_and_run::, rustc_driver::run_compiler::{closure#1}>
38: >::set::, rustc_driver::run_compiler::{closure#1}>::{closure#0}, core::result::Result<(), rustc_errors::ErrorGuaranteed>>
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
error: internal compiler error: unexpected panic
note: the compiler unexpectedly panicked. this is a bug.
note: we would appreciate a bug report: https://github.com/rust-lang/rust/issues/new?labels=C-bug%2C+I-ICE%2C+T-compiler&template=ice.md
note: rustc 1.64.0-dev running on x86_64-unknown-linux-gnu
note: compiler flags: --crate-type lib -C embed-bitcode=no -C debuginfo=2 -C incremental
note: some of the compiler flags provided by cargo are hidden
query stack during panic:
#0 [layout_of] computing layout of `core::result::Result, error::ShapeError>`
#1 [mir_drops_elaborated_and_const_checked] elaborating drops for `slice::::try_from`
#2 [optimized_mir] optimizing MIR for `slice::::try_from`
end of query stack
```