From 9ac23dde37efc573ef57efa92bdf63c2f05846a9 Mon Sep 17 00:00:00 2001 From: Adwin White Date: Wed, 18 Sep 2024 17:30:27 +0800 Subject: [PATCH 1/3] Get rid of niche selection's dependence on fields's order --- compiler/rustc_abi/src/layout.rs | 13 ++++--------- tests/ui/structs-enums/type-sizes.rs | 20 ++++++++++++++++++++ 2 files changed, 24 insertions(+), 9 deletions(-) diff --git a/compiler/rustc_abi/src/layout.rs b/compiler/rustc_abi/src/layout.rs index f4de4e06d1b07..01593d34c9788 100644 --- a/compiler/rustc_abi/src/layout.rs +++ b/compiler/rustc_abi/src/layout.rs @@ -527,15 +527,10 @@ impl LayoutCalculator { let count = (niche_variants.end().index() as u128 - niche_variants.start().index() as u128) + 1; - // Find the field with the largest niche - let (field_index, niche, (niche_start, niche_scalar)) = variants[largest_variant_index] - .iter() - .enumerate() - .filter_map(|(j, field)| Some((j, field.largest_niche?))) - .max_by_key(|(_, niche)| niche.available(dl)) - .and_then(|(j, niche)| Some((j, niche, niche.reserve(dl, count)?)))?; - let niche_offset = - niche.offset + variant_layouts[largest_variant_index].fields.offset(field_index); + // Use the largest niche in the largest variant. + let niche = variant_layouts[largest_variant_index].largest_niche?; + let (niche_start, niche_scalar) = niche.reserve(dl, count)?; + let niche_offset = niche.offset; let niche_size = niche.value.size(dl); let size = variant_layouts[largest_variant_index].size.align_to(align.abi); diff --git a/tests/ui/structs-enums/type-sizes.rs b/tests/ui/structs-enums/type-sizes.rs index 5ca9c8678b790..f49ce33841af3 100644 --- a/tests/ui/structs-enums/type-sizes.rs +++ b/tests/ui/structs-enums/type-sizes.rs @@ -209,6 +209,23 @@ struct ReorderEndNiche { b: MiddleNiche4, } +// We want that the niche selection doesn't depend on order of the fields. See issue #125630. +pub enum NicheFieldOrder1 { + A { + x: NonZero, + y: [NonZero; 2], + }, + B([u64; 2]), +} + +pub enum NicheFieldOrder2 { + A { + y: [NonZero; 2], + x: NonZero, + }, + B([u64; 2]), +} + // standins for std types which we want to be laid out in a reasonable way struct RawVecDummy { @@ -260,6 +277,9 @@ pub fn main() { size_of::>()); assert_eq!(size_of::(), size_of::<&'static ()>()); + assert_eq!(size_of::(), 24); + assert_eq!(size_of::(), 24); + assert_eq!(size_of::>>(), size_of::<(bool, &())>()); assert_eq!(size_of::>>(), size_of::<(bool, &())>()); assert_eq!(size_of::>>(), size_of::<(bool, &())>()); From c4574cba3bb0b24532f43dbacc2c738d4b20394d Mon Sep 17 00:00:00 2001 From: Adwin White Date: Thu, 19 Sep 2024 10:22:32 +0800 Subject: [PATCH 2/3] Adapt test to new layout --- src/tools/miri/tests/fail/uninit/padding-enum.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/tools/miri/tests/fail/uninit/padding-enum.rs b/src/tools/miri/tests/fail/uninit/padding-enum.rs index a9628799b7d30..4ba059caf8b18 100644 --- a/src/tools/miri/tests/fail/uninit/padding-enum.rs +++ b/src/tools/miri/tests/fail/uninit/padding-enum.rs @@ -17,8 +17,8 @@ fn main() { assert!(matches!(*p.as_ptr(), E::None)); // Turns out the discriminant is (currently) stored - // in the 2nd pointer, so the first half is padding. - let c = &p as *const _ as *const u8; + // in the 1st pointer, so the second half is padding. + let c = (&p as *const mem::MaybeUninit).byte_add(mem::size_of::<&'static ()>()) as *const u8; // Read a padding byte. let _val = *c.add(0); //~^ERROR: uninitialized From 937b09b389d17a07cd5e246696ac091aa53e35a1 Mon Sep 17 00:00:00 2001 From: Adwin White Date: Thu, 19 Sep 2024 16:49:39 +0800 Subject: [PATCH 3/3] Make the intention of the miri test more clear --- src/tools/miri/tests/fail/uninit/padding-enum.rs | 5 +++-- src/tools/miri/tests/fail/uninit/padding-enum.stderr | 4 ++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/src/tools/miri/tests/fail/uninit/padding-enum.rs b/src/tools/miri/tests/fail/uninit/padding-enum.rs index 4ba059caf8b18..e1a16bea23ccb 100644 --- a/src/tools/miri/tests/fail/uninit/padding-enum.rs +++ b/src/tools/miri/tests/fail/uninit/padding-enum.rs @@ -18,9 +18,10 @@ fn main() { // Turns out the discriminant is (currently) stored // in the 1st pointer, so the second half is padding. - let c = (&p as *const mem::MaybeUninit).byte_add(mem::size_of::<&'static ()>()) as *const u8; + let c = &p as *const _ as *const u8; + let padding_offset = mem::size_of::<&'static ()>(); // Read a padding byte. - let _val = *c.add(0); + let _val = *c.add(padding_offset); //~^ERROR: uninitialized } } diff --git a/src/tools/miri/tests/fail/uninit/padding-enum.stderr b/src/tools/miri/tests/fail/uninit/padding-enum.stderr index 66d3092c9ba89..765e7cc4e637d 100644 --- a/src/tools/miri/tests/fail/uninit/padding-enum.stderr +++ b/src/tools/miri/tests/fail/uninit/padding-enum.stderr @@ -1,8 +1,8 @@ error: Undefined Behavior: using uninitialized data, but this operation requires initialized memory --> tests/fail/uninit/padding-enum.rs:LL:CC | -LL | let _val = *c.add(0); - | ^^^^^^^^^ using uninitialized data, but this operation requires initialized memory +LL | let _val = *c.add(padding_offset); + | ^^^^^^^^^^^^^^^^^^^^^^ using uninitialized data, but this operation requires initialized memory | = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information