From f905afddd6985af993022cce0dfef48fa20ce518 Mon Sep 17 00:00:00 2001 From: Elinor Fung Date: Thu, 28 Jan 2021 17:42:24 -0800 Subject: [PATCH] Take SuppressGCTransition into account in IL stub caching * Use updated stub flags (taking into account SuppressGCTransition) when caching * Add tests --- src/coreclr/src/vm/dllimport.cpp | 29 +++-- src/coreclr/src/vm/dllimport.h | 4 +- .../SuppressGCTransitionNative.cpp | 12 ++ .../SuppressGCTransitionTest.cs | 108 +++++++++++++++++- 4 files changed, 143 insertions(+), 10 deletions(-) diff --git a/src/coreclr/src/vm/dllimport.cpp b/src/coreclr/src/vm/dllimport.cpp index 9a99718582fde3..dfe1ba93f02d34 100644 --- a/src/coreclr/src/vm/dllimport.cpp +++ b/src/coreclr/src/vm/dllimport.cpp @@ -260,7 +260,8 @@ class ILStubState : public StubState DWORD dwStubFlags, int iLCIDParamIdx, MethodDesc* pTargetMD) - : m_slIL(dwStubFlags, pStubModule, signature, pTypeContext, pTargetMD, iLCIDParamIdx, fTargetHasThis, fStubHasThis) + : m_slIL(dwStubFlags, pStubModule, signature, pTypeContext, pTargetMD, iLCIDParamIdx, fTargetHasThis, fStubHasThis) + , m_dwStubFlags(dwStubFlags) { STANDARD_VM_CONTRACT; @@ -292,7 +293,7 @@ class ILStubState : public StubState { WRAPPER_NO_CONTRACT; m_slIL.Begin(dwStubFlags); - m_dwStubFlags = dwStubFlags; + _ASSERTE(m_dwStubFlags == dwStubFlags); } void MarshalReturn(MarshalInfo* pInfo, int argOffset) @@ -1204,6 +1205,8 @@ class ILStubState : public StubState TokenLookupMap* GetTokenLookupMap() { WRAPPER_NO_CONTRACT; return m_slIL.GetTokenLookupMap(); } + DWORD GetFlags() const { return m_dwStubFlags; } + protected: CQuickBytes m_qbNativeFnSigBuffer; NDirectStubLinker m_slIL; @@ -1350,7 +1353,7 @@ class PInvoke_ILStubState : public ILStubState pTypeContext, TargetHasThis(dwStubFlags), StubHasThis(dwStubFlags), - dwStubFlags, + UpdateStubFlags(dwStubFlags, pTargetMD), iLCIDParamIdx, pTargetMD) { @@ -1363,6 +1366,15 @@ class PInvoke_ILStubState : public ILStubState } private: + static DWORD UpdateStubFlags(DWORD dwStubFlags, MethodDesc* pTargetMD) + { + if (TargetSuppressGCTransition(dwStubFlags, pTargetMD)) + { + dwStubFlags |= NDIRECTSTUB_FL_SUPPRESSGCTRANSITION; + } + return dwStubFlags; + } + static BOOL TargetHasThis(DWORD dwStubFlags) { // @@ -1381,6 +1393,11 @@ class PInvoke_ILStubState : public ILStubState // return SF_IsForwardDelegateStub(dwStubFlags); } + + static BOOL TargetSuppressGCTransition(DWORD dwStubFlags, MethodDesc* pTargetMD) + { + return SF_IsForwardStub(dwStubFlags) && pTargetMD && pTargetMD->ShouldSuppressGCTransition(); + } }; #ifdef FEATURE_COMINTEROP @@ -4642,7 +4659,6 @@ MethodDesc* CreateInteropILStub( CorNativeLinkType nlType, CorNativeLinkFlags nlFlags, CorPinvokeMap unmgdCallConv, - DWORD dwStubFlags, // NDirectStubFlags int nParamTokens, mdParamDef* pParamTokenArray, int iLCIDArg, @@ -4676,6 +4692,8 @@ MethodDesc* CreateInteropILStub( // and vararg pinvoke. // + DWORD dwStubFlags = pss->GetFlags(); + #ifdef FEATURE_COMINTEROP // // Try to locate predefined IL stub either defined in user code or hardcoded in CLR @@ -5018,7 +5036,6 @@ MethodDesc* NDirect::CreateCLRToNativeILStub( nlType, nlFlags, unmgdCallConv, - dwStubFlags, numParamTokens, pParamTokenArray, iLCIDArg); @@ -5092,7 +5109,6 @@ MethodDesc* NDirect::CreateFieldAccessILStub( (CorNativeLinkType)0, (CorNativeLinkFlags)0, (CorPinvokeMap)0, - dwStubFlags, numParamTokens, pParamTokenArray, -1); @@ -5201,7 +5217,6 @@ MethodDesc* NDirect::CreateStructMarshalILStub(MethodTable* pMT) (CorNativeLinkType)0, (CorNativeLinkFlags)0, (CorPinvokeMap)0, - dwStubFlags, numParamTokens, pParamTokenArray, -1, diff --git a/src/coreclr/src/vm/dllimport.h b/src/coreclr/src/vm/dllimport.h index b24ab6e3e6c8e8..f6593c9524b1eb 100644 --- a/src/coreclr/src/vm/dllimport.h +++ b/src/coreclr/src/vm/dllimport.h @@ -150,7 +150,8 @@ enum NDirectStubFlags #ifdef FEATURE_COMINTEROP NDIRECTSTUB_FL_FIELDGETTER = 0x00002000, // COM->CLR field getter NDIRECTSTUB_FL_FIELDSETTER = 0x00004000, // COM->CLR field setter - // unused = 0x00008000, +#endif // FEATURE_COMINTEROP + NDIRECTSTUB_FL_SUPPRESSGCTRANSITION = 0x00008000, // unused = 0x00010000, // unused = 0x00020000, // unused = 0x00080000, @@ -158,7 +159,6 @@ enum NDirectStubFlags // unused = 0x00200000, // unused = 0x00400000, // unused = 0x00800000, -#endif // FEATURE_COMINTEROP // internal flags -- these won't ever show up in an NDirectStubHashBlob NDIRECTSTUB_FL_FOR_NUMPARAMBYTES = 0x10000000, // do just enough to return the right value from Marshal.NumParamBytes diff --git a/src/tests/Interop/PInvoke/Attributes/SuppressGCTransition/SuppressGCTransitionNative.cpp b/src/tests/Interop/PInvoke/Attributes/SuppressGCTransition/SuppressGCTransitionNative.cpp index 322bc4797668c2..ae86a3dbc2e15b 100644 --- a/src/tests/Interop/PInvoke/Attributes/SuppressGCTransition/SuppressGCTransitionNative.cpp +++ b/src/tests/Interop/PInvoke/Attributes/SuppressGCTransition/SuppressGCTransitionNative.cpp @@ -21,3 +21,15 @@ BOOL DLL_EXPORT NextUInt(/* out */ uint32_t *n) *n = (++_n); return TRUE; } + +typedef int (STDMETHODVCALLTYPE *CALLBACKPROC)(int n); + +extern "C" +BOOL DLL_EXPORT STDMETHODVCALLTYPE InvokeCallback(CALLBACKPROC cb, int* n) +{ + if (cb == nullptr || n == nullptr) + return FALSE; + + *n = cb((++_n)); + return TRUE; +} diff --git a/src/tests/Interop/PInvoke/Attributes/SuppressGCTransition/SuppressGCTransitionTest.cs b/src/tests/Interop/PInvoke/Attributes/SuppressGCTransition/SuppressGCTransitionTest.cs index 7cdc08a4e033d6..8c94239ad05bab 100644 --- a/src/tests/Interop/PInvoke/Attributes/SuppressGCTransition/SuppressGCTransitionTest.cs +++ b/src/tests/Interop/PInvoke/Attributes/SuppressGCTransition/SuppressGCTransitionTest.cs @@ -24,6 +24,34 @@ static class SuppressGCTransitionNative [DllImport(nameof(SuppressGCTransitionNative), EntryPoint = "NextUInt")] public static extern unsafe bool NextUInt_NoInline_GCTransition(int* n); + [DllImport(nameof(SuppressGCTransitionNative), CallingConvention=CallingConvention.Cdecl, EntryPoint = "InvokeCallback")] + [SuppressGCTransition] + public static extern unsafe int InvokeCallbackFuncPtr_Inline_NoGCTransition(delegate* unmanaged[Cdecl] cb, int* n); + + [DllImport(nameof(SuppressGCTransitionNative), CallingConvention=CallingConvention.Cdecl, EntryPoint = "InvokeCallback")] + public static extern unsafe int InvokeCallbackFuncPtr_Inline_GCTransition(delegate* unmanaged[Cdecl] cb, int* n); + + [DllImport(nameof(SuppressGCTransitionNative), CallingConvention=CallingConvention.Cdecl, EntryPoint = "InvokeCallback")] + [SuppressGCTransition] + public static extern unsafe bool InvokeCallbackFuncPtr_NoInline_NoGCTransition(delegate* unmanaged[Cdecl] cb, int* n); + + [DllImport(nameof(SuppressGCTransitionNative), CallingConvention=CallingConvention.Cdecl, EntryPoint = "InvokeCallback")] + public static extern unsafe bool InvokeCallbackFuncPtr_NoInline_GCTransition(delegate* unmanaged[Cdecl] cb, int* n); + + [DllImport(nameof(SuppressGCTransitionNative), CallingConvention=CallingConvention.Cdecl, EntryPoint = "InvokeCallback")] + [SuppressGCTransition] + public static extern unsafe int InvokeCallbackVoidPtr_Inline_NoGCTransition(void* cb, int* n); + + [DllImport(nameof(SuppressGCTransitionNative), CallingConvention=CallingConvention.Cdecl, EntryPoint = "InvokeCallback")] + public static extern unsafe int InvokeCallbackVoidPtr_Inline_GCTransition(void* cb, int* n); + + [DllImport(nameof(SuppressGCTransitionNative), CallingConvention=CallingConvention.Cdecl, EntryPoint = "InvokeCallback")] + [SuppressGCTransition] + public static extern unsafe bool InvokeCallbackVoidPtr_NoInline_NoGCTransition(void* cb, int* n); + + [DllImport(nameof(SuppressGCTransitionNative), CallingConvention=CallingConvention.Cdecl, EntryPoint = "InvokeCallback")] + public static extern unsafe bool InvokeCallbackVoidPtr_NoInline_GCTransition(void* cb, int* n); + public static IntPtr GetNextUIntFunctionPointer() { IntPtr mod = GetNativeLibrary(); @@ -146,8 +174,79 @@ private static int CallAsFunctionPointer(int expected) Assert.AreEqual(expected, n); return n + 1; } + [UnmanagedCallersOnly(CallConvs = new[] { typeof(CallConvCdecl) })] + private static int ReturnInt(int value) + { + return value; + } + [MethodImpl(MethodImplOptions.NoInlining)] + private static int ILStubCache_NoGCTransition_GCTransition(int expected) + { + // This test uses a callback marked UnmanagedCallersOnly as a way to verify that + // SuppressGCTransition is taken into account when caching IL stubs. + // It calls functions with the same signature, differing only in SuppressGCTransition. + // When calling an UnmanagedCallersOnly method, the runtime validates that the GC is in + // pre-emptive mode. If not, it throws a fatal error that cannot be caught and crashes. + // If the stub for the p/invoke with the transition suppressed is incorrectly reused for + // the p/invoke without the suppression, invoking the callback would produce a fatal error. + Console.WriteLine($"{nameof(ILStubCache_NoGCTransition_GCTransition)} ({expected}) ..."); + + int n; - public static int Main() + // Call function that has SuppressGCTransition + SuppressGCTransitionNative.InvokeCallbackFuncPtr_Inline_NoGCTransition(null, null); + + // Call function with same (blittable) signature, but without SuppressGCTransition. + // IL stub should not be re-used, GC transition should occur, and callback should be invoked. + SuppressGCTransitionNative.InvokeCallbackFuncPtr_Inline_GCTransition(&ReturnInt, &n); + Assert.AreEqual(expected++, n); + + // Call function that has SuppressGCTransition + SuppressGCTransitionNative.InvokeCallbackFuncPtr_NoInline_NoGCTransition(null, null); + + // Call function with same (non-blittable) signature, but without SuppressGCTransition + // IL stub should not be re-used, GC transition should occur, and callback should be invoked. + SuppressGCTransitionNative.InvokeCallbackFuncPtr_NoInline_GCTransition(&ReturnInt, &n); + Assert.AreEqual(expected++, n); + + return n + 1; + } + [MethodImpl(MethodImplOptions.NoInlining)] + private static int ILStubCache_GCTransition_NoGCTransition(int expected) + { + // This test uses a callback marked UnmanagedCallersOnly as a way to verify that + // SuppressGCTransition is taken into account when caching IL stubs. + // It calls functions with the same signature, differing only in SuppressGCTransition. + // When calling an UnmanagedCallersOnly method, the runtime validates that the GC is in + // pre-emptive mode. If not, it throws a fatal error that cannot be caught and crashes. + Console.WriteLine($"{nameof(ILStubCache_GCTransition_NoGCTransition)} ({expected}) ..."); + + int n; + + void* cb = (delegate* unmanaged[Cdecl])&ReturnInt; + + // Call function that does not have SuppressGCTransition + SuppressGCTransitionNative.InvokeCallbackVoidPtr_Inline_GCTransition(cb, &n); + Assert.AreEqual(expected++, n); + + // Call function with same (blittable) signature, but with SuppressGCTransition. + // IL stub should not be re-used, GC transition not should occur, and callback invocation should fail. + SuppressGCTransitionNative.InvokeCallbackVoidPtr_Inline_NoGCTransition(cb, &n); + Assert.AreEqual(expected++, n); + + // Call function that does not have SuppressGCTransition + SuppressGCTransitionNative.InvokeCallbackVoidPtr_NoInline_GCTransition(cb, &n); + Assert.AreEqual(expected++, n); + + // Call function with same (non-blittable) signature, but with SuppressGCTransition + // IL stub should not be re-used, GC transition not should occur, and callback invocation should fail. + expected = n + 1; + SuppressGCTransitionNative.InvokeCallbackVoidPtr_NoInline_NoGCTransition(cb, &n); + Assert.AreEqual(expected++, n); + + return n + 1; + } + public static int Main(string[] args) { try { @@ -159,6 +258,13 @@ public static int Main() n = Mixed(n); n = Mixed_TightLoop(n); n = CallAsFunctionPointer(n); + n = ILStubCache_NoGCTransition_GCTransition(n); + + if (args.Length != 0 && args[0].Equals("ILStubCache", StringComparison.OrdinalIgnoreCase)) + { + // This test intentionally results in a fatal error, so only run when manually specified + n = ILStubCache_GCTransition_NoGCTransition(n); + } } catch (Exception e) {