From 487e0275b201552756c95795c4dd4454e1fcdda2 Mon Sep 17 00:00:00 2001 From: Alice Ryhl Date: Mon, 18 Jul 2022 21:19:39 +0200 Subject: [PATCH 1/3] tokio: avoid temporary references in Link impls --- tokio/build.rs | 4 +++ tokio/src/io/driver/scheduled_io.rs | 12 ++++++-- tokio/src/macros/addr_of.rs | 48 +++++++++++++++++++++++++++++ tokio/src/macros/mod.rs | 3 ++ tokio/src/runtime/task/core.rs | 12 ++++++-- tokio/src/runtime/task/mod.rs | 3 +- tokio/src/sync/batch_semaphore.rs | 18 ++++++----- tokio/src/sync/broadcast.rs | 12 ++++++-- tokio/src/sync/notify.rs | 11 +++++-- tokio/src/time/driver/entry.rs | 11 +++++-- tokio/src/util/idle_notified_set.rs | 19 ++++++------ 11 files changed, 123 insertions(+), 30 deletions(-) create mode 100644 tokio/src/macros/addr_of.rs diff --git a/tokio/build.rs b/tokio/build.rs index 4c04a84d050..6dee6c37749 100644 --- a/tokio/build.rs +++ b/tokio/build.rs @@ -7,6 +7,10 @@ fn main() { if ac.probe_rustc_version(1, 59) { autocfg::emit("tokio_const_thread_local") } + + if !ac.probe_rustc_version(1, 51) { + autocfg::emit("tokio_no_addr_of") + } } Err(e) => { diff --git a/tokio/src/io/driver/scheduled_io.rs b/tokio/src/io/driver/scheduled_io.rs index 76f93431ba2..229fe7b1c13 100644 --- a/tokio/src/io/driver/scheduled_io.rs +++ b/tokio/src/io/driver/scheduled_io.rs @@ -66,6 +66,14 @@ cfg_io_readiness! { _p: PhantomPinned, } + generate_addr_of_methods! { + impl<> Waiter { + unsafe fn addr_of_pointers(self: NonNull) -> NonNull> { + &self.pointers + } + } + } + /// Future returned by `readiness()`. struct Readiness<'a> { scheduled_io: &'a ScheduledIo, @@ -399,8 +407,8 @@ cfg_io_readiness! { ptr } - unsafe fn pointers(mut target: NonNull) -> NonNull> { - NonNull::from(&mut target.as_mut().pointers) + unsafe fn pointers(target: NonNull) -> NonNull> { + Waiter::addr_of_pointers(target) } } diff --git a/tokio/src/macros/addr_of.rs b/tokio/src/macros/addr_of.rs new file mode 100644 index 00000000000..e6d790658d7 --- /dev/null +++ b/tokio/src/macros/addr_of.rs @@ -0,0 +1,48 @@ +#[cfg(tokio_no_addr_of)] +macro_rules! generate_addr_of_methods { + ( + impl<$($gen:ident)*> $struct_name:ty {$( + $(#[$attrs:meta])* + $vis:vis unsafe fn $fn_name:ident(self: NonNull) -> NonNull<$field_type:ty> { + &self$(.$field_name:tt)+ + } + )*} + ) => { + impl<$($gen)*> $struct_name {$( + $(#[$attrs])* + $vis unsafe fn $fn_name(me: ::core::ptr::NonNull) -> ::core::ptr::NonNull<$field_type> { + let me = me.as_ptr(); + let me_u8 = me as *mut u8; + + let field_offset = { + let me_ref = &*me; + let field_ref_u8 = (&me_ref $(.$field_name)+ ) as *const $field_type as *const u8; + field_ref_u8.offset_from(me_u8) + }; + + ::core::ptr::NonNull::new_unchecked(me_u8.offset(field_offset).cast()) + } + )*} + }; +} + +#[cfg(not(tokio_no_addr_of))] +macro_rules! generate_addr_of_methods { + ( + impl<$($gen:ident)*> $struct_name:ty {$( + $(#[$attrs:meta])* + $vis:vis unsafe fn $fn_name:ident(self: NonNull) -> NonNull<$field_type:ty> { + &self$(.$field_name:tt)+ + } + )*} + ) => { + impl<$($gen)*> $struct_name {$( + $(#[$attrs])* + $vis unsafe fn $fn_name(me: ::core::ptr::NonNull) -> ::core::ptr::NonNull<$field_type> { + let me = me.as_ptr(); + let field = ::std::ptr::addr_of_mut!((*me) $(.$field_name)+ ); + ::core::ptr::NonNull::new_unchecked(field) + } + )*} + }; +} diff --git a/tokio/src/macros/mod.rs b/tokio/src/macros/mod.rs index a1839c83052..57678c67bc9 100644 --- a/tokio/src/macros/mod.rs +++ b/tokio/src/macros/mod.rs @@ -15,6 +15,9 @@ mod ready; #[macro_use] mod thread_local; +#[macro_use] +mod addr_of; + cfg_trace! { #[macro_use] mod trace; diff --git a/tokio/src/runtime/task/core.rs b/tokio/src/runtime/task/core.rs index 548c56da3d4..a512921c124 100644 --- a/tokio/src/runtime/task/core.rs +++ b/tokio/src/runtime/task/core.rs @@ -60,7 +60,7 @@ pub(crate) struct Header { /// Task state. pub(super) state: State, - pub(super) owned: UnsafeCell>, + pub(super) owned: linked_list::Pointers
, /// Pointer to next task, used with the injection queue. pub(super) queue_next: UnsafeCell>>, @@ -86,6 +86,14 @@ pub(crate) struct Header { pub(super) id: Option, } +generate_addr_of_methods! { + impl<> Header { + pub(super) unsafe fn addr_of_owned(self: NonNull) -> NonNull> { + &self.owned + } + } +} + unsafe impl Send for Header {} unsafe impl Sync for Header {} @@ -111,7 +119,7 @@ impl Cell { Box::new(Cell { header: Header { state, - owned: UnsafeCell::new(linked_list::Pointers::new()), + owned: linked_list::Pointers::new(), queue_next: UnsafeCell::new(None), vtable: raw::vtable::(), owner_id: UnsafeCell::new(0), diff --git a/tokio/src/runtime/task/mod.rs b/tokio/src/runtime/task/mod.rs index e73b3f35a54..c2903f840e1 100644 --- a/tokio/src/runtime/task/mod.rs +++ b/tokio/src/runtime/task/mod.rs @@ -473,8 +473,7 @@ unsafe impl linked_list::Link for Task { } unsafe fn pointers(target: NonNull
) -> NonNull> { - // Not super great as it avoids some of looms checking... - NonNull::from(target.as_ref().owned.with_mut(|ptr| &mut *ptr)) + Header::addr_of_owned(target) } } diff --git a/tokio/src/sync/batch_semaphore.rs b/tokio/src/sync/batch_semaphore.rs index 4db88351d0c..23e6e2adfb2 100644 --- a/tokio/src/sync/batch_semaphore.rs +++ b/tokio/src/sync/batch_semaphore.rs @@ -112,6 +112,14 @@ struct Waiter { _p: PhantomPinned, } +generate_addr_of_methods! { + impl<> Waiter { + unsafe fn addr_of_pointers(self: NonNull) -> NonNull> { + &self.pointers + } + } +} + impl Semaphore { /// The maximum number of permits which a semaphore can hold. /// @@ -704,12 +712,6 @@ impl std::error::Error for TryAcquireError {} /// /// `Waiter` is forced to be !Unpin. unsafe impl linked_list::Link for Waiter { - // XXX: ideally, we would be able to use `Pin` here, to enforce the - // invariant that list entries may not move while in the list. However, we - // can't do this currently, as using `Pin<&'a mut Waiter>` as the `Handle` - // type would require `Semaphore` to be generic over a lifetime. We can't - // use `Pin<*mut Waiter>`, as raw pointers are `Unpin` regardless of whether - // or not they dereference to an `!Unpin` target. type Handle = NonNull; type Target = Waiter; @@ -721,7 +723,7 @@ unsafe impl linked_list::Link for Waiter { ptr } - unsafe fn pointers(mut target: NonNull) -> NonNull> { - NonNull::from(&mut target.as_mut().pointers) + unsafe fn pointers(target: NonNull) -> NonNull> { + Waiter::addr_of_pointers(target) } } diff --git a/tokio/src/sync/broadcast.rs b/tokio/src/sync/broadcast.rs index fa6f46b0a4c..69ede8f89ce 100644 --- a/tokio/src/sync/broadcast.rs +++ b/tokio/src/sync/broadcast.rs @@ -361,6 +361,14 @@ struct Waiter { _p: PhantomPinned, } +generate_addr_of_methods! { + impl<> Waiter { + unsafe fn addr_of_pointers(self: NonNull) -> NonNull> { + &self.pointers + } + } +} + struct RecvGuard<'a, T> { slot: RwLockReadGuard<'a, Slot>, } @@ -1140,8 +1148,8 @@ unsafe impl linked_list::Link for Waiter { ptr } - unsafe fn pointers(mut target: NonNull) -> NonNull> { - NonNull::from(&mut target.as_mut().pointers) + unsafe fn pointers(target: NonNull) -> NonNull> { + Waiter::addr_of_pointers(target) } } diff --git a/tokio/src/sync/notify.rs b/tokio/src/sync/notify.rs index 2af9bacae80..83bd6823694 100644 --- a/tokio/src/sync/notify.rs +++ b/tokio/src/sync/notify.rs @@ -214,7 +214,6 @@ enum NotificationType { } #[derive(Debug)] -#[repr(C)] // required by `linked_list::Link` impl struct Waiter { /// Intrusive linked-list pointers. pointers: linked_list::Pointers, @@ -229,6 +228,14 @@ struct Waiter { _p: PhantomPinned, } +generate_addr_of_methods! { + impl<> Waiter { + unsafe fn addr_of_pointers(self: NonNull) -> NonNull> { + &self.pointers + } + } +} + /// Future returned from [`Notify::notified()`]. /// /// This future is fused, so once it has completed, any future calls to poll @@ -950,7 +957,7 @@ unsafe impl linked_list::Link for Waiter { } unsafe fn pointers(target: NonNull) -> NonNull> { - target.cast() + Waiter::addr_of_pointers(target) } } diff --git a/tokio/src/time/driver/entry.rs b/tokio/src/time/driver/entry.rs index f0ea898e120..41b38dd86c9 100644 --- a/tokio/src/time/driver/entry.rs +++ b/tokio/src/time/driver/entry.rs @@ -339,6 +339,14 @@ pub(crate) struct TimerShared { _p: PhantomPinned, } +generate_addr_of_methods! { + impl<> TimerShared { + unsafe fn addr_of_pointers(self: NonNull) -> NonNull> { + &self.driver_state.0.pointers + } + } +} + impl TimerShared { pub(super) fn new() -> Self { Self { @@ -421,7 +429,6 @@ impl TimerShared { /// padded. This contains the information that the driver thread accesses most /// frequently to minimize contention. In particular, we move it away from the /// waker, as the waker is updated on every poll. -#[repr(C)] // required by `link_list::Link` impl struct TimerSharedPadded { /// A link within the doubly-linked list of timers on a particular level and /// slot. Valid only if state is equal to Registered. @@ -476,7 +483,7 @@ unsafe impl linked_list::Link for TimerShared { unsafe fn pointers( target: NonNull, ) -> NonNull> { - target.cast() + TimerShared::addr_of_pointers(target) } } diff --git a/tokio/src/util/idle_notified_set.rs b/tokio/src/util/idle_notified_set.rs index 71f3a32a853..ce8ff9e9a71 100644 --- a/tokio/src/util/idle_notified_set.rs +++ b/tokio/src/util/idle_notified_set.rs @@ -88,10 +88,6 @@ enum List { /// move it out from this entry to prevent it from getting leaked. (Since the /// two linked lists are emptied in the destructor of `IdleNotifiedSet`, the /// value should not be leaked.) -/// -/// This type is `#[repr(C)]` because its `linked_list::Link` implementation -/// requires that `pointers` is the first field. -#[repr(C)] struct ListEntry { /// The linked list pointers of the list this entry is in. pointers: linked_list::Pointers>, @@ -105,6 +101,14 @@ struct ListEntry { _pin: PhantomPinned, } +generate_addr_of_methods! { + impl ListEntry { + unsafe fn addr_of_pointers(self: NonNull) -> NonNull>> { + &self.pointers + } + } +} + // With mutable access to the `IdleNotifiedSet`, you can get mutable access to // the values. unsafe impl Send for IdleNotifiedSet {} @@ -453,11 +457,6 @@ unsafe impl linked_list::Link for ListEntry { unsafe fn pointers( target: NonNull>, ) -> NonNull>> { - // Safety: The pointers struct is the first field and ListEntry is - // `#[repr(C)]` so this cast is safe. - // - // We do this rather than doing a field access since `std::ptr::addr_of` - // is too new for our MSRV. - target.cast() + ListEntry::addr_of_pointers(target) } } From 229b80fe4cd50986ea91b7d550239a00b2e6e9b5 Mon Sep 17 00:00:00 2001 From: Alice Ryhl Date: Mon, 18 Jul 2022 21:31:05 +0200 Subject: [PATCH 2/3] Add comments --- tokio/src/macros/addr_of.rs | 31 ++++++++++++++++++------------- 1 file changed, 18 insertions(+), 13 deletions(-) diff --git a/tokio/src/macros/addr_of.rs b/tokio/src/macros/addr_of.rs index e6d790658d7..51d488972d9 100644 --- a/tokio/src/macros/addr_of.rs +++ b/tokio/src/macros/addr_of.rs @@ -1,4 +1,7 @@ -#[cfg(tokio_no_addr_of)] +//! This module defines a macro that lets you go from a raw pointer to a struct +//! to a raw pointer to a field of the struct. + +#[cfg(not(tokio_no_addr_of))] macro_rules! generate_addr_of_methods { ( impl<$($gen:ident)*> $struct_name:ty {$( @@ -12,21 +15,16 @@ macro_rules! generate_addr_of_methods { $(#[$attrs])* $vis unsafe fn $fn_name(me: ::core::ptr::NonNull) -> ::core::ptr::NonNull<$field_type> { let me = me.as_ptr(); - let me_u8 = me as *mut u8; - - let field_offset = { - let me_ref = &*me; - let field_ref_u8 = (&me_ref $(.$field_name)+ ) as *const $field_type as *const u8; - field_ref_u8.offset_from(me_u8) - }; - - ::core::ptr::NonNull::new_unchecked(me_u8.offset(field_offset).cast()) + let field = ::std::ptr::addr_of_mut!((*me) $(.$field_name)+ ); + ::core::ptr::NonNull::new_unchecked(field) } )*} }; } -#[cfg(not(tokio_no_addr_of))] +// The `addr_of_mut!` macro is only available for MSRV at least 1.51.0. This +// version of the macro uses a workaround for older versions of rustc. +#[cfg(tokio_no_addr_of)] macro_rules! generate_addr_of_methods { ( impl<$($gen:ident)*> $struct_name:ty {$( @@ -40,8 +38,15 @@ macro_rules! generate_addr_of_methods { $(#[$attrs])* $vis unsafe fn $fn_name(me: ::core::ptr::NonNull) -> ::core::ptr::NonNull<$field_type> { let me = me.as_ptr(); - let field = ::std::ptr::addr_of_mut!((*me) $(.$field_name)+ ); - ::core::ptr::NonNull::new_unchecked(field) + let me_u8 = me as *mut u8; + + let field_offset = { + let me_ref = &*me; + let field_ref_u8 = (&me_ref $(.$field_name)+ ) as *const $field_type as *const u8; + field_ref_u8.offset_from(me_u8) + }; + + ::core::ptr::NonNull::new_unchecked(me_u8.offset(field_offset).cast()) } )*} }; From b3f15a85d57ac6017b424ff9244f557721415527 Mon Sep 17 00:00:00 2001 From: Alice Ryhl Date: Mon, 18 Jul 2022 21:57:26 +0200 Subject: [PATCH 3/3] Remove comment --- tokio/src/time/driver/entry.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tokio/src/time/driver/entry.rs b/tokio/src/time/driver/entry.rs index 41b38dd86c9..eb33e9a2aa2 100644 --- a/tokio/src/time/driver/entry.rs +++ b/tokio/src/time/driver/entry.rs @@ -326,7 +326,7 @@ pub(super) type EntryList = crate::util::linked_list::LinkedList,