Stream: t-compiler/wg-polonius

Topic: rustc compare-mode polonius


lqd (May 07 2019 at 16:32, on Zulip):

btw @nikomatsakis I had this to update the current polonius test expectations, was that what you wanted ? (if so I'll send a PR)

lqd (May 07 2019 at 16:36, on Zulip):

probably the most "interesting" thing is the comment on the 2 issue-40510 tests, as I'm not sure I understand how they would pass in the regular full-nll compare-mode: they don't have .nll.stderr files, are not ignored and yet the .stderr is expecting a warning :thinking:

lqd (May 07 2019 at 16:38, on Zulip):

so it's likely possible to make those 2 tests pass correctly, but I'm not familiar enough with the compare-mode tests to know how

nikomatsakis (May 07 2019 at 17:01, on Zulip):

btw nikomatsakis I had this to update the current polonius test expectations, was that what you wanted ? (if so I'll send a PR)

Seems good, I guess the main bit of work is to investigate whether the errors are indeed expected.

Matthew Jasper (May 07 2019 at 17:23, on Zulip):

I'm not sure I understand how they would pass in the regular full-nll compare-mode

The full nll compare mode pr hadn't been merged yet.

lqd (May 07 2019 at 17:32, on Zulip):

ah yes, thanks Matthew

lqd (May 07 2019 at 17:34, on Zulip):

... investigate whether the errors are indeed expected.

that is, whether the blessed output reflects errors we expect, versus bugs in -Zpolonius ?

nikomatsakis (May 07 2019 at 17:45, on Zulip):

yeah so -- I guess changes to error messages seem fine

nikomatsakis (May 07 2019 at 17:45, on Zulip):

I just remember I had some short list of failures before that I had reviewed

lqd (May 07 2019 at 17:46, on Zulip):

the sheer number of them also makes me think they should be looked at anyway before opening a PR

nikomatsakis (May 07 2019 at 17:46, on Zulip):

this list of failures looks much larger

nikomatsakis (May 07 2019 at 17:46, on Zulip):

like, the list I had before was ~22 tests

nikomatsakis (May 07 2019 at 17:46, on Zulip):

not sure what changed

lqd (May 07 2019 at 17:46, on Zulip):

yeah same

lqd (May 07 2019 at 17:46, on Zulip):

I feel something changed indeed :) I'll keep working on that

lqd (May 07 2019 at 17:48, on Zulip):

the smoke tests ensure the basic things still work so there's at least that, the rest might have regressed discreetly

lokalmatador (May 07 2019 at 19:02, on Zulip):

hi

Matthew Jasper (May 07 2019 at 19:06, on Zulip):

Polonius compare mode compares against NLL compare mode, so the diff will be smaller with #60171 merged

lqd (May 07 2019 at 19:38, on Zulip):

yeah I'll rebase on top of #60171 soon to see how it changes things, a smaller diff will be indeed much easier to analyze/bisect

lqd (May 09 2019 at 11:35, on Zulip):

test result: FAILED. 5443 passed; 19 failed now that's more like it :upside_down:

nikomatsakis (May 09 2019 at 12:05, on Zulip):

@lqd what changed? just a rebase?

lqd (May 09 2019 at 12:11, on Zulip):

retesting over matthew's #60171 indeed

lqd (May 09 2019 at 12:11, on Zulip):

(which hasn't landed yet)

lqd (May 09 2019 at 12:12, on Zulip):

the other 200 differences were because that compare-mode polonius runs with NLLs while the NLL compare-mode runs in migrate-mode

lqd (May 09 2019 at 12:14, on Zulip):

with #60171 the NLL compare-mode will run with the regular NLLs

lqd (May 09 2019 at 17:29, on Zulip):

OK so here are these 19 tests behaving differently under polonius (in a future where #60171 has switched the NLL compare-mode to full NLLs), and I've done a quick WIP first pass at analyzing the results here.

lqd (May 09 2019 at 17:30, on Zulip):

I'll keep working on it this week, to make it actually presentable/understandable, add links and diffs etc, in addition to filling in more content ofc -- so that it can be validated/invalidated.

lqd (May 09 2019 at 17:30, on Zulip):

That being said, it seems to me at least 5 tests should be ignored or a dedicated polonius output identical to the NLL one should be added. Then, a couple are duplicates or similar to other failures. Then, some slight expected differences in diagnostics handling. And a handful "looking interesting", that is, requiring a more in-depth investigation.

lqd (May 09 2019 at 17:44, on Zulip):

(I also wanted to check a thing with Matthew later about the NLL and Polonius compare modes, in that, in cases where the polonius output was expected to be the same as the NLL one, but the latter was manually tested via revisions instead of the compare-mode, whether we should add polonius revisions there as well)

lqd (May 09 2019 at 17:48, on Zulip):

(I also added "correctness and testsuite analysis" to the roadmap like we mentioned yesterday -- and will do the meeting minutes PR soon, including the one from 2 weeks ago I apparently haven’t done either...)

Albin Stjerna (May 13 2019 at 11:49, on Zulip):

@lqd How do you run these tests by the way?

lqd (May 13 2019 at 12:19, on Zulip):

using x.py test, it has a set of flags to trigger the compare mode (and there's one for NLLs, and one for Polonius) like so ./x.py test -i --stage 1 src/test/ui --compare-mode polonius, here for the ui test suite

lqd (May 13 2019 at 12:21, on Zulip):

also, the specific results are ran over #60171 (which Felix is reviewing), so running them on master will produce 200 more failures or so

Albin Stjerna (May 13 2019 at 15:48, on Zulip):

Ah, ok, and I haven't merged in a while either

Albin Stjerna (May 13 2019 at 15:48, on Zulip):

Thanks!

lqd (May 13 2019 at 22:06, on Zulip):

update: the first pass feels somewhat presentable (now a pause to do the meeting minutes PR I've been forgetting) and then hopefully fill more content in this document if I can

lqd (May 14 2019 at 11:29, on Zulip):

hey @Matthew Jasper I was wondering about cases like this in compare mode. The Polonius output is the same as the NLL output, and the test is ignored in NLL compare mode and manually checked via revisions. Would you say that we should ignore them in polonius compare-mode, add the expected Polonius output, by having dedicated revisions, each time tests are constructed in this manner ?

lqd (May 14 2019 at 11:35, on Zulip):

(or maybe there was a compare-mode feature one could use for these cases where one test should be similar to another existing one, for the cases like these where most existing things should continue working as-is in the new feature superset)

Matthew Jasper (May 14 2019 at 12:23, on Zulip):

Ignoring or adding a revision sounds good.

lqd (May 14 2019 at 14:20, on Zulip):

thanks! For these most obvious ones I was thinking to both ignore them and open an issue similar to what we have for NLLs coverage about revisions: "before long, review the uses of '// ignore' in polonius mode to ensure good coverage" as these 4-5 tests are right now literally testing only the NLL part, and failing in compare-mode only. just until things are more "proven" (and CI gated), not have too many revisions testing the same things, nll/migrate/polonius * 2015/2018 to maintain, etc

lqd (Jul 03 2019 at 11:12, on Zulip):

in the current failures analysis @nikomatsakis I ended up with this interesting error. I talked about it with Matthew here and they seem to think it's a problem in fact generation. Since it looks like a simplified version of the dreaded 47680 I wondered if you thought it should be fixed in rustc or in the polonius analysis :thinking:

lqd (Jul 03 2019 at 11:17, on Zulip):

in that the loan from the previous iteration of the loop should be killed or not considered live

lqd (Jul 03 2019 at 11:30, on Zulip):

(this is one of the cases where I'd have found useful to have datalog provenance, so I might look into hacking a simple helper into datafrog)

lqd (Jul 04 2019 at 11:46, on Zulip):

and the MIR annotated with some facts: here (some of the annotations I added are artifacts of Polonius interning so I could better follow the datalog provenance)

lqd (Jul 04 2019 at 16:23, on Zulip):

I think I got it

lqd (Jul 04 2019 at 16:29, on Zulip):

the loans are not killed because we only generate those facts for assignments (and assignments to locals only, not projections, as Matthew mentioned). This case here is not an assignment, it's a call where the destination place has loans. This specific case seems to pass when I kill those loans

lqd (Jul 04 2019 at 16:47, on Zulip):

it also makes borrowck/borrowck-lend-flow-loop.rs pass, so there's that.
(but barely none of the rand reductions, even though I initially started with those, before mixing in a little bit of #47680)

lqd (Jul 04 2019 at 17:30, on Zulip):

@Matthew Jasper rn I'm doing this when visiting a terminator call, do I also need to take special care of possible edge cases with calls, or can I just kill the loans like so on the call's destination ?

Matthew Jasper (Jul 04 2019 at 20:04, on Zulip):

That should be fine, although you should probably try to only do it on the return edge from the call (not the unwind edge).

Matthew Jasper (Jul 04 2019 at 20:05, on Zulip):

It may also be more effective to kill loans on StorageDead statements instead, that may need investigation though.

lqd (Jul 04 2019 at 20:19, on Zulip):

@Matthew Jasper ok I see, thanks a lot for all the help :)

lqd (Jul 05 2019 at 10:58, on Zulip):

plot twist, the test passes for rustc -Zpolonius, but polonius itself actually still reports an error (it used to report 2 before the change), so that's strange. As for the analysis, the remaining error might be because in a single mir statement, the loan is invalidated at the start point but killed at the mid point :thinking: I'll keep tracing

lqd (Jul 06 2019 at 00:34, on Zulip):

I may have a theory, related to invalidating and killing loans in the same mir statement: that happens at 2 different points. The computation of the region/provenance seems to include the statement's start point (because the loans are killed on mid points) and the mid point (because we stop propagating the TC from kill points), and if the region is live and contains the loan (eg in a loop) there will be an error at the start point. I'm not sure I know the reasons why invalidates/killed are at the locations they are, but maybe they should be at the same point (at least if the loan is the same) ? (This is, I believe, causing the 2nd error reported by Polonius I mentioned in the previous message — but I'd still like to understand why rustc didn't mind this error though)

Matthew Jasper (Jul 06 2019 at 07:57, on Zulip):

If you're not killing loans for StorageDead statements, then you'll need to change this https://github.com/rust-lang/rust/blob/b820c761744db080ff7a4ba3ac88d259065cb836/src/librustc_mir/borrow_check/nll/invalidation.rs#L197 to Shallow(None).

Matthew Jasper (Jul 06 2019 at 07:59, on Zulip):

The kill and the invalidates facts are at the same point because we want this to be an error:

let (a, b) = (0, 1);
let mut x = &mut a;
let y = &mut x; // L
x = &mut b; // This both kills and invalidates the loan at L
drop(y);
lqd (Jul 06 2019 at 08:01, on Zulip):

ah interesting thank you, I think some loans are indeed killed on StorageDead, it didn’t seem to be all the time (or maybe not and I'm just thinking about invalidations)

lqd (Jul 06 2019 at 08:05, on Zulip):

yes indeed; they’re lowered as 2 "polonius points" today, for the same "MIR point", I wonder if this was linked to 2PB or something else

lqd (Jul 06 2019 at 08:18, on Zulip):

the rules are setup to handle this, invalidating L on the start point and killing L on the mid point, the loan being live on start makes your example an error

lqd (Jul 06 2019 at 08:19, on Zulip):

(regardless of the kill)

lqd (Jul 06 2019 at 08:26, on Zulip):

so this lowering works for sure in most cases, but not where in MIR a loan is both invalidated and killed at the same point and the same loan is added to a "borrow region" (which also happens in mid points IIRC)

lqd (Jul 06 2019 at 08:30, on Zulip):

making eg this an error

let mut buf = &mut 0;
loop {
    buf = index_mut(buf); // accepted by NLL, rejected by Polonius
}
lqd (Jul 06 2019 at 08:41, on Zulip):

(because the loan from the previous iteration will be live on these 2 points)

lqd (Jul 06 2019 at 09:07, on Zulip):

and if my understanding is correct, it's also the cause of this additional error

let mut x = Vec::new();
loop {
    let mut y = 0; // Polonius rejects this as an assignment to borrowed value
    x.push(&mut y); // here we also have the expected error: y doesn't live long enough
}

(so it's like an off-by-one error where in these specific cases the regions contain the loans a couple points too long)

lqd (Jul 08 2019 at 13:23, on Zulip):

The fix might be obvious but it's not yet the case to me. Things I've tried which didn't work:

nikomatsakis (Jul 09 2019 at 10:14, on Zulip):

It may also be more effective to kill loans on StorageDead statements instead, that may need investigation though.

this should definitely be ok

nikomatsakis (Jul 09 2019 at 10:17, on Zulip):

So, @lqd, reading back over the log, a few questions:

lqd (Jul 09 2019 at 10:18, on Zulip):

unfortunately not

nikomatsakis (Jul 09 2019 at 10:18, on Zulip):

It seems like the "points" we use for polonius may be insufficiently precise

nikomatsakis (Jul 09 2019 at 10:18, on Zulip):

/me thinks

lqd (Jul 09 2019 at 10:18, on Zulip):

I only narrowed it down to this pattern of loops causing problems

nikomatsakis (Jul 09 2019 at 10:19, on Zulip):
let mut buf = &mut 0;
loop {
    buf = index_mut(buf); // accepted by NLL, rejected by Polonius
}

this pattern?

lqd (Jul 09 2019 at 10:19, on Zulip):

in general yes

nikomatsakis (Jul 09 2019 at 10:19, on Zulip):

where presumably index_mut is fn(&mut u32) -> &mut 32

lqd (Jul 09 2019 at 10:19, on Zulip):

but in mir it's a bit more precise

lqd (Jul 09 2019 at 10:19, on Zulip):

here

lqd (Jul 09 2019 at 10:20, on Zulip):

my "fix" only fixes one of the errors (the bottom one) but not the first where we have the invalidation preceding the kill, but the "previous iteration" loan is live on start

lqd (Jul 09 2019 at 10:21, on Zulip):

note that this passes in rustc though, which is another strange thing

nikomatsakis (Jul 09 2019 at 10:22, on Zulip):

Did you try to kill loans on StorageDead?

lqd (Jul 09 2019 at 10:22, on Zulip):

in that polonius -engine returns an error but rustc seems to ignore it (maybe a remnant of switching from computing all the live loans to errors)

nikomatsakis (Jul 09 2019 at 10:22, on Zulip):

Huh

nikomatsakis (Jul 09 2019 at 10:22, on Zulip):

That is strange

lqd (Jul 09 2019 at 10:23, on Zulip):

no I have not but I can try this quick I think

nikomatsakis (Jul 09 2019 at 10:23, on Zulip):

And a bit disturbing :)

lqd (Jul 09 2019 at 10:23, on Zulip):

exactly :)

nikomatsakis (Jul 09 2019 at 10:23, on Zulip):

It seems like

StorageDead(_5);                 // bb4[4]: p32-33

ought to kill L2

nikomatsakis (Jul 09 2019 at 10:23, on Zulip):

(er, wait, is that right?)

lqd (Jul 09 2019 at 10:24, on Zulip):

I'm killing it at the call rn

nikomatsakis (Jul 09 2019 at 10:24, on Zulip):

Yes, that's right.

nikomatsakis (Jul 09 2019 at 10:24, on Zulip):

Well, both of them basically destroy the value of _5

nikomatsakis (Jul 09 2019 at 10:24, on Zulip):

although the semantics of StorageDead are pretty unclear

lqd (Jul 09 2019 at 10:24, on Zulip):

yeah, I also feel we've seen with the generators optimizations that we might not always have StorageDead's everywhere

lqd (Jul 09 2019 at 10:25, on Zulip):

so it seemed safer at the call, but it wasn't enough anyway :)

nikomatsakis (Jul 09 2019 at 10:25, on Zulip):

for unwind paths we don't generate them all the time

lqd (Jul 09 2019 at 10:26, on Zulip):

killing it at the StorageDead fixes the error darn it

nikomatsakis (Jul 09 2019 at 10:28, on Zulip):

well so I think both spots are correct to do the kill

nikomatsakis (Jul 09 2019 at 10:28, on Zulip):

I think you could make a test where the call is required

nikomatsakis (Jul 09 2019 at 10:29, on Zulip):

although i'm wondering what it is :P

nikomatsakis (Jul 09 2019 at 10:29, on Zulip):

it might be that it's hard to do because rustc introduces temporaries

nikomatsakis (Jul 09 2019 at 10:29, on Zulip):

killing it at the StorageDead fixes the error darn it

were you saying just killing it at StorageDead is sufficient?

lqd (Jul 09 2019 at 10:33, on Zulip):

yes

lqd (Jul 09 2019 at 10:33, on Zulip):

just killing it at the storagedead was sufficient

lqd (Jul 09 2019 at 10:33, on Zulip):

to stop both errors, I think (I have to juggle branches ;)

lqd (Jul 09 2019 at 10:35, on Zulip):

are there complications on killing loans on StorageDead ? like, I was wondering whether it wouldn't "lengthen" the regions

nikomatsakis (Jul 09 2019 at 10:35, on Zulip):

so I think that if you have foo = bar(..) where foo was already initialized, we will always generate a temporary intermediate

nikomatsakis (Jul 09 2019 at 10:35, on Zulip):

this is just generally true for foo = ...

nikomatsakis (Jul 09 2019 at 10:36, on Zulip):

are there complications on killing loans on StorageDead ? like, I was wondering whether it wouldn't "lengthen" the regions

say more? I've always assumed we would kill loans there, I think it's actually kinda necessary

lqd (Jul 09 2019 at 10:36, on Zulip):

maybe my thinking is too focused on this example

lqd (Jul 09 2019 at 10:37, on Zulip):

where the StorageDead is a bit later than the call

lqd (Jul 09 2019 at 10:38, on Zulip):

so I didn't want to unexpectedly kills loan "too late" but it doesn't look like it'd matter, esp if it's necessary to do so :)

lqd (Jul 09 2019 at 10:39, on Zulip):

I'll try this out this afternoon, it'd be super nice to have this taken care of

nikomatsakis (Jul 09 2019 at 10:40, on Zulip):

ah, I see what you mean

nikomatsakis (Jul 09 2019 at 10:40, on Zulip):

I mean I agree that we should kill loans at the call return point, too

nikomatsakis (Jul 09 2019 at 10:40, on Zulip):

though I'm trying to come up with an example that would show why :)

lqd (Jul 09 2019 at 10:40, on Zulip):

:)

nikomatsakis (Jul 09 2019 at 10:41, on Zulip):

(it may be that you have to write MIR by hand to show why)

lqd (Jul 09 2019 at 10:42, on Zulip):

can we simply kill loans at both mir statements or do we have to somehow kill at only one of the two depending on the situation ?

lqd (Jul 09 2019 at 10:42, on Zulip):

seems alright to me but I don't know

nikomatsakis (Jul 09 2019 at 10:56, on Zulip):

both is fine

lqd (Jul 09 2019 at 10:57, on Zulip):

sweet I'll try it soon :)

nikomatsakis (Jul 09 2019 at 11:01, on Zulip):

basically any time that the previous value of x is destroyed, we should be able to kill loans of x

nikomatsakis (Jul 09 2019 at 11:02, on Zulip):

(or, as in this case, projections from x)

nikomatsakis (Jul 09 2019 at 11:02, on Zulip):

(note though that if x itself is borrowed (and not *x), then assigning to x may be an error itself)

lqd (Jul 09 2019 at 11:07, on Zulip):

it's true that we also don't handle projections now

lqd (Jul 09 2019 at 13:30, on Zulip):

yesssssssss (thanks Matthew and Niko, that's 2 less rustc regressions, and 7-10 of the different other minimizations I had)

lqd (Jul 15 2019 at 19:29, on Zulip):

only 2 failures left to go (everytime I rebase there's usually another new failure :joy:)

lqd (Jul 16 2019 at 18:57, on Zulip):

(hopefully I have fixed the cases related to assignments to projections, and only the drop data overflow remains, when asking for nll-facts)

lqd (Jul 16 2019 at 23:07, on Zulip):

I think I'll investigate the overflow in a follow-up PR

lqd (Jul 16 2019 at 23:09, on Zulip):

ok so the result of these last couple months of work on this: mostly complete write-up, and the PR #62736 for the fixes and tests

lqd (Jul 17 2019 at 21:25, on Zulip):

@Matthew Jasper thanks for the super quick review

lqd (Jul 17 2019 at 21:25, on Zulip):

also, agreed about your CI + tests comment

lqd (Jul 17 2019 at 21:28, on Zulip):

maybe we can enable them on CI after the correctness and perf investigations have advanced a bit more, at the very least so that we don't slow down CI too much either (the nll compare mode seemed to slow down enough that it's not run on all builders IIRC)

lqd (Jul 17 2019 at 21:30, on Zulip):

if we manage to enable full NLLs this year, maybe we can switch the compare mode to polonius without impacting things :) (not that it takes particularly long to run or anything; I'll think I'll time the 2 tomorrow to make sure, but I haven't noticed a big difference)

lqd (Jul 17 2019 at 21:33, on Zulip):

(I wonder which test suites do we run the nll compare mode on, only ui or some of the others)

Matthew Jasper (Jul 17 2019 at 21:51, on Zulip):

ui and run-pass

lqd (Jul 17 2019 at 21:53, on Zulip):

I've had some "interesting" behaviour on one of the run-pass tests earlier today, the float saturating cast one didn't want to terminate

lqd (Jul 17 2019 at 21:57, on Zulip):

in any case I'll keep working on the last error, try to quantify how long it takes to run, check the run-pass as well, and we can revisit this important question hopefully soon

lqd (Jul 17 2019 at 21:59, on Zulip):

(and likely rebase the PR even sooner than that, as another PR in the queue is going to update a lot of the blessed outputs :)

lqd (Jul 18 2019 at 10:10, on Zulip):

ah yes it's fact generation taking forever/OOM-ing on run-pass/numbers-arithmetic/saturating-float-casts.rs

lqd (Jul 18 2019 at 10:11, on Zulip):

I guess we got ourselves a fact-generation benchmark

lqd (Jul 18 2019 at 14:27, on Zulip):

this is materializing 10948 x 136664 outlives constraints in this test (15GB+)

lqd (Jul 25 2019 at 10:09, on Zulip):

yay #62736 landed

Matthew Jasper (Aug 03 2019 at 09:29, on Zulip):

So, I've finally had a look at issues/issue-38591.rs. The reason that this is failing only for Polonius is that we don't calculate liveness for variables with no free regions with normal NLL, but we generate facts for all variables with Polonius. This shouldn't be too hard to fix, but it's probably easier to wait until we have the final fact generation code for liveness.

lqd (Aug 03 2019 at 11:17, on Zulip):

oh thanks for looking into it ! makes sense

lqd (Aug 03 2019 at 11:21, on Zulip):

I’m looking into "interesting" cases about statics, which Polonius dislikes for some unknown reason as of yet

lqd (Aug 03 2019 at 11:22, on Zulip):

but doesn’t trigger errors when called by rustc so, yeah

Albin Stjerna (Aug 04 2019 at 10:53, on Zulip):

So, I've finally had a look at issues/issue-38591.rs. The reason that this is failing only for Polonius is that we don't calculate liveness for variables with no free regions with normal NLL, but we generate facts for all variables with Polonius. This shouldn't be too hard to fix, but it's probably easier to wait until we have the final fact generation code for liveness.

This sounds a lot like it might interact with the liveness code I'm working on, right?

Matthew Jasper (Aug 04 2019 at 11:01, on Zulip):

yes

lqd (Oct 04 2019 at 11:40, on Zulip):

Matthews's PR #64749 has now landed :tada: a lot of inconsequential diagnostics differences have been eliminated, and the last (known) bug in fact generation has been fixed!

Last update: Nov 15 2019 at 20:35UTC