make evaluation track whether outlives relationships mattered #54401

nikomatsakis · 2018-09-20T17:58:58Z

Previously, evaluation ignored outlives relationships. Since we using
evaluation to skip the "normal" trait selection (which enforces
outlives relationships) this led to incorrect results in some cases.

Fixes #54302

r? @arielb1

rust-highfive · 2018-09-20T18:14:28Z

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.


[00:05:52] travis_fold:start:tidy
travis_time:start:tidy
tidy check
[00:05:53] tidy error: /checkout/src/librustc/traits/select.rs:319: line longer than 100 chars
[00:05:54] some tidy checks failed
[00:05:54] 
[00:05:54] 
[00:05:54] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor" "--quiet"
[00:05:54] 
[00:05:54] 
[00:05:54] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:05:54] Build completed unsuccessfully in 0:00:55
[00:05:54] Build completed unsuccessfully in 0:00:55
[00:05:54] Makefile:79: recipe for target 'tidy' failed
[00:05:54] make: *** [tidy] Error 1

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:0c40e0b5
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
---
travis_time:end:0185ecc8:start=1537466793949573596,finish=1537466793953632315,duration=4058719
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:0deeab0c
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:05c59895
travis_time:start:05c59895
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:11891b20
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

bors · 2018-09-21T01:20:24Z

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

arielb1 · 2018-09-21T18:36:57Z

I feel that this mismatch between evaluation and "normal" selection is fairly likely to cause quite a few problems - ICEs and the like (i.e., evaluation returns "success" for &'a str: DeserializeOwned for every 'a, but selection returns failure).

Also, the current implementation might kick a bunch of types out of the evaluation fastpath during trans, which is also quite unfortunate.

How good looking are universe-based lifetimes? Can this wait for them?

rust-highfive · 2018-09-21T23:18:20Z

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.


[00:05:46] travis_fold:start:tidy
travis_time:start:tidy
tidy check
[00:05:46] tidy error: /checkout/src/librustc/traits/select.rs:335: line longer than 100 chars
[00:05:47] some tidy checks failed
[00:05:47] 
[00:05:47] 
[00:05:47] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor" "--quiet"
[00:05:47] 
[00:05:47] 
[00:05:47] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:05:47] Build completed unsuccessfully in 0:00:50
[00:05:47] Build completed unsuccessfully in 0:00:50
[00:05:47] Makefile:79: recipe for target 'tidy' failed
[00:05:47] make: *** [tidy] Error 1

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:010af6e8
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
---
travis_time:end:09fedd70:start=1537571451532001575,finish=1537571451536499649,duration=4498074
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:06049700
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:23f6d0d8
travis_time:start:23f6d0d8
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:18478965
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

bors · 2018-09-22T17:03:33Z

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

nikomatsakis · 2018-09-25T15:45:39Z

@arielb1

I feel that this mismatch between evaluation and "normal" selection is fairly likely to cause quite a few problems - ICEs and the like (i.e., evaluation returns "success" for &'a str: DeserializeOwned for every 'a, but selection returns failure).

I don't disagree. This is partial motivator for getting rid of the distinction in the move to chalk-style trait evaluation (which I expect to be taking place in earnest this fall). In answer to your question about universes, I've been working on a branch for that on the side, hoping to open soon though I have to resolve a few complications.

All of that said, it feels like this is a gaping soundness hole that we ought to close sooner rather than later. This PR seemed like a pretty simple and clean fix. Do you see any particular problems with it, or does it just not seem to go far enough for your taste?

nikomatsakis · 2018-09-25T15:47:16Z

Also, the current implementation might kick a bunch of types out of the evaluation fastpath during trans, which is also quite unfortunate.

We could probably measure this — I was considering that we could make it less pessimistic by making evaluation look at the outlives code to see if it trivially holds.

