Miri engine: stronger type-based sanity check for assignments

[RalfJung]

r? @oli-obk @eddyb
Fixes #70405

That issue says

be sure to also add appropriate mutability checks to the patterns (mutable for the source, immutable for the dest)

I decided not to do that because I see no good reason to do it. The engine does not care either way, the assignment will happen correctly.

[eddyb]

Are there remaining layout.layout in miri? Since that was what we were looking at.

[RalfJung]

There's one:

rust/src/librustc_mir/interpret/operand.rs

Line 219 in 8045865

layout.layout, layout2.layout,

That is verifying that the given layout matches the one we would have computed, so I think here comparing layouts for equality actually makes sense.

[eddyb]

Looks like that's all "destination layout" type stuff? i.e. where you might have a layout from knowing where a value might be written?
I think it should be better named/described as such, and should also use a type check.

[RalfJung]

I think it should be better named/described as such

I mean the function is called from_known_layout and documented as

// Use the existing layout if given (but sanity check in debug mode),
// or compute the layout.

That seems pretty clear?

[RalfJung]

and should also use a type check.

In fact I wonder why this does not just compare the types... let me try that (but it might have the same potential mismatches as assignments).

[eddyb]

"existing layout" doesn't explain that it's a destination layout, whereas the one that might be computed or compared against is a source one.

If we end up doing subtyping checks, the direction matters.

[RalfJung]

I don't think the callers here uniformly make one source and one destination.

The check we do here should be symmetric. And indeed the actual relation we care about is not subtyping, it is layout compatibility -- and that relation is symmetric. Subtyping is just an approximation of that (and it needs more properties, which makes it asymmetric).

I would find it highly suspicious if mir_assign_valid_types ended up being asymmetric.

[RalfJung]

That CI failure is interesting, it requires actually building the testcase, not just checking it... then we get the following error:

thread 'rustc' panicked at 'type mismatch when copying!
src: for<'r, 's> fn(u32, &'r str, &'s std::cell::RefCell<std::vec::Vec<u32>>),
dest: for<'r> fn(u32, &'r str, &std::cell::RefCell<std::vec::Vec<u32>>)', src/librustc_mir/interpret/place.rs:883:9

Looks like we need to also blanket-accept functions with different signatures? All function pointers have the same layout, so that should be fine.

[eddyb]

Oh, dear, higher-ranked subtyping. I forgot we do that (cc @nikomatsakis).
I think during codegen it causes a lot of problems and nowadays we just automatically cast types when they don't match...

Keep in mind you can wrap that fn type in any number of types, as long as they're not invariant, it will still cause issues.

I wonder if there's a way to erase bound lifetimes like that, deeply, which is generally unsound (e.g. they might be inside projections, which would change the resulting type), but it would make comparison without ty::relate, possible.

[RalfJung]

Keep in mind you can wrap that fn type in any number of types, as long as they're not invariant, it will still cause issues.

Tests seem to pass when I just add FnPtr to the pairs that mir_assign_valid_types accepts, but you are saying that is not sufficient?

Maybe comparing the layout was not such a silly idea after all?^^

[eddyb]

Tests seem to pass when I just add FnPtr to the pairs that mir_assign_valid_types accepts, but you are saying that is not sufficient?

There's presumably no tests for those same types wrapped in tuples, Option, etc.

Maybe comparing the layout was not such a silly idea after all?^^

Just because it's much much shallower, so it's barely a sanity check at all.

[RalfJung]

There's presumably no tests for those same types wrapped in tuples, Option, etc.

So you are saying this could happen with arbitrarily complex types, not just Abi::Scalar?

[eddyb]

So you are saying this could happen with arbitrarily complex types, not just Abi::Scalar?

Yes, assignment (or passing values to functions) is asymmetric in Rust, there's a subtyping relationship, it implicitly "converts" types, deeply (as deep as variance lets it). I wish this was an explicit coercion in MIR, but then again you see what happened with &mut T -> &T: it became implicit.

[RalfJung]

Yes, assignment (or passing values to functions) is asymmetric in Rust, there's a subtyping relationship

For passing values to functions, Miri is fine -- it uses a transmute-safe copy there, because caller and callee signature might differ.

