Rollup merge of #157352 - xmakro:fix/parallel-dep-graph-determinism, r=petrochenkov

Make the retained dep graph deterministic under the parallel frontend

Part of #154314.

The in-memory dep graph kept for `-Zquery-dep-graph` is built by `GraphEncoder::record`, which pushed each node using `try_lock` and silently dropped the node when the lock was contended. Single-threaded the only contention is a query forced re-entrantly from within `with_retained_dep_graph`, so dropping was harmless. Under the parallel frontend several encoder threads record nodes at the same time, so a contended `try_lock` dropped nodes and edges at random, leaving the retained graph nondeterministic. The dep-graph ui tests, which assert on its contents, then failed intermittently.

The reentrancy came from `check_paths` emitting its diagnostics while holding the retained graph: the error path calls `def_path_str`, which can create new dep nodes and re-enter the encoder. This reads the path results into owned values while the lock is held and emits the diagnostics afterwards, so `record` never re-enters while the lock is held. `record` can then block on the lock unconditionally instead of using `try_lock`, so concurrent encoders never drop a node or edge.

Author: jhpratt

compiler/rustc_data_structures/src/graph/linked_graph/mod.rs

@@ -45,17 +45,19 @@ mod tests;
 /// This graph implementation predates the later [graph traits](crate::graph),
 /// and does not implement those traits, so it has its own implementations of a
 /// few basic graph algorithms.
+#[derive(Clone)]
 pub struct LinkedGraph<N, E> {
     nodes: IndexVec<NodeIndex, Node<N>>,
     edges: Vec<Edge<E>>,
 }
 
+#[derive(Clone)]
 pub struct Node<N> {
     first_edge: [EdgeIndex; 2], // see module comment
     pub data: Option<N>,
 }
 
-#[derive(Debug)]
+#[derive(Clone, Debug)]
 pub struct Edge<E> {
     next_edge: [EdgeIndex; 2], // see module comment
     source: NodeIndex,

compiler/rustc_incremental/src/assert_dep_graph.rs

@@ -57,8 +57,14 @@ use crate::errors;
 #[allow(missing_docs)]
 pub(crate) fn assert_dep_graph(tcx: TyCtxt<'_>) {
     tcx.dep_graph.with_ignore(|| {
+        // Clone the retained dep graph once and share it between the graph dump and the path
+        // checks below, rather than locking and cloning it separately for each.
+        let retained_dep_graph = tcx.dep_graph.retained_dep_graph();
+
         if tcx.sess.opts.unstable_opts.dump_dep_graph {
-            tcx.dep_graph.with_retained_dep_graph(dump_graph);
+            if let Some(graph) = &retained_dep_graph {
+                dump_graph(graph);
+            }
         }
 
         if !tcx.sess.opts.unstable_opts.query_dep_graph {
@@ -92,7 +98,7 @@ pub(crate) fn assert_dep_graph(tcx: TyCtxt<'_>) {
         }
 
         // Check paths.
-        check_paths(tcx, &if_this_changed, &then_this_would_need);
+        check_paths(tcx, retained_dep_graph.as_ref(), &if_this_changed, &then_this_would_need);
     })
 }
 
@@ -172,30 +178,33 @@ impl<'tcx> Visitor<'tcx> for IfThisChanged<'tcx> {
     }
 }
 
-fn check_paths<'tcx>(tcx: TyCtxt<'tcx>, if_this_changed: &Sources, then_this_would_need: &Targets) {
-    // Return early here so as not to construct the query, which is not cheap.
+fn check_paths<'tcx>(
+    tcx: TyCtxt<'tcx>,
+    retained_dep_graph: Option<&RetainedDepGraph>,
+    if_this_changed: &Sources,
+    then_this_would_need: &Targets,
+) {
     if if_this_changed.is_empty() {
         for &(target_span, _, _, _) in then_this_would_need {
             tcx.dcx().emit_err(errors::MissingIfThisChanged { span: target_span });
         }
         return;
     }
-    tcx.dep_graph.with_retained_dep_graph(|query| {
-        for &(_, source_def_id, ref source_dep_node) in if_this_changed {
-            let dependents = query.transitive_predecessors(source_dep_node);
-            for &(target_span, ref target_pass, _, ref target_dep_node) in then_this_would_need {
-                if !dependents.contains(&target_dep_node) {
-                    tcx.dcx().emit_err(errors::NoPath {
-                        span: target_span,
-                        source: tcx.def_path_str(source_def_id),
-                        target: *target_pass,
-                    });
-                } else {
-                    tcx.dcx().emit_err(errors::Ok { span: target_span });
-                }
+    let Some(query) = retained_dep_graph else { return };
+    for &(_, source_def_id, ref source_dep_node) in if_this_changed {
+        let dependents = query.transitive_predecessors(source_dep_node);
+        for &(target_span, ref target_pass, _, ref target_dep_node) in then_this_would_need {
+            if !dependents.contains(&target_dep_node) {
+                tcx.dcx().emit_err(errors::NoPath {
+                    span: target_span,
+                    source: tcx.def_path_str(source_def_id),
+                    target: *target_pass,
+                });
+            } else {
+                tcx.dcx().emit_err(errors::Ok { span: target_span });
             }
         }
-    });
+    }
 }
 
 fn dump_graph(graph: &RetainedDepGraph) {

compiler/rustc_middle/src/dep_graph/graph.rs

@@ -195,10 +195,12 @@ impl DepGraph {
         self.data.is_some()
     }
 
-    pub fn with_retained_dep_graph(&self, f: impl Fn(&RetainedDepGraph)) {
-        if let Some(data) = &self.data {
-            data.current.encoder.with_retained_dep_graph(f)
-        }
+    /// Returns a clone of the in-memory retained dep graph, if it is being built
+    /// (i.e. `-Zquery-dep-graph` is set). Cloning rather than exposing the lock keeps
+    /// callers from holding it while forcing queries, which would deadlock against a
+    /// reentrant `record` under the parallel frontend.
+    pub fn retained_dep_graph(&self) -> Option<RetainedDepGraph> {
+        self.data.as_ref().and_then(|data| data.current.encoder.retained_dep_graph())
     }
 
     pub fn assert_ignored(&self) {

compiler/rustc_middle/src/dep_graph/retained.rs

@@ -9,6 +9,7 @@ use super::{DepNode, DepNodeIndex};
 /// Normally, dependencies recorded during the current session are written to
 /// disk and then forgotten, to avoid wasting memory on information that is
 /// not needed when the compiler is working correctly.
+#[derive(Clone)]
 pub struct RetainedDepGraph {
     pub inner: LinkedGraph<DepNode, ()>,
     pub indices: FxHashMap<DepNode, NodeIndex>,

compiler/rustc_middle/src/dep_graph/serialized.rs

@@ -636,10 +636,12 @@ impl EncoderState {
 
             // Outline the build of the full dep graph as it's typically disabled and cold.
             outline(move || {
-                // Do not ICE when a query is called from within `with_query`.
-                if let Some(retained_graph) = &mut retained_graph.try_lock() {
-                    retained_graph.push(index, *node, &edges);
-                }
+                // Block on the lock rather than using `try_lock`: under the parallel frontend
+                // several threads record nodes concurrently, and dropping a node on lock
+                // contention would make the retained graph nondeterministic. Readers take a
+                // clone of the graph (`retained_dep_graph`) rather than holding the lock, so
+                // this never deadlocks against a reentrant `record`.
+                retained_graph.lock().push(index, *node, &edges);
             });
         }
 
@@ -874,10 +876,8 @@ impl GraphEncoder {
         GraphEncoder { status, retained_graph, profiler: sess.prof.clone() }
     }
 
-    pub(crate) fn with_retained_dep_graph(&self, f: impl Fn(&RetainedDepGraph)) {
-        if let Some(retained_graph) = &self.retained_graph {
-            f(&retained_graph.lock())
-        }
+    pub(crate) fn retained_dep_graph(&self) -> Option<RetainedDepGraph> {
+        self.retained_graph.as_ref().map(|retained_graph| retained_graph.lock().clone())
     }
 
     /// Encodes a node that does not exists in the previous graph.

tests/ui/dep-graph/dep-graph-assoc-type-codegen.rs

@@ -1,6 +1,6 @@
 // Test that when a trait impl changes, fns whose body uses that trait
 // must also be recompiled.
-//@ ignore-parallel-frontend dep graph
+
 //@ incremental
 //@ compile-flags: -Z query-dep-graph
 

tests/ui/dep-graph/dep-graph-caller-callee.rs

@@ -3,7 +3,7 @@
 
 //@ incremental
 //@ compile-flags: -Z query-dep-graph
-//@ ignore-parallel-frontend dep graph
+
 #![feature(rustc_attrs)]
 #![allow(dead_code)]
 

tests/ui/dep-graph/dep-graph-struct-signature.rs

@@ -1,6 +1,6 @@
 // Test cases where a changing struct appears in the signature of fns
 // and methods.
-//@ ignore-parallel-frontend dep graph
+
 //@ incremental
 //@ compile-flags: -Z query-dep-graph
 

tests/ui/dep-graph/dep-graph-trait-impl-two-traits-same-method.rs

@@ -1,6 +1,6 @@
 // Test that adding an impl to a trait `Foo` DOES affect functions
 // that only use `Bar` if they have methods in common.
-//@ ignore-parallel-frontend dep graph
+
 //@ incremental
 //@ compile-flags: -Z query-dep-graph
 

tests/ui/dep-graph/dep-graph-trait-impl.rs

@@ -1,6 +1,6 @@
 // Test that when a trait impl changes, fns whose body uses that trait
 // must also be recompiled.
-//@ ignore-parallel-frontend dep graph
+
 //@ incremental
 //@ compile-flags: -Z query-dep-graph