Remove an unnecessary restriction in dest_prop

[JakobDegen]

I had asked about this on Zulip but didn't receive a response, so putting up this PR that makes the change I think we can. If it turns out that this is wrong, hopefully I'll find out here. Reposting my Zulip comment:

Not sure what channel to put this into, so using this as a fallback. The dest prop MIR opt has this comment:

//!   Subtle case: If `dest` is a, or projects through a union, then we have to make sure that there
//!   remains an assignment to it, since that sets the "active field" of the union. But if `src` is
//!   a ZST, it might not be initialized, so there might not be any use of it before the assignment,
//!   and performing the optimization would simply delete the assignment, leaving `dest`
//!   uninitialized.

In particular, the claim seems to be that we can't take

x = ();
y.field = x;

where y is a union having field: () as one of its variants, and optimize the entire thing away (assuming x is unused otherwise). As far as I know though, Rust unions don't have active fields. Is this comment correct and am I missing something? Is there a worry about this interacting poorly with FFI code/C unions/LTO or something?

This PR just removes that comment and the associated code. Also it fixes one unrelated comment that did not match the code it was commenting on.

r? rust-lang/mir-opt

[oli-obk]

A few things:

• we should test dest prop through unions, especially around type-changing uses and multiple assignments on different fields
• we should ask the original author of that comment whether they remember more details
• won't this allow setting one union field x, reading x once and another time reading y, and dest prop will eliminate the assignment to x and directly assign to the place where it's read? Thus leaving y uninitialized, even though it should not be?

[JakobDegen]

@oli-obk :

• Yes, this is a good point. I am planning to work on getting dest prop closer to a state where it can be enabled by default in the coming days/weeks, so I can add such tests then. I could also add them with this PR, but I also worry that whatever I end up changing will just turn out to break the tests, so I'd prefer to hold off on the tests until after I figure out what needs to changing. (Maybe a good middle ground would be to open an issue documenting this need? I can do that if so)
• @jonas-schievink thoughts on this?
• I don't think this is a concern, because the pass currently does not optimize out assignments that have a source place that has any projections (code). If this were to be added (I have no plans to do so), this is indeed something to think about, but it's also not unique to unions. Enums basically have the same problem. In that case, it's more subtle, because afaik it's not possible to define an enum with a layout that statically guarantees you can do some type punning through it, but it should definitely be possible for unsafe code to use addr_of! or whatever to compute the layout that rustc chose, and then do whatever trickery with it that it wants.

[JakobDegen]

By the way, a useful example to illustrate the idea behind this PR is the following:

// #[repr(C)]
union Foo {
    a: (),
    _b: u32,
}

fn main() {
    unsafe {
        let f: Foo = std::mem::MaybeUninit::uninit().assume_init();
        dbg!(f.a)
    }
}

This compiles, runs, and passes MIRI (with all the hard mode flags enabled) today both with the #[repr(C)] commented and with it uncommented. I believe that is supposed to be the case. However, it may also be that the compiler more conservative around this given that almost nothing here is normative. In that case though, I would still encourage changing the documentation to say something to the extent of "we don't optimize anything that touches unions, because this requires more complicated justification in terms of rustc's memory model."

[oli-obk]

Ok, that makes sense, thanks! So I guess imo we should just keep the test you removed to demonstrate how dest_prop changes this union field write.

[JakobDegen]

I've readded the test. One thing I wanted to ask also: There is an assert in the code indicating that self-assignment is UB. This seems to be specific to MIR, since obviously it's possible to self-assign in Rust. What is the benefit of this being UB?

[jonas-schievink]

As far as I know though, Rust unions don't have active fields.

I'm not sure if this has been officially defined, but AFAIK they have active fields just like C unions, and Miri will check this, which I believe is what made me add this restriction.

The problem is not with code like

x = ();
some_union.field = x;

because dest_prop will just turn that into

some_union.field = ();
nop;

but with having just the union field write without the preceding initialization of x, which is a thing that at least used to happen when I wrote this (because arguably ZSTs could always be considered initialized).

If those conditions are met, what will happen is that dest_prop will remove the only store to the union, which leaves it with no active field, so future reads from any union fields are detected by Miri as UB.

[oli-obk]

and Miri will check this

Hmm... I don't think we know about active fields at all, at least not explicitly. A ZST write will not actually do anything in miri, so I don't see how removing it could affect anything.

That said, maybe it should be brought up with the UCG working group?

[oli-obk]

interesting that we're now not marking the _1 local as live anymore, but using it right below. Maybe that is what would be causing miri to appear to care about unions being not assigned to?

[JakobDegen]

Ah, I didn't catch this when I saw the test. Yeah, that seems like a bug. Let me try and see if there's an easy fix for this

[JakobDegen]

@jonas-schievink the example I posted above should show that this doesn't seem to be the case, at least right now. Reading a () out of a union is DB (at least according to MIRI) even when the union is entirely uninitialized (and so there cannot be an active field). Also, I just found that it is said rather clearly here: https://doc.rust-lang.org/stable/reference/items/unions.html#reading-and-writing-union-fields

Unions have no notion of an "active field". Instead, every union access just interprets the storage at the type of the field used for the access.

[jonas-schievink]

Ah, wait, it might have been that Miri complained about the whole union not having been initialized (since it is never written to), not about the wrong variant being used

[JakobDegen]

Even then though, the same thing happens in my example above where I create and read from an uninitialized union just fine

[oli-obk]

Locals without any storage markers whatsoever are considered always live. I was only wondering why the opt removes the storage markers, it's not incorrect to do so

[jonas-schievink]

Storage markers are deleted because I didn't write an algorithm for merging them, since that seemed pretty nontrivial. Also, there were talks about inferring them automatically, which would remove the need to care about them.

[oli-obk]

StorageDead is needed if any references are taken. Because the borrow may outlive the last use of the local. But for dest prop, borrows are already stopping the opt, so that's indeed ok.

[oli-obk]

@bors r+ rollup

[bors]

📌 Commit 57c4163 has been approved by oli-obk