Stream: t-compiler

Topic: pr-52782-dangly-paths-for-box


pnkfelix (Aug 01 2018 at 11:25, on Zulip):

@eddyb so here's a super simple example of something that breaks if you leave out the (shallow) Write(StorageDeadOrDrop) of the deref-box:

pnkfelix (Aug 01 2018 at 11:25, on Zulip):
fn return_local_box_borrow<'a>() -> &'a i32 {
    let b = Box::new(10);
    &*b
}
pnkfelix (Aug 01 2018 at 11:26, on Zulip):

if you leave out that step, the above code gets accepted by NLL.

DPC (Aug 01 2018 at 11:26, on Zulip):

(deleted)

eddyb (Aug 01 2018 at 11:30, on Zulip):

okay but why? isn't the lifetime of the borrow tied to b somehow?

eddyb (Aug 01 2018 at 11:30, on Zulip):

I'd expect it to at least not be able to escape the function

eddyb (Aug 01 2018 at 11:30, on Zulip):

'a shouldn't outlive b

eddyb (Aug 01 2018 at 11:31, on Zulip):

(I think this was @RalfJ's point - lol I can force subscribe them)

eddyb (Aug 01 2018 at 11:32, on Zulip):

what happens if you move b to another local variable? does the borrow survive?

eddyb (Aug 01 2018 at 11:32, on Zulip):

since a move of a b shouldn't touch *b either

eddyb (Aug 01 2018 at 11:33, on Zulip):

or, say, call mem::drop(b), which would have no local Drop(b) to speak of - nvm it would, it's drop elaboration that removes it later

eddyb (Aug 01 2018 at 11:33, on Zulip):

it really feels like StorageDead(b) should kill the &*b but maybe I'm missing something

eddyb (Aug 01 2018 at 11:34, on Zulip):

maybe "variables have no scope anymore" and the only thing that can end a borrow is explicit invalidation of the thing being borrowed, and *b is distinct enough from b

eddyb (Aug 01 2018 at 11:34, on Zulip):

NLL is a brave new world I suppose

eddyb (Aug 01 2018 at 11:35, on Zulip):

is... regionck... just dead/swallowed whole by NLL?

pnkfelix (Aug 01 2018 at 11:55, on Zulip):

(sorry i was distracted for a bit)

pnkfelix (Aug 01 2018 at 11:56, on Zulip):

so lets see if I can address these in turn

pnkfelix (Aug 01 2018 at 11:56, on Zulip):

I don't see why 'a itself shouldn't outlive b.

pnkfelix (Aug 01 2018 at 11:57, on Zulip):

after all, if the user passed in input: &'a i32, then I should certainly be able to return &*input, right?

pnkfelix (Aug 01 2018 at 11:58, on Zulip):

if you do a move of b, then that does error. It says "cannot move out of b because it is borrowed."

pnkfelix (Aug 01 2018 at 11:59, on Zulip):

but that makes sense, since a move of b will cause a (Deep, Write(WriteKind::Move) on b

pnkfelix (Aug 01 2018 at 12:02, on Zulip):

and likewise, before PR #52782, the drop of b would do a similar Deep Write (because Box<T> implements Drop)

pnkfelix (Aug 01 2018 at 12:03, on Zulip):

@eddyb wrote:

it really feels like StorageDead(b) should kill the &*b but maybe I'm missing something

Right now a StorageDead statement causes a Shallow Write. But maybe the right answer here is for these other steps to be attached there?

eddyb (Aug 01 2018 at 12:06, on Zulip):

aaah, so the deep version handles the general case, and your PR has to manually emulate the necessary steps!

pnkfelix (Aug 01 2018 at 12:06, on Zulip):

I guess that is a question: where should &*b get invalidated, by the drop(b) or by the StorageDead(b)

pnkfelix (Aug 01 2018 at 12:06, on Zulip):

aaah, so the deep version handles the general case, and your PR has to manually emulate the necessary steps!

Yes!

eddyb (Aug 01 2018 at 12:06, on Zulip):

okay I didn't realize a move was deep by default

pnkfelix (Aug 01 2018 at 12:06, on Zulip):

Yeah, I think it has to be, right?

pnkfelix (Aug 01 2018 at 12:07, on Zulip):

/me thinks

eddyb (Aug 01 2018 at 12:07, on Zulip):

have you seen https://github.com/rust-lang/rust/pull/52782#discussion_r206831556?

pnkfelix (Aug 01 2018 at 12:07, on Zulip):

have you seen https://github.com/rust-lang/rust/pull/52782#discussion_r206831556?

no I had not seen that yet. Let me read it.

eddyb (Aug 01 2018 at 12:08, on Zulip):

specifically the part where Drop(place) should always invalidate the value at place despite being a noop for some types (place is *b here)

pnkfelix (Aug 01 2018 at 12:08, on Zulip):

that ... might be a more elegant way to deal with this. Let me see.

eddyb (Aug 01 2018 at 12:11, on Zulip):

you know, I almost seriously proposed adding &own T to rustc internally to model locals which may be unsized :P

pnkfelix (Aug 01 2018 at 12:12, on Zulip):

well a tree-like presentation can be a pain for understand the flow of a conversation over time. I guess internals.rust-lang.org's engine gets this right

pnkfelix (Aug 01 2018 at 12:12, on Zulip):

(except that I hate that I have to scroll in order to get it to load content. But that's an orthogonal issue.)

pnkfelix (Aug 01 2018 at 12:15, on Zulip):

specifically the part where Drop(place) should always invalidate the value at place despite being a noop for some types (place is *b here)

My immediate thought is that this might cause problems for other similar cases where we have a &mut T that isn't actually getting written. But maybe if the type is !needs_drop, then we should just use a Shallow write rather than a Deep one? I wonder if that would do the trick.

pnkfelix (Aug 01 2018 at 12:19, on Zulip):

I wrote:

Right now a StorageDead statement causes a Shallow Write. \[...\]

and in fact that implies that the Shallow write to b (as opposed to *b) is not necessary in my PR. Because that is already done by the StorageDead statement.

pnkfelix (Aug 01 2018 at 12:20, on Zulip):

so, yay, I can definitely remove that.

eddyb (Aug 01 2018 at 12:21, on Zulip):

you mean you can remove step 3? that'd be nice :D

pnkfelix (Aug 01 2018 at 12:22, on Zulip):

I can definitely remove step 3

pnkfelix (Aug 01 2018 at 12:22, on Zulip):

and I am seeing if I can replace step 2 with a revision of the handling of the general case

pnkfelix (Aug 01 2018 at 12:23, on Zulip):

so that instead of only doing a write for needs_drop stuff

pnkfelix (Aug 01 2018 at 12:23, on Zulip):

it will do some kind of write even if its non needs_drop. It might need to be a shallow write though

eddyb (Aug 01 2018 at 12:24, on Zulip):

/me crosses fingers his obsession with semantic optimization of logic doesn't bite us back later

pnkfelix (Aug 01 2018 at 12:25, on Zulip):

right, so if the general case does a deep write in all cases, then we "fail" to fix #45696

pnkfelix (Aug 01 2018 at 12:25, on Zulip):

that's totally unsurprising

pnkfelix (Aug 01 2018 at 12:27, on Zulip):

but the (good?) news is that doing a deep write doesn't break anything except #45696

eddyb (Aug 01 2018 at 12:28, on Zulip):

which is entirely expected, right? I mean, it was doing that before

pnkfelix (Aug 01 2018 at 12:28, on Zulip):

No this is more general

pnkfelix (Aug 01 2018 at 12:29, on Zulip):

because this is changing code for the non-box cases too

pnkfelix (Aug 01 2018 at 12:29, on Zulip):

(right?)

eddyb (Aug 01 2018 at 12:29, on Zulip):

oh I thought you changed your PR, oops, you meant StorageDead?

pnkfelix (Aug 01 2018 at 12:29, on Zulip):

as in, I'm playing games with the _ => ... arm at the end of the match

pnkfelix (Aug 01 2018 at 12:29, on Zulip):

I haven't pushed any changes to my visible branch yet

eddyb (Aug 01 2018 at 12:29, on Zulip):

Drop then?

pnkfelix (Aug 01 2018 at 12:29, on Zulip):

yeah I'm changing visit_terminator_drop

pnkfelix (Aug 01 2018 at 12:30, on Zulip):

We could talk about whether this belongs in StorageDead instead

pnkfelix (Aug 01 2018 at 12:30, on Zulip):

but that sounds like a more invasive change

eddyb (Aug 01 2018 at 12:30, on Zulip):

oh yeah we were talking about Drop(x) invalidating x even if !needs_drop

pnkfelix (Aug 01 2018 at 12:33, on Zulip):

I'm sort of surprised this change didn't break anything besides #45696 though. I'm going to play around with it a little more, try to double check that this doesn't represent some weakness in our test suite.

eddyb (Aug 01 2018 at 12:35, on Zulip):

well, Box<T> is the only thing that owns without getting StorageDead called on the owned (T) later

pnkfelix (Aug 01 2018 at 12:36, on Zulip):

Yes, but a deep write on drop(x) seems bad when x: &mut T, right?

eddyb (Aug 01 2018 at 12:36, on Zulip):

do these deep things go through references?

pnkfelix (Aug 01 2018 at 12:36, on Zulip):

that I think is one of the main distinctions

eddyb (Aug 01 2018 at 12:36, on Zulip):

and StorageDead is not deep either?

pnkfelix (Aug 01 2018 at 12:36, on Zulip):

StorageDead is just shallow

eddyb (Aug 01 2018 at 12:37, on Zulip):

okay that is suspicious, I agree

pnkfelix (Aug 01 2018 at 12:37, on Zulip):

that I think is one of the main distinctions

let me double-check this claim

eddyb (Aug 01 2018 at 12:39, on Zulip):

so wait, why would a move be deep but a drop & StorageDead be shallow?!

pnkfelix (Aug 01 2018 at 12:39, on Zulip):

because a move can subsequently do something to the &mut borrowed thing?

pnkfelix (Aug 01 2018 at 12:40, on Zulip):

but a drop just dies?

pnkfelix (Aug 01 2018 at 12:40, on Zulip):

not the move itself

pnkfelix (Aug 01 2018 at 12:40, on Zulip):

but the thing you moved it into, that is

eddyb (Aug 01 2018 at 12:40, on Zulip):

oh so it's the repossession of the value being moved?

pnkfelix (Aug 01 2018 at 12:40, on Zulip):

am I making sense?

pnkfelix (Aug 01 2018 at 12:40, on Zulip):

yeah that's how I look at it at least

pnkfelix (Aug 01 2018 at 12:40, on Zulip):

I'm sure ariel and niko have a more disciplined mental model here

eddyb (Aug 01 2018 at 12:41, on Zulip):

so it's a combination of stealing the value (which has the deep implications) and invalidating the place where the value was (like writing undef to it. which has the shallow implications)

pnkfelix (Aug 01 2018 at 12:42, on Zulip):

Yeah so places_conflict.rs has the relevant code here

pnkfelix (Aug 01 2018 at 13:00, on Zulip):

okay that is suspicious, I agree

Ah, the reason that the tests pass is that we don't even generate the Drop(x) MIR instruction when x: &mut T...

pnkfelix (Aug 01 2018 at 13:00, on Zulip):

we just have a StorageDead

pnkfelix (Aug 01 2018 at 13:01, on Zulip):

so let me keep looking/poking...

pnkfelix (Aug 01 2018 at 13:06, on Zulip):

so I think the only cases that break when I add this Deep Write to the base case for visit_terminator_drop end up being related to #45696 in the end, because in order to observe the new Deep write that I've emitted here, you need to have some Drop statement emitted in the first place, which means you need to have some kind of droppable thing that owns the &mut T in question

eddyb (Aug 01 2018 at 13:08, on Zulip):

what if... you remove the optimization from librustc_mir/build/scope.rs?

eddyb (Aug 01 2018 at 13:09, on Zulip):

does shallow vs deep make a huge difference then? I hope that nothing breaks because of all the extra drops

pnkfelix (Aug 01 2018 at 13:09, on Zulip):

you mean the early return if !needs_drop at the beginning of fn schedule_drop ?

pnkfelix (Aug 01 2018 at 13:10, on Zulip):

I imagine that if I were to do that, then the shallow vs deep distinction in this case would be hugely important.

pnkfelix (Aug 01 2018 at 13:10, on Zulip):

but hey, since I'm already experimenting, I can find out.

pnkfelix (Aug 01 2018 at 13:15, on Zulip):

does shallow vs deep make a huge difference then? I hope that nothing breaks because of all the extra drops

To be clear: I'm not planning on putting in a Deep write. If I did that, then #45696 would not get fixed, and this PR would be pointless.

pnkfelix (Aug 01 2018 at 13:15, on Zulip):

I was just trying it out as an experiment to try to understand the implications of such a change.

eddyb (Aug 01 2018 at 13:19, on Zulip):

yeah, I'm just curious what happens

pnkfelix (Aug 01 2018 at 13:23, on Zulip):

as am I. I first want to double check that this change actually works when the write is Shallow. If so, then I'll start playing with removing the optimization in schedule_drop

pnkfelix (Aug 01 2018 at 14:31, on Zulip):

okay, if you make the Deep Write happen both when needs_drop and !needs_drop, and you remove the optimization in schedule_drop, then a bunch of tests break. The simplest instance being

fn return_local_reborrow<'a>(x: &'a mut i32) -> &'a mut i32 {
    &mut *x
}
eddyb (Aug 01 2018 at 14:32, on Zulip):

things are starting to make sense :D

pnkfelix (Aug 01 2018 at 14:32, on Zulip):

now to find out what happens with my planned change to use Shallow Write if we get rid of the optimization in schedule_drop

pnkfelix (Aug 01 2018 at 14:38, on Zulip):

interesting: even with just a Shallow Write, if you get rid of the optimization in schedule_drop, we get 13 test failures ... (compared to 19 failures with Deep Write when killing the schedule_drop optimization).

pnkfelix (Aug 01 2018 at 14:38, on Zulip):

O

pnkfelix (Aug 01 2018 at 14:38, on Zulip):

Oh wait

pnkfelix (Aug 01 2018 at 14:38, on Zulip):

I need a control group

eddyb (Aug 01 2018 at 14:38, on Zulip):

#science

pnkfelix (Aug 01 2018 at 14:38, on Zulip):

As in, I need to try the schedule_drop change without adding an Write to this case at all. :)

eddyb (Aug 01 2018 at 14:38, on Zulip):

right, there might be latent "bugs"

pnkfelix (Aug 01 2018 at 14:39, on Zulip):

trying that now

pnkfelix (Aug 01 2018 at 14:46, on Zulip):

heh. 12 test failures from the schedule_drop change if I leave the Write change out of MIR-borrowck

pnkfelix (Aug 01 2018 at 14:47, on Zulip):

so there was one case where the Shallow Write on !needs_drop caused a failure if we don't include that optimization in schedule_drop. I'll work on identifying that and documenting it somewhere in the code.

pnkfelix (Aug 01 2018 at 14:48, on Zulip):

(This is based on the compile-fail tests; i arguably also need to test against the ui tests)

pnkfelix (Aug 01 2018 at 14:58, on Zulip):

or.. I just get 12 test failures this time. Shoot I wish I had saved that previous run's transcript

pnkfelix (Aug 01 2018 at 14:58, on Zulip):

I bet its one of the intermittent compile-fail tests that keeps biting me on this box

eddyb (Aug 01 2018 at 15:00, on Zulip):

the LLVM/link/something race one? I see the same thing, heh. what are the other 12?

eddyb (Aug 01 2018 at 15:00, on Zulip):

there might be a simple bug to fix, I'll look into it if you can share some logs

