cancels of immediate rvalue cleanups are broken

[thestinger]

Original problem here, a simplified test case identifying the issue is in a comment below:

[unsafe_no_drop_flag] doesn't work on UnsafeArc if it is immediate

A failure is thrown by the runtime (and results in an uncaught C++ exception) if the type is changed to a newtype struct or if small struct types are treated as immediate.

[thestinger]

Nominating for the production ready milestone. This is an indicator of something seriously wrong in the cleanup code. Non-immediate types go down a separate path, and are always zeroed completely.

[thestinger]

I narrowed this down to a specific line in the code. The follow change works around the issue:

diff --git a/src/libstd/rt/kill.rs b/src/libstd/rt/kill.rs
index 6043ae3..93be953 100644
--- a/src/libstd/rt/kill.rs
+++ b/src/libstd/rt/kill.rs
@@ -580,7 +580,8 @@ 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 = self.kill_handle.take_unwrap().unwrap();
+                let tmp = self.kill_handle.take_unwrap();
+                let mut inner = tmp.unwrap();
                 if inner.any_child_failed {
                     success = false;
                 } else {
diff --git a/src/libstd/unstable/sync.rs b/src/libstd/unstable/sync.rs
index f3945d8..7505263 100644
--- a/src/libstd/unstable/sync.rs
+++ b/src/libstd/unstable/sync.rs
@@ -26,7 +26,7 @@ use vec;
 /// An atomically reference counted pointer.
 ///
 /// Enforces no shared-memory safety.
-//#[unsafe_no_drop_flag] FIXME: #9758
+#[unsafe_no_drop_flag]
 pub struct UnsafeArc<T> {
     data: *mut ArcData<T>,
 }

So this is caused by an rvalue having the destructor called despite being moved from.

[thestinger]

Simplified test case:

#[unsafe_no_drop_flag]
struct Foo {
    drop_flag: u32, generation: u32
}

impl Foo {
    fn take(&mut self) -> Foo {
        println!("(take) stealing from: {:?}", *self);
        let ret = Foo { drop_flag: self.drop_flag, generation: self.generation + 1 };
        self.drop_flag = 0;
        println!("(take) returning: {:?}", ret);
        ret
    }
}

impl Drop for Foo {
    fn drop(&mut self) {
        println!("destroying: {:?}", *self)
    }
}

fn main() {
    let mut x = Foo { drop_flag: 1, generation: 0 };
    let _y = x.take().take();
}

Output:

(take) stealing from: Foo{drop_flag: 1u32, generation: 0u32}
(take) returning: Foo{drop_flag: 1u32, generation: 1u32}
destroying: Foo{drop_flag: 0u32, generation: 0u32}
(take) stealing from: Foo{drop_flag: 1u32, generation: 1u32}
(take) returning: Foo{drop_flag: 1u32, generation: 2u32}
destroying: Foo{drop_flag: 0u32, generation: 0u32}
destroying: Foo{drop_flag: 1u32, generation: 1u32}
destroying: Foo{drop_flag: 1u32, generation: 2u32}
destroying: Foo{drop_flag: 0u32, generation: 0u32}

This line should not be there:

destroying: Foo{drop_flag: 1u32, generation: 1u32}

It is clearly being zeroed out in the take call.

[thestinger]

@metajack is running into a double-free caused by this bug or at least an incredibly similar one with this type in servo and I'm nearly sure the &mut ~5 bug is related.

[thestinger]

An even simpler one:

#[unsafe_no_drop_flag]
struct Foo {
    drop_flag: u32, generation: u32
}

impl Foo {
    fn take(&mut self) -> Foo {
        println!("(take) stealing from: {:?}", *self);
        let ret = Foo { drop_flag: self.drop_flag, generation: self.generation + 1 };
        self.drop_flag = 0;
        println!("(take) returning: {:?}", ret);
        ret
    }
}

impl Drop for Foo {
    fn drop(&mut self) {
        println!("destroying: {:?}", *self)
    }
}

fn main() {
    (Foo { drop_flag: 1, generation: 0 }).take();
}

(take) stealing from: Foo{drop_flag: 1u32, generation: 0u32}
(take) returning: Foo{drop_flag: 1u32, generation: 1u32}
destroying: Foo{drop_flag: 0u32, generation: 0u32}
destroying: Foo{drop_flag: 1u32, generation: 1u32}
destroying: Foo{drop_flag: 1u32, generation: 0u32}

I'll try to fix it, but even though the problem is obvious I don't really know this area of trans at all.

[catamorphism]

Fix for #7972 will close this, so, closing

[thestinger]

This didn't actually end up being fixed, despite my minimized test cases now working.

[emberian]

The minimized testcase doesn't work for me (assuming the fix landed, looks like it did):

(take) stealing from: Foo{drop_flag: 1u32, generation: 0u32}
(take) returning: Foo{drop_flag: 1u32, generation: 1u32}
destroying: Foo{drop_flag: 0u32, generation: 0u32}
destroying: Foo{drop_flag: 1u32, generation: 1u32}
destroying: Foo{drop_flag: 0u32, generation: 0u32}

@thestinger did you ever narrow this down any, trans-wise?

[thestinger]

@cmr: as stated in my previous comment, I fixed the minimized test cases I created. The problem still exists, as you can see by adding #[unsafe_no_drop_flag] to UnsafeArc and not changing the line I identified as causing the issue.

[nikomatsakis]

cc me -- this is related to my work on #3511

[nikomatsakis]

in particular I removed the whole notion of canceling a cleanup

[alexcrichton]

Closing, all tests in this bug work, unsafe_no_drop_flag exists on UnsafeArc, and #11585 will land soon making this issue moot.

[emberian]

Does the testsuite cover it?

On Thu, Jan 16, 2014 at 4:00 PM, Alex Crichton notifications@github.comwrote:

Closed #9758 #9758.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/9758
.

[alexcrichton]

It can't really because there's no knowledge any more of what this bug even was. All evidence of the bug has been fixed and a minimized test case was never found.

[emberian]

ok sure

On Thu, Jan 16, 2014 at 4:04 PM, Alex Crichton notifications@github.comwrote:

It can't really because there's no knowledge any more of what this bug
even was. All evidence of the bug has been fixed and a minimized test case
was never found.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/9758#issuecomment-32546644
.