Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

don't succeed evaluate_obligation query if new opaque types were registered #98614

Merged
merged 6 commits into from
Jul 8, 2022
Merged

don't succeed evaluate_obligation query if new opaque types were registered #98614

merged 6 commits into from
Jul 8, 2022

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Jun 28, 2022 •
edited
Loading

fixes #98608
fixes #98604

The root cause of all this is that in type flag computation we entirely ignore nongeneric things like struct fields and the signature of function items. So if a flag had to be set for a struct if it is set for a field, that will only happen if the field is generic, as only the generic parameters are checked.

I now believe we cannot use type flags to handle opaque types. They seem like the wrong tool for this.

Instead, this PR replaces the previous logic by adding a new variant of EvaluatedToOk: EvaluatedToOkModuloOpaqueTypes, which says that there were some opaque types that got hidden types bound, but that binding may not have been legal (because we don't know if the opaque type was in its defining scope or not).

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jun 28, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 28, 2022
@oli-obk
Copy link
Contributor Author

oli-obk commented Jun 28, 2022

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jun 28, 2022
@bors
Copy link
Contributor

bors commented Jun 28, 2022

⌛ Trying commit b5c69c28e7c6bc516bcd2a3da1f041127305ee50 with merge 1456cc0b7aca695b7c1ccea221577f816127e562...

@compiler-errors
Copy link
Member

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@bors
Copy link
Contributor

bors commented Jun 28, 2022

⌛ Trying commit 8a10a1153dd020a97c988ebe49805dbe2162dbb5 with merge e0f6bec5731bbf3046e9fcef78660e2a4a2d2ee8...

@bors
Copy link
Contributor

bors commented Jun 28, 2022

☀️ Try build successful - checks-actions
Build commit: e0f6bec5731bbf3046e9fcef78660e2a4a2d2ee8 (e0f6bec5731bbf3046e9fcef78660e2a4a2d2ee8)

@rust-timer
Copy link
Collaborator

Queued e0f6bec5731bbf3046e9fcef78660e2a4a2d2ee8 with parent 00ebeb8, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (e0f6bec5731bbf3046e9fcef78660e2a4a2d2ee8): comparison url.

Instruction count

  • Primary benchmarks: 😿 relevant regressions found
  • Secondary benchmarks: 😿 relevant regressions found
mean1 max count2
Regressions 😿
(primary)		 1.8% 4.6% 53
Regressions 😿
(secondary)		 3.0% 6.7% 30
Improvements 🎉
(primary)		 N/A N/A 0
Improvements 🎉
(secondary)		 -0.3% -0.3% 3
All 😿🎉 (primary) 1.8% 4.6% 53

Max RSS (memory usage)

Results
  • Primary benchmarks: 😿 relevant regressions found
  • Secondary benchmarks: 😿 relevant regression found
mean1 max count2
Regressions 😿
(primary)		 3.4% 10.0% 22
Regressions 😿
(secondary)		 3.2% 3.2% 1
Improvements 🎉
(primary)		 -1.9% -1.9% 1
Improvements 🎉
(secondary)		 N/A N/A 0
All 😿🎉 (primary) 3.2% 10.0% 23

Cycles

Results
  • Primary benchmarks: 😿 relevant regressions found
  • Secondary benchmarks: 😿 relevant regressions found
mean1 max count2
Regressions 😿
(primary)		 3.5% 6.2% 27
Regressions 😿
(secondary)		 4.1% 5.7% 10
Improvements 🎉
(primary)		 N/A N/A 0
Improvements 🎉
(secondary)		 N/A N/A 0
All 😿🎉 (primary) 3.5% 6.2% 27

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf +perf-regression

Footnotes

  1. the arithmetic mean of the percent change ↩ ↩2 ↩3

  2. number of relevant changes ↩ ↩2 ↩3

@rustbot rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Jun 28, 2022
@compiler-errors
Copy link
Member

Oof

@oli-obk
Copy link
Contributor Author

oli-obk commented Jun 29, 2022

@bors try @rust-timer queue

while this has a real impact in secondary benchmarks, the primary ones could be due to other commits, so I've only pushed the actual soundness fix commit

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jun 29, 2022
@bors
Copy link
Contributor

bors commented Jun 29, 2022

⌛ Trying commit bb2e623870d2d12adcf320489e689c191899081d with merge 11ffe800cb85b4ba2042596b0f857b463df891ab...

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@compiler-errors
Copy link
Member

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the stable-accepted Accepted for backporting to the compiler in the stable channel. label Jul 7, 2022
@bors
Copy link
Contributor

bors commented Jul 7, 2022

⌛ Testing commit 0b863e0 with merge ed511e32a055a69d2fda3cbbf32cebe37e43cdd9...

@bors
Copy link
Contributor

bors commented Jul 7, 2022

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 7, 2022
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@oli-obk
Copy link
Contributor Author

oli-obk commented Jul 8, 2022

@bors r=wesleywiser

@bors
Copy link
Contributor

bors commented Jul 8, 2022

📌 Commit d6b93eb has been approved by wesleywiser

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 8, 2022
@bors
Copy link
Contributor

bors commented Jul 8, 2022

⌛ Testing commit d6b93eb with merge 052495d...

@bors
Copy link
Contributor

bors commented Jul 8, 2022

☀️ Test successful - checks-actions
Approved by: wesleywiser
Pushing 052495d to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 8, 2022
@bors bors merged commit 052495d into rust-lang:master Jul 8, 2022
@rustbot rustbot added this to the 1.64.0 milestone Jul 8, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (052495d): comparison url.

Instruction count

  • Primary benchmarks: no relevant changes found
  • Secondary benchmarks: 🎉 relevant improvement found
mean1 max count2
Regressions 😿
(primary)		 N/A N/A 0
Regressions 😿
(secondary)		 N/A N/A 0
Improvements 🎉
(primary)		 N/A N/A 0
Improvements 🎉
(secondary)		 -1.3% -1.3% 1
All 😿🎉 (primary) N/A N/A 0

Max RSS (memory usage)

Results
  • Primary benchmarks: 😿 relevant regressions found
  • Secondary benchmarks: no relevant changes found
mean1 max count2
Regressions 😿
(primary)		 1.1% 1.1% 2
Regressions 😿
(secondary)		 N/A N/A 0
Improvements 🎉
(primary)		 N/A N/A 0
Improvements 🎉
(secondary)		 N/A N/A 0
All 😿🎉 (primary) 1.1% 1.1% 2

Cycles

Results
  • Primary benchmarks: mixed results
  • Secondary benchmarks: no relevant changes found
mean1 max count2
Regressions 😿
(primary)		 3.1% 3.1% 1
Regressions 😿
(secondary)		 N/A N/A 0
Improvements 🎉
(primary)		 -2.3% -2.3% 2
Improvements 🎉
(secondary)		 N/A N/A 0
All 😿🎉 (primary) -0.5% 3.1% 3

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Footnotes

  1. the arithmetic mean of the percent change ↩ ↩2 ↩3

  2. number of relevant changes ↩ ↩2 ↩3

@Mark-Simulacrum Mark-Simulacrum removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jul 9, 2022
@Mark-Simulacrum Mark-Simulacrum modified the milestones: 1.64.0, 1.63.0 Jul 9, 2022
@Mark-Simulacrum Mark-Simulacrum mentioned this pull request Jul 9, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 10, 2022
@bors
Auto merge of rust-lang#99098 - Mark-Simulacrum:beta-next, r=Mark-Sim…
93ef0cd 
…ulacrum

[beta] backport rollup

*  Return a FxIndexSet in is_late_bound query. rust-lang#98959
*  rustdoc: filter '_ lifetimes from ty::PolyTraitRef rust-lang#98727
* don't succeed evaluate_obligation query if new opaque types were registered rust-lang#98614
*  Update llvm-project rust-lang#98567

There's a few more as-yet-unapproved/unmerged PRs that'll land later, but creating a partial rollup for now so that we can include at least some PRs in the first crater run.

r? `@Mark-Simulacrum`
@Mark-Simulacrum Mark-Simulacrum removed the stable-nominated Nominated for backporting to the compiler in the stable channel. label Jul 15, 2022
@Mark-Simulacrum Mark-Simulacrum modified the milestones: 1.63.0, 1.62.1 Jul 15, 2022
@Mark-Simulacrum Mark-Simulacrum mentioned this pull request Jul 15, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 16, 2022
@bors
Auto merge of rust-lang#99299 - Mark-Simulacrum:stable-next, r=Mark-S…
e092d0b 
…imulacrum

[stable] 1.62.1 release

This bundles:

*  Windows: Fallback for overlapped I/O rust-lang#98950
*  don't succeed evaluate_obligation query if new opaque types were registered rust-lang#98614
*  Mitigate MMIO stale data vulnerability rust-lang#98126
*  Return a FxIndexSet in is_late_bound query. rust-lang#99219

Also bumps the version number to 1.62.1 and includes a short release notes section for the release.

r? `@Mark-Simulacrum`
@steffahn steffahn mentioned this pull request Aug 28, 2022
Closed
@oli-obk oli-obk deleted the take_unsound_opaque_types branch August 31, 2022 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@compiler-errors compiler-errors compiler-errors approved these changes

@wesleywiser wesleywiser wesleywiser approved these changes

Assignees

@wesleywiser wesleywiser

Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. stable-accepted Accepted for backporting to the compiler in the stable channel. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.62.1