Layout `Variants::Multiple` has tons of redundancy, e.g. `Scalar` enums have `Aggregate` variants

[RalfJung]

See here for an elaboration of the redundancy. Below follows the original issue that lead to us looking into this.

---

I just discovered something rather strange in our enum layouts: std::sync::atomic::Ordering has

           abi: Scalar(
               Initialized {
                   value: Int(
                       I8,
                       false,
                   ),
                   valid_range: 0..=4,
               },
           ),

but its variants look like

                   Layout {
                       size: Size(1 bytes),
                       align: AbiAndPrefAlign {
                           abi: Align(1 bytes),
                           pref: Align(8 bytes),
                       },
                       abi: Aggregate {
                           sized: true,
                       },
                       fields: Arbitrary {
                           offsets: [],
                           memory_index: [],
                       },
                       largest_niche: None,
                       variants: Single {
                           index: 0,
                       },
                       max_repr_align: None,
                       unadjusted_abi_align: Align(1 bytes),
                   },

Note the abi: Aggregate!

I am surprised that this has not already lead to ICEs in CTFE/Miri -- via downcasting, one can end up with Immediate::Scalar that have Aggregate ABI, which can quickly panic.

This is a bug, right? If the enum has Scalar layout, then all its variants should also have that layout? For ScalarPair enums, this seems to already be the case.
Cc @oli-obk @eddyb

[RalfJung]

Ah, that's related to this skip in the layout sanity check:

rust/compiler/rustc_ty_utils/src/layout_sanity_check.rs

Lines 267 to 278 in 37cd634

	// Skip empty variants.
	if variant.size == Size::ZERO
	|| variant.fields.count() == 0
	|| variant.abi.is_uninhabited()
	{
	// These are never actually accessed anyway, so we can skip the coherence check
	// for them. They also fail that check, since they have
	// `Aggregate`/`Uninhabited` ABI even when the main type is
	// `Scalar`/`ScalarPair`. (Note that sometimes, variants with fields have size
	// 0, and sometimes, variants without fields have non-0 size.)
	continue;
	}

In Miri it is the visitor creating these downcasts. Maye it just shouldn't downcast in some cases? However, while I can see that it makes sense to never downcast with variant.fields.count() == 0 (this is always valid) or variant.abi.is_uninhabited() (this is always invalid), I am not sure what to do with neither of these cases apply but we have variant.size == Size::ZERO...

[eddyb]

This is a bug, right? If the enum has Scalar layout, then all its variants should also have that layout (and same for ScalarPair)?

It's... more complicated than that. The enum behaves like union { tag, Variant1, Variant2, ... }.
What you have there for the variant is a fieldless Aggregate which should maybe be called a ZFT ("zero-field type"), though even if it had fields it might still not be an issue as long as there's no data (i.e. it's a tree of ZSTs).

You can see some special-casing for this in codegen, I believe, where ZST Aggregates are the "simplest" Immediates, before Scalars and Vectors (but enums or their variants tend not to be treated as immediates in codegen, sadly, even if they theoretically could, if discriminants were read/written in a more flexible way).

---

What I'm referring to is a bit more obvious when you have a ScalarPair enum: if one of its two Scalars is a tag, then some variants may be Scalar (and some Aggregate if they have no data), but no variants will be ScalarPair.
Sorry, while writing that, I realized that I'm not 100% sure that a variant can be Scalar, like in Option<u8>, does the variant having an u8 at offset of 1 rule it out from being Scalar? It probably should/would but I'm not sure.

Either way, my point is that the variant doesn't include the tag, so it can't have an abi that does (arguably a variant shouldn't even have an abi field of its own, but that's harder to encode in the Layout types than the status quo).

[RalfJung]

What I'm referring to is a bit more obvious when you have a ScalarPair enum: if one of its two Scalars is a tag, then some variants may be Scalar (and some Aggregate if they have no data), but no variants will be ScalarPair.

That's not what happens though. Variants do include the tag. Looking at this type:

#[rustc_layout(debug)]
type T = std::result::Result<usize, ()>;

then both variants have

                       abi: ScalarPair(
                           Initialized {
                               value: Int(
                                   I64,
                                   false,
                               ),
                               valid_range: 0..=1,
                           },
                           Union {
                               value: Int(
                                   I64,
                                   false,
                               ),
                           },
                       ),

