UnsafePinned: implement coroutine lowering changes#152946
UnsafePinned: implement coroutine lowering changes#152946b-naber wants to merge 4 commits intorust-lang:mainfrom
Conversation
|
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt |
rustbot
added
S-waiting-on-review
|
r? @mati865 rustbot has assigned @mati865. Use Why was this reviewer chosen?The reviewer was selected based on:
|
This comment has been minimized.
This comment has been minimized.
rust-cloud-vms
bot
force-pushed
the
coroutine-unsafe-pinned
branch
from
50482bd to
b226e46
Compare
compiler/rustc_ty_utils/src/abi.rs
Outdated
| PointerKind::SharedRef { frozen } => frozen, | ||
| PointerKind::MutableRef { unpin } => unpin && noalias_mut_ref, | ||
| PointerKind::MutableRef { unpin, unsafe_unpin } => { | ||
| (unpin || unsafe_unpin) && noalias_mut_ref |
There was a problem hiding this comment.
This looks very wrong, it means a safe impl Unpin for X can add noalias to a type that contains an UnsafePinned!
There was a problem hiding this comment.
I am very surprised that the codegen tests I added are not catching this. Specifically these two
// CHECK: @trait_box_pin1(ptr noundef nonnull align 1{{( %0)?}}, {{.+}} noalias noundef readonly align {{.*}} dereferenceable({{.*}}){{( %1)?}})
#[no_mangle]
pub fn trait_box_pin1(_: Box<dyn Drop + Unpin>) {}
// CHECK: @trait_box_pin2(ptr noundef nonnull align 1{{( %0)?}}, {{.+}} noalias noundef readonly align {{.*}} dereferenceable({{.*}}){{( %1)?}})
#[no_mangle]
pub fn trait_box_pin2(_: Box<dyn Drop + UnsafeUnpin>) {}
They have one of the traits so shouldn't this check add the noalias?
There was a problem hiding this comment.
Ah, you only changed the logic for &mut, not for Box. Argh. Clearly we need more tests.^^
There was a problem hiding this comment.
This looks very wrong, it means a safe
impl Unpin for Xcan addnoaliasto a type that contains anUnsafePinned!
oh yes ^^. Really it should just be unsafe_unpin, right? But we cannot do this because of backward compatibility issues? It's kind of unfortunate that we cannot use noalias on the coroutine struct argument because of this. Can you say how much using noalias for the coroutine struct would give us in terms of optimization potential? Would it maybe make sense to use an internal type analogous to UnsafePinned for the coroutine struct?
There was a problem hiding this comment.
Eventually, it should just be unsafe_unpin, yes. For now, we keep the Unpin hack alive, to avoid Miri going red everywhere at the same time.
Why did you change this code at all? What's wrong with unpin && noalias_mut_ref?
There was a problem hiding this comment.
Ok that makes sense.
Initially I was unaware of #151365 and implemented those changes with only unsafe_unpinned && noalias_mut_ref as the condition. When rebasing I saw your commit and was confused, because I thought that the goal of introducing UnsafePinned in coroutines was to allow using noalias more liberally there, and then included the condition in this PR without really thinking things through.
There was a problem hiding this comment.
The goal is to remove the Unpin hack, but we have to take this one step at a time. We're transitioning from "noalias if A" to "noalias if B" via "noalias if A && B" to ensure that the intermediate state is sound for both old and new code. This PR will move a lot of code to the new world. :)
…rk-Simulacrum extend unpin noalias tests to cover mutable references rust-lang#152946 made a change to the logic for this attribute that the test should have flagged as problematic -- but the test only checked `Box`, not `&mut`, and those have independent code paths. So extend the test to also cover `&mut`. @b-naber would be nice if you could confirm that the added tests do fail with your PR.
Rollup merge of #152970 - RalfJung:unpin-noalias-tests, r=Mark-Simulacrum extend unpin noalias tests to cover mutable references #152946 made a change to the logic for this attribute that the test should have flagged as problematic -- but the test only checked `Box`, not `&mut`, and those have independent code paths. So extend the test to also cover `&mut`. @b-naber would be nice if you could confirm that the added tests do fail with your PR.
|
I'm not a good fit for this code, perhaps r? @RalfJung ? Feel free to reroll |
|
|
|
Definitely not, I don't know the coroutine lowering code at all. @rustbot reroll |
|
@cjgillot Could you maybe review this? |
rust-cloud-vms
bot
force-pushed
the
coroutine-unsafe-pinned
branch
from
b226e46 to
d8b3c3d
Compare
|
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. |
This comment has been minimized.
This comment has been minimized.
rust-cloud-vms
bot
force-pushed
the
coroutine-unsafe-pinned
branch
from
d8b3c3d to
3d40415
Compare
…crum extend unpin noalias tests to cover mutable references rust-lang/rust#152946 made a change to the logic for this attribute that the test should have flagged as problematic -- but the test only checked `Box`, not `&mut`, and those have independent code paths. So extend the test to also cover `&mut`. @b-naber would be nice if you could confirm that the added tests do fail with your PR.
7 participants
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