Inspect enum discriminant *after* calling its destructor #24765
Conversation
…iant enum. (This may not be the *best* fix, compared to e.g. returning `_match::NoBranch` from `trans_switch` on a zero-variant enum. But it is one of the *simplest* fixes available.)
without invoking the Drop::drop implementation. This is necessary for dealing with an enum that switches own `self` to a different variant while running its destructor. Fix rust-lang#23611.
This addresses to-do in my code, and simplifies this method a lot to boot. (The necessary enum dispatch has now effectively been shifted entirely into the scheduled cleanup code for the enum contents.)
|
r? @Aatch (rust_highfive has picked a reviewer for you, use r? to override) |
|
I just want to say I'm always amazed by the thoroughness and clarity of your test cases. 👍 |
|
This looks great. I second @jakub-'s commendation for the test case. :) |
|
⌛ Testing commit b0a4808 with merge 7fd975e... |
|
💔 Test failed - auto-mac-64-opt |
|
weird! i ran that test a bunch locally... |
|
(though I admit I always only ran it in debug builds ... maybe something about the optimized non-debug build is making it think it can skip a bunch of the stuff that I expect it to do? will check locally about non-debug build...) |
|
Okay compiling the test with That is sort of scary. (I assume LLVM is finding some way to justify this transformation of the new code I am generating for destructors... maybe I am reading an undefined value from somewhere ...) |
|
Here is a test case that presents problems for both the rust master nightly build and the version from this branch: http://is.gd/2Hxj9P Here is a transcript comparing with/without (If you take out the |
|
Seems to be related to lifetime intrinsics. Compiling with them disabled does not expose the problem. |
|
Here is a smaller test for investigation purposes (not suitable for test suite): #[allow(dead_code)] enum E { A(u64, u32), B(u64, u32), }
impl Drop for E {
#[inline(never)]
fn drop(&mut self) {
let p = self as *const E as *const u64;
for i in 0.. (((std::mem::size_of::<E>() + 7)/8)) {
println!("i: {} p[i]: {:x}", i, unsafe { *p.offset(i as isize) });
}
unsafe { read(self as *mut E); }
}
}
fn main() {
let _e1 = E::B(0xFE113_410C4, 0xAAAA_AAAA);
}
#[inline(never)] unsafe fn read(p: *mut E) { std::ptr::read(p); }This, when compiled with According to what I see so far, we never properly initialize the drop flag for this test when compiling with without the Note that the lines with |
|
I think this is due to the way that |
|
Yep, that's it, stage1 libstd just finished with the Rvalue changed to a Lvalue (not sure if that's correct, but worked for a test), and both, the playpen and the most small testcase work with that. |
That is, scheduled drops are executed in reverse order, so for correctness, we *schedule* the lifetime end before we schedule the drop, so that when they are executed, the drop will be executed *before* the lifetime end.
… which is wrong. Kudos to dotdash for tracking down this fix. Presumably the use of `ByRef` was because this value is a reference to the drop-flag; but an Lvalue will serve just as well for that. dotdash argues: > since the drop_flag is in its "final home", Lvalue seems to be the > correct choice.
|
⌛ Testing commit 805349a with merge 48cbb8b... |
|
💔 Test failed - auto-linux-64-nopt-t |
|
i don't understand why the above test failed. Here is the relevant (?) portion of the log file: |
|
@bors retry |
Inspect enum discriminant *after* calling its destructor Includes some drive-by cleanup (e.g. changed some field and method names to reflect fill-on-drop; added comments about zero-variant enums being classified as `_match::Single`). Probably the most invasive change was the expansion of the maps `available_drop_glues` and `drop_glues` to now hold two different kinds of drop glues; there is the (old) normal drop glue, and there is (new) drop-contents glue that jumps straight to dropping the contents of a struct or enum, skipping its destructor. * For all types that do not have user-defined Drop implementations, the normal glue is generated as usual (i.e. recursively dropping the fields of the data structure). (And this actually is exactly what the newly-added drop-contents glue does as well.) * For types that have user-defined Drop implementations, the "normal" drop glue now schedules a cleanup before invoking the `Drop::drop` method that will call the drop-contents glue after that invocation returns. Fix #23611. ---- Is this a breaking change? The prior behavior was totally unsound, and it seems unreasonable that anyone was actually relying on it. Nonetheless, since there is a user-visible change to the language semantics, I guess I will conservatively mark this as a: [breaking-change] (To see an example of what sort of user-visible change this causes, see the comments in the regression test.)
|
triage: beta-nominated Just ensuring discussion; I am not a strong advocate for cherry-pick ) |
rust-highfive
added
the
beta-nominated
|
oh man, i had so many opportunities to land that name-change clean up that I had promised. Let me do that now. |
rename `schedule_drop_{enum,adt}_contents`.
addresses review nit from rust-lang#24765 (it was my mistake for not doing this earlier before it landed).
|
not accepted for beta backport. |
pnkfelix
removed
the
beta-nominated
Inspect enum discriminant after calling its destructor
Includes some drive-by cleanup (e.g. changed some field and method names to reflect fill-on-drop; added comments about zero-variant enums being classified as
_match::Single).Probably the most invasive change was the expansion of the maps
available_drop_gluesanddrop_gluesto now hold two different kinds of drop glues; there is the (old) normal drop glue, and there is (new) drop-contents glue that jumps straight to dropping the contents of a struct or enum, skipping its destructor.For all types that do not have user-defined Drop implementations, the normal glue is generated as usual (i.e. recursively dropping the fields of the data structure).
(And this actually is exactly what the newly-added drop-contents glue does as well.)
For types that have user-defined Drop implementations, the "normal" drop glue now schedules a cleanup before invoking the
Drop::dropmethod that will call the drop-contents glue after that invocation returns.Fix #23611.
Is this a breaking change? The prior behavior was totally unsound, and it seems unreasonable that anyone was actually relying on it.
Nonetheless, since there is a user-visible change to the language semantics, I guess I will conservatively mark this as a:
[breaking-change]
(To see an example of what sort of user-visible change this causes, see the comments in the regression test.)