[MIR] Refactor Drop to be a Terminator #31307
Conversation
nagisa
force-pushed
the
mir-drop-terminator
branch
2 times, most recently
from
0aa44b4 to
29b2c39
Compare
nagisa
changed the title
nagisa
force-pushed
the
mir-drop-terminator
branch
4 times, most recently
from
4e8e746 to
7983294
Compare
|
|
nagisa
force-pushed
the
mir-drop-terminator
branch
from
27d220a to
230903b
Compare
|
@nagisa ok, so I left various comments -- as I said, I'm still feeling unsure about the dynamic drop part of this patch. I'm pretty keen on the rest of it, though there is one downside to making "free" be a call to a lang-item. In particular, it makes its semantics opaque to borrowck. @pnkfelix (who is working on implementing the "move analysis" from borrowck) and I have been discussing whether we ought to be checking the Anyway, I wonder if you'd be up for separating out the dynamic drop part of this patch from the rest? I'd rather land the first part and keep discussing the dynamic drop question a bit more. |
|
PS, in case it's not clear, nice work. |
nagisa
force-pushed
the
mir-drop-terminator
branch
from
230903b to
f5f8f32
Compare
|
Scope reduced and nits addressed (hopefully didn’t miss any) |
nagisa
changed the title
|
☔ The latest upstream changes (presumably #31385) made this pull request unmergeable. Please resolve the merge conflicts. |
This gets rid of Drop(Free, _) MIR construct by synthesizing a call to language item which takes care of dropping instead.
This helps to avoid the unpleasant restriction of being unable to have multiple successors in non-contiguous block of memory.
We used to have CallKind only because there was a requirement to have all successors in a contiguous memory block. Now that the requirement is gone, remove the CallKind and instead just have the necessary information inline. Awesome!
The structure of the old translator as well as MIR assumed that drop glue cannot possibly panic and
translated the drops accordingly. However, in presence of `Drop::drop` this assumption can be
trivially shown to be untrue. As such, the Rust code like the following would never print number 2:
```rust
struct Droppable(u32);
impl Drop for Droppable {
fn drop(&mut self) {
if self.0 == 1 { panic!("Droppable(1)") } else { println!("{}", self.0) }
}
}
fn main() {
let x = Droppable(2);
let y = Droppable(1);
}
```
While the behaviour is allowed according to the language rules (we allow drops to not run), that’s
a very counter-intuitive behaviour. We fix this in MIR by allowing `Drop` to have a target to take
on divergence and connect the drops in such a way so the leftover drops are executed when some drop
unwinds.
Note, that this commit still does not implement the translator part of changes necessary for the
grand scheme of things to fully work, so the actual observed behaviour does not change yet. Coming
soon™.
See rust-lang#14875.
With this commit we now finally execute all the leftover drops once some drop panics for some reason!
nagisa
force-pushed
the
mir-drop-terminator
branch
from
f5f8f32 to
5638847
Compare
|
@bors r+ very nice. |
|
📌 Commit 5638847 has been approved by |
|
⌛ Testing commit 5638847 with merge c01a2bf... |
|
💔 Test failed - auto-win-msvc-64-opt |
The MSVC SEH is still not implemented, so we go ahead and ignore it.
The scope of these refactorings is a little bit bigger than the title implies. See each commit for details. I’m submitting this for nitpicking now (the first 4 commits), because I feel the basic idea/implementation is sound and should work. I will eventually expand this PR to cover the translator changes necessary for all this to work (+ tests), ~~and perhaps implement a dynamic dropping scheme while I’m at it as well.~~ r? @nikomatsakis
|
💔 Test failed - auto-mac-32-opt |
|
@bors: retry On Fri, Feb 5, 2016 at 6:59 PM, bors notifications@github.com wrote:
|
|
⚡ Previous build results for auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-mac-ios-opt are reusable. Rebuilding only auto-linux-64-debug-opt, auto-linux-64-x-android-t, auto-mac-32-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... |
The scope of these refactorings is a little bit bigger than the title implies. See each commit for details.
I’m submitting this for nitpicking now (the first 4 commits), because I feel the basic idea/implementation is sound and should work. I will eventually expand this PR to cover the translator changes necessary for all this to work (+ tests),
and perhaps implement a dynamic dropping scheme while I’m at it as well.r? @nikomatsakis