And that is quite required. Downcast leaves the offset the same, and so if downcast to the first variant had Scalar layout that would clearly be invalid (the data starts at offset 8 but Scalar cannot have leading padding).

However, with this type

#[rustc_layout(debug)]
type T = std::result::Result<&'static [u8], ()>;

we have a similar first variant

                       abi: ScalarPair(
                           Initialized {
                               value: Pointer(
                                   AddressSpace(
                                       0,
                                   ),
                               ),
                               valid_range: 1..=18446744073709551615,
                           },
                           Initialized {
                               value: Int(
                                   I64,
                                   false,
                               ),
                               valid_range: 0..=18446744073709551615,
                           },
                       ),

but the 2nd variant now is

                       abi: Aggregate {
                           sized: true,
                       },

This is a problem since it means a downcast to the 2nd variant creates invalid data (an immediate with Aggregate ABI), and there is no simple check for the visitor to avoid that downcast. (We can't just skip everything zero-sized since ! is zero-sized and clearly we must complain if that were present.)

[eddyb]

rust/compiler/rustc_abi/src/layout.rs

Lines 677 to 691 in 8771282

	// If we pick a "clever" (by-value) ABI, we might have to adjust the ABI of the
	// variants to ensure they are consistent. This is because a downcast is
	// semantically a NOP, and thus should not affect layout.
	if matches!(abi, Abi::Scalar(..) | Abi::ScalarPair(..)) {
	for variant in &mut layout_variants {
	// We only do this for variants with fields; the others are not accessed anyway.
	// Also do not overwrite any already existing "clever" ABIs.
	if variant.fields.count() > 0 && matches!(variant.abi, Abi::Aggregate { .. }) {
	variant.abi = abi;
	// Also need to bump up the size and alignment, so that the entire value fits in here.
	variant.size = cmp::max(variant.size, size);
	variant.align.abi = cmp::max(variant.align.abi, align.abi);
	}
	}
	}

Hah, we already do "variants don't have their own abi" but by writing to it (instead of disallowing access).

Just remove the variant.fields.count() > 0 condition and the related comment, looks like a microopt?

[RalfJung]

Sadly that does not seem to help. After all, this type does have fields in each variant

#[rustc_layout(debug)]
type T = std::result::Result<&'static [u8], ()>;

and yet its 2nd variant gets Aggregate layout. Removing the variant.fields.count() > 0 && doesn't change that.

[eddyb]

Sadly that does not seem to help. After all, this type does have fields in each variant

#[rustc_layout(debug)]
type T = std::result::Result<&'static [u8], ()>;

and yet its 2nd variant gets Aggregate layout. Removing the variant.fields.count() > 0 && doesn't change that.

That's a niche enum, not a tag one, whereas the code I linked is tag-specific. It's possible niches also need to be changed, but it would likely look a bit different anyway (and I don't know that there is anything like that snippet I linked already).

[eddyb]

Honestly, reviewing the definition of Layout, for variant Layouts:

• fields: FieldsShape: definitely wanted/needed
• variants: always equal to Variants::Single { index } (of its own VariantIdx in the enum)
• abi, size, align: almost always reset to the whole enum's
  • (maybe the cases where this doesn't happen are either buggy or silently ignored?)
• largest_niche, max_repr_align, unadjusted_abi_align: never used, AFAICT
  • (that is, I assume that these are only ever read from full types, not variants)

So a potentially clean solution might be to change Variants::Multiple itself

• from variants: IndexVec<VariantIdx, LayoutS>
• to variant_fields: IndexVec<VariantIdx, FieldsShape>

The only complication is the overreliance on TyAndLayout::for_variant, which makes a whole TyAndLayout, but doing so with the new scheme would require separating FieldsShape and Variants from the other LayoutS fields, or some more cursed contrivance (or just rewriting all for_variant callers...).

[RalfJung]

Turns out that the cases variant.size == Size::ZERO and variant.abi.is_uninhabited() are already handled correctly. So I just had to add a check in the visitor to not do place projection when variant.fields.count() == 0.

@eddyb do you want to keep this open to track the refactoring you suggested?

[eddyb]

do you want to keep this open to track the refactoring you suggested?

I think so, at least until someone tries it and realizes it's too disruptive (but I think long-term removing redundancy like this should be an improvement - if truly redundant, that is).