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

MIR drop order is sometimes odd #32433

Closed
nikomatsakis opened this issue Mar 22, 2016 · 6 comments
Closed

MIR drop order is sometimes odd #32433

nikomatsakis opened this issue Mar 22, 2016 · 6 comments
Labels
A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@nikomatsakis
Copy link
Contributor

As @eddyb noted on IRC:

15:04 <eddyb> this is weird: http://is.gd/Zjyjmd - it's dropping the outermost temporary, then the innermost, then the middle - shouldn't it be inside-out? or outside-in?
15:04 <eddyb> old trans appears to call them outside-in
15:04 <eddyb> and this drops x before foo(&x) http://is.gd/P1ergF

Related test:

src/test/run-pass/issue-23338-ensure-param-drop-order.rs

@nikomatsakis nikomatsakis added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Mar 22, 2016
@eddyb eddyb added the A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html label Mar 22, 2016
@nagisa
Copy link
Member

nagisa commented Apr 20, 2016

I’m certainly surprised we do not drop things in reverse order for the 2nd test case. Once we “fix” that, the first issue should fix itself too.

@eddyb
Copy link
Member

eddyb commented Apr 27, 2016

@pnkfelix
Copy link
Member

pnkfelix commented Apr 27, 2016

Another interesting difference:

    foo(&foo(&Foo(0), 1), 2);

prints: 2 0 1

while

    &foo(&foo(&Foo(0), 1), 2);

prints: 0 1 2


(both of the above print 2 1 0 if you discard the #[rustc_mir].)

@eddyb
Copy link
Member

eddyb commented Apr 27, 2016

@pnkfelix I think that's some statement handling, i.e. the outermost foo call in the first example doesn't get the drop from as_temp.

Manishearth added a commit to Manishearth/rust that referenced this issue Apr 27, 2016
mir: drop temps outside-in by scheduling the drops inside-out.

It was backwards all along, but only noticeable with multiple drops in one rvalue scope. Fixes rust-lang#32433.
@eddyb
Copy link
Member

eddyb commented May 7, 2016

Here's my latest testcase: https://play.rust-lang.org/?gist=7da9d41eee66dabeeaa62454f885da6b&version=nightly&backtrace=0
It prints 1 0 correctly (short-lived temporary in tail dropped before variable).
However, MIR, even with the initial fix in #33239, prints 0, 1.

The problem appears to be that the short-lived Foo(1) has temp_lifetime: Some(CodeExtent(7/Misc(6))), where 7 is the ID of main's whole body.
The debug log starts like this:

DEBUG:rustc_mir::build::scope: push_scope(CodeExtent(4/CallSiteScope { fn_id: 4, body_id: 6 }))
DEBUG:rustc_mir::build::scope: push_scope(CodeExtent(5/ParameterScope { fn_id: 4, body_id: 6 }))
DEBUG:rustc_mir::build::scope: push_scope(CodeExtent(7/Misc(6)))
DEBUG:rustc_mir::build::scope: push_scope(CodeExtent(8/Remainder(BlockRemainder { block: 6, first_statement_index: 0 })))

That means HAIR is telling rustc_mir::build that Foo(1) should only be outlived by the arguments, i.e. the variable should drop before Foo(1), and MIR follows suit.

The CodeExtent comes from let temp_lifetime = cx.tcx.region_maps.temporary_scope(expr.id);.
Does that mean rustc doesn't understand temporaries correctly?

Whoopty doo. Days since last soundness bug: 0 (#33490)

bors added a commit that referenced this issue May 11, 2016
mir: drop temps outside-in by scheduling the drops inside-out.

It was backwards all along, but only noticeable with multiple drops in one rvalue scope. Fixes #32433.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants