fix "still mutable" ice while metrics are enabled

Author: yaahc

compiler/rustc_data_structures/src/sync/freeze.rs

@@ -88,7 +88,7 @@ impl<T> FreezeLock<T> {
     #[inline]
     #[track_caller]
     pub fn write(&self) -> FreezeWriteGuard<'_, T> {
-        self.try_write().expect("still mutable")
+        self.try_write().expect("data should not be frozen if we're still attempting to mutate it")
     }
 
     #[inline]

compiler/rustc_driver_impl/src/lib.rs

@@ -348,10 +348,6 @@ pub fn run_compiler(at_args: &[String], callbacks: &mut (dyn Callbacks + Send))
             // Make sure name resolution and macro expansion is run.
             let _ = tcx.resolver_for_lowering();
 
-            if let Some(metrics_dir) = &sess.opts.unstable_opts.metrics_dir {
-                dump_feature_usage_metrics(tcx, metrics_dir);
-            }
-
             if callbacks.after_expansion(compiler, tcx) == Compilation::Stop {
                 return early_exit();
             }
@@ -370,6 +366,10 @@ pub fn run_compiler(at_args: &[String], callbacks: &mut (dyn Callbacks + Send))
 
             tcx.ensure_ok().analysis(());
 
+            if let Some(metrics_dir) = &sess.opts.unstable_opts.metrics_dir {
+                dump_feature_usage_metrics(tcx, metrics_dir);
+            }
+
             if callbacks.after_analysis(compiler, tcx) == Compilation::Stop {
                 return early_exit();
             }

compiler/rustc_middle/src/query/mod.rs

@@ -1894,6 +1894,11 @@ rustc_queries! {
 
     // The macro which defines `rustc_metadata::provide_extern` depends on this query's name.
     // Changing the name should cause a compiler error, but in case that changes, be aware.
+    //
+    // The hash should not be calculated before the `analysis` pass is complete, specifically
+    // until `tcx.untracked().definitions.freeze()` has been called, otherwise if incremental
+    // compilation is enabled calculating this hash can freeze this structure too early in
+    // compilation and cause subsequent crashes when attempting to write to `definitions`
     query crate_hash(_: CrateNum) -> Svh {
         eval_always
         desc { "looking up the hash a crate" }

tests/run-make/unstable-feature-usage-metrics-incremental/main.rs

@@ -0,0 +1,16 @@
+#![feature(ascii_char)] // random lib feature
+#![feature(box_patterns)] // random lang feature
+
+// picked arbitrary unstable features, just need a random lib and lang feature, ideally ones that
+// won't be stabilized any time soon so we don't have to update this test
+fn main() {
+    for s in quix("foo/bar") {
+        print!("{s}");
+    }
+    println!();
+}
+
+// need a latebound var to trigger the incremental compilation ICE
+fn quix(foo: &str) -> impl Iterator<Item = &'_ str> + '_ {
+    foo.split('/')
+}

tests/run-make/unstable-feature-usage-metrics-incremental/rmake.rs

@@ -0,0 +1,94 @@
+//! This test checks if unstable feature usage metric dump files `unstable-feature-usage*.json` work
+//! as expected.
+//!
+//! - Basic sanity checks on a default ICE dump.
+//!
+//! See <https://github.com/rust-lang/rust/issues/129485>.
+//!
+//! # Test history
+//!
+//! - forked from dump-ice-to-disk test, which has flakeyness issues on i686-mingw, I'm assuming
+//! those will be present in this test as well on the same platform
+
+//@ ignore-windows
+//FIXME(#128911): still flakey on i686-mingw.
+
+use std::path::{Path, PathBuf};
+
+use run_make_support::rfs::create_dir_all;
+use run_make_support::{
+    cwd, filename_contains, has_extension, rfs, run_in_tmpdir, rustc, serde_json,
+    shallow_find_files,
+};
+
+fn find_feature_usage_metrics<P: AsRef<Path>>(dir: P) -> Vec<PathBuf> {
+    shallow_find_files(dir, |path| {
+        if filename_contains(path, "unstable_feature_usage") && has_extension(path, "json") {
+            true
+        } else {
+            dbg!(path);
+            false
+        }
+    })
+}
+
+fn main() {
+    test_metrics_dump();
+    test_metrics_errors();
+}
+
+#[track_caller]
+fn test_metrics_dump() {
+    run_in_tmpdir(|| {
+        let metrics_dir = cwd().join("metrics");
+        create_dir_all(&metrics_dir);
+        rustc()
+            .input("main.rs")
+            .incremental("incremental")
+            .env("RUST_BACKTRACE", "short")
+            .arg(format!("-Zmetrics-dir={}", metrics_dir.display()))
+            .run();
+        let mut metrics = find_feature_usage_metrics(&metrics_dir);
+        let json_path =
+            metrics.pop().expect("there should be one metrics file in the output directory");
+
+        // After the `pop` above, there should be no files left.
+        assert!(
+            metrics.is_empty(),
+            "there should be no more than one metrics file in the output directory"
+        );
+
+        let message = rfs::read_to_string(json_path);
+        let mut parsed: serde_json::Value =
+            serde_json::from_str(&message).expect("metrics should be dumped as json");
+        // remove timestamps
+        assert!(parsed["lib_features"][0]["timestamp"].is_number());
+        assert!(parsed["lang_features"][0]["timestamp"].is_number());
+        parsed["lib_features"][0]["timestamp"] = serde_json::json!(null);
+        parsed["lang_features"][0]["timestamp"] = serde_json::json!(null);
+        let expected = serde_json::json!(
+            {
+                "lib_features":[{"symbol":"ascii_char", "timestamp":null}],
+                "lang_features":[{"symbol":"box_patterns","since":null, "timestamp":null}]
+            }
+        );
+
+        assert_eq!(expected, parsed);
+    });
+}
+
+#[track_caller]
+fn test_metrics_errors() {
+    run_in_tmpdir(|| {
+        rustc()
+            .input("main.rs")
+            .incremental("incremental")
+            .env("RUST_BACKTRACE", "short")
+            .arg("-Zmetrics-dir=invaliddirectorythatdefinitelydoesntexist")
+            .run_fail()
+            .assert_stderr_contains(
+                "error: cannot dump feature usage metrics: No such file or directory",
+            )
+            .assert_stdout_not_contains("internal compiler error");
+    });
+}