DiagnosticsServer ENABLE_PERFMAP command not resilient executing early-in-startup

[hoyosjs]

Description

#122430 found the following assert:

[tracee] ASSERT FAILED
[tracee] 	Expression: IsCrstInitialized()
[tracee] 	Location:   line 221 in /__w/1/s/src/coreclr/vm/crst.cpp
[tracee] 	Function:   Enter
[tracee] 	Process:    145042

at this stack:

 # Child-SP          RetAddr               Call Site
00 0000f912`62becf30 0000f912`a3d2728c     libc_so!__wait4+0x74 [/usr/src/debug/glibc-2.38-15.azl3.aarch64/posix/../sysdeps/unix/sysv/linux/wait4.c @ 30] 
01 0000f912`62becf60 0000f912`a3d284f4     libcoreclr!PROCCreateCrashDump+0x514 [/__w/1/s/src/coreclr/pal/src/thread/process.cpp @ 2564] 
02 0000f912`62becfe0 0000f912`a3d252e0     libcoreclr!PROCCreateCrashDumpIfEnabled+0xae8 [/__w/1/s/src/coreclr/pal/src/thread/process.cpp @ 2808] 
03 0000f912`62bed080 0000f912`a3cee458 (T) libcoreclr!PROCAbort+0x38 [/__w/1/s/src/coreclr/pal/src/thread/process.cpp @ 2843] 
04 (Inline Function) --------`--------     libcoreclr!invoke_previous_action+0xdc [/__w/1/s/src/coreclr/pal/src/exception/signal.cpp @ 447] 
05 0000f912`62bed0b0 0000f912`a445b7a0     libcoreclr!sigsegv_handler [/__w/1/s/src/coreclr/pal/src/exception/signal.cpp @ 642] 
06 0000f912`62bed0f0 0000f912`a3d2f150 (T) linux_vdso_so!_kernel_rt_sigreturn
07 0000f912`62bee350 0000f912`a3ceb98c     libcoreclr!CONTEXT_GetSveLengthFromOS+0x4 [/__w/1/s/src/coreclr/pal/src/arch/arm64/context2.S @ 305] 
08 0000f912`62bee350 0000f912`a38eb188     libcoreclr!DebugBreak+0x2e4 [/__w/1/s/src/coreclr/pal/src/debug/debug.cpp @ 414] 
09 0000f912`62bee430 0000f912`a39c3834     libcoreclr!CrstBase::Enter+0x1a0 [/__w/1/s/src/coreclr/vm/crst.cpp @ 229] 
0a (Inline Function) --------`--------     libcoreclr!CrstBase::CrstHolder::CrstHolder+0x10 [/__w/1/s/src/coreclr/vm/crst.h @ 349] 
0b 0000f912`62bee470 0000f912`a3700744     libcoreclr!PerfMap::Enable+0x48 [/__w/1/s/src/coreclr/vm/perfmap.cpp @ 93] 
0c (Inline Function) --------`--------     libcoreclr!ds_rt_enable_perfmap+0x18 [/__w/1/s/src/coreclr/vm/eventing/eventpipe/ds-rt-coreclr.h @ 317] 
0d (Inline Function) --------`--------     libcoreclr!process_protocol_helper_enable_perfmap+0x80 [/__w/1/s/src/native/eventpipe/ds-portable-rid.c @ 1072] 
0e 0000f912`62bee640 0000f912`a3705678     libcoreclr!ds_process_protocol_helper_handle_ipc_message+0x31c [/__w/1/s/src/native/eventpipe/ds-portable-rid.c @ 1245] 
0f (Inline Function) --------`--------     libcoreclr!server_loop_tick+0x198 [/__w/1/s/src/native/eventpipe/ds-server.c @ 158] 
10 0000f912`62bee7e0 0000f912`a3d2b144     libcoreclr!server_thread+0x270 [/__w/1/s/src/native/eventpipe/ds-protocol.c @ 185] 
11 0000f912`62bee860 0000f912`a3eb0944     libcoreclr!CorUnix::CPalThread::ThreadEntry+0x350 [/__w/1/s/src/coreclr/pal/src/thread/thread.cpp @ 1601] 
12 0000f912`62bee920 0000f912`a3f16ccc     libc_so!start_thread+0x2a4 [/usr/src/debug/glibc-2.38-15.azl3.aarch64/nptl/pthread_create.c @ 444] 
13 0000f912`62beea40 ffffffff`ffffffff     libc_so!thread_start+0xc [/usr/src/debug/glibc-2.38-15.azl3.aarch64/misc/../sysdeps/unix/sysv/linux/aarch64/clone3.S @ 79] 
14 0000f912`62beea40 00000000`00000000     0xffffffff`ffffffff

while the main thread hasn't properly set up the Crst used for perfmaps.

Reproduction Steps

send a command early on

Expected behavior

either fail command or delay response

Actual behavior

