Skip to content

miri doesn't drop values moved into closures with let _ = x #135

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

Closed
oli-obk opened this issue Feb 9, 2017 · 5 comments · Fixed by #142
Closed

miri doesn't drop values moved into closures with let _ = x #135

oli-obk opened this issue Feb 9, 2017 · 5 comments · Fixed by #142
Labels
C-bug Category: This is a bug.

Comments

@oli-obk
Copy link
Contributor

oli-obk commented Feb 9, 2017

The following code works fine if drop(x) is used instead of let _ = x. The closure function generated contains just a _0 = () and a return. Trans probably does some magic, but I haven't been able to locate it.

struct Foo<'a>(&'a mut bool);

impl<'a> Drop for Foo<'a> {
    fn drop(&mut self) {
        *self.0 = true;
    }
}

fn f<T: FnOnce()>(t: T) {
    t()
}

fn main() {
    let mut ran_drop = false;
    {
        let x = Foo(&mut ran_drop);
        let x = move || { let _ = x; };
        f(x);
    }
    assert!(ran_drop);
}
@solson
Copy link
Member

solson commented Feb 9, 2017

@oli-obk Weird... the closure body ought to explicitly drop the closure environment argument in MIR.

@solson solson added the C-bug Category: This is a bug. label Feb 9, 2017
@solson
Copy link
Member

solson commented Feb 13, 2017

I figured out the problem here.

move || { let _ = x; }; never by-value uses any of its captures, so it's a closure whose inherent signature is fn(&self). When we call it through FnOnce::call_once (in f(x)), it has to use a shim.

Our shim acts like the following function, where self is the closure type and body is the closure fn:

fn call_once_shim(self) {
    body(&self)
    // NOTE: self should be dropped at the end of scope, but Miri doesn't do it.
}

As you can see in the shim code I linked above, we do add this self to a vec of temporaries to be freed when the stack frame is freed, but we don't ever execute the drop glue for it. (It's freed here.)

@solson
Copy link
Member

solson commented Feb 13, 2017

Note that if you force the closure to by-value use a separate value to remove Miri's use of the shim, then the closure's drop glue is properly called, as in this test I added.

EDIT: Actually I think in that case the closure body itself will drop x, since it's a fn(self) signature closure, responsible for freeing the by-value self argument which contains the capture x.

EDIT 2: ...which I guess means the closure body executes the closure's drop glue, so I wasn't wrong, just imprecise. :P

@oli-obk
Copy link
Contributor Author

oli-obk commented Feb 13, 2017

Great analysis! I'll take this bug, drop code is my fault anyway ;)

@solson
Copy link
Member

solson commented Feb 13, 2017

It would also be fixed once @arielb1's rewrite of these shims into explicit MIR bodies is done in rustc, but I'm not sure about the timeline on that. If it's hard to fix in Miri we can wait, otherwise it's fine to do it now.

EDIT: relevant rustc PR is here.

@oli-obk oli-obk mentioned this issue Feb 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants