Skip to content

trans: Fix missing closure env drop-glue in trans-item collector. #38822

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

Merged
merged 2 commits into from
Jan 8, 2017

Conversation

michaelwoerister
Copy link
Member

FnOnce adapters automatically generated by the compiler introduce a call to drop the closure environment. The collector didn't pick up on that because this drop call does not show up in MIR. That could lead to an assertion being triggered if the drop-glue for the environment wasn't instantiated via something else.

Fixes #38810

cc @arielb1

r? @eddyb or @nikomatsakis

@eddyb
Copy link
Member

eddyb commented Jan 4, 2017

@bors r+

@bors
Copy link
Collaborator

bors commented Jan 4, 2017

📌 Commit ab8fff2 has been approved by eddyb

@michaelwoerister
Copy link
Member Author

@eddyb I just pushed a commit with an additional fix. Could you please review that too?

@eddyb
Copy link
Member

eddyb commented Jan 4, 2017

@bors r+

@bors
Copy link
Collaborator

bors commented Jan 4, 2017

📌 Commit 8b94267 has been approved by eddyb

@bors
Copy link
Collaborator

bors commented Jan 8, 2017

⌛ Testing commit 8b94267 with merge 0576869...

bors added a commit that referenced this pull request Jan 8, 2017
trans: Fix missing closure env drop-glue in trans-item collector.

FnOnce adapters automatically generated by the compiler introduce a call to drop the closure environment. The collector didn't pick up on that because this drop call does not show up in MIR. That could lead to an assertion being triggered if the drop-glue for the environment wasn't instantiated via something else.

Fixes #38810

cc @arielb1

r? @eddyb or @nikomatsakis
@bors
Copy link
Collaborator

bors commented Jan 8, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: eddyb
Pushing 0576869 to master...

@bors bors merged commit 8b94267 into rust-lang:master Jan 8, 2017
@KalitaAlexey
Copy link
Contributor

@michaelwoerister, @eddyb
I see no tests were added.
Why is that?

@michaelwoerister
Copy link
Member Author

The main reason is that the failure cannot show up with the current CGU partitioning scheme, only with different scheme I have in a local branch. But I'll try to come up with a test case once that scheme gets merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants