Fix ProfileEnter / ProfileLeave

[janvorli]

These two functions can be entered in preemptive mode for UnmanagedCallersOnly methods and also on thread that was not known to runtime. This change switches to cooperative mode if the thread is known to runtime and early outs when the thread was not known.

[ghost]

Tagging subscribers to this area: @tommcdon
See info in area-owners.md if you want to be subscribed.

Issue Details

---

These two functions can be entered in preemptive mode for UnmanagedCallersOnly methods and also on thread that was not known to runtime. This change switches to cooperative mode if the thread is known to runtime and early outs when the thread was not known.

Author:	janvorli
Assignees:	janvorli
Labels:	area-Diagnostics-coreclr
Milestone:	-

[jkotas]

The behavior of the Enter/Leave profiler callbacks on unmanaged callers only methods was discussed in #68760 .

• Do we need to add a test for this?

• Short-circuiting the callback when the thread is not setup yet is going to lead to mismatched Enter/Leave callbacks. We will skip Enter, but Leave is going to be issued since the thread is going to be setup by the time Leave gets called.

• This particular flavor of the callback is used when the profiler wants to inspect method arguments. The inspection of method arguments is not going to work well in all cases since it assumes managed calling convention. The unmanaged callers only methods can have calling convention that differs from managed calling convention.

What is the behavior that we want these callbacks to have for unmanaged callers only methods? Would it be better for the profilers if these callbacks are not issues for unmanaged callers only methods?

[davmason]

What is the behavior that we want these callbacks to have for unmanaged callers only methods? Would it be better for the profilers if these callbacks are not issues for unmanaged callers only methods?

The ELT hooks aren't that broadly used, but we have seen people use them to create managed shadow stacks, to do performance profiling, and for code coverage. I think most ELT users would prefer having hooks for all managed methods, but could make do with skipping UnmanagedCallersOnly methods since they are rare. Having mismatched Enter and Leave hooks would most likely break them though, we definitely want to avoid that.

Could we set up the thread instead of returning in the Enter hook?

[janvorli]

@jkotas, @davmason I have added setting up the runtime thread data structures if they don't exist at the entry to the two methods.

[jkotas]

The change is going to address the immediate problem that you have run into, but I think the profiler scenario as a whole (inspection of method argu,ments) is still going to be broken for the unmanagedcallersonly methods.

Potential alternative is to stop issuing these callbacks for unmanagedcallersonly methods as discussed in #68760 (comment)

I will leave it to @davmason to make a call on this.

[janvorli]

It is still unclear to me why we are not hitting the failures in the CI. I keep hitting this locally even without any local changes. The UnmanagedCallersOnly method is the System.Diagnostics.Tracing.EtwEventProvider.Callback:

runtime/src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventProvider.cs

Lines 836 to 839 in afed0a9

	[UnmanagedCallersOnly]
	private static unsafe void Callback(Guid* sourceId, int isEnabled, byte level,
	long matchAnyKeywords, long matchAllKeywords, Interop.Advapi32.EVENT_FILTER_DESCRIPTOR* filterData, void* callbackContext)
	{

 # Child-SP          RetAddr               Call Site
00 0000003a`73d7bfd8 00007ffd`bd15f4e9     KERNELBASE!DebugBreak+0x2
01 0000003a`73d7bfe0 00007ffd`bcc41ea0     coreclr!DbgAssertDialog+0x1b9 [F:\git\runtime8\src\coreclr\utilcode\debug.cpp @ 464] 
02 0000003a`73d7c100 00007ffd`bcec40b6     coreclr!Thread::ObjectRefFlush+0x80 [F:\git\runtime8\src\coreclr\vm\threads.cpp @ 5886] 
03 0000003a`73d7c130 00007ffd`bd119d8e     coreclr!ProfileEnter+0xa6 [F:\git\runtime8\src\coreclr\vm\proftoeeinterfaceimpl.cpp @ 10757] 
04 0000003a`73d7c670 00007ffd`5d8d0b79     coreclr!ProfileEnterNaked+0x6e [F:\git\runtime8\src\coreclr\vm\amd64\AsmHelpers.asm @ 528] 
05 0000003a`73d7c740 00007ffe`923403ad     System_Private_CoreLib!System.Diagnostics.Tracing.EtwEventProvider.Callback(System.Guid*, Int32, Byte, Int64, Int64, EVENT_FILTER_DESCRIPTOR*, Void*)+0x39
06 0000003a`73d7c7d0 00007ffe`92341ace     ntdll!EtwpEventApiCallback+0xe9
07 0000003a`73d7c880 00007ffe`92342079     ntdll!EtwpUpdateEnableInfoAndCallback+0xd6
08 0000003a`73d7c8d0 00007ffe`923421ba     ntdll!EtwpRegisterProvider+0xed
09 0000003a`73d7c9e0 00007ffe`923414d0     ntdll!EtwNotificationRegister+0xba
0a 0000003a`73d7ca20 00007ffd`5d8cf6e0     ntdll!EtwEventRegister+0x20
0b 0000003a`73d7ca60 00007ffd`5d8cf4d4     System_Private_CoreLib!System.Diagnostics.Tracing.EtwEventProvider.Register+0x140
0c 0000003a`73d7cb50 00007ffd`5d8c759c     System_Private_CoreLib!System.Diagnostics.Tracing.EventProvider.Register+0x94
0d 0000003a`73d7cb90 00007ffd`5d8c7128     System_Private_CoreLib!System.Diagnostics.Tracing.EventSource.Initialize+0x2cc
0e 0000003a`73d7cd40 00007ffd`5d8c700f     System_Private_CoreLib!System.Diagnostics.Tracing.EventSource..ctor+0xe8
0f 0000003a`73d7cd90 00007ffd`5d8d0027     System_Private_CoreLib!System.Diagnostics.Tracing.EventSource..ctor+0x7f
10 0000003a`73d7cde0 00007ffd`5d8cff11     System_Private_CoreLib!System.Diagnostics.Tracing.NativeRuntimeEventSource..ctor+0xc7
11 0000003a`73d7ce80 00007ffd`bd11a003     System_Private_CoreLib!System.Diagnostics.Tracing.NativeRuntimeEventSource..cctor+0x41

[jkotas]

I keep hitting this locally even without any local change

The stacktrace suggests that you have an active ETW session on your machine. For example, it can be a ETW session from VS or defender. The CI machines tend to be clean, they do not have random ETW sessions like that.

[davmason]

Potential alternative is to stop issuing these callbacks for unmanagedcallersonly methods as discussed in #68760 (comment)

I will leave it to @davmason to make a call on this.

I would like to take Jan's change as it is. I think there is value in having ELT hooks even without argument inspection. I filed #89617 against myself to track sorting out the argument inspection part - we should at a minimum return a failure HR.