From 38d308785f84f7037e63c04c0de1cb20d1c8284d Mon Sep 17 00:00:00 2001 From: cynecx Date: Thu, 2 Jan 2020 00:22:53 +0100 Subject: [PATCH 01/16] channel: Fix unsoundness issue (ManuallyDrop to MaybeUninit) --- crossbeam-channel/src/flavors/list.rs | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/crossbeam-channel/src/flavors/list.rs b/crossbeam-channel/src/flavors/list.rs index ae286d3e4..4ceaecd70 100644 --- a/crossbeam-channel/src/flavors/list.rs +++ b/crossbeam-channel/src/flavors/list.rs @@ -2,7 +2,7 @@ use std::cell::UnsafeCell; use std::marker::PhantomData; -use std::mem::{self, ManuallyDrop}; +use std::mem::MaybeUninit; use std::ptr; use std::sync::atomic::{self, AtomicPtr, AtomicUsize, Ordering}; use std::time::Instant; @@ -42,7 +42,7 @@ const MARK_BIT: usize = 1; /// A slot in a block. struct Slot { /// The message. - msg: UnsafeCell>, + msg: UnsafeCell>, /// The state of the slot. state: AtomicUsize, @@ -72,7 +72,13 @@ struct Block { impl Block { /// Creates an empty block. fn new() -> Block { - unsafe { mem::zeroed() } + // SAFETY: This is safe because: + // [1] `Block::next` (AtomicPtr) may be safely zero initialized. + // [2] `Block::slots` (Array) may be safely zero initialized because of [3, 4]. + // [3] `Slot::msg` (UnsafeCell) may be safely zero initialized because it + // holds a MaybeUninit. + // [4] `Slot::state` (AtomicUsize) may be safely zero initialized. + unsafe { MaybeUninit::zeroed().assume_init() } } /// Waits until the next pointer is set. @@ -280,7 +286,7 @@ impl Channel { let block = token.list.block as *mut Block; let offset = token.list.offset; let slot = (*block).slots.get_unchecked(offset); - slot.msg.get().write(ManuallyDrop::new(msg)); + slot.msg.get().write(MaybeUninit::new(msg)); slot.state.fetch_or(WRITE, Ordering::Release); // Wake a sleeping receiver. @@ -385,8 +391,7 @@ impl Channel { let offset = token.list.offset; let slot = (*block).slots.get_unchecked(offset); slot.wait_write(); - let m = slot.msg.get().read(); - let msg = ManuallyDrop::into_inner(m); + let msg = slot.msg.get().read().assume_init(); // Destroy the block if we've reached the end, or if another thread wanted to destroy but // couldn't because we were busy reading from the slot. @@ -572,7 +577,8 @@ impl Drop for Channel { if offset < BLOCK_CAP { // Drop the message in the slot. let slot = (*block).slots.get_unchecked(offset); - ManuallyDrop::drop(&mut *(*slot).msg.get()); + let p = &mut *slot.msg.get(); + ptr::drop_in_place(p.as_mut_ptr()); } else { // Deallocate the block and move to the next one. let next = (*block).next.load(Ordering::Relaxed); From 7b35a3d17f5c5ec53f70576d4ca0e18be8ed289b Mon Sep 17 00:00:00 2001 From: cynecx Date: Thu, 2 Jan 2020 00:28:13 +0100 Subject: [PATCH 02/16] SegQueue: Fix unsoundness issue (ManuallyDrop to MaybeUninit) --- crossbeam-queue/src/seg_queue.rs | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/crossbeam-queue/src/seg_queue.rs b/crossbeam-queue/src/seg_queue.rs index 2ddfe8b3f..1e5973654 100644 --- a/crossbeam-queue/src/seg_queue.rs +++ b/crossbeam-queue/src/seg_queue.rs @@ -2,7 +2,7 @@ use alloc::boxed::Box; use core::cell::UnsafeCell; use core::fmt; use core::marker::PhantomData; -use core::mem::{self, ManuallyDrop}; +use core::mem::MaybeUninit; use core::ptr; use core::sync::atomic::{self, AtomicPtr, AtomicUsize, Ordering}; @@ -30,7 +30,7 @@ const HAS_NEXT: usize = 1; /// A slot in a block. struct Slot { /// The value. - value: UnsafeCell>, + value: UnsafeCell>, /// The state of the slot. state: AtomicUsize, @@ -60,7 +60,13 @@ struct Block { impl Block { /// Creates an empty block that starts at `start_index`. fn new() -> Block { - unsafe { mem::zeroed() } + // SAFETY: This is safe because: + // [1] `Block::next` (AtomicPtr) may be safely zero initialized. + // [2] `Block::slots` (Array) may be safely zero initialized because of [3, 4]. + // [3] `Slot::value` (UnsafeCell) may be safely zero initialized because it + // holds a MaybeUninit. + // [4] `Slot::state` (AtomicUsize) may be safely zero initialized. + unsafe { MaybeUninit::zeroed().assume_init() } } /// Waits until the next pointer is set. @@ -244,7 +250,7 @@ impl SegQueue { // Write the value into the slot. let slot = (*block).slots.get_unchecked(offset); - slot.value.get().write(ManuallyDrop::new(value)); + slot.value.get().write(MaybeUninit::new(value)); slot.state.fetch_or(WRITE, Ordering::Release); return; @@ -339,8 +345,7 @@ impl SegQueue { // Read the value. let slot = (*block).slots.get_unchecked(offset); slot.wait_write(); - let m = slot.value.get().read(); - let value = ManuallyDrop::into_inner(m); + let value = slot.value.get().read().assume_init(); // Destroy the block if we've reached the end, or if another thread wanted to // destroy but couldn't because we were busy reading from the slot. @@ -451,7 +456,8 @@ impl Drop for SegQueue { if offset < BLOCK_CAP { // Drop the value in the slot. let slot = (*block).slots.get_unchecked(offset); - ManuallyDrop::drop(&mut *(*slot).value.get()); + let p = &mut *slot.value.get(); + ptr::drop_in_place(p.as_mut_ptr()); } else { // Deallocate the block and move to the next one. let next = (*block).next.load(Ordering::Relaxed); From 68e8708c2dda24e4b3b1807f5f301ca2b8ffa357 Mon Sep 17 00:00:00 2001 From: cynecx Date: Thu, 2 Jan 2020 00:37:25 +0100 Subject: [PATCH 03/16] deque: Fix unsoundness issue (ManuallyDrop to MaybeUninit) --- crossbeam-deque/src/lib.rs | 35 ++++++++++++++++++----------------- 1 file changed, 18 insertions(+), 17 deletions(-) diff --git a/crossbeam-deque/src/lib.rs b/crossbeam-deque/src/lib.rs index 70ffecc42..22f4cfd55 100644 --- a/crossbeam-deque/src/lib.rs +++ b/crossbeam-deque/src/lib.rs @@ -97,7 +97,7 @@ use std::cmp; use std::fmt; use std::iter::FromIterator; use std::marker::PhantomData; -use std::mem::{self, ManuallyDrop}; +use std::mem::{self, MaybeUninit}; use std::ptr; use std::sync::atomic::{self, AtomicIsize, AtomicPtr, AtomicUsize, Ordering}; use std::sync::Arc; @@ -1140,7 +1140,7 @@ const HAS_NEXT: usize = 1; /// A slot in a block. struct Slot { /// The task. - task: UnsafeCell>, + task: UnsafeCell>, /// The state of the slot. state: AtomicUsize, @@ -1170,7 +1170,13 @@ struct Block { impl Block { /// Creates an empty block that starts at `start_index`. fn new() -> Block { - unsafe { mem::zeroed() } + // SAFETY: This is safe because: + // [1] `Block::next` (AtomicPtr) may be safely zero initialized. + // [2] `Block::slots` (Array) may be safely zero initialized because of [3, 4]. + // [3] `Slot::task` (UnsafeCell) may be safely zero initialized because it + // holds a MaybeUninit. + // [4] `Slot::state` (AtomicUsize) may be safely zero initialized. + unsafe { MaybeUninit::zeroed().assume_init() } } /// Waits until the next pointer is set. @@ -1329,7 +1335,7 @@ impl Injector { // Write the task into the slot. let slot = (*block).slots.get_unchecked(offset); - slot.task.get().write(ManuallyDrop::new(task)); + slot.task.get().write(MaybeUninit::new(task)); slot.state.fetch_or(WRITE, Ordering::Release); return; @@ -1422,8 +1428,7 @@ impl Injector { // Read the task. let slot = (*block).slots.get_unchecked(offset); slot.wait_write(); - let m = slot.task.get().read(); - let task = ManuallyDrop::into_inner(m); + let task = slot.task.get().read().assume_init(); // Destroy the block if we've reached the end, or if another thread wanted to destroy // but couldn't because we were busy reading from the slot. @@ -1548,8 +1553,7 @@ impl Injector { // Read the task. let slot = (*block).slots.get_unchecked(offset + i); slot.wait_write(); - let m = slot.task.get().read(); - let task = ManuallyDrop::into_inner(m); + let task = slot.task.get().read().assume_init(); // Write it into the destination queue. dest_buffer.write(dest_b.wrapping_add(i as isize), task); @@ -1561,8 +1565,7 @@ impl Injector { // Read the task. let slot = (*block).slots.get_unchecked(offset + i); slot.wait_write(); - let m = slot.task.get().read(); - let task = ManuallyDrop::into_inner(m); + let task = slot.task.get().read().assume_init(); // Write it into the destination queue. dest_buffer.write(dest_b.wrapping_add((batch_size - 1 - i) as isize), task); @@ -1704,8 +1707,7 @@ impl Injector { // Read the task. let slot = (*block).slots.get_unchecked(offset); slot.wait_write(); - let m = slot.task.get().read(); - let task = ManuallyDrop::into_inner(m); + let task = slot.task.get().read().assume_init(); match dest.flavor { Flavor::Fifo => { @@ -1714,8 +1716,7 @@ impl Injector { // Read the task. let slot = (*block).slots.get_unchecked(offset + i + 1); slot.wait_write(); - let m = slot.task.get().read(); - let task = ManuallyDrop::into_inner(m); + let task = slot.task.get().read().assume_init(); // Write it into the destination queue. dest_buffer.write(dest_b.wrapping_add(i as isize), task); @@ -1728,8 +1729,7 @@ impl Injector { // Read the task. let slot = (*block).slots.get_unchecked(offset + i + 1); slot.wait_write(); - let m = slot.task.get().read(); - let task = ManuallyDrop::into_inner(m); + let task = slot.task.get().read().assume_init(); // Write it into the destination queue. dest_buffer.write(dest_b.wrapping_add((batch_size - 1 - i) as isize), task); @@ -1804,7 +1804,8 @@ impl Drop for Injector { if offset < BLOCK_CAP { // Drop the task in the slot. let slot = (*block).slots.get_unchecked(offset); - ManuallyDrop::drop(&mut *(*slot).task.get()); + let p = &mut *slot.task.get(); + ptr::drop_in_place(p.as_mut_ptr()); } else { // Deallocate the block and move to the next one. let next = (*block).next.load(Ordering::Relaxed); From e0fd465b9b7de8b60b0b588e264aca1fa92fbd18 Mon Sep 17 00:00:00 2001 From: cynecx Date: Mon, 6 Jan 2020 20:40:04 +0100 Subject: [PATCH 04/16] epoch: Fix fixme in deferred.rs (mem::uninitialized -> MaybeUninit) --- crossbeam-epoch/src/deferred.rs | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/crossbeam-epoch/src/deferred.rs b/crossbeam-epoch/src/deferred.rs index 3d22ee633..4f620de92 100644 --- a/crossbeam-epoch/src/deferred.rs +++ b/crossbeam-epoch/src/deferred.rs @@ -1,7 +1,7 @@ use alloc::boxed::Box; use core::fmt; use core::marker::PhantomData; -use core::mem; +use core::mem::{self, MaybeUninit}; use core::ptr; /// Number of words a piece of `Data` can hold. @@ -36,11 +36,8 @@ impl Deferred { unsafe { if size <= mem::size_of::() && align <= mem::align_of::() { - // TODO(taiki-e): when the minimum supported Rust version is bumped to 1.36+, - // replace this with `mem::MaybeUninit`. - #[allow(deprecated)] - let mut data: Data = mem::uninitialized(); - ptr::write(&mut data as *mut Data as *mut F, f); + let mut data = MaybeUninit::::uninit(); + ptr::write(data.as_mut_ptr() as *mut F, f); unsafe fn call(raw: *mut u8) { let f: F = ptr::read(raw as *mut F); @@ -49,16 +46,13 @@ impl Deferred { Deferred { call: call::, - data, + data: data.assume_init(), _marker: PhantomData, } } else { let b: Box = Box::new(f); - // TODO(taiki-e): when the minimum supported Rust version is bumped to 1.36+, - // replace this with `mem::MaybeUninit`. - #[allow(deprecated)] - let mut data: Data = mem::uninitialized(); - ptr::write(&mut data as *mut Data as *mut Box, b); + let mut data = MaybeUninit::::uninit(); + ptr::write(data.as_mut_ptr() as *mut Box, b); unsafe fn call(raw: *mut u8) { let b: Box = ptr::read(raw as *mut Box); @@ -67,7 +61,7 @@ impl Deferred { Deferred { call: call::, - data, + data: data.assume_init(), _marker: PhantomData, } } From f48c1c763a73467e2fde6158c0abc2faa162ae81 Mon Sep 17 00:00:00 2001 From: cynecx Date: Mon, 6 Jan 2020 20:41:17 +0100 Subject: [PATCH 05/16] epoch: fix unsoundness in sync/queue.rs (ManuallyDrop/uninitialized -> MaybeUninit) --- crossbeam-epoch/src/sync/queue.rs | 52 +++++++++++++++---------------- 1 file changed, 26 insertions(+), 26 deletions(-) diff --git a/crossbeam-epoch/src/sync/queue.rs b/crossbeam-epoch/src/sync/queue.rs index 08584002b..4c8c3e792 100644 --- a/crossbeam-epoch/src/sync/queue.rs +++ b/crossbeam-epoch/src/sync/queue.rs @@ -8,8 +8,7 @@ //! Simon Doherty, Lindsay Groves, Victor Luchangco, and Mark Moir. 2004b. Formal Verification of a //! Practical Lock-Free Queue Algorithm. https://doi.org/10.1007/978-3-540-30232-2_7 -use core::mem::{self, ManuallyDrop}; -use core::ptr; +use core::mem::MaybeUninit; use core::sync::atomic::Ordering::{Acquire, Relaxed, Release}; use crossbeam_utils::CachePadded; @@ -29,11 +28,11 @@ pub struct Queue { struct Node { /// The slot in which a value of type `T` can be stored. /// - /// The type of `data` is `ManuallyDrop` because a `Node` doesn't always contain a `T`. + /// The type of `data` is `MaybeUninit` because a `Node` doesn't always contain a `T`. /// For example, the sentinel node in a queue never contains a value: its slot is always empty. /// Other nodes start their life with a push operation and contain a value until it gets popped /// out. After that such empty nodes get added to the collector for destruction. - data: ManuallyDrop, + data: MaybeUninit, next: Atomic>, } @@ -49,11 +48,8 @@ impl Queue { head: CachePadded::new(Atomic::null()), tail: CachePadded::new(Atomic::null()), }; - // TODO(taiki-e): when the minimum supported Rust version is bumped to 1.36+, - // replace this with `mem::MaybeUninit`. - #[allow(deprecated)] let sentinel = Owned::new(Node { - data: unsafe { mem::uninitialized() }, + data: MaybeUninit::uninit(), next: Atomic::null(), }); unsafe { @@ -93,7 +89,7 @@ impl Queue { /// Adds `t` to the back of the queue, possibly waking up threads blocked on `pop`. pub fn push(&self, t: T, guard: &Guard) { let new = Owned::new(Node { - data: ManuallyDrop::new(t), + data: MaybeUninit::new(t), next: Atomic::null(), }); let new = Owned::into_shared(new, guard); @@ -126,7 +122,8 @@ impl Queue { let _ = self.tail.compare_and_set(tail, next, Release, guard); } guard.defer_destroy(head); - Some(ManuallyDrop::into_inner(ptr::read(&n.data))) + // TODO: Replace with MaybeUninit::read when api is stable + Some(n.data.as_ptr().read()) }) .map_err(|_| ()) }, @@ -145,22 +142,25 @@ impl Queue { let head = self.head.load(Acquire, guard); let h = unsafe { head.deref() }; let next = h.next.load(Acquire, guard); - match unsafe { next.as_ref() } { - Some(n) if condition(&n.data) => unsafe { - self.head - .compare_and_set(head, next, Release, guard) - .map(|_| { - let tail = self.tail.load(Relaxed, guard); - // Advance the tail so that we don't retire a pointer to a reachable node. - if head == tail { - let _ = self.tail.compare_and_set(tail, next, Release, guard); - } - guard.defer_destroy(head); - Some(ManuallyDrop::into_inner(ptr::read(&n.data))) - }) - .map_err(|_| ()) - }, - None | Some(_) => Ok(None), + unsafe { + match next.as_ref() { + Some(n) if condition(&*n.data.as_ptr()) => { + self.head + .compare_and_set(head, next, Release, guard) + .map(|_| { + let tail = self.tail.load(Relaxed, guard); + // Advance the tail so that we don't retire a pointer to a reachable node. + if head == tail { + let _ = self.tail.compare_and_set(tail, next, Release, guard); + } + guard.defer_destroy(head); + // TODO: Replace with MaybeUninit::read when api is stable + Some(n.data.as_ptr().read()) + }) + .map_err(|_| ()) + } + None | Some(_) => Ok(None), + } } } From 6f3a3c17ea89fe16f55246cdfb64d684a52a5292 Mon Sep 17 00:00:00 2001 From: cynecx Date: Mon, 6 Jan 2020 20:47:47 +0100 Subject: [PATCH 06/16] queue: fix unsoundness issue in array_queue (introduce MaybeUninit) --- crossbeam-queue/src/array_queue.rs | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/crossbeam-queue/src/array_queue.rs b/crossbeam-queue/src/array_queue.rs index 683daad8b..b174adf62 100644 --- a/crossbeam-queue/src/array_queue.rs +++ b/crossbeam-queue/src/array_queue.rs @@ -12,7 +12,7 @@ use alloc::vec::Vec; use core::cell::UnsafeCell; use core::fmt; use core::marker::PhantomData; -use core::mem; +use core::mem::{self, MaybeUninit}; use core::ptr; use core::sync::atomic::{self, AtomicUsize, Ordering}; @@ -29,7 +29,7 @@ struct Slot { stamp: AtomicUsize, /// The value in this slot. - value: UnsafeCell, + value: UnsafeCell>, } /// A bounded multi-producer multi-consumer queue. @@ -186,9 +186,7 @@ impl ArrayQueue { ) { Ok(_) => { // Write the value into the slot and update the stamp. - unsafe { - slot.value.get().write(value); - } + unsafe { slot.value.get().write(MaybeUninit::new(value)) } slot.stamp.store(tail + 1, Ordering::Release); return Ok(()); } @@ -266,7 +264,7 @@ impl ArrayQueue { ) { Ok(_) => { // Read the value from the slot and update the stamp. - let msg = unsafe { slot.value.get().read() }; + let msg = unsafe { slot.value.get().read().assume_init() }; slot.stamp .store(head.wrapping_add(self.one_lap), Ordering::Release); return Ok(msg); From 71d2799afe690ef9d07bb77614c6bb164cb8e673 Mon Sep 17 00:00:00 2001 From: cynecx Date: Mon, 6 Jan 2020 20:51:03 +0100 Subject: [PATCH 07/16] channel: fix unsoundness issue in array flavor (introduce MaybeUninit) --- crossbeam-channel/src/flavors/array.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/crossbeam-channel/src/flavors/array.rs b/crossbeam-channel/src/flavors/array.rs index 1537c0939..1665059fd 100644 --- a/crossbeam-channel/src/flavors/array.rs +++ b/crossbeam-channel/src/flavors/array.rs @@ -15,7 +15,7 @@ use std::cell::UnsafeCell; use std::marker::PhantomData; -use std::mem; +use std::mem::{self, MaybeUninit}; use std::ptr; use std::sync::atomic::{self, AtomicUsize, Ordering}; use std::time::Instant; @@ -33,7 +33,7 @@ struct Slot { stamp: AtomicUsize, /// The message in this slot. - msg: UnsafeCell, + msg: UnsafeCell>, } /// The token type for the array flavor. @@ -233,7 +233,7 @@ impl Channel { let slot: &Slot = &*(token.array.slot as *const Slot); // Write the message into the slot and update the stamp. - slot.msg.get().write(msg); + slot.msg.get().write(MaybeUninit::new(msg)); slot.stamp.store(token.array.stamp, Ordering::Release); // Wake a sleeping receiver. @@ -323,7 +323,7 @@ impl Channel { let slot: &Slot = &*(token.array.slot as *const Slot); // Read the message from the slot and update the stamp. - let msg = slot.msg.get().read(); + let msg = slot.msg.get().read().assume_init(); slot.stamp.store(token.array.stamp, Ordering::Release); // Wake a sleeping sender. From 5362ad3effe644ec98a80b01e9d881cc0231fc2a Mon Sep 17 00:00:00 2001 From: cynecx Date: Mon, 6 Jan 2020 21:00:40 +0100 Subject: [PATCH 08/16] epoch: remove Debug impl on Node --- crossbeam-epoch/src/sync/queue.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/crossbeam-epoch/src/sync/queue.rs b/crossbeam-epoch/src/sync/queue.rs index 4c8c3e792..700aad680 100644 --- a/crossbeam-epoch/src/sync/queue.rs +++ b/crossbeam-epoch/src/sync/queue.rs @@ -24,7 +24,6 @@ pub struct Queue { tail: CachePadded>>, } -#[derive(Debug)] struct Node { /// The slot in which a value of type `T` can be stored. /// From dce3e118f2778423113e6d6019dd05d88cf7e0e6 Mon Sep 17 00:00:00 2001 From: cynecx Date: Mon, 6 Jan 2020 21:24:58 +0100 Subject: [PATCH 09/16] channel: add missing drop_in_place for array channel flavour's msgs --- crossbeam-channel/src/flavors/array.rs | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/crossbeam-channel/src/flavors/array.rs b/crossbeam-channel/src/flavors/array.rs index 1665059fd..a9c8b65d1 100644 --- a/crossbeam-channel/src/flavors/array.rs +++ b/crossbeam-channel/src/flavors/array.rs @@ -542,7 +542,17 @@ impl Drop for Channel { }; unsafe { - self.buffer.add(index).drop_in_place(); + let ptr = self.buffer.add(index); + { + // This requires an extra scope because when we drop the Slot, + // reference to it should not exist. + let slot = &mut *ptr; + let msg = &mut *slot.msg.get(); + // Drop the message (MaybeUninit). + msg.as_mut_ptr().drop_in_place(); + } + // Drop slot (This should be a no-op). + ptr.drop_in_place(); } } From d78de60ea78c3aa0eeca66f42dda0154a7cc89fc Mon Sep 17 00:00:00 2001 From: cynecx Date: Mon, 6 Jan 2020 21:32:53 +0100 Subject: [PATCH 10/16] queue: add missing drop_in_place for array_queue slot values --- crossbeam-queue/src/array_queue.rs | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/crossbeam-queue/src/array_queue.rs b/crossbeam-queue/src/array_queue.rs index b174adf62..2f76bd432 100644 --- a/crossbeam-queue/src/array_queue.rs +++ b/crossbeam-queue/src/array_queue.rs @@ -413,7 +413,17 @@ impl Drop for ArrayQueue { }; unsafe { - self.buffer.add(index).drop_in_place(); + let ptr = self.buffer.add(index); + { + // This requires an extra scope because when we drop the Slot, + // reference to it should not exist. + let slot = &mut *ptr; + let value = &mut *slot.value.get(); + // Drop the message (MaybeUninit). + value.as_mut_ptr().drop_in_place(); + } + // Drop slot (This should be a no-op). + ptr.drop_in_place(); } } From 1a8cbe13fb4c42874aed9269b52bdde28cc540ef Mon Sep 17 00:00:00 2001 From: cynecx Date: Mon, 6 Jan 2020 21:47:26 +0100 Subject: [PATCH 11/16] drop_in_place cleanups --- crossbeam-channel/src/flavors/list.rs | 2 +- crossbeam-deque/src/lib.rs | 4 ++-- crossbeam-queue/src/seg_queue.rs | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/crossbeam-channel/src/flavors/list.rs b/crossbeam-channel/src/flavors/list.rs index 4ceaecd70..e32bc0c03 100644 --- a/crossbeam-channel/src/flavors/list.rs +++ b/crossbeam-channel/src/flavors/list.rs @@ -578,7 +578,7 @@ impl Drop for Channel { // Drop the message in the slot. let slot = (*block).slots.get_unchecked(offset); let p = &mut *slot.msg.get(); - ptr::drop_in_place(p.as_mut_ptr()); + p.as_mut_ptr().drop_in_place(); } else { // Deallocate the block and move to the next one. let next = (*block).next.load(Ordering::Relaxed); diff --git a/crossbeam-deque/src/lib.rs b/crossbeam-deque/src/lib.rs index 22f4cfd55..942a9646c 100644 --- a/crossbeam-deque/src/lib.rs +++ b/crossbeam-deque/src/lib.rs @@ -218,7 +218,7 @@ impl Drop for Inner { // Go through the buffer from front to back and drop all tasks in the queue. let mut i = f; while i != b { - ptr::drop_in_place(buffer.deref().at(i)); + buffer.deref().at(i).drop_in_place(); i = i.wrapping_add(1); } @@ -1805,7 +1805,7 @@ impl Drop for Injector { // Drop the task in the slot. let slot = (*block).slots.get_unchecked(offset); let p = &mut *slot.task.get(); - ptr::drop_in_place(p.as_mut_ptr()); + p.as_mut_ptr().drop_in_place(); } else { // Deallocate the block and move to the next one. let next = (*block).next.load(Ordering::Relaxed); diff --git a/crossbeam-queue/src/seg_queue.rs b/crossbeam-queue/src/seg_queue.rs index 1e5973654..0e96853a6 100644 --- a/crossbeam-queue/src/seg_queue.rs +++ b/crossbeam-queue/src/seg_queue.rs @@ -457,7 +457,7 @@ impl Drop for SegQueue { // Drop the value in the slot. let slot = (*block).slots.get_unchecked(offset); let p = &mut *slot.value.get(); - ptr::drop_in_place(p.as_mut_ptr()); + p.as_mut_ptr().drop_in_place(); } else { // Deallocate the block and move to the next one. let next = (*block).next.load(Ordering::Relaxed); From 7de64fe01926724ad9910b52303e43a49f6e0131 Mon Sep 17 00:00:00 2001 From: cynecx Date: Thu, 9 Jan 2020 16:52:35 +0100 Subject: [PATCH 12/16] cleanup drop impl for ArrayQueue / Channel by eliminating one no-op drop_in_place --- crossbeam-channel/src/flavors/array.rs | 15 +++++---------- crossbeam-queue/src/array_queue.rs | 15 +++++---------- 2 files changed, 10 insertions(+), 20 deletions(-) diff --git a/crossbeam-channel/src/flavors/array.rs b/crossbeam-channel/src/flavors/array.rs index a9c8b65d1..9cd47a293 100644 --- a/crossbeam-channel/src/flavors/array.rs +++ b/crossbeam-channel/src/flavors/array.rs @@ -542,17 +542,12 @@ impl Drop for Channel { }; unsafe { - let ptr = self.buffer.add(index); - { - // This requires an extra scope because when we drop the Slot, - // reference to it should not exist. - let slot = &mut *ptr; + let p = { + let slot = &mut *self.buffer.add(index); let msg = &mut *slot.msg.get(); - // Drop the message (MaybeUninit). - msg.as_mut_ptr().drop_in_place(); - } - // Drop slot (This should be a no-op). - ptr.drop_in_place(); + msg.as_mut_ptr() + }; + p.drop_in_place(); } } diff --git a/crossbeam-queue/src/array_queue.rs b/crossbeam-queue/src/array_queue.rs index 2f76bd432..b8055a29d 100644 --- a/crossbeam-queue/src/array_queue.rs +++ b/crossbeam-queue/src/array_queue.rs @@ -413,17 +413,12 @@ impl Drop for ArrayQueue { }; unsafe { - let ptr = self.buffer.add(index); - { - // This requires an extra scope because when we drop the Slot, - // reference to it should not exist. - let slot = &mut *ptr; + let p = { + let slot = &mut *self.buffer.add(index); let value = &mut *slot.value.get(); - // Drop the message (MaybeUninit). - value.as_mut_ptr().drop_in_place(); - } - // Drop slot (This should be a no-op). - ptr.drop_in_place(); + value.as_mut_ptr() + }; + p.drop_in_place(); } } From b5525b73a5955c5f39157f088b78b93e89436491 Mon Sep 17 00:00:00 2001 From: cynecx Date: Thu, 6 Feb 2020 17:46:26 +0100 Subject: [PATCH 13/16] Use maybe-uninit crate to bridge backwards compatibility issues with MaybeUninit --- crossbeam-channel/Cargo.toml | 3 +++ crossbeam-channel/src/flavors/array.rs | 4 +++- crossbeam-channel/src/flavors/list.rs | 3 ++- crossbeam-channel/src/lib.rs | 1 + crossbeam-deque/Cargo.toml | 3 +++ crossbeam-deque/src/lib.rs | 6 +++++- crossbeam-epoch/Cargo.toml | 1 + crossbeam-epoch/src/deferred.rs | 4 +++- crossbeam-epoch/src/lib.rs | 2 ++ crossbeam-epoch/src/sync/queue.rs | 3 ++- crossbeam-queue/Cargo.toml | 1 + crossbeam-queue/src/array_queue.rs | 4 +++- crossbeam-queue/src/lib.rs | 2 ++ crossbeam-queue/src/seg_queue.rs | 3 ++- 14 files changed, 33 insertions(+), 7 deletions(-) diff --git a/crossbeam-channel/Cargo.toml b/crossbeam-channel/Cargo.toml index 2b212a93e..d9e436687 100644 --- a/crossbeam-channel/Cargo.toml +++ b/crossbeam-channel/Cargo.toml @@ -15,6 +15,9 @@ description = "Multi-producer multi-consumer channels for message passing" keywords = ["channel", "mpmc", "select", "golang", "message"] categories = ["algorithms", "concurrency", "data-structures"] +[dependencies] +maybe-uninit = "2.0.0" + [dependencies.crossbeam-utils] version = "0.7" path = "../crossbeam-utils" diff --git a/crossbeam-channel/src/flavors/array.rs b/crossbeam-channel/src/flavors/array.rs index 9cd47a293..659fce69a 100644 --- a/crossbeam-channel/src/flavors/array.rs +++ b/crossbeam-channel/src/flavors/array.rs @@ -15,13 +15,15 @@ use std::cell::UnsafeCell; use std::marker::PhantomData; -use std::mem::{self, MaybeUninit}; +use std::mem; use std::ptr; use std::sync::atomic::{self, AtomicUsize, Ordering}; use std::time::Instant; use crossbeam_utils::{Backoff, CachePadded}; +use maybe_uninit::MaybeUninit; + use context::Context; use err::{RecvTimeoutError, SendTimeoutError, TryRecvError, TrySendError}; use select::{Operation, SelectHandle, Selected, Token}; diff --git a/crossbeam-channel/src/flavors/list.rs b/crossbeam-channel/src/flavors/list.rs index e32bc0c03..dd9e6e9de 100644 --- a/crossbeam-channel/src/flavors/list.rs +++ b/crossbeam-channel/src/flavors/list.rs @@ -2,13 +2,14 @@ use std::cell::UnsafeCell; use std::marker::PhantomData; -use std::mem::MaybeUninit; use std::ptr; use std::sync::atomic::{self, AtomicPtr, AtomicUsize, Ordering}; use std::time::Instant; use crossbeam_utils::{Backoff, CachePadded}; +use maybe_uninit::MaybeUninit; + use context::Context; use err::{RecvTimeoutError, SendTimeoutError, TryRecvError, TrySendError}; use select::{Operation, SelectHandle, Selected, Token}; diff --git a/crossbeam-channel/src/lib.rs b/crossbeam-channel/src/lib.rs index 1d5160056..898239632 100644 --- a/crossbeam-channel/src/lib.rs +++ b/crossbeam-channel/src/lib.rs @@ -348,6 +348,7 @@ #![warn(missing_debug_implementations)] extern crate crossbeam_utils; +extern crate maybe_uninit; mod channel; mod context; diff --git a/crossbeam-deque/Cargo.toml b/crossbeam-deque/Cargo.toml index a8dd915ae..35edd56df 100644 --- a/crossbeam-deque/Cargo.toml +++ b/crossbeam-deque/Cargo.toml @@ -15,6 +15,9 @@ description = "Concurrent work-stealing deque" keywords = ["chase-lev", "lock-free", "scheduler", "scheduling"] categories = ["algorithms", "concurrency", "data-structures"] +[dependencies] +maybe-uninit = "2.0.0" + [dependencies.crossbeam-epoch] version = "0.8" path = "../crossbeam-epoch" diff --git a/crossbeam-deque/src/lib.rs b/crossbeam-deque/src/lib.rs index 942a9646c..59004174d 100644 --- a/crossbeam-deque/src/lib.rs +++ b/crossbeam-deque/src/lib.rs @@ -92,12 +92,14 @@ extern crate crossbeam_epoch as epoch; extern crate crossbeam_utils as utils; +extern crate maybe_uninit; + use std::cell::{Cell, UnsafeCell}; use std::cmp; use std::fmt; use std::iter::FromIterator; use std::marker::PhantomData; -use std::mem::{self, MaybeUninit}; +use std::mem; use std::ptr; use std::sync::atomic::{self, AtomicIsize, AtomicPtr, AtomicUsize, Ordering}; use std::sync::Arc; @@ -105,6 +107,8 @@ use std::sync::Arc; use epoch::{Atomic, Owned}; use utils::{Backoff, CachePadded}; +use maybe_uninit::MaybeUninit; + // Minimum buffer capacity. const MIN_CAP: usize = 64; // Maximum number of tasks that can be stolen in `steal_batch()` and `steal_batch_and_pop()`. diff --git a/crossbeam-epoch/Cargo.toml b/crossbeam-epoch/Cargo.toml index d76b57935..629eacf12 100644 --- a/crossbeam-epoch/Cargo.toml +++ b/crossbeam-epoch/Cargo.toml @@ -24,6 +24,7 @@ sanitize = [] # Makes it more likely to trigger any potential data races. [dependencies] cfg-if = "0.1.2" +maybe-uninit = "2.0.0" memoffset = "0.5" [dependencies.crossbeam-utils] diff --git a/crossbeam-epoch/src/deferred.rs b/crossbeam-epoch/src/deferred.rs index 4f620de92..a0970f115 100644 --- a/crossbeam-epoch/src/deferred.rs +++ b/crossbeam-epoch/src/deferred.rs @@ -1,9 +1,11 @@ use alloc::boxed::Box; use core::fmt; use core::marker::PhantomData; -use core::mem::{self, MaybeUninit}; +use core::mem; use core::ptr; +use maybe_uninit::MaybeUninit; + /// Number of words a piece of `Data` can hold. /// /// Three words should be enough for the majority of cases. For example, you can fit inside it the diff --git a/crossbeam-epoch/src/lib.rs b/crossbeam-epoch/src/lib.rs index 2498f9dd8..282bbe90f 100644 --- a/crossbeam-epoch/src/lib.rs +++ b/crossbeam-epoch/src/lib.rs @@ -64,6 +64,8 @@ extern crate cfg_if; #[cfg(feature = "std")] extern crate core; +extern crate maybe_uninit; + cfg_if! { if #[cfg(feature = "alloc")] { extern crate alloc; diff --git a/crossbeam-epoch/src/sync/queue.rs b/crossbeam-epoch/src/sync/queue.rs index 700aad680..b6404568f 100644 --- a/crossbeam-epoch/src/sync/queue.rs +++ b/crossbeam-epoch/src/sync/queue.rs @@ -8,11 +8,12 @@ //! Simon Doherty, Lindsay Groves, Victor Luchangco, and Mark Moir. 2004b. Formal Verification of a //! Practical Lock-Free Queue Algorithm. https://doi.org/10.1007/978-3-540-30232-2_7 -use core::mem::MaybeUninit; use core::sync::atomic::Ordering::{Acquire, Relaxed, Release}; use crossbeam_utils::CachePadded; +use maybe_uninit::MaybeUninit; + use {unprotected, Atomic, Guard, Owned, Shared}; // The representation here is a singly-linked list, with a sentinel node at the front. In general diff --git a/crossbeam-queue/Cargo.toml b/crossbeam-queue/Cargo.toml index 0e50c878b..7b5c97920 100644 --- a/crossbeam-queue/Cargo.toml +++ b/crossbeam-queue/Cargo.toml @@ -22,6 +22,7 @@ alloc = ["crossbeam-utils/alloc"] [dependencies] cfg-if = "0.1.2" +maybe-uninit = "2.0.0" [dependencies.crossbeam-utils] version = "0.7" diff --git a/crossbeam-queue/src/array_queue.rs b/crossbeam-queue/src/array_queue.rs index b8055a29d..d272d1953 100644 --- a/crossbeam-queue/src/array_queue.rs +++ b/crossbeam-queue/src/array_queue.rs @@ -12,12 +12,14 @@ use alloc::vec::Vec; use core::cell::UnsafeCell; use core::fmt; use core::marker::PhantomData; -use core::mem::{self, MaybeUninit}; +use core::mem; use core::ptr; use core::sync::atomic::{self, AtomicUsize, Ordering}; use crossbeam_utils::{Backoff, CachePadded}; +use maybe_uninit::MaybeUninit; + use err::{PopError, PushError}; /// A slot in a queue. diff --git a/crossbeam-queue/src/lib.rs b/crossbeam-queue/src/lib.rs index 2eda6b0df..fd9939207 100644 --- a/crossbeam-queue/src/lib.rs +++ b/crossbeam-queue/src/lib.rs @@ -17,6 +17,8 @@ extern crate cfg_if; #[cfg(feature = "std")] extern crate core; +extern crate maybe_uninit; + cfg_if! { if #[cfg(feature = "alloc")] { extern crate alloc; diff --git a/crossbeam-queue/src/seg_queue.rs b/crossbeam-queue/src/seg_queue.rs index 0e96853a6..b52da4f5c 100644 --- a/crossbeam-queue/src/seg_queue.rs +++ b/crossbeam-queue/src/seg_queue.rs @@ -2,12 +2,13 @@ use alloc::boxed::Box; use core::cell::UnsafeCell; use core::fmt; use core::marker::PhantomData; -use core::mem::MaybeUninit; use core::ptr; use core::sync::atomic::{self, AtomicPtr, AtomicUsize, Ordering}; use crossbeam_utils::{Backoff, CachePadded}; +use maybe_uninit::MaybeUninit; + use err::PopError; // Bits indicating the state of a slot: From a86b3b6a684a70e2854ce6f8d1ca46f18a0f53eb Mon Sep 17 00:00:00 2001 From: cynecx Date: Mon, 10 Feb 2020 16:23:44 +0100 Subject: [PATCH 14/16] Cleanup some things (Removed stale comment and add semicolon to express statement) --- crossbeam-epoch/src/sync/queue.rs | 1 - crossbeam-queue/src/array_queue.rs | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/crossbeam-epoch/src/sync/queue.rs b/crossbeam-epoch/src/sync/queue.rs index b6404568f..bb4c71db4 100644 --- a/crossbeam-epoch/src/sync/queue.rs +++ b/crossbeam-epoch/src/sync/queue.rs @@ -154,7 +154,6 @@ impl Queue { let _ = self.tail.compare_and_set(tail, next, Release, guard); } guard.defer_destroy(head); - // TODO: Replace with MaybeUninit::read when api is stable Some(n.data.as_ptr().read()) }) .map_err(|_| ()) diff --git a/crossbeam-queue/src/array_queue.rs b/crossbeam-queue/src/array_queue.rs index d272d1953..8de1ea0b5 100644 --- a/crossbeam-queue/src/array_queue.rs +++ b/crossbeam-queue/src/array_queue.rs @@ -188,7 +188,7 @@ impl ArrayQueue { ) { Ok(_) => { // Write the value into the slot and update the stamp. - unsafe { slot.value.get().write(MaybeUninit::new(value)) } + unsafe { slot.value.get().write(MaybeUninit::new(value)); } slot.stamp.store(tail + 1, Ordering::Release); return Ok(()); } From 8abcc14007c1b8c8ba5d5c366b47e0640fc5756e Mon Sep 17 00:00:00 2001 From: cynecx Date: Mon, 10 Feb 2020 16:42:05 +0100 Subject: [PATCH 15/16] Fix formatting issues in crossbeam-queue/src/array_queue.rs --- crossbeam-queue/src/array_queue.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/crossbeam-queue/src/array_queue.rs b/crossbeam-queue/src/array_queue.rs index 8de1ea0b5..45b055abd 100644 --- a/crossbeam-queue/src/array_queue.rs +++ b/crossbeam-queue/src/array_queue.rs @@ -188,7 +188,9 @@ impl ArrayQueue { ) { Ok(_) => { // Write the value into the slot and update the stamp. - unsafe { slot.value.get().write(MaybeUninit::new(value)); } + unsafe { + slot.value.get().write(MaybeUninit::new(value)); + } slot.stamp.store(tail + 1, Ordering::Release); return Ok(()); } From 8903df8970454be368b00b867c668e53773df0eb Mon Sep 17 00:00:00 2001 From: cynecx Date: Tue, 11 Feb 2020 04:09:28 +0100 Subject: [PATCH 16/16] Use more precise unsafe scopes --- crossbeam-epoch/src/sync/queue.rs | 34 +++++++++++++++---------------- 1 file changed, 16 insertions(+), 18 deletions(-) diff --git a/crossbeam-epoch/src/sync/queue.rs b/crossbeam-epoch/src/sync/queue.rs index bb4c71db4..99fb6a1c4 100644 --- a/crossbeam-epoch/src/sync/queue.rs +++ b/crossbeam-epoch/src/sync/queue.rs @@ -142,24 +142,22 @@ impl Queue { let head = self.head.load(Acquire, guard); let h = unsafe { head.deref() }; let next = h.next.load(Acquire, guard); - unsafe { - match next.as_ref() { - Some(n) if condition(&*n.data.as_ptr()) => { - self.head - .compare_and_set(head, next, Release, guard) - .map(|_| { - let tail = self.tail.load(Relaxed, guard); - // Advance the tail so that we don't retire a pointer to a reachable node. - if head == tail { - let _ = self.tail.compare_and_set(tail, next, Release, guard); - } - guard.defer_destroy(head); - Some(n.data.as_ptr().read()) - }) - .map_err(|_| ()) - } - None | Some(_) => Ok(None), - } + match unsafe { next.as_ref() } { + Some(n) if condition(unsafe { &*n.data.as_ptr() }) => unsafe { + self.head + .compare_and_set(head, next, Release, guard) + .map(|_| { + let tail = self.tail.load(Relaxed, guard); + // Advance the tail so that we don't retire a pointer to a reachable node. + if head == tail { + let _ = self.tail.compare_and_set(tail, next, Release, guard); + } + guard.defer_destroy(head); + Some(n.data.as_ptr().read()) + }) + .map_err(|_| ()) + }, + None | Some(_) => Ok(None), } }