Stream: t-compiler/wg-nll

Topic: #53695 unusable variables


lqd (Sep 12 2018 at 13:21, on Zulip):

@pnkfelix is this what you had in mind for the #53695 experiment ? if so, out of the 6693 NLL ui tests my build ran, 7 failed (as it impacts the diagnostics showing where the borrow is used later).

pnkfelix (Sep 12 2018 at 13:22, on Zulip):

yes that looks right

pnkfelix (Sep 12 2018 at 13:22, on Zulip):

Is there an easy way for you to share the changes to the diagnostics? Maybe --bless and then diff?

pnkfelix (Sep 12 2018 at 13:23, on Zulip):

of course the thing we are probably more concerned about is run-pass failures

lqd (Sep 12 2018 at 13:23, on Zulip):

yeah I'll gather this now

davidtwco (Sep 12 2018 at 13:23, on Zulip):

I got the impression that you'd expect more diagnostics because this would cause things that previously wouldn't error until used to error straight away.

pnkfelix (Sep 12 2018 at 13:23, on Zulip):

i.e. places where the injected read causes a previous working test to start failling

lqd (Sep 12 2018 at 13:24, on Zulip):

I can run the run-pass tests without having to go through the UI tests first I assume ?

pnkfelix (Sep 12 2018 at 13:24, on Zulip):

If you want to do a run that focuses just on run-pass, I think you do --mode=run-pass to compiletest

davidtwco (Sep 12 2018 at 13:24, on Zulip):

I'd normally do x.py test src/test/run-pass or with the migrated run-pass tests x.py test src/test/ui --test-args run-pass.

pnkfelix (Sep 12 2018 at 13:25, on Zulip):

(I'm actually poking at that code now as part of #54047

lqd (Sep 12 2018 at 13:25, on Zulip):

ok thank you both, I'll gather more data and report back

pnkfelix (Sep 12 2018 at 13:25, on Zulip):

@lqd the suggestion from @davidtwco is probably good too.

pnkfelix (Sep 12 2018 at 13:26, on Zulip):

if one just did x.py test run-pass, would that check every path that matches .*run-pass.* ?

lqd (Sep 12 2018 at 13:27, on Zulip):

yeah I'll try David's suggestion!

davidtwco (Sep 12 2018 at 13:28, on Zulip):

If I'm not mistaken, it wouldn't. I think that it matches against these paths to find the right step registered in bootstrap.

lqd (Sep 12 2018 at 13:31, on Zulip):

ui-tests.diff

lqd (Sep 12 2018 at 13:31, on Zulip):

then half of the mir-opt tests failed, and I'll try the run-pass now

davidtwco (Sep 12 2018 at 13:32, on Zulip):

IIRC mir-opt just compares the MIR to make sure it generates what is expected - so adding a new statement would make me expect those to fail.

lqd (Sep 12 2018 at 13:32, on Zulip):

yeah same

lqd (Sep 12 2018 at 13:33, on Zulip):

would it be easier if we saw these on Travis ? that is, if I make a PR, would the builds stop at the ui tests failures or could we see the whole picture of the fallout ?

davidtwco (Sep 12 2018 at 13:33, on Zulip):

It stops after one suite fails.

lqd (Sep 12 2018 at 13:33, on Zulip):

alright :)

davidtwco (Sep 12 2018 at 13:34, on Zulip):

You can run x.py test --no-fail-fast locally and it'll just go through all of the suites.

lqd (Sep 12 2018 at 13:35, on Zulip):

@davidtwco how do you clear the cache so tests are not ignored when reran btw ?

lqd (Sep 12 2018 at 13:35, on Zulip):

(I want to check the previous --bless didn't impact the run-pass)

davidtwco (Sep 12 2018 at 13:36, on Zulip):

rm -r build/x86_64-unknown-linux-gnu/test (replace that with a similar path for whatever target you're testing against)

davidtwco (Sep 12 2018 at 13:36, on Zulip):

It's something like that.

pnkfelix (Sep 12 2018 at 13:36, on Zulip):

I think its build/$TARGET-$HOSTOS/test/

davidtwco (Sep 12 2018 at 13:37, on Zulip):

I wrote it once and have CTRL+R'd since.

lqd (Sep 12 2018 at 13:39, on Zulip):

(sorry if this question doesn't make sense) do we need to do something so that the run-pass is ran in nll mode ?

lqd (Sep 12 2018 at 13:40, on Zulip):

(if it doesn't make sense and NLL tests are in there as well: there are no failures)

davidtwco (Sep 12 2018 at 13:40, on Zulip):

UI runs once with the default compare-mode and then again with the NLL compare-mode.

lqd (Sep 12 2018 at 13:40, on Zulip):

yeah but the run-pass doesn't, right ?

davidtwco (Sep 12 2018 at 13:41, on Zulip):

There are some tests that have #![features(nll)] and run on NLL regardless. But then on the compare-mode NLL run, every test is compiled (and run) with that.

davidtwco (Sep 12 2018 at 13:41, on Zulip):

Oh, those in the run-pass suite. No.

davidtwco (Sep 12 2018 at 13:41, on Zulip):

Only those in the run-pass suite that are specifically annotated with NLL will run with NLL.

lqd (Sep 12 2018 at 13:41, on Zulip):

right

lqd (Sep 12 2018 at 13:43, on Zulip):

then however many NLL tests there are in the run-pass suite are seemingly unaffected @pnkfelix

pnkfelix (Sep 12 2018 at 13:44, on Zulip):

yeah but the run-pass doesn't, right ?

this is why I filed and have been attempting to fix #53764

lqd (Sep 12 2018 at 13:45, on Zulip):

sensible :)

pnkfelix (Sep 12 2018 at 13:45, on Zulip):

You might be able to make headway by adding -Z borrowck=mir -Z two-phase-borrows to your RUSTFLAGS environment variable

lqd (Sep 12 2018 at 13:46, on Zulip):

let's try that

pnkfelix (Sep 12 2018 at 13:46, on Zulip):

Or you could just locally test your experiment atop my PR #54139 which migrates 99.9% of the remainder of run-pass to ui

lqd (Sep 12 2018 at 13:47, on Zulip):

something tells me changing rustflags is going to rebuild from scratch though

lqd (Sep 12 2018 at 13:52, on Zulip):

@pnkfelix yup :/ maybe it should be one of the other rustflags ?

pnkfelix (Sep 12 2018 at 13:52, on Zulip):

are you using x.py, or are you running compiletest directly?

lqd (Sep 12 2018 at 13:53, on Zulip):

x.py

lqd (Sep 12 2018 at 13:53, on Zulip):

david's line to be precise

pnkfelix (Sep 12 2018 at 13:53, on Zulip):

what I would do is pass --verbose to x.py test to see how its invoking compiletest to do the run-pass test suite, and then run compiletest directly with RUST_FLAGS

pnkfelix (Sep 12 2018 at 13:53, on Zulip):

that should (I hope) avoid the rebuild issue

pnkfelix (Sep 12 2018 at 13:54, on Zulip):

though if its already rebuilding, this advice is coming too late?

lqd (Sep 12 2018 at 13:54, on Zulip):

yeah I'll try that as soon as it's done rebuilding :)

lqd (Sep 12 2018 at 14:35, on Zulip):

@pnkfelix I added nll flags wherever I could, the env var, in compiletest's host-rustcflags and target-rustcflags: results

pnkfelix (Sep 12 2018 at 14:37, on Zulip):

okay that's good. And you already shared the ui/ results (because it looks like your build host is one where the PRs to move a lot of run-pass to ui/ have landed)

pnkfelix (Sep 12 2018 at 14:37, on Zulip):

yes?

pnkfelix (Sep 12 2018 at 14:37, on Zulip):

/me goes to look at the diff (of ui results)

lqd (Sep 12 2018 at 14:38, on Zulip):

yes, it's this morning's master

pnkfelix (Sep 12 2018 at 14:39, on Zulip):

ugh these diagnostic changes look like they will hurt us

pnkfelix (Sep 12 2018 at 14:39, on Zulip):

we should figure out a way to ignore this fake read in terms of how we report the diagnostics

lqd (Sep 12 2018 at 14:39, on Zulip):

yeah :)

pnkfelix (Sep 12 2018 at 14:41, on Zulip):

but still, its a pretty small number of diagnostic regressions

pnkfelix (Sep 12 2018 at 14:42, on Zulip):

my intuition is that we should consider landing this

lqd (Sep 12 2018 at 14:42, on Zulip):

did this come up before ? either during the recent "making plans" (I don't think it did) or when you discussed this fix with @Matthew Jasper a couple weeks back IIRC ?

pnkfelix (Sep 12 2018 at 14:42, on Zulip):

@nikomatsakis ^

pnkfelix (Sep 12 2018 at 14:43, on Zulip):

did this come up before ? either during the recent "making plans" (I don't think it did) or when you discussed this fix with @Matthew Jasper a couple weeks back IIRC ?

"this" being the changes to diagnostics that result from ReadForMatch? or are you referring to something else?

lqd (Sep 12 2018 at 14:43, on Zulip):

yeah, it looks like only this diagnostics suffers, that's good, the fallout is small and can be fixed :)

lqd (Sep 12 2018 at 14:43, on Zulip):

yes, "this" being this diagnostics regression

lqd (Sep 12 2018 at 14:44, on Zulip):

I don't remember it coming up when we talked about "what could possibly go wrong if we add a fake read" then

lqd (Sep 12 2018 at 14:44, on Zulip):

(esp, how we'd fix it, if it did come up)

pnkfelix (Sep 12 2018 at 14:48, on Zulip):

its possible I'm wrong and this diagnostic will be an "improvement"

pnkfelix (Sep 12 2018 at 14:48, on Zulip):

or at least bring us closer to what AST-borrowck reported in such cases

lqd (Sep 12 2018 at 15:52, on Zulip):

its possible I'm wrong and this diagnostic will be an "improvement"

surely not this modified diagnostic ?

davidtwco (Sep 12 2018 at 15:53, on Zulip):

It could be if the original diagnostic was misleading.

davidtwco (Sep 12 2018 at 15:53, on Zulip):

(because we weren't doing a fake read earlier)

lqd (Sep 12 2018 at 15:55, on Zulip):

yeah but this diagnostic only shows up when there is a real read, and shows where that read happens (which ofc in this case is changed by the fake read)

lqd (Sep 12 2018 at 15:56, on Zulip):

but yeah at least it's an interesting thought

lqd (Sep 12 2018 at 15:56, on Zulip):

and we can probably have both the compile error in this case, and the correct diagnostic in all other cases

nikomatsakis (Sep 12 2018 at 16:14, on Zulip):

interesting

nikomatsakis (Sep 12 2018 at 16:14, on Zulip):

I think fixing the diagnostics shouldn't be too hard

lqd (Sep 12 2018 at 17:10, on Zulip):

@nikomatsakis would I need add the fake ReadForMatch around here as well ? (this is new from your ascription support PR)

nikomatsakis (Sep 12 2018 at 17:11, on Zulip):

yep

lqd (Sep 12 2018 at 17:11, on Zulip):

will do

lqd (Sep 13 2018 at 13:09, on Zulip):

@pnkfelix if we wanted to keep these fake reads, how do you think we should fix the diagnostic regression they produce ?

pnkfelix (Sep 13 2018 at 13:11, on Zulip):

I'm not sure we should worry about "fixing" it...

pnkfelix (Sep 13 2018 at 13:11, on Zulip):

the scenario where this arises is pretty limited

pnkfelix (Sep 13 2018 at 13:12, on Zulip):

and in some ways the new diagnostic just points out the core of the problem: if you want to store something in a local, it should not have dangling references to temporaries from the stack that was formed during evaluation of its initializer expression.

lqd (Sep 13 2018 at 13:12, on Zulip):

do you think we should then bless the 6-7 tests diffs ?

pnkfelix (Sep 13 2018 at 13:12, on Zulip):

yeah I'm now on the side of blessing them, I think

lqd (Sep 13 2018 at 13:12, on Zulip):

interesting

pnkfelix (Sep 13 2018 at 13:13, on Zulip):

might be good to double check with other members of @WG-compiler-nll

lqd (Sep 13 2018 at 13:13, on Zulip):

agreed

lqd (Sep 13 2018 at 13:15, on Zulip):

for convenience, the diagnostic differences are here

nikomatsakis (Sep 13 2018 at 14:01, on Zulip):

hmm, @lqd, I'd probably be ok with blessing them, but I think it may be fairly easy to improve them

nikomatsakis (Sep 13 2018 at 14:01, on Zulip):

(where is the PR?)

nikomatsakis (Sep 13 2018 at 14:02, on Zulip):

oh, there isn't one?

lqd (Sep 13 2018 at 14:02, on Zulip):

not yet, as I thought we'd fix the regressions first ?

lqd (Sep 13 2018 at 14:02, on Zulip):

the diagnostics regressions that is

lqd (Sep 13 2018 at 14:04, on Zulip):

but if everyone wants to bless these diffs I can make a PR soon

nikomatsakis (Sep 13 2018 at 14:20, on Zulip):

I always prefer to have a PR

nikomatsakis (Sep 13 2018 at 14:20, on Zulip):

feel free to mark it as [WIP]

nikomatsakis (Sep 13 2018 at 14:20, on Zulip):

but it makes the workflow easier

nikomatsakis (Sep 13 2018 at 14:20, on Zulip):

feel free to mark it as [WIP]

(in that case, bors won't land it)

nikomatsakis (Sep 13 2018 at 14:20, on Zulip):

by this I mean: even if we don't bless the regressions, I'd prefer to have a PR

nikomatsakis (Sep 13 2018 at 14:20, on Zulip):

so I can see the diffs, make comments, etc

nikomatsakis (Sep 13 2018 at 14:21, on Zulip):

actually my preferred workflow is this:

Always run the tests with --bless, so we can easily see the diffs

nikomatsakis (Sep 13 2018 at 14:21, on Zulip):

then we can separately decide whether to land it in that state or not :)

lqd (Sep 13 2018 at 14:24, on Zulip):

:) ok I'll push the type ascription commit + the blessed tests to my branch and make a PR then

nikomatsakis (Sep 13 2018 at 14:41, on Zulip):

cool :)

lqd (Sep 13 2018 at 14:49, on Zulip):

@davidtwco we mentioned the mir-opt suite yesterday, do you know how to update them ? --bless doesn't seem to do so

davidtwco (Sep 13 2018 at 14:51, on Zulip):

I think I've had to do it once. I think I just wrote an extra line until it passed.

lqd (Sep 13 2018 at 14:53, on Zulip):

oh

lqd (Sep 13 2018 at 14:53, on Zulip):

alright thank you

lqd (Sep 13 2018 at 14:55, on Zulip):

since they all fail now I was wondering if there was an expected way to fix those, or at least not deal with them until we have to, when the PR is finished

lqd (Sep 13 2018 at 14:56, on Zulip):

I guess it'll just fail on Travis, but at least I'd have wanted to see if these were the only ones failing now, I don't know how close to the end of the tests these are run by x.py test

nikomatsakis (Sep 13 2018 at 15:02, on Zulip):

since they all fail now I was wondering if there was an expected way to fix those, or at least not deal with them until we have to, when the PR is finished

you have to edit them by hand to make the "expected" match the actual

nikomatsakis (Sep 13 2018 at 15:02, on Zulip):

plausibly we should remove them

nikomatsakis (Sep 13 2018 at 15:03, on Zulip):

they are annoying

nikomatsakis (Sep 13 2018 at 15:03, on Zulip):

and/or port them to ui tests

nikomatsakis (Sep 13 2018 at 15:03, on Zulip):

they were good when we were just bootstrapping but we have better a test suite now

nikomatsakis (Sep 13 2018 at 15:03, on Zulip):

(I mean the NLL ones specifically)

lqd (Sep 13 2018 at 15:23, on Zulip):

opened https://github.com/rust-lang/rust/pull/54188 for discussion, so it's very WIP, but here goes nothing

nikomatsakis (Sep 14 2018 at 15:07, on Zulip):

@lqd I left a comment. I think that if we want to identify this case and give a custom error, it would be relatively easy to do so.

nikomatsakis (Sep 14 2018 at 15:07, on Zulip):

I think the current error is not that bad

nikomatsakis (Sep 14 2018 at 15:07, on Zulip):

so maybe it's fine as is

lqd (Sep 14 2018 at 15:08, on Zulip):

@nikomatsakis what would ideally want ?

nikomatsakis (Sep 14 2018 at 15:08, on Zulip):

I left a suggestion in the issue but that's I think a key question :)

lqd (Sep 14 2018 at 15:08, on Zulip):

I can do what is suggested in the issue

nikomatsakis (Sep 14 2018 at 15:08, on Zulip):

actually

nikomatsakis (Sep 14 2018 at 15:08, on Zulip):

maybe just this?

nikomatsakis (Sep 14 2018 at 15:08, on Zulip):
error[E0597]: `pointer` does not live long enough
  --> $DIR/wf-method-late-bound-regions.rs:30:18
   |
LL |     let dangling = {
   |         -------- borrow later stored here
LL |         let pointer = Box::new(42);
LL |         f2.xmute(&pointer)
   |                  ^^^^^^^^ borrowed value does not live long enough
LL |     };
   | - `pointer` dropped here while still borrowed
nikomatsakis (Sep 14 2018 at 15:09, on Zulip):

that is, "borrow later stored here" and not "borrow later used here"?

lqd (Sep 14 2018 at 15:09, on Zulip):

oh interesting

pnkfelix (Sep 14 2018 at 15:10, on Zulip):

next PL I invent, the destination of an imperative update is going to be on the RHS.

pnkfelix (Sep 14 2018 at 15:10, on Zulip):

(and its going to use =: as the operator!)

lqd (Sep 14 2018 at 15:11, on Zulip):

with the FakeReadCause the message in the 0597 error can be adjusted easily I think ?

lqd (Sep 14 2018 at 15:12, on Zulip):

sounds straightforward

nikomatsakis (Sep 14 2018 at 15:13, on Zulip):

seems fairly easy

lqd (Sep 14 2018 at 15:13, on Zulip):

yeah, ok I'll do that

lqd (Sep 14 2018 at 15:14, on Zulip):

I was wondering what kind of comment would be need at the point where we inject the fake read ?

lqd (Sep 14 2018 at 15:14, on Zulip):

what I have was tailored to the original experiment: "let's see what this breaks"

lqd (Sep 14 2018 at 15:15, on Zulip):

that is starting here

nikomatsakis (Sep 14 2018 at 15:20, on Zulip):

yeah, I think something like

nikomatsakis (Sep 14 2018 at 15:22, on Zulip):

Officially, the semantics of

let pattern = <expr>;

is that <expr> is evaluated into a temporary and then this temporary is matched into the pattern.
However, if we see the simple pattern let var = <expr>, we optimize this to evaluate <expr> directly
into the variable var. This is mostly unobservable, but in some cases it can affect the borrow checker,
as in #53695. Therefore, we insert a "fake read" here to ensure that we get appropriate errors.

nikomatsakis (Sep 14 2018 at 15:22, on Zulip):

we could put more detail, but perhaps that suffices

lqd (Sep 14 2018 at 15:25, on Zulip):

nice

lqd (Sep 14 2018 at 15:26, on Zulip):

thanks niko

lqd (Sep 14 2018 at 16:56, on Zulip):

@nikomatsakis in refactoring ReadForMatch into FakeRead do I also need to rename the -Z nll_dont_emit_read_for_match flag ?

lqd (Sep 14 2018 at 17:04, on Zulip):

(I assume this user-facing flag can stay the same, only needing renaming its documentation to update the kind of statement to its new name)

nikomatsakis (Sep 14 2018 at 17:59, on Zulip):

@lqd I would not change the -Z flag

lqd (Sep 14 2018 at 19:11, on Zulip):

haha Matthew's PR just landed as I was finishing this

nikomatsakis (Sep 14 2018 at 19:23, on Zulip):

which?

lqd (Sep 14 2018 at 19:24, on Zulip):

#54088

lqd (Sep 14 2018 at 19:24, on Zulip):

just a tiny conflict no biggie :)

lqd (Sep 14 2018 at 19:24, on Zulip):

I'll rebase and push shortly

nikomatsakis (Sep 14 2018 at 19:31, on Zulip):

@lqd I left a few nits on the PR

nikomatsakis (Sep 14 2018 at 19:31, on Zulip):

just about where to put the comments

lqd (Sep 14 2018 at 19:31, on Zulip):

@nikomatsakis I've updated #54188 to add the "borrow later stored here" message

nikomatsakis (Sep 14 2018 at 19:31, on Zulip):

looks great! :100:

lqd (Sep 14 2018 at 19:31, on Zulip):

dang you're fast

lqd (Sep 14 2018 at 19:32, on Zulip):

thanks for the comments, they're spot on

lqd (Sep 14 2018 at 19:35, on Zulip):

there is a decent comment at the fake read match insertion point, do you want me to move it to FakeReadCause::ForMatch ?

nikomatsakis (Sep 14 2018 at 19:37, on Zulip):

I would move it in the same way

lqd (Sep 14 2018 at 19:43, on Zulip):

(tests will still fail as I still haven't updated the mir-opt suite yet, which I will try to do tomorrow as I'm 2h late for dinner ;)

lqd (Sep 14 2018 at 19:50, on Zulip):

(and probably why it should still stay as a WIP PR ?)

lqd (Sep 14 2018 at 20:29, on Zulip):

(comments moved, helper function hopefully acceptable -- I'll update the tests tomorrow though I really gotta go)

lqd (Sep 14 2018 at 23:16, on Zulip):

I'm unsure about this change: without checking the fake read cause, the fake reads for lets would show up in this min_const_fn check for a match (that matches were detected in const fn because of their fake reads was surprising but ok). Maybe these tests are run after mir-optso we'll see how they go

nikomatsakis (Sep 15 2018 at 00:32, on Zulip):

hmm, that might want to just check for FakeReadCause::ForMatch...but all in all that feels a bit hokey in any case

nikomatsakis (Sep 15 2018 at 00:32, on Zulip):

it's ok though min_const_fn is a bit of a hack

nikomatsakis (Sep 15 2018 at 00:32, on Zulip):

(temporary hack)

nikomatsakis (Sep 15 2018 at 00:32, on Zulip):

I am not sure whether let x was meant to be included (cc @Oli?)

lqd (Sep 15 2018 at 01:57, on Zulip):

I think min_const_fn was so minimal that let x is not included. Either way, it would make more sense to look for FakeReadCause::ForMatch so I'll do this in the mornng

oli (Sep 15 2018 at 05:42, on Zulip):

Yes, let bindigs are forbidden. But even if you remove the fake read, you should still get a min const fn error for the switch terminator

lqd (Sep 15 2018 at 07:24, on Zulip):

hmm the CI seems to have passed so maybe everything works

lqd (Sep 15 2018 at 07:36, on Zulip):

(most likely it doesn't run all the the tests that bors does, as I think ui-fulldeps fails rn — maybe not, I'm confused. It seems to have been run in Travis, while some fail locally, for reasons of help messages disappearing when proc macros panic :confused: )

nikomatsakis (Sep 15 2018 at 10:13, on Zulip):

@lqd it seems you are correct, but I would guess that this must be being enforced in some other way.

lqd (Sep 15 2018 at 10:13, on Zulip):

yeah through mir assignments IIRC

lqd (Sep 15 2018 at 10:14, on Zulip):

@nikomatsakis in any case I'll defer to your judgment on how to proceed :) (in the meantime I'll take care of the crater run results)

nikomatsakis (Sep 15 2018 at 10:17, on Zulip):

@lqd in that case it seems fine to leave it the way you had it.

nikomatsakis (Sep 15 2018 at 10:17, on Zulip):

I .. presume we have tests of all this stuff

nikomatsakis (Sep 15 2018 at 10:18, on Zulip):

can't remember

nikomatsakis (Sep 15 2018 at 10:18, on Zulip):

but dear me I would hope so

nikomatsakis (Sep 15 2018 at 10:18, on Zulip):

by "this stuff" I mean the things that are excluded by min_const_fn

lqd (Sep 15 2018 at 10:18, on Zulip):

I remember oli and centril going back and forth on tests about this, for a while

nikomatsakis (Sep 15 2018 at 10:18, on Zulip):

in that case, it seems like if we want to grow, we can update the tests, and we'll find this case readily enough

nikomatsakis (Sep 15 2018 at 10:18, on Zulip):

yes, I remember that

lqd (Sep 15 2018 at 10:18, on Zulip):

and they wanted to be very conservative, and strongly validate it, so I presume we do as well

lqd (Sep 17 2018 at 15:37, on Zulip):

@nikomatsakis is there anything more I can do for #54188 ?

nikomatsakis (Sep 17 2018 at 15:38, on Zulip):

r+

nikomatsakis (Sep 17 2018 at 15:38, on Zulip):

looks good to me!

lqd (Sep 17 2018 at 15:39, on Zulip):

yay :)

lqd (Sep 18 2018 at 15:12, on Zulip):

@nikomatsakis btw I've rebased #54188

lqd (Sep 18 2018 at 15:34, on Zulip):

@davidtwco thanks :)

lqd (Sep 20 2018 at 08:17, on Zulip):

hum, I'm not sure I understand why this linker error pops up in #54188 ? should I look into it in more detail @nikomatsakis ?

pnkfelix (Sep 20 2018 at 09:39, on Zulip):

It might be spurious; we've been having some linker issues connected to codegen-units and/or incremental compilation

pnkfelix (Sep 20 2018 at 09:40, on Zulip):

Also, just in case you didn't notice, this has the // run-rustfix attribute; so the thing being compiled when linker fails is the extern-const.fixed

pnkfelix (Sep 20 2018 at 09:42, on Zulip):

(which has replaced the const C: u8 with a static C: u8;.) I don't see how that could be related to your PR, unless somehow you've affected codegen for statics as well as let-bound variables...?

lqd (Sep 20 2018 at 09:55, on Zulip):

I'm confused about why the codegen would only be affected after running rustfix, what does it do to statics that regular compilation and linking don't, and are thus unaffected ?

lqd (Sep 20 2018 at 09:56, on Zulip):

but it may very well be such a case

lqd (Sep 20 2018 at 10:00, on Zulip):

locally I did just have an error for the ui/duplicate/dupe-symbols-7.rs case (ui tests in compare mode) and passes successfully if I run tests again

lqd (Sep 20 2018 at 10:01, on Zulip):

I'll try to reproduce these rustfix cases at lunch break

lqd (Sep 20 2018 at 10:09, on Zulip):

(to be clearer, I would be less confused if we had zero other tests similar static C: u8 which would also fail, before getting to the rustfixed tests :)

pnkfelix (Sep 20 2018 at 10:23, on Zulip):

part of the problem is that when we get failures connected to codegen-units, they are often connected to how the code was partitioned into those units, which can mean that similar looking code can have wildly different probabilty of causing a link-time failure...

lqd (Sep 20 2018 at 10:26, on Zulip):

yeah :/ anyway I'm just rerunning everything locally again, and then I'll focus on these specific tests

pnkfelix (Sep 20 2018 at 10:26, on Zulip):

Also, I somehow was unaware of codegen-units being set to N (where N > 1) by default in all modes. Which happened in PR #46910 (landed December 25 2017...)

pnkfelix (Sep 20 2018 at 10:27, on Zulip):

It was probably a good thing in terms of finding codegen-units related bugs, compared to our prior mode of only turning codegen-units on for non-optimizing mode, while our test suite was running with optimization on by default...

pnkfelix (Sep 20 2018 at 10:28, on Zulip):

But the compiler team needs to make sure to be proactive about identifying such bugs and then fixing them...

pnkfelix (Sep 20 2018 at 10:29, on Zulip):

I'm going to cc @mw on your PR in the meantime, as they might have insight that I lack about how to address this

lqd (Sep 20 2018 at 10:29, on Zulip):

mixing this with "interesting" cases of incremental, multithreading llvm codegen, the various LTOs configurations, etc :)

pnkfelix (Sep 20 2018 at 10:30, on Zulip):

in fact, I will also nominate the PR since it would be nice to discuss this during the compiler team meeting later today

lqd (Sep 20 2018 at 10:30, on Zulip):

didn't you also discuss this in the lang meeting last week ?

lqd (Sep 20 2018 at 10:31, on Zulip):

it would be good in any case, since it's a bit low-key but has possibly impact over lots of things

pnkfelix (Sep 20 2018 at 10:32, on Zulip):

We didn't discuss codegen-unit linker issues in lang meeting

lqd (Sep 20 2018 at 10:32, on Zulip):

oh understood

pnkfelix (Sep 20 2018 at 10:32, on Zulip):

That is, the lang meeting was about the change to the language semantics, but I just want to talk with compiler team about the test failure you encoutered.

lqd (Sep 20 2018 at 10:33, on Zulip):

hopefully I'll discover more about whether this is spurious or reproducible today before the meeting

lqd (Sep 20 2018 at 10:47, on Zulip):

@pnkfelix do you know how to run the rustfix test suite ? it is not x.py test src/test/rustfix apparently

pnkfelix (Sep 20 2018 at 10:48, on Zulip):

I think you just run the ui tests

pnkfelix (Sep 20 2018 at 10:48, on Zulip):

the // run-rustfix attribute is what is causing that rustfix to be run

pnkfelix (Sep 20 2018 at 10:48, on Zulip):

but you may also be able to just cut-and-paste the rustc invocation that the log includes

pnkfelix (Sep 20 2018 at 10:49, on Zulip):

because the output from rustfix is actually checked into the repository

pnkfelix (Sep 20 2018 at 10:49, on Zulip):

