From 47dcf3f24b48c075ed0f754d1d830d6b22b46d46 Mon Sep 17 00:00:00 2001 From: lateralusX Date: Mon, 31 Jul 2023 17:49:11 +0200 Subject: [PATCH 1/4] Fix rare deadlock using interp full AOT fallback. GC thread doing gcdump will dump the EventPipe events after world has restarted but before releasing GC lock. There was one case where we logged a bulk type during that face where a type didn't have its finalizer data initialized and at the same time main thread running interpreter held loader lock and tried to acquire GC lock, that triggers a deadlock since the GC thread (still holding the GC lock) would trigger logic to initialize the finalizer data, but that in turn requires the GC lock. Fix delays the fire of GC dump events until after we completed GC. All GC dump events have been cached into a temp file and will be written into EventPipe, the only potential issue with this is that we keep vtable pointers in cache that will be resolved when emitting EventPipe event, after releasing GC lock, but since we currently won't unload vtables, that is not an issue, but needs to be addressed if/when we implement ability to unload vtables. We would then need to root the vtables while stored in temporary cache. --- src/mono/mono/eventpipe/ep-rt-mono-runtime-provider.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/mono/mono/eventpipe/ep-rt-mono-runtime-provider.c b/src/mono/mono/eventpipe/ep-rt-mono-runtime-provider.c index 151dafced3e8e..0324cb17381e7 100644 --- a/src/mono/mono/eventpipe/ep-rt-mono-runtime-provider.c +++ b/src/mono/mono/eventpipe/ep-rt-mono-runtime-provider.c @@ -4165,6 +4165,9 @@ calculate_live_keywords ( ep_requires_lock_held (); } +// TODO: If/when we can unload vtables, we would need to temporary +// root the vtable pointers currently stored in buffered gc events. +// Once all events are fired, we can remove root from GC. static void gc_event_callback ( @@ -4199,7 +4202,6 @@ gc_event_callback ( if (is_gc_heap_dump_enabled (context)) { EP_ASSERT (context->state == GC_HEAP_DUMP_CONTEXT_STATE_DUMP); gc_heap_dump_context_build_roots (context); - fire_buffered_gc_events (context); } break; } @@ -4289,6 +4291,7 @@ gc_heap_dump_trigger_callback (MonoProfiler *prof) mono_profiler_set_gc_event_callback (_ep_rt_mono_default_profiler_provider, gc_event_callback); mono_gc_collect (mono_gc_max_generation ()); mono_profiler_set_gc_event_callback (_ep_rt_mono_default_profiler_provider, NULL); + fire_buffered_gc_events (heap_dump_context); } heap_dump_context->state = GC_HEAP_DUMP_CONTEXT_STATE_END; From c53bd9a1cbb5dcfa7db02b2f5734fa71480b7b46 Mon Sep 17 00:00:00 2001 From: lateralusX Date: Tue, 1 Aug 2023 13:00:26 +0200 Subject: [PATCH 2/4] Enable GC dump test on Mono. --- src/tests/issues.targets | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/tests/issues.targets b/src/tests/issues.targets index 53e2109bec919..da0d4fd320234 100644 --- a/src/tests/issues.targets +++ b/src/tests/issues.targets @@ -1895,9 +1895,6 @@ needs triage - - needs triage - needs triage From 3f98ac7e3679de47b852391b965802d9cfbcab1d Mon Sep 17 00:00:00 2001 From: lateralusX Date: Tue, 1 Aug 2023 16:57:18 +0200 Subject: [PATCH 3/4] Exclude gcdump on WASM. --- src/tests/issues.targets | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/tests/issues.targets b/src/tests/issues.targets index da0d4fd320234..d1b48dbc3c484 100644 --- a/src/tests/issues.targets +++ b/src/tests/issues.targets @@ -3520,6 +3520,9 @@ System.Diagnostics.Process is not supported on wasm + + System.Diagnostics.Process is not supported on wasm + From a452a5b527ac3f7acb6d142fc3d0ecdcf2a684c6 Mon Sep 17 00:00:00 2001 From: lateralusX Date: Tue, 1 Aug 2023 16:58:09 +0200 Subject: [PATCH 4/4] Make sure GC dump waits on GCStop before closing session. --- src/tests/tracing/eventpipe/gcdump/gcdump.cs | 23 ++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/src/tests/tracing/eventpipe/gcdump/gcdump.cs b/src/tests/tracing/eventpipe/gcdump/gcdump.cs index 346cfca11242e..7819d9e93ecf2 100644 --- a/src/tests/tracing/eventpipe/gcdump/gcdump.cs +++ b/src/tests/tracing/eventpipe/gcdump/gcdump.cs @@ -20,6 +20,8 @@ namespace Tracing.Tests.EventSourceError // Regression test for https://github.com/dotnet/runtime/issues/38639 public class GCDumpTest { + private static bool _seenGCStart = false; + private static bool _seenGCStop = false; private static int _bulkTypeCount = 0; private static int _bulkNodeCount = 0; private static int _bulkEdgeCount = 0; @@ -27,6 +29,7 @@ public class GCDumpTest private static int _bulkRootStaticVarCount = 0; private static readonly ulong GC_HeapDump_Keyword = 0x100000UL; + private static ManualResetEvent _gcStopReceived = new ManualResetEvent(false); public static int Main() { @@ -48,11 +51,17 @@ public static int Main() private static Action _eventGeneratingAction = () => { - // This space intentionally left blank + // Wait up to 10 seconds to receive GCStop event. + _gcStopReceived.WaitOne(10000); }; private static Func> _DoesRundownContainMethodEvents = (source) => { + source.Clr.GCStart += (GCStartTraceData data) => + { + _seenGCStart = true; + }; + source.Clr.TypeBulkType += (GCBulkTypeTraceData data) => { _bulkTypeCount += data.Count; @@ -78,13 +87,21 @@ public static int Main() _bulkRootStaticVarCount += data.Count; }; + source.Clr.GCStop += (GCEndTraceData data) => + { + _seenGCStop = true; + _gcStopReceived.Set(); + }; + return () => { // Hopefully it is low enough to be resilient to changes in the runtime // and high enough to catch issues. There should be between hundreds and thousands // for each, but the number is variable and the point of the test is to verify // that we get any events at all. - if (_bulkTypeCount > 50 + if (_seenGCStart + && _seenGCStop + && _bulkTypeCount > 50 && _bulkNodeCount > 50 && _bulkEdgeCount > 50 && _bulkRootEdgeCount > 50 @@ -95,6 +112,8 @@ public static int Main() Console.WriteLine($"Test failed due to missing GC heap events."); + Console.WriteLine($"_seenGCStart = {_seenGCStart}"); + Console.WriteLine($"_seenGCStop = {_seenGCStop}"); Console.WriteLine($"_bulkTypeCount = {_bulkTypeCount}"); Console.WriteLine($"_bulkNodeCount = {_bulkNodeCount}"); Console.WriteLine($"_bulkEdgeCount = {_bulkEdgeCount}");