e.g., I believe it is true that if we see a T: 'static and T either has no regions or has 'static as its only free region, then we know that holds, and so we could return just Ok. (Similarly with 'erased, I guess?)

Previously, evaluation ignored outlives relationships. Since we using evaluation to skip the "normal" trait selection (which enforces outlives relationships) this led to incorrect results in some cases.

arielb1 · 2018-09-25T17:04:01Z

let's see how bad does this regress performance due to caching losses

@bors try

bors · 2018-09-25T17:04:38Z

⌛ Trying commit 563ba10 with merge 643b173...

@arielb1

make evaluation track whether outlives relationships mattered Previously, evaluation ignored outlives relationships. Since we using evaluation to skip the "normal" trait selection (which enforces outlives relationships) this led to incorrect results in some cases. Fixes #54302 r? @arielb1

bors · 2018-09-25T19:17:05Z

☀️ Test successful - status-travis
State: approved= try=True

nikomatsakis · 2018-09-25T19:18:38Z

@rust-timer build 643b173

rust-timer · 2018-09-25T19:18:39Z

Success: Queued 643b173 with parent ae36663, comparison URL.

nikomatsakis · 2018-09-26T13:29:22Z

The perf results seem to show minimal impact. I suspect the regressions (and improvements) shown are the result of noise:

syn-opt	avg: 0.8%?	min: 0.0%?	max: 2.8%?
clean-opt	51,610,067,699.00	53,059,845,272.00	2.8%
baseline incremental-opt	52,560,065,219.00	52,591,270,670.00	0.1%
clean incremental-opt	3,000,552,136.00	3,001,633,129.00	0.0%
patched incremental: println-opt	25,848,871,118.00	25,880,211,293.00	0.1%

style-servo-opt	avg: 0.6%	min: -0.0%	max: 2.6%
clean-opt	935,364,607,039.00	959,995,759,316.00	2.6%
baseline incremental-opt	1,363,214,051,941.00	1,363,351,122,470.00	0.0%
clean incremental-opt	75,435,841,874.00	75,521,318,871.00	0.1%??
patched incremental: println-opt	1,136,078,938,220.00	1,136,248,978,732.00	0.0%
patched incremental: b9b3e592dd cherry picked-opt	1,209,845,321,348.00	1,209,753,421,219.00	-0.0%

inflate-opt	avg: -0.0%?	min: -1.7%?	max: 1.2%?
clean-opt	78,019,599,701.00	78,953,029,169.00	1.2%
baseline incremental-opt	80,337,293,732.00	78,968,375,117.00	-1.7%
clean incremental-opt	1,894,385,626.00	1,892,355,665.00	-0.1%
patched incremental: println-opt	77,536,761,080.00	77,996,146,688.00	0.6%

nikomatsakis · 2018-09-26T13:30:35Z

Well, perhaps I was hasty in calling them noise. Still, it's a bit suspicious for clean-opt to show a regression but baseline incremental-opt not to do so, since they are doing all of the same trait operations.

arielb1 · 2018-09-27T18:25:00Z

src/librustc/traits/select.rs

@@ -900,7 +917,13 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> {
        {
            debug!("evaluate_stack({:?}) --> recursive",
                   stack.fresh_trait_ref);
-            let cycle = stack.iter().skip(1).take(rec_index + 1);
+
+            // Subtle: when checking for a coinductive cycle, we do


Could you give an example where this subtle thing is true? It certainly seems like we are comparing freshened regions, and that should be OK: freshening does not touch LBRs (even escaping LBRs), and trait evaluation pretty much works with the staticized version of a type, ignoring non-late-bound regions.

nikomatsakis · 2018-10-02T14:28:31Z

Closing in favor of #54624

@nikomatsakis

…tsakis handle outlives predicates in trait evaluation This handles higher-ranked outlives predicates in trait evaluation the same way they are handled in projection. Fixes rust-lang#54302. I think this is a more correct fix than rust-lang#54401 because it fixes the root case in evaluation instead of making evaluation used in less cases. However, we might want to go to a direction closer to @nikomatsakis's solution with Chalk. r? @nikomatsakis

@nikomatsakis

handle outlives predicates in trait evaluation This handles higher-ranked outlives predicates in trait evaluation the same way they are handled in projection. Fixes #54302. I think this is a more correct fix than #54401 because it fixes the root case in evaluation instead of making evaluation used in less cases. However, we might want to go to a direction closer to @nikomatsakis's solution with Chalk. r? @nikomatsakis

rust-highfive assigned arielb1 Sep 20, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 20, 2018

nikomatsakis force-pushed the issue-54302-hrtb-fail branch from 8732915 to 34424f6 Compare September 21, 2018 23:03

nikomatsakis added 3 commits September 25, 2018 11:48

make evaluation track whether outlives relationships mattered

ebe5c30

Previously, evaluation ignored outlives relationships. Since we using evaluation to skip the "normal" trait selection (which enforces outlives relationships) this led to incorrect results in some cases.

select.rs: rustfmt

521f9da

pacify the mercilous tidy

563ba10

nikomatsakis force-pushed the issue-54302-hrtb-fail branch from 86c806c to 563ba10 Compare September 25, 2018 16:45

pnkfelix mentioned this pull request Sep 27, 2018

Trait with HRTB blanket impl is implemented when it shouldn't be #54302

Closed

arielb1 reviewed Sep 27, 2018

View reviewed changes

arielb1 mentioned this pull request Sep 27, 2018

handle outlives predicates in trait evaluation #54624

Merged

nikomatsakis closed this Oct 2, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

make evaluation track whether outlives relationships mattered #54401

make evaluation track whether outlives relationships mattered #54401

nikomatsakis commented Sep 20, 2018

rust-highfive commented Sep 20, 2018

bors commented Sep 21, 2018

arielb1 commented Sep 21, 2018 •

edited

Loading

rust-highfive commented Sep 21, 2018

bors commented Sep 22, 2018

nikomatsakis commented Sep 25, 2018

nikomatsakis commented Sep 25, 2018

arielb1 commented Sep 25, 2018

bors commented Sep 25, 2018

bors commented Sep 25, 2018

nikomatsakis commented Sep 25, 2018

rust-timer commented Sep 25, 2018

nikomatsakis commented Sep 26, 2018

nikomatsakis commented Sep 26, 2018

arielb1 Sep 27, 2018

nikomatsakis commented Oct 2, 2018

make evaluation track whether outlives relationships mattered #54401

make evaluation track whether outlives relationships mattered #54401

Conversation

nikomatsakis commented Sep 20, 2018

rust-highfive commented Sep 20, 2018

bors commented Sep 21, 2018

arielb1 commented Sep 21, 2018 • edited Loading

rust-highfive commented Sep 21, 2018

bors commented Sep 22, 2018

nikomatsakis commented Sep 25, 2018

nikomatsakis commented Sep 25, 2018

arielb1 commented Sep 25, 2018

bors commented Sep 25, 2018

bors commented Sep 25, 2018

nikomatsakis commented Sep 25, 2018

rust-timer commented Sep 25, 2018

nikomatsakis commented Sep 26, 2018

nikomatsakis commented Sep 26, 2018

arielb1 Sep 27, 2018

Choose a reason for hiding this comment

nikomatsakis commented Oct 2, 2018

arielb1 commented Sep 21, 2018 •

edited

Loading