assert hit in checked - unknown behavior at runtime.

Regression?

No response

Known Workarounds

No response

Configuration

No response

Other information

No response

[lateralusX]

PerfMap::Initialize,

runtime/src/coreclr/vm/ceemain.cpp

Line 733 in 13ee790

PerfMap::Initialize();

, happens after diagnostic server startup,

runtime/src/coreclr/vm/ceemain.cpp

Line 709 in 13ee790

DiagnosticServerAdapter::Initialize();

.

Either perfmap needs to be initialized before diagnostic server starts up, or we need to add an initialization check in PerfMap::Enable (adding a bool return code) meaning that the perfmap enable IPC command can fail, or do an automatic initialize of perfmap on first call to enable.

If we make PerfMap::Initialize reentrant we mainly need to make sure the init of the Crst is only done once before first use, we could use a dynamic allocated Crst and CAS operation similar as done in Module::InitializeDynamicILCrst or InitializeLogging. If we need to have the IPC command wait for runtime to reach passed PerfMap::Initialize then we would need a manual reset event and a initialize bool that will hold any enable call until PerMap has been fully initialized.

Any commands exposed through diagnostic server needs to make sure they are fully operational once diagnostic server is up and running. If command needs initialization logic it needs to happen as part of the command or before diagnostic server have started in EEStartupHelper

[mdh1418]

Given that PerfMap::Initialize() invokes PerfMap::Enable(perfMapType, sendExisting: false)

runtime/src/coreclr/vm/perfmap.cpp

Line 52 in 283e216

PerfMap::Enable(perfMapType, false);

and diagnostic_server's call to enable perfmaps ds_rt_enable_perfmap invokes PerfMap::Enable(perfMapType, sendExisting: true)

runtime/src/coreclr/vm/eventing/eventpipe/ds-rt-coreclr.h

Line 317 in 283e216

PerfMap::Enable(perfMapType, true);

It seems the intent is for diagnostic server to only enable perfmap after initialization. Does PerfMap::Initialize() have any dependencies?

Moreover, even if it was intended for the diagnostic server to send PerfMap commands before startup's PerfMap::Initialize(), then Process Disable Perfmap whose contract is "instructs the runtime to stop emitting perfmap or jitdump files for the process" will be broken, as that command will respond through the IPC channel with "success", but perfmaps might be later enabled through startup's PerfMap::Initialize(). We could fix this by adding calls to PerfMap::Initialize() like in the above do an automatic initialize of perfmap on first call to enable, but that means if more commands could be added to PerfMap, each one would need PerfMap::Initialize().

If we make the IPC command wait for other components to setup, how long should the runtime take to respond to clients? For commands like Enable PerfMap/Disable PerfMap, would it be better to just queue the command, since the command only fails at parsing/unsupported, and does no extra validation of PerfMap::Enable/Disable.

How many of DiagnosticServer's commands are time sensitive enough that it makes a difference whether DiagnosticServer initializes later in ceemain.cpp? Right now, given that DiagnosticServer::Initialize and PerfMap::Initialize are close in proximity, the simplest resolution for now is the bump PerfMap up earlier, as long as it has no dependencies on anything in between

runtime/src/coreclr/vm/ceemain.cpp

Lines 709 to 733 in 7a7baaa

	DiagnosticServerAdapter::Initialize();
	DiagnosticServerAdapter::PauseForDiagnosticsMonitor();
	#endif // FEATURE_PERFTRACING
	#ifdef FEATURE_GDBJIT
	// Initialize gdbjit
	NotifyGdb::Initialize();
	#endif // FEATURE_GDBJIT
	#ifdef FEATURE_EVENT_TRACE
	// Initialize event tracing early so we can trace CLR startup time events.
	InitializeEventTracing();
	// Fire the EE startup ETW event
	ETWFireEvent(EEStartupStart_V1);
	#endif // FEATURE_EVENT_TRACE
	InitGSCookie();
	#ifdef LOGGING
	InitializeLogging();
	#endif
	#ifdef FEATURE_PERFMAP
	PerfMap::Initialize();

One scenario that comes to mind, if one used the DiagnosticServer to set DOTNET_PerfMapEnabled, through Process SetEnvironmentVariable. Though given it hasn't been documented, this scenario is likely not used.

[lateralusX]

We should keep diagnostic server startup where it is since it needs to cover as much of the startup sequence as possible for the startup tracing scenarios.

If the perfmap commands have the assumption that any default perfmap initialization has been done for the process, meaning you should be able to do a disable/enable sequence, then these commands cannot be executed until PerfMap::Initialize have completed.

Doesn't look like Enable with sendExisting=false have too many dependencies, so it might be possible to move it up before diagnostic server initialize but having that said I believe a better approach is to fix this in PerfMap logic and make it self-contained. We should make enable/disable do lazy initialization in case not already explicitly initialized.

[lateralusX]

I can draft a suggested fix for this during the week.