Treat Drop as a rmw operation #107271

zeegomo · 2023-01-24T18:55:42Z

Previously, a Drop terminator was considered a move in MIR. This commit changes the behavior to only treat Drop as a mutable access to the dropped place.

In order for this change to be correct, we need to guarantee that

A dropped value won't be used again
Places that appear in a drop won't be used again before a
subsequent initialization.

We can ensure this to be correct at MIR construction because Drop will only be emitted when a variable goes out of scope, thus having:

(1) as there is no way of reaching the old value. drop-elaboration
will also remove any uninitialized drop.
(2) as the place can't be named following the end of the scope.

However, the initialization status, previously tracked by moves, should also be tied to the execution of a Drop, hence the additional logic in the dataflow analyses.

From discussion in this thread, originating from rust-lang/compiler-team#558.
See also #104488 (comment)

rustbot · 2023-01-24T18:55:51Z

r? @davidtwco

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

oli-obk · 2023-01-25T15:59:05Z

r? @oli-obk

compiler/rustc_mir_dataflow/src/drop_flag_effects.rs

oli-obk · 2023-01-25T16:08:42Z

compiler/rustc_mir_dataflow/src/move_paths/builder.rs

@@ -390,10 +391,6 @@ impl<'b, 'a, 'tcx> Gatherer<'b, 'a, 'tcx> {
                self.create_move_path(place);
                self.gather_init(place.as_ref(), InitKind::Deep);
            }
