diff --git a/src/coreclr/inc/CrstTypes.def b/src/coreclr/inc/CrstTypes.def index 8cd820022658cd..e3ae5275951f87 100644 --- a/src/coreclr/inc/CrstTypes.def +++ b/src/coreclr/inc/CrstTypes.def @@ -236,7 +236,7 @@ Crst IJWHash End Crst ILStubGen - AcquiredBefore DeadlockDetection UniqueStack + AcquiredBefore DeadlockDetection UniqueStack UnresolvedClassLock FusionAppCtx AssemblyLoader End Crst InstMethodHashTable diff --git a/src/coreclr/inc/crsttypes_generated.h b/src/coreclr/inc/crsttypes_generated.h index 713f6bd5b33f33..28eb23c9e267d4 100644 --- a/src/coreclr/inc/crsttypes_generated.h +++ b/src/coreclr/inc/crsttypes_generated.h @@ -170,7 +170,7 @@ int g_rgCrstLevelMap[] = 1, // CrstHandleTable 7, // CrstIJWFixupData 0, // CrstIJWHash - 6, // CrstILStubGen + 14, // CrstILStubGen 0, // CrstInlineTrackingMap 18, // CrstInstMethodHashTable 0, // CrstInterfaceDispatchGlobalLists diff --git a/src/coreclr/vm/dllimport.cpp b/src/coreclr/vm/dllimport.cpp index afa5b7a2401458..68aec52fd682af 100644 --- a/src/coreclr/vm/dllimport.cpp +++ b/src/coreclr/vm/dllimport.cpp @@ -229,10 +229,10 @@ static bool IsSharedStubScenario(DWORD dwStubFlags) return false; } - if (SF_IsForwardPInvokeStub(dwStubFlags) && !SF_IsCALLIStub(dwStubFlags) && !SF_IsVarArgStub(dwStubFlags)) - { - return false; - } + // Forward P/Invoke stubs (non-CALLI, non-vararg) are not shared across different + // target methods, but they still use the ILStubCache for de-duplication when + // multiple threads race to create a stub for the same target. The hash blob for + // these stubs is keyed by target MethodDesc rather than by signature. return true; } @@ -4081,10 +4081,37 @@ namespace PCCOR_SIGNATURE pvNativeType; }; - ILStubHashBlob* CreateHashBlob(PInvokeStubParameters* pParams) + ILStubHashBlob* CreateHashBlob(PInvokeStubParameters* pParams, MethodDesc* pTargetMD) { STANDARD_VM_CONTRACT; + // The hash blob for a forward P/Invoke stub is just the target MethodDesc pointer. + // In order to help avoid collisions, ensure various blob sizes are different. + // See asserts below. + constexpr size_t forwardPInvokeHashBlobSize = sizeof(ILStubHashBlobBase) + sizeof(MethodDesc*); + + DWORD dwStubFlags = pParams->m_dwStubFlags; + + // The target MethodDesc may be NULL for field marshalling. + // Forward P/Invoke stubs (non-CALLI, non-vararg) each target a specific method, + // so the hash blob is just the target MethodDesc pointer. This ensures racing + // threads for the same P/Invoke converge on the same DynamicMethodDesc in the + // ILStubCache, while different P/Invoke methods get distinct cache entries. + if (pTargetMD != NULL + && SF_IsForwardPInvokeStub(dwStubFlags) + && !SF_IsCALLIStub(dwStubFlags) + && !SF_IsVarArgStub(dwStubFlags)) + { + const size_t blobSize = forwardPInvokeHashBlobSize; + NewArrayHolder pBytes = new BYTE[blobSize]; + ZeroMemory(pBytes, blobSize); + ILStubHashBlob* pBlob = (ILStubHashBlob*)(BYTE*)pBytes; + pBlob->m_cbSizeOfBlob = blobSize; + memcpy(pBlob->m_rgbBlobData, &pTargetMD, sizeof(pTargetMD)); + pBytes.SuppressRelease(); + return pBlob; + } + PInvokeStubHashBlob* pBlob; IMDInternalImport* pInternalImport = pParams->m_pModule->GetMDImport(); @@ -4139,6 +4166,8 @@ namespace if (cbSizeOfBlob.IsOverflow()) COMPlusThrowHR(COR_E_OVERFLOW); + _ASSERTE(cbSizeOfBlob.Value() != forwardPInvokeHashBlobSize); + static_assert(nltMaxValue <= 0xFF); static_assert(nlfMaxValue <= 0xFF); static_assert(pmMaxValue <= 0xFFFF); @@ -4795,16 +4824,14 @@ namespace class ILStubCreatorHelper { public: - ILStubCreatorHelper(MethodDesc *pTargetMD, - PInvokeStubParameters* pParams - ) : - m_pTargetMD(pTargetMD), - m_pParams(pParams), - m_pStubMD(NULL), - m_bILStubCreator(false) + ILStubCreatorHelper(PInvokeStubParameters* pParams, MethodDesc *pTargetMD) + : m_pParams(pParams) + , m_pTargetMD(pTargetMD) + , m_pStubMD(NULL) + , m_bILStubCreator(false) { STANDARD_VM_CONTRACT; - m_pHashParams = CreateHashBlob(m_pParams); + m_pHashParams = CreateHashBlob(m_pParams, m_pTargetMD); } ~ILStubCreatorHelper() @@ -4866,11 +4893,10 @@ namespace } private: - MethodDesc* m_pTargetMD; PInvokeStubParameters* m_pParams; - NewArrayHolder m_pHashParams; - AllocMemTracker* m_pAmTracker; + MethodDesc* m_pTargetMD; MethodDesc* m_pStubMD; + NewArrayHolder m_pHashParams; AllocMemTracker m_amTracker; bool m_bILStubCreator; // Only the creator can remove the ILStub from the Cache }; //ILStubCreatorHelper @@ -4966,7 +4992,7 @@ namespace // remove it from cache if OOM occurs { - ILStubCreatorHelper ilStubCreatorHelper(pTargetMD, ¶ms); + ILStubCreatorHelper ilStubCreatorHelper(¶ms, pTargetMD); // take the domain level lock ListLockHolder pILStubLock(AppDomain::GetCurrentDomain()->GetILStubGenLock()); @@ -5685,18 +5711,12 @@ PCODE PInvoke::GetStubForILStub(PInvokeMethodDesc* pNMD, MethodDesc** ppStubMD, PInvokeLink(pNMD); } - // - // NOTE: there is a race in updating this MethodDesc. We depend on all - // threads getting back the same DynamicMethodDesc for a particular - // PInvokeMethodDesc, in that case, the locking around the actual JIT - // operation will prevent the code from being jitted more than once. - // By the time we get here, all threads get the same address of code - // back from the JIT operation and they all just fill in the same value - // here. - // - // In the NGEN case, all threads will get the same preimplemented code - // address much like the JIT case. - // + // NOTE: For IL-stub-backed P/Invoke stubs (i.e., when *ppStubMD is non-null + // and the stub is obtained via JitILStub/ILStubCache), racing threads will + // resolve to the same DynamicMethodDesc via the ILStubCache (keyed by the + // target MethodDesc). The locking around the JIT operation prevents the + // code from being jitted more than once, and all threads get back the same + // PCODE. See CreateHashBlob() for the hashing logic. return pStub; }