Preserve same vtable pointer when cloning raw waker, to fix Waker::will_wake #121622
Conversation
Currently fails:
---- task::test_waker_will_wake_clone stdout ----
thread 'task::test_waker_will_wake_clone' panicked at library/alloc/tests/task.rs:17:5:
assertion failed: waker.will_wake(&clone)
rustbot
added
S-waiting-on-review
|
Promoteds are generated in each CGU (code generation unit) they are referenced from. For optimized builds with multiple codegen units where vtable function is inlined, the promoted end up duplicated and the original test case still fails. I think a more robust approach is to mark clone_waker as always inline, so that it is always generated in the same CGU as the vtable (within a single CGU structurally equivalent constants are deduplicated at LLVM IR code generation time). Without generic statics we don't really have a great solution to this kind of problems :-(. |
|
@tmiasko good call, I confirmed that works. |
|
I am uncertain whether we are better off with this change (always inline clone_waker) or with reverting #119863. Are there cases where If the deduplication of structurally equivalent constants is done by rustc at LLVM IR code generation time while the deduplication of generic function instantiations is done at link time then potentially yes, right? |
|
Thinking about it more, I think this PR is optimal, at least for the LLVM backend. There is no way that cloning a RawWaker returned by raw_waker() would produce a clone that has a different vtable pointer than the original. Not sure about Miri, or any non-LLVM backend, but that's what the "best-effort basis" part of |
|
Makes sense to me. @bors r+ |
bors
added
S-waiting-on-bors
Rollup of 5 pull requests Successful merges: - rust-lang#120761 (Add initial support for DataFlowSanitizer) - rust-lang#121622 (Preserve same vtable pointer when cloning raw waker, to fix Waker::will_wake) - rust-lang#121716 (match lowering: Lower bindings in a predictable order) - rust-lang#121731 (Now that inlining, mir validation and const eval all use reveal-all, we won't be constraining hidden types here anymore) - rust-lang#121841 (`f16` and `f128` step 2: intrinsics) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#121622 - dtolnay:wake, r=cuviper Preserve same vtable pointer when cloning raw waker, to fix Waker::will_wake Fixes rust-lang#121600. As `@jkarneges` identified in rust-lang#121600 (comment), the issue is two different const promotions produce two statics at different addresses, which may or may not later be deduplicated by the linker (in this case not). Prior to rust-lang#119863, the content of the statics was compared, and they were equal. After, the address of the statics are compared and they are not equal. It is documented that `will_wake` _"works on a best-effort basis, and may return false even when the Wakers would awaken the same task"_ so this PR fixes a quality-of-implementation issue, not a correctness issue.
|
I tested this in rust-lang/futures-rs#2834, but this seems to only work in debug build. |
will_wake tests fail on Miri and that is expected Follow-up to rust-lang#121622 r? `@cuviper` `@dtolnay`
will_wake tests fail on Miri and that is expected Follow-up to rust-lang#121622 r? ``@cuviper`` ``@dtolnay``
will_wake tests fail on Miri and that is expected Follow-up to rust-lang#121622 r? ```@cuviper``` ```@dtolnay```
Rollup merge of rust-lang#122016 - RalfJung:will_wake, r=dtolnay will_wake tests fail on Miri and that is expected Follow-up to rust-lang#121622 r? ```@cuviper``` ```@dtolnay```
dtolnay
added
A-async-await
Fixes #121600.
As @jkarneges identified in #121600 (comment), the issue is two different const promotions produce two statics at different addresses, which may or may not later be deduplicated by the linker (in this case not).
Prior to #119863, the content of the statics was compared, and they were equal. After, the address of the statics are compared and they are not equal.
It is documented that
will_wake"works on a best-effort basis, and may return false even when the Wakers would awaken the same task" so this PR fixes a quality-of-implementation issue, not a correctness issue.