-
-            TerminatorKind::Drop { place, target: _, unwind: _ } => {
-                self.gather_move(place);


Would it suffice to just do the first part here (create_move_path) instead of having drop_flag_effects_for_location to redo this work?

Not sure I understand what you mean. This builder basically records moves and inits, and Drop is neither of them.
drop_flag_effects_for_location (which may be renamed to initialization_effects) now complements this information with Drops.
What would creating a move path gain us?

Yea, I managed to confuse myself a bit. You're right. It's just a bit scary how far apart the initialization tracking and move checking are.

So the next change would be to change DropAndReplace to stop moving out of the dropped value. The only operation we need to do in the move path builder is to gather the operand with which we are replacing the existing value. There's not even a need to handle this case in initialization tracking logic, as it will be initialized right after. Or do you plan to just remove DropAndReplace in favor of a Drop terminator followed by a regular assignment?

Or do you plan to just remove DropAndReplace in favor of a Drop terminator followed by a regular assignment?

yes, that's what I would like to have.

So the next change would be to change DropAndReplace to stop moving out of the dropped value

AFAIK DropAndReplace does not move out of the dropped place.
Indeed, in the move path builder DropAndReplace already does the exact same thing as an Assign (barring the Operand / Rvalue difference)

DropAndReplace already does the exact same thing as an Assign

Yes, as I understand it, DropAndReplace is a hack that was added to work around the following issue: If you treat a drop as being a move, then in this code, x is unnecessarily captured by value.

let mut x = String::new(); || { x = "foo".to_string(); }

cjgillot · 2023-01-26T20:08:59Z

Can the two invariants you rely on be checked by MIR validation ?

zeegomo · 2023-01-26T23:06:08Z

Can the two invariants you rely on be checked by MIR validation ?

I thought a bit about it and I actually think that by making drop de-initializing a place we have this checked for free by the borrow checker.

Places that appear in a drop won't be used again before a subsequent initialization.

That's already part of the borrowck tasks.

A dropped value won't be used again

How can we reach that value?

the dropped place: we can't for the reasons above
a place from which it was previously moved from: access after move disallowed by borrowck
references: this may be a bit more tricky. For sure, drops are only emitted after the variable goes out of scope, so no borrow could be live by this time anyway. I didn't change anything in the nll code though

To be precise, we also need to check that we only execute drops on initialized places, but we already relied on drop-elab to ensure that and this change should not introduce any further reliance or assumption.

oli-obk · 2023-01-27T09:17:22Z

I think we should check that all other dataflow analyses properly handle Drop now, even if their behaviour is still correct as nothing really changed wrt Drop for existing code that already successfully compiles. I worry that some analyses are now subtly wrong.

So a few things:

adjust the documentation of TerminatorKind::Drop to explain the new behaviour
check existing dataflow and other analyses to make sure they handle Drop correctly
- ValueAnalysis should probably flood the Drop place now?
- does this change anything that we should check in miri?
is there any reason to keep the Drop terminator around and not just make it a function call to drop_in_place?

zeegomo · 2023-01-27T10:18:31Z

I think we should check that all other dataflow analyses properly handle Drop now, even if their behaviour is still correct as nothing really changed wrt Drop for existing code that already successfully compiles. I worry that some analyses are now subtly wrong.

check existing dataflow and other analyses to make sure they handle Drop correctly
* ValueAnalysis should probably flood the Drop place now?
* does this change anything that we should check in miri?

I can try this but I'm not too familiar with all the stuff that's around the compiler or miri. Is there a way I can check/validate these assumptions?

is there any reason to keep the Drop terminator around and not just make it a function call to drop_in_place?

I think for now it's just easier to keep the existing terminator. Also, executing the destructor has a bit of added meaning (i.e. de-initializing the value) compared to a general function, I guess we would need to check the function in every place that needs to handle drops specially, like dataflow analyses or I think even the borrowck in the current implementation.
AFAIK drop_in_place is a standard function call until we build drop shims, which is too late for most analyses

oli-obk · 2023-01-27T10:20:59Z

AFAIK drop_in_place is a standard function call until we build drop shims, which is too late for most analyses

it is already a lang item. Though you're right, we'd end up treating regular calls to drop_in_place just like Drop terminators, and that would probably be wrong.

oli-obk · 2023-01-27T10:22:20Z

I can try this but I'm not too familiar with all the stuff that's around the compiler or miri. Is there a way I can check/validate these assumptions?

Very hard at this stage I think. The closest would probably be to use the custom_mir attributes to create MIR that can't be generated via regular Rust code and add that as miri tests to see what happens if our assumptions around Drop are not held up.

zeegomo · 2023-01-27T12:00:40Z

So, not sure if it's helpful but trying to do a write-up for the impact of this change on all analyses that I could find so that someone expert can validate my reasoning.

in rustc_mir_dataflow:

drop_flag_effects: this should be easy, we certainly don't want duplicated drops.
ValueAnalysis: Drop(place) now floods place with Value::bottom.
BorrowedLocals: partially relevant. Drop impl could launder a reference in the environment, but if the place is considered uninitialized after the drop should we allow this?
Maybe(Transitive)LiveLocals: Drop remains a Use, should have no impact
MaybeStorageLive / MaybeRequiresStorage: no impact

in the borrowck:

nll's liveness analysis: ignore drop as the last use, no impact
constraing generation: Drop may kill all borrows on the local? probably not necessary and we can still conservatively rely on StorageDead.
borrowck::dataflow::Borrows: same as above
unused muts: no impact

in const eval:

interpreter: should Drop write uninit bytes in place?
check_consts: no impact
promotion: no idea, should not

in rustc_mir_transform:

const_prop/const_prop_lin: mark as uninit on Drop? kind of related to const eval

Generally speaking, I'm wondering whether drop de-initializing a place has broader implications. Was that already established? Maybe it's something we need to check in miri?

oli-obk · 2023-01-27T12:03:56Z

should Drop write uninit bytes in place?

yea this is a pre-existing concern in miri. Since we don't do it for moving out of things, I don't think we should do it for Drop (or at least change them together).

oli-obk · 2023-01-27T12:07:24Z

partially relevant. Drop impl could launder a reference in the environment, but if the place is considered uninitialized after the drop should we allow this?

we should look at this one together with #[may_dangle]. I do not have borrowck and dropck semantics of may_dangle in my cache, so it'll take some time.

anyway, I think it's time to ping our current MIR semantics expert: @JakobDegen

JakobDegen · 2023-01-27T16:59:18Z

does this change anything that we should check in miri?

No. All these questions were conclusively (to the extent that's possibly) answered on the operational side in #103957 . That PR decided that drop terminators are - operationally - drop_in_place calls if and only if there is any drop glue, nothing more, nothing less. The documentation on the terminator also already reflects that.

should Drop write uninit bytes in place?

This was discussed on Zulip the other day - if we wanted this, then it would have to be a property of drop_in_place, and I'm fairly strongly opposed to it. Regardless, it's orthogonal to this PR.

In order for this change to be correct, we need to guarantee that

A dropped value won't be used again

Places that appear in a drop won't be used again before a subsequent initialization.

Sorry for the nits, but I think this is worth picking apart a little bit. This change is definitely "well motivated" in the sense that it moves borrowck towards agreeing with the opsem. The more precise question to ask is whether borrowck is still sound after this change. This is a question that T-types is probably most qualified to answer, especially because (as Oli identified) #[may_dangle] and such interactions.

Edit: I previously thought I had an interesting example where this made a difference but I was wrong. There is this obvious case though, which would be nice to have as a test:

#![feature(custom_mir, core_intrinsics)]

use core::intrinsics::mir::*;

#[custom_mir(dialect = "built")]
fn drop_term<T>(t: &mut T) {
    mir!(
        {
            Drop(*t, exit)
        }
        
        exit = {
            Return()
        }
    )
}

zeegomo · 2023-01-28T11:18:23Z

#![feature(custom_mir, core_intrinsics)]

use core::intrinsics::mir::*;

#[custom_mir(dialect = "built")]
fn drop_term<T>(t: &mut T) {
    mir!(
        {
            Drop(*t, exit)
        }
        
        exit = {
            Return()
        }
    )
}

So this looks like invalid MIR but I think I get your point.
Previously this was an error in the borrowck and now it's not.
However, it's still rejected by a drop elaboration ICE as we're trying to drop *t, which does not have an associated move path, since we don't create move paths for places behind references (even if we add an assignment like *t = Move(T) before).
What should we do in such cases? IMO it's something the compiler should never emit and it's fine to ICE, but not sure what is the recommended approach, especially with regard to tests

JakobDegen · 2023-01-29T00:27:23Z

What should we do in such cases?

Write some documentation :) . The ICE here is potentially fine, but "what does drop elaboration expect Mir to look like" needs to be written down somewhere. People will accidentally violate those rules

rustbot · 2023-01-29T22:44:23Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

zeegomo · 2023-01-29T22:50:53Z

I've added a doc comment on ElaborateDrops, including the move path assumptions, not sure that's what you were thinking of.

Thinking of further steps, checking for valid move paths now safely ignores DropAndReplace (as we will replace it right away), which we plan to remove. Should we introduce a similar check to reject ill-formed MIR or can we make assumptions about the validity of the input MIR without checking (it seems like we do already with regard to some borrowck guarantees)?

Previously, a Drop terminator was considered a move in MIR. This commit changes the behavior to only treat Drop as a mutable access to the dropped place. In order for this change to be correct, we need to guarantee that a) A dropped value won't be used again b) Places that appear in a drop won't be used again before a subsequent initialization. We can ensure this to be correct at MIR construction because Drop will only be emitted when a variable goes out of scope, thus having: (a) as there is no way of reaching the old value. drop-elaboration will also remove any uninitialized drop. (b) as the place can't be named following the end of the scope. However, the initialization status, previously tracked by moves, should also be tied to the execution of a Drop, hence the additional logic in the dataflow analyses.

