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

Fix ICE in incremental compilation when rust-src component is changed #83591

Closed
wants to merge 3 commits into from
Closed

Fix ICE in incremental compilation when rust-src component is changed #83591

wants to merge 3 commits into from

Conversation

Aaron1011
Copy link
Member

@Aaron1011 Aaron1011 commented Mar 27, 2021 •
edited
Loading

When importing source files from an external crate, we try to remap 'virtualized' filenames pointing into the standard library. However, we only do this remapping if the rust-src component is present on disk. As a result, the values we decode from a foreign crate's metadata depend on untracked global state.

This PR explicitly tracks our dependency on this global state via the crate_hash query. Instead of just returning an Svh, we return a CrateHashAndState, which holds the current state of the rust-src component. As a result, changes to this will cause all queries that depend on crate_hash (e.g. all external queries) to get re-run, avoiding the ICEs that result from an unexpected hash change.

@rust-highfive
Copy link
Collaborator

r? @davidtwco

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 27, 2021
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@Aaron1011 Aaron1011 force-pushed the fix-incr-devirtualized branch from b8c8979 to 3b40763 Compare March 28, 2021 19:52
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@Aaron1011 Aaron1011 force-pushed the fix-incr-devirtualized branch from 3b40763 to 779fedc Compare April 3, 2021 19:23
@Aaron1011
Copy link
Member Author

@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 Apr 3, 2021
@bors
Copy link
Contributor

bors commented Apr 3, 2021

⌛ Trying commit 779fedc95fbc68b2c3ea1065a3aef383fd383c29 with merge 87a37cc83f9cf37be440e573e062eaec4f60dc74...

@bors
Copy link
Contributor

bors commented Apr 3, 2021

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

@rust-timer
Copy link
Collaborator

Queued 87a37cc83f9cf37be440e573e062eaec4f60dc74 with parent 97717a5, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (87a37cc83f9cf37be440e573e062eaec4f60dc74): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

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

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Apr 3, 2021
@davidtwco
Copy link
Member

@Aaron1011 is this still a work in progress or are you looking for a review?

@Aaron1011
Copy link
Member Author

I'm still trying to reproduce the ICE with src/test/run-make-fulldeps/incr-add-rust-src-component/Makefile - it should be happening now, but it isn't for some reason. Once I've done that, I'll add the actual fix (which is making a custom PartialEq and HashStable impl)

@davidtwco davidtwco 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 Apr 5, 2021
@Aaron1011
Copy link
Member Author

We don't build the standard library with remapped debug info on CI, which means that the test src/test/run-make-fulldeps/incr-add-rust-src-component doesn't do anything.

@Aaron1011 Aaron1011 mentioned this pull request Apr 6, 2021
Closed
@Aaron1011 Aaron1011 force-pushed the fix-incr-devirtualized branch from 779fedc to 07a6ec0 Compare April 8, 2021 20:49
@Aaron1011
Copy link
Member Author

@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 Apr 8, 2021
@bors
Copy link
Contributor

bors commented Apr 8, 2021

⌛ Trying commit 07a6ec0cd2c7f67b786f83624b2a819cc6aa5384 with merge fe9de0761e7a673c82626a3dbc1e45b532645214...

@bors
Copy link
Contributor

bors commented Apr 8, 2021

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

@rust-timer
Copy link
Collaborator

Queued fe9de0761e7a673c82626a3dbc1e45b532645214 with parent 1255053, future comparison URL.

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Apr 9, 2021
@Aaron1011
Properly track virtual path remapping during incremental compilation
8317aa1 
When the `rust-src` component is installed, rustc will automatically
remap paths from the standard library to point into the local system.
The target filesystem location is stored in
`tcx.sess.real_rust_source_base_dir`

This is a form of global state that's not tracked by the incremental
compilation system, which can lead to ICEs when the `rust-src` component
is added or removed between compilation sessions.

This PR fixes several bugs in incremental compilation
* We now hash `real_rust_source_base_dir` when computing a crate hash,
  since it affects any spans that we import from foreign crates (e.g.
  via macros or inlining).
* The `crate_hash` and `host_crate_hash` quereis now return a wrapper
  around `Svh`, which also hashes the value of
  `real_rust_source_base_dir` in the *current* compilation session.
  This reflects the fact that changes to this value affect how we decode
  values from all foreign crates.
* We now properly re-compute the `name_hash` of an imported `SourceFile`
  when we remap a path.

There was a test for this kind of issue, but it was broken when the
standard library was moved to `library/std`, and was no longer actually
testing anything. To ensure that this doesn't happen again, I've
modified the test to build a binary that outputs a remapped standard
library path (via a panic), and verifies that the precense of `rust-src`
during compilation correctly affects the output of the compiled binary.
@Aaron1011
Use Fingerprint instead of full path
2275f81
@Aaron1011 Aaron1011 mentioned this pull request Apr 13, 2021
Closed
@Aaron1011 Aaron1011 force-pushed the fix-incr-devirtualized branch from 899a5ea to 7edda7b Compare April 13, 2021 19:42
@Aaron1011
Copy link
Member Author

@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 Apr 13, 2021
@bors
Copy link
Contributor

bors commented Apr 13, 2021

⌛ Trying commit 7edda7b with merge eb7cdc743679b9eec36e733c4853fe8997193e86...

@Aaron1011
Copy link
Member Author

According to #83813 (comment), we actually dont want remapped paths in crate metadata. So, this PR might actually be redundant.

@bors
Copy link
Contributor

bors commented Apr 13, 2021

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

@rust-timer
Copy link
Collaborator

Queued eb7cdc743679b9eec36e733c4853fe8997193e86 with parent 5c13042, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (eb7cdc743679b9eec36e733c4853fe8997193e86): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

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

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Apr 13, 2021
@Aaron1011 Aaron1011 changed the title [WIP] Fix ICE in incremental compilation when rust-src component is changed Fix ICE in incremental compilation when rust-src component is changed Apr 19, 2021
@Aaron1011 Aaron1011 removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Apr 19, 2021
@Aaron1011
Copy link
Member Author

This is now ready for review. The perf results look bad, but all of the large regressions are on extremely short benchmarks (e.g. helloworld). All of the larger results show minimal (< 1%) regressions. I suspect that these regressions are caused by the increase in the one-time cost of importing all source files from a foreign crate.

A large number of people are hitting #84341 in various forms. I'm not 100% certain that this PR will fix all of those issues - however, it will definitely fix some, and will allow us to narrow down the cause of any ones which aren't fixed.

cc @rust-lang/wg-incr-comp

@Aaron1011
Copy link
Member Author

While this will probably generate merge conflicts with #83813, I believe the two PRs are actually independent of each other. I believe that #83813 will cause us to stop writing devirtualized paths into the metadata - however, any devirtualized paths that we use (e.g. in diagnostics) will still affect query results. Therefore, I think it will still be necessary for us to explicitly track the state of devirtualization via the crate_hash result.

@cbeuw cbeuw mentioned this pull request Apr 21, 2021
Merged
@davidtwco
Copy link
Member

These changes look good to me, but someone else from incr-comp should probably take a look.

r? @wesleywiser

@Aaron1011 Aaron1011 added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 22, 2021
@Aaron1011
Copy link
Member Author

There's been additional discussion in #83813. The PR might end up being unecessary after all, or we might want to do with the simpler approach of wiping the entire incremental cache with rust-src changes. In any case, that PR will generate merge conflicts, so let's wait for it to get merged.

@Aaron1011
Copy link
Member Author

Superseded by #84233, which invalidates the entire incremental cache when rust-src changes.

@Aaron1011 Aaron1011 closed this May 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidtwco davidtwco davidtwco approved these changes

Assignees

@wesleywiser wesleywiser

Labels
S-blocked Status: Blocked on something else such as an RFC or other implementation work.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@Aaron1011 @rust-highfive @rust-log-analyzer @rust-timer @bors @davidtwco @wesleywiser @rustbot