From 548673da440baad66e8b6f454346b14837f71715 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Thu, 18 Jul 2024 10:16:28 -0700 Subject: [PATCH 1/3] For profiler callbacks, always load the thread Also, add tests for profiler callbacks on a new thread to the runtime --- src/coreclr/vm/dllimport.cpp | 10 +---- .../native/transitions/transitions.cpp | 10 ++++- src/tests/profiler/transitions/transitions.cs | 42 +++++++++++++++++-- 3 files changed, 48 insertions(+), 14 deletions(-) diff --git a/src/coreclr/vm/dllimport.cpp b/src/coreclr/vm/dllimport.cpp index 41fa1842c5c13..2d36566a3fb2f 100644 --- a/src/coreclr/vm/dllimport.cpp +++ b/src/coreclr/vm/dllimport.cpp @@ -2272,15 +2272,7 @@ DWORD NDirectStubLinker::EmitProfilerBeginTransitionCallback(ILCodeStream* pcsEm EmitLoadStubContext(pcsEmit, dwStubFlags); } - if (SF_IsForwardStub(dwStubFlags)) - { - pcsEmit->EmitLDLOC(GetThreadLocalNum()); - } - else - { - // we use a null pThread to indicate reverse interop - pcsEmit->EmitLoadNullPtr(); - } + pcsEmit->EmitLDLOC(GetThreadLocalNum()); // In the unmanaged delegate case, we need the "this" object to retrieve the MD // in StubHelpers::ProfilerEnterCallback(). diff --git a/src/tests/profiler/native/transitions/transitions.cpp b/src/tests/profiler/native/transitions/transitions.cpp index 13984e0837cb2..3966c887440e9 100644 --- a/src/tests/profiler/native/transitions/transitions.cpp +++ b/src/tests/profiler/native/transitions/transitions.cpp @@ -69,6 +69,14 @@ extern "C" EXPORT void STDMETHODCALLTYPE DoPInvoke(int(*callback)(int), int i) printf("PInvoke received i=%d\n", callback(i)); } +extern "C" EXPORT void STDMETHODCALLTYPE DoPInvokeWithCallbackOnOtherThread(int(*callback)(int), int i) +{ + int j = 0; + std::thread t([&j, callback, i] { j = callback(i); }); + t.join(); + printf("PInvoke with callback on other thread received i=%d\n", j); +} + HRESULT Transitions::UnmanagedToManagedTransition(FunctionID functionID, COR_PRF_TRANSITION_REASON reason) { @@ -124,4 +132,4 @@ bool Transitions::FunctionIsTargetFunction(FunctionID functionID, TransitionInst } return true; -} \ No newline at end of file +} diff --git a/src/tests/profiler/transitions/transitions.cs b/src/tests/profiler/transitions/transitions.cs index b513d9c7717de..d0d6a7df67825 100644 --- a/src/tests/profiler/transitions/transitions.cs +++ b/src/tests/profiler/transitions/transitions.cs @@ -32,7 +32,7 @@ private static int DoDelegateReversePInvokeNonBlittable(bool b) public static int BlittablePInvokeToBlittableInteropDelegate() { InteropDelegate del = DoDelegateReversePInvoke; - + DoPInvoke((delegate* unmanaged)Marshal.GetFunctionPointerForDelegate(del), 13); GC.KeepAlive(del); @@ -42,13 +42,23 @@ public static int BlittablePInvokeToBlittableInteropDelegate() public static int NonBlittablePInvokeToNonBlittableInteropDelegate() { InteropDelegateNonBlittable del = DoDelegateReversePInvokeNonBlittable; - + DoPInvokeNonBlitable((delegate* unmanaged)Marshal.GetFunctionPointerForDelegate(del), true); GC.KeepAlive(del); return 100; } + public static int BlittablePInvokeToBlittableInteropDelegateOnOtherThread() + { + InteropDelegate del = DoDelegateReversePInvoke; + + DoPInvokeWithCallbackOnOtherThread((delegate* unmanaged)Marshal.GetFunctionPointerForDelegate(del), 13); + GC.KeepAlive(del); + + return 100; + } + [UnmanagedCallersOnly] private static int DoReversePInvoke(int i) { @@ -61,6 +71,9 @@ private static int DoReversePInvoke(int i) [DllImport("Profiler", EntryPoint = nameof(DoPInvoke))] public static extern void DoPInvokeNonBlitable(delegate* unmanaged callback, bool i); + [DllImport("Profiler")] + public static extern void DoPInvokeWithCallbackOnOtherThread(delegate* unmanaged callback, int i); + public static int BlittablePInvokeToUnmanagedCallersOnly() { DoPInvoke(&DoReversePInvoke, 13); @@ -75,6 +88,13 @@ public static int NonBlittablePInvokeToUnmanagedCallersOnly() return 100; } + public static int BlittablePInvokeToUnmanagedCallersOnlyOnOtherThread() + { + DoPInvokeWithCallbackOnOtherThread(&DoReversePInvoke, 13); + + return 100; + } + public static int Main(string[] args) { if (args.Length > 1 && args[0].Equals("RunTest", StringComparison.OrdinalIgnoreCase)) @@ -89,6 +109,10 @@ public static int Main(string[] args) return NonBlittablePInvokeToUnmanagedCallersOnly(); case nameof(NonBlittablePInvokeToNonBlittableInteropDelegate): return NonBlittablePInvokeToNonBlittableInteropDelegate(); + case nameof(BlittablePInvokeToUnmanagedCallersOnlyOnOtherThread): + return BlittablePInvokeToUnmanagedCallersOnlyOnOtherThread(); + case nameof(BlittablePInvokeToBlittableInteropDelegateOnOtherThread): + return BlittablePInvokeToBlittableInteropDelegateOnOtherThread(); } } @@ -104,12 +128,22 @@ public static int Main(string[] args) if (!RunProfilerTest(nameof(NonBlittablePInvokeToUnmanagedCallersOnly), nameof(DoPInvokeNonBlitable), nameof(DoReversePInvoke))) { - return 101; + return 103; } if (!RunProfilerTest(nameof(NonBlittablePInvokeToNonBlittableInteropDelegate), nameof(DoPInvokeNonBlitable), "Invoke")) { - return 102; + return 104; + } + + if (!RunProfilerTest(nameof(BlittablePInvokeToUnmanagedCallersOnlyOnOtherThread), nameof(DoPInvokeWithCallbackOnOtherThread), nameof(DoReversePInvoke))) + { + return 105; + } + + if (!RunProfilerTest(nameof(BlittablePInvokeToBlittableInteropDelegateOnOtherThread), nameof(DoPInvokeWithCallbackOnOtherThread), "Invoke")) + { + return 106; } return 100; From 4100f31498e694ff51b281345793146c14508e2e Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Thu, 18 Jul 2024 13:37:18 -0700 Subject: [PATCH 2/3] Add export to def file --- src/tests/profiler/native/profiler.def | 1 + 1 file changed, 1 insertion(+) diff --git a/src/tests/profiler/native/profiler.def b/src/tests/profiler/native/profiler.def index 37a59858dbfe0..11976272a2133 100644 --- a/src/tests/profiler/native/profiler.def +++ b/src/tests/profiler/native/profiler.def @@ -5,4 +5,5 @@ EXPORTS DllCanUnloadNow PRIVATE PassCallbackToProfiler PRIVATE DoPInvoke PRIVATE + DoPInvokeWithCallbackOnOtherThread PRIVATE From 307c76b52c71677f2bb441796a44bf1c13324508 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Fri, 19 Jul 2024 10:27:16 -0700 Subject: [PATCH 3/3] Add asserts --- src/coreclr/vm/stubhelpers.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/coreclr/vm/stubhelpers.cpp b/src/coreclr/vm/stubhelpers.cpp index e533422417c16..eba0ee96a639c 100644 --- a/src/coreclr/vm/stubhelpers.cpp +++ b/src/coreclr/vm/stubhelpers.cpp @@ -547,6 +547,7 @@ FCIMPL3(SIZE_T, StubHelpers::ProfilerBeginTransitionCallback, SIZE_T pSecretPara } { + _ASSERTE(pThread != nullptr); GCX_PREEMP_THREAD_EXISTS(pThread); ProfilerManagedToUnmanagedTransitionMD(pRealMD, COR_PRF_TRANSITION_CALL); @@ -577,6 +578,7 @@ FCIMPL2(void, StubHelpers::ProfilerEndTransitionCallback, MethodDesc* pRealMD, T // and the transition requires us to set up a HMF. HELPER_METHOD_FRAME_BEGIN_0(); { + _ASSERTE(pThread != nullptr); GCX_PREEMP_THREAD_EXISTS(pThread); ProfilerUnmanagedToManagedTransitionMD(pRealMD, COR_PRF_TRANSITION_RETURN);