[release/9.0] Fix edge cases in Tarjan GC bridge (Android) (#114391)

* Fix dump_processor_state debug code to compile and work on Android (#112970)

Fix typo in GC bridge comparison message (SCCS -> XREFS)

* [mono] Add a few bridge tests (#113703)

* [mono][sgen] Fix DUMP_GRAPH debug option build for tarjan bridge

* [mono][sgen] Don't create ScanData* during debug dumping of SCCs

It serves no purpose and it would later crash the runtime since we didn't patch the lockword back in place.

* [mono][sgen] Fix some null deref crashes in DUMP_GRAPH debug option

* [mono][tests] Add bridge tests

These are ported from some of the bridge tests we had on mono/mono. In order to test them we compare between the output of the new and the tarjan bridge.

* Fix an edge case in the Tarjan GC bridge that leads to losing xref information (#112825)

* Fix an edge case in the Tarjan SCC that lead to losing xref information

In the Tarjan SCC bridge processing there's a color graph used to find out
connections between SCCs. There was a rare case which only manifested when
a cycle in the object graph points to another cycle that points to a bridge
object. We only recognized direct bridge pointers but not pointers to other
non-bridge SCCs that in turn point to bridges and where we already calculated
the xrefs. These xrefs were then lost.

* Add test case to sgen-bridge-pathologies and add an assert to catch the original bug

* Add review

---------

Co-authored-by: Vlad Brezae <[email protected]>

* [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)

* Disable test on wasm

---------

Co-authored-by: Vlad Brezae <[email protected]>
Co-authored-by: Alexander Köplinger <[email protected]>

Author: 3 people

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

@@ -316,24 +316,24 @@ dump_processor_state (SgenBridgeProcessor *p)
 {
 	int i;
 
-	printf ("------\n");
-	printf ("SCCS %d\n", p->num_sccs);
+	g_message ("------\n");
+	g_message ("SCCS %d\n", p->num_sccs);
 	for (i = 0; i < p->num_sccs; ++i) {
 		int j;
 		MonoGCBridgeSCC *scc = p->api_sccs [i];
-		printf ("\tSCC %d:", i);
+		g_message ("\tSCC %d:", i);
 		for (j = 0; j < scc->num_objs; ++j) {
 			MonoObject *obj = scc->objs [j];
-			printf (" %p(%s)", obj, SGEN_LOAD_VTABLE (obj)->klass->name);
+			g_message (" %p(%s)", obj, m_class_get_name (SGEN_LOAD_VTABLE (obj)->klass));
 		}
-		printf ("\n");
+		g_message ("\n");
 	}
 
-	printf ("XREFS %d\n", p->num_xrefs);
+	g_message ("XREFS %d\n", p->num_xrefs);
 	for (i = 0; i < p->num_xrefs; ++i)
-		printf ("\t%d -> %d\n", p->api_xrefs [i].src_scc_index, p->api_xrefs [i].dst_scc_index);
+		g_message ("\t%d -> %d\n", p->api_xrefs [i].src_scc_index, p->api_xrefs [i].dst_scc_index);
 
-	printf ("-------\n");
+	g_message ("-------\n");
 }
 */
 
@@ -352,7 +352,7 @@ sgen_compare_bridge_processor_results (SgenBridgeProcessor *a, SgenBridgeProcess
 	if (a->num_sccs != b->num_sccs)
 		g_error ("SCCS count expected %d but got %d", a->num_sccs, b->num_sccs);
 	if (a->num_xrefs != b->num_xrefs)
-		g_error ("SCCS count expected %d but got %d", a->num_xrefs, b->num_xrefs);
+		g_error ("XREFS count expected %d but got %d", a->num_xrefs, b->num_xrefs);
 
 	/*
 	 * First we build a hash of each object in `a` to its respective SCC index within

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

@@ -400,16 +400,7 @@ static const char*
 safe_name_bridge (GCObject *obj)
 {
 	GCVTable vt = SGEN_LOAD_VTABLE (obj);
-	return vt->klass->name;
-}
-
-static ScanData*
-find_or_create_data (GCObject *obj)
-{
-	ScanData *entry = find_data (obj);
-	if (!entry)
-		entry = create_data (obj);
-	return entry;
+	return m_class_get_name (vt->klass);
 }
 #endif
 
@@ -566,10 +557,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);
@@ -593,7 +589,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)
@@ -700,11 +696,11 @@ compute_low_index (ScanData *data, GCObject *obj)
 	obj = bridge_object_forward (obj);
 	other = find_data (obj);
 
-#if DUMP_GRAPH
-	printf ("\tcompute low %p ->%p (%s) %p (%d / %d, color %p)\n", data->obj, obj, safe_name_bridge (obj), other, other ? other->index : -2, other ? other->low_index : -2, other->color);
-#endif
 	if (!other)
 		return;
+#if DUMP_GRAPH
+	printf ("\tcompute low %p ->%p (%s) %p (%d / %d, color %p)\n", data->obj, obj, safe_name_bridge (obj), other, other ? other->index : -2, other->low_index, other->color);
+#endif
 
 	g_assert (other->state != INITIAL);
 
@@ -777,24 +773,32 @@ create_scc (ScanData *data)
 	gboolean found = FALSE;
 	gboolean found_bridge = FALSE;
 	ColorData *color_data = NULL;
+	gboolean can_reduce_color = TRUE;
 
 	for (i = dyn_array_ptr_size (&loop_stack) - 1; i >= 0; --i) {
 		ScanData *other = (ScanData *)dyn_array_ptr_get (&loop_stack, i);
 		found_bridge |= other->is_bridge;
+		if (dyn_array_ptr_size (&other->xrefs) > 0) {
+			// This scc will have more xrefs than the ones from the color_merge_array,
+			// we will need to create a new color to store this information.
+			can_reduce_color = FALSE;
+		}
 		if (found_bridge || other == data)
 			break;
 	}
 
 	if (found_bridge) {
 		color_data = new_color (TRUE);
 		++num_colors_with_bridges;
-	} else {
+	} else if (can_reduce_color) {
 		color_data = reduce_color ();
+	} else {
+		color_data = new_color (FALSE);
 	}
 #if DUMP_GRAPH
 	printf ("|SCC %p rooted in %s (%p) has bridge %d\n", color_data, safe_name_bridge (data->obj), data->obj, found_bridge);
 	printf ("\tloop stack: ");
-	for (int i = 0; i < dyn_array_ptr_size (&loop_stack); ++i) {
+	for (i = 0; i < dyn_array_ptr_size (&loop_stack); ++i) {
 		ScanData *other = dyn_array_ptr_get (&loop_stack, i);
 		printf ("(%d/%d)", other->index, other->low_index);
 	}
@@ -824,10 +828,19 @@ 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);
+		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);
 
 		if (other == data) {
@@ -837,11 +850,22 @@ 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
-	printf ("\tpoints-to-colors: ");
-	for (int i = 0; i < dyn_array_ptr_size (&color_data->other_colors); i++)
-		printf ("%p ", dyn_array_ptr_get (&color_data->other_colors, i));
-	printf ("\n");
+    if (color_data) {
+        printf ("\tpoints-to-colors: ");
+        for (i = 0; i < dyn_array_ptr_size (&color_data->other_colors); i++)
+            printf ("%p ", dyn_array_ptr_get (&color_data->other_colors, i));
+        printf ("\n");
+    }
 #endif
 }
 
@@ -966,8 +990,11 @@ dump_color_table (const char *why, gboolean do_index)
 				printf (" bridges: ");
 				for (j = 0; j < dyn_array_ptr_size (&cd->bridges); ++j) {
 					GCObject *obj = dyn_array_ptr_get (&cd->bridges, j);
-					ScanData *data = find_or_create_data (obj);
-					printf ("%d ", data->index);
+					ScanData *data = find_data (obj);
+					if (!data)
+						printf ("%p ", obj);
+					else
+						printf ("%p(%d) ", obj, data->index);
 				}
 			}
 			printf ("\n");
@@ -1027,7 +1054,7 @@ processing_stw_step (void)
 #if defined (DUMP_GRAPH)
 	printf ("----summary----\n");
 	printf ("bridges:\n");
-	for (int i = 0; i < bridge_count; ++i) {
+	for (i = 0; i < bridge_count; ++i) {
 		ScanData *sd = find_data (dyn_array_ptr_get (&registered_bridges, i));
 		printf ("\t%s (%p) index %d color %p\n", safe_name_bridge (sd->obj), sd->obj, sd->index, sd->color);
 	}
@@ -1141,6 +1168,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);
 		}
 	}