From 0da9409e083a1da93c183e1167c96d582848847a Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 27 Aug 2023 18:01:05 +0200 Subject: [PATCH] rustc_abi: audit uses of is_zst; fix a case of giving an enum insufficient alignment --- compiler/rustc_abi/src/layout.rs | 25 ++++++++++++++++++------- tests/ui/layout/enum.rs | 18 ++++++++++++++++++ tests/ui/layout/enum.stderr | 14 ++++++++++++++ tests/ui/layout/struct.rs | 12 ++++++++++++ tests/ui/layout/struct.stderr | 14 ++++++++++++++ 5 files changed, 76 insertions(+), 7 deletions(-) create mode 100644 tests/ui/layout/enum.rs create mode 100644 tests/ui/layout/enum.stderr create mode 100644 tests/ui/layout/struct.rs create mode 100644 tests/ui/layout/struct.stderr diff --git a/compiler/rustc_abi/src/layout.rs b/compiler/rustc_abi/src/layout.rs index a8a1a90572d..76c5ac460a1 100644 --- a/compiler/rustc_abi/src/layout.rs +++ b/compiler/rustc_abi/src/layout.rs @@ -157,8 +157,10 @@ pub trait LayoutCalculator { // for non-ZST uninhabited data (mostly partial initialization). let absent = |fields: &IndexSlice>| { let uninhabited = fields.iter().any(|f| f.abi().is_uninhabited()); - let is_zst = fields.iter().all(|f| f.0.is_zst()); - uninhabited && is_zst + // We cannot ignore alignment; that might lead us to entirely discard a variant and + // produce an enum that is less aligned than it should be! + let is_1zst = fields.iter().all(|f| f.0.is_1zst()); + uninhabited && is_1zst }; let (present_first, present_second) = { let mut present_variants = variants @@ -358,8 +360,11 @@ pub trait LayoutCalculator { match layout.fields { FieldsShape::Arbitrary { ref mut offsets, .. } => { for (j, offset) in offsets.iter_enumerated_mut() { + // keep ZST at offset 0 to simplify Scalar/ScalarPair layout if !variants[i][j].0.is_zst() { *offset += this_offset; + } else { + debug_assert_eq!(offset.bytes(), 0); } } } @@ -504,7 +509,7 @@ pub trait LayoutCalculator { // to make room for a larger discriminant. for field_idx in st.fields.index_by_increasing_offset() { let field = &field_layouts[FieldIdx::from_usize(field_idx)]; - if !field.0.is_zst() || field.align().abi.bytes() != 1 { + if !field.0.is_1zst() { start_align = start_align.min(field.align().abi); break; } @@ -603,12 +608,15 @@ pub trait LayoutCalculator { abi = Abi::Scalar(tag); } else { // Try to use a ScalarPair for all tagged enums. + // That's possible only if we can find a common primitive type for all variants. let mut common_prim = None; let mut common_prim_initialized_in_all_variants = true; for (field_layouts, layout_variant) in iter::zip(variants, &layout_variants) { let FieldsShape::Arbitrary { ref offsets, .. } = layout_variant.fields else { panic!(); }; + // We skip *all* ZST here and later check if we are good in terms of alignment. + // This lets us handle some cases involving aligned ZST. let mut fields = iter::zip(field_layouts, offsets).filter(|p| !p.0.0.is_zst()); let (field, offset) = match (fields.next(), fields.next()) { (None, None) => { @@ -954,8 +962,10 @@ fn univariant( }; ( - // Place ZSTs first to avoid "interesting offsets", especially with only one - // or two non-ZST fields. This helps Scalar/ScalarPair layouts. + // Place ZSTs first to avoid "interesting offsets", especially with only + // one or two non-ZST fields. This helps Scalar/ScalarPair layouts. Note + // that these can ignore even some aligned ZST as long as the alignment + // is less than that of the scalar, hence we treat *all* ZST like that. !f.0.is_zst(), // Then place largest alignments first. cmp::Reverse(alignment_group_key(f)), @@ -1073,9 +1083,10 @@ fn univariant( let size = min_size.align_to(align.abi); let mut layout_of_single_non_zst_field = None; let mut abi = Abi::Aggregate { sized }; - // Unpack newtype ABIs and find scalar pairs. + // Try to make this a Scalar/ScalarPair. if sized && size.bytes() > 0 { - // All other fields must be ZSTs. + // We skip *all* ZST here and later check if we are good in terms of alignment. + // This lets us handle some cases involving aligned ZST. let mut non_zst_fields = fields.iter_enumerated().filter(|&(_, f)| !f.0.is_zst()); match (non_zst_fields.next(), non_zst_fields.next(), non_zst_fields.next()) { diff --git a/tests/ui/layout/enum.rs b/tests/ui/layout/enum.rs new file mode 100644 index 00000000000..7ac2eaa8600 --- /dev/null +++ b/tests/ui/layout/enum.rs @@ -0,0 +1,18 @@ +// normalize-stderr-test "pref: Align\([1-8] bytes\)" -> "pref: $$PREF_ALIGN" +//! Various enum layout tests. + +#![feature(rustc_attrs)] +#![feature(never_type)] +#![crate_type = "lib"] + +#[rustc_layout(align)] +enum UninhabitedVariantAlign { //~ERROR: abi: Align(2 bytes) + A([u8; 32]), + B([u16; 0], !), // make sure alignment in uninhabited fields is respected +} + +#[rustc_layout(size)] +enum UninhabitedVariantSpace { //~ERROR: size: Size(16 bytes) + A, + B([u8; 15], !), // make sure there is space being reserved for this field. +} diff --git a/tests/ui/layout/enum.stderr b/tests/ui/layout/enum.stderr new file mode 100644 index 00000000000..d6bc7780ce2 --- /dev/null +++ b/tests/ui/layout/enum.stderr @@ -0,0 +1,14 @@ +error: align: AbiAndPrefAlign { abi: Align(2 bytes), pref: $PREF_ALIGN } + --> $DIR/enum.rs:9:1 + | +LL | enum UninhabitedVariantAlign { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: size: Size(16 bytes) + --> $DIR/enum.rs:15:1 + | +LL | enum UninhabitedVariantSpace { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: aborting due to 2 previous errors + diff --git a/tests/ui/layout/struct.rs b/tests/ui/layout/struct.rs new file mode 100644 index 00000000000..e74cf5a952b --- /dev/null +++ b/tests/ui/layout/struct.rs @@ -0,0 +1,12 @@ +// normalize-stderr-test "pref: Align\([1-8] bytes\)" -> "pref: $$PREF_ALIGN" +//! Various struct layout tests. + +#![feature(rustc_attrs)] +#![feature(never_type)] +#![crate_type = "lib"] + +#[rustc_layout(abi)] +struct AlignedZstPreventsScalar(i16, [i32; 0]); //~ERROR: abi: Aggregate + +#[rustc_layout(abi)] +struct AlignedZstButStillScalar(i32, [i16; 0]); //~ERROR: abi: Scalar diff --git a/tests/ui/layout/struct.stderr b/tests/ui/layout/struct.stderr new file mode 100644 index 00000000000..b61c9a99cce --- /dev/null +++ b/tests/ui/layout/struct.stderr @@ -0,0 +1,14 @@ +error: abi: Aggregate { sized: true } + --> $DIR/struct.rs:9:1 + | +LL | struct AlignedZstPreventsScalar(i16, [i32; 0]); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: abi: Scalar(Initialized { value: Int(I32, true), valid_range: 0..=4294967295 }) + --> $DIR/struct.rs:12:1 + | +LL | struct AlignedZstButStillScalar(i32, [i16; 0]); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: aborting due to 2 previous errors +