Skip to content

Commit

Permalink
util: Reflect recent upstream changes on Arc and Wake
Browse files Browse the repository at this point in the history
  • Loading branch information
taiki-e committed Jun 21, 2024
1 parent 4bc15d9 commit ba4d9e6
Show file tree
Hide file tree
Showing 5 changed files with 68 additions and 55 deletions.
2 changes: 2 additions & 0 deletions .github/.cspell/project-dictionary.txt
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,8 @@ versatilepb
virt
vmlinux
vmovdqa
vtable
vtables
wokwi
xadd
xchg
Expand Down
76 changes: 38 additions & 38 deletions portable-atomic-util/src/arc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -357,15 +356,18 @@ impl<T> Arc<T> {
/// 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
///
Expand Down Expand Up @@ -408,9 +410,13 @@ impl<T> Arc<T> {
/// 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
/// <code>[Arc::try_unwrap]\(this).[ok][Result::ok]()</code>, 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
///
Expand Down Expand Up @@ -475,14 +481,11 @@ impl<T> Arc<T> {
///
/// // 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
Expand Down Expand Up @@ -1824,20 +1827,17 @@ impl<T: ?Sized> Clone for Weak<T> {
/// ```
#[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 }
Expand Down
1 change: 1 addition & 0 deletions portable-atomic-util/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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))]

Expand Down
19 changes: 15 additions & 4 deletions portable-atomic-util/src/task.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -120,6 +122,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 {
// SAFETY: the caller must uphold the safety contract.
unsafe { Arc::increment_strong_count(waker as *const W) };
Expand Down
25 changes: 12 additions & 13 deletions portable-atomic-util/tests/arc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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
Expand Down

0 comments on commit ba4d9e6

Please sign in to comment.