pnkfelix (Aug 01 2018 at 15:22, on Zulip):

a log like this: https://gist.github.com/1d49a599cdaac9030bb15518463b8fd8 ?

pnkfelix (Aug 01 2018 at 15:23, on Zulip):

or do you want RUST_LOG output?

pnkfelix (Aug 01 2018 at 15:26, on Zulip):

hmm, skimming over this myself, its "obvious" what the two-phase borrow breakage is... its treating the newly introduced drop as a use and complaining about it being a second use when we expect only one.

pnkfelix (Aug 01 2018 at 15:30, on Zulip):

and then the const-eval errors all seem like something where the new Drops are probably causing us to emit three (redundant) errors instead of "just" two?

pnkfelix (Aug 01 2018 at 15:35, on Zulip):

so I don't think there 's evidence of any serious bug lying in wait.

eddyb (Aug 01 2018 at 15:39, on Zulip):

only compile-fail fails? run-pass is fine?

eddyb (Aug 01 2018 at 15:41, on Zulip):

@pnkfelix I think I like these results, they seem to be confirming that trivial Drops are harmless/unnecessary

pnkfelix (Aug 01 2018 at 15:46, on Zulip):

oh no I would need to check run-pass too

pnkfelix (Aug 01 2018 at 15:46, on Zulip):

my setup here is sort of optimized for only focusing on one test suite at a time

eddyb (Aug 01 2018 at 15:46, on Zulip):

try this :P ./x.py test --no-fail-fast --stage 1 src/test/{mir-opt,codegen,codegen-units,incremental,debuginfo,parse-fail,ui,compile-fail,run-fail,run-pass}

pnkfelix (Aug 01 2018 at 15:46, on Zulip):

I'll look into that after I finish rebasing the PR

pnkfelix (Aug 01 2018 at 15:47, on Zulip):

(and double checking that it works...)

pnkfelix (Aug 01 2018 at 15:48, on Zulip):

uh oh github is error 500'ing on my PR. :(

Jake Goulding (Aug 01 2018 at 16:29, on Zulip):

GH had a hiccup there, seems back

pnkfelix (Aug 02 2018 at 12:04, on Zulip):

@eddyb @RalfJ do you two like the new version of PR #52782 ?

RalfJ (Aug 02 2018 at 15:06, on Zulip):

I cannot really read NLL code and get anything out of that^^

RalfJ (Aug 02 2018 at 15:06, on Zulip):

my previous comments were entirely based on @eddyb's comments

pnkfelix (Aug 02 2018 at 15:07, on Zulip):

fair enough

Last update: Nov 16 2019 at 02:15UTC