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

Avoid Operand::Copy with &mut T #72093

Merged
merged 3 commits into from
May 26, 2020
Merged

Avoid Operand::Copy with &mut T #72093

merged 3 commits into from
May 26, 2020

Conversation

jonas-schievink
Copy link
Contributor

@jonas-schievink jonas-schievink commented May 10, 2020

This is generally unsound to do, as the copied type is assumed to implement
Copy.

Closes #46420

@rust-highfive
Copy link
Collaborator

r? @petrochenkov

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 10, 2020
@jonas-schievink

This comment has been minimized.

@petrochenkov
Copy link
Contributor

r? @RalfJung

@RalfJung
Copy link
Member

This code is entirely unfamiliar to me, sorry... no idea why @petrochenkov thought I might be a good reviewer here.
r? @oli-obk maybe?

@rust-highfive rust-highfive assigned oli-obk and unassigned RalfJung May 11, 2020
@@ -688,7 +688,7 @@ fn build_call_shim<'tcx>(

let rcvr = rcvr_adjustment.map(|rcvr_adjustment| match rcvr_adjustment {
Adjustment::Identity => Operand::Move(rcvr_place()),
Adjustment::Deref => Operand::Copy(tcx.mk_place_deref(rcvr_place())),
Adjustment::Deref => Operand::Move(tcx.mk_place_deref(rcvr_place())), // Can't copy `&mut`
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a mir-opt test showing this

@oli-obk
Copy link
Contributor

oli-obk commented May 11, 2020

I think if we're enforcing this invariant, we should have a mir pass that validates the various (I guess we have more) invariants that we want to be upheld. We could have a flag that runs this MirPass after every other MirPass to ensure that all of our transformations are sound (enough). We don't want to do this in production, as that'll regress perf, but we could run the pass once before going to llvm.

That said, I am not sure what actual effects Operand::Copy has vs Operand::Move except for maybe borrowck? I always thought that post-borrowck we could normalize everything to Operand::Copy (or do my preferred change which is replace Copy and Move by a single variant and add a bool flag that defines which variant we're using here).

cc @rust-lang/wg-mir-opt

@RalfJung
Copy link
Member

RalfJung commented May 11, 2020

There are ideas about making a Move actually replace the source operand with uninitialized memory (@eddyb suggested this AFAIK -- there is an issue about this somewhere but I cannot find it right now). But so far it is unclear which optimizations this actually enables, and having a read perform mutation like that could also cause plenty of problems on its own.

EDIT: Here is the issue: rust-lang/unsafe-code-guidelines#188.

@jonas-schievink
Copy link
Contributor Author

Having Operand::Move uninitialize the moved-from place would certainly help out with generator optimizations and also makes NRVO simpler

@RalfJung
Copy link
Member

RalfJung commented May 11, 2020

@jonas-schievink please post details in the issue I linked. So far, any time someone claimed that, a closer look revealed deinit-on-Move wasn't actually sufficient and we rather need something like #61849 to also rule out re-initialization via raw pointers.

@jonas-schievink
Copy link
Contributor Author

Ah, yes, that one is what I meant

@RalfJung
Copy link
Member

RalfJung commented May 11, 2020

#61849 is very different from rust-lang/unsafe-code-guidelines#188.

#61849 is basically free(ptr); ptr = malloc(...);. So, even if there are aliases elsewhere they are now definitely dead, and even the observed address can change.

Deinit-on-Move (rust-lang/unsafe-code-guidelines#188) changes the content of the storage, but on its own it doesn't do anything about aliases (though maybe Stacked Borrows could be used to achieve something like that, further research is needed here). And it certainly does nothing wrt. equality of observed addresses.

@Elinvynia Elinvynia 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 May 20, 2020
@bors
Copy link
Contributor

bors commented May 21, 2020

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

@jonas-schievink
Copy link
Contributor Author

I've added a small validation pass and a -Zvalidate-mir flag to run it after each optimization pass.

@RalfJung
Copy link
Member

I've added a small validation pass and a -Zvalidate-mir flag to run it after each optimization pass.

That's awesome, I about about 100 ideas for things this could check.^^ Just talked about that with @eddyb earlier today, in fact.

Can we run that pass at least once, say at the beginning and end of mir_optimized (in particular, before and after generator transform), even without the -Z flag when debug assertions are enabled? Then we could cover the entire test suite on the debug CI builders.

@jonas-schievink
Copy link
Contributor Author

I ran it on the testsuite already and found nothing. Right now it only checks 2 invariants though (Copy-ness of Operand::Copy, and self-assignments). Which other ones did you have in mind? Is the conversation you had public?

@RalfJung
Copy link
Member

No that was PMs on Zulip.

The main other check is: for every assignment, make sure the type of LHS and RHS match. (They are not always fully equal though, just "compatible enough"). We could also check that there is only one Return in the entire function. We could check that the operand of a Call is of type FnDef or FnPtr. And so on.

@RalfJung
Copy link
Member

I ran it on the testsuite already and found nothing.

Good to know that this is true right now, but I'd prefer to make sure it stays that way. :) A check after every pass is probably overkill for that, but some check should IMO be done by default when debug assertions are enabled.

@oli-obk
Copy link
Contributor

oli-obk commented May 25, 2020

I agree with running this check once when debug assertions are enabled. Maybe at the end of optimized_mir so we at least know that the final thing we give to codegen is sane?

r=me with that

@jonas-schievink
Copy link
Contributor Author

@bors r=oli-obk rollup=never

@bors
Copy link
Contributor

bors commented May 25, 2020

📌 Commit fe1753a has been approved by oli-obk

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 25, 2020
@bors
Copy link
Contributor

bors commented May 26, 2020

⌛ Testing commit fe1753a with merge cb272d5...

@bors
Copy link
Contributor

bors commented May 26, 2020

☀️ Test successful - checks-azure
Approved by: oli-obk
Pushing cb272d5 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 26, 2020
@bors bors merged commit cb272d5 into rust-lang:master May 26, 2020
@@ -179,6 +191,11 @@ pub fn run_passes(
}

body.phase = mir_phase;

if mir_phase == MirPhase::Optimized {
validate::Validator { when: format!("end of phase {:?}", mir_phase) }
Copy link
Member

Choose a reason for hiding this comment

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

So we run it here even without debug assertions? Isn't that expensive?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The pass does very little work, so it's unlikely to be an issue

Copy link
Member

Choose a reason for hiding this comment

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

The pass does very little work

That is something I want to change.^^ #72796 does some non-trivial type folding.

@jonas-schievink jonas-schievink deleted the unmut branch May 30, 2020 21:32
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Jun 28, 2020
…ewjasper

MIR sanity check: validate types on assignment

This expands the MIR validation added by @jonas-schievink in rust-lang#72093 to also check that on an assignment, the types of both sides match.

Cc @eddyb @oli-obk
Manishearth added a commit to Manishearth/rust that referenced this pull request Jun 28, 2020
…ewjasper

MIR sanity check: validate types on assignment

This expands the MIR validation added by @jonas-schievink in rust-lang#72093 to also check that on an assignment, the types of both sides match.

Cc @eddyb @oli-obk
Manishearth added a commit to Manishearth/rust that referenced this pull request Jun 28, 2020
…ewjasper

MIR sanity check: validate types on assignment

This expands the MIR validation added by @jonas-schievink in rust-lang#72093 to also check that on an assignment, the types of both sides match.

Cc @eddyb @oli-obk
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MIR InstCombine introduces copies of mutable borrows
7 participants