[MIR] Some initial zeroing implementation work by nagisa · Pull Request #31430 · rust-lang/rust

nagisa · 2016-02-05T13:20:00Z

Zeroing on-drop seems to work fine. Still thinking about the best way to approach zeroing on-move.

(based on top of the other drop PR; only the last 2 commits are relevant)

nagisa · 2016-02-05T13:20:11Z

r? @nikomatsakis

rust-highfive · 2016-02-05T13:20:12Z

r? @alexcrichton

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

nikomatsakis · 2016-02-05T20:31:49Z

This looks pretty good. I think we also need to zero on moves (Use), and then we should be pretty much set.

nikomatsakis · 2016-02-05T20:32:55Z

@bors r+ (should be no conflict with other PR landing, though one hopes it doesn't bounce)

bors · 2016-02-05T20:32:56Z

📌 Commit 69b6f72 has been approved by nikomatsakis

nagisa · 2016-02-06T11:08:02Z

@bors r=nikomatsakis eef0734

(rebased, since github didn’t quite figure it out I think?)

bors · 2016-02-06T11:08:02Z

🙀 eef0734 is not a valid commit SHA. Please try again with 569b18f.

nagisa · 2016-02-06T11:08:54Z

@bors r=nikomatsakis

bors · 2016-02-06T11:08:54Z

📌 Commit 569b18f has been approved by nikomatsakis

bors · 2016-02-06T23:22:12Z

⌛ Testing commit 569b18f with merge 831a453...

bors · 2016-02-07T00:16:57Z

💔 Test failed - auto-linux-64-nopt-t

bors · 2016-02-09T10:52:40Z

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

nagisa · 2016-02-09T20:00:18Z

I’m genuinely confused about the test failure here.

nikomatsakis · 2016-02-18T20:50:29Z

Indeed, it's confusing. I pulled the branch locally and am doing a build, just to see what I see.

nikomatsakis · 2016-02-18T21:38:29Z

@nagisa maybe rebase and we'll just give it the ol' bors retry?

nikomatsakis · 2016-02-19T09:57:05Z

never mind, the error is quite reproducible if you configure with --disable-optimize-tests

Hopefully the author caught all the cases. For the mir_dynamic_drops_3 test case the ratio of memsets to other instructions is 12%. On the other hand we actually do not double drop for at least the test cases provided anymore in MIR.

nagisa · 2016-02-24T19:07:19Z

@nikomatsakis re-r?

This has changed somewhat considerably since last time and now is a full-ish implementation.

In MIR we previously tried to match `let x in { exprs; let y in { exprs; }}` with our data structures which is rather unwieldy, espeicially because it requires some sort of recursion or stack to process, while, a flat list of statements is enough – lets only relinquish their lifetime at the end of the block (i.e. end of the list). Also fixesrust-lang#31853.

nikomatsakis · 2016-03-01T22:29:35Z

@bors r+

bors · 2016-03-01T22:29:36Z

📌 Commit d1a1239 has been approved by nikomatsakis

bors · 2016-03-01T23:30:50Z

⌛ Testing commit d1a1239 with merge 339a409...

Zeroing on-drop seems to work fine. Still thinking about the best way to approach zeroing on-move. (based on top of the other drop PR; only the last 2 commits are relevant)

bors · 2016-03-02T02:40:06Z

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-mac-ios-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-opt

SimonSapin · 2016-03-07T23:46:33Z

I don’t understand the code changes but the PR title mentions zeroing rather than filling. Are we reverting https://internals.rust-lang.org/t/attention-hackers-filling-drop/1715 ?

nagisa · 2016-03-07T23:50:10Z

This is using the same technique used by the current translator, so you could also call it filling (I personally see no distinction). This PR also affects MIR only (which is not used by default) and is a temporary solution until we make non-filling (stack flags) drops happen.

SimonSapin · 2016-03-08T00:30:09Z

Alright. My worry is code that uses #[unsafe_no_drop_flag] and compares stuff to std::mem::POST_DROP_USIZE or std::mem::dropped(). I suppose it’ll be unaffected as long as MIR is not enabled by default.

Is it the plan to not enable MIR until it comes with proper drop flags on the stack and unsafe_no_drop_flag becomes unnecessary?

arielb1 · 2016-03-08T11:52:18Z

@SimonSapin

This uses the same kind of filling that non-MIR uses. We should revert this commit when pnkfelix gets stack-flag drop working.

rust-highfive assigned alexcrichton Feb 5, 2016

rust-highfive assigned nikomatsakis and unassigned alexcrichton Feb 5, 2016

nagisa force-pushed the mir-dyndrop branch from 21c3e14 to 69b6f72Compare February 5, 2016 13:44

nagisa force-pushed the mir-dyndrop branch from 69b6f72 to 569b18fCompare February 6, 2016 11:07

nagisa force-pushed the mir-dyndrop branch from 569b18f to 5896735Compare February 24, 2016 18:43

nagisa added 2 commits February 24, 2016 21:05

Implement filling drop in MIR
ba26efb
Hopefully the author caught all the cases. For the mir_dynamic_drops_3 test case the ratio of memsets to other instructions is 12%. On the other hand we actually do not double drop for at least the test cases provided anymore in MIR.

Add some tests for dynamic dropping
fbd8171

nagisa force-pushed the mir-dyndrop branch from 5896735 to a5e723bCompare February 24, 2016 19:05

nagisa added 2 commits February 24, 2016 22:04

Fix tests
356bf52

nagisa force-pushed the mir-dyndrop branch from a5e723b to 356bf52Compare February 24, 2016 20:04

fix drop of fat ptrs
b7f53b8

nagisa force-pushed the mir-dyndrop branch from 41e431a to b7f53b8Compare February 25, 2016 22:06

Nits and cleanups
d1a1239

nagisa force-pushed the mir-dyndrop branch from 2ac47b6 to d1a1239Compare February 26, 2016 12:15

bors merged commit d1a1239 into rust-lang:masterMar 2, 2016

This was referenced Mar 2, 2016
Distinguish fn item types to allow reification from nothing to fn pointers. #31710
Merged
typestrong const integers #30587
Merged

llogiq mentioned this pull request Mar 6, 2016
TWIR 121's CotW + PRs rust-lang/this-week-in-rust#176
Closed

Uh oh!

Uh oh!

Conversation

nagisa commented Feb 5, 2016

Uh oh!

nagisa commented Feb 5, 2016

Uh oh!

rust-highfive commented Feb 5, 2016

Uh oh!

nikomatsakis commented Feb 5, 2016

Uh oh!

nikomatsakis commented Feb 5, 2016

Uh oh!

bors commented Feb 5, 2016

Uh oh!

nagisa commented Feb 6, 2016

Uh oh!

bors commented Feb 6, 2016

Uh oh!

nagisa commented Feb 6, 2016

Uh oh!

bors commented Feb 6, 2016

Uh oh!

bors commented Feb 6, 2016

Uh oh!

bors commented Feb 7, 2016

Uh oh!

bors commented Feb 9, 2016

Uh oh!

nagisa commented Feb 9, 2016

Uh oh!

nikomatsakis commented Feb 18, 2016

Uh oh!

nikomatsakis commented Feb 18, 2016

Uh oh!

nikomatsakis commented Feb 19, 2016

Uh oh!

nagisa commented Feb 24, 2016

Uh oh!

nikomatsakis commented Mar 1, 2016

Uh oh!

bors commented Mar 1, 2016

Uh oh!

bors commented Mar 1, 2016

Uh oh!

bors commented Mar 2, 2016

Uh oh!

SimonSapin commented Mar 7, 2016

Uh oh!

nagisa commented Mar 7, 2016

Uh oh!

SimonSapin commented Mar 8, 2016

Uh oh!

arielb1 commented Mar 8, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants