Skip to content

Commit

Permalink
Auto merge of #51901 - rust-lang:weak-unboxing, r=alexcrichton
Browse files Browse the repository at this point in the history
Rc: remove unused allocation and fix segfault in Weak::new()

Same as #50357 did for `Arc`.

Fixes #48493
  • Loading branch information
bors committed Jul 6, 2018
2 parents e06c875 + 96d1f27 commit 3d553bd
Show file tree
Hide file tree
Showing 6 changed files with 192 additions and 44 deletions.
59 changes: 36 additions & 23 deletions src/liballoc/rc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -253,14 +253,15 @@ use core::hash::{Hash, Hasher};
use core::intrinsics::abort;
use core::marker;
use core::marker::{Unsize, PhantomData};
use core::mem::{self, align_of_val, forget, size_of_val, uninitialized};
use core::mem::{self, align_of_val, forget, size_of_val};
use core::ops::Deref;
use core::ops::CoerceUnsized;
use core::ptr::{self, NonNull};
use core::convert::From;

use alloc::{Global, Alloc, Layout, box_free, handle_alloc_error};
use string::String;
use sync::is_dangling;
use vec::Vec;

struct RcBox<T: ?Sized> {
Expand Down Expand Up @@ -1153,6 +1154,10 @@ impl<T> From<Vec<T>> for Rc<[T]> {
/// [`None`]: ../../std/option/enum.Option.html#variant.None
#[stable(feature = "rc_weak", since = "1.4.0")]
pub struct Weak<T: ?Sized> {
// This is a `NonNull` to allow optimizing the size of this type in enums,
// but it is not necessarily a valid pointer.
// `Weak::new` sets this to a dangling pointer so that it doesn’t need
// to allocate space on the heap.
ptr: NonNull<RcBox<T>>,
}

Expand All @@ -1165,8 +1170,8 @@ impl<T: ?Sized> !marker::Sync for Weak<T> {}
impl<T: ?Sized + Unsize<U>, U: ?Sized> CoerceUnsized<Weak<U>> for Weak<T> {}

impl<T> Weak<T> {
/// Constructs a new `Weak<T>`, allocating memory for `T` without initializing
/// it. Calling [`upgrade`] on the return value always gives [`None`].
/// Constructs a new `Weak<T>`, without allocating any memory.
/// Calling [`upgrade`] on the return value always gives [`None`].
///
/// [`upgrade`]: struct.Weak.html#method.upgrade
/// [`None`]: ../../std/option/enum.Option.html
Expand All @@ -1181,14 +1186,8 @@ impl<T> Weak<T> {
/// ```
#[stable(feature = "downgraded_weak", since = "1.10.0")]
pub fn new() -> Weak<T> {
unsafe {
Weak {
ptr: Box::into_raw_non_null(box RcBox {
strong: Cell::new(0),
weak: Cell::new(1),
value: uninitialized(),
}),
}
Weak {
ptr: NonNull::dangling(),
}
}
}
Expand Down Expand Up @@ -1222,13 +1221,25 @@ impl<T: ?Sized> Weak<T> {
/// ```
#[stable(feature = "rc_weak", since = "1.4.0")]
pub fn upgrade(&self) -> Option<Rc<T>> {
if self.strong() == 0 {
let inner = self.inner()?;
if inner.strong() == 0 {
None
} else {
self.inc_strong();
inner.inc_strong();
Some(Rc { ptr: self.ptr, phantom: PhantomData })
}
}

/// Return `None` when the pointer is dangling and there is no allocated `RcBox`,
/// i.e. this `Weak` was created by `Weak::new`
#[inline]
fn inner(&self) -> Option<&RcBox<T>> {
if is_dangling(self.ptr) {
None
} else {
Some(unsafe { self.ptr.as_ref() })
}
}
}

#[stable(feature = "rc_weak", since = "1.4.0")]
Expand Down Expand Up @@ -1258,12 +1269,14 @@ impl<T: ?Sized> Drop for Weak<T> {
/// assert!(other_weak_foo.upgrade().is_none());
/// ```
fn drop(&mut self) {
unsafe {
self.dec_weak();
if let Some(inner) = self.inner() {
inner.dec_weak();
// the weak count starts at 1, and will only go to zero if all
// the strong pointers have disappeared.
if self.weak() == 0 {
Global.dealloc(self.ptr.cast(), Layout::for_value(self.ptr.as_ref()));
if inner.weak() == 0 {
unsafe {
Global.dealloc(self.ptr.cast(), Layout::for_value(self.ptr.as_ref()));
}
}
}
}
Expand All @@ -1284,7 +1297,9 @@ impl<T: ?Sized> Clone for Weak<T> {
/// ```
#[inline]
fn clone(&self) -> Weak<T> {
self.inc_weak();
if let Some(inner) = self.inner() {
inner.inc_weak()
}
Weak { ptr: self.ptr }
}
}
Expand Down Expand Up @@ -1317,7 +1332,7 @@ impl<T> Default for Weak<T> {
}
}

// NOTE: We checked_add here to deal with mem::forget safety. In particular
// NOTE: We checked_add here to deal with mem::forget safely. In particular
// if you mem::forget Rcs (or Weaks), the ref-count can overflow, and then
// you can free the allocation while outstanding Rcs (or Weaks) exist.
// We abort because this is such a degenerate scenario that we don't care about
Expand Down Expand Up @@ -1370,12 +1385,10 @@ impl<T: ?Sized> RcBoxPtr<T> for Rc<T> {
}
}

impl<T: ?Sized> RcBoxPtr<T> for Weak<T> {
impl<T: ?Sized> RcBoxPtr<T> for RcBox<T> {
#[inline(always)]
fn inner(&self) -> &RcBox<T> {
unsafe {
self.ptr.as_ref()
}
self
}
}

Expand Down
50 changes: 29 additions & 21 deletions src/liballoc/sync.rs
Original file line number Diff line number Diff line change
Expand Up @@ -43,9 +43,6 @@ use vec::Vec;
/// necessarily) at _exactly_ `MAX_REFCOUNT + 1` references.
const MAX_REFCOUNT: usize = (isize::MAX) as usize;

/// A sentinel value that is used for the pointer of `Weak::new()`.
const WEAK_EMPTY: usize = 1;

/// A thread-safe reference-counting pointer. 'Arc' stands for 'Atomically
/// Reference Counted'.
///
Expand Down Expand Up @@ -239,9 +236,9 @@ impl<T: ?Sized + Unsize<U>, U: ?Sized> CoerceUnsized<Arc<U>> for Arc<T> {}
#[stable(feature = "arc_weak", since = "1.4.0")]
pub struct Weak<T: ?Sized> {
// This is a `NonNull` to allow optimizing the size of this type in enums,
// but it is actually not truly "non-null". A `Weak::new()` will set this
// to a sentinel value, instead of needing to allocate some space in the
// heap.
// but it is not necessarily a valid pointer.
// `Weak::new` sets this to a dangling pointer so that it doesn’t need
// to allocate space on the heap.
ptr: NonNull<ArcInner<T>>,
}

Expand Down Expand Up @@ -1035,14 +1032,18 @@ impl<T> Weak<T> {
/// ```
#[stable(feature = "downgraded_weak", since = "1.10.0")]
pub fn new() -> Weak<T> {
unsafe {
Weak {
ptr: NonNull::new_unchecked(WEAK_EMPTY as *mut _),
}
Weak {
ptr: NonNull::dangling(),
}
}
}

pub(crate) fn is_dangling<T: ?Sized>(ptr: NonNull<T>) -> bool {
let address = ptr.as_ptr() as *mut () as usize;
let align = align_of_val(unsafe { ptr.as_ref() });
address == align
}

impl<T: ?Sized> Weak<T> {
/// Attempts to upgrade the `Weak` pointer to an [`Arc`], extending
/// the lifetime of the value if successful.
Expand Down Expand Up @@ -1074,11 +1075,7 @@ impl<T: ?Sized> Weak<T> {
pub fn upgrade(&self) -> Option<Arc<T>> {
// We use a CAS loop to increment the strong count instead of a
// fetch_add because once the count hits 0 it must never be above 0.
let inner = if self.ptr.as_ptr() as *const u8 as usize == WEAK_EMPTY {
return None;
} else {
unsafe { self.ptr.as_ref() }
};
let inner = self.inner()?;

// Relaxed load because any write of 0 that we can observe
// leaves the field in a permanently zero state (so a
Expand Down Expand Up @@ -1109,6 +1106,17 @@ impl<T: ?Sized> Weak<T> {
}
}
}

/// Return `None` when the pointer is dangling and there is no allocated `ArcInner`,
/// i.e. this `Weak` was created by `Weak::new`
#[inline]
fn inner(&self) -> Option<&ArcInner<T>> {
if is_dangling(self.ptr) {
None
} else {
Some(unsafe { self.ptr.as_ref() })
}
}
}

#[stable(feature = "arc_weak", since = "1.4.0")]
Expand All @@ -1126,10 +1134,10 @@ impl<T: ?Sized> Clone for Weak<T> {
/// ```
#[inline]
fn clone(&self) -> Weak<T> {
let inner = if self.ptr.as_ptr() as *const u8 as usize == WEAK_EMPTY {
return Weak { ptr: self.ptr };
let inner = if let Some(inner) = self.inner() {
inner
} else {
unsafe { self.ptr.as_ref() }
return Weak { 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
Expand Down Expand Up @@ -1204,10 +1212,10 @@ impl<T: ?Sized> Drop for Weak<T> {
// weak count can only be locked if there was precisely one weak ref,
// meaning that drop could only subsequently run ON that remaining weak
// ref, which can only happen after the lock is released.
let inner = if self.ptr.as_ptr() as *const u8 as usize == WEAK_EMPTY {
return;
let inner = if let Some(inner) = self.inner() {
inner
} else {
unsafe { self.ptr.as_ref() }
return
};

if inner.weak.fetch_sub(1, Release) == 1 {
Expand Down
55 changes: 55 additions & 0 deletions src/liballoc/tests/arc.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
// Copyright 2018 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution and at
// http://rust-lang.org/COPYRIGHT.
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

use std::any::Any;
use std::sync::{Arc, Weak};

#[test]
fn uninhabited() {
enum Void {}
let mut a = Weak::<Void>::new();
a = a.clone();
assert!(a.upgrade().is_none());

let mut a: Weak<Any> = a; // Unsizing
a = a.clone();
assert!(a.upgrade().is_none());
}

#[test]
fn slice() {
let a: Arc<[u32; 3]> = Arc::new([3, 2, 1]);
let a: Arc<[u32]> = a; // Unsizing
let b: Arc<[u32]> = Arc::from(&[3, 2, 1][..]); // Conversion
assert_eq!(a, b);

// Exercise is_dangling() with a DST
let mut a = Arc::downgrade(&a);
a = a.clone();
assert!(a.upgrade().is_some());
}

#[test]
fn trait_object() {
let a: Arc<u32> = Arc::new(4);
let a: Arc<Any> = a; // Unsizing

// Exercise is_dangling() with a DST
let mut a = Arc::downgrade(&a);
a = a.clone();
assert!(a.upgrade().is_some());

let mut b = Weak::<u32>::new();
b = b.clone();
assert!(b.upgrade().is_none());
let mut b: Weak<Any> = b; // Unsizing
b = b.clone();
assert!(b.upgrade().is_none());
}
2 changes: 2 additions & 0 deletions src/liballoc/tests/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,12 +32,14 @@ extern crate rand;
use std::hash::{Hash, Hasher};
use std::collections::hash_map::DefaultHasher;

mod arc;
mod binary_heap;
mod btree;
mod cow_str;
mod fmt;
mod heap;
mod linked_list;
mod rc;
mod slice;
mod str;
mod string;
Expand Down
55 changes: 55 additions & 0 deletions src/liballoc/tests/rc.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
// Copyright 2018 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution and at
// http://rust-lang.org/COPYRIGHT.
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

use std::any::Any;
use std::rc::{Rc, Weak};

#[test]
fn uninhabited() {
enum Void {}
let mut a = Weak::<Void>::new();
a = a.clone();
assert!(a.upgrade().is_none());

let mut a: Weak<Any> = a; // Unsizing
a = a.clone();
assert!(a.upgrade().is_none());
}

#[test]
fn slice() {
let a: Rc<[u32; 3]> = Rc::new([3, 2, 1]);
let a: Rc<[u32]> = a; // Unsizing
let b: Rc<[u32]> = Rc::from(&[3, 2, 1][..]); // Conversion
assert_eq!(a, b);

// Exercise is_dangling() with a DST
let mut a = Rc::downgrade(&a);
a = a.clone();
assert!(a.upgrade().is_some());
}

#[test]
fn trait_object() {
let a: Rc<u32> = Rc::new(4);
let a: Rc<Any> = a; // Unsizing

// Exercise is_dangling() with a DST
let mut a = Rc::downgrade(&a);
a = a.clone();
assert!(a.upgrade().is_some());

let mut b = Weak::<u32>::new();
b = b.clone();
assert!(b.upgrade().is_none());
let mut b: Weak<Any> = b; // Unsizing
b = b.clone();
assert!(b.upgrade().is_none());
}
15 changes: 15 additions & 0 deletions src/test/run-pass/weak-new-uninhabited-issue-48493.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// Copyright 2018 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution and at
// http://rust-lang.org/COPYRIGHT.
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

fn main() {
enum Void {}
std::rc::Weak::<Void>::new();
std::sync::Weak::<Void>::new();
}

0 comments on commit 3d553bd

Please sign in to comment.