From 058b1357bcd11fdb5e0e084a845ae47af74a92b3 Mon Sep 17 00:00:00 2001 From: Filip Navara Date: Sun, 23 Feb 2025 11:31:55 +0100 Subject: [PATCH 1/3] 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. --- src/mono/mono/metadata/sgen-tarjan-bridge.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/mono/mono/metadata/sgen-tarjan-bridge.c b/src/mono/mono/metadata/sgen-tarjan-bridge.c index 80451627fc5321..1ca588d21cc87e 100644 --- a/src/mono/mono/metadata/sgen-tarjan-bridge.c +++ b/src/mono/mono/metadata/sgen-tarjan-bridge.c @@ -739,7 +739,7 @@ create_scc (ScanData *data) 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; + found_bridge |= other->is_bridge || dyn_array_ptr_size (&other->xrefs) > 0; if (found_bridge || other == data) break; } From e60945ae0f9a0cce97737d86c9f80d91e1fa832d Mon Sep 17 00:00:00 2001 From: Filip Navara Date: Mon, 3 Mar 2025 11:32:58 +0100 Subject: [PATCH 2/3] Add test case to sgen-bridge-pathologies and add an assert to catch the original bug --- src/mono/mono/metadata/sgen-tarjan-bridge.c | 5 ++- .../mono/tests/sgen-bridge-pathologies.cs | 32 +++++++++++++++++++ 2 files changed, 36 insertions(+), 1 deletion(-) diff --git a/src/mono/mono/metadata/sgen-tarjan-bridge.c b/src/mono/mono/metadata/sgen-tarjan-bridge.c index 1ca588d21cc87e..de0bbd90812a2d 100644 --- a/src/mono/mono/metadata/sgen-tarjan-bridge.c +++ b/src/mono/mono/metadata/sgen-tarjan-bridge.c @@ -785,8 +785,11 @@ create_scc (ScanData *data) // 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) + if (color_data) { add_other_colors (color_data, &other->xrefs); + } else { + g_assert (dyn_array_ptr_size (&other->xrefs) == 0); + } dyn_array_ptr_uninit (&other->xrefs); if (other == data) { diff --git a/src/mono/mono/tests/sgen-bridge-pathologies.cs b/src/mono/mono/tests/sgen-bridge-pathologies.cs index 7c8dbc08731167..bebf78cbcce894 100644 --- a/src/mono/mono/tests/sgen-bridge-pathologies.cs +++ b/src/mono/mono/tests/sgen-bridge-pathologies.cs @@ -18,6 +18,11 @@ public class NonBridge public object Link; } +public class NonBridge2 : NonBridge +{ + public object Link2; +} + class Driver { const int OBJ_COUNT = 200 * 1000; const int LINK_COUNT = 2; @@ -207,6 +212,32 @@ static void Spider () { c.Links.Add (last_level); } + /* + * Simulates a graph with two nested cycles that is produces by + * the async state machine when `async Task M()` method gets its + * continuation rooted by an Action held by RunnableImplementor + * (ie. the task continuation is hooked through the SynchronizationContext + * implentation and rooted only by Android bridge objects). + */ + static void NestedCycles () + { + Bridge runnableImplementor = new Bridge (); + Bridge byteArrayOutputStream = new Bridge (); + NonBridge2 action = new NonBridge2 (); + NonBridge displayClass = new NonBridge (); + NonBridge2 asyncStateMachineBox = new NonBridge2 (); + NonBridge2 asyncStreamWriter = new NonBridge2 (); + + runnableImplementor.Links.Add(action); + action.Link = displayClass; + action.Link2 = asyncStateMachineBox; + displayClass.Link = action; + asyncStateMachineBox.Link = asyncStreamWriter; + asyncStateMachineBox.Link2 = action; + asyncStreamWriter.Link = byteArrayOutputStream; + asyncStreamWriter.Link2 = asyncStateMachineBox; + } + static void RunTest (ThreadStart setup) { var t = new Thread (setup); @@ -231,6 +262,7 @@ static int Main () RunTest (SetupDeadList); RunTest (SetupSelfLinks); RunTest (Spider); + RunTest (NestedCycles); for (int i = 0; i < 0; ++i) { GC.Collect (); From dc9b877ba5601d95473275ae82e52066e4db25aa Mon Sep 17 00:00:00 2001 From: Vlad Brezae Date: Mon, 24 Mar 2025 17:06:41 +0200 Subject: [PATCH 3/3] Add review --- src/mono/mono/metadata/sgen-tarjan-bridge.c | 12 +++++-- .../mono/tests/sgen-bridge-pathologies.cs | 32 ------------------- src/tests/GC/Features/Bridge/Bridge.cs | 2 +- 3 files changed, 11 insertions(+), 35 deletions(-) diff --git a/src/mono/mono/metadata/sgen-tarjan-bridge.c b/src/mono/mono/metadata/sgen-tarjan-bridge.c index de0bbd90812a2d..e4d43d24d89caa 100644 --- a/src/mono/mono/metadata/sgen-tarjan-bridge.c +++ b/src/mono/mono/metadata/sgen-tarjan-bridge.c @@ -736,10 +736,16 @@ 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 || dyn_array_ptr_size (&other->xrefs) > 0; + 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; } @@ -747,8 +753,10 @@ create_scc (ScanData *data) 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); diff --git a/src/mono/mono/tests/sgen-bridge-pathologies.cs b/src/mono/mono/tests/sgen-bridge-pathologies.cs index bebf78cbcce894..7c8dbc08731167 100644 --- a/src/mono/mono/tests/sgen-bridge-pathologies.cs +++ b/src/mono/mono/tests/sgen-bridge-pathologies.cs @@ -18,11 +18,6 @@ public class NonBridge public object Link; } -public class NonBridge2 : NonBridge -{ - public object Link2; -} - class Driver { const int OBJ_COUNT = 200 * 1000; const int LINK_COUNT = 2; @@ -212,32 +207,6 @@ static void Spider () { c.Links.Add (last_level); } - /* - * Simulates a graph with two nested cycles that is produces by - * the async state machine when `async Task M()` method gets its - * continuation rooted by an Action held by RunnableImplementor - * (ie. the task continuation is hooked through the SynchronizationContext - * implentation and rooted only by Android bridge objects). - */ - static void NestedCycles () - { - Bridge runnableImplementor = new Bridge (); - Bridge byteArrayOutputStream = new Bridge (); - NonBridge2 action = new NonBridge2 (); - NonBridge displayClass = new NonBridge (); - NonBridge2 asyncStateMachineBox = new NonBridge2 (); - NonBridge2 asyncStreamWriter = new NonBridge2 (); - - runnableImplementor.Links.Add(action); - action.Link = displayClass; - action.Link2 = asyncStateMachineBox; - displayClass.Link = action; - asyncStateMachineBox.Link = asyncStreamWriter; - asyncStateMachineBox.Link2 = action; - asyncStreamWriter.Link = byteArrayOutputStream; - asyncStreamWriter.Link2 = asyncStateMachineBox; - } - static void RunTest (ThreadStart setup) { var t = new Thread (setup); @@ -262,7 +231,6 @@ static int Main () RunTest (SetupDeadList); RunTest (SetupSelfLinks); RunTest (Spider); - RunTest (NestedCycles); for (int i = 0; i < 0; ++i) { GC.Collect (); diff --git a/src/tests/GC/Features/Bridge/Bridge.cs b/src/tests/GC/Features/Bridge/Bridge.cs index f667dc5ceac63d..58812b4755710e 100644 --- a/src/tests/GC/Features/Bridge/Bridge.cs +++ b/src/tests/GC/Features/Bridge/Bridge.cs @@ -385,7 +385,7 @@ public static int Main(string[] args) RunGraphTest(SetupDeadList); RunGraphTest(SetupSelfLinks); -// RunGraphTest(NestedCycles); // Fixed by Filip + RunGraphTest(NestedCycles); // RunGraphTest(Spider); // Crashes return 100; }