diff --git a/src/coreclr/vm/dllimport.cpp b/src/coreclr/vm/dllimport.cpp index 43174f4eb11cd..458a4d881e86d 100644 --- a/src/coreclr/vm/dllimport.cpp +++ b/src/coreclr/vm/dllimport.cpp @@ -4745,11 +4745,25 @@ namespace RemoveILStubCacheEntry(); } + inline bool CreatedTheAssociatedPublishedStubMD() + { + return m_bILStubCreator; + } + inline void GetStubMethodDesc() { WRAPPER_NO_CONTRACT; + // The creator flag represents ownership of the associated stub MD and indicates that the + // stub MD has not been removed from the cache, so the lookup below is guaranteed to return + // this owned published stub MD. +#ifdef _DEBUG + MethodDesc* pPreexistingStubMD = m_pStubMD; + bool createdThePreexistingMD = m_bILStubCreator; +#endif // _DEBUG + m_pStubMD = ::GetStubMethodDesc(m_pTargetMD, m_pParams, m_pHashParams, &m_amTracker, m_bILStubCreator, m_pStubMD); + _ASSERTE(!createdThePreexistingMD || (m_bILStubCreator && (m_pStubMD == pPreexistingStubMD))); } inline void RemoveILStubCacheEntry() @@ -4776,20 +4790,6 @@ namespace m_amTracker.SuppressRelease(); } - DEBUG_NOINLINE static void HolderEnter(ILStubCreatorHelper *pThis) - { - WRAPPER_NO_CONTRACT; - ANNOTATION_SPECIAL_HOLDER_CALLER_NEEDS_DYNAMIC_CONTRACT; - pThis->GetStubMethodDesc(); - } - - DEBUG_NOINLINE static void HolderLeave(ILStubCreatorHelper *pThis) - { - WRAPPER_NO_CONTRACT; - ANNOTATION_SPECIAL_HOLDER_CALLER_NEEDS_DYNAMIC_CONTRACT; - pThis->RemoveILStubCacheEntry(); - } - private: MethodDesc* m_pTargetMD; NDirectStubParameters* m_pParams; @@ -4800,8 +4800,6 @@ namespace bool m_bILStubCreator; // Only the creator can remove the ILStub from the Cache }; //ILStubCreatorHelper - typedef Wrapper ILStubCreatorHelperHolder; - MethodDesc* CreateInteropILStub( ILStubState* pss, StubSigDesc* pSigDesc, @@ -4875,8 +4873,8 @@ namespace pSigDesc->m_pMT ); - // The following two ILStubCreatorHelperHolder are to recover the status when an - // exception happen during the generation of the IL stubs. We need to free the + // The following ILStubCreatorHelper is to recover the status when an + // exception happens during the generation of the IL stubs. We need to free the // memory allocated and restore the ILStubCache. // // The following block is logically divided into two phases. The first phase is @@ -4886,7 +4884,7 @@ namespace // // ilStubCreatorHelper contains an instance of AllocMemTracker which tracks the // allocated memory during the creation of MethodDesc so that we are able to remove - // them when releasing the ILStubCreatorHelperHolder or destructing ILStubCreatorHelper + // them when destructing ILStubCreatorHelper // When removing IL Stub from Cache, we have a constraint that only the thread which // creates the stub can remove it. Otherwise, any thread hits cache and gets the stub will @@ -4899,10 +4897,8 @@ namespace ListLockHolder pILStubLock(pLoaderModule->GetDomain()->GetILStubGenLock()); { - // The holder will free the allocated MethodDesc and restore the ILStubCache - // if exception happen. - ILStubCreatorHelperHolder pCreateOrGetStubHolder(&ilStubCreatorHelper); - pStubMD = pCreateOrGetStubHolder->GetStubMD(); + ilStubCreatorHelper.GetStubMethodDesc(); + pStubMD = ilStubCreatorHelper.GetStubMD(); /////////////////////////////// // @@ -4916,16 +4912,11 @@ namespace ListLockEntryLockHolder pEntryLock(pEntry, FALSE); - // We can release the holder for the first phase now - pCreateOrGetStubHolder.SuppressRelease(); - // We have the entry lock we need to use, so we can release the global lock. pILStubLock.Release(); { - // The holder will free the allocated MethodDesc and restore the ILStubCache - // if exception happen. The reason to get the holder again is to - ILStubCreatorHelperHolder pGenILHolder(&ilStubCreatorHelper); + ilStubCreatorHelper.GetStubMethodDesc(); if (!pEntryLock.DeadlockAwareAcquire()) { @@ -4950,11 +4941,11 @@ namespace pILStubLock.Acquire(); // Assure that pStubMD we have now has not been destroyed by other threads - pGenILHolder->GetStubMethodDesc(); + ilStubCreatorHelper.GetStubMethodDesc(); - while (pStubMD != pGenILHolder->GetStubMD()) + while (pStubMD != ilStubCreatorHelper.GetStubMD()) { - pStubMD = pGenILHolder->GetStubMD(); + pStubMD = ilStubCreatorHelper.GetStubMD(); pEntry.Assign(ListLockEntry::Find(pILStubLock, pStubMD, "il stub gen lock")); pEntryLock.Assign(pEntry, FALSE); @@ -4980,7 +4971,7 @@ namespace pILStubLock.Acquire(); - pGenILHolder->GetStubMethodDesc(); + ilStubCreatorHelper.GetStubMethodDesc(); } } @@ -5064,22 +5055,38 @@ namespace sgh.SuppressRelease(); } - if (pGeneratedNewStub) - { - *pGeneratedNewStub = true; - } - pEntry->m_hrResultCode = S_OK; break; } // Link the MethodDesc onto the method table with the lock taken AddMethodDescChunkWithLockTaken(¶ms, pStubMD); - - pGenILHolder.SuppressRelease(); } } } + + // Callers use the new stub indicator to distinguish between 1) the case where a new stub + // MD was generated during this call and 2) the case where this function attached to a stub + // MD that was generated by some other call (either a call that completed earlier or a call + // on a racing thread). In particular, reliably detecting case (1) is crucial because it is + // the only case where this call permanently publishes a new stub MD into the cache, + // meaning it is the only case where the caller cannot safely free any allocations (such as + // a signature buffer) which the stub MD might reference. + // + // Set the indicator if and only if the stub MD that will be imminiently returned to the + // caller was created by the code above (and will therefore become a permanent member of + // the cache when the SuppressRelease occurs below). Note that, in the presence of racing + // threads, the current call may or may not have carried out IL generation for the stub; + // the only important thing is whether the current call was the one that created the stub + // MD earlier on. + if (ilStubCreatorHelper.CreatedTheAssociatedPublishedStubMD()) + { + if (pGeneratedNewStub) + { + *pGeneratedNewStub = true; + } + } + ilStubCreatorHelper.SuppressRelease(); }