Skip to content

/rust

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

Reinitialize the dropflag hint in bindings #27413

Closed
wants to merge 3 commits into from

Conversation

Reviewers
No reviews
Assignees

@nikomatsakis nikomatsakis

Labels
None yet
Projects
None yet
Milestone
No milestone
5 participants
@pnkfelix @rust-highfive @nikomatsakis @bors @alexcrichton
@pnkfelix
Copy link
Member

pnkfelix commented Jul 31, 2015

Reinitialize the dropflag hint in occurrences of variable bindings.

Such bindings can occur in loops, and thus the binding can be executed after a previous move cleared the flag, thus necessitating the flag be reset to DTOR_NEEDED_HINT.

Fix #27401.

pnkfelix added some commits Jul 31, 2015

@pnkfelix
Reinitialize the dropflag hint in binding occurrences. 
Such bindings can occur in loops, and thus the binding can be executed
after a previous move cleared the flag, thus necessitating the flag be
reset to `DTOR_NEEDED_HINT`.

Fix #27401.
173dc0f
@pnkfelix
Regression tests for Issue #27401.
Loading status checks…
0f314f9

@rust-highfive rust-highfive assigned nikomatsakis Jul 31, 2015

@rust-highfive

This comment has been minimized.

Sign in to view
Copy link
Collaborator

rust-highfive commented Jul 31, 2015

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)
@pnkfelix

This comment has been minimized.

Sign in to view
Copy link
Member Author

pnkfelix commented Jul 31, 2015

hat-tip to @alexcrichton for narrowing down the crates.io unit test failures that exposed this bug!
@pnkfelix
placate `make tidy`.
Loading status checks…
c681d30
@nikomatsakis

This comment has been minimized.

Sign in to view
Copy link
Contributor

nikomatsakis commented Jul 31, 2015

r+ --- seems good, the only nit is that there could be a test for the dummy local case
@pnkfelix

This comment has been minimized.

Sign in to view
Copy link
Member Author

pnkfelix commented Jul 31, 2015

@bors r=nikomatsakis c681d30
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Jul 31, 2015

⌛️ Testing commit c681d30 with merge eb16343...

bors added a commit that referenced this pull request Jul 31, 2015

@bors
Auto merge of #27413 - pnkfelix:fix-issue-27401, r=nikomatsakis 
Reinitialize the dropflag hint in occurrences of variable bindings.

Such bindings can occur in loops, and thus the binding can be executed after a previous move cleared the flag, thus necessitating the flag be reset to `DTOR_NEEDED_HINT`.

Fix #27401.
eb16343
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Jul 31, 2015

💔 Test failed - auto-mac-64-opt
@alexcrichton

This comment has been minimized.

Sign in to view
Copy link
Member

alexcrichton commented Jul 31, 2015

@bors: retry

On Fri, Jul 31, 2015 at 1:13 AM, bors notifications@github.com wrote:

[image: 💔] Test failed - auto-mac-64-opt
http://buildbot.rust-lang.org/builders/auto-mac-64-opt/builds/5919

—
Reply to this email directly or view it on GitHub
#27413 (comment).
@pnkfelix

This comment has been minimized.

Sign in to view
Copy link
Member Author

pnkfelix commented Jul 31, 2015

@bors p=1
@pnkfelix

This comment has been minimized.

Sign in to view
Copy link
Member Author

pnkfelix commented Jul 31, 2015

(i upp'ed priority because I want to get quicker feedback about problems in landing this PR, since I want to ensure it gets into beta but I may be traveling during the time that beta is cut.)

bors added a commit that referenced this pull request Jul 31, 2015

@bors
Auto merge of #27413 - pnkfelix:fix-issue-27401, r=nikomatsakis 
Reinitialize the dropflag hint in occurrences of variable bindings.

Such bindings can occur in loops, and thus the binding can be executed after a previous move cleared the flag, thus necessitating the flag be reset to `DTOR_NEEDED_HINT`.

Fix #27401.
8344236
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Jul 31, 2015

⌛️ Testing commit c681d30 with merge 8344236...
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Jul 31, 2015

💔 Test failed - auto-mac-32-opt
@pnkfelix

This comment has been minimized.

Sign in to view
Copy link
Member Author

pnkfelix commented Aug 5, 2015

An update: this PR appears to either inject a double-free, or somehow expose a latent one. My suspicion is that the double-free is being "injected", though the real bug is probably somewhere deeper in the non-zeroing moves implementation. Still looking.
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Aug 5, 2015

☔️ The latest upstream changes (presumably #27529) made this pull request unmergeable. Please resolve the merge conflicts.
@pnkfelix

This comment has been minimized.

Sign in to view
Copy link
Member Author

pnkfelix commented Aug 7, 2015

closing for now; I'll revive this later, but for the short-term we're just disabling NZM; see #27582

@pnkfelix pnkfelix closed this Aug 7, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.