Refactor fingerprint reconstruction by Mark-Simulacrum · Pull Request #89343 · rust-lang/rust

Mark-Simulacrum · 2021-09-28T23:24:02Z

This PR replaces can_reconstruct_query_key with fingerprint_style, which returns the style of the fingerprint for that query. This allows us to avoid trying to extract a DefId (or equivalent) from keys which are reconstructible because they're () but not as DefIds.

This is done with the goal of fixing -Zdump-dep-graph, which seems to have broken a while ago (I didn't try to bisect). Currently even on a fn main() {} file it'll ICE (you need to also pass -Zquery-dep-graph for it to work at all), and this patch indirectly fixes the cause of that ICE. This also adds a test for it continuing to work.

rust-highfive · 2021-09-28T23:24:05Z

r? @matthewjasper

(rust-highfive has picked a reviewer for you, use r? to override)

Mark-Simulacrum · 2021-09-28T23:25:01Z

cc @rust-lang/wg-incr-comp -- I'm not sure if folks recall the intent of has_params, which might be helpful to determine if this is the "right" thing to do.

compiler/rustc_middle/src/ty/query.rs

cjgillot · 2021-09-29T17:16:58Z

IIUC, the crash happens when trying to debug-print a DepNode whch parameter () by mistakenly trying to extract a DefId from a Fingerprint which is not a proper DefPathHash. The bug is therefore in rustc_middle::dep_graph::dep_node::DepNodeExt::extract_def_id which tests can_reconstruct_query_key() to know whether the query key is a CrateNum/LocalDefId/DefId. I think this PR hides the problem rather than solving it. The correct fix would be to create a separate has_def_path_hash field in DepKindStruct which tests whether the query key is any of the three, and equivalently if a DefId can be extracted from the fingerprint.

Mark-Simulacrum · 2021-09-29T17:22:06Z

Hm, that seems like a good thing to tackle, I agree. I think this PR is still desirable (or we should just remove has_params), since it seems like the meaning of that is not too clear right now.

I'll have to dig into the 2 manually declared queries with has_params: false, and see if that's significant for them...

Mark-Simulacrum · 2021-10-02T16:44:31Z

Alright, replaced pretty much the whole PR with different patch -- and extracted #89466 for the base macro changes which are not actually needed for this PR now.

I looked into fully removing has_params but didn't end up making that change. The only uses left are the debug assert in DepKind::new_no_params and the debug_node method. The latter could be removed with no loss, I believe -- just a difference in how nodes look (printed as foo today, foo() with that condition dropped). Since has_params is true currently for all but Null and TraitSelect DepKinds (neither of which is a proper query), it's probably the case that this could be refactored. But seems like future work.

cjgillot · 2021-10-02T16:47:54Z

compiler/rustc_middle/src/dep_graph/dep_node.rs

+        match kind.fingerprint_style() {
+            FingerprintStyle::Opaque => Err(()),
+            FingerprintStyle::Unit => {
+                if !kind.has_params {


FingerprintStyle::Unit should now be equivalent to has_params == false, isn't it? We should be able to get rid of has_params.

Not currently. A query with () key currently "has_params"; the only two things to not have params are Null and TraitSelect which are not generated through the normal macro infrastructure. I dropped the "fix" for that from this branch for now, though I can re-add it, it's not big (b2b53bb9a4f2f5e1f96f8f01ad7b378f5eb1b4d1).

cjgillot · 2021-10-02T22:12:04Z

@bors r+

bors · 2021-10-02T22:12:05Z

📌 Commit 63aaf88 has been approved by cjgillot

…cjgillot Refactor fingerprint reconstruction This PR replaces can_reconstruct_query_key with fingerprint_style, which returns the style of the fingerprint for that query. This allows us to avoid trying to extract a DefId (or equivalent) from keys which *are* reconstructible because they're () but not as DefIds. This is done with the goal of fixing -Zdump-dep-graph, which seems to have broken a while ago (I didn't try to bisect). Currently even on a `fn main() {}` file it'll ICE (you need to also pass -Zquery-dep-graph for it to work at all), and this patch indirectly fixes the cause of that ICE. This also adds a test for it continuing to work.

bors · 2021-10-05T13:19:02Z

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

Keys can be reconstructed from fingerprints that are not DefPathHash, but then we cannot extract a DefId from them.

Mark-Simulacrum · 2021-10-07T02:23:19Z

@bors r=cjgillot rollup=iffy

bors · 2021-10-07T02:23:20Z

📌 Commit 85c4fd81799de01e8eb3a84f96a13baafc8de407 has been approved by cjgillot

bors · 2021-10-09T09:41:05Z

⌛ Testing commit 85c4fd81799de01e8eb3a84f96a13baafc8de407 with merge 8f2565b630f71928671276797f4659fcc679d795...

bors · 2021-10-09T10:00:50Z

💔 Test failed - checks-actions

Mark-Simulacrum · 2021-10-09T12:14:15Z

@bors r=cjgillot rollup=iffy

bors · 2021-10-09T12:14:17Z

📌 Commit 415a9a2 has been approved by cjgillot

bors · 2021-10-09T13:13:10Z

⌛ Testing commit 415a9a2 with merge 15491d7...

bors · 2021-10-09T16:01:24Z

☀️ Test successful - checks-actions
Approved by: cjgillot
Pushing 15491d7 to master...

rust-timer · 2021-10-09T17:34:13Z

Finished benchmarking commit (15491d7): comparison url.

Summary: This benchmark run did not return any relevant changes.

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

@rustbot label: -perf-regression

Debug logging during incremental compilation had been broken for some time, until rust-lang#89343 fixed it (among other things). Add a test so this is less likely to break without being noticed. This test is nearly a copy of the `src/test/ui/rustc-rust-log.rs` test, but tests debug logging in the incremental compliation code paths.

…t, r=Mark-Simulacrum Add test for debug logging during incremental compilation Debug logging during incremental compilation had been broken for some time, until rust-lang#89343 fixed it (among other things). Add a test so this is less likely to break without being noticed. This test is nearly a copy of the `src/test/ui/rustc-rust-log.rs` test, but tests debug logging in the incremental compliation code paths.

rust-highfive assigned matthewjasper Sep 28, 2021

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

Mark-Simulacrum commented Sep 28, 2021

View reviewed changes

compiler/rustc_middle/src/ty/query.rs Outdated Show resolved Hide resolved

cjgillot self-assigned this Sep 29, 2021

Mark-Simulacrum unassigned matthewjasper Sep 29, 2021

cjgillot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 29, 2021

Mark-Simulacrum force-pushed the no-args-queries branch from b2b53bb to 0d1ff33 Compare October 2, 2021 16:22

Mark-Simulacrum changed the title ~~Fix -Zdump-dep-graph~~ Refactor fingerprint reconstruction Oct 2, 2021

Mark-Simulacrum force-pushed the no-args-queries branch 2 times, most recently from 175631d to 3d73c4d Compare October 2, 2021 16:31

Mark-Simulacrum added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 2, 2021

This comment has been minimized.

Sign in to view

Mark-Simulacrum force-pushed the no-args-queries branch 2 times, most recently from 9173a20 to 63aaf88 Compare October 2, 2021 16:44

cjgillot reviewed Oct 2, 2021

View reviewed changes

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 Oct 2, 2021

Mark-Simulacrum mentioned this pull request Oct 3, 2021

Add dep-graph collector rust-lang/rustc-perf#1046

Merged

Manishearth mentioned this pull request Oct 3, 2021

Rollup of 6 pull requests #89481

Closed

Query the fingerprint style during key reconstruction

6f78eed

Keys can be reconstructed from fingerprints that are not DefPathHash, but then we cannot extract a DefId from them.

Mark-Simulacrum force-pushed the no-args-queries branch from 63aaf88 to 85c4fd8 Compare October 7, 2021 02:21

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 7, 2021

This comment has been minimized.

Sign in to view

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 Oct 9, 2021

Add a test that -Zquery-dep-graph -Zdump-dep-graph works

415a9a2

Mark-Simulacrum force-pushed the no-args-queries branch from 85c4fd8 to 415a9a2 Compare October 9, 2021 12:14

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 Oct 9, 2021

bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 9, 2021

bors merged commit 15491d7 into rust-lang:master Oct 9, 2021

rustbot added this to the 1.57.0 milestone Oct 9, 2021

Mark-Simulacrum deleted the no-args-queries branch October 9, 2021 17:02

tgnottingham mentioned this pull request Oct 18, 2021

Add test for debug logging during incremental compilation #90029

Merged

Uh oh!

Conversation

Mark-Simulacrum commented Sep 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rust-highfive commented Sep 28, 2021

Uh oh!

Mark-Simulacrum commented Sep 28, 2021

Uh oh!

Uh oh!

cjgillot commented Sep 29, 2021

Uh oh!

Mark-Simulacrum commented Sep 29, 2021

Uh oh!

This comment has been minimized.

Mark-Simulacrum commented Oct 2, 2021

Uh oh!

cjgillot Oct 2, 2021

Choose a reason for hiding this comment

Uh oh!

Mark-Simulacrum Oct 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjgillot commented Oct 2, 2021

Uh oh!

bors commented Oct 2, 2021

Uh oh!

bors commented Oct 5, 2021

Uh oh!

Mark-Simulacrum commented Oct 7, 2021

Uh oh!

bors commented Oct 7, 2021

Uh oh!

bors commented Oct 9, 2021

Uh oh!

This comment has been minimized.

bors commented Oct 9, 2021

Uh oh!

Mark-Simulacrum commented Oct 9, 2021

Uh oh!

bors commented Oct 9, 2021

Uh oh!

bors commented Oct 9, 2021

Uh oh!

bors commented Oct 9, 2021

Uh oh!

rust-timer commented Oct 9, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

Mark-Simulacrum commented Sep 28, 2021 •

edited

Loading

Mark-Simulacrum Oct 2, 2021 •

edited

Loading