From 6b75e92afe174696bd00eaa8283ad9e3b1d01582 Mon Sep 17 00:00:00 2001 From: Ben Blum Date: Tue, 23 Jul 2013 17:17:34 -0400 Subject: [PATCH] UnsafeArc methods return unsafe pointers, so are not themselves unsafe. --- src/libextra/arc.rs | 6 +- src/libstd/rt/kill.rs | 20 ++-- src/libstd/unstable/sync.rs | 229 ++++++++++++++++++------------------ 3 files changed, 127 insertions(+), 128 deletions(-) diff --git a/src/libextra/arc.rs b/src/libextra/arc.rs index 9479e47ed8c..d4bf1d480ed 100644 --- a/src/libextra/arc.rs +++ b/src/libextra/arc.rs @@ -136,7 +136,7 @@ impl Arc { */ pub fn unwrap(self) -> T { let Arc { x: x } = self; - unsafe { x.unwrap() } + x.unwrap() } } @@ -250,7 +250,7 @@ impl MutexArc { */ pub fn unwrap(self) -> T { let MutexArc { x: x } = self; - let inner = unsafe { x.unwrap() }; + let inner = x.unwrap(); let MutexArcInner { failed: failed, data: data, _ } = inner; if failed { fail!(~"Can't unwrap poisoned MutexArc - another task failed inside!"); @@ -469,7 +469,7 @@ impl RWArc { */ pub fn unwrap(self) -> T { let RWArc { x: x, _ } = self; - let inner = unsafe { x.unwrap() }; + let inner = x.unwrap(); let RWArcInner { failed: failed, data: data, _ } = inner; if failed { fail!(~"Can't unwrap poisoned RWArc - another task failed inside!") diff --git a/src/libstd/rt/kill.rs b/src/libstd/rt/kill.rs index f7f11a402b8..e691bf51ea5 100644 --- a/src/libstd/rt/kill.rs +++ b/src/libstd/rt/kill.rs @@ -328,7 +328,7 @@ impl KillHandle { } // Try to see if all our children are gone already. - match unsafe { self.try_unwrap() } { + match self.try_unwrap() { // Couldn't unwrap; children still alive. Reparent entire handle as // our own tombstone, to be unwrapped later. Left(this) => { @@ -340,7 +340,7 @@ impl KillHandle { // Prefer to check tombstones that were there first, // being "more fair" at the expense of tail-recursion. others.take().map_consume_default(true, |f| f()) && { - let mut inner = unsafe { this.take().unwrap() }; + let mut inner = this.take().unwrap(); (!inner.any_child_failed) && inner.child_tombstones.take_map_default(true, |f| f()) } @@ -429,7 +429,7 @@ impl Death { do self.on_exit.take_map |on_exit| { if success { // We succeeded, but our children might not. Need to wait for them. - let mut inner = unsafe { self.kill_handle.take_unwrap().unwrap() }; + let mut inner = self.kill_handle.take_unwrap().unwrap(); if inner.any_child_failed { success = false; } else { @@ -555,7 +555,7 @@ mod test { // Without another handle to child, the try unwrap should succeed. child.reparent_children_to(&mut parent); - let mut parent_inner = unsafe { parent.unwrap() }; + let mut parent_inner = parent.unwrap(); assert!(parent_inner.child_tombstones.is_none()); assert!(parent_inner.any_child_failed == false); } @@ -570,7 +570,7 @@ mod test { child.notify_immediate_failure(); // Without another handle to child, the try unwrap should succeed. child.reparent_children_to(&mut parent); - let mut parent_inner = unsafe { parent.unwrap() }; + let mut parent_inner = parent.unwrap(); assert!(parent_inner.child_tombstones.is_none()); // Immediate failure should have been propagated. assert!(parent_inner.any_child_failed); @@ -592,7 +592,7 @@ mod test { // Otherwise, due to 'link', it would try to tombstone. child2.reparent_children_to(&mut parent); // Should successfully unwrap even though 'link' is still alive. - let mut parent_inner = unsafe { parent.unwrap() }; + let mut parent_inner = parent.unwrap(); assert!(parent_inner.child_tombstones.is_none()); // Immediate failure should have been propagated by first child. assert!(parent_inner.any_child_failed); @@ -611,7 +611,7 @@ mod test { // Let parent collect tombstones. util::ignore(link); // Must have created a tombstone - let mut parent_inner = unsafe { parent.unwrap() }; + let mut parent_inner = parent.unwrap(); assert!(parent_inner.child_tombstones.take_unwrap()()); assert!(parent_inner.any_child_failed == false); } @@ -630,7 +630,7 @@ mod test { // Let parent collect tombstones. util::ignore(link); // Must have created a tombstone - let mut parent_inner = unsafe { parent.unwrap() }; + let mut parent_inner = parent.unwrap(); // Failure must be seen in the tombstone. assert!(parent_inner.child_tombstones.take_unwrap()() == false); assert!(parent_inner.any_child_failed == false); @@ -650,7 +650,7 @@ mod test { // Let parent collect tombstones. util::ignore(link); // Must have created a tombstone - let mut parent_inner = unsafe { parent.unwrap() }; + let mut parent_inner = parent.unwrap(); assert!(parent_inner.child_tombstones.take_unwrap()()); assert!(parent_inner.any_child_failed == false); } @@ -671,7 +671,7 @@ mod test { // Let parent collect tombstones. util::ignore(link); // Must have created a tombstone - let mut parent_inner = unsafe { parent.unwrap() }; + let mut parent_inner = parent.unwrap(); // Failure must be seen in the tombstone. assert!(parent_inner.child_tombstones.take_unwrap()() == false); assert!(parent_inner.any_child_failed == false); diff --git a/src/libstd/unstable/sync.rs b/src/libstd/unstable/sync.rs index 16bbd33136c..4c52d897a72 100644 --- a/src/libstd/unstable/sync.rs +++ b/src/libstd/unstable/sync.rs @@ -93,114 +93,121 @@ impl UnsafeAtomicRcBox { } #[inline] - pub unsafe fn get(&self) -> *mut T - { - let mut data: ~AtomicRcBoxData = cast::transmute(self.data); - assert!(data.count.load(Acquire) > 0); // no barrier is really needed - let r: *mut T = data.data.get_mut_ref(); - cast::forget(data); - return r; + pub fn get(&self) -> *mut T { + unsafe { + let mut data: ~AtomicRcBoxData = cast::transmute(self.data); + // FIXME(#6598) Change Acquire to Relaxed. + assert!(data.count.load(Acquire) > 0); + let r: *mut T = data.data.get_mut_ref(); + cast::forget(data); + return r; + } } #[inline] - pub unsafe fn get_immut(&self) -> *T - { - let data: ~AtomicRcBoxData = cast::transmute(self.data); - assert!(data.count.load(Acquire) > 0); // no barrier is really needed - let r: *T = data.data.get_ref(); - cast::forget(data); - return r; + pub fn get_immut(&self) -> *T { + unsafe { + let data: ~AtomicRcBoxData = cast::transmute(self.data); + assert!(data.count.load(Acquire) > 0); // no barrier is really needed + let r: *T = data.data.get_ref(); + cast::forget(data); + return r; + } } /// Wait until all other handles are dropped, then retrieve the enclosed /// data. See extra::arc::Arc for specific semantics documentation. /// If called when the task is already unkillable, unwrap will unkillably /// block; otherwise, an unwrapping task can be killed by linked failure. - pub unsafe fn unwrap(self) -> T { + pub fn unwrap(self) -> T { let this = Cell::new(self); // argh do task::unkillable { - let mut this = this.take(); - let mut data: ~AtomicRcBoxData = cast::transmute(this.data); - // Set up the unwrap protocol. - let (p1,c1) = comm::oneshot(); // () - let (p2,c2) = comm::oneshot(); // bool - // Try to put our server end in the unwrapper slot. - // This needs no barrier -- it's protected by the release barrier on - // the xadd, and the acquire+release barrier in the destructor's xadd. - // FIXME(#6598) Change Acquire to Relaxed. - if data.unwrapper.fill(~(c1,p2), Acquire).is_none() { - // Got in. Tell this handle's destructor not to run (we are now it). - this.data = ptr::mut_null(); - // Drop our own reference. - let old_count = data.count.fetch_sub(1, Release); - assert!(old_count >= 1); - if old_count == 1 { - // We were the last owner. Can unwrap immediately. - // AtomicOption's destructor will free the server endpoint. - // FIXME(#3224): it should be like this - // let ~AtomicRcBoxData { data: user_data, _ } = data; - // user_data - data.data.take_unwrap() - } else { - // The *next* person who sees the refcount hit 0 will wake us. - let p1 = Cell::new(p1); // argh - // Unlike the above one, this cell is necessary. It will get - // taken either in the do block or in the finally block. - let c2_and_data = Cell::new((c2,data)); - do (|| { - do task::rekillable { p1.take().recv(); } - // Got here. Back in the 'unkillable' without getting killed. - let (c2, data) = c2_and_data.take(); - c2.send(true); + unsafe { + let mut this = this.take(); + let mut data: ~AtomicRcBoxData = cast::transmute(this.data); + // Set up the unwrap protocol. + let (p1,c1) = comm::oneshot(); // () + let (p2,c2) = comm::oneshot(); // bool + // Try to put our server end in the unwrapper slot. + // This needs no barrier -- it's protected by the release barrier on + // the xadd, and the acquire+release barrier in the destructor's xadd. + // FIXME(#6598) Change Acquire to Relaxed. + if data.unwrapper.fill(~(c1,p2), Acquire).is_none() { + // Got in. Tell this handle's destructor not to run (we are now it). + this.data = ptr::mut_null(); + // Drop our own reference. + let old_count = data.count.fetch_sub(1, Release); + assert!(old_count >= 1); + if old_count == 1 { + // We were the last owner. Can unwrap immediately. + // AtomicOption's destructor will free the server endpoint. // FIXME(#3224): it should be like this // let ~AtomicRcBoxData { data: user_data, _ } = data; // user_data - let mut data = data; data.data.take_unwrap() - }).finally { - if task::failing() { - // Killed during wait. Because this might happen while - // someone else still holds a reference, we can't free - // the data now; the "other" last refcount will free it. + } else { + // The *next* person who sees the refcount hit 0 will wake us. + let p1 = Cell::new(p1); // argh + // Unlike the above one, this cell is necessary. It will get + // taken either in the do block or in the finally block. + let c2_and_data = Cell::new((c2,data)); + do (|| { + do task::rekillable { p1.take().recv(); } + // Got here. Back in the 'unkillable' without getting killed. let (c2, data) = c2_and_data.take(); - c2.send(false); - cast::forget(data); - } else { - assert!(c2_and_data.is_empty()); + c2.send(true); + // FIXME(#3224): it should be like this + // let ~AtomicRcBoxData { data: user_data, _ } = data; + // user_data + let mut data = data; + data.data.take_unwrap() + }).finally { + if task::failing() { + // Killed during wait. Because this might happen while + // someone else still holds a reference, we can't free + // the data now; the "other" last refcount will free it. + let (c2, data) = c2_and_data.take(); + c2.send(false); + cast::forget(data); + } else { + assert!(c2_and_data.is_empty()); + } } } + } else { + // If 'put' returns the server end back to us, we were rejected; + // someone else was trying to unwrap. Avoid guaranteed deadlock. + cast::forget(data); + fail!("Another task is already unwrapping this Arc!"); } - } else { - // If 'put' returns the server end back to us, we were rejected; - // someone else was trying to unwrap. Avoid guaranteed deadlock. - cast::forget(data); - fail!("Another task is already unwrapping this Arc!"); } } } /// As unwrap above, but without blocking. Returns 'Left(self)' if this is /// not the last reference; 'Right(unwrapped_data)' if so. - pub unsafe fn try_unwrap(self) -> Either, T> { - let mut this = self; // FIXME(#4330) mutable self - let mut data: ~AtomicRcBoxData = cast::transmute(this.data); - // This can of course race with anybody else who has a handle, but in - // such a case, the returned count will always be at least 2. If we - // see 1, no race was possible. All that matters is 1 or not-1. - let count = data.count.load(Acquire); - assert!(count >= 1); - // The more interesting race is one with an unwrapper. They may have - // already dropped their count -- but if so, the unwrapper pointer - // will have been set first, which the barriers ensure we will see. - // (Note: using is_empty(), not take(), to not free the unwrapper.) - if count == 1 && data.unwrapper.is_empty(Acquire) { - // Tell this handle's destructor not to run (we are now it). - this.data = ptr::mut_null(); - // FIXME(#3224) as above - Right(data.data.take_unwrap()) - } else { - cast::forget(data); - Left(this) + pub fn try_unwrap(self) -> Either, T> { + unsafe { + let mut this = self; // FIXME(#4330) mutable self + let mut data: ~AtomicRcBoxData = cast::transmute(this.data); + // This can of course race with anybody else who has a handle, but in + // such a case, the returned count will always be at least 2. If we + // see 1, no race was possible. All that matters is 1 or not-1. + let count = data.count.load(Acquire); + assert!(count >= 1); + // The more interesting race is one with an unwrapper. They may have + // already dropped their count -- but if so, the unwrapper pointer + // will have been set first, which the barriers ensure we will see. + // (Note: using is_empty(), not take(), to not free the unwrapper.) + if count == 1 && data.unwrapper.is_empty(Acquire) { + // Tell this handle's destructor not to run (we are now it). + this.data = ptr::mut_null(); + // FIXME(#3224) as above + Right(data.data.take_unwrap()) + } else { + cast::forget(data); + Left(this) + } } } } @@ -370,7 +377,7 @@ impl Exclusive { pub fn unwrap(self) -> T { let Exclusive { x: x } = self; // Someday we might need to unkillably unwrap an Exclusive, but not today. - let inner = unsafe { x.unwrap() }; + let inner = x.unwrap(); let ExData { data: user_data, _ } = inner; // will destroy the LittleLock user_data } @@ -472,51 +479,43 @@ mod tests { #[test] fn arclike_unwrap_basic() { - unsafe { - let x = UnsafeAtomicRcBox::new(~~"hello"); - assert!(x.unwrap() == ~~"hello"); - } + let x = UnsafeAtomicRcBox::new(~~"hello"); + assert!(x.unwrap() == ~~"hello"); } #[test] fn arclike_try_unwrap() { - unsafe { - let x = UnsafeAtomicRcBox::new(~~"hello"); - assert!(x.try_unwrap().expect_right("try_unwrap failed") == ~~"hello"); - } + let x = UnsafeAtomicRcBox::new(~~"hello"); + assert!(x.try_unwrap().expect_right("try_unwrap failed") == ~~"hello"); } #[test] fn arclike_try_unwrap_fail() { - unsafe { - let x = UnsafeAtomicRcBox::new(~~"hello"); - let x2 = x.clone(); - let left_x = x.try_unwrap(); - assert!(left_x.is_left()); - util::ignore(left_x); - assert!(x2.try_unwrap().expect_right("try_unwrap none") == ~~"hello"); - } + let x = UnsafeAtomicRcBox::new(~~"hello"); + let x2 = x.clone(); + let left_x = x.try_unwrap(); + assert!(left_x.is_left()); + util::ignore(left_x); + assert!(x2.try_unwrap().expect_right("try_unwrap none") == ~~"hello"); } #[test] fn arclike_try_unwrap_unwrap_race() { // When an unwrap and a try_unwrap race, the unwrapper should always win. - unsafe { - let x = UnsafeAtomicRcBox::new(~~"hello"); - let x2 = Cell::new(x.clone()); - let (p,c) = comm::stream(); - do task::spawn { - c.send(()); - assert!(x2.take().unwrap() == ~~"hello"); - c.send(()); - } - p.recv(); - task::yield(); // Try to make the unwrapper get blocked first. - let left_x = x.try_unwrap(); - assert!(left_x.is_left()); - util::ignore(left_x); - p.recv(); + let x = UnsafeAtomicRcBox::new(~~"hello"); + let x2 = Cell::new(x.clone()); + let (p,c) = comm::stream(); + do task::spawn { + c.send(()); + assert!(x2.take().unwrap() == ~~"hello"); + c.send(()); } + p.recv(); + task::yield(); // Try to make the unwrapper get blocked first. + let left_x = x.try_unwrap(); + assert!(left_x.is_left()); + util::ignore(left_x); + p.recv(); } #[test]