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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions library/alloc/src/task.rs
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,15 @@ impl<W: Wake + Send + Sync + 'static> From<Arc<W>> for RawWaker {
#[inline(always)]
fn raw_waker<W: Wake + Send + Sync + 'static>(waker: Arc<W>) -> RawWaker {
// Increment the reference count of the arc to clone it.
//
// The #[inline(always)] is to ensure that raw_waker and clone_waker are
// always generated in the same code generation unit as one another, and
// therefore that the structurally identical const-promoted RawWakerVTable
// within both functions is deduplicated at LLVM IR code generation time.
// This allows optimizing Waker::will_wake to a single pointer comparison of
// the vtable pointers, rather than comparing all four function pointers
// within the vtables.
#[inline(always)]
unsafe fn clone_waker<W: Wake + Send + Sync + 'static>(waker: *const ()) -> RawWaker {
unsafe { Arc::increment_strong_count(waker as *const W) };
RawWaker::new(
Expand Down Expand Up @@ -304,6 +313,10 @@ impl<W: LocalWake + 'static> From<Rc<W>> for RawWaker {
#[inline(always)]
fn local_raw_waker<W: LocalWake + 'static>(waker: Rc<W>) -> RawWaker {
// Increment the reference count of the Rc to clone it.
//
// Refer to the comment on raw_waker's clone_waker regarding why this is
// always inline.
#[inline(always)]
unsafe fn clone_waker<W: LocalWake + 'static>(waker: *const ()) -> RawWaker {
unsafe { Rc::increment_strong_count(waker as *const W) };
RawWaker::new(
Expand Down
2 changes: 2 additions & 0 deletions library/alloc/tests/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
#![feature(thin_box)]
#![feature(strict_provenance)]
#![feature(drain_keep_rest)]
#![feature(local_waker)]
#![allow(internal_features)]
#![deny(fuzzy_provenance_casts)]
#![deny(unsafe_op_in_unsafe_fn)]
Expand All @@ -62,6 +63,7 @@ mod rc;
mod slice;
mod str;
mod string;
mod task;
mod thin_box;
mod vec;
mod vec_deque;
Expand Down
34 changes: 34 additions & 0 deletions library/alloc/tests/task.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
use alloc::rc::Rc;
use alloc::sync::Arc;
use alloc::task::{LocalWake, Wake};
use core::task::{LocalWaker, Waker};

#[test]
fn test_waker_will_wake_clone() {
struct NoopWaker;

impl Wake for NoopWaker {
fn wake(self: Arc<Self>) {}
}

let waker = Waker::from(Arc::new(NoopWaker));
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.

}

#[test]
fn test_local_waker_will_wake_clone() {
struct NoopWaker;

impl LocalWake for NoopWaker {
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

let clone = waker.clone();

assert!(waker.will_wake(&clone));
assert!(clone.will_wake(&waker));
}
Loading