Re: subtyping, I agree the type system check should be asymmetric like it is. But the run-time check in Miri has no good reason to be asymmetric, IMO. At type checking time we are converting types aka sets of values; at run-time we are converting an individual value; the former have an asymmetric relation but the latter a symmetric one.

[oli-obk]

I think we could erase regions and try comparing the result again.

Also I do wonder about what @eddyb said, so we may end up in the false arm if the assignment is between two instances of struct Foo<T>(T); where T is used for two function pointers that only differ between their HKL.

[eddyb]

You can't trivially erase lifetimes bound by higher-ranked types (I don't think HKL means anything, there is no 'Foo<'a, 'b>).

I should come up with an example showing lifetime depending on the shape of bound lifetimes.

[eddyb]

Oh that was trivial, this example shows different sizes based on lifetimes (playground):

trait Trait {
    type Assoc;
}
struct Foo<T: Trait>(T::Assoc);

impl Trait for fn(&'static ()) {
    type Assoc = u8;
}

impl Trait for for<'a> fn(&'a ()) {
    type Assoc = u16;
}

fn main() {
    use std::mem::size_of;
    
    dbg!(size_of::<Foo<fn(&'static ())>>());
    dbg!(size_of::<Foo<for<'b> fn(&'b ())>>());
}

[RalfJung]

Oh that was trivial, this example shows different sizes based on lifetimes

Well yeah, with associated types that is not too surprising.
The question is, will any of these more interesting kinds of subtyping actually be exploited by MIR?

The one place where this layout equality check was not just a sanity check, but crucial, is copy_op_transmute, where we need to reliably detect all cases that actually need to go through memory for their transmute, and cannot be done just by swapping out the layout part of an ImmTy. And since ImmTy can only represent Scalar and ScalarPair, I am pretty sure the old check reliably caught such problems. Since we also assert the size to be equal, really the only possible problem is ScalarPair: transmuting an (u64, u64) to (u64, u32) requires appropriate truncation of the second component, but both do have the same size, so reusing the same Immediate would give a wrong result.

So, as far as I can tell, the old code her isn't wrong, even though the sanity check (for the cases where it really just is a sanity check) was pretty weak. The new sanity check, as you say, is wrong (but for copy_op_transmute it should still be correct). As as-is, the PR program cannot land.

However, given this function type problem, is there even a reasonable type-based way to correctly implement the copy_op_transmute check?

[eddyb]

You could do a deep erasure and gate it on variance. Because if there's anything involving associated types, the relevant parameters are marked as invariant (e.g. Foo<T> is invariant wrt T).

You won't be able to just ignore the invariant parts though, you'd still need to anonymize the lifetimes, i.e. Foo<for<'b> fn(&'b ())> and Foo<for<'c> fn(&'c ())> has to turn into the same type (roughly Foo<for<'0> fn(&'0 ())>).

EDIT: @nikomatsakis has just informed me that we already anonymize late-bound regions when erasing all the other ones:

rust/src/librustc_middle/ty/erase_regions.rs

Line 50 in a80ec3b

let u = self.tcx.anonymize_late_bound_regions(t);

---

Most of the tools are already there, there's just no single TypeFolder that does all of this.

Actually, I wonder if codegen (and const-eval and layout, all of which use the regular lifetime-erasing functionality) should do something similar, might solve the problems we've seen and papered over (it would require though that nothing that could potentially reach the trait system is fully erased, only anonymized). cc @rust-lang/compiler

[RalfJung]

sigh these import changes are a huge pain to rebase (and rustfmt makes it worse and it makes the diffs bigger). This is the second time in 4 days.

Since it doesn't seem like we are actually going to solve this problem here, I am going to turn this PR into a mere refactoring without functional changes.

[RalfJung]

@oli-obk @eddyb this PR is now mostly just a refactoring, moving the "assignment compat" check into a separate method but still mostly based on layout equality testing. That seems to be the best we can do right now.

[eddyb]

Thanks, the src.ty == dest.ty fast path + limiting layout equality to Scalar and ScalarPair makes me quite happy!

If we end up doing subtyping-aware erasure, we should do it uniformly so codegen benefits as well.

@bors r+

[bors]

📌 Commit 343b3f0 has been approved by eddyb