Auto merge of #151509 - Zoxc:gen-color-race, r=zetanumbers,petrochenkov

Handle race when coloring nodes concurrently as both green and red

This fixes a race where a duplicate dep node gets written to the dep graph if a node was marked as green and promoted during execution, then marked as red after execution.

This can occur when a `no_hash` query A depends on a query B which cannot be forced so it was not colored when starting execution of query A. During the execution of query A it will execute query B and color it green. Before A finishes another thread tries to mark A green, this time succeeding as B is now green, and A gets promoted and written to metadata. Execution of A then finishes and because it's `no_hash` we assume the result changed and thus we color the node again, now as red and write it to metadata again. This doesn't happen with non-`no_hash` queries as they will be green if all their dependencies are green.

This changes the code coloring nodes red to also use `compare_exchange` to deal with this race ensuring that the coloring of nodes only happens once.

Fixes #150018
Fixes #142778
Fixes #141540

Author: bors

compiler/rustc_query_system/src/dep_graph/graph.rs

@@ -788,17 +788,22 @@ impl<D: Deps> DepGraphData<D> {
         }
     }
 
-    fn promote_node_and_deps_to_current(&self, prev_index: SerializedDepNodeIndex) -> DepNodeIndex {
+    fn promote_node_and_deps_to_current(
+        &self,
+        prev_index: SerializedDepNodeIndex,
+    ) -> Option<DepNodeIndex> {
         self.current.debug_assert_not_in_new_nodes(&self.previous, prev_index);
 
         let dep_node_index = self.current.encoder.send_promoted(prev_index, &self.colors);
 
         #[cfg(debug_assertions)]
-        self.current.record_edge(
-            dep_node_index,
-            *self.previous.index_to_node(prev_index),
-            self.previous.fingerprint_by_index(prev_index),
-        );
+        if let Some(dep_node_index) = dep_node_index {
+            self.current.record_edge(
+                dep_node_index,
+                *self.previous.index_to_node(prev_index),
+                self.previous.fingerprint_by_index(prev_index),
+            );
+        }
 
         dep_node_index
     }
@@ -1018,8 +1023,10 @@ impl<D: Deps> DepGraphData<D> {
         // There may be multiple threads trying to mark the same dep node green concurrently
 
         // We allocating an entry for the node in the current dependency graph and
-        // adding all the appropriate edges imported from the previous graph
-        let dep_node_index = self.promote_node_and_deps_to_current(prev_dep_node_index);
+        // adding all the appropriate edges imported from the previous graph.
+        //
+        // `no_hash` nodes may fail this promotion due to already being conservatively colored red.
+        let dep_node_index = self.promote_node_and_deps_to_current(prev_dep_node_index)?;
 
         // ... and finally storing a "Green" entry in the color map.
         // Multiple threads can all write the same color here
@@ -1390,26 +1397,27 @@ impl DepNodeColorMap {
     }
 
     /// This tries to atomically mark a node green and assign `index` as the new
-    /// index. This returns `Ok` if `index` gets assigned, otherwise it returns
-    /// the already allocated index in `Err`.
-    #[inline]
-    pub(super) fn try_mark_green(
+    /// index if `green` is true, otherwise it will try to atomicaly mark it red.
+    ///
+    /// This returns `Ok` if `index` gets assigned or the node is marked red, otherwise it returns
+    /// the already allocated index in `Err` if it is green already. If it was already
+    /// red, `Err(None)` is returned.
+    #[inline(always)]
+    pub(super) fn try_mark(
         &self,
         prev_index: SerializedDepNodeIndex,
         index: DepNodeIndex,
-    ) -> Result<(), DepNodeIndex> {
+        green: bool,
+    ) -> Result<(), Option<DepNodeIndex>> {
         let value = &self.values[prev_index];
         match value.compare_exchange(
             COMPRESSED_UNKNOWN,
-            index.as_u32(),
+            if green { index.as_u32() } else { COMPRESSED_RED },
             Ordering::Relaxed,
             Ordering::Relaxed,
         ) {
             Ok(_) => Ok(()),
-            Err(v) => Err({
-                assert_ne!(v, COMPRESSED_RED, "tried to mark a red node as green");
-                DepNodeIndex::from_u32(v)
-            }),
+            Err(v) => Err(if v == COMPRESSED_RED { None } else { Some(DepNodeIndex::from_u32(v)) }),
         }
     }
 
@@ -1432,7 +1440,7 @@ impl DepNodeColorMap {
     pub(super) fn insert_red(&self, index: SerializedDepNodeIndex) {
         let value = self.values[index].swap(COMPRESSED_RED, Ordering::Release);
         // Sanity check for duplicate nodes
-        assert_eq!(value, COMPRESSED_UNKNOWN, "trying to encode a dep node twice");
+        assert_eq!(value, COMPRESSED_UNKNOWN, "tried to color an already colored node as red");
     }
 }
 

compiler/rustc_query_system/src/dep_graph/serialized.rs

@@ -898,15 +898,12 @@ impl<D: Deps> GraphEncoder<D> {
 
         let index = self.status.next_index(&mut *local);
 
-        if is_green {
-            // Use `try_mark_green` to avoid racing when `send_promoted` is called concurrently
-            // on the same index.
-            match colors.try_mark_green(prev_index, index) {
-                Ok(()) => (),
-                Err(dep_node_index) => return dep_node_index,
-            }
-        } else {
-            colors.insert_red(prev_index);
+        // Use `try_mark` to avoid racing when `send_promoted` is called concurrently
+        // on the same index.
+        match colors.try_mark(prev_index, index, is_green) {
+            Ok(()) => (),
+            Err(None) => panic!("dep node {:?} is unexpectedly red", prev_index),
+            Err(Some(dep_node_index)) => return dep_node_index,
         }
 
         self.status.bump_index(&mut *local);
@@ -918,21 +915,21 @@ impl<D: Deps> GraphEncoder<D> {
     /// from the previous dep graph and expects all edges to already have a new dep node index
     /// assigned.
     ///
-    /// This will also ensure the dep node is marked green.
+    /// This will also ensure the dep node is marked green if `Some` is returned.
     #[inline]
     pub(crate) fn send_promoted(
         &self,
         prev_index: SerializedDepNodeIndex,
         colors: &DepNodeColorMap,
-    ) -> DepNodeIndex {
+    ) -> Option<DepNodeIndex> {
         let _prof_timer = self.profiler.generic_activity("incr_comp_encode_dep_graph");
 
         let mut local = self.status.local.borrow_mut();
         let index = self.status.next_index(&mut *local);
 
         // Use `try_mark_green` to avoid racing when `send_promoted` or `send_and_color`
         // is called concurrently on the same index.
-        match colors.try_mark_green(prev_index, index) {
+        match colors.try_mark(prev_index, index, true) {
             Ok(()) => {
                 self.status.bump_index(&mut *local);
                 self.status.encode_promoted_node(
@@ -942,7 +939,7 @@ impl<D: Deps> GraphEncoder<D> {
                     colors,
                     &mut *local,
                 );
-                index
+                Some(index)
             }
             Err(dep_node_index) => dep_node_index,
         }