(part of // run-rustfix is comparing the output that rustfix to the checked in file)

pnkfelix (Sep 20 2018 at 10:50, on Zulip):

So you may be able to just run this:

"/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/ui/extern/extern-const.fixed" "--target=x86_64-unknown-linux-gnu" "--error-format" "json" "-Zui-testing" "-C" "prefer-dynamic" "-o" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/extern/extern-const/a" "-Crpath" "-Zunstable-options" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-Z" "continue-parse-after-error" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/extern/extern-const/auxiliary"

(adapted accordingly to your local machine)

lqd (Sep 20 2018 at 10:50, on Zulip):

it's run as part of the existing ui tests ?

pnkfelix (Sep 20 2018 at 10:50, on Zulip):

yes i think so

lqd (Sep 20 2018 at 10:50, on Zulip):

if so, then no it's not reproducible over here :/

pnkfelix (Sep 20 2018 at 10:50, on Zulip):

Yeah I'm not totally surprised to hear that

pnkfelix (Sep 20 2018 at 10:51, on Zulip):

I don't know how deterministic our codegen-units partitioning is

lqd (Sep 20 2018 at 10:51, on Zulip):

apart from the spurious error I mentioned earlier, which went away when rerunning tests, they seem to pass

pnkfelix (Sep 20 2018 at 10:51, on Zulip):

you could try manually pumping up the codegen units for that one test

pnkfelix (Sep 20 2018 at 10:52, on Zulip):

(this is why I want to discuss this among the compiler team; to get feedback about whether pumping up the #codegen-units is a decent strategy to increase likelihood of such problems...)

lqd (Sep 20 2018 at 10:52, on Zulip):

agreed, this seems worthwhile to discuss :)

pnkfelix (Sep 20 2018 at 10:52, on Zulip):

I've seen github comments with @mw advising others to pump up to 99 (or even 999?) codegen-units to try to replicate bugs...

pnkfelix (Sep 20 2018 at 10:53, on Zulip):

The other Q I have is whether a retry might just work

pnkfelix (Sep 20 2018 at 10:53, on Zulip):

or if the codegen-unit partitioning is sufficiently deterministic that we do not expect retry's to fix the problem.

lqd (Sep 20 2018 at 10:54, on Zulip):

I guess it'll be another interesting question for the meeting

lqd (Sep 20 2018 at 10:55, on Zulip):

even though y'all might have talked about it in the "spurious ui tests failures" topic in #t-compiler

pnkfelix (Sep 20 2018 at 10:55, on Zulip):

Yeah I'm taking notes on them as I think of them (in the github issue)

pnkfelix (Sep 20 2018 at 10:55, on Zulip):

well I feel like that topic is perhaps broader

pnkfelix (Sep 20 2018 at 10:55, on Zulip):

(but I could be wrong)

lqd (Sep 20 2018 at 10:57, on Zulip):

running the command manually does fail reliably!

pnkfelix (Sep 20 2018 at 10:58, on Zulip):

oh you've reproduced locally then?

pnkfelix (Sep 20 2018 at 10:58, on Zulip):

that is a good sign

lqd (Sep 20 2018 at 10:59, on Zulip):

it might be using already built things though

lqd (Sep 20 2018 at 10:59, on Zulip):

but yeah, maybe a good sign

pnkfelix (Sep 20 2018 at 10:59, on Zulip):

Yes; I had thought from what you said above that you were not seeing the linker failure locally under any scenario

lqd (Sep 20 2018 at 11:00, on Zulip):

I do not see the error running the ui tests

lqd (Sep 20 2018 at 11:00, on Zulip):

as you said, I tried adapting the command failing on CI to my build, and this fails

pnkfelix (Sep 20 2018 at 11:00, on Zulip):

This is how compiletest is being invoked:

"/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/compiletest" "--compile-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib" "--run-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/x86_64-unknown-linux-gnu/lib" "--rustc-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "--src-base" "/checkout/src/test/ui" "--build-base" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui" "--stage-id" "stage2-x86_64-unknown-linux-gnu" "--mode" "ui" "--target" "x86_64-unknown-linux-gnu" "--host" "x86_64-unknown-linux-gnu" "--llvm-filecheck" "/checkout/obj/build/x86_64-unknown-linux-gnu/llvm/build/bin/FileCheck" "--host-rustcflags" "-Crpath -Zunstable-options " "--target-rustcflags" "-Crpath -Zunstable-options  -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--docck-python" "/usr/bin/python2.7" "--lldb-python" "/usr/bin/python2.7" "--gdb" "/usr/bin/gdb" "--llvm-version" "8.0.0svn\n" "--cc" "" "--cxx" "" "--cflags" "" "--llvm-components" "" "--llvm-cxxflags" "" "--adb-path" "adb" "--adb-test-dir" "/data/tmp/work" "--android-cross-path" "" "--color" "always"

so you could try adapting that locally

pnkfelix (Sep 20 2018 at 11:01, on Zulip):

(you'd want to make sure you delete the stamp file, if any, for the test that is failing first)

pnkfelix (Sep 20 2018 at 11:02, on Zulip):

it looks like the x.py command being run is x.py check (which ... I am not familiar with; I only use x.py build and x.py test ...)

pnkfelix (Sep 20 2018 at 11:04, on Zulip):

And that is confusing to me, because I would have thought that x.py check's running of cargo check would not link code ... but maybe I misunderstand the scope of cargo check

pnkfelix (Sep 20 2018 at 11:05, on Zulip):

oh no I'm totally wrong

lqd (Sep 20 2018 at 11:05, on Zulip):

@pnkfelix you and me rn https://i.imgur.com/UaLpJOK.png

pnkfelix (Sep 20 2018 at 11:05, on Zulip):

it is using x.py test; its the Makefile target that is named check

lqd (Sep 20 2018 at 11:19, on Zulip):

@pnkfelix I remove the built tests, run ./x.py test for that test, it passes, run the command which failed on CI, it fails, log

pnkfelix (Sep 20 2018 at 11:20, on Zulip):

@lqd okay. Out of curiosity, did you try running the compiletest invocation I posted?

pnkfelix (Sep 20 2018 at 11:20, on Zulip):

(admittedly it would probably be a pain to adapt to your local system)

pnkfelix (Sep 20 2018 at 11:21, on Zulip):

anyway, the fact that you can replicate the failure via the command should serve well enough to track this down

lqd (Sep 20 2018 at 11:21, on Zulip):

is that what is output at the end of a x.py test failure ?

pnkfelix (Sep 20 2018 at 11:21, on Zulip):

Yes it is part of that output I believe

pnkfelix (Sep 20 2018 at 11:21, on Zulip):

I was just cut-and-pasting it out of the raw Travis log

lqd (Sep 20 2018 at 11:21, on Zulip):

I did, but I'll retry if you want; do you want me to try after removing the built tests or not ?

pnkfelix (Sep 20 2018 at 11:21, on Zulip):

You will need to remove the stamp files

pnkfelix (Sep 20 2018 at 11:22, on Zulip):

I don't know if you need to remove the built object code

pnkfelix (Sep 20 2018 at 11:22, on Zulip):

Sorry

pnkfelix (Sep 20 2018 at 11:22, on Zulip):

And its not a big deal

pnkfelix (Sep 20 2018 at 11:22, on Zulip):

Oh sorry, if you already tried compiletest then maybe don't bother

pnkfelix (Sep 20 2018 at 11:22, on Zulip):

I was just curious

lqd (Sep 20 2018 at 11:22, on Zulip):

it looked like what ./x.py test does naturally

pnkfelix (Sep 20 2018 at 11:22, on Zulip):

sure

pnkfelix (Sep 20 2018 at 11:23, on Zulip):

just trying to gather data

lqd (Sep 20 2018 at 11:23, on Zulip):

which ignored this test, as it didn't fail

lqd (Sep 20 2018 at 11:23, on Zulip):

sure

pnkfelix (Sep 20 2018 at 11:23, on Zulip):

since you do see the problem when you run the command ...

pnkfelix (Sep 20 2018 at 11:23, on Zulip):

wait, you say it ignored this test

lqd (Sep 20 2018 at 11:23, on Zulip):

yeah it's mind boggling

pnkfelix (Sep 20 2018 at 11:23, on Zulip):

as in, it wasn't tested at all? That makes it sound like you do have stamp files lying around?

pnkfelix (Sep 20 2018 at 11:24, on Zulip):

You could try running touch on the input .rs file for this test

pnkfelix (Sep 20 2018 at 11:24, on Zulip):

if you don't want to bother with trying to track down the stamp files

lqd (Sep 20 2018 at 11:25, on Zulip):

I'm nuking the whole tests to try the compiletest command again

lqd (Sep 20 2018 at 11:25, on Zulip):

it'll take a while, and I won't know if this specific test was ignored or not

lqd (Sep 20 2018 at 11:26, on Zulip):

but I do consistently get similar linker errors, but not on that test, and rerunning makes these errors go away

pnkfelix (Sep 20 2018 at 11:26, on Zulip):

yeah, that's a problem with the current output

pnkfelix (Sep 20 2018 at 11:26, on Zulip):

we used to print out every test name as we ran

pnkfelix (Sep 20 2018 at 11:26, on Zulip):

obviously the newer output is nicer for the common case when you don't care about the test names as you run

lqd (Sep 20 2018 at 11:26, on Zulip):

CI does that so maybe we can still have this info ?

pnkfelix (Sep 20 2018 at 11:26, on Zulip):

but I would like to know if there's a way to get back the prior behavior...

pnkfelix (Sep 20 2018 at 11:26, on Zulip):

hmm good point

pnkfelix (Sep 20 2018 at 11:29, on Zulip):

Bet you its this [00:01:44] configure: rust.verbose-tests := True

lqd (Sep 20 2018 at 11:30, on Zulip):

oh

lqd (Sep 20 2018 at 11:31, on Zulip):

I'll try to flip this switch in config.toml

lqd (Sep 20 2018 at 11:36, on Zulip):

(flipping the switch works, rerunning the ui tests and then the compiletest command directly)

lqd (Sep 20 2018 at 12:00, on Zulip):

@pnkfelix update: 1) rerunning x.py test to build the tests correctly with the verbose-tests option and to get the compiletest command in case of failure: I made sure the test failing the PR ran (test [ui] ui/extern/extern-const.rs ... ok) 2) I also later had an ICE 3) the ICE disappeared when rerunning the compiletest command, test [ui] ui/run-pass/rfcs/rfc-2126-extern-absolute-paths/test.rs ... ok 4) then I removed the tests and reran the compiletest command and the ui suite passed (including test [ui] ui/extern/extern-const.rs ... ok)