oli-obk · 2023-02-07T14:31:36Z

@bors r+

bors · 2023-02-07T14:31:40Z

📌 Commit 68c1e2f has been approved by oli-obk

It is now in the queue for this repository.

Treat Drop as a rmw operation Previously, a Drop terminator was considered a move in MIR. This commit changes the behavior to only treat Drop as a mutable access to the dropped place. In order for this change to be correct, we need to guarantee that 1. A dropped value won't be used again 2. Places that appear in a drop won't be used again before a subsequent initialization. We can ensure this to be correct at MIR construction because Drop will only be emitted when a variable goes out of scope, thus having: * (1) as there is no way of reaching the old value. drop-elaboration will also remove any uninitialized drop. * (2) as the place can't be named following the end of the scope. However, the initialization status, previously tracked by moves, should also be tied to the execution of a Drop, hence the additional logic in the dataflow analyses. From discussion in [this thread](https://rust-lang.zulipchat.com/#narrow/stream/233931-t-compiler.2Fmajor-changes/topic/.60DROP.60.20to.20.60DROP_IF.60.20compiler-team.23558), originating from rust-lang/compiler-team#558. See also rust-lang#104488 (comment)

…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#105641 (Implement cursors for BTreeMap) - rust-lang#107271 (Treat Drop as a rmw operation) - rust-lang#107710 (Update strip-ansi-escapes and vte) - rust-lang#107758 (Change `arena_cache` to not alter the declared query result) - rust-lang#107777 (Make `derive_const` derive properly const-if-const impls) - rust-lang#107780 (Rename `replace_bound_vars_with_*` to `instantiate_binder_with_*`) - rust-lang#107793 (Add missing tracking issue for `RawOsError`) - rust-lang#107807 (Fix small debug typo) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

rustbot assigned davidtwco Jan 24, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 24, 2023

zeegomo force-pushed the drop-rmw branch 2 times, most recently from c38da15 to 49e68f1 Compare January 25, 2023 13:30

