Replace zeroing-on-drop with filling-on-drop. by pnkfelix · Pull Request #23535 · rust-lang/rust

pnkfelix · 2015-03-19T23:26:21Z

Replace zeroing-on-drop with filling-on-drop.

This is meant to set the stage for removing all zeroing and filling (on drop) in the future.

Note that the code is meant to be entirely abstract with respect to the particular values used for the drop flags: the final commit demonstrates how to go from zeroing-on-drop to filling-on-drop by changing the value of three constants (in two files).

See further discussion on the internals thread:
http://internals.rust-lang.org/t/attention-hackers-filling-drop/1715/11

[breaking-change] especially for structs / enums using #[unsafe_no_drop_flag].

rust-highfive · 2015-03-19T23:26:32Z

r? @nrc

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

pnkfelix · 2015-03-19T23:26:35Z

r? @nikomatsakis

huonw · 2015-03-19T23:32:04Z

Could this be a wrapper around init_dropped?

I tried to make it such a wrapper at one point in the history of the PR, but I got tired of trying to work out the staging issues involved...

Oh.
I guess this might work? (it might not too.)
#[cfg(stage0)]pubunsafefndropped<T>() -> T{zeroed()}#[cfg(not(stage0))]pubunsafefndropped<T>() -> T{ intrinsics::init_dropped()}

okay i will try that tomorrow morning

ah it works like a charm, thanks huon, now I feel dumb. :)

pnkfelix · 2015-03-19T23:32:32Z

(@nrc notes that this PR is probably a good candidate for some run-pass-valgrind tests.)

pnkfelix · 2015-03-20T07:15:33Z

I may add a lint for enums using #[unsafe_no_drop_flag] because this really breaks those. (Is anyone using them?)

eddyb · 2015-03-20T11:57:13Z

@pnkfelix IME I don't recall ever seeing Drop implemented on enums, I wonder if we can quickly count them across the ecosystem.
The attribute itself applied on enums would be easy to grep for.

nikomatsakis · 2015-03-24T08:40:37Z

Oh, man, I totally missed this PR! Sorry for the delay @pnkfelix

nikomatsakis · 2015-03-24T15:36:29Z

r+ -- no real nits, just some questions

pnkfelix · 2015-03-25T15:50:37Z

@bors try

bors · 2015-03-25T15:50:42Z

⌛ Trying commit fc87aa8 with merge 6efae5d...

bors · 2015-03-25T16:15:02Z

💔 Test failed - try-bsd

pnkfelix · 2015-03-25T17:47:01Z

@bors try

bors · 2015-03-25T17:47:07Z

⌛ Trying commit 94392c8 with merge 969b386...

Replace zeroing-on-drop with filling-on-drop. This is meant to set the stage for removing *all* zeroing and filling (on drop) in the future. Note that the code is meant to be entirely abstract with respect to the particular values used for the drop flags: the final commit demonstrates how to go from zeroing-on-drop to filling-on-drop by changing the value of three constants (in two files). See further discussion on the internals thread: http://internals.rust-lang.org/t/attention-hackers-filling-drop/1715/11 [breaking-change] especially for structs / enums using `#[unsafe_no_drop_flag]`.

bors · 2015-03-25T18:20:02Z

💔 Test failed - try-mac

Manishearth · 2015-03-26T05:08:36Z

/Users/rustbuild/src/rust-buildbot/slave/try-mac/build/src/librustc_trans/trans/glue.rs:43:5: 43:33 error: unused import, #[deny(unused_imports)] on by default
/Users/rustbuild/src/rust-buildbot/slave/try-mac/build/src/librustc_trans/trans/glue.rs:43 use session::config::NoDebugInfo;
^~~~~~~~~~~~~~~~~~~~~~~~~~~~
error: aborting due to previous error
make: *** [x86_64-apple-darwin/stage1/lib/rustlib/x86_64-apple-darwin/lib/stamp.rustc_trans] Error 101
make: *** Waiting for unfinished jobs....

pnkfelix · 2015-03-26T09:25:39Z

@bors try fd65e4f

bors · 2015-03-26T09:25:43Z

⌛ Trying commit fd65e4f with merge a21e18f...

Replace zeroing-on-drop with filling-on-drop. This is meant to set the stage for removing *all* zeroing and filling (on drop) in the future. Note that the code is meant to be entirely abstract with respect to the particular values used for the drop flags: the final commit demonstrates how to go from zeroing-on-drop to filling-on-drop by changing the value of three constants (in two files). See further discussion on the internals thread: http://internals.rust-lang.org/t/attention-hackers-filling-drop/1715/11 [breaking-change] especially for structs / enums using `#[unsafe_no_drop_flag]`.

bors · 2015-03-26T10:32:54Z

💔 Test failed - try-mac

Refactored code so that the drop-flag values for initialized (`DTOR_NEEDED`) versus dropped (`DTOR_DONE`) are given explicit names. Add `mem::dropped()` (which with `DTOR_DONE == 0` is semantically the same as `mem::zeroed`, but the point is that it abstracts away from the particular choice of value for `DTOR_DONE`). Filling-drop needs to use something other than `ptr::read_and_zero`, so I added such a function: `ptr::read_and_drop`. But, libraries should not use it if they can otherwise avoid it. Fixes to tests to accommodate filling-drop.

…ile-fail tests. (I.e. the idea being, lets catch errors in these basic constructs sometime *before* we start doing the doc tests.)

…ues.

…e PR.

bors · 2015-03-26T18:32:19Z

⌛ Trying commit e2cc8b1 with merge 961e035...

bors · 2015-03-26T21:10:05Z

☀️ Test successful - try-bsd, try-linux, try-mac, try-win-32, try-win-64

pnkfelix · 2015-03-26T22:02:26Z

@bors r=nikomatsakis e2cc8b1

bors · 2015-03-26T22:02:29Z

⌛ Testing commit e2cc8b1 with merge b50bf4a...

bors · 2015-03-26T23:39:21Z

💔 Test failed - auto-mac-64-opt

pnkfelix · 2015-03-27T00:08:05Z

@bors r=nikomatsakis b68ca84

bors · 2015-03-27T10:11:40Z

⌛ Testing commit b68ca84 with merge 74ab665...

bors · 2015-03-27T12:17:04Z

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

pnkfelix · 2015-03-27T13:37:57Z

@bors retry

bors · 2015-03-27T15:26:55Z

⌛ Testing commit b68ca84 with merge be7f6ac...

bors · 2015-03-27T17:59:38Z

💔 Test failed - auto-win-32-nopt-t

alexcrichton · 2015-03-27T18:03:20Z

@bors: retry

On Fri, Mar 27, 2015 at 10:59 AM, bors notifications@github.com wrote:

[image: 💔] Test failed - auto-win-32-nopt-t
http://buildbot.rust-lang.org/builders/auto-win-32-nopt-t/builds/3922
—
Reply to this email directly or view it on GitHub
#23535 (comment).

Replace zeroing-on-drop with filling-on-drop. This is meant to set the stage for removing *all* zeroing and filling (on drop) in the future. Note that the code is meant to be entirely abstract with respect to the particular values used for the drop flags: the final commit demonstrates how to go from zeroing-on-drop to filling-on-drop by changing the value of three constants (in two files). See further discussion on the internals thread: http://internals.rust-lang.org/t/attention-hackers-filling-drop/1715/11 [breaking-change] especially for structs / enums using `#[unsafe_no_drop_flag]`.

bors · 2015-03-28T01:53:29Z

⌛ Testing commit b68ca84 with merge 46fa43d...

pnkfelix · 2015-06-05T08:49:06Z

Note: #26025 points out a potential bug injected by filling-drop; namely, there is a switch on the discriminant of an enum, which will now be set to a value outside the range of typical discriminants, and ends up jumping into an unreachable block of the switch (which is then treated as undefined-behavior by LLVM).

eddyb · 2015-06-05T09:19:49Z

@pnkfelix but wouldn't there be a check for that value before the match on the discriminant?
Though I'm not aware of all the filling-drop details.

When I suggested the "poke as little to disable all possible dtors" interim solution, enum variants were the interesting case, because you could search for a variant with little (or no, e.g. None) data that could drop and set the discriminant to it.

Is that viable at all here? Or since this is a temporary measure, after all, maybe just add a case for the discriminant being filled with the value - but what if it's a valid discriminant for that enum?
Oh, it doesn't matter, because the data inside should be inert as well.

pnkfelix · 2015-06-05T10:35:54Z

@eddyb we might need to add such a check-before-switch if filling-drop stays as it is today. But historically, as I understand it from what niko tells me, we were relying on the fact that every enum that could implement Drop also had 0 as a valid discriminant, and thus the compiler was freely inspecting the discriminant even on moved/dropped enum values.

