For enums with ScalarPair ABI, some variants have Aggregate ABI

[RalfJung]

I noticed this while working on #96185: I added the following sanity check in Miri's operand_downcast:

--- a/compiler/rustc_const_eval/src/interpret/operand.rs
+++ b/compiler/rustc_const_eval/src/interpret/operand.rs
@@ -441,6 +441,14 @@ pub fn operand_downcast(
                 // (In particular, no check about whether this is even the active variant -- that's by design,
                 // see https://github.com/rust-lang/rust/issues/93688#issuecomment-1032929496.)
                 let layout = op.layout.for_variant(self, variant);
+                if matches!(op.layout.abi, Abi::ScalarPair { .. }) {
+                    assert!(
+                        matches!(layout.abi, Abi::ScalarPair { .. } | Abi::Scalar { .. }),
+                        "downcast to {variant:?} turned ScalarPair layout into non-scalar layout: {:#?} to {:#?}",
+                        op.layout,
+                        layout,
+                    );
+                }
                 OpTy { layout, ..*op }
             }
         })

This assertion indeed fails. When downcasting std::option::Option<*mut std::ffi::c_void> to variant 1, the ABI changes from ScalarPair to Aggegreate.

downcast to 1 turned ScalarPair layout into non-scalar layout: TyAndLayout {
    ty: std::option::Option<*mut std::ffi::c_void>,
    layout: Layout {
        fields: Arbitrary {
            offsets: [
                Size {
                    raw: 0,
                },
            ],
            memory_index: [
                0,
            ],
        },
        variants: Multiple {
            tag: Initialized {
                value: Int(
                    I64,
                    false,
                ),
                valid_range: 0..=1,
            },
            tag_encoding: Direct,
            tag_field: 0,
            variants: [
                Layout {
                    fields: Arbitrary {
                        offsets: [],
                        memory_index: [],
                    },
                    variants: Single {
                        index: 0,
                    },
                    abi: Aggregate {
                        sized: true,
                    },
                    largest_niche: None,
                    align: AbiAndPrefAlign {
                        abi: Align {
                            pow2: 0,
                        },
                        pref: Align {
                            pow2: 3,
                        },
                    },
                    size: Size {
                        raw: 8,
                    },
                },
                Layout {
                    fields: Arbitrary {
                        offsets: [
                            Size {
                                raw: 8,
                            },
                        ],
                        memory_index: [
                            0,
                        ],
                    },
                    variants: Single {
                        index: 1,
                    },
                    abi: Aggregate {
                        sized: true,
                    },
                    largest_niche: None,
                    align: AbiAndPrefAlign {
                        abi: Align {
                            pow2: 3,
                        },
                        pref: Align {
                            pow2: 3,
                        },
                    },
                    size: Size {
                        raw: 16,
                    },
                },
            ],
        },
        abi: ScalarPair(
            Initialized {
                value: Int(
                    I64,
                    false,
                ),
                valid_range: 0..=1,
            },
            Initialized {
                value: Pointer,
                valid_range: 0..=18446744073709551615,
            },
        ),
        largest_niche: Some(
            Niche {
                offset: Size {
                    raw: 0,
                },
                value: Int(
                    I64,
                    false,
                ),
                valid_range: 0..=1,
            },
        ),
        align: AbiAndPrefAlign {
            abi: Align {
                pow2: 3,
            },
            pref: Align {
                pow2: 3,
            },
        },
        size: Size {
            raw: 16,
        },
    },
} to TyAndLayout {
    ty: std::option::Option<*mut std::ffi::c_void>,
    layout: Layout {
        fields: Arbitrary {
            offsets: [
                Size {
                    raw: 8,
                },
            ],
            memory_index: [
                0,
            ],
        },
        variants: Single {
            index: 1,
        },
        abi: Aggregate {
            sized: true,
        },
        largest_niche: None,
        align: AbiAndPrefAlign {
            abi: Align {
                pow2: 3,
            },
            pref: Align {
                pow2: 3,
            },
        },
        size: Size {
            raw: 16,
        },
    },
}

Cc @oli-obk @erikdesjardins

[eddyb]

The reason is this part:

                Layout {
                    fields: Arbitrary {
                        offsets: [
                            Size { raw: 8 },
                        ],
                        memory_index: [
                            0,
                        ],
                    },
                    abi: Aggregate {
                        sized: true,
                    },

i.e. the variant has a single field at offset 8 - this cannot correspond to Abi::Scalar, which must have the scalar at offset 0.

If you could manually inject padding at the start of a regular struct, you'd see the same thing happen - any specialized Abi has to be replaced by Abi::Aggregate because of the hole at the start.
(I think -Z randomize-layout might be able to do that, you should try miri with it, heh)

[RalfJung]

@eddyb I certainly would not expect this to have Scalar layout. :D I expect it to have ScalarPair layout, since it is the result of a no-op projection from a ScalarPair type.

With the current situation, how do you propose Miri handles a situation like the following? (I have no idea how this is not a problem in codegen either.)

• load/pass around some data at ScalarPair type, represented as an immediate
• do a downcast projection, which keeps the data representation unchanged but changes the type to be an Aggregate
• now do something with that downcast data, like projecting to a field

Usually, to access a field of a type with immediate pair representation, we need the information from the ScalarPair layout about offset and size of the two fields. But we don't have that information any more. So Miri does a guess that I hope is right, but my confidence is not very high. In codegen I don't even see support for this; the following code seems to assume that all pair immediates have ScalarPair layout:

rust/compiler/rustc_codegen_ssa/src/mir/operand.rs

Line 208 in d32ce37

(OperandValue::Pair(a_llval, b_llval), Abi::ScalarPair(a, b)) => {

[RalfJung]

An alternative fix suggested by @eddyb would be to not support operand_downcast in the interpreter, and instead force an allocation. That is what codegen does.

However, if we want to be able to check that an operand is valid at its type, then we have to be able to downcast operands. I am not sure how we could avoid this. Basically operands should be closed under "looking into" the value and taking it apart -- that seems like a sensible requirement IMO, but is not possible to uphold with our current enum layouts.

[eddyb]

I do not particularly prefer the current situation with codegen, I only brought it up as an explanation for why it's generally unaffected by such inconsistencies.

It would IMO be preferable to have discriminant reads/writes and downcasts (and, eventually, field writes, not just reads) working in codegen on scalars (and pairs thereof).

So definitely don't limit/deoptimize miri on my account, though in specific details I'm not necessarily sure what the best course for action is.

---

That said, keeping the Abi of the enum for variants, even when they contain a subset of the leaves accessible through fields, seems reasonable (if nothing breaks because of it, ofc).

One could argue it's akin to the enum using Abi::ScalarPair despite its only direct field being the tag/niche (i.e. just one of the two scalars).

Taking this further, where the enum has to treat the variant field scalars it incorporates into such a ScalarPair as if they were MaybeUninit (i.e. resetting their validity range), variants could do the opposite to the tag/niche field that would show up in their ScalarPair (forcing it to a validity range only suitable for that variant).

But that last part is just speculation, and it wouldn't come in handy until variants get user-exposed types (e.g. Some<u8> being its own type, with an Abi of ScalarPair(u8 in 1..=1, u8 in 0..=255)) or something similar.