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.

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

Add Shared pointer and have {Arc, Rc} use it #29110

Merged
merged 1 commit into from
Oct 17, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
11 changes: 6 additions & 5 deletions src/liballoc/arc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -79,9 +79,8 @@ use core::cmp::Ordering;
use core::mem::{align_of_val, size_of_val};
use core::intrinsics::{drop_in_place, abort};
use core::mem;
use core::nonzero::NonZero;
use core::ops::{Deref, CoerceUnsized};
use core::ptr;
use core::ptr::{self, Shared};
use core::marker::Unsize;
use core::hash::{Hash, Hasher};
use core::{usize, isize};
Expand Down Expand Up @@ -124,12 +123,13 @@ const MAX_REFCOUNT: usize = (isize::MAX) as usize;
pub struct Arc<T: ?Sized> {
// FIXME #12808: strange name to try to avoid interfering with
// field accesses of the contained type via Deref
_ptr: NonZero<*mut ArcInner<T>>,
_ptr: Shared<ArcInner<T>>,
}

unsafe impl<T: ?Sized + Sync + Send> Send for Arc<T> { }
unsafe impl<T: ?Sized + Sync + Send> Sync for Arc<T> { }

#[cfg(not(stage0))] // remove cfg after new snapshot
impl<T: ?Sized + Unsize<U>, U: ?Sized> CoerceUnsized<Arc<U>> for Arc<T> {}

/// A weak pointer to an `Arc`.
Expand All @@ -141,12 +141,13 @@ impl<T: ?Sized + Unsize<U>, U: ?Sized> CoerceUnsized<Arc<U>> for Arc<T> {}
pub struct Weak<T: ?Sized> {
// FIXME #12808: strange name to try to avoid interfering with
// field accesses of the contained type via Deref
_ptr: NonZero<*mut ArcInner<T>>,
_ptr: Shared<ArcInner<T>>,
}

unsafe impl<T: ?Sized + Sync + Send> Send for Weak<T> { }
unsafe impl<T: ?Sized + Sync + Send> Sync for Weak<T> { }

#[cfg(not(stage0))] // remove cfg after new snapshot
impl<T: ?Sized + Unsize<U>, U: ?Sized> CoerceUnsized<Weak<U>> for Weak<T> {}

#[stable(feature = "rust1", since = "1.0.0")]
Expand Down Expand Up @@ -190,7 +191,7 @@ impl<T> Arc<T> {
weak: atomic::AtomicUsize::new(1),
data: data,
};
Arc { _ptr: unsafe { NonZero::new(Box::into_raw(x)) } }
Arc { _ptr: unsafe { Shared::new(Box::into_raw(x)) } }
}

/// Unwraps the contained value if the `Arc<T>` has only one strong reference.
Expand Down
1 change: 1 addition & 0 deletions src/liballoc/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@
#![feature(placement_in_syntax)]
#![feature(placement_new_protocol)]
#![feature(raw)]
#![feature(shared)]
#![feature(staged_api)]
#![feature(unboxed_closures)]
#![feature(unique)]
Expand Down
11 changes: 6 additions & 5 deletions src/liballoc/rc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -163,9 +163,8 @@ use core::hash::{Hasher, Hash};
use core::intrinsics::{assume, drop_in_place, abort};
use core::marker::{self, Unsize};
use core::mem::{self, align_of_val, size_of_val, forget};
use core::nonzero::NonZero;
use core::ops::{CoerceUnsized, Deref};
use core::ptr;
use core::ptr::{self, Shared};

use heap::deallocate;

