UnsafePinned: implement coroutine lowering changes

[b-naber]

View all comments

Implements the lowering part for #125735

This introduces a visitor that looks for self-referential fields in the coroutine struct and wraps all of these types into UnsafePinned. I've also included a commit where we change the condition for using noalias. That change is based on my understanding of the RFC, but is likely wrong given the change in #151365. I've included it for now to further discuss this.

cc @RalfJung

[rustbot]

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

[rustbot]

r? @mati865

rustbot has assigned @mati865.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: compiler, mir, mir-opt
• compiler, mir, mir-opt expanded to 68 candidates
• Random selection from 15 candidates

[mati865]

I'm not a good fit for this code, perhaps r? @RalfJung ?

Feel free to reroll

[rustbot]

RalfJung is not on the review rotation at the moment.
They may take a while to respond.

[RalfJung]

Definitely not, I don't know the coroutine lowering code at all.

@rustbot reroll

[b-naber]

@cjgillot Could you maybe review this?

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[b-naber]

@rustbot author

[b-naber]

I am very confused. I didn't expect anything nearly that complicated.^^ The compiler can already figure out that a generator has a self-referential borrow and hence cannot be gen move. Doesn't that same analysis let us figure out which fields could be borrowed across a yield point, and hence should be UnsafePinned?

(I just realized I have no idea what any of that does with raw pointers.^^)

Sorry for the confusion. I think you are right, the MaybeBorrowedLocals analysis should be sufficient to infer all self-referential fields. If a local is kept alive due to an outstanding borrow then it must be a self-referential field.

That analyis also takes into account raw pointers. We only take the information from that analysis into account when calculating the live_locals if the coroutine is !movable though. I haven't investigated the segfault example though and don't know whether the reason why that segfaults is actually related to this.

[RalfJung]

Movable coroutines indeed should not get any UnsafePinned.

[RalfJung]

That test failing is a great sign! Quoting from the test description

//! FIXME: This test should pass! However, `async fn` does not yet use `UnsafePinned`.
//! This is a regression test for <https://github.com/rust-lang/rust/issues/137750>:
//! `UnsafePinned` must include the effects of `UnsafeCell`.

[rustbot]

The Miri subtree was changed

cc @rust-lang/miri

[RalfJung]

This comment does nothing, please remove it. (Why did you even add this? Even in the rustc ui test suite this would do nothing. It's spelled // @run-pass there. But the Miri test suite is a little different.)

[b-naber]

There's another test that uses this: https://github.com/rust-lang/rust/blob/main/src/tools/miri/tests/pass/packed-struct-dyn-trait.rs, which is why I used this.

[RalfJung]

Ah, not sure how that snuck in, thanks for pointing that out. But you'll notice most pass tests don't have such an annotation.

[rustbot]

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

[b-naber]

@rustbot ready