I don't know how viable it would be to attempt to fill with a "valid" enum variant. It seems sort of sketchy to me; it would require the filling code to be more aware of structural layout than it is today... I'd prefer to invest effort in finishing nonzeroing drop.

eddyb · 2015-06-05T11:43:20Z

@alexcrichton's ret void seems fine as long as it's limited to drop glue - I just checked #26025 and it isn't; might not be hard to add a parameter controlling that.

rust-highfive assigned nrc Mar 19, 2015

rust-highfive assigned nikomatsakis and unassigned nrc Mar 19, 2015

huonw reviewed Mar 19, 2015
View reviewed changes

Manishearth mentioned this pull request Mar 20, 2015
Remove rt::default_sched_threads and RUST_THREADS. #23534
Closed

pnkfelix force-pushed the fsk-filling-drop branch from 94392c8 to fd65e4fCompare March 26, 2015 09:24

pnkfelix added 4 commits March 26, 2015 14:08

Regression tests for issues uncovered only post the run-pass and comp…
7c671e5
…ile-fail tests. (I.e. the idea being, lets catch errors in these basic constructs sometime *before* we start doing the doc tests.)

filling-drop: switch DTOR_NEEDED and DTOR_DONE to non-trivial val…
5bc35b1
…ues.

A better core::mem::dropped implementation suggested by huonw on th…
5733726
…e PR.

workaround pretty-print bugs to placate make check-pretty.
b68ca84

bors merged commit b68ca84 into rust-lang:masterMar 28, 2015

This was referenced May 26, 2015
ManuallyDrop type gives precise control of dtors of inline data. rust-lang/rfcs#197
Closed
Dropping SmallVecN is unsound servo/rust-smallvec#4
Closed

pnkfelix mentioned this pull request Jun 5, 2015
rustc: Update LLVM #26025
Merged

bluss mentioned this pull request Jun 6, 2015
implement "dynamic drop" semantics using flags on the stack rather than zeroing #5016
Closed

pnkfelix mentioned this pull request Feb 9, 2016
debugging trans: initialize drop-flag byte with special pattern and check for it on drop #21777
Closed

hanna-kruppe mentioned this pull request Oct 3, 2016
RFC: Abort by default rust-lang/rfcs#1759
Closed

Uh oh!

Uh oh!

Conversation

pnkfelix commented Mar 19, 2015

Uh oh!

rust-highfive commented Mar 19, 2015

Uh oh!

pnkfelix commented Mar 19, 2015

Uh oh!

huonwMar 19, 2015

Choose a reason for hiding this comment

Uh oh!

pnkfelixMar 19, 2015

Choose a reason for hiding this comment

Uh oh!

huonwMar 19, 2015

Choose a reason for hiding this comment

Uh oh!

pnkfelixMar 20, 2015

Choose a reason for hiding this comment

Uh oh!

pnkfelixMar 23, 2015

Choose a reason for hiding this comment

Uh oh!

pnkfelix commented Mar 19, 2015

Uh oh!

pnkfelix commented Mar 20, 2015

Uh oh!

eddyb commented Mar 20, 2015

Uh oh!

nikomatsakis commented Mar 24, 2015

Uh oh!

nikomatsakis commented Mar 24, 2015

Uh oh!

pnkfelix commented Mar 25, 2015

Uh oh!

bors commented Mar 25, 2015

Uh oh!

bors commented Mar 25, 2015

Uh oh!

pnkfelix commented Mar 25, 2015

Uh oh!

bors commented Mar 25, 2015

Uh oh!

bors commented Mar 25, 2015

Uh oh!

Manishearth commented Mar 26, 2015

Uh oh!

pnkfelix commented Mar 26, 2015

Uh oh!

bors commented Mar 26, 2015

Uh oh!

bors commented Mar 26, 2015

Uh oh!

bors commented Mar 26, 2015

Uh oh!

bors commented Mar 26, 2015

Uh oh!

pnkfelix commented Mar 26, 2015

Uh oh!

bors commented Mar 26, 2015

Uh oh!

bors commented Mar 26, 2015

Uh oh!

pnkfelix commented Mar 27, 2015

Uh oh!

bors commented Mar 27, 2015

Uh oh!

bors commented Mar 27, 2015

Uh oh!

pnkfelix commented Mar 27, 2015

Uh oh!

bors commented Mar 27, 2015

Uh oh!

bors commented Mar 27, 2015

Uh oh!

alexcrichton commented Mar 27, 2015

Uh oh!

bors commented Mar 28, 2015

Uh oh!