Expand All @@ -184,12 +183,13 @@ struct RcBox<T: ?Sized> {
pub struct Rc<T: ?Sized> {
// FIXME #12808: strange names to try to avoid interfering with field
// accesses of the contained type via Deref
_ptr: NonZero<*mut RcBox<T>>,
_ptr: Shared<RcBox<T>>,
}

impl<T: ?Sized> !marker::Send for Rc<T> {}
impl<T: ?Sized> !marker::Sync for Rc<T> {}

#[cfg(not(stage0))] // remove cfg after new snapshot
impl<T: ?Sized+Unsize<U>, U: ?Sized> CoerceUnsized<Rc<U>> for Rc<T> {}

impl<T> Rc<T> {
Expand All @@ -210,7 +210,7 @@ impl<T> Rc<T> {
// pointers, which ensures that the weak destructor never frees
// the allocation while the strong destructor is running, even
// if the weak pointer is stored inside the strong one.
_ptr: NonZero::new(Box::into_raw(box RcBox {
_ptr: Shared::new(Box::into_raw(box RcBox {
strong: Cell::new(1),
weak: Cell::new(1),
value: value,
Expand Down Expand Up @@ -712,12 +712,13 @@ impl<T> fmt::Pointer for Rc<T> {
pub struct Weak<T: ?Sized> {
// FIXME #12808: strange names to try to avoid interfering with
// field accesses of the contained type via Deref
_ptr: NonZero<*mut RcBox<T>>,
_ptr: Shared<RcBox<T>>,
}

impl<T: ?Sized> !marker::Send for Weak<T> {}
impl<T: ?Sized> !marker::Sync for Weak<T> {}

#[cfg(not(stage0))] // remove cfg after new snapshot
impl<T: ?Sized+Unsize<U>, U: ?Sized> CoerceUnsized<Weak<U>> for Weak<T> {}

impl<T: ?Sized> Weak<T> {
Expand Down
69 changes: 67 additions & 2 deletions src/libcore/ptr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,11 @@

use clone::Clone;
use intrinsics;
use ops::Deref;
use ops::{CoerceUnsized, Deref};
use fmt;
use hash;
use option::Option::{self, Some, None};
use marker::{PhantomData, Send, Sized, Sync};
use marker::{Copy, PhantomData, Send, Sized, Sync, Unsize};
use mem;
use nonzero::NonZero;

Expand Down Expand Up @@ -532,3 +532,68 @@ impl<T> fmt::Pointer for Unique<T> {
fmt::Pointer::fmt(&*self.pointer, f)
}
}

/// A wrapper around a raw `*mut T` that indicates that the possessor
/// of this wrapper has shared ownership of the referent. Useful for
/// building abstractions like `Rc<T>` or `Arc<T>`, which internally
/// use raw pointers to manage the memory that they own.
#[unstable(feature = "shared", reason = "needs an RFC to flesh out design",
issue = "0")]
pub struct Shared<T: ?Sized> {
pointer: NonZero<*const T>,
// NOTE: this marker has no consequences for variance, but is necessary
// for dropck to understand that we logically own a `T`.
//
// For details, see:
// https://github.com/rust-lang/rfcs/blob/master/text/0769-sound-generic-drop.md#phantom-data
_marker: PhantomData<T>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this not affect variance? Or are you saying that it has the same effect owning *const T above has, therefore it doesn't have additional consequences for variance?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the same note that is in Unique above.

}

/// `Shared` pointers are not `Send` because the data they reference may be aliased.
// NB: This impl is unnecessary, but should provide better error messages.
#[unstable(feature = "shared", issue = "0")]
impl<T: ?Sized> !Send for Shared<T> { }

/// `Shared` pointers are not `Sync` because the data they reference may be aliased.
// NB: This impl is unnecessary, but should provide better error messages.
#[unstable(feature = "shared", issue = "0")]
impl<T: ?Sized> !Sync for Shared<T> { }

#[unstable(feature = "shared", issue = "0")]
impl<T: ?Sized> Shared<T> {
/// Creates a new `Shared`.
pub unsafe fn new(ptr: *mut T) -> Self {
Shared { pointer: NonZero::new(ptr), _marker: PhantomData }
}
}

#[unstable(feature = "shared", issue = "0")]
impl<T: ?Sized> Clone for Shared<T> {
fn clone(&self) -> Self {
*self
}
}

#[unstable(feature = "shared", issue = "0")]
impl<T: ?Sized> Copy for Shared<T> { }

#[cfg(not(stage0))] // remove cfg after new snapshot
#[unstable(feature = "shared", issue = "0")]
impl<T: ?Sized, U: ?Sized> CoerceUnsized<Shared<U>> for Shared<T> where T: Unsize<U> { }

#[unstable(feature = "shared", issue = "0")]
impl<T: ?Sized> Deref for Shared<T> {
type Target = *mut T;

#[inline]
fn deref(&self) -> &*mut T {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This interface feels a little off to me, but since this isn't a stable API it can be bikeshedded later.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's the same interface exposed by Unique.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a bad interface because it removes the optimizations NonZero can trigger (the control-flow one, not the layout optimizations which are unaffected).
NonZero and wrappers such as Unique and Shared (which seem superfluous to me, btw, especially with all this extra boiler plating), should use a Cell-like API, allowing LLVM to reason about their non-nullability.

unsafe { mem::transmute(&*self.pointer) }
}
}

#[unstable(feature = "shared", issue = "0")]
impl<T> fmt::Pointer for Shared<T> {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
fmt::Pointer::fmt(&*self.pointer, f)
}
}
1 change: 1 addition & 0 deletions src/libstd/sync/mutex.rs
Original file line number Diff line number Diff line change
Expand Up @@ -293,6 +293,7 @@ impl<T: ?Sized> Mutex<T> {

#[stable(feature = "rust1", since = "1.0.0")]
impl<T: ?Sized> Drop for Mutex<T> {
#[unsafe_destructor_blind_to_params]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Were this and the rwlock changes needed for this PR? I have a feeling this probably isn't too bad, but I just want to make sure that this wasn't something super subtle that came up when fixing Rc and Arc

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aha I see it was needed to make the tests pass, makes sense to me!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These were necessary to keep the run-pass/dropck_legal_cycles.rs test passing. From what I understand, it was an oversight that the attribute wasn't applied in these locations when the same changes were made to the collection types' destructors in #28861.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah they are needed; otherwise the dropck-legal-cycles test won't pass. @apasel422 mentioned this on the other ticket, but not here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@apasel422 yeah the oversight was that I shouldn't have said "boy that was easy, didn't have to add that attribute in too many places"; I should have said "hmm, why are these tests passing currently without that attribute ... ooh, missing PhantomData"

fn drop(&mut self) {
// This is actually safe b/c we know that there is no further usage of
// this mutex (it's up to the user to arrange for a mutex to get
Expand Down
1 change: 1 addition & 0 deletions src/libstd/sync/rwlock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -314,6 +314,7 @@ impl<T: ?Sized> RwLock<T> {

#[stable(feature = "rust1", since = "1.0.0")]
impl<T: ?Sized> Drop for RwLock<T> {
#[unsafe_destructor_blind_to_params]
fn drop(&mut self) {
// IMPORTANT: This code needs to be kept in sync with `RwLock::into_inner`.
unsafe { self.inner.lock.destroy() }
Expand Down
34 changes: 34 additions & 0 deletions src/test/compile-fail/issue-29106.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
// Copyright 2015 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::rc::Rc;
use std::sync::Arc;

struct Foo<'a>(&'a String);

impl<'a> Drop for Foo<'a> {
fn drop(&mut self) {
println!("{:?}", self.0);
}
}

fn main() {
{
let (y, x);
x = "alive".to_string();
y = Arc::new(Foo(&x)); //~ ERROR `x` does not live long enough
}

{
let (y, x);
x = "alive".to_string();
y = Rc::new(Foo(&x)); //~ ERROR `x` does not live long enough
}
}
32 changes: 32 additions & 0 deletions src/test/run-pass/issue-29037.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
// Copyright 2015 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.

// This test ensures that each pointer type `P<X>` is covariant in `X`.

use std::rc::Rc;
use std::sync::Arc;

fn a<'r>(x: Box<&'static str>) -> Box<&'r str> {
x
}

fn b<'r, 'w>(x: &'w &'static str) -> &'w &'r str {
x
}

fn c<'r>(x: Arc<&'static str>) -> Arc<&'r str> {
x
}

fn d<'r>(x: Rc<&'static str>) -> Rc<&'r str> {
x
}

fn main() {}