Skip to content

Commit

Permalink
compiler: Make partially_check_layout fallible
Browse files Browse the repository at this point in the history
We already propagate LayoutErrors in some cases, so double down on that.
This addresses an ICE that is reached only with debug assertions.
  • Loading branch information
workingjubilee committed Oct 18, 2024
1 parent f2f1965 commit 12ce35a
Show file tree
Hide file tree
Showing 2 changed files with 30 additions and 18 deletions.
8 changes: 5 additions & 3 deletions compiler/rustc_ty_utils/src/layout.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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> {
Expand Down
40 changes: 25 additions & 15 deletions compiler/rustc_ty_utils/src/layout/invariant.rs
Original file line number Diff line number Diff line change
@@ -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<TyAndLayout<'tcx>>> {
let tcx = cx.tcx();

// Type-level uninhabitedness should always imply ABI uninhabitedness.
Expand All @@ -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
Expand Down Expand Up @@ -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<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);
Expand All @@ -74,21 +84,19 @@ 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,
"size mismatch between ABI and layout in {layout:#?}"
);

// 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);
Expand All @@ -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.
Expand Down Expand Up @@ -153,15 +161,15 @@ 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 { .. } => {
// Checked below.
}
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:#?}");
Expand Down Expand Up @@ -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() {
Expand Down Expand Up @@ -293,4 +302,5 @@ pub(super) fn partially_check_layout<'tcx>(cx: &LayoutCx<'tcx>, layout: &TyAndLa
}
}
}
Ok(())
}

0 comments on commit 12ce35a

Please sign in to comment.