Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Preserve same vtable pointer when cloning raw waker, to fix Waker::will_wake #121622

Merged
merged 2 commits into from
Mar 3, 2024

Conversation

dtolnay
Copy link
Member

@dtolnay dtolnay commented Feb 26, 2024

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.

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
Copy link
Collaborator

rustbot commented Feb 26, 2024

r? @cuviper

rustbot has assigned @cuviper.
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

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Feb 26, 2024
@tmiasko
Copy link
Contributor

tmiasko commented Feb 26, 2024

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 :-(.

@dtolnay
Copy link
Member Author

dtolnay commented Feb 27, 2024

@tmiasko good call, I confirmed that works.

@dtolnay
Copy link
Member Author

dtolnay commented Feb 27, 2024

I am uncertain whether we are better off with this change (always inline clone_waker) or with reverting #119863.

Are there cases where Waker::will_wake will return true prior to #119863, and will return false after this PR? In other words, cases where the program would end up with multiple identical copies of RawWakerVTable::new(..., wake::<W>, ...) all referring to the same instantiation of wake::<W> etc.

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?

@dtolnay
Copy link
Member Author

dtolnay commented Feb 27, 2024

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 will_wake is for.

@cuviper
Copy link
Member

cuviper commented Mar 1, 2024

Makes sense to me.

@bors r+

@bors
Copy link
Contributor

bors commented Mar 1, 2024

📌 Commit db535ba has been approved by cuviper

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 1, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 2, 2024
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
@bors bors merged commit b0ca9b5 into rust-lang:master Mar 3, 2024
11 checks passed
@rustbot rustbot added this to the 1.78.0 milestone Mar 3, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Mar 3, 2024
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.
let clone = waker.clone();

assert!(waker.will_wake(&clone));
assert!(clone.will_wake(&waker));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These tests are testing things which are not guaranteed by the language, they just happen to be what rustc currently does. In particular, these tests fail in Miri.

Not sure if that is intentional; there's no comment here indicating such an intent.

@taiki-e
Copy link
Member

taiki-e commented Mar 4, 2024

I tested this in rust-lang/futures-rs#2834, but this seems to only work in debug build.
In release mode, a test that passed before #119863 now fails. https://github.com/rust-lang/futures-rs/actions/runs/8129901894/job/22233663120?pr=2834

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 5, 2024
will_wake tests fail on Miri and that is expected

Follow-up to rust-lang#121622
r? `@cuviper` `@dtolnay`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 5, 2024
will_wake tests fail on Miri and that is expected

Follow-up to rust-lang#121622
r? ``@cuviper`` ``@dtolnay``
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 5, 2024
will_wake tests fail on Miri and that is expected

Follow-up to rust-lang#121622
r? ```@cuviper``` ```@dtolnay```
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Mar 6, 2024
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```
fn wake(self: Rc<Self>) {}
}

let waker = LocalWaker::from(Rc::new(NoopWaker));
Copy link
Member

@RalfJung RalfJung Mar 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More problems with this test... it seems to have a memory leak. Is that expected?

0.107460   error: memory leaked: alloc3172879 (Rust heap, size: 16, align: 8), allocated here:
  0.000050      --> /home/runner/work/miri-test-libstd/miri-test-libstd/rust-src-patched/library/alloc/src/alloc.rs:100:9
  0.000009       |
  0.000007   100 |         __rust_alloc(layout.size(), layout.align())
  0.000006       |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  0.000006       |
  0.000005       = note: BACKTRACE:
  0.000006       = note: inside `std::alloc::alloc` at /home/runner/work/miri-test-libstd/miri-test-libstd/rust-src-patched/library/alloc/src/alloc.rs:100:9: 100:52
  0.000005       = note: inside `std::alloc::Global::alloc_impl` at /home/runner/work/miri-test-libstd/miri-test-libstd/rust-src-patched/library/alloc/src/alloc.rs:183:73: 183:86
  0.000007       = note: inside `<std::alloc::Global as std::alloc::Allocator>::allocate` at /home/runner/work/miri-test-libstd/miri-test-libstd/rust-src-patched/library/alloc/src/alloc.rs:243:9: 243:39
  0.000011       = note: inside `alloc::alloc::exchange_malloc` at /home/runner/work/miri-test-libstd/miri-test-libstd/rust-src-patched/library/alloc/src/alloc.rs:332:11: 332:34
  0.000011       = note: inside `std::boxed::Box::<std::rc::RcBox<task::test_local_waker_will_wake_clone::NoopWaker>>::new` at /home/runner/work/miri-test-libstd/miri-test-libstd/rust-src-patched/library/alloc/src/boxed.rs:218:9: 218:20
  0.000010       = note: inside `std::rc::Rc::<task::test_local_waker_will_wake_clone::NoopWaker>::new` at /home/runner/work/miri-test-libstd/miri-test-libstd/rust-src-patched/library/alloc/src/rc.rs:398:27: 398:94
  0.000011   note: inside `task::test_local_waker_will_wake_clone`
  0.000010      --> alloc_miri_test/../library/alloc/tests/task.rs:31:34
  0.000009       |
  0.000009   31  |     let waker = LocalWaker::from(Rc::new(NoopWaker));
  0.000009       |                                  ^^^^^^^^^^^^^^^^^^

Any idea how to fix that?

This reproduces in a stand-alone test (run it in Miri to see the leak).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Filed as #122180

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Waker::will_wake() broken on nightly
7 participants