rustbot assigned oli-obk and unassigned davidtwco Jan 25, 2023

oli-obk reviewed Jan 25, 2023

View reviewed changes

compiler/rustc_mir_dataflow/src/drop_flag_effects.rs Outdated Show resolved Hide resolved

oli-obk reviewed Jan 25, 2023

View reviewed changes

zeegomo force-pushed the drop-rmw branch 2 times, most recently from 686aeab to 1401ddb Compare January 25, 2023 16:58

zeegomo force-pushed the drop-rmw branch from 1401ddb to 7e0ba0c Compare January 27, 2023 12:19

This comment has been minimized.

Sign in to view

zeegomo force-pushed the drop-rmw branch from 7e0ba0c to 834bc41 Compare January 27, 2023 12:42

zeegomo force-pushed the drop-rmw branch from 834bc41 to e568147 Compare January 29, 2023 22:44

This comment has been minimized.

Sign in to view

zeegomo force-pushed the drop-rmw branch from e568147 to 68c1e2f Compare January 29, 2023 23:21

zeegomo mentioned this pull request Feb 7, 2023

First attempt at removing DropAndReplace #104488

Closed

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 7, 2023

Dylan-DPC mentioned this pull request Feb 8, 2023

Rollup of 6 pull requests #107794

Closed

This was referenced Feb 8, 2023

Rollup of 9 pull requests #107810

Closed

Rollup of 8 pull requests #107811

Merged

bors merged commit 05748c6 into rust-lang:master Feb 8, 2023

rustbot added this to the 1.69.0 milestone Feb 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Treat Drop as a rmw operation #107271

Treat Drop as a rmw operation #107271

zeegomo commented Jan 24, 2023 •

edited

Loading

rustbot commented Jan 24, 2023

oli-obk commented Jan 25, 2023

oli-obk Jan 25, 2023

zeegomo Jan 25, 2023 •

edited

Loading

oli-obk Jan 27, 2023

zeegomo Jan 27, 2023

JakobDegen Jan 27, 2023

cjgillot commented Jan 26, 2023

zeegomo commented Jan 26, 2023 •

edited

Loading

oli-obk commented Jan 27, 2023

zeegomo commented Jan 27, 2023

oli-obk commented Jan 27, 2023

oli-obk commented Jan 27, 2023

zeegomo commented Jan 27, 2023

oli-obk commented Jan 27, 2023

oli-obk commented Jan 27, 2023

This comment has been minimized.

JakobDegen commented Jan 27, 2023 •

edited

Loading

zeegomo commented Jan 28, 2023 •

edited

Loading

JakobDegen commented Jan 29, 2023

rustbot commented Jan 29, 2023

zeegomo commented Jan 29, 2023

This comment has been minimized.

oli-obk commented Feb 7, 2023

bors commented Feb 7, 2023

Treat Drop as a rmw operation #107271

Treat Drop as a rmw operation #107271

Conversation

zeegomo commented Jan 24, 2023 • edited Loading

rustbot commented Jan 24, 2023

oli-obk commented Jan 25, 2023

oli-obk Jan 25, 2023

Choose a reason for hiding this comment

zeegomo Jan 25, 2023 • edited Loading

Choose a reason for hiding this comment

oli-obk Jan 27, 2023

Choose a reason for hiding this comment

zeegomo Jan 27, 2023

Choose a reason for hiding this comment

JakobDegen Jan 27, 2023

Choose a reason for hiding this comment

cjgillot commented Jan 26, 2023

zeegomo commented Jan 26, 2023 • edited Loading

oli-obk commented Jan 27, 2023

zeegomo commented Jan 27, 2023

oli-obk commented Jan 27, 2023

oli-obk commented Jan 27, 2023

zeegomo commented Jan 27, 2023

oli-obk commented Jan 27, 2023

oli-obk commented Jan 27, 2023

This comment has been minimized.

JakobDegen commented Jan 27, 2023 • edited Loading

zeegomo commented Jan 28, 2023 • edited Loading

JakobDegen commented Jan 29, 2023

rustbot commented Jan 29, 2023

zeegomo commented Jan 29, 2023

This comment has been minimized.

oli-obk commented Feb 7, 2023

bors commented Feb 7, 2023

zeegomo commented Jan 24, 2023 •

edited

Loading

zeegomo Jan 25, 2023 •

edited

Loading

zeegomo commented Jan 26, 2023 •

edited

Loading

JakobDegen commented Jan 27, 2023 •

edited

Loading

zeegomo commented Jan 28, 2023 •

edited

Loading