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

rustc_mir: use dataflow analysis to determine whether locals were moved out of. #63518

Closed
wants to merge 2 commits into from

Conversation

@eddyb
Copy link
Member

commented Aug 13, 2019

This replaces one of the remaining cases where a qualif bit is being unset (which prevent further work in this area), with a more robust dataflow analysis pass.

That is, when a local was moved out of, its NeedsDrop bit was unset, to force its eventual Drop to be treated as a noop, but now that's handled by the local not being "maybe-initialized" at the Drop.

Note that the first commit fixes a preexisting bug, and should be reviewed separately.

r? @matthewjasper cc @ecstatic-morse @oli-obk

@Centril

This comment has been minimized.

Copy link
Member

commented Aug 13, 2019

cc #49146

@oli-obk
Copy link
Contributor

left a comment

I finally see a dataflow pass that I can understand 😆 Maybe in the future I can actually write my own.

Left some questions, lgtm otherwise

// Handled separately, in `propagate_call_return`.
PlaceContext::MutatingUse(MutatingUseContext::Call) => {}

// Any assignment to any part of the local initializes it

This comment has been minimized.

Copy link
@oli-obk

oli-obk Aug 13, 2019

Contributor

Why is that just during assignments? Shouldn't StorageLive already be "correct enough" to handle it?

This comment has been minimized.

Copy link
@eddyb

eddyb Aug 13, 2019

Author Member

Because let x; drop(x); is a noop.

match context {
// Only `Operand::Move` and `StorageDead` can deinitialize a local,
// and only if they act on the whole local, not just some part of it.
PlaceContext::NonMutatingUse(NonMutatingUseContext::Move)

This comment has been minimized.

Copy link
@oli-obk

oli-obk Aug 13, 2019

Contributor

aren't moves followed by StorageDead? If there's a reason to special case Move here, why not also special case moving out of the sole field of a struct?

This comment has been minimized.

Copy link
@eddyb

eddyb Aug 13, 2019

Author Member

Drop is between Move and StorageDead, so it can only be affected by the Move.

If there's a reason to special case Move here, why not also special case moving out of the sole field of a struct?

Sure, we could do that, but we weren't doing it before in qualify_consts, so I don't think we should start now.

impl<'a, 'tcx> BitDenotation<'tcx> for MaybeInitializedLocals<'a, 'tcx> {
type Idx = Local;
fn name() -> &'static str {
"has_been_borrowed_locals"

This comment has been minimized.

Copy link
@ecstatic-morse

ecstatic-morse Aug 13, 2019

Contributor

This should get changed

This comment has been minimized.

Copy link
@eddyb

eddyb Aug 14, 2019

Author Member

Oops! You can tell I started by copy-pasting lol.

@matthewjasper
Copy link
Contributor

left a comment

Doesn't this break cases like this?

struct A;

impl Drop for A {
    fn drop(&mut self) {}
}

const C: Option<A> = {
    let mut x = Some(A);
    let y = x;
    x = None;
    y
}; //~ Drop of `x` was previously OK.
@@ -131,6 +131,9 @@ pub fn categorize(context: PlaceContext) -> Option<DefUse> {

PlaceContext::MutatingUse(MutatingUseContext::Store) |

// FIXME(eddyb) this should probably not even appear here, ever.
PlaceContext::MutatingUse(MutatingUseContext::DropAndReplace) |

This comment has been minimized.

Copy link
@matthewjasper

matthewjasper Aug 13, 2019

Contributor

This should be a DefUse::Drop so that we reject:

struct A<'a>(&'a mut i32);

impl Drop for A<'_> {
    fn drop(&mut self) {
        *self.0 = 1;
    }
}

fn main() {
    let mut a = 0;
    let mut b = 0;
    let mut x = A(&mut a);
    let y = &a;   //~ ERROR
    x = A(&mut b);
}

I'm a bit concerned that there's no test for this already.

This comment has been minimized.

Copy link
@eddyb

eddyb Aug 14, 2019

Author Member

Does MIR borrowck use this? I thought this was only for generators.

This comment has been minimized.

Copy link
@matthewjasper

matthewjasper Aug 14, 2019

Contributor

Yes, this is used for borrowck's liveness.

This comment has been minimized.

Copy link
@eddyb

eddyb Aug 14, 2019

Author Member

Well, if it's not DefUse::Def then it's not treated like a mutation, which seems wrong for DropAndReplace?

Then again, there's also this:

// Borrows only consider their local used at the point of the borrow.
// This won't affect the results since we use this analysis for generators
// and we only care about the result at suspension points. Borrows cannot
// cross suspension points so this behavior is unproblematic.
PlaceContext::MutatingUse(MutatingUseContext::Borrow) |

This comment has been minimized.

Copy link
@matthewjasper

matthewjasper Aug 16, 2019

Contributor

Who uses DefUse::Def for mutation? It's for statements/terminators that completely overwrite/invalidate the current value, without reading it.

This comment has been minimized.

Copy link
@eddyb

eddyb Aug 18, 2019

Author Member

Ugh, I find this def-use terminology almost useless outside of its SSA native habitat. I was talking to @ecstatic-morse recently with the same complaint.

I also find it kind of funny that the comment claims "Borrows cannot cross suspension points so this behavior is unproblematic" which is not really the case for the generators used by async fn.

@@ -980,7 +982,8 @@ impl PlaceContext {
/// Returns `true` if this place context represents a drop.
pub fn is_drop(&self) -> bool {
match *self {
PlaceContext::MutatingUse(MutatingUseContext::Drop) => true,
PlaceContext::MutatingUse(MutatingUseContext::Drop) |
PlaceContext::MutatingUse(MutatingUseContext::DropAndReplace) => true,

This comment has been minimized.

Copy link
@matthewjasper

matthewjasper Aug 13, 2019

Contributor

I don't think that DropAndReplace should be here.

This comment has been minimized.

Copy link
@eddyb

eddyb Aug 14, 2019

Author Member

Then I'd say is_drop shouldn't exist and it should be inlined, so each callsite is correct on its own.

@@ -1477,7 +1469,20 @@ impl<'a, 'tcx> Visitor<'tcx> for Checker<'a, 'tcx> {
projection: None,
} = *place {
if NeedsDrop::in_local(self, local) {
Some(self.body.local_decls[local].source_info.span)
// Check whether the local hasn't been moved out of already.
// FIXME(eddyb) use a more efficient dataflow cursor.

This comment has been minimized.

Copy link
@matthewjasper

matthewjasper Aug 13, 2019

Contributor

Does this mean DataflowResultsCursor or are you thinking of something else? I guess there's no need to use a cursor now since we check the state at most once per block.

This comment has been minimized.

Copy link
@eddyb

eddyb Aug 14, 2019

Author Member

Huh, why is DataflowResultsCursor not being used in the generator code, that's where I got this code from. I guess I can look into using the cursor API.

@eddyb

This comment has been minimized.

Copy link
Member Author

commented Aug 14, 2019

Doesn't this break cases like this?

Uhhhhh that's a bug! We never intended for you to be able to unset qualifs like that, except for this move thing, which shouldn't have used NeedsDrop...

I guess in the future dataflow-based system (that @ecstatic-morse is working on) we could have the assignment to x cause all previous assignments to be ignored, but we probably won't do it right away.

That code compiles on beta though, which... is bad, since I think it's going to be on stable in a few days.

@RalfJung

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

But how can such code be okay, given that it calls a non-const drop?

@eddyb

This comment has been minimized.

Copy link
Member Author

commented Aug 14, 2019

@RalfJung The mir::TerminatorKind::Drop won't call any Drop trait impl, because the value is None.

But we wanted to be more conservative and assume that because a Some was previously written to that local, it ended up with a sticky NeedsDrop, it's just that NeedsDrop was also used to signal that the value has been moved out (which makes any drop a noop), and we never considered the interaction between that and reinitialization.

@RalfJung

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

The mir::TerminatorKind::Drop won't call any Drop trait impl, because the value is None.

Ah, I misread the code, thanks.

@eddyb

This comment has been minimized.

Copy link
Member Author

commented Aug 14, 2019

If @ecstatic-morse can confirm their code can just as accurately track the fact that a value was moved out of on all paths leading to a point in the CFG, then this is not a blocker and I guess I'll just close this PR (though the first commit, with the DropAndReplace fixes, should still be landed separately).

@ecstatic-morse

This comment has been minimized.

Copy link
Contributor

commented Aug 18, 2019

@eddyb

I believe that the current behavior (unsetting NeedsDrop when a value is moved out of) is compatible with my formulation of dataflow-based const qualification. It's basically just a "kill" of the NeedsDrop qualif. I wasn't sure if this was adding more functionality on top of that.

The real roadblock occurs when one qualification is mutated dependent on the value of a different qualification as occurs here, where HasMutInterior is replaced with IsNotPromotable and the new IsNotPromotable is propagated across the CFG. This means that the dataflow pass for HasMutInterior and the one for IsNotPromotable must be carried out simultaneously. The comment above that code suggests that this is just for improved diagnostics, so I'm thinking we can change the const-checker to check for HasMutInterior when looking for violations as well as IsNotPromotable. I'm ashamed to admit that I haven't even commented out the lines which mutate qualifs to see which tests break. I was hoping someone remembered what problem that code was solving.

There's actually another place in the current analysis where other qualifs depend on IsNotPromotable, but this is not problematic since it only occurs after qualifs have been propagated.

AFAICT, these are the only two places where a dependency across qualifs exists. If we could remove the first one while preserving the current behavior of the const qualification pass, it would make adding dataflow-based const qualification a lot simpler.

@eddyb

This comment has been minimized.

Copy link
Member Author

commented Aug 19, 2019

Okay I'm pretty sure that's enough, we can close this. Thanks!

@eddyb eddyb closed this Aug 19, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.