enum padding incorrectly has Scalar::Initialized layout

[RalfJung]

#94527 extended our Scalar layout with the notion of whether a scalar has to be initialized or not. Miri enforces this, and also uses it to know whether it can use a more efficient representation for scalars that have to be initialized.

However, in some situations we currently compute a Scalar::Initialized layout when it should be Scalar::Union (aka "may be uninitialized" -- maybe we should rename it to Scalar::MaybeUninit). Specifically, Option<u32> has:

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

Note that this claims that the second field must be always initialized. This is not true though, since for a None that second field is padding and hence does not have to be initialized.

This causes rust-lang/miri#2068.
@oli-obk any idea how hard this is to fix?

(To be clear, this bug only causes problems in Miri, not during regular codegen -- at least for now, since we don't [yet] tell LLVM about this "must be initialized" thing.)

[RalfJung]

Here's a stand-alone reproducer for Miri:

use std::mem::MaybeUninit;

fn main() { unsafe {
    let mut x = [MaybeUninit::<i32>::zeroed(); 3];
    // Put in a ptr into the last 8 bytes.
    x.as_mut_ptr().offset(1).cast::<&i32>().write_unaligned(&0);
    // Read first 8 bytes as an `Option<i32>`
    let c = x.as_ptr().cast::<Option<i32>>().read();
    println!("{:?}", c);
} }

This can probably be turned into a CTFE testcase as well.

[RalfJung]

FWIW one reason this went unnoticed is that we don't actually check ScalarPair stuff during validation:

rust/compiler/rustc_const_eval/src/interpret/validity.rs

Lines 822 to 827 in 38004b7

	Abi::ScalarPair { .. } | Abi::Vector { .. } => {
	// These have fields that we already visited above, so we already checked
	// all their scalar-level restrictions.
	// There is also no equivalent to `rustc_layout_scalar_valid_range_start`
	// that would make skipping them here an issue.
	}

Maybe we should have a sort of debug mode where we do that check anyway? (For normal Miri operation this sounds like a waste though.)

[RalfJung]

Ah, that testcase will actually still fail since it is still copying parts of a pointer.

A testcase that demonstrates the bug would either require us to validate ScalarPair, or it'd have to be something horrible where parts of that padding are uninit, and we'd test if the init'ed parts are preserved. (Which is not actually guaranteed, padding can reset to uninit on a typed copy like this.)

[oli-obk]

Oh fun. So this is specifically about the enum layout and not the layout of a downcasted variant, right?

[RalfJung]

Yes. I have not looked at the layout of the downcasted variants.

[RalfJung]

Variants seem to be even wilder (according to my debugging while working on #96185): the variant of a ScalarPair enum might have Aggregate layout! That's a disaster since Miri assumes that all ScalarPair values have ScalarPair layout. That seemed like a reasonable sanity check, but apparently it is not true...

Except codegen also assumes this, but somehow there it works out. Maybe it handles downcasts differently.