Skip to content

fix "still mutable" ice while metrics are enabled #139502

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.

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

Merged
merged 1 commit into from
Apr 10, 2025

Conversation

yaahc
Copy link
Member

@yaahc yaahc commented Apr 7, 2025

Resolves "still mutable" ICE discovered by @matthiaskrgr here: #t-docs-rs > metrics intitiative @ 💬

This was caused by invoking crate_hash before the definitions struct was frozen here:

tcx.untracked().definitions.freeze();

resolved by moving metrics dumping to occur after analysis freezes the definitions

I'm guessing we didn't discover this in CI because the problem only occurs when you try to calculate the crash hash with incremental compilation enabled when it tries to freeze the definitions here:

let definitions = tcx.untracked().definitions.freeze();

my understanding is that this causes us to freeze the definitions too early in compilation, then we subsequently try to mutate them, likely during analysis, and this causes the ICE.

r? @bjorn3

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 7, 2025
Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there any run-make tests for these metrics flags? It would be nice to have a test for this to actually show that this flag works without ICEing in incremental.

@yaahc
Copy link
Member Author

yaahc commented Apr 7, 2025

Are there any run-make tests for these metrics flags? It would be nice to have a test for this to actually show that this flag works without ICEing in incremental.

we have a run-make test but I doubt it enables incremental compilation. I can enable that, question is do I just do that on the same test, do I create a second otherwise identical test that uses incremental, or is there some way to make the test run twice, once with and once without incremental compilation enabled?

@compiler-errors
Copy link
Member

A second test is best, minimized to whatever you need to repro the breakage. Also probably best to check that it faithfully reproduces the ICE without the change in this PR.

@matthiaskrgr
Copy link
Member

maybe we could also change "still mutable" to be slightly more descriptive? 😅

@yaahc
Copy link
Member Author

yaahc commented Apr 8, 2025

maybe we could also change "still mutable" to be slightly more descriptive? 😅

probably something like "freezelock should not be frozen since we're still trying to write to it"

@yaahc yaahc force-pushed the still-mutable-ice branch from e33daf0 to e60650c Compare April 8, 2025 21:11
@rustbot rustbot added the A-run-make Area: port run-make Makefiles to rmake.rs label Apr 8, 2025
@rustbot
Copy link
Collaborator

rustbot commented Apr 8, 2025

This PR modifies run-make tests.

cc @jieyouxu

@yaahc

This comment was marked as outdated.

@yaahc yaahc force-pushed the still-mutable-ice branch from e60650c to 3397a3f Compare April 8, 2025 21:15
@yaahc yaahc force-pushed the still-mutable-ice branch from 3397a3f to 6f55015 Compare April 8, 2025 22:00
@yaahc
Copy link
Member Author

yaahc commented Apr 9, 2025

I got it btw, I needed a late bound parameter which seemed to be the common factor in both of the repros I encountered

https://github.com./rust-lang/rust/pull/139502/files#diff-c576e89e24b5bfa584fdb04e336f6768068b3abc216621232b1dcdc18a3e5315R14

@yaahc yaahc requested a review from bjorn3 April 9, 2025 22:15
@bjorn3
Copy link
Member

bjorn3 commented Apr 10, 2025

@bors r+

@bors
Copy link
Collaborator

bors commented Apr 10, 2025

📌 Commit 6f55015 has been approved by bjorn3

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 Apr 10, 2025
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 10, 2025
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#139502 (fix "still mutable" ice while metrics are enabled)
 - rust-lang#139510 (Rename some `name` variables as `ident`.)
 - rust-lang#139606 (Update compiletest to Edition 2024)
 - rust-lang#139609 (compiletest: don't use stringly paths for `compose_and_run`)
 - rust-lang#139614 (Avoid empty identifiers for delegate params and args.)
 - rust-lang#139626 (Remove unnecessary `mut` in test.)
 - rust-lang#139630 (Miri subtree update)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 0e9c4fb into rust-lang:master Apr 10, 2025
6 checks passed
@rustbot rustbot added this to the 1.88.0 milestone Apr 10, 2025
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Apr 10, 2025
Rollup merge of rust-lang#139502 - yaahc:still-mutable-ice, r=bjorn3

fix "still mutable" ice while metrics are enabled

Resolves "still mutable" ICE discovered by `@matthiaskrgr` here: [#t-docs-rs > metrics intitiative @ 💬](https://rust-lang.zulipchat.com/#narrow/channel/356853-t-docs-rs/topic/metrics.20intitiative/near/510490790)

This was caused by invoking `crate_hash` before the `definitions` struct was frozen here: https://github.com./rust-lang/rust/blob/e643f59f6da3a84f43e75dea99afaa5b041ea6bf/compiler/rustc_interface/src/passes.rs#L951

resolved by moving metrics dumping to occur after `analysis` freezes the definitions

I'm guessing we didn't discover this in CI because the problem only occurs when you try to calculate the crash hash with incremental compilation enabled when it tries to freeze the definitions here: https://github.com./rust-lang/rust/blob/e643f59f6da3a84f43e75dea99afaa5b041ea6bf/compiler/rustc_middle/src/hir/map.rs#L1172

my understanding is that this causes us to freeze the definitions too early in compilation, then we subsequently try to mutate them, likely during `analysis`, and this causes the ICE.

r? `@bjorn3`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-run-make Area: port run-make Makefiles to rmake.rs S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants