[mono][sgen] Make color visible to client permanent (#121247)

A color (SCC) that isn't containing any bridge objects is made visible
to client if xrefs_in * xrefs_out is greater than 60. Later on in bridge
processing, we need to build the callback to pass for .net android.
During this stage, we reduce from the full set of SCCs to SCCs that
should be visible to client (containing bridge objects or satisfying the
above condition). If an SCC has an xref to a color that is not visible
to client, we need to do a recursive traversal to find all neighbors
that are visible to client. The problem is that this process can end up
making an SCC no longer visible to client, leading to inconsistencies in
the computation. Consider a color(C1) that has a neighbor that is not
visible to client(C2). In this final stage, we compute the neighbors of
C1 by traversing recursively through the neighbors of C2. If C2 ends up
pointing to colors that were already neighbors of C1, then, following
this computation, C1 would end up with fewer xrefs_out, making the color
no longer visible to client. This make future checks incorrect,
resulting in building incorrect graph for client.

This scenario seems rare in practice, we should have gotten way more
reports otherwise. We fix this by pinning the visible_to_client property
for a color once it first satisfies it, so it will no longer matter how
many actual xrefs the color has.

Fixes assertions like:
```
* Assertion at /home/vbrezae/Xamarin/repos/runtime/src/mono/mono/metadata/sgen-tarjan-bridge.c:1151, condition `color_visible_to_client (cd)' not met
```

Author: BrzVlad

src/coreclr/gc/gcbridge.cpp

@@ -186,6 +186,11 @@ struct ColorData
     // Count of colors that list this color in their otherColors
     unsigned incomingColors : INCOMING_COLORS_BITS;
     unsigned visited : 1;
+    // ColorVisibleToClient for a ColorData* can change over the course of bridge processing which
+    // is problematic. We fix this by setting this flag when a color is detected as visible to client.
+    // Once the flag is set, the color is pinned to being visible to client, even though it could lose
+    // some xrefs, making it not satisfy the BridgelessColorIsHeavy condition.
+    unsigned visibleToClient : 1;
 };
 
 // Represents one managed object. Equivalent of new/old bridge "HashEntry"
@@ -233,7 +238,19 @@ static bool BridgelessColorIsHeavy(ColorData* data)
 // Should color be made visible to client?
 static bool ColorVisibleToClient(ColorData* data)
 {
-    return DynPtrArraySize(&data->bridges) || BridgelessColorIsHeavy(data);
+    if (data->visibleToClient)
+        return true;
+
+    if (DynPtrArraySize(&data->bridges) || BridgelessColorIsHeavy(data))
+    {
+        data->visibleToClient = true;
+        return true;
+    }
+    else
+    {
+        return false;
+    }
+
 }
 
 // Stacks of ScanData objects used for tarjan algorithm.
@@ -1186,9 +1203,9 @@ static MarkCrossReferencesArgs* BuildSccCallbackData()
         ColorData* cd;
         for (cd = &cur->data[0]; cd < cur->nextData; cd++)
         {
-            int bridges = DynPtrArraySize(&cd->bridges);
-            if (!(bridges || BridgelessColorIsHeavy(cd)))
+            if (!ColorVisibleToClient(cd))
                 continue;
+            int bridges = DynPtrArraySize(&cd->bridges);
 
             apiSccs[apiIndex].Count = bridges;
             uintptr_t *contexts = new (nothrow) uintptr_t[bridges];

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

@@ -96,6 +96,11 @@ typedef struct {
 	// Count of colors that list this color in their other_colors
 	unsigned incoming_colors : INCOMING_COLORS_BITS;
 	unsigned visited : 1;
+	// color_visible_to_client for a ColorData* can change over the course of bridge processing which
+	// is problematic. We fix this by setting this flag when a color is detected as visible to client.
+	// Once the flag is set, the color is pinned to being visible to client, even though it could lose
+	// some xrefs, making it not satisfy the bridgeless_color_is_heavy condition.
+	unsigned visible_to_client : 1;
 } ColorData;
 
 // Represents one managed object. Equivalent of new/old bridge "HashEntry"
@@ -140,8 +145,17 @@ bridgeless_color_is_heavy (ColorData *data) {
 
 // Should color be made visible to client?
 static gboolean
-color_visible_to_client (ColorData *data) {
-	return dyn_array_ptr_size (&data->bridges) || bridgeless_color_is_heavy (data);
+color_visible_to_client (ColorData *data)
+{
+	if (data->visible_to_client)
+		return TRUE;
+
+	if (dyn_array_ptr_size (&data->bridges) || bridgeless_color_is_heavy (data)) {
+		data->visible_to_client = TRUE;
+		return TRUE;
+	} else {
+		return FALSE;
+	}
 }
 
 // Stacks of ScanData objects used for tarjan algorithm.
@@ -1105,9 +1119,9 @@ processing_build_callback_data (int generation)
 	for (cur = root_color_bucket; cur; cur = cur->next) {
 		ColorData *cd;
 		for (cd = &cur->data [0]; cd < cur->next_data; ++cd) {
-			int bridges = dyn_array_ptr_size (&cd->bridges);
-			if (!(bridges || bridgeless_color_is_heavy (cd)))
+			if (!color_visible_to_client (cd))
 				continue;
+			int bridges = dyn_array_ptr_size (&cd->bridges);
 
 			api_sccs [api_index] = (MonoGCBridgeSCC *)sgen_alloc_internal_dynamic (sizeof (MonoGCBridgeSCC) + sizeof (MonoObject*) * bridges, INTERNAL_MEM_BRIDGE_DATA, TRUE);
 			api_sccs [api_index]->is_alive = FALSE;

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

@@ -89,6 +89,12 @@ public class Bridge1 : BridgeBase
     }
 }
 
+[InlineArray(14)]
+public struct InlineData
+{
+    public object obj;
+}
+
 // 128 size
 public class Bridge14 : BridgeBase
 {
@@ -107,7 +113,7 @@ public class NonBridge2 : NonBridge
 
 public class NonBridge14
 {
-    public object a,b,c,d,e,f,g,h,i,j,k,l,m,n;
+    public InlineData Data;
 }
 
 
@@ -403,6 +409,27 @@ static void SetupFragmentation<TBridge, TNonBridge>()
         }
     }
 
+    public static void BridgelessHeavyColorChanging()
+    {
+        Bridge1[] left = new Bridge1[8];
+        for (int i = 0; i < 8; i++)
+            left[i] = new Bridge1();
+        Bridge[] right = new Bridge[7];
+        for (int i = 0; i < 7; i++)
+            right[i] = new Bridge();
+        NonBridge2 right7 = new NonBridge2();
+
+        NonBridge14 mid = new NonBridge14();
+
+        for (int i = 0; i < 8; i++)
+            left[i].Link = mid;
+        for (int i = 0; i < 7; i++)
+            mid.Data[i] = right[i];
+        mid.Data[7] = right7;
+        right7.Link = right[6];
+        right7.Link2 = right[5];
+    }
+
     public static int Main(string[] args)
     {
         TestLinkedList();
@@ -417,6 +444,7 @@ public static int Main(string[] args)
         RunGraphTest(NestedCycles);
         RunGraphTest(FauxHeavyNodeWithCycles);
         RunGraphTest(Spider);
+        RunGraphTest(BridgelessHeavyColorChanging);
         return 100;
     }
 }