From ba4d9e6f43f265ff49b66b8d24db33f7765118c9 Mon Sep 17 00:00:00 2001 From: Taiki Endo Date: Sat, 22 Jun 2024 05:27:12 +0900 Subject: [PATCH] util: Reflect recent upstream changes on Arc and Wake --- .github/.cspell/project-dictionary.txt | 2 + portable-atomic-util/src/arc.rs | 76 +++++++++++++------------- portable-atomic-util/src/lib.rs | 1 + portable-atomic-util/src/task.rs | 19 +++++-- portable-atomic-util/tests/arc.rs | 25 ++++----- 5 files changed, 68 insertions(+), 55 deletions(-) diff --git a/.github/.cspell/project-dictionary.txt b/.github/.cspell/project-dictionary.txt index f541bc02..d3c35c1c 100644 --- a/.github/.cspell/project-dictionary.txt +++ b/.github/.cspell/project-dictionary.txt @@ -162,6 +162,8 @@ versatilepb virt vmlinux vmovdqa +vtable +vtables wokwi xadd xchg diff --git a/portable-atomic-util/src/arc.rs b/portable-atomic-util/src/arc.rs index b10f70c1..2567055f 100644 --- a/portable-atomic-util/src/arc.rs +++ b/portable-atomic-util/src/arc.rs @@ -2,15 +2,14 @@ // This module is based on alloc::sync::Arc. // -// The code has been adjusted to work with stable Rust and to -// avoid UBs (https://github.com/rust-lang/rust/issues/119241). +// The code has been adjusted to work with stable Rust. // -// Source: https://github.com/rust-lang/rust/blob/5151b8c42712c473e7da56e213926b929d0212ef/library/alloc/src/sync.rs. +// Source: https://github.com/rust-lang/rust/blob/893f95f1f7c663c67c884120003b3bf21b0af61a/library/alloc/src/sync.rs. // // Copyright & License of the original code: -// - https://github.com/rust-lang/rust/blob/5151b8c42712c473e7da56e213926b929d0212ef/COPYRIGHT -// - https://github.com/rust-lang/rust/blob/5151b8c42712c473e7da56e213926b929d0212ef/LICENSE-APACHE -// - https://github.com/rust-lang/rust/blob/5151b8c42712c473e7da56e213926b929d0212ef/LICENSE-MIT +// - https://github.com/rust-lang/rust/blob/893f95f1f7c663c67c884120003b3bf21b0af61a/COPYRIGHT +// - https://github.com/rust-lang/rust/blob/893f95f1f7c663c67c884120003b3bf21b0af61a/LICENSE-APACHE +// - https://github.com/rust-lang/rust/blob/893f95f1f7c663c67c884120003b3bf21b0af61a/LICENSE-MIT #![allow(clippy::must_use_candidate)] // align to alloc::sync::Arc #![allow(clippy::undocumented_unsafe_blocks)] // TODO: most of the unsafe codes were inherited from alloc::sync::Arc @@ -357,15 +356,18 @@ impl Arc { /// This will succeed even if there are outstanding weak references. /// /// It is strongly recommended to use [`Arc::into_inner`] instead if you don't - /// want to keep the `Arc` in the [`Err`] case. - /// Immediately dropping the [`Err`] payload, like in the expression - /// `Arc::try_unwrap(this).ok()`, can still cause the strong count to - /// drop to zero and the inner value of the `Arc` to be dropped: - /// For instance if two threads each execute this expression in parallel, then - /// there is a race condition. The threads could first both check whether they - /// have the last clone of their `Arc` via `Arc::try_unwrap`, and then - /// both drop their `Arc` in the call to [`ok`][`Result::ok`], - /// taking the strong count from two down to zero. + /// keep the `Arc` in the [`Err`] case. + /// Immediately dropping the [`Err`]-value, as the expression + /// `Arc::try_unwrap(this).ok()` does, can cause the strong count to + /// drop to zero and the inner value of the `Arc` to be dropped. + /// For instance, if two threads execute such an expression in parallel, + /// there is a race condition without the possibility of unsafety: + /// The threads could first both check whether they own the last instance + /// in `Arc::try_unwrap`, determine that they both do not, and then both + /// discard and drop their instance in the call to [`ok`][`Result::ok`]. + /// In this scenario, the value inside the `Arc` is safely destroyed + /// by exactly one of the threads, but neither thread will ever be able + /// to use the value. /// /// # Examples /// @@ -408,9 +410,13 @@ impl Arc { /// it is guaranteed that exactly one of the calls returns the inner value. /// This means in particular that the inner value is not dropped. /// - /// The similar expression `Arc::try_unwrap(this).ok()` does not - /// offer such a guarantee. See the last example below - /// and the documentation of [`Arc::try_unwrap`]. + /// [`Arc::try_unwrap`] is conceptually similar to `Arc::into_inner`, but it + /// is meant for different use-cases. If used as a direct replacement + /// for `Arc::into_inner` anyway, such as with the expression + /// [Arc::try_unwrap]\(this).[ok][Result::ok](), then it does + /// **not** give the same guarantee as described in the previous paragraph. + /// For more information, see the examples below and read the documentation + /// of [`Arc::try_unwrap`]. /// /// # Examples /// @@ -475,14 +481,11 @@ impl Arc { /// /// // Create a long list and clone it /// let mut x = LinkedList::new(); - /// # #[cfg(not(miri))] - /// for i in 0..100000 { + /// let size = 100000; + /// # let size = if cfg!(miri) { 100 } else { size }; + /// for i in 0..size { /// x.push(i); // Adds i to the front of x /// } - /// # #[cfg(miri)] // Miri is slow - /// # for i in 0..100 { - /// # x.push(i); // Adds i to the front of x - /// # } /// let y = x.clone(); /// /// // Drop the clones in parallel @@ -1824,20 +1827,17 @@ impl Clone for Weak { /// ``` #[inline] fn clone(&self) -> Self { - let inner = if let Some(inner) = self.inner() { - inner - } else { - return Self { ptr: self.ptr }; - }; - // See comments in Arc::clone() for why this is relaxed. This can use a - // fetch_add (ignoring the lock) because the weak count is only locked - // where are *no other* weak pointers in existence. (So we can't be - // running this code in that case). - let old_size = inner.weak.fetch_add(1, Relaxed); - - // See comments in Arc::clone() for why we do this (for mem::forget). - if old_size > MAX_REFCOUNT { - abort(); + if let Some(inner) = self.inner() { + // See comments in Arc::clone() for why this is relaxed. This can use a + // fetch_add (ignoring the lock) because the weak count is only locked + // where are *no other* weak pointers in existence. (So we can't be + // running this code in that case). + let old_size = inner.weak.fetch_add(1, Relaxed); + + // See comments in Arc::clone() for why we do this (for mem::forget). + if old_size > MAX_REFCOUNT { + abort(); + } } Self { ptr: self.ptr } diff --git a/portable-atomic-util/src/lib.rs b/portable-atomic-util/src/lib.rs index 22fdc9db..3d0292d4 100644 --- a/portable-atomic-util/src/lib.rs +++ b/portable-atomic-util/src/lib.rs @@ -57,6 +57,7 @@ See [#1] for other primitives being considered for addition to this crate. clippy::std_instead_of_alloc, clippy::std_instead_of_core, )] +#![allow(clippy::inline_always)] // docs.rs only (cfg is enabled by docs.rs, not build script) #![cfg_attr(docsrs, feature(doc_cfg))] diff --git a/portable-atomic-util/src/task.rs b/portable-atomic-util/src/task.rs index cdd954dc..ee121710 100644 --- a/portable-atomic-util/src/task.rs +++ b/portable-atomic-util/src/task.rs @@ -4,12 +4,14 @@ // This module is based on alloc::task::Wake. // -// Source: https://github.com/rust-lang/rust/blob/5151b8c42712c473e7da56e213926b929d0212ef/library/alloc/src/task.rs. +// The code has been adjusted to work with stable Rust. +// +// Source: https://github.com/rust-lang/rust/blob/893f95f1f7c663c67c884120003b3bf21b0af61a/library/alloc/src/task.rs. // // Copyright & License of the original code: -// - https://github.com/rust-lang/rust/blob/5151b8c42712c473e7da56e213926b929d0212ef/COPYRIGHT -// - https://github.com/rust-lang/rust/blob/5151b8c42712c473e7da56e213926b929d0212ef/LICENSE-APACHE -// - https://github.com/rust-lang/rust/blob/5151b8c42712c473e7da56e213926b929d0212ef/LICENSE-MIT +// - https://github.com/rust-lang/rust/blob/893f95f1f7c663c67c884120003b3bf21b0af61a/COPYRIGHT +// - https://github.com/rust-lang/rust/blob/893f95f1f7c663c67c884120003b3bf21b0af61a/LICENSE-APACHE +// - https://github.com/rust-lang/rust/blob/893f95f1f7c663c67c884120003b3bf21b0af61a/LICENSE-MIT use core::{ mem::ManuallyDrop, @@ -120,6 +122,15 @@ impl From> for RawWaker { #[inline(always)] fn raw_waker(waker: Arc) -> 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(waker: *const ()) -> RawWaker { // SAFETY: the caller must uphold the safety contract. unsafe { Arc::increment_strong_count(waker as *const W) }; diff --git a/portable-atomic-util/tests/arc.rs b/portable-atomic-util/tests/arc.rs index bff3576b..66efa058 100644 --- a/portable-atomic-util/tests/arc.rs +++ b/portable-atomic-util/tests/arc.rs @@ -30,23 +30,22 @@ fn default() { assert_eq!(&v[..], ""); } -// https://github.com/rust-lang/rust/blob/5151b8c42712c473e7da56e213926b929d0212ef/library/alloc/src/sync/tests.rs +// https://github.com/rust-lang/rust/blob/893f95f1f7c663c67c884120003b3bf21b0af61a/library/alloc/src/sync/tests.rs #[allow(clippy::many_single_char_names, clippy::undocumented_unsafe_blocks)] mod alloc_tests { use std::{ convert::TryInto, - sync::{ - atomic::Ordering::{Acquire, SeqCst}, - mpsc::channel, - Mutex, - }, + sync::{mpsc::channel, Mutex}, thread, }; - use portable_atomic as atomic; + use portable_atomic::{ + AtomicBool, AtomicUsize, + Ordering::{Acquire, SeqCst}, + }; use portable_atomic_util::{Arc, Weak}; - struct Canary(*mut atomic::AtomicUsize); + struct Canary(*mut AtomicUsize); impl Drop for Canary { fn drop(&mut self) { @@ -337,16 +336,16 @@ mod alloc_tests { #[test] fn drop_arc() { - let mut canary = atomic::AtomicUsize::new(0); - let x = Arc::new(Canary(&mut canary as *mut atomic::AtomicUsize)); + let mut canary = AtomicUsize::new(0); + let x = Arc::new(Canary(&mut canary as *mut AtomicUsize)); drop(x); assert!(canary.load(Acquire) == 1); } #[test] fn drop_arc_weak() { - let mut canary = atomic::AtomicUsize::new(0); - let arc = Arc::new(Canary(&mut canary as *mut atomic::AtomicUsize)); + let mut canary = AtomicUsize::new(0); + let arc = Arc::new(Canary(&mut canary as *mut AtomicUsize)); let arc_weak = Arc::downgrade(&arc); assert!(canary.load(Acquire) == 0); drop(arc); @@ -463,7 +462,7 @@ mod alloc_tests { #[test] #[cfg_attr(target_os = "emscripten", ignore)] fn test_weak_count_locked() { - let mut a = Arc::new(atomic::AtomicBool::new(false)); + let mut a = Arc::new(AtomicBool::new(false)); let a2 = a.clone(); let t = thread::spawn(move || { // Miri is too slow