Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

safe transmute: support Single enums #126358

Merged
merged 1 commit into from
Jun 13, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 1 addition & 4 deletions compiler/rustc_const_eval/src/interpret/discriminant.rs
Original file line number Diff line number Diff line change
Expand Up @@ -241,10 +241,7 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
variant_index: VariantIdx,
) -> InterpResult<'tcx, Option<(ScalarInt, usize)>> {
match self.layout_of(ty)?.variants {
abi::Variants::Single { index } => {
assert_eq!(index, variant_index);
Copy link
Member Author

@jswrenn jswrenn Jun 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing this assertion was recommended by @RalfJung in #125811 (comment), and I agree with his assessment that it isn't a load-bearing assertion. An index mismatch here doesn't suggest a bug in computing the tag.

That said, removing this assertion is irrelevant to fixing the bug — the salient changes are all in compiler/rustc_transmute/src/layout/tree.rs. Those changes fix the ICE even with this assertion intact.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest to replace the assertion by a comment, like

// The tag of a `Single` enum is like the tag of the niched variant: there's no tag
// as the discriminant is encoded entirely implicitly.
// If `write_discriminant` ever hits this case, we do a "validation read" to ensure
// the the right discriminant is encoded implicitly, so any attempt to write the
// wrong discriminant for a `Single` enum will reliably result in UB.

Ok(None)
}
abi::Variants::Single { .. } => Ok(None),

abi::Variants::Multiple {
tag_encoding: TagEncoding::Direct,
Expand Down
75 changes: 54 additions & 21 deletions compiler/rustc_transmute/src/layout/tree.rs
Original file line number Diff line number Diff line change
Expand Up @@ -331,30 +331,63 @@ pub(crate) mod rustc {
assert!(def.is_enum());
let layout = ty_and_layout.layout;

if let Variants::Multiple { tag_field, .. } = layout.variants() {
// For enums (but not coroutines), the tag field is
// currently always the first field of the layout.
assert_eq!(*tag_field, 0);
}
// Computes the variant of a given index.
let layout_of_variant = |index| {
let tag = cx.tcx.tag_for_variant((ty_and_layout.ty, index));
let variant_def = Def::Variant(def.variant(index));
let variant_ty_and_layout = ty_and_layout.for_variant(&cx, index);
Self::from_variant(variant_def, tag, variant_ty_and_layout, layout.size, cx)
};

let variants = def.discriminants(cx.tcx()).try_fold(
Self::uninhabited(),
|variants, (idx, ref discriminant)| {
let tag = cx.tcx.tag_for_variant((ty_and_layout.ty, idx));
let variant_def = Def::Variant(def.variant(idx));
let variant_ty_and_layout = ty_and_layout.for_variant(&cx, idx);
let variant = Self::from_variant(
variant_def,
tag,
variant_ty_and_layout,
layout.size,
cx,
// We consider three kinds of enums, each demanding a different
// treatment of their layout computation:
// 1. enums that are uninhabited
// 2. enums for which all but one variant is uninhabited
// 3. enums with multiple inhabited variants
match layout.variants() {
compiler-errors marked this conversation as resolved.
Show resolved Hide resolved
_ if layout.abi.is_uninhabited() => {
// Uninhabited enums are usually (always?) zero-sized. In
// the (unlikely?) event that an uninhabited enum is
// non-zero-sized, this assert will trigger an ICE, and this
// code should be modified such that a `layout.size` amount
// of uninhabited bytes is returned instead.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you tried an enum like enum E { Variant(!, i32) }? That may give rise to a non-zero-sized uninhabited enum.

//
// Uninhabited enums are currently implemented such that
// their layout is described with `Variants::Single`, even
// though they don't necessarily have a 'single' variant to
// defer to. That said, we don't bother specifically
// matching on `Variants::Single` in this arm because the
// behavioral principles here remain true even if, for
// whatever reason, the compiler describes an uninhabited
// enum with `Variants::Multiple`.
assert_eq!(layout.size, Size::ZERO);
Ok(Self::uninhabited())
}
Variants::Single { index } => {
// `Variants::Single` on non-uninhabited enums denotes that
// the enum delegates its layout to the variant at `index`.
layout_of_variant(*index)
}
Variants::Multiple { tag_field, .. } => {
// `Variants::Multiple` denotes an enum with multiple
// inhabited variants. The layout of such an enum is the
// disjunction of the layouts of its tagged variants.

// For enums (but not coroutines), the tag field is
// currently always the first field of the layout.
assert_eq!(*tag_field, 0);

let variants = def.discriminants(cx.tcx()).try_fold(
Self::uninhabited(),
|variants, (idx, ref discriminant)| {
let variant = layout_of_variant(idx)?;
Result::<Self, Err>::Ok(variants.or(variant))
},
)?;
Result::<Self, Err>::Ok(variants.or(variant))
},
)?;

return Ok(Self::def(Def::Adt(def)).then(variants));
return Ok(Self::def(Def::Adt(def)).then(variants));
}
}
}

/// Constructs a `Tree` from a 'variant-like' layout.
Expand Down
34 changes: 0 additions & 34 deletions tests/crashes/125811.rs

This file was deleted.

26 changes: 26 additions & 0 deletions tests/ui/transmutability/enums/uninhabited_optimization.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
//@ check-pass
//! Tests that we do not regress rust-lang/rust#125811
#![feature(transmutability)]

fn assert_transmutable<T>()
where
(): std::mem::BikeshedIntrinsicFrom<T>
{}

enum Uninhabited {}

enum SingleInhabited {
X,
Y(Uninhabited)
}

enum SingleUninhabited {
X(Uninhabited),
Y(Uninhabited),
}

fn main() {
assert_transmutable::<Uninhabited>();
assert_transmutable::<SingleInhabited>();
assert_transmutable::<SingleUninhabited>();
}
Loading