[SGen/Tarjan] Handle edge case with node heaviness changing due to deduplication (#113044)

* [SGen/Tarjan] Handle edge case with node heaviness changing due to deduplication

Do early deduplication

Fix Windows build

Add test cases to sgen-bridge-pathologies

* Move test code

* Remove old code

* Add extra check (no change to functionality)

Author: filipnavara

src/mono/mono/metadata/sgen-tarjan-bridge.c

@@ -525,10 +525,15 @@ find_in_cache (int *insert_index)
 
 // Populate other_colors for a give color (other_colors represent the xrefs for this color)
 static void
-add_other_colors (ColorData *color, DynPtrArray *other_colors)
+add_other_colors (ColorData *color, DynPtrArray *other_colors, gboolean check_visited)
 {
 	for (int i = 0; i < dyn_array_ptr_size (other_colors); ++i) {
 		ColorData *points_to = (ColorData *)dyn_array_ptr_get (other_colors, i);
+		if (check_visited) {
+			if (points_to->visited)
+				continue;
+			points_to->visited = TRUE;
+		}
 		dyn_array_ptr_add (&color->other_colors, points_to);
 		// Inform targets
 		points_to->incoming_colors = MIN (points_to->incoming_colors + 1, INCOMING_COLORS_MAX);
@@ -552,7 +557,7 @@ new_color (gboolean has_bridges)
 	cd = alloc_color_data ();
 	cd->api_index = -1;
 
-	add_other_colors (cd, &color_merge_array);
+	add_other_colors (cd, &color_merge_array, FALSE);
 
 	/* if cacheSlot >= 0, it means we prepared a given slot to receive the new color */
 	if (cacheSlot >= 0)
@@ -791,12 +796,18 @@ create_scc (ScanData *data)
 			dyn_array_ptr_add (&color_data->bridges, other->obj);
 		}
 
-		// Maybe we should make sure we are not adding duplicates here. It is not really a problem
-		// since we will get rid of duplicates before submitting the SCCs to the client in gather_xrefs
-		if (color_data) {
-			add_other_colors (color_data, &other->xrefs);
-		} else {
-			g_assert (dyn_array_ptr_size (&other->xrefs) == 0);
+		if (dyn_array_ptr_size (&other->xrefs) > 0) {
+			g_assert (color_data != NULL);
+			g_assert (can_reduce_color == FALSE);
+			// We need to eliminate duplicates early otherwise the heaviness property
+			// can change in gather_xrefs and it breaks down the loop that reports the
+			// xrefs to the client.
+			//
+			// We reuse the visited flag to mark the objects that are already part of
+			// the color_data array. The array was created above with the new_color call
+			// and xrefs were populated from color_merge_array, which is already
+			// deduplicated and every entry is marked as visited.
+			add_other_colors (color_data, &other->xrefs, TRUE);
 		}
 		dyn_array_ptr_uninit (&other->xrefs);
 
@@ -807,6 +818,15 @@ create_scc (ScanData *data)
 	}
 	g_assert (found);
 
+	// Clear the visited flag on nodes that were added with add_other_colors in the loop above
+	if (!can_reduce_color) {
+		for (i = dyn_array_ptr_size (&color_merge_array); i < dyn_array_ptr_size (&color_data->other_colors); i++) {
+			ColorData *cd = (ColorData *)dyn_array_ptr_get (&color_data->other_colors, i);
+			g_assert (cd->visited);
+			cd->visited = FALSE;
+		}
+	}
+
 #if DUMP_GRAPH
     if (color_data) {
         printf ("\tpoints-to-colors: ");
@@ -1116,6 +1136,7 @@ processing_build_callback_data (int generation)
 			gather_xrefs (cd);
 			reset_xrefs (cd);
 			dyn_array_ptr_set_all (&cd->other_colors, &color_merge_array);
+			g_assert (color_visible_to_client (cd));
 			xref_count += dyn_array_ptr_size (&cd->other_colors);
 		}
 	}

src/tests/GC/Features/Bridge/Bridge.cs

@@ -279,6 +279,35 @@ static void NestedCycles()
         asyncStreamWriter.Link2 = asyncStateMachineBox;
     }
 
+	// Simulates a graph where a heavy node has its fanout components
+	// represented by cycles with back-references to the heavy node and
+	// references to the same bridge objects.
+	// This enters a pathological path in the SCC contraction where the
+	// links to the bridge objects need to be correctly deduplicated. The
+	// deduplication causes the heavy node to no longer be heavy.
+	static void FauxHeavyNodeWithCycles()
+	{
+		Bridge fanout = new Bridge();
+
+		// Need enough edges for the node to be considered heavy by bridgeless_color_is_heavy
+		NonBridge[] fauxHeavyNode = new NonBridge[100];
+		for (int i = 0; i < fauxHeavyNode.Length; i++)
+        {
+			NonBridge2 cycle = new NonBridge2();
+			cycle.Link = fanout;
+			cycle.Link2 = fauxHeavyNode;
+			fauxHeavyNode[i] = cycle;
+		}
+
+		// Need at least HEAVY_REFS_MIN + 1 fan-in nodes
+		Bridge[] faninNodes = new Bridge[3];
+		for (int i = 0; i < faninNodes.Length; i++)
+        {
+			faninNodes[i] = new Bridge();
+			faninNodes[i].Links.Add(fauxHeavyNode);
+		}
+	}
+
     static void RunGraphTest(Action test)
     {
         Console.WriteLine("Start test {0}", test.Method.Name);
@@ -386,6 +415,7 @@ public static int Main(string[] args)
         RunGraphTest(SetupDeadList);
         RunGraphTest(SetupSelfLinks);
         RunGraphTest(NestedCycles);
+        RunGraphTest(FauxHeavyNodeWithCycles);
 //        RunGraphTest(Spider); // Crashes
         return 100;
     }