From 23d09aebc8c6b89ba86bce2c38a0fc31f227d722 Mon Sep 17 00:00:00 2001 From: Oli Scherer Date: Thu, 3 Nov 2022 14:45:42 +0000 Subject: [PATCH 1/9] Do not use scalar layout if there are ZSTs with alignment > 1 --- compiler/rustc_abi/src/layout.rs | 62 ++++++++++++++------- tests/ui/layout/debug.rs | 21 +++++++ tests/ui/layout/debug.stderr | 95 +++++++++++++++++++++++++++++++- 3 files changed, 157 insertions(+), 21 deletions(-) diff --git a/compiler/rustc_abi/src/layout.rs b/compiler/rustc_abi/src/layout.rs index b4597d5bc78..1bcc44237a3 100644 --- a/compiler/rustc_abi/src/layout.rs +++ b/compiler/rustc_abi/src/layout.rs @@ -731,36 +731,58 @@ fn layout_of_union( let optimize = !repr.inhibit_union_abi_opt(); let mut size = Size::ZERO; - let mut abi = Abi::Aggregate { sized: true }; + let mut abi = None; + let mut biggest_zst_align = align; + let mut biggest_non_zst_align = align; let only_variant = &variants[FIRST_VARIANT]; for field in only_variant { - assert!(field.0.is_sized()); - align = align.max(field.align()); + assert!(!field.0.is_unsized()); - // If all non-ZST fields have the same ABI, forward this ABI - if optimize && !field.0.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 } + if optimize { + // If all non-ZST fields have the same ABI, forward this ABI + if field.0.is_zst() { + biggest_zst_align = biggest_zst_align.max(field.align()); + } else { + biggest_non_zst_align = biggest_non_zst_align.max(field.align()); + // 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 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); } - 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 }; } } + align = align.max(field.align()); size = cmp::max(size, field.size()); } + let abi = match abi { + None => Abi::Aggregate { sized: true }, + Some(non_zst_abi) => { + 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. + Abi::Aggregate { sized: true } + } else { + non_zst_abi + } + } + }; + if let Some(pack) = repr.pack { align = align.min(AbiAndPrefAlign::new(pack)); } diff --git a/tests/ui/layout/debug.rs b/tests/ui/layout/debug.rs index a282e71235c..c506b98f16e 100644 --- a/tests/ui/layout/debug.rs +++ b/tests/ui/layout/debug.rs @@ -17,6 +17,27 @@ union U { f1: (i32, i32), f3: 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, +} + +#[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; //~ ERROR: layout_of + fn f() -> T { 0i32 } diff --git a/tests/ui/layout/debug.stderr b/tests/ui/layout/debug.stderr index c5e1c41d130..6f6ab13eac5 100644 --- a/tests/ui/layout/debug.stderr +++ b/tests/ui/layout/debug.stderr @@ -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) = 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; + | ^^^^^^ + +error: aborting due to 9 previous errors From a3800535b1c9213fa99d897d317bfcf0ba7bf426 Mon Sep 17 00:00:00 2001 From: Luqman Aden Date: Wed, 30 Nov 2022 23:09:51 -0800 Subject: [PATCH 2/9] Add helper methods inherent_align and to_union on Abi. --- compiler/rustc_abi/src/lib.rs | 27 ++++++++++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-) diff --git a/compiler/rustc_abi/src/lib.rs b/compiler/rustc_abi/src/lib.rs index d01a9b00304..bbbc417e892 100644 --- a/compiler/rustc_abi/src/lib.rs +++ b/compiler/rustc_abi/src/lib.rs @@ -1,11 +1,11 @@ #![cfg_attr(feature = "nightly", feature(step_trait, rustc_attrs, min_specialization))] -use std::fmt; #[cfg(feature = "nightly")] use std::iter::Step; use std::num::{NonZeroUsize, ParseIntError}; use std::ops::{Add, AddAssign, Mul, RangeInclusive, Sub}; use std::str::FromStr; +use std::{cmp, fmt}; use bitflags::bitflags; use rustc_data_structures::intern::Interned; @@ -1272,6 +1272,31 @@ pub fn is_uninhabited(&self) -> bool { pub fn is_scalar(&self) -> bool { matches!(*self, Abi::Scalar(_)) } + + /// Returns the fixed alignment of this ABI, if any + pub fn inherent_align(&self, cx: &C) -> Option { + Some(match *self { + Abi::Scalar(s) => s.align(cx), + Abi::ScalarPair(s1, s2) => { + AbiAndPrefAlign::new(cmp::max(s1.align(cx).abi, s2.align(cx).abi)) + } + Abi::Vector { element, count } => { + cx.data_layout().vector_align(element.size(cx) * count) + } + Abi::Uninhabited | Abi::Aggregate { .. } => return None, + }) + } + + /// Discard valid range information and allow undef + pub fn to_union(&self) -> Self { + assert!(self.is_sized()); + match *self { + Abi::Scalar(s) => Abi::Scalar(s.to_union()), + Abi::ScalarPair(s1, s2) => Abi::ScalarPair(s1.to_union(), s2.to_union()), + Abi::Vector { element, count } => Abi::Vector { element: element.to_union(), count }, + Abi::Uninhabited | Abi::Aggregate { .. } => Abi::Aggregate { sized: true }, + } + } } #[derive(PartialEq, Eq, Hash, Clone, Debug)] From 4f4f22b11cad95d54dbc59d6613c4df767e7de64 Mon Sep 17 00:00:00 2001 From: Luqman Aden Date: Wed, 30 Nov 2022 23:12:04 -0800 Subject: [PATCH 3/9] Incorporate review feedback from 103926. --- compiler/rustc_abi/src/layout.rs | 75 +++++++++++++++----------------- 1 file changed, 35 insertions(+), 40 deletions(-) diff --git a/compiler/rustc_abi/src/layout.rs b/compiler/rustc_abi/src/layout.rs index 1bcc44237a3..356a3b5cb06 100644 --- a/compiler/rustc_abi/src/layout.rs +++ b/compiler/rustc_abi/src/layout.rs @@ -729,39 +729,34 @@ fn layout_of_union( align = align.max(AbiAndPrefAlign::new(repr_align)); } - let optimize = !repr.inhibit_union_abi_opt(); + let mut optimize = !repr.inhibit_union_abi_opt(); let mut size = Size::ZERO; - let mut abi = None; - let mut biggest_zst_align = align; - let mut biggest_non_zst_align = align; + let mut common_non_zst_abi_and_align: Option<(Abi, AbiAndPrefAlign)> = None; let only_variant = &variants[FIRST_VARIANT]; for field in only_variant { - assert!(!field.0.is_unsized()); + assert!(field.0.is_sized()); - if optimize { - // If all non-ZST fields have the same ABI, forward this ABI - if field.0.is_zst() { - biggest_zst_align = biggest_zst_align.max(field.align()); - } else { - biggest_non_zst_align = biggest_non_zst_align.max(field.align()); - // 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 !field.0.is_zst() && optimize { + // Discard valid range information and allow undef + let field_abi = field.abi().to_union(); - if let Some(abi) = &mut abi { - if *abi != field_abi { - // different fields have different ABI: reset to Aggregate - *abi = Abi::Aggregate { sized: true }; - } + if let Some((abi, align)) = &mut common_non_zst_abi_and_align { + if *abi != field_abi { + // Different fields have different ABI: disable opt + optimize = false; } else { - abi = Some(field_abi); + // Fields with the same non-Aggregate ABI should also + // have the same alignment + if !matches!(abi, Abi::Aggregate { .. }) { + assert_eq!( + align.abi, + field.align().abi, + "non-Aggregate field with matching ABI but differing alignment" + ); + } } + } else { + common_non_zst_abi_and_align = Some((field_abi, field.align())); } } @@ -769,24 +764,24 @@ fn layout_of_union( size = cmp::max(size, field.size()); } - let abi = match abi { - None => Abi::Aggregate { sized: true }, - Some(non_zst_abi) => { - 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. - Abi::Aggregate { sized: true } - } else { - non_zst_abi - } - } - }; - if let Some(pack) = repr.pack { align = align.min(AbiAndPrefAlign::new(pack)); } + // If all non-ZST fields have the same ABI, we may forward that ABI + // for the union as a whole, unless otherwise inhibited. + let abi = match (optimize, common_non_zst_abi_and_align) { + (false, _) | (_, None) => Abi::Aggregate { sized: true }, + (true, Some((abi, _))) => { + if abi.inherent_align(dl).map(|a| a.abi) != Some(align.abi) { + // Mismatched alignment: disable opt + Abi::Aggregate { sized: true } + } else { + abi + } + } + }; + Some(LayoutS { variants: Variants::Single { index: FIRST_VARIANT }, fields: FieldsShape::Union(NonZeroUsize::new(only_variant.len())?), From d5ab3a06d2a7a16f8774ea0b3c2455465b0962a9 Mon Sep 17 00:00:00 2001 From: Luqman Aden Date: Sat, 18 Feb 2023 17:42:16 -0800 Subject: [PATCH 4/9] Add test cases for #104802. --- tests/ui/layout/debug.rs | 23 +++++++- tests/ui/layout/debug.stderr | 108 +++++++++++++++++++++++++++++++---- 2 files changed, 120 insertions(+), 11 deletions(-) diff --git a/tests/ui/layout/debug.rs b/tests/ui/layout/debug.rs index c506b98f16e..78dbb1a299e 100644 --- a/tests/ui/layout/debug.rs +++ b/tests/ui/layout/debug.rs @@ -1,8 +1,9 @@ // normalize-stderr-test "pref: Align\([1-8] bytes\)" -> "pref: $$PREF_ALIGN" -#![feature(never_type, rustc_attrs, type_alias_impl_trait)] +#![feature(never_type, rustc_attrs, type_alias_impl_trait, repr_simd)] #![crate_type = "lib"] #[rustc_layout(debug)] +#[derive(Copy, Clone)] enum E { Foo, Bar(!, i32, i32) } //~ ERROR: layout_of #[rustc_layout(debug)] @@ -35,6 +36,26 @@ pub union Y { //~ ERROR: layout_of a: [u16; 0], } +#[rustc_layout(debug)] +#[repr(packed(1))] +union P1 { x: u32 } //~ ERROR: layout_of + +#[rustc_layout(debug)] +#[repr(packed(1))] +union P2 { x: (u32, u32) } //~ ERROR: layout_of + +#[repr(simd)] +#[derive(Copy, Clone)] +struct F32x4(f32, f32, f32, f32); + +#[rustc_layout(debug)] +#[repr(packed(1))] +union P3 { x: F32x4 } //~ ERROR: layout_of + +#[rustc_layout(debug)] +#[repr(packed(1))] +union P4 { x: E } //~ ERROR: layout_of + #[rustc_layout(debug)] type X = std::mem::MaybeUninit; //~ ERROR: layout_of diff --git a/tests/ui/layout/debug.stderr b/tests/ui/layout/debug.stderr index 6f6ab13eac5..c296c2ba797 100644 --- a/tests/ui/layout/debug.stderr +++ b/tests/ui/layout/debug.stderr @@ -81,7 +81,7 @@ error: layout_of(E) = Layout { ], }, } - --> $DIR/debug.rs:6:1 + --> $DIR/debug.rs:7:1 | LL | enum E { Foo, Bar(!, i32, i32) } | ^^^^^^ @@ -125,7 +125,7 @@ error: layout_of(S) = Layout { index: 0, }, } - --> $DIR/debug.rs:9:1 + --> $DIR/debug.rs:10:1 | LL | struct S { f1: i32, f2: (), f3: i32 } | ^^^^^^^^ @@ -147,7 +147,7 @@ error: layout_of(U) = Layout { index: 0, }, } - --> $DIR/debug.rs:12:1 + --> $DIR/debug.rs:13:1 | LL | union U { f1: (i32, i32), f3: i32 } | ^^^^^^^ @@ -276,7 +276,7 @@ error: layout_of(std::result::Result) = Layout { ], }, } - --> $DIR/debug.rs:15:1 + --> $DIR/debug.rs:16:1 | LL | type Test = Result; | ^^^^^^^^^ @@ -302,7 +302,7 @@ error: layout_of(i32) = Layout { index: 0, }, } - --> $DIR/debug.rs:18:1 + --> $DIR/debug.rs:19:1 | LL | type T = impl std::fmt::Debug; | ^^^^^^ @@ -324,7 +324,7 @@ error: layout_of(V) = Layout { index: 0, }, } - --> $DIR/debug.rs:21:1 + --> $DIR/debug.rs:22:1 | LL | pub union V { | ^^^^^^^^^^^ @@ -346,7 +346,7 @@ error: layout_of(W) = Layout { index: 0, }, } - --> $DIR/debug.rs:27:1 + --> $DIR/debug.rs:28:1 | LL | pub union W { | ^^^^^^^^^^^ @@ -368,11 +368,99 @@ error: layout_of(Y) = Layout { index: 0, }, } - --> $DIR/debug.rs:33:1 + --> $DIR/debug.rs:34:1 | LL | pub union Y { | ^^^^^^^^^^^ +error: layout_of(P1) = Layout { + size: Size(4 bytes), + align: AbiAndPrefAlign { + abi: Align(1 bytes), + pref: $PREF_ALIGN, + }, + abi: Aggregate { + sized: true, + }, + fields: Union( + 1, + ), + largest_niche: None, + variants: Single { + index: 0, + }, + } + --> $DIR/debug.rs:41:1 + | +LL | union P1 { x: u32 } + | ^^^^^^^^ + +error: layout_of(P2) = Layout { + size: Size(8 bytes), + align: AbiAndPrefAlign { + abi: Align(1 bytes), + pref: $PREF_ALIGN, + }, + abi: Aggregate { + sized: true, + }, + fields: Union( + 1, + ), + largest_niche: None, + variants: Single { + index: 0, + }, + } + --> $DIR/debug.rs:45:1 + | +LL | union P2 { x: (u32, u32) } + | ^^^^^^^^ + +error: layout_of(P3) = Layout { + size: Size(16 bytes), + align: AbiAndPrefAlign { + abi: Align(1 bytes), + pref: $PREF_ALIGN, + }, + abi: Aggregate { + sized: true, + }, + fields: Union( + 1, + ), + largest_niche: None, + variants: Single { + index: 0, + }, + } + --> $DIR/debug.rs:53:1 + | +LL | union P3 { x: F32x4 } + | ^^^^^^^^ + +error: layout_of(P4) = Layout { + size: Size(12 bytes), + align: AbiAndPrefAlign { + abi: Align(1 bytes), + pref: $PREF_ALIGN, + }, + abi: Aggregate { + sized: true, + }, + fields: Union( + 1, + ), + largest_niche: None, + variants: Single { + index: 0, + }, + } + --> $DIR/debug.rs:57:1 + | +LL | union P4 { x: E } + | ^^^^^^^^ + error: layout_of(std::mem::MaybeUninit) = Layout { size: Size(1 bytes), align: AbiAndPrefAlign { @@ -395,10 +483,10 @@ error: layout_of(std::mem::MaybeUninit) = Layout { index: 0, }, } - --> $DIR/debug.rs:39:1 + --> $DIR/debug.rs:60:1 | LL | type X = std::mem::MaybeUninit; | ^^^^^^ -error: aborting due to 9 previous errors +error: aborting due to 13 previous errors From f2d81defa1e78921db326835fc9a7e21475868d1 Mon Sep 17 00:00:00 2001 From: Luqman Aden Date: Wed, 30 Nov 2022 23:16:57 -0800 Subject: [PATCH 5/9] Add additional test case for repr(packed) allowing union abi opt to kick in. --- tests/ui/layout/debug.rs | 4 ++++ tests/ui/layout/debug.stderr | 31 +++++++++++++++++++++++++++++-- 2 files changed, 33 insertions(+), 2 deletions(-) diff --git a/tests/ui/layout/debug.rs b/tests/ui/layout/debug.rs index 78dbb1a299e..46171880a6f 100644 --- a/tests/ui/layout/debug.rs +++ b/tests/ui/layout/debug.rs @@ -56,6 +56,10 @@ union P3 { x: F32x4 } //~ ERROR: layout_of #[repr(packed(1))] union P4 { x: E } //~ ERROR: layout_of +#[rustc_layout(debug)] +#[repr(packed(1))] +union P5 { zst: [u16; 0], byte: u8 } //~ ERROR: layout_of + #[rustc_layout(debug)] type X = std::mem::MaybeUninit; //~ ERROR: layout_of diff --git a/tests/ui/layout/debug.stderr b/tests/ui/layout/debug.stderr index c296c2ba797..b9fa1b299e9 100644 --- a/tests/ui/layout/debug.stderr +++ b/tests/ui/layout/debug.stderr @@ -461,6 +461,33 @@ error: layout_of(P4) = Layout { LL | union P4 { x: E } | ^^^^^^^^ +error: layout_of(P5) = 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:61:1 + | +LL | union P5 { zst: [u16; 0], byte: u8 } + | ^^^^^^^^ + error: layout_of(std::mem::MaybeUninit) = Layout { size: Size(1 bytes), align: AbiAndPrefAlign { @@ -483,10 +510,10 @@ error: layout_of(std::mem::MaybeUninit) = Layout { index: 0, }, } - --> $DIR/debug.rs:60:1 + --> $DIR/debug.rs:64:1 | LL | type X = std::mem::MaybeUninit; | ^^^^^^ -error: aborting due to 13 previous errors +error: aborting due to 14 previous errors From 3b1e535f36ac4c47dc91d0e3394dca72fb86db0c Mon Sep 17 00:00:00 2001 From: Luqman Aden Date: Sat, 18 Feb 2023 19:21:07 -0800 Subject: [PATCH 6/9] Factor out checks in layout check and add helper inherent_size. --- compiler/rustc_abi/src/lib.rs | 23 ++++- .../rustc_ty_utils/src/layout_sanity_check.rs | 94 ++++++++----------- compiler/rustc_ty_utils/src/lib.rs | 1 + 3 files changed, 63 insertions(+), 55 deletions(-) diff --git a/compiler/rustc_abi/src/lib.rs b/compiler/rustc_abi/src/lib.rs index bbbc417e892..9c8a59979aa 100644 --- a/compiler/rustc_abi/src/lib.rs +++ b/compiler/rustc_abi/src/lib.rs @@ -1273,7 +1273,7 @@ pub fn is_scalar(&self) -> bool { matches!(*self, Abi::Scalar(_)) } - /// Returns the fixed alignment of this ABI, if any + /// Returns the fixed alignment of this ABI, if any is mandated. pub fn inherent_align(&self, cx: &C) -> Option { Some(match *self { Abi::Scalar(s) => s.align(cx), @@ -1287,6 +1287,27 @@ pub fn inherent_align(&self, cx: &C) -> Option(&self, cx: &C) -> Option { + Some(match *self { + Abi::Scalar(s) => { + // No padding in scalars. + s.size(cx) + } + Abi::ScalarPair(s1, s2) => { + // May have some padding between the pair. + let field2_offset = s1.size(cx).align_to(s2.align(cx).abi); + (field2_offset + s2.size(cx)).align_to(self.inherent_align(cx)?.abi) + } + Abi::Vector { element, count } => { + // No padding in vectors, except possibly for trailing padding + // to make the size a multiple of align (e.g. for vectors of size 3). + (element.size(cx) * count).align_to(self.inherent_align(cx)?.abi) + } + Abi::Uninhabited | Abi::Aggregate { .. } => return None, + }) + } + /// Discard valid range information and allow undef pub fn to_union(&self) -> Self { assert!(self.is_sized()); diff --git a/compiler/rustc_ty_utils/src/layout_sanity_check.rs b/compiler/rustc_ty_utils/src/layout_sanity_check.rs index ed513cb3c7f..c4a4cda6801 100644 --- a/compiler/rustc_ty_utils/src/layout_sanity_check.rs +++ b/compiler/rustc_ty_utils/src/layout_sanity_check.rs @@ -4,7 +4,7 @@ }; use rustc_target::abi::*; -use std::cmp; +use std::assert_matches::assert_matches; /// Enforce some basic invariants on layouts. pub(super) fn sanity_check_layout<'tcx>( @@ -68,21 +68,31 @@ fn skip_newtypes<'tcx>( } fn check_layout_abi<'tcx>(cx: &LayoutCx<'tcx, TyCtxt<'tcx>>, layout: &TyAndLayout<'tcx>) { + // Verify the ABI mandated alignment and size. + let align = layout.abi.inherent_align(cx).map(|align| align.abi); + let size = layout.abi.inherent_size(cx); + let Some((align, size)) = align.zip(size) else { + assert_matches!( + layout.layout.abi(), + Abi::Uninhabited | Abi::Aggregate { .. }, + "ABI unexpectedly missing alignment and/or size in {layout:#?}" + ); + return + }; + assert_eq!( + layout.layout.align().abi, + align, + "alignment mismatch between ABI and layout in {layout:#?}" + ); + assert_eq!( + layout.layout.size(), + size, + "size mismatch between ABI and layout in {layout:#?}" + ); + + // Verify per-ABI invariants match layout.layout.abi() { - Abi::Scalar(scalar) => { - // No padding in scalars. - let size = scalar.size(cx); - let align = scalar.align(cx).abi; - assert_eq!( - layout.layout.size(), - size, - "size mismatch between ABI and layout in {layout:#?}" - ); - assert_eq!( - layout.layout.align().abi, - align, - "alignment mismatch between ABI and layout in {layout:#?}" - ); + Abi::Scalar(_) => { // Check that this matches the underlying field. let inner = skip_newtypes(cx, layout); assert!( @@ -135,24 +145,6 @@ fn check_layout_abi<'tcx>(cx: &LayoutCx<'tcx, TyCtxt<'tcx>>, layout: &TyAndLayou } } Abi::ScalarPair(scalar1, scalar2) => { - // Sanity-check scalar pairs. Computing the expected size and alignment is a bit of work. - let size1 = scalar1.size(cx); - let align1 = scalar1.align(cx).abi; - let size2 = scalar2.size(cx); - let align2 = scalar2.align(cx).abi; - let align = cmp::max(align1, align2); - let field2_offset = size1.align_to(align2); - let size = (field2_offset + size2).align_to(align); - assert_eq!( - layout.layout.size(), - size, - "size mismatch between ABI and layout in {layout:#?}" - ); - assert_eq!( - layout.layout.align().abi, - align, - "alignment mismatch between ABI and layout in {layout:#?}", - ); // Check that the underlying pair of fields matches. let inner = skip_newtypes(cx, layout); assert!( @@ -189,8 +181,9 @@ fn check_layout_abi<'tcx>(cx: &LayoutCx<'tcx, TyCtxt<'tcx>>, layout: &TyAndLayou "`ScalarPair` layout for type with less than two non-ZST fields: {inner:#?}" ) }); - assert!( - fields.next().is_none(), + assert_matches!( + fields.next(), + None, "`ScalarPair` layout for type with at least three non-ZST fields: {inner:#?}" ); // The fields might be in opposite order. @@ -200,6 +193,10 @@ fn check_layout_abi<'tcx>(cx: &LayoutCx<'tcx, TyCtxt<'tcx>>, layout: &TyAndLayou (offset2, field2, offset1, field1) }; // The fields should be at the right offset, and match the `scalar` layout. + let size1 = scalar1.size(cx); + let align1 = scalar1.align(cx).abi; + let size2 = scalar2.size(cx); + let align2 = scalar2.align(cx).abi; assert_eq!( offset1, Size::ZERO, @@ -213,10 +210,12 @@ fn check_layout_abi<'tcx>(cx: &LayoutCx<'tcx, TyCtxt<'tcx>>, layout: &TyAndLayou field1.align.abi, align1, "`ScalarPair` first field with bad align in {inner:#?}", ); - assert!( - matches!(field1.abi, Abi::Scalar(_)), + assert_matches!( + field1.abi, + Abi::Scalar(_), "`ScalarPair` first field with bad ABI in {inner:#?}", ); + let field2_offset = size1.align_to(align2); assert_eq!( offset2, field2_offset, "`ScalarPair` second field at bad offset in {inner:#?}", @@ -229,27 +228,14 @@ fn check_layout_abi<'tcx>(cx: &LayoutCx<'tcx, TyCtxt<'tcx>>, layout: &TyAndLayou field2.align.abi, align2, "`ScalarPair` second field with bad align in {inner:#?}", ); - assert!( - matches!(field2.abi, Abi::Scalar(_)), + assert_matches!( + field2.abi, + Abi::Scalar(_), "`ScalarPair` second field with bad ABI in {inner:#?}", ); } - Abi::Vector { count, element } => { - // No padding in vectors, except possibly for trailing padding to make the size a multiple of align. - let size = element.size(cx) * count; - let align = cx.data_layout().vector_align(size).abi; - let size = size.align_to(align); // needed e.g. for vectors of size 3 + Abi::Vector { element, .. } => { assert!(align >= element.align(cx).abi); // just sanity-checking `vector_align`. - assert_eq!( - layout.layout.size(), - size, - "size mismatch between ABI and layout in {layout:#?}" - ); - assert_eq!( - layout.layout.align().abi, - align, - "alignment mismatch between ABI and layout in {layout:#?}" - ); // FIXME: Do some kind of check of the inner type, like for Scalar and ScalarPair. } Abi::Uninhabited | Abi::Aggregate { .. } => {} // Nothing to check. diff --git a/compiler/rustc_ty_utils/src/lib.rs b/compiler/rustc_ty_utils/src/lib.rs index 9d5a72a73cd..73a2f6af579 100644 --- a/compiler/rustc_ty_utils/src/lib.rs +++ b/compiler/rustc_ty_utils/src/lib.rs @@ -5,6 +5,7 @@ //! This API is completely unstable and subject to change. #![doc(html_root_url = "https://doc.rust-lang.org/nightly/nightly-rustc/")] +#![feature(assert_matches)] #![feature(iterator_try_collect)] #![feature(let_chains)] #![feature(never_type)] From c63a204e239f8360cfe8e35946e43a87a1c77577 Mon Sep 17 00:00:00 2001 From: Luqman Aden Date: Sat, 18 Feb 2023 19:55:11 -0800 Subject: [PATCH 7/9] Don't discard preferred alignment in scalar pair. --- compiler/rustc_abi/src/lib.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/compiler/rustc_abi/src/lib.rs b/compiler/rustc_abi/src/lib.rs index 9c8a59979aa..6d96b3db93c 100644 --- a/compiler/rustc_abi/src/lib.rs +++ b/compiler/rustc_abi/src/lib.rs @@ -1,11 +1,11 @@ #![cfg_attr(feature = "nightly", feature(step_trait, rustc_attrs, min_specialization))] +use std::fmt; #[cfg(feature = "nightly")] use std::iter::Step; use std::num::{NonZeroUsize, ParseIntError}; use std::ops::{Add, AddAssign, Mul, RangeInclusive, Sub}; use std::str::FromStr; -use std::{cmp, fmt}; use bitflags::bitflags; use rustc_data_structures::intern::Interned; @@ -1277,9 +1277,7 @@ pub fn is_scalar(&self) -> bool { pub fn inherent_align(&self, cx: &C) -> Option { Some(match *self { Abi::Scalar(s) => s.align(cx), - Abi::ScalarPair(s1, s2) => { - AbiAndPrefAlign::new(cmp::max(s1.align(cx).abi, s2.align(cx).abi)) - } + Abi::ScalarPair(s1, s2) => s1.align(cx).max(s2.align(cx)), Abi::Vector { element, count } => { cx.data_layout().vector_align(element.size(cx) * count) } From 012f9a333b4017f5cb3d7917b03132b79a26dc09 Mon Sep 17 00:00:00 2001 From: Luqman Aden Date: Sun, 19 Feb 2023 16:25:07 -0800 Subject: [PATCH 8/9] Review feedback --- compiler/rustc_abi/src/layout.rs | 35 ++++++++++++++++++++------------ compiler/rustc_abi/src/lib.rs | 2 +- 2 files changed, 23 insertions(+), 14 deletions(-) diff --git a/compiler/rustc_abi/src/layout.rs b/compiler/rustc_abi/src/layout.rs index 356a3b5cb06..e46c171d731 100644 --- a/compiler/rustc_abi/src/layout.rs +++ b/compiler/rustc_abi/src/layout.rs @@ -729,34 +729,43 @@ fn layout_of_union( align = align.max(AbiAndPrefAlign::new(repr_align)); } - let mut optimize = !repr.inhibit_union_abi_opt(); + // If all the non-ZST fields have the same ABI and union ABI optimizations aren't + // disabled, we can use that common ABI for the union as a whole. + struct AbiMismatch; + let mut common_non_zst_abi_and_align = if repr.inhibit_union_abi_opt() { + // Can't optimize + Err(AbiMismatch) + } else { + Ok(None) + }; + let mut size = Size::ZERO; - let mut common_non_zst_abi_and_align: Option<(Abi, AbiAndPrefAlign)> = None; let only_variant = &variants[FIRST_VARIANT]; for field in only_variant { assert!(field.0.is_sized()); - if !field.0.is_zst() && optimize { + if !field.0.is_zst() && !common_non_zst_abi_and_align.is_err() { // Discard valid range information and allow undef let field_abi = field.abi().to_union(); - if let Some((abi, align)) = &mut common_non_zst_abi_and_align { - if *abi != field_abi { + if let Ok(Some((common_abi, common_align))) = &mut common_non_zst_abi_and_align { + if *common_abi != field_abi { // Different fields have different ABI: disable opt - optimize = false; + common_non_zst_abi_and_align = Err(AbiMismatch); } else { // Fields with the same non-Aggregate ABI should also // have the same alignment - if !matches!(abi, Abi::Aggregate { .. }) { + if !matches!(common_abi, Abi::Aggregate { .. }) { assert_eq!( - align.abi, + *common_align, field.align().abi, "non-Aggregate field with matching ABI but differing alignment" ); } } } else { - common_non_zst_abi_and_align = Some((field_abi, field.align())); + // First non-ZST field: record its ABI and alignment + common_non_zst_abi_and_align = Ok(Some((field_abi, field.align().abi))); } } @@ -770,11 +779,11 @@ fn layout_of_union( // If all non-ZST fields have the same ABI, we may forward that ABI // for the union as a whole, unless otherwise inhibited. - let abi = match (optimize, common_non_zst_abi_and_align) { - (false, _) | (_, None) => Abi::Aggregate { sized: true }, - (true, Some((abi, _))) => { + let abi = match common_non_zst_abi_and_align { + Err(AbiMismatch) | Ok(None) => Abi::Aggregate { sized: true }, + Ok(Some((abi, _))) => { if abi.inherent_align(dl).map(|a| a.abi) != Some(align.abi) { - // Mismatched alignment: disable opt + // Mismatched alignment (e.g. union is #[repr(packed)]): disable opt Abi::Aggregate { sized: true } } else { abi diff --git a/compiler/rustc_abi/src/lib.rs b/compiler/rustc_abi/src/lib.rs index 6d96b3db93c..43db66a3c28 100644 --- a/compiler/rustc_abi/src/lib.rs +++ b/compiler/rustc_abi/src/lib.rs @@ -1306,7 +1306,7 @@ pub fn inherent_size(&self, cx: &C) -> Option { }) } - /// Discard valid range information and allow undef + /// Discard validity range information and allow undef. pub fn to_union(&self) -> Self { assert!(self.is_sized()); match *self { From 8e7714d3bb81e41ed3e812415626acbabd20ff02 Mon Sep 17 00:00:00 2001 From: Luqman Aden Date: Fri, 5 May 2023 16:30:32 -0700 Subject: [PATCH 9/9] Reorder to keep duplicate checks in sync. --- compiler/rustc_abi/src/layout.rs | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/compiler/rustc_abi/src/layout.rs b/compiler/rustc_abi/src/layout.rs index e46c171d731..3d97d9b4895 100644 --- a/compiler/rustc_abi/src/layout.rs +++ b/compiler/rustc_abi/src/layout.rs @@ -744,12 +744,20 @@ fn layout_of_union( for field in only_variant { assert!(field.0.is_sized()); - if !field.0.is_zst() && !common_non_zst_abi_and_align.is_err() { + align = align.max(field.align()); + size = cmp::max(size, field.size()); + + if field.0.is_zst() { + // Nothing more to do for ZST fields + continue; + } + + if let Ok(common) = common_non_zst_abi_and_align { // Discard valid range information and allow undef let field_abi = field.abi().to_union(); - if let Ok(Some((common_abi, common_align))) = &mut common_non_zst_abi_and_align { - if *common_abi != field_abi { + if let Some((common_abi, common_align)) = common { + if common_abi != field_abi { // Different fields have different ABI: disable opt common_non_zst_abi_and_align = Err(AbiMismatch); } else { @@ -757,7 +765,7 @@ fn layout_of_union( // have the same alignment if !matches!(common_abi, Abi::Aggregate { .. }) { assert_eq!( - *common_align, + common_align, field.align().abi, "non-Aggregate field with matching ABI but differing alignment" ); @@ -768,9 +776,6 @@ fn layout_of_union( common_non_zst_abi_and_align = Ok(Some((field_abi, field.align().abi))); } } - - align = align.max(field.align()); - size = cmp::max(size, field.size()); } if let Some(pack) = repr.pack {