From 12ce35a1000ff0f968b2e38b6e9e750dd6e56103 Mon Sep 17 00:00:00 2001 From: Jubilee Young Date: Thu, 17 Oct 2024 16:26:04 -0700 Subject: [PATCH] compiler: Make `partially_check_layout` fallible We already propagate LayoutErrors in some cases, so double down on that. This addresses an ICE that is reached only with debug assertions. --- compiler/rustc_ty_utils/src/layout.rs | 8 ++-- .../rustc_ty_utils/src/layout/invariant.rs | 40 ++++++++++++------- 2 files changed, 30 insertions(+), 18 deletions(-) diff --git a/compiler/rustc_ty_utils/src/layout.rs b/compiler/rustc_ty_utils/src/layout.rs index 38b292afe8d71..9553b3f6c9d5b 100644 --- a/compiler/rustc_ty_utils/src/layout.rs +++ b/compiler/rustc_ty_utils/src/layout.rs @@ -80,9 +80,11 @@ fn layout_of<'tcx>( record_layout_for_printing(&cx, layout); } - invariant::partially_check_layout(&cx, &layout); - - Ok(layout) + if let Err(err) = invariant::partially_check_layout(&cx, &layout) { + Err(map_error(&cx, ty, err)) + } else { + Ok(layout) + } } fn error<'tcx>(cx: &LayoutCx<'tcx>, err: LayoutError<'tcx>) -> &'tcx LayoutError<'tcx> { diff --git a/compiler/rustc_ty_utils/src/layout/invariant.rs b/compiler/rustc_ty_utils/src/layout/invariant.rs index 6cf114b74c13d..6f25ed8cab178 100644 --- a/compiler/rustc_ty_utils/src/layout/invariant.rs +++ b/compiler/rustc_ty_utils/src/layout/invariant.rs @@ -1,11 +1,18 @@ use std::assert_matches::assert_matches; +use rustc_abi::LayoutCalculatorError; use rustc_middle::bug; use rustc_middle::ty::layout::{HasTyCtxt, LayoutCx, TyAndLayout}; use rustc_target::abi::*; /// Enforce some basic invariants on layouts. -pub(super) fn partially_check_layout<'tcx>(cx: &LayoutCx<'tcx>, layout: &TyAndLayout<'tcx>) { +/// +/// Note most of the code is skipped if debug assertions are not enabled, +/// and we bail left and right, thus the name of "partially check". +pub(super) fn partially_check_layout<'tcx>( + cx: &LayoutCx<'tcx>, + layout: &TyAndLayout<'tcx>, +) -> Result<(), LayoutCalculatorError>> { let tcx = cx.tcx(); // Type-level uninhabitedness should always imply ABI uninhabitedness. @@ -22,7 +29,7 @@ pub(super) fn partially_check_layout<'tcx>(cx: &LayoutCx<'tcx>, layout: &TyAndLa if !cfg!(debug_assertions) { // Stop here, the rest is kind of expensive. - return; + return Ok(()); } /// Yields non-ZST fields of the type @@ -64,7 +71,10 @@ pub(super) fn partially_check_layout<'tcx>(cx: &LayoutCx<'tcx>, layout: &TyAndLa *layout } - fn check_layout_abi<'tcx>(cx: &LayoutCx<'tcx>, layout: &TyAndLayout<'tcx>) { + fn check_layout_abi<'tcx>( + cx: &LayoutCx<'tcx>, + layout: &TyAndLayout<'tcx>, + ) -> Result<(), LayoutCalculatorError>> { // 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); @@ -74,13 +84,11 @@ pub(super) fn partially_check_layout<'tcx>(cx: &LayoutCx<'tcx>, layout: &TyAndLa Abi::Uninhabited | Abi::Aggregate { .. }, "ABI unexpectedly missing alignment and/or size in {layout:#?}" ); - return; + return Ok(()); }; - assert_eq!( - layout.layout.align().abi, - align, - "alignment mismatch between ABI and layout in {layout:#?}" - ); + if layout.layout.align().abi != align { + return Err(LayoutCalculatorError::ReprConflict); + } assert_eq!( layout.layout.size(), size, @@ -88,7 +96,7 @@ pub(super) fn partially_check_layout<'tcx>(cx: &LayoutCx<'tcx>, layout: &TyAndLa ); // Verify per-ABI invariants - match layout.layout.abi() { + let _unit = match layout.layout.abi() { Abi::Scalar(_) => { // Check that this matches the underlying field. let inner = skip_newtypes(cx, layout); @@ -104,7 +112,7 @@ pub(super) fn partially_check_layout<'tcx>(cx: &LayoutCx<'tcx>, layout: &TyAndLa } FieldsShape::Union(..) => { // FIXME: I guess we could also check something here? Like, look at all fields? - return; + return Ok(()); } FieldsShape::Arbitrary { .. } => { // Should be an enum, the only field is the discriminant. @@ -153,7 +161,7 @@ pub(super) fn partially_check_layout<'tcx>(cx: &LayoutCx<'tcx>, layout: &TyAndLa if matches!(inner.layout.variants(), Variants::Multiple { .. }) { // FIXME: ScalarPair for enums is enormously complicated and it is very hard // to check anything about them. - return; + return Ok(()); } match inner.layout.fields() { FieldsShape::Arbitrary { .. } => { @@ -161,7 +169,7 @@ pub(super) fn partially_check_layout<'tcx>(cx: &LayoutCx<'tcx>, layout: &TyAndLa } FieldsShape::Union(..) => { // FIXME: I guess we could also check something here? Like, look at all fields? - return; + return Ok(()); } _ => { panic!("`ScalarPair` layout with unexpected field shape in {inner:#?}"); @@ -236,10 +244,11 @@ pub(super) fn partially_check_layout<'tcx>(cx: &LayoutCx<'tcx>, layout: &TyAndLa // FIXME: Do some kind of check of the inner type, like for Scalar and ScalarPair. } Abi::Uninhabited | Abi::Aggregate { .. } => {} // Nothing to check. - } + }; + Ok(_unit) } - check_layout_abi(cx, layout); + let _unit = check_layout_abi(cx, layout)?; if let Variants::Multiple { variants, .. } = &layout.variants { for variant in variants.iter() { @@ -293,4 +302,5 @@ pub(super) fn partially_check_layout<'tcx>(cx: &LayoutCx<'tcx>, layout: &TyAndLa } } } + Ok(()) }