Auto merge of #51901 - rust-lang:weak-unboxing, r=alexcrichton

Rc: remove unused allocation and fix segfault in Weak::new()

Same as #50357 did for `Arc`.

Fixes #48493

Author: bors

src/liballoc/rc.rs

@@ -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> {
@@ -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>>,
 }
 
@@ -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
@@ -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(),
         }
     }
 }
@@ -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")]
@@ -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()));
+                }
             }
         }
     }
@@ -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 }
     }
 }
@@ -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
@@ -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
     }
 }
 

src/liballoc/sync.rs

@@ -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'.
 ///
@@ -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>>,
 }
 
@@ -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.
@@ -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
@@ -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")]
@@ -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
@@ -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 {

src/liballoc/tests/arc.rs

@@ -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());
+}

src/liballoc/tests/lib.rs

@@ -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;

src/liballoc/tests/rc.rs

@@ -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());
+}

src/test/run-pass/weak-new-uninhabited-issue-48493.rs

@@ -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();
+}