Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Desugaring of drop and replace at MIR build #107844

Merged
merged 4 commits into from
Mar 5, 2023

Conversation

zeegomo
Copy link
Contributor

@zeegomo zeegomo commented Feb 9, 2023

This commit desugars the drop and replace deriving from an
assignment at MIR build, avoiding the construction of the
DropAndReplace terminator (which will be removed in a following PR).

In order to retain the same error messages for replaces a new
DesugaringKind::Replace variant is introduced.

The changes in the borrowck are also useful for future work in moving drop elaboration
before borrowck, as no DropAndReplace would be present there anymore.

Notes on test diffs:

  • tests/ui/borrowck/issue-58776-borrowck-scans-children: the assignment deriving from the desugaring kills the borrow.
  • tests/ui/async-await/async-fn-size-uninit-locals.rs, tests/mir-opt/issue_41110.test.ElaborateDrops.after.mir, tests/mir-opt/issue_41888.main.ElaborateDrops.after.mir: drop elaboration generates (or reads from) a useless drop flag due to an issue with the dataflow analysis. Will be fixed independently by Remove dead unwinds before drop elaboration #106430.

See #104488 for more context

@rustbot
Copy link
Collaborator

rustbot commented Feb 9, 2023

r? @lcnr

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

@rustbot 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 Feb 9, 2023
@rustbot
Copy link
Collaborator

rustbot commented Feb 9, 2023

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@lcnr
Copy link
Contributor

lcnr commented Feb 10, 2023

r? compiler

@rustbot rustbot assigned eholk and unassigned lcnr Feb 10, 2023
Some(kind),
),
WriteKind::StorageDeadOrDrop => {
if let Some(DesugaringKind::Replace) = place_span.1.desugaring_kind() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may deserve a helper function. The fact that checking desugaring_kind() is enough comes from the fact that DesugaringKind::Replace can only be created after the other (HIR-based) variants.


// create the new block for the assignment in the case of unwinding
let assign_unwind = self.cfg.start_new_cleanup_block();
self.cfg.push_assign(assign_unwind, source_info, place, value.clone());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having the assignment even in the unwind case is a bit surprising. This is the current behaviour, so ok. Should eventually be removed in the future?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As long as drop can unwind, it is necessary to avoid leaving partially destroyed places behind. See #30380 for an example.

@@ -8,6 +8,7 @@ fn a() {
//~^ NOTE `vec[_]` is borrowed here
vec[0] = Box::new(4); //~ ERROR cannot assign
//~^ NOTE `vec[_]` is assigned to here
//~| NOTE in this expansion of desugaring of drop and replace
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should DesugaringKind::Replace be filtered out from diagnostic notes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not shown in human readable output but only in json, like other desugarings (for, ?, ...). I think it makes sense they all have the same behavior

@@ -40,7 +40,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
// Generate better code for things that don't need to be
// dropped.
if lhs.ty.needs_drop(this.tcx, this.param_env) {
let rhs = unpack!(block = this.as_local_operand(block, rhs));
let rhs = unpack!(block = this.as_local_rvalue(block, rhs));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a little surprising to me that this wasn't .as_local_rvalue already. Were we just less precise than we could be before?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh no, I guess this change is because build_drop_and_replace was changed to take an rvalue.

@eholk
Copy link
Contributor

eholk commented Feb 18, 2023

This looks pretty good to me. I'm a little curious about why the futures got bigger in async-fn-size-uninit-locals.rs, but it's only one byte so I don't think it's a blocker.

I'd feel better letting someone else do the final signoff though. Maybe @cjgillot?

@bors r? @cjgillot

@rustbot rustbot assigned cjgillot and unassigned eholk Feb 18, 2023
Copy link
Contributor

@cjgillot cjgillot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you make the ElaborateDrops tests output ElaborateDrops.diff instead of ElaborateDrops.after.mir? It will be easier to check the output.

compiler/rustc_mir_build/src/build/scope.rs Outdated Show resolved Hide resolved
tests/mir-opt/issue_41110.test.ElaborateDrops.after.mir Outdated Show resolved Hide resolved
// A drop and replace behind a pointer/array/whatever.
// The borrow checker requires that these locations are initialized before the assignment,
// so we just leave an unconditional drop.
assert!(!data.is_cleanup);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I have a clear idea of MIR looks like in that case. Do you have an example?

Copy link
Contributor Author

@zeegomo zeegomo Feb 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fn replace(c: &mut String) {
    *c = "new".to_string();
}

fn main() {
    let mut c = "old".to_string();
    replace(&mut c);
}

the body of replace gets translated to something like:

bb1: {
        _2 = <something that builds the new string>;
        drop((*_1)) -> [return: bb3, unwind: bb2]; 
    }

bb2 (cleanup): {
        (*_1) = move _2;
        resume;    
}

bb3: {
        (*_1) = move _2;
        return;
    }

Under normal conditions, dropping something behind a pointer would be bad (drop((*_1))), but in a drop and replace it's fine because it's always initialized before any other use.
assert!(!data.is_cleanup) is there because a drop and replace is never emitted in the unwind path so it shouldn't be possible to hit this branch.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case, should we assert that we will assign to *_1 starting both target and unwind bbs? Just to future-proof against changes to MIR building.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's what the check for DesugaringKind::Replace is for.
Alternatively we could check for assignments, but it could be that some unrelated code is inserted before those that does not access that variable and is therefore valid but breaks the check.
For this reason I was kind of considering DesugaringKind::Replace as a guarantee that there will be an assignment later to the same place that is compliant with the semantics of a replace, without having to know exactly how the desugaring is done.
However, I don't see any reason why we should insert stuff between the drop and the assignment for now, so if you think it's safer that way it's quite easy to do. I 'm just worrying it might be a bit sensitive to reorderings in the MIR

@zeegomo
Copy link
Contributor Author

zeegomo commented Feb 23, 2023

Once #106430 is merged I'll rebase this PR on top of that to make reviewing easier

@bors
Copy link
Contributor

bors commented Feb 25, 2023

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

@cjgillot cjgillot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 25, 2023
@zeegomo
Copy link
Contributor Author

zeegomo commented Feb 27, 2023

@rustbot label -S-waiting-on-author +S-waiting-on-review

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 27, 2023
@zeegomo
Copy link
Contributor Author

zeegomo commented Mar 3, 2023

I've looked at the failure and it seems like the code for generating coverage spans for closure signatures is a bit fragile.
As far as I understand this PR changes MIR in a way such that the unstable sort in

initial_spans.sort_unstable_by(|a, b| {
now reorders two statements with the same span the other way around and this messes with later processing.
Changing it to a stable sort fixes this issue and also add coverage info in other closure sigs where it was missing before (presumably because of the same problem).

Since it looks like we are fine with some closures sigs not having coverage info and changing to a stable sort could have perf implications, I would go ahead with this for now (changed the expected output) and deal with this problem in a separate PR

@cjgillot
Copy link
Contributor

cjgillot commented Mar 3, 2023

@bors r+

@bors
Copy link
Contributor

bors commented Mar 3, 2023

📌 Commit d1f7fa5 has been approved by cjgillot

It is now in the queue for this repository.

@bors 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 Mar 3, 2023
@matthiaskrgr
Copy link
Member

@bors rollup=never let's get this out of the queue

@bors
Copy link
Contributor

bors commented Mar 5, 2023

⌛ Testing commit d1f7fa5 with merge 14c54b6...

@bors
Copy link
Contributor

bors commented Mar 5, 2023

☀️ Test successful - checks-actions
Approved by: cjgillot
Pushing 14c54b6 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 5, 2023
@bors bors merged commit 14c54b6 into rust-lang:master Mar 5, 2023
@rustbot rustbot added this to the 1.70.0 milestone Mar 5, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (14c54b6): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-1.0% [-1.1%, -1.0%] 2
Improvements ✅
(secondary)
-2.0% [-2.6%, -0.3%] 7
All ❌✅ (primary) -1.0% [-1.1%, -1.0%] 2

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
0.9% [0.9%, 0.9%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.9% [0.9%, 0.9%] 1

Cycles

This benchmark run did not return any relevant results for this metric.

zeegomo added a commit to zeegomo/rust that referenced this pull request Mar 5, 2023
Desugaring DropAndReplace at MIR build (rust-lang#107844) fixed issue
70919. Add regressions tests, borrowed from rust-lang#102078, to ensure we
check for this in the future.

Co-authored-by: Aaron Hill <aa1ronham@gmail.com>
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 6, 2023
Add regression tests for issue 70919

Desugaring DropAndReplace at MIR build (rust-lang#107844) fixed rust-lang#70919.
Add regressions tests, borrowed from rust-lang#102078, to ensure we check for this in the future.

cc `@Aaron1011`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 6, 2023
Add regression tests for issue 70919

Desugaring DropAndReplace at MIR build (rust-lang#107844) fixed rust-lang#70919.
Add regressions tests, borrowed from rust-lang#102078, to ensure we check for this in the future.

cc ``@Aaron1011``
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Mar 8, 2023
…asko

Remove DropAndReplace terminator

rust-lang#107844 made DropAndReplace unused, let's remove it completely from the codebase.
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Mar 8, 2023
…asko

Remove DropAndReplace terminator

rust-lang#107844 made DropAndReplace unused, let's remove it completely from the codebase.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 8, 2023
…asko

Remove DropAndReplace terminator

rust-lang#107844 made DropAndReplace unused, let's remove it completely from the codebase.
@ChayimFriedman2
Copy link
Contributor

This PR made the following code compile:

struct Foo<'a> {
    v: &'a mut (),
}

impl Drop for Foo<'_> {
    fn drop(&mut self) {}
}

fn bar() {
    let mut v = ();
    let mut x = Foo { v: &mut v };
    drop(x);
    x = Foo { v: &mut v };
}

I didn't see an FCP, so I'm noting.

flip1995 pushed a commit to flip1995/rust that referenced this pull request Mar 10, 2023
…asko

Remove DropAndReplace terminator

rust-lang#107844 made DropAndReplace unused, let's remove it completely from the codebase.
bjorn3 pushed a commit to bjorn3/rust that referenced this pull request Mar 15, 2023
…asko

Remove DropAndReplace terminator

rust-lang#107844 made DropAndReplace unused, let's remove it completely from the codebase.
sunshowers added a commit to oxidecomputer/omicron that referenced this pull request Aug 2, 2023
We previously had to use an atomic to work around a Rust compiler bug.
However, now that rust-lang/rust#107844 is
fixed, this can just be `FnMut`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.