From cd92d2c77fe420568045bdc41b4acc4265408eb7 Mon Sep 17 00:00:00 2001 From: Flaper Fesp Date: Sun, 18 Aug 2013 00:51:39 +0200 Subject: [PATCH 1/6] Make rekillable consistent with unkillable As for now, rekillable is an unsafe function, instead, it should behave just like unkillable by encapsulating unsafe code within an unsafe block. This patch does that and removes unsafe blocks that were encapsulating rekillable calls throughout rust's libs. Fixes #8232 --- src/libextra/sync.rs | 48 +++++++++++++++++------------------------- src/libstd/task/mod.rs | 43 +++++++++++++++++++++++-------------- 2 files changed, 46 insertions(+), 45 deletions(-) diff --git a/src/libextra/sync.rs b/src/libextra/sync.rs index afb4cf3943aba..1952c35eb9df2 100644 --- a/src/libextra/sync.rs +++ b/src/libextra/sync.rs @@ -135,9 +135,7 @@ impl Sem { do task::unkillable { do (|| { self.acquire(); - unsafe { - do task::rekillable { blk() } - } + do task::rekillable { blk() } }).finally { self.release(); } @@ -234,10 +232,8 @@ impl<'self> Condvar<'self> { // signaller already sent -- I mean 'unconditionally' in contrast // with acquire().) do (|| { - unsafe { - do task::rekillable { - let _ = WaitEnd.take_unwrap().recv(); - } + do task::rekillable { + let _ = WaitEnd.take_unwrap().recv(); } }).finally { // Reacquire the condvar. Note this is back in the unkillable @@ -516,14 +512,12 @@ impl RWLock { * 'write' from other tasks will run concurrently with this one. */ pub fn write(&self, blk: &fn() -> U) -> U { - unsafe { - do task::unkillable { - (&self.order_lock).acquire(); - do (&self.access_lock).access { - (&self.order_lock).release(); - do task::rekillable { - blk() - } + do task::unkillable { + (&self.order_lock).acquire(); + do (&self.access_lock).access { + (&self.order_lock).release(); + do task::rekillable { + blk() } } } @@ -562,16 +556,14 @@ impl RWLock { // which can't happen until T2 finishes the downgrade-read entirely. // The astute reader will also note that making waking writers use the // order_lock is better for not starving readers. - unsafe { - do task::unkillable { - (&self.order_lock).acquire(); - do (&self.access_lock).access_cond |cond| { - (&self.order_lock).release(); - do task::rekillable { - let opt_lock = Just(&self.order_lock); - blk(&Condvar { sem: cond.sem, order: opt_lock, - token: NonCopyable::new() }) - } + do task::unkillable { + (&self.order_lock).acquire(); + do (&self.access_lock).access_cond |cond| { + (&self.order_lock).release(); + do task::rekillable { + let opt_lock = Just(&self.order_lock); + blk(&Condvar { sem: cond.sem, order: opt_lock, + token: NonCopyable::new() }) } } } @@ -606,10 +598,8 @@ impl RWLock { (&self.access_lock).acquire(); (&self.order_lock).release(); do (|| { - unsafe { - do task::rekillable { - blk(RWLockWriteMode { lock: self, token: NonCopyable::new() }) - } + do task::rekillable { + blk(RWLockWriteMode { lock: self, token: NonCopyable::new() }) } }).finally { let writer_or_last_reader; diff --git a/src/libstd/task/mod.rs b/src/libstd/task/mod.rs index e76b81a904df2..bac4991d858ff 100644 --- a/src/libstd/task/mod.rs +++ b/src/libstd/task/mod.rs @@ -597,21 +597,34 @@ pub fn unkillable(f: &fn() -> U) -> U { } } -/// The inverse of unkillable. Only ever to be used nested in unkillable(). -pub unsafe fn rekillable(f: &fn() -> U) -> U { +/** + * Makes killable a task marked as unkillable + * + * # Example + * + * ~~~ + * do task::unkillable { + * do task::rekillable { + * // Task is killable + * } + * } + */ +pub fn rekillable(f: &fn() -> U) -> U { use rt::task::Task; - if in_green_task_context() { - let t = Local::unsafe_borrow::(); - do (|| { - (*t).death.allow_kill((*t).unwinder.unwinding); + unsafe { + if in_green_task_context() { + let t = Local::unsafe_borrow::(); + do (|| { + (*t).death.allow_kill((*t).unwinder.unwinding); + f() + }).finally { + (*t).death.inhibit_kill((*t).unwinder.unwinding); + } + } else { + // FIXME(#3095): As in unkillable(). f() - }).finally { - (*t).death.inhibit_kill((*t).unwinder.unwinding); } - } else { - // FIXME(#3095): As in unkillable(). - f() } } @@ -646,11 +659,9 @@ fn test_kill_rekillable_task() { do run_in_newsched_task { do task::try { do task::unkillable { - unsafe { - do task::rekillable { - do task::spawn { - fail!(); - } + do task::rekillable { + do task::spawn { + fail!(); } } } From c4093b4a83dc1660257c88ca7834c46530010985 Mon Sep 17 00:00:00 2001 From: Flaper Fesp Date: Sun, 18 Aug 2013 18:07:20 +0200 Subject: [PATCH 2/6] Testing rekillable fails when called from outside an unkillable block --- src/libstd/task/mod.rs | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/src/libstd/task/mod.rs b/src/libstd/task/mod.rs index bac4991d858ff..c66296723b10f 100644 --- a/src/libstd/task/mod.rs +++ b/src/libstd/task/mod.rs @@ -598,7 +598,8 @@ pub fn unkillable(f: &fn() -> U) -> U { } /** - * Makes killable a task marked as unkillable + * Makes killable a task marked as unkillable. This + * is meant to be used only nested in unkillable. * * # Example * @@ -607,6 +608,7 @@ pub fn unkillable(f: &fn() -> U) -> U { * do task::rekillable { * // Task is killable * } + * // Task is unkillable again * } */ pub fn rekillable(f: &fn() -> U) -> U { @@ -1237,6 +1239,20 @@ fn test_unkillable_nested() { po.recv(); } +#[ignore(reason = "linked failure")] +#[test] +#[ignore(cfg(windows))] +#[should_fail] +fn test_rekillable_not_nested() { + do rekillable { + // This should fail before + // receiving anything since + // this block should be nested + // into a unkillable block. + yield(); + } +} + #[test] fn test_child_doesnt_ref_parent() { // If the child refcounts the parent task, this will stack overflow when From 5fc4045d789ed752c49b9368a2cc9a1cd8c341d5 Mon Sep 17 00:00:00 2001 From: Flaper Fesp Date: Tue, 20 Aug 2013 01:26:05 +0200 Subject: [PATCH 3/6] Don't make the runtime exit on illegal calls --- src/libstd/rt/kill.rs | 4 +++- src/libstd/task/mod.rs | 48 +++++++++++++++++++++++++++++------------- src/llvm | 2 +- 3 files changed, 37 insertions(+), 17 deletions(-) diff --git a/src/libstd/rt/kill.rs b/src/libstd/rt/kill.rs index b0b425e3aee4a..afef699390b74 100644 --- a/src/libstd/rt/kill.rs +++ b/src/libstd/rt/kill.rs @@ -647,7 +647,9 @@ impl Death { /// All calls must be paired with a preceding call to inhibit_kill. #[inline] pub fn allow_kill(&mut self, already_failing: bool) { - rtassert!(self.unkillable != 0); + if self.unkillable == 0 { + fail!("illegal call of rekillable"); + } self.unkillable -= 1; if self.unkillable == 0 { rtassert!(self.kill_handle.is_some()); diff --git a/src/libstd/task/mod.rs b/src/libstd/task/mod.rs index c66296723b10f..d094623b57ba3 100644 --- a/src/libstd/task/mod.rs +++ b/src/libstd/task/mod.rs @@ -671,7 +671,39 @@ fn test_kill_rekillable_task() { } } -#[test] #[should_fail] +#[test] +#[ignore(cfg(windows))] +#[should_fail] +fn test_rekillable_not_nested() { + do rekillable { + // This should fail before + // receiving anything since + // this block should be nested + // into a unkillable block. + yield(); + } +} + + +#[test] +#[ignore(cfg(windows))] +fn test_rekillable_nested_failure() { + + let result = do task::try { + do unkillable { + do rekillable { + let (port,chan) = comm::stream(); + do task::spawn { chan.send(()); fail!(); } + port.recv(); // wait for child to exist + port.recv(); // block forever, expect to get killed. + } + } + }; + assert!(result.is_err()); +} + + +#[test] #[should_fail] #[ignore(cfg(windows))] fn test_cant_dup_task_builder() { let mut builder = task(); builder.unlinked(); @@ -1239,20 +1271,6 @@ fn test_unkillable_nested() { po.recv(); } -#[ignore(reason = "linked failure")] -#[test] -#[ignore(cfg(windows))] -#[should_fail] -fn test_rekillable_not_nested() { - do rekillable { - // This should fail before - // receiving anything since - // this block should be nested - // into a unkillable block. - yield(); - } -} - #[test] fn test_child_doesnt_ref_parent() { // If the child refcounts the parent task, this will stack overflow when diff --git a/src/llvm b/src/llvm index 0964c68ddf2c6..f67442eee27d3 160000 --- a/src/llvm +++ b/src/llvm @@ -1 +1 @@ -Subproject commit 0964c68ddf2c67ce455e7443a06f4bb3db9e92bb +Subproject commit f67442eee27d3d075a65cf7f9a70f7ec6649ffd1 From 5e80e0cbf415219bb4fb483a24bd8cc03836d315 Mon Sep 17 00:00:00 2001 From: Flaper Fesp Date: Tue, 20 Aug 2013 02:10:53 +0200 Subject: [PATCH 4/6] Decrement unkillable counter before failing --- src/libstd/rt/kill.rs | 4 +++- src/libstd/task/mod.rs | 4 ++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/src/libstd/rt/kill.rs b/src/libstd/rt/kill.rs index afef699390b74..94df621ce7666 100644 --- a/src/libstd/rt/kill.rs +++ b/src/libstd/rt/kill.rs @@ -648,7 +648,9 @@ impl Death { #[inline] pub fn allow_kill(&mut self, already_failing: bool) { if self.unkillable == 0 { - fail!("illegal call of rekillable"); + // we need to decrement the counter before failing. + self.unkillable -= 1; + fail!("Cannot enter a rekillable() block without a surrounding unkillable()"); } self.unkillable -= 1; if self.unkillable == 0 { diff --git a/src/libstd/task/mod.rs b/src/libstd/task/mod.rs index d094623b57ba3..74e1af540c0a9 100644 --- a/src/libstd/task/mod.rs +++ b/src/libstd/task/mod.rs @@ -651,8 +651,8 @@ fn test_kill_unkillable_task() { } } -#[ignore(reason = "linked failure")] #[test] +#[ignore(cfg(windows))] fn test_kill_rekillable_task() { use rt::test::*; @@ -672,8 +672,8 @@ fn test_kill_rekillable_task() { } #[test] -#[ignore(cfg(windows))] #[should_fail] +#[ignore(cfg(windows))] fn test_rekillable_not_nested() { do rekillable { // This should fail before From df84925b45ca32c40430a114449066aebc58508d Mon Sep 17 00:00:00 2001 From: Flaper Fesp Date: Tue, 27 Aug 2013 09:53:31 +0200 Subject: [PATCH 5/6] Rebased and replaced yield with deschedule --- src/libstd/task/mod.rs | 2 +- src/llvm | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libstd/task/mod.rs b/src/libstd/task/mod.rs index 74e1af540c0a9..b1439fd763d67 100644 --- a/src/libstd/task/mod.rs +++ b/src/libstd/task/mod.rs @@ -680,7 +680,7 @@ fn test_rekillable_not_nested() { // receiving anything since // this block should be nested // into a unkillable block. - yield(); + deschedule(); } } diff --git a/src/llvm b/src/llvm index f67442eee27d3..0964c68ddf2c6 160000 --- a/src/llvm +++ b/src/llvm @@ -1 +1 @@ -Subproject commit f67442eee27d3d075a65cf7f9a70f7ec6649ffd1 +Subproject commit 0964c68ddf2c67ce455e7443a06f4bb3db9e92bb From cc59d9609740bd325a796ba0831af15b87f502ed Mon Sep 17 00:00:00 2001 From: Flaper Fesp Date: Tue, 27 Aug 2013 20:09:57 +0200 Subject: [PATCH 6/6] Trailing space --- src/libstd/task/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libstd/task/mod.rs b/src/libstd/task/mod.rs index b1439fd763d67..f872c2614b9f6 100644 --- a/src/libstd/task/mod.rs +++ b/src/libstd/task/mod.rs @@ -696,7 +696,7 @@ fn test_rekillable_nested_failure() { do task::spawn { chan.send(()); fail!(); } port.recv(); // wait for child to exist port.recv(); // block forever, expect to get killed. - } + } } }; assert!(result.is_err());