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.

Sign up for GitHub

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

Try to avoid noalias attributes on intrusive linked lists #3654

Merged
merged 1 commit into from
Mar 29, 2021
Merged

Try to avoid noalias attributes on intrusive linked lists #3654

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
151 changes: 113 additions & 38 deletions tokio/src/util/linked_list.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,11 @@
//! structure's APIs are `unsafe` as they require the caller to ensure the
//! specified node is actually contained by the list.

use core::cell::UnsafeCell;
use core::fmt;
use core::marker::PhantomData;
use core::marker::{PhantomData, PhantomPinned};
use core::mem::ManuallyDrop;
use core::ptr::NonNull;
use core::ptr::{self, NonNull};

/// An intrusive linked list.
///
Expand Down Expand Up @@ -60,11 +61,40 @@ pub(crate) unsafe trait Link {

/// Previous / next pointers
pub(crate) struct Pointers<T> {
inner: UnsafeCell<PointersInner<T>>,
}
/// We do not want the compiler to put the `noalias` attribute on mutable
/// references to this type, so the type has been made `!Unpin` with a
/// `PhantomPinned` field.
///
/// Additionally, we never access the `prev` or `next` fields directly, as any
/// such access would implicitly involve the creation of a reference to the
/// field, which we want to avoid since the fields are not `!Unpin`, and would
/// hence be given the `noalias` attribute if we were to do such an access.
/// As an alternative to accessing the fields directly, the `Pointers` type
/// provides getters and setters for the two fields, and those are implemented
/// using raw pointer casts and offsets, which is valid since the struct is
/// #[repr(C)].
///
/// See this link for more information:
/// https://github.com/rust-lang/rust/pull/82834
#[repr(C)]
struct PointersInner<T> {
/// The previous node in the list. null if there is no previous node.
///
/// This field is accessed through pointer manipulation, so it is not dead code.
#[allow(dead_code)]
prev: Option<NonNull<T>>,

/// The next node in the list. null if there is no previous node.
///
/// This field is accessed through pointer manipulation, so it is not dead code.
#[allow(dead_code)]
next: Option<NonNull<T>>,

/// This type is !Unpin due to the heuristic from:
/// https://github.com/rust-lang/rust/pull/82834
_pin: PhantomPinned,
}

unsafe impl<T: Send> Send for Pointers<T> {}
Expand All @@ -91,11 +121,11 @@ impl<L: Link> LinkedList<L, L::Target> {
let ptr = L::as_raw(&*val);
assert_ne!(self.head, Some(ptr));
unsafe {
L::pointers(ptr).as_mut().next = self.head;
L::pointers(ptr).as_mut().prev = None;
L::pointers(ptr).as_mut().set_next(self.head);
L::pointers(ptr).as_mut().set_prev(None);

if let Some(head) = self.head {
L::pointers(head).as_mut().prev = Some(ptr);
L::pointers(head).as_mut().set_prev(Some(ptr));
}

self.head = Some(ptr);
Expand All @@ -111,16 +141,16 @@ impl<L: Link> LinkedList<L, L::Target> {
pub(crate) fn pop_back(&mut self) -> Option<L::Handle> {
unsafe {
let last = self.tail?;
self.tail = L::pointers(last).as_ref().prev;
self.tail = L::pointers(last).as_ref().get_prev();

if let Some(prev) = L::pointers(last).as_ref().prev {
L::pointers(prev).as_mut().next = None;
if let Some(prev) = L::pointers(last).as_ref().get_prev() {
L::pointers(prev).as_mut().set_next(None);
} else {
self.head = None
}

L::pointers(last).as_mut().prev = None;
L::pointers(last).as_mut().next = None;
L::pointers(last).as_mut().set_prev(None);
L::pointers(last).as_mut().set_next(None);

Some(L::from_raw(last))
}
Expand All @@ -143,31 +173,35 @@ impl<L: Link> LinkedList<L, L::Target> {
/// The caller **must** ensure that `node` is currently contained by
/// `self` or not contained by any other list.
pub(crate) unsafe fn remove(&mut self, node: NonNull<L::Target>) -> Option<L::Handle> {
if let Some(prev) = L::pointers(node).as_ref().prev {
debug_assert_eq!(L::pointers(prev).as_ref().next, Some(node));
L::pointers(prev).as_mut().next = L::pointers(node).as_ref().next;
if let Some(prev) = L::pointers(node).as_ref().get_prev() {
debug_assert_eq!(L::pointers(prev).as_ref().get_next(), Some(node));
L::pointers(prev)
.as_mut()
.set_next(L::pointers(node).as_ref().get_next());
} else {
if self.head != Some(node) {
return None;
}

self.head = L::pointers(node).as_ref().next;
self.head = L::pointers(node).as_ref().get_next();
}

if let Some(next) = L::pointers(node).as_ref().next {
debug_assert_eq!(L::pointers(next).as_ref().prev, Some(node));
L::pointers(next).as_mut().prev = L::pointers(node).as_ref().prev;
if let Some(next) = L::pointers(node).as_ref().get_next() {
debug_assert_eq!(L::pointers(next).as_ref().get_prev(), Some(node));
L::pointers(next)
.as_mut()
.set_prev(L::pointers(node).as_ref().get_prev());
} else {
// This might be the last item in the list
if self.tail != Some(node) {
return None;
}

self.tail = L::pointers(node).as_ref().prev;
self.tail = L::pointers(node).as_ref().get_prev();
}

L::pointers(node).as_mut().next = None;
L::pointers(node).as_mut().prev = None;
L::pointers(node).as_mut().set_next(None);
L::pointers(node).as_mut().set_prev(None);

Some(L::from_raw(node))
}
Expand Down Expand Up @@ -224,7 +258,7 @@ cfg_rt_multi_thread! {
fn next(&mut self) -> Option<&'a T::Target> {
let curr = self.curr?;
// safety: the pointer references data contained by the list
self.curr = unsafe { T::pointers(curr).as_ref() }.next;
self.curr = unsafe { T::pointers(curr).as_ref() }.get_next();

// safety: the value is still owned by the linked list.
Some(unsafe { &*curr.as_ptr() })
Expand Down Expand Up @@ -265,7 +299,7 @@ cfg_io_readiness! {
fn next(&mut self) -> Option<Self::Item> {
while let Some(curr) = self.curr {
// safety: the pointer references data contained by the list
self.curr = unsafe { T::pointers(curr).as_ref() }.next;
self.curr = unsafe { T::pointers(curr).as_ref() }.get_next();

// safety: the value is still owned by the linked list.
if (self.filter)(unsafe { &mut *curr.as_ptr() }) {
Expand All @@ -284,17 +318,58 @@ impl<T> Pointers<T> {
/// Create a new set of empty pointers
pub(crate) fn new() -> Pointers<T> {
Pointers {
prev: None,
next: None,
inner: UnsafeCell::new(PointersInner {
prev: None,
next: None,
_pin: PhantomPinned,
}),
}
}

fn get_prev(&self) -> Option<NonNull<T>> {
// SAFETY: prev is the first field in PointersInner, which is #[repr(C)].
unsafe {
let inner = self.inner.get();
let prev = inner as *const Option<NonNull<T>>;
ptr::read(prev)
}
}
fn get_next(&self) -> Option<NonNull<T>> {
// SAFETY: next is the second field in PointersInner, which is #[repr(C)].
unsafe {
let inner = self.inner.get();
let prev = inner as *const Option<NonNull<T>>;
let next = prev.add(1);
ptr::read(next)
}
}

fn set_prev(&mut self, value: Option<NonNull<T>>) {
// SAFETY: prev is the first field in PointersInner, which is #[repr(C)].
unsafe {
let inner = self.inner.get();
let prev = inner as *mut Option<NonNull<T>>;
ptr::write(prev, value);
}
}
fn set_next(&mut self, value: Option<NonNull<T>>) {
// SAFETY: next is the second field in PointersInner, which is #[repr(C)].
unsafe {
let inner = self.inner.get();
let prev = inner as *mut Option<NonNull<T>>;
let next = prev.add(1);
ptr::write(next, value);
}
}
}

impl<T> fmt::Debug for Pointers<T> {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
let prev = self.get_prev();
let next = self.get_next();
f.debug_struct("Pointers")
.field("prev", &self.prev)
.field("next", &self.next)
.field("prev", &prev)
.field("next", &next)
.finish()
}
}
Expand All @@ -321,7 +396,7 @@ mod tests {
}

unsafe fn from_raw(ptr: NonNull<Entry>) -> Pin<&'a Entry> {
Pin::new(&*ptr.as_ptr())
Pin::new_unchecked(&*ptr.as_ptr())
}

unsafe fn pointers(mut target: NonNull<Entry>) -> NonNull<Pointers<Entry>> {
Expand Down Expand Up @@ -361,8 +436,8 @@ mod tests {

macro_rules! assert_clean {
($e:ident) => {{
assert!($e.pointers.next.is_none());
assert!($e.pointers.prev.is_none());
assert!($e.pointers.get_next().is_none());
assert!($e.pointers.get_prev().is_none());
}};
}

Expand Down Expand Up @@ -460,8 +535,8 @@ mod tests {
assert_clean!(a);

assert_ptr_eq!(b, list.head);
assert_ptr_eq!(c, b.pointers.next);
assert_ptr_eq!(b, c.pointers.prev);
assert_ptr_eq!(c, b.pointers.get_next());
assert_ptr_eq!(b, c.pointers.get_prev());

let items = collect_list(&mut list);
assert_eq!([31, 7].to_vec(), items);
Expand All @@ -476,8 +551,8 @@ mod tests {
assert!(list.remove(ptr(&b)).is_some());
assert_clean!(b);

assert_ptr_eq!(c, a.pointers.next);
assert_ptr_eq!(a, c.pointers.prev);
assert_ptr_eq!(c, a.pointers.get_next());
assert_ptr_eq!(a, c.pointers.get_prev());

let items = collect_list(&mut list);
assert_eq!([31, 5].to_vec(), items);
Expand All @@ -493,7 +568,7 @@ mod tests {
assert!(list.remove(ptr(&c)).is_some());
assert_clean!(c);

assert!(b.pointers.next.is_none());
assert!(b.pointers.get_next().is_none());
assert_ptr_eq!(b, list.tail);

let items = collect_list(&mut list);
Expand All @@ -516,8 +591,8 @@ mod tests {
assert_ptr_eq!(b, list.head);
assert_ptr_eq!(b, list.tail);

assert!(b.pointers.next.is_none());
assert!(b.pointers.prev.is_none());
assert!(b.pointers.get_next().is_none());
assert!(b.pointers.get_prev().is_none());

let items = collect_list(&mut list);
assert_eq!([7].to_vec(), items);
Expand All @@ -536,8 +611,8 @@ mod tests {
assert_ptr_eq!(a, list.head);
assert_ptr_eq!(a, list.tail);

assert!(a.pointers.next.is_none());
assert!(a.pointers.prev.is_none());
assert!(a.pointers.get_next().is_none());
assert!(a.pointers.get_prev().is_none());

let items = collect_list(&mut list);
assert_eq!([5].to_vec(), items);
Expand Down