lqd (Sep 20 2018 at 12:02, on Zulip):

6) I reran our previous command failing CI, which I can only hope would be valid after compiling and running the ui tests (that is, not missing various .a files or whatever it needs), and it failed with the undefined reference to C

lqd (Sep 20 2018 at 12:04, on Zulip):

I don't know what is going anymore :)

lqd (Sep 20 2018 at 12:07, on Zulip):

the command uses -L ./build/x86_64-unknown-linux-gnu/test/ui/extern/extern-const/auxiliary which doesn't exist in my build

lqd (Sep 20 2018 at 12:08, on Zulip):

maybe it's at a different place here, I'll try and look for that

pnkfelix (Sep 20 2018 at 12:15, on Zulip):

Well don't look too hard

pnkfelix (Sep 20 2018 at 12:15, on Zulip):

I think compiletest unconditionally adds that -L options

pnkfelix (Sep 20 2018 at 12:15, on Zulip):

to increase the linker's search paths

pnkfelix (Sep 20 2018 at 12:16, on Zulip):

but my understanding is that -L <dir> has no effect if <dir> does not exist...

lqd (Sep 20 2018 at 12:17, on Zulip):

yeah I feel like this test might not need the auxiliary folder some other tests need

lqd (Sep 20 2018 at 12:18, on Zulip):

also the CI has an './obj' dir that we don't, locally it seems everything is in './build'

lqd (Sep 20 2018 at 12:20, on Zulip):

or maybe obj has noting to do with .o and .a and it's just the rust checkout

pnkfelix (Sep 20 2018 at 12:20, on Zulip):

my guess is that they go into /checkout/obj before they run any x.py command

pnkfelix (Sep 20 2018 at 12:20, on Zulip):

or at least, that matches my own work flow

lqd (Sep 20 2018 at 12:21, on Zulip):

oh ok, not my own but maybe I should lol

pnkfelix (Sep 20 2018 at 12:21, on Zulip):

(i.e., I don't run x.py at the root directory of the rust.git checkout. I keep it isolated.)

pnkfelix (Sep 20 2018 at 12:21, on Zulip):

but I'm not the normal case here; I think most people do it like you describe

pnkfelix (Sep 20 2018 at 12:21, on Zulip):

(I believe that's why they "we" started generating output into a build subdirectory rather than the current working directory)

lqd (Sep 20 2018 at 12:21, on Zulip):

yeah it seems you're right, they have checkout/src and checkout/obj/build

lqd (Sep 20 2018 at 12:24, on Zulip):

the maybe I didn't adapt the command to my local build correctly, I just changed every /obj/build into ./build

pnkfelix (Sep 20 2018 at 12:24, on Zulip):

that sounds like it should work, as long as you did it across the board

pnkfelix (Sep 20 2018 at 12:25, on Zulip):

So based on the conversation so far, I take it we have two questions:

pnkfelix (Sep 20 2018 at 12:25, on Zulip):

1. Why is the rustc invocation on the .checked file causing a linker failure. This is the thing you were able to replicate locally

pnkfelix (Sep 20 2018 at 12:26, on Zulip):

2. Why aren't you seeing the linker failure when you run x.py test locally, even though you were able to replicate 1. I.e. how can it be that your test run isn't hitting the same steps that Travis does?

pnkfelix (Sep 20 2018 at 12:26, on Zulip):

Okay. And right now you are focusing on question 2

pnkfelix (Sep 20 2018 at 12:26, on Zulip):

Which is fine as long as you care about the question.

lqd (Sep 20 2018 at 12:27, on Zulip):

(the other random linking and ICE errors unrelated to this test also disappear when rerunning ./x.py test)

pnkfelix (Sep 20 2018 at 12:27, on Zulip):

(It is certainly a good question, since it might be a symptom of something wrong with your testing work flow)

pnkfelix (Sep 20 2018 at 12:27, on Zulip):

Right, we definitely have a number of linking and ICEs that disappear mysteriously on re-run

lqd (Sep 20 2018 at 12:31, on Zulip):

@pnkfelix I'm really confused about this test

lqd (Sep 20 2018 at 12:31, on Zulip):

(I'm focusing on question 1 now)

lqd (Sep 20 2018 at 12:32, on Zulip):

this is an extern static, what is rustc trying to do, building an .o/dylib/etc that doesn't care about the undefined reference ?

lqd (Sep 20 2018 at 12:33, on Zulip):

anything else, building an executable for this main for instance, will fail linking IIUC

pnkfelix (Sep 20 2018 at 12:33, on Zulip):

well, the original code had an extern const... and rustfix updates that to an extern static, because extern const is deprecated...

pnkfelix (Sep 20 2018 at 12:33, on Zulip):

(I infer)

pnkfelix (Sep 20 2018 at 12:34, on Zulip):

I would hope that as part of these, we also supply a file that fills in that static. Unless the linkage for that extern static variable is weak...? But rustfix doesn't annotate it as such

pnkfelix (Sep 20 2018 at 12:34, on Zulip):

Anyway I would track down when that test itself was added

pnkfelix (Sep 20 2018 at 12:34, on Zulip):

the .fixed file, that is

lqd (Sep 20 2018 at 12:34, on Zulip):

ok I'll do that

lqd (Sep 20 2018 at 12:35, on Zulip):

it probably was one of the compile-fail tests which were merged into ui

pnkfelix (Sep 20 2018 at 12:38, on Zulip):

yeah. I'm backtracking from the parent of that PR

pnkfelix (Sep 20 2018 at 12:39, on Zulip):

hmm weird, I don't see an extern/ nor an extern-const.rs yet in the parent tree

lqd (Sep 20 2018 at 12:40, on Zulip):

it's a couple commits before yeah

lqd (Sep 20 2018 at 12:40, on Zulip):

that is, a couple commits up there are extern tests

pnkfelix (Sep 20 2018 at 12:40, on Zulip):

oh wait, maybe git blame is the answer here

lqd (Sep 20 2018 at 12:40, on Zulip):

I can't find the one I'm looking for or the extern folder, so there's some reorg there

pnkfelix (Sep 20 2018 at 12:40, on Zulip):

https://github.com/rust-lang/rust/commit/98a04291e4db92ae86d7e3a20b5763cb926ebfbf

lqd (Sep 20 2018 at 12:42, on Zulip):

yay

pnkfelix (Sep 20 2018 at 12:42, on Zulip):

which is part of #50724

pnkfelix (Sep 20 2018 at 12:42, on Zulip):

but so weird, how did this ever pass rustc being run on the .fixed code? Is it legal to do this without providing a object file that defines the static?

lqd (Sep 20 2018 at 12:44, on Zulip):

for a lib/.a maybe, for an executable I would assume not

lqd (Sep 20 2018 at 12:44, on Zulip):

and if this was part of a compile-fail that would make sense

pnkfelix (Sep 20 2018 at 12:45, on Zulip):

you know what I advise, right now?

lqd (Sep 20 2018 at 12:45, on Zulip):

but I'm not even sure of that anymore (if it was a compile-fail or not) (also rustfix is not supposed to work flawlessly on code with compile errors (I guess those are linking errors rather than compiling/parsing so it still works nicely :)

pnkfelix (Sep 20 2018 at 12:45, on Zulip):

move this test back to compile-fail

pnkfelix (Sep 20 2018 at 12:46, on Zulip):

tag it with the issue that's about reviewing the port of those tests

pnkfelix (Sep 20 2018 at 12:46, on Zulip):

(as well as any other relevant issue numbers, to save people the trouble of digging through the git history the way that we have.)

pnkfelix (Sep 20 2018 at 12:47, on Zulip):

and then we'll retry landing your PR

lqd (Sep 20 2018 at 12:47, on Zulip):

ok I'll try that after work, thanks Felix

pnkfelix (Sep 20 2018 at 12:47, on Zulip):

In other words, this rat hole is fun and all, but lets try to make resolving it orthogonal to landing the work that resolves #53695 :rat: :hole:

lqd (Sep 20 2018 at 12:48, on Zulip):

:)

pnkfelix (Sep 20 2018 at 12:50, on Zulip):

oh wait

pnkfelix (Sep 20 2018 at 12:50, on Zulip):

shoot, it predates the compile-fail to ui migration

pnkfelix (Sep 20 2018 at 12:50, on Zulip):

whatever's happening here, its not an artifact of that

lqd (Sep 20 2018 at 12:52, on Zulip):

are we trying to successfully compile a rustfixed file where the original is bound to fail compiling ?

lqd (Sep 20 2018 at 12:53, on Zulip):

or am I just missing an auxiliary folder lol

lqd (Sep 20 2018 at 12:56, on Zulip):

@pnkfelix do you happen to have a build/x86_64-unknown-linux-gnu/test/ui/extern/extern-const/auxiliary folder in any recent build ?

lqd (Sep 20 2018 at 12:59, on Zulip):

also you probably have more important stuff to do...

pnkfelix (Sep 20 2018 at 13:05, on Zulip):

I don't have that directory

pnkfelix (Sep 20 2018 at 13:05, on Zulip):

and that's on a successful test run

pnkfelix (Sep 20 2018 at 13:05, on Zulip):

what the heck is going on

pnkfelix (Sep 20 2018 at 13:06, on Zulip):

(i'm considering my effort here to be part of prep for the compiler meeting in an hour)

lqd (Sep 20 2018 at 13:07, on Zulip):

so it's indeed probably automatically adding the -L like you said earlier

pnkfelix (Sep 20 2018 at 13:07, on Zulip):

hmm I think I might even have the object code that the successful compile generates

pnkfelix (Sep 20 2018 at 13:13, on Zulip):

well that's interestin... in the playpen the .fixed code fails to compile (link failure), but on my linux machine it succeeds...

lqd (Sep 20 2018 at 13:14, on Zulip):

it will depend if you make it Run or just Build

pnkfelix (Sep 20 2018 at 13:14, on Zulip):

oh so we must default to some sort of dynamic linkage then?

lqd (Sep 20 2018 at 13:14, on Zulip):

I think so yeah

pnkfelix (Sep 20 2018 at 13:14, on Zulip):

or the Build command only bulids to a .rlib, not to a binary? Hmm...

lqd (Sep 20 2018 at 13:16, on Zulip):

dynamic linking for this specific test I think

pnkfelix (Sep 20 2018 at 13:16, on Zulip):

even show assembly doesn't work

lqd (Sep 20 2018 at 13:16, on Zulip):

that is, if you cargo build it in a regular project it will fail to build (fail to link to be precise, with the same error)

pnkfelix (Sep 20 2018 at 13:16, on Zulip):

so rustc on the file will succeed, but cargo build will fail?

lqd (Sep 20 2018 at 13:17, on Zulip):

rustc with the flags that compile-test passes you mean ?

lqd (Sep 20 2018 at 13:17, on Zulip):

it does for both cases I tihnk

lqd (Sep 20 2018 at 13:17, on Zulip):

rustc will work, cargo build won't

lqd (Sep 20 2018 at 13:18, on Zulip):

my goodness

pnkfelix (Sep 20 2018 at 13:20, on Zulip):

Okay, something new:

pnkfelix (Sep 20 2018 at 13:20, on Zulip):

inserting a real use of _x will force the failure

lqd (Sep 20 2018 at 13:20, on Zulip):

@pnkfelix do you know if we can get the command compiletest executes for a specific test ? just like it shows when the test fails, but I'd like to compare the one it runs on the "unfixed code" vs the fixed which fails

pnkfelix (Sep 20 2018 at 13:20, on Zulip):

OHO

pnkfelix (Sep 20 2018 at 13:20, on Zulip):

OH OH OH

pnkfelix (Sep 20 2018 at 13:20, on Zulip):

you're inserting a FakeRead

lqd (Sep 20 2018 at 13:20, on Zulip):

OH

pnkfelix (Sep 20 2018 at 13:20, on Zulip):

I wonder if that actually is affecting codegen for this test

pnkfelix (Sep 20 2018 at 13:20, on Zulip):

okay

pnkfelix (Sep 20 2018 at 13:20, on Zulip):

new plan

pnkfelix (Sep 20 2018 at 13:20, on Zulip):

add // skip-trans to this test

pnkfelix (Sep 20 2018 at 13:21, on Zulip):

or wait

pnkfelix (Sep 20 2018 at 13:21, on Zulip):

I dont' know if that's supported in ui/ on master

pnkfelix (Sep 20 2018 at 13:21, on Zulip):

one of my PR adds support for it

pnkfelix (Sep 20 2018 at 13:21, on Zulip):

darn

pnkfelix (Sep 20 2018 at 13:21, on Zulip):

anyway, yeah, I totally bet that LLVM is optimizing away the use

pnkfelix (Sep 20 2018 at 13:21, on Zulip):

or something

pnkfelix (Sep 20 2018 at 13:22, on Zulip):

and your PR is forcing it to show up

lqd (Sep 20 2018 at 13:22, on Zulip):

these FakeReads don't seem to be removed by a MIR pass before codegen ? I think niko and matthew were having such a conversation about this recently

pnkfelix (Sep 20 2018 at 13:22, on Zulip):

but this test as written seems kind of bogus to me

pnkfelix (Sep 20 2018 at 13:22, on Zulip):

or at least

pnkfelix (Sep 20 2018 at 13:22, on Zulip):

I don't know if we should be guaranteeing that the compiler will actually compile the extern-const.fixed variant.

pnkfelix (Sep 20 2018 at 13:23, on Zulip):

I'll file a new bug regarding that question.

nikomatsakis (Sep 20 2018 at 13:34, on Zulip):

I don't know how deterministic our codegen-units partitioning is

afaik it is quite deterministic

nikomatsakis (Sep 20 2018 at 13:34, on Zulip):

what is the current status here?

pnkfelix (Sep 20 2018 at 13:35, on Zulip):

new data, cargo build --release builds successfully, while cargo build does not

pnkfelix (Sep 20 2018 at 13:36, on Zulip):

filed #54388

pnkfelix (Sep 20 2018 at 13:36, on Zulip):

The current status I think is that this PR is exposing a bug elsewhere

nikomatsakis (Sep 20 2018 at 13:48, on Zulip):

I see

pnkfelix (Sep 20 2018 at 18:58, on Zulip):

@lqd I posted my new plan of attack for your PR as a comment

lqd (Sep 20 2018 at 19:01, on Zulip):

@pnkfelix merci felix :) sounds like an interesting thing to try at the very least, maybe it'll blow up elsewhere in other debug tests

lqd (Sep 20 2018 at 19:05, on Zulip):

@pnkfelix though if LLVM is involved, I was worried it'd mean that FakeReads now, and ReadForMatch before, really do impact codegen, while I was assuming they would not

pnkfelix (Sep 20 2018 at 19:06, on Zulip):

well since you can replicate locally, we could see if the LLVM IR is being changed

pnkfelix (Sep 20 2018 at 19:06, on Zulip):

on this test case

pnkfelix (Sep 20 2018 at 19:06, on Zulip):

but only if you're really really curious...

lqd (Sep 20 2018 at 19:07, on Zulip):

I can check the IR tomorrow, and do your suggestion tonight

lqd (Sep 20 2018 at 19:08, on Zulip):

or maybe should I try the suggestion only after checking the IR ?

lqd (Sep 20 2018 at 19:18, on Zulip):

I assume the "CI before r+" didn't run this test ?

pnkfelix (Sep 20 2018 at 20:31, on Zulip):

I assume the "CI before r+" didn't run this test ?

I don't understand what you're asking here. Or rather, my guess is that it did run the test, but the behavior we are seeing is non-deterministic (at least for builds w/o -g)

pnkfelix (Sep 20 2018 at 20:32, on Zulip):

because I think that codegen-units are affecting LLVM's ability to optimize away the references to the symbol that is later causing the link-time failure.

lqd (Sep 20 2018 at 20:53, on Zulip):

I mean it seemed the test code was deterministic when we tried it manually both with a debug and release builds, but non-deterministic inside x.py and CI

pnkfelix (Sep 20 2018 at 21:01, on Zulip):

well, one thing I've been wondering about that: Do you have your own settings in a config.toml file?

pnkfelix (Sep 20 2018 at 21:01, on Zulip):

or do you not use one?

lqd (Sep 20 2018 at 21:02, on Zulip):

I do use one

pnkfelix (Sep 20 2018 at 21:02, on Zulip):

can you gist it for me?

lqd (Sep 20 2018 at 21:03, on Zulip):

I'm not on the same computer as before, but can reproduce the "fails on debug, succeeds on release"

lqd (Sep 20 2018 at 21:03, on Zulip):

yeah I will gist it

pnkfelix (Sep 20 2018 at 21:03, on Zulip):

(basically I'm musing if we should be changing more of those settings to try to get ourselves in a closer state to what the CI runs before we run x.py ourselves. Because there is some reason it generates that command line that you were able to use to reproduce the problem locally...)

pnkfelix (Sep 20 2018 at 21:04, on Zulip):

the "fails on debug" is almost certainly due to the embedded debug-info in the object file causing the linker to say "HEY! Where's that symbol!?!"

pnkfelix (Sep 20 2018 at 21:04, on Zulip):

(but its also possible that its not optimizing away the read in that scenario)

pnkfelix (Sep 20 2018 at 21:05, on Zulip):

(these are all reasons why it might be interesting to look at the LLVM IR)

lqd (Sep 20 2018 at 21:06, on Zulip):

it's very vanilla IIRC, oh maybe you're thinking of enabling llvm assertions ?

pnkfelix (Sep 20 2018 at 21:06, on Zulip):

no, I know from experience that we are nowhere near LLVM assertion clean, unfortunately.

lqd (Sep 20 2018 at 21:06, on Zulip):

heh

pnkfelix (Sep 20 2018 at 21:06, on Zulip):

you're right, this is very vanilla

pnkfelix (Sep 20 2018 at 21:07, on Zulip):

There's this: debuginfo-lines = true

lqd (Sep 20 2018 at 21:07, on Zulip):

hmm

pnkfelix (Sep 20 2018 at 21:07, on Zulip):

i dunno if that's just affecting the rustc build itself, or if it affects the build products during testing

pnkfelix (Sep 20 2018 at 21:08, on Zulip):

(one would think #debuginfo-tests = true would be the thing that controls the latter...)

pnkfelix (Sep 20 2018 at 21:10, on Zulip):

but yeah, I don't see any smoking guns here

pnkfelix (Sep 20 2018 at 21:10, on Zulip):

one would hope use-jemalloc = false would not affect e.g. codegen-unit partitioning...

pnkfelix (Sep 20 2018 at 21:11, on Zulip):

(Do you have jemalloc turned off so that you can use other tools that need the system allocator? just curious)

lqd (Sep 20 2018 at 21:12, on Zulip):

I do have it turned off so I can profile yeah (I have never tried running valgrind and others without it being turned off but maybe it would work)

pnkfelix (Sep 20 2018 at 21:13, on Zulip):

anyway I'd say you should just kill the // run-rustfix line in that test and then ask someone to re-r+ the PR

pnkfelix (Sep 20 2018 at 21:13, on Zulip):

but if you won't have time to do that tonight, that's fine. Just let me know and that way I'll see about putting up a PR to fix the test.

lqd (Sep 20 2018 at 21:13, on Zulip):

yeah I was testing this, and seemed to work (it also needs the .fixed deleted otherwise it panics)

pnkfelix (Sep 20 2018 at 21:13, on Zulip):

oh of course

lqd (Sep 20 2018 at 21:14, on Zulip):

I was trying to get you the ir but with the command from earlier I can't find it whenI --emit=llvm-ir :/

pnkfelix (Sep 20 2018 at 21:14, on Zulip):

it ends up in the --output-dir in the command I think

pnkfelix (Sep 20 2018 at 21:14, on Zulip):

or did the command use -o ?

lqd (Sep 20 2018 at 21:15, on Zulip):

it did yeah, I should be looking for $test-name.ll right ?

pnkfelix (Sep 20 2018 at 21:15, on Zulip):

if it uses -o then I think you end up overwriting the executable with the LLVM IR.

pnkfelix (Sep 20 2018 at 21:15, on Zulip):

its the best

lqd (Sep 20 2018 at 21:15, on Zulip):

oh ok lol

lqd (Sep 20 2018 at 21:15, on Zulip):

lol it did

lqd (Sep 20 2018 at 21:16, on Zulip):

aptly named ./build/x86_64-apple-darwin/test/ui/extern/extern-const/a, let me gist that to you

lqd (Sep 20 2018 at 21:17, on Zulip):

here it is

lqd (Sep 20 2018 at 21:25, on Zulip):

ok I've updated #54188 to disable rustfix on extern-const.rs as you proposed, let's see how it fares on CI @pnkfelix (but tomorrow, for our TZ :)

pnkfelix (Sep 20 2018 at 21:28, on Zulip):

right, so I think this is where we have the read from the undefined static C: https://gist.github.com/lqd/c2bc1eb891b59c6258fdcfd4961bed7b#file-extern-const-ll-L135

pnkfelix (Sep 20 2018 at 21:29, on Zulip):

and I assume that on a rustc build without your PR, we don't see that %0 = load i8, i8* @C, align 1

lqd (Sep 20 2018 at 21:31, on Zulip):

I might have messed up the thing that removes FakeReads from reaching codegen

pnkfelix (Sep 20 2018 at 21:31, on Zulip):

that's a decent theory. It would be good to check it.

lqd (Sep 20 2018 at 21:31, on Zulip):

or we don't have such a thing and FakeReads were RealReads all along m night shyamalan plot twist

pnkfelix (Sep 20 2018 at 21:32, on Zulip):

oh

pnkfelix (Sep 20 2018 at 21:33, on Zulip):

plot twist indeed

pnkfelix (Sep 20 2018 at 21:33, on Zulip):

--emit llvm-ir on my rustc does have %0 = load i8, i8* @C, align 1

lqd (Sep 20 2018 at 21:33, on Zulip):

we've been plot twisted!

pnkfelix (Sep 20 2018 at 21:33, on Zulip):

(at least, it does at -C opt-level=0 ... looking at other scenarios now...)

lqd (Sep 20 2018 at 21:34, on Zulip):

@pnkfelix thanks for helping out btw, doing this alone would have been way harder (probably impossible)

pnkfelix (Sep 20 2018 at 21:34, on Zulip):

might LTO be getting rid of it, hmm ...

pnkfelix (Sep 20 2018 at 21:37, on Zulip):

It still links even when I disable ThinLTO and am at -C opt-level=0.

lqd (Sep 20 2018 at 21:48, on Zulip):

@pnkfelix do you remember where should I look for something (like a MIR pass) eliminating ReadForMatch before codegen ? (IIRC you created ReadForMatch so I'll check with you :)

Matthew Jasper (Sep 20 2018 at 22:20, on Zulip):

ReadForMatch reaches codegen (but the PR I have open for matches will need to add a pass to remove them, for other reasons).

lqd (Sep 20 2018 at 22:23, on Zulip):

yeah I seemed to remember your & niko's conversation about that

lqd (Sep 21 2018 at 11:34, on Zulip):

@pnkfelix thanks for nudging bors

Last update: Nov 21 2019 at 23:50UTC