Fix debugger funceval deadlock (#72179)
authorNoah Falk <noahfalk@users.noreply.github.com>
Fri, 22 Jul 2022 23:52:27 +0000 (16:52 -0700)
committerGitHub <noreply@github.com>
Fri, 22 Jul 2022 23:52:27 +0000 (16:52 -0700)
Fixes #60565

Visual Studio devs reported that debugger funcevals were deadlocking because the
PinnedHeapHandleTableCrst was held while threads were suspended. This change
refactors that code path so that the AllocateHandles() operation where the lock is held
gets split into two parts and the GC allocation where the debugger could suspend is
outside the region where the critical section is held.

In the old code the PinnedHeapHandleTable was synchronized by one of two different locks, either the AppDomainHandleTable lock or the GlobalStringLiteralMap. In the new code AppDomainHandleTable lock is renamed to PinnedHeapHandleTable lock and this lock is always what synchronizes the PinnedHeapHandleTable code. In the string literal code path the GlobalStringLiteralMap is taken first and the PinnedHeapHandleTable lock is taken second, but the PinnedHeapHandleTable is no longer reliant on that outer GlobalStringLiteralMap lock for correctness.

In terms of testing I can verify under a debugger that I can suspend in the GC allocation point and the PinnedHeapHandleTable lock isn't held. This doesn't of course prevent other locks from being held so at best it is a partial fix for the issue. Nobody had a known repro so I wasn't able to verify anything more specifically. I also confirmed the race cases on the InterlockedExchange paths worked how they were intended by forcing them with a native debugger.

src/coreclr/inc/CrstTypes.def
src/coreclr/inc/crsttypes.h
src/coreclr/vm/appdomain.cpp
src/coreclr/vm/appdomain.hpp
src/coreclr/vm/stringliteralmap.cpp

index d4f8118..3d2cdcf 100644 (file)
@@ -77,9 +77,9 @@ Crst AppDomainCache
     AcquiredBefore UniqueStack UnresolvedClassLock
 End
 
-Crst AppDomainHandleTable
+Crst PinnedHeapHandleTable
     AcquiredBefore AvailableParamTypes HandleTable IbcProfile SyncBlockCache SystemDomainDelayedUnloadList
-                   ThreadStore SystemDomain
+                   SystemDomain
 End
 
 Crst ArgBasedStubCache
@@ -185,7 +185,7 @@ Crst DelegateToFPtrHash
 End
 
 Crst DomainLocalBlock
-    AcquiredBefore AppDomainHandleTable IbcProfile LoaderHeap SystemDomainDelayedUnloadList UniqueStack
+    AcquiredBefore PinnedHeapHandleTable IbcProfile LoaderHeap SystemDomainDelayedUnloadList UniqueStack
 End
 
 Crst DynamicIL
@@ -230,7 +230,7 @@ Crst GCCover
 End
 
 Crst GlobalStrLiteralMap
-    AcquiredBefore HandleTable IbcProfile SyncBlockCache SystemDomainDelayedUnloadList ThreadStore UniqueStack
+    AcquiredBefore PinnedHeapHandleTable HandleTable IbcProfile SyncBlockCache SystemDomainDelayedUnloadList ThreadStore UniqueStack
 End
 
 Crst HandleTable
@@ -256,7 +256,7 @@ Crst InstMethodHashTable
 End
 
 Crst Interop
-    AcquiredBefore AppDomainHandleTable AvailableParamTypes ClassInit DeadlockDetection DomainLocalBlock
+    AcquiredBefore PinnedHeapHandleTable AvailableParamTypes ClassInit DeadlockDetection DomainLocalBlock
                    HandleTable InstMethodHashTable InteropData JitGenericHandleCache LoaderHeap SigConvert
                    StubDispatchCache StubUnwindInfoHeapSegments SyncBlockCache TypeIDMap UnresolvedClassLock
                    PendingTypeLoadEntry
@@ -305,7 +305,7 @@ Crst LeafLock
 End
 
 Crst LoaderAllocator
-    AcquiredBefore AppDomainHandleTable HandleTable UniqueStack ThreadStore
+    AcquiredBefore PinnedHeapHandleTable HandleTable UniqueStack ThreadStore
     AcquiredAfter DomainLocalBlock
 End
 
@@ -334,7 +334,7 @@ Crst Module
 End
 
 Crst ModuleFixup
-    AcquiredBefore AppDomainHandleTable GlobalStrLiteralMap IbcProfile SyncBlockCache
+    AcquiredBefore PinnedHeapHandleTable GlobalStrLiteralMap IbcProfile SyncBlockCache
 End
 
 Crst ModuleLookupTable
@@ -353,7 +353,7 @@ Crst PEImage
 End
 
 Crst PendingTypeLoadEntry
-    AcquiredBefore AppDomainCache AppDomainHandleTable AssemblyLoader AvailableClass AvailableParamTypes
+    AcquiredBefore AppDomainCache PinnedHeapHandleTable AssemblyLoader AvailableClass AvailableParamTypes
                    BaseDomain ClassInit DeadlockDetection DebuggerController DebuggerJitInfo DebuggerMutex
                    DomainLocalBlock Exception ExecuteManRangeLock FuncPtrStubs
                    FusionAppCtx GlobalStrLiteralMap HandleTable IbcProfile
index 93ea7f8..b5e52ae 100644 (file)
 enum CrstType
 {
     CrstAppDomainCache = 0,
-    CrstAppDomainHandleTable = 1,
-    CrstArgBasedStubCache = 2,
-    CrstAssemblyList = 3,
-    CrstAssemblyLoader = 4,
-    CrstAvailableClass = 5,
-    CrstAvailableParamTypes = 6,
-    CrstBaseDomain = 7,
-    CrstCCompRC = 8,
-    CrstClassFactInfoHash = 9,
-    CrstClassInit = 10,
-    CrstClrNotification = 11,
-    CrstCodeFragmentHeap = 12,
-    CrstCodeVersioning = 13,
-    CrstCOMCallWrapper = 14,
-    CrstCOMWrapperCache = 15,
-    CrstDataTest1 = 16,
-    CrstDataTest2 = 17,
-    CrstDbgTransport = 18,
-    CrstDeadlockDetection = 19,
-    CrstDebuggerController = 20,
-    CrstDebuggerFavorLock = 21,
-    CrstDebuggerHeapExecMemLock = 22,
-    CrstDebuggerHeapLock = 23,
-    CrstDebuggerJitInfo = 24,
-    CrstDebuggerMutex = 25,
-    CrstDelegateToFPtrHash = 26,
-    CrstDomainLocalBlock = 27,
-    CrstDynamicIL = 28,
-    CrstDynamicMT = 29,
-    CrstEtwTypeLogHash = 30,
-    CrstEventPipe = 31,
-    CrstEventStore = 32,
-    CrstException = 33,
-    CrstExecutableAllocatorLock = 34,
-    CrstExecuteManRangeLock = 35,
-    CrstExternalObjectContextCache = 36,
-    CrstFCall = 37,
-    CrstFuncPtrStubs = 38,
-    CrstFusionAppCtx = 39,
-    CrstGCCover = 40,
-    CrstGlobalStrLiteralMap = 41,
-    CrstHandleTable = 42,
-    CrstIbcProfile = 43,
-    CrstIJWFixupData = 44,
-    CrstIJWHash = 45,
-    CrstILStubGen = 46,
-    CrstInlineTrackingMap = 47,
-    CrstInstMethodHashTable = 48,
-    CrstInterop = 49,
-    CrstInteropData = 50,
-    CrstIsJMCMethod = 51,
-    CrstISymUnmanagedReader = 52,
-    CrstJit = 53,
-    CrstJitGenericHandleCache = 54,
-    CrstJitInlineTrackingMap = 55,
-    CrstJitPatchpoint = 56,
-    CrstJitPerf = 57,
-    CrstJumpStubCache = 58,
-    CrstLeafLock = 59,
-    CrstListLock = 60,
-    CrstLoaderAllocator = 61,
-    CrstLoaderAllocatorReferences = 62,
-    CrstLoaderHeap = 63,
-    CrstManagedObjectWrapperMap = 64,
-    CrstMethodDescBackpatchInfoTracker = 65,
-    CrstModule = 66,
-    CrstModuleFixup = 67,
-    CrstModuleLookupTable = 68,
-    CrstMulticoreJitHash = 69,
-    CrstMulticoreJitManager = 70,
-    CrstNativeImageEagerFixups = 71,
-    CrstNativeImageLoad = 72,
-    CrstNls = 73,
-    CrstNotifyGdb = 74,
-    CrstObjectList = 75,
-    CrstPEImage = 76,
-    CrstPendingTypeLoadEntry = 77,
-    CrstPgoData = 78,
-    CrstPinnedByrefValidation = 79,
+    CrstArgBasedStubCache = 1,
+    CrstAssemblyList = 2,
+    CrstAssemblyLoader = 3,
+    CrstAvailableClass = 4,
+    CrstAvailableParamTypes = 5,
+    CrstBaseDomain = 6,
+    CrstCCompRC = 7,
+    CrstClassFactInfoHash = 8,
+    CrstClassInit = 9,
+    CrstClrNotification = 10,
+    CrstCodeFragmentHeap = 11,
+    CrstCodeVersioning = 12,
+    CrstCOMCallWrapper = 13,
+    CrstCOMWrapperCache = 14,
+    CrstDataTest1 = 15,
+    CrstDataTest2 = 16,
+    CrstDbgTransport = 17,
+    CrstDeadlockDetection = 18,
+    CrstDebuggerController = 19,
+    CrstDebuggerFavorLock = 20,
+    CrstDebuggerHeapExecMemLock = 21,
+    CrstDebuggerHeapLock = 22,
+    CrstDebuggerJitInfo = 23,
+    CrstDebuggerMutex = 24,
+    CrstDelegateToFPtrHash = 25,
+    CrstDomainLocalBlock = 26,
+    CrstDynamicIL = 27,
+    CrstDynamicMT = 28,
+    CrstEtwTypeLogHash = 29,
+    CrstEventPipe = 30,
+    CrstEventStore = 31,
+    CrstException = 32,
+    CrstExecutableAllocatorLock = 33,
+    CrstExecuteManRangeLock = 34,
+    CrstExternalObjectContextCache = 35,
+    CrstFCall = 36,
+    CrstFuncPtrStubs = 37,
+    CrstFusionAppCtx = 38,
+    CrstGCCover = 39,
+    CrstGlobalStrLiteralMap = 40,
+    CrstHandleTable = 41,
+    CrstIbcProfile = 42,
+    CrstIJWFixupData = 43,
+    CrstIJWHash = 44,
+    CrstILStubGen = 45,
+    CrstInlineTrackingMap = 46,
+    CrstInstMethodHashTable = 47,
+    CrstInterop = 48,
+    CrstInteropData = 49,
+    CrstIsJMCMethod = 50,
+    CrstISymUnmanagedReader = 51,
+    CrstJit = 52,
+    CrstJitGenericHandleCache = 53,
+    CrstJitInlineTrackingMap = 54,
+    CrstJitPatchpoint = 55,
+    CrstJitPerf = 56,
+    CrstJumpStubCache = 57,
+    CrstLeafLock = 58,
+    CrstListLock = 59,
+    CrstLoaderAllocator = 60,
+    CrstLoaderAllocatorReferences = 61,
+    CrstLoaderHeap = 62,
+    CrstManagedObjectWrapperMap = 63,
+    CrstMethodDescBackpatchInfoTracker = 64,
+    CrstModule = 65,
+    CrstModuleFixup = 66,
+    CrstModuleLookupTable = 67,
+    CrstMulticoreJitHash = 68,
+    CrstMulticoreJitManager = 69,
+    CrstNativeImageEagerFixups = 70,
+    CrstNativeImageLoad = 71,
+    CrstNls = 72,
+    CrstNotifyGdb = 73,
+    CrstObjectList = 74,
+    CrstPEImage = 75,
+    CrstPendingTypeLoadEntry = 76,
+    CrstPgoData = 77,
+    CrstPinnedByrefValidation = 78,
+    CrstPinnedHeapHandleTable = 79,
     CrstProfilerGCRefDataFreeList = 80,
     CrstProfilingAPIStatus = 81,
     CrstRCWCache = 82,
@@ -146,7 +146,6 @@ enum CrstType
 int g_rgCrstLevelMap[] =
 {
     10,         // CrstAppDomainCache
-    14,         // CrstAppDomainHandleTable
     3,          // CrstArgBasedStubCache
     0,          // CrstAssemblyList
     12,         // CrstAssemblyLoader
@@ -186,7 +185,7 @@ int g_rgCrstLevelMap[] =
     7,          // CrstFuncPtrStubs
     10,         // CrstFusionAppCtx
     10,         // CrstGCCover
-    13,         // CrstGlobalStrLiteralMap
+    15,         // CrstGlobalStrLiteralMap
     1,          // CrstHandleTable
     0,          // CrstIbcProfile
     8,          // CrstIJWFixupData
@@ -212,7 +211,7 @@ int g_rgCrstLevelMap[] =
     3,          // CrstManagedObjectWrapperMap
     10,         // CrstMethodDescBackpatchInfoTracker
     5,          // CrstModule
-    15,         // CrstModuleFixup
+    16,         // CrstModuleFixup
     4,          // CrstModuleLookupTable
     0,          // CrstMulticoreJitHash
     13,         // CrstMulticoreJitManager
@@ -225,6 +224,7 @@ int g_rgCrstLevelMap[] =
     19,         // CrstPendingTypeLoadEntry
     4,          // CrstPgoData
     0,          // CrstPinnedByrefValidation
+    14,         // CrstPinnedHeapHandleTable
     0,          // CrstProfilerGCRefDataFreeList
     13,         // CrstProfilingAPIStatus
     4,          // CrstRCWCache
@@ -270,7 +270,6 @@ int g_rgCrstLevelMap[] =
 LPCSTR g_rgCrstNameMap[] =
 {
     "CrstAppDomainCache",
-    "CrstAppDomainHandleTable",
     "CrstArgBasedStubCache",
     "CrstAssemblyList",
     "CrstAssemblyLoader",
@@ -349,6 +348,7 @@ LPCSTR g_rgCrstNameMap[] =
     "CrstPendingTypeLoadEntry",
     "CrstPgoData",
     "CrstPinnedByrefValidation",
+    "CrstPinnedHeapHandleTable",
     "CrstProfilerGCRefDataFreeList",
     "CrstProfilingAPIStatus",
     "CrstRCWCache",
index 4ac4264..9fbccfb 100644 (file)
@@ -109,34 +109,28 @@ CrstStatic          SystemDomain::m_SystemDomainCrst;
 CrstStatic          SystemDomain::m_DelayedUnloadCrst;
 
 // Constructor for the PinnedHeapHandleBucket class.
-PinnedHeapHandleBucket::PinnedHeapHandleBucket(PinnedHeapHandleBucket *pNext, DWORD Size, BaseDomain *pDomain)
+PinnedHeapHandleBucket::PinnedHeapHandleBucket(PinnedHeapHandleBucket *pNext, PTRARRAYREF pinnedHandleArrayObj, DWORD size, BaseDomain *pDomain)
 : m_pNext(pNext)
-, m_ArraySize(Size)
+, m_ArraySize(size)
 , m_CurrentPos(0)
 , m_CurrentEmbeddedFreePos(0) // hint for where to start a search for an embedded free item
 {
     CONTRACTL
     {
         THROWS;
-        GC_TRIGGERS;
+        GC_NOTRIGGER;
         MODE_COOPERATIVE;
         PRECONDITION(CheckPointer(pDomain));
         INJECT_FAULT(COMPlusThrowOM(););
     }
     CONTRACTL_END;
 
-    PTRARRAYREF HandleArrayObj;
-
-    // Allocate the array in the large object heap.
-    OVERRIDE_TYPE_LOAD_LEVEL_LIMIT(CLASS_LOADED);
-    HandleArrayObj = (PTRARRAYREF)AllocateObjectArray(Size, g_pObjectClass, /* bAllocateInPinnedHeap = */TRUE);
-
     // Retrieve the pointer to the data inside the array. This is legal since the array
-    // is located in the large object heap and is guaranteed not to move.
-    m_pArrayDataPtr = (OBJECTREF *)HandleArrayObj->GetDataPtr();
+    // is located in the pinned object heap and is guaranteed not to move.
+    m_pArrayDataPtr = (OBJECTREF *)pinnedHandleArrayObj->GetDataPtr();
 
     // Store the array in a strong handle to keep it alive.
-    m_hndHandleArray = pDomain->CreateStrongHandle((OBJECTREF)HandleArrayObj);
+    m_hndHandleArray = pDomain->CreateStrongHandle((OBJECTREF)pinnedHandleArrayObj);
 }
 
 
@@ -243,9 +237,7 @@ PinnedHeapHandleTable::PinnedHeapHandleTable(BaseDomain *pDomain, DWORD InitialB
     }
     CONTRACTL_END;
 
-#ifdef _DEBUG
-    m_pCrstDebug = NULL;
-#endif
+    m_Crst.Init(CrstPinnedHeapHandleTable, CRST_UNSAFE_COOPGC);
 }
 
 
@@ -268,101 +260,9 @@ PinnedHeapHandleTable::~PinnedHeapHandleTable()
     }
 }
 
-//*****************************************************************************
-//
-// LOCKING RULES FOR AllocateHandles() and ReleaseHandles()  12/08/2004
-//
-//
-// These functions are not protected by any locking in this location but rather the callers are
-// assumed to be doing suitable locking  for the handle table.  The handle table itself is
-// behaving rather like a thread-agnostic collection class -- it doesn't want to know
-// much about the outside world and so it is just doing its job with no awareness of
-// thread notions.
-//
-// The instance in question is
-// There are two locations you can find a PinnedHeapHandleTable
-// 1) there is one in every BaseDomain, it is used to keep track of the static members
-//     in that domain
-// 2) there is one in the System Domain that is used for the GlobalStringLiteralMap
-//
-// the one in (2) is not the same as the one that is in the BaseDomain object that corresponds
-// to the SystemDomain -- that one is basically stilborn because the string literals don't go
-// there and of course the System Domain has no code loaded into it -- only regular
-// AppDomains (like Domain 0) actually execute code.  As a result handle tables are in
-// practice used either for string literals or for static members but never for both.
-// At least not at this writing.
-//
-// Now it's useful to consider what the locking discipline is for these classes.
-//
-// ---------
-//
-// First case: (easiest) is the statics members
-//
-// Each BaseDomain has its own critical section
-//
-// BaseDomain::AllocateObjRefPtrsInLargeTable takes a lock with
-//        CrstHolder ch(&m_PinnedHeapHandleTableCrst);
-//
-// it does this before it calls AllocateHandles which suffices.  It does not call ReleaseHandles
-// at any time (although ReleaseHandles may be called via AllocateHandles if the request
-// doesn't fit in the current block, the remaining handles at the end of the block are released
-// automatically as part of allocation/recycling)
-//
-// note: Recycled handles are only used during String Literal allocation because we only try
-// to recycle handles if the allocation request is for exactly one handle.
-//
-// The handles in the BaseDomain handle table are released when the Domain is unloaded
-// as the GC objects become rootless at that time.
-//
-// This dispenses with all of the Handle tables except the one that is used for string literals
-//
-// ---------
-//
-// Second case:  Allocation for use in a string literal
-//
-// AppDomainStringLiteralMap::GetStringLiteral
-// leads to calls to
-//     PinnedHeapHandleBlockHolder constructor
-//     leads to calls to
-//          m_Data = pOwner->AllocateHandles(nCount);
-//
-// before doing this  AppDomainStringLiteralMap::GetStringLiteral takes this lock
-//
-//    CrstHolder gch(&(SystemDomain::GetGlobalStringLiteralMap()->m_HashTableCrstGlobal));
-//
-// which is the lock for the hash table that it owns
-//
-// STRINGREF *AppDomainStringLiteralMap::GetInternedString
-//
-// has a similar call path and uses the same approach and  the same lock
-// this covers all the paths which allocate
-//
-// ---------
-//
-// Third case:  Releases for use in a string literal entry
-//
-// CrstHolder gch(&(SystemDomain::GetGlobalStringLiteralMap()->m_HashTableCrstGlobal));
-// taken in the AppDomainStringLiteralMap functions below protects the 3 ways that this can happen
-//
-// case 3a)
-//
-// AppDomainStringLiteralMap::GetStringLiteral() can call StringLiteralEntry::Release in some
-// error cases, leading to the same stack as above
-//
-// case 3b)
-//
-// AppDomainStringLiteralMap::GetInternedString() can call StringLiteralEntry::Release in some
-// error cases, leading to the same stack as above
-//
-// case 3c)
-//
-// The same code paths in 3b and 3c and also end up releasing if an exception is thrown
-// during their processing.  Both these paths use a StringLiteralEntryHolder to assist in cleanup,
-// the StaticRelease method of the StringLiteralEntry gets called, which in turn calls the
-// Release method.
-
 
 // Allocate handles from the large heap handle table.
+// This function is thread-safe for concurrent invocation by multiple callers
 OBJECTREF* PinnedHeapHandleTable::AllocateHandles(DWORD nRequested)
 {
     CONTRACTL
@@ -375,14 +275,13 @@ OBJECTREF* PinnedHeapHandleTable::AllocateHandles(DWORD nRequested)
     }
     CONTRACTL_END;
 
-    // SEE "LOCKING RULES FOR AllocateHandles() and ReleaseHandles()" above
-
-    // the lock must be registered and already held by the caller per contract
 #ifdef _DEBUG
-    _ASSERTE(m_pCrstDebug != NULL);
-    _ASSERTE(m_pCrstDebug->OwnedByCurrentThread());
+    _ASSERTE(!m_Crst.OwnedByCurrentThread());
 #endif
 
+    // beware: we leave and re-enter this lock below in this method
+    CrstHolderWithState lockHolder(&m_Crst);
+
     if (nRequested == 1 && m_cEmbeddedFree != 0)
     {
         // special casing singleton requests to look for slots that can be re-used
@@ -414,28 +313,70 @@ OBJECTREF* PinnedHeapHandleTable::AllocateHandles(DWORD nRequested)
 
 
     // Retrieve the remaining number of handles in the bucket.
-    DWORD NumRemainingHandlesInBucket = (m_pHead != NULL) ? m_pHead->GetNumRemainingHandles() : 0;
+    DWORD numRemainingHandlesInBucket = (m_pHead != NULL) ? m_pHead->GetNumRemainingHandles() : 0;
+    PTRARRAYREF pinnedHandleArrayObj = NULL;
+    DWORD nextBucketSize = min(m_NextBucketSize * 2, MAX_BUCKETSIZE);
 
     // create a new block if this request doesn't fit in the current block
-    if (nRequested > NumRemainingHandlesInBucket)
+    if (nRequested > numRemainingHandlesInBucket)
     {
-        if (m_pHead != NULL)
+        // create a new bucket for this allocation
+        // We need a block big enough to hold the requested handles
+        DWORD newBucketSize = max(m_NextBucketSize, nRequested);
+
+        // Leave the lock temporarily to do the GC allocation
+        // 
+        // Why do we need to do certain work outside the lock? Because if we didn't this can happen:
+        // 1. AllocateHandles needs the GC to allocate
+        // 2. Anything which invokes the GC might also get suspended by the managed debugger which
+        //    will block the thread inside the lock
+        // 3. The managed debugger can run function-evaluation on any thread
+        // 4. Those func-evals might need to allocate handles
+        // 5. The func-eval can't acquire the lock to allocate handles because the thread in
+        //    step (3) still holds the lock. The thread in step (3) won't release the lock until the
+        //    debugger allows it to resume. The debugger won't resume until the funceval completes.
+        // 6. This either creates a deadlock or forces the debugger to abort the func-eval with a bad
+        //    user experience.
+        //
+        // This is only a partial fix to the func-eval problem. Some of the callers to AllocateHandles()
+        // are holding their own different locks farther up the stack. To address this more completely
+        // we probably need to change the fundamental invariant that all GC suspend points are also valid
+        // debugger suspend points. Changes in that area have proven to be error-prone in the past and we
+        // don't yet have the appropriate testing to validate that a future attempt gets it correct.
+        lockHolder.Release();
         {
-            // mark the handles in that remaining region as available for re-use
-            ReleaseHandles(m_pHead->CurrentPos(), NumRemainingHandlesInBucket);
-
-            // mark what's left as having been used
-            m_pHead->ConsumeRemaining();
+            OVERRIDE_TYPE_LOAD_LEVEL_LIMIT(CLASS_LOADED);
+            pinnedHandleArrayObj = (PTRARRAYREF)AllocateObjectArray(newBucketSize, g_pObjectClass, /* bAllocateInPinnedHeap = */TRUE);
         }
+        lockHolder.Acquire();
 
-        // create a new bucket for this allocation
+        // after leaving and re-entering the lock anything we verified or computed above using internal state could
+        // have changed. We need to retest if we still need the new allocation.
+        numRemainingHandlesInBucket = (m_pHead != NULL) ? m_pHead->GetNumRemainingHandles() : 0;
+        if (nRequested > numRemainingHandlesInBucket)
+        {
+            if (m_pHead != NULL)
+            {
+                // mark the handles in that remaining region as available for re-use
+                ReleaseHandlesLocked(m_pHead->CurrentPos(), numRemainingHandlesInBucket);
 
-        // We need a block big enough to hold the requested handles
-        DWORD NewBucketSize = max(m_NextBucketSize, nRequested);
+                // mark what's left as having been used
+                m_pHead->ConsumeRemaining();
+            }
 
-        m_pHead = new PinnedHeapHandleBucket(m_pHead, NewBucketSize, m_pDomain);
+            m_pHead = new PinnedHeapHandleBucket(m_pHead, pinnedHandleArrayObj, newBucketSize, m_pDomain);
 
-        m_NextBucketSize = min(m_NextBucketSize * 2, MAX_BUCKETSIZE);
+            // we already computed nextBucketSize to be double the previous size above, but it is possible that
+            // other threads increased m_NextBucketSize while the lock was unheld. We want to ensure
+            // m_NextBucketSize never shrinks even if nextBucketSize is no longer m_NextBucketSize*2.
+            m_NextBucketSize = max(m_NextBucketSize, nextBucketSize);
+        }
+        else
+        {
+            // we didn't need the allocation after all
+            // no handle has been created to root this so the GC may be able to reclaim and reuse it
+            pinnedHandleArrayObj = NULL;
+        }
     }
 
     return m_pHead->AllocateHandles(nRequested);
@@ -443,6 +384,7 @@ OBJECTREF* PinnedHeapHandleTable::AllocateHandles(DWORD nRequested)
 
 //*****************************************************************************
 // Release object handles allocated using AllocateHandles().
+// This function is thread-safe for concurrent invocation by multiple callers
 void PinnedHeapHandleTable::ReleaseHandles(OBJECTREF *pObjRef, DWORD nReleased)
 {
     CONTRACTL
@@ -454,12 +396,27 @@ void PinnedHeapHandleTable::ReleaseHandles(OBJECTREF *pObjRef, DWORD nReleased)
     }
     CONTRACTL_END;
 
-    // SEE "LOCKING RULES FOR AllocateHandles() and ReleaseHandles()" above
+#ifdef _DEBUG
+    _ASSERTE(!m_Crst.OwnedByCurrentThread());
+#endif
+
+    CrstHolder ch(&m_Crst);
+    ReleaseHandlesLocked(pObjRef, nReleased);
+}
+
+void PinnedHeapHandleTable::ReleaseHandlesLocked(OBJECTREF *pObjRef, DWORD nReleased)
+{
+    CONTRACTL
+    {
+        NOTHROW;
+        GC_NOTRIGGER;
+        MODE_COOPERATIVE;
+        PRECONDITION(CheckPointer(pObjRef));
+    }
+    CONTRACTL_END;
 
-    // the lock must be registered and already held by the caller per contract
 #ifdef _DEBUG
-    _ASSERTE(m_pCrstDebug != NULL);
-    _ASSERTE(m_pCrstDebug->OwnedByCurrentThread());
+    _ASSERTE(m_Crst.OwnedByCurrentThread());
 #endif
 
     OBJECTREF pPreallocatedSentinelObject = ObjectFromHandle(g_pPreallocatedSentinelObject);
@@ -681,9 +638,6 @@ void BaseDomain::Init()
     m_ILStubGenLock.Init(CrstILStubGen, CrstFlags(CRST_REENTRANCY), TRUE);
     m_NativeTypeLoadLock.Init(CrstInteropData, CrstFlags(CRST_REENTRANCY), TRUE);
 
-    // Pinned heap handle table CRST.
-    m_PinnedHeapHandleTableCrst.Init(CrstAppDomainHandleTable);
-
     m_crstLoaderAllocatorReferences.Init(CrstLoaderAllocatorReferences);
     // Has to switch thread to GC_NOTRIGGER while being held (see code:BaseDomain#AssemblyListLock)
     m_crstAssemblyList.Init(CrstAssemblyList, CrstFlags(
@@ -851,31 +805,27 @@ OBJECTREF* BaseDomain::AllocateObjRefPtrsInLargeTable(int nRequested, OBJECTREF*
         return *ppLazyAllocate;
     }
 
-    // Enter preemptive state, take the lock and go back to cooperative mode.
-    {
-        CrstHolder ch(&m_PinnedHeapHandleTableCrst);
-        GCX_COOP();
-
-        if (ppLazyAllocate && *ppLazyAllocate)
-        {
-            // Allocation already happened
-            return *ppLazyAllocate;
-        }
-
-        // Make sure the large heap handle table is initialized.
-        if (!m_pPinnedHeapHandleTable)
-            InitPinnedHeapHandleTable();
+    GCX_COOP();
 
-        // Allocate the handles.
-        OBJECTREF* result = m_pPinnedHeapHandleTable->AllocateHandles(nRequested);
+    // Make sure the large heap handle table is initialized.
+    if (!m_pPinnedHeapHandleTable)
+        InitPinnedHeapHandleTable();
 
-        if (ppLazyAllocate)
+    // Allocate the handles.
+    OBJECTREF* result = m_pPinnedHeapHandleTable->AllocateHandles(nRequested);
+    if (ppLazyAllocate)
+    {
+        // race with other threads that might be doing the same concurrent allocation
+        if (InterlockedCompareExchangeT<OBJECTREF*>(ppLazyAllocate, result, NULL) != NULL)
         {
-            *ppLazyAllocate = result;
+            // we lost the race, release our handles and use the handles from the
+            // winning thread
+            m_pPinnedHeapHandleTable->ReleaseHandles(result, nRequested);
+            result = *ppLazyAllocate;
         }
-
-        return result;
     }
+
+    return result;
 }
 
 #endif // !DACCESS_COMPILE
@@ -955,17 +905,17 @@ void BaseDomain::InitPinnedHeapHandleTable()
     {
         THROWS;
         GC_TRIGGERS;
-        MODE_ANY;
-        PRECONDITION(m_pPinnedHeapHandleTable==NULL);
+        MODE_COOPERATIVE;
         INJECT_FAULT(COMPlusThrowOM(););
     }
     CONTRACTL_END;
 
-    m_pPinnedHeapHandleTable = new PinnedHeapHandleTable(this, STATIC_OBJECT_TABLE_BUCKET_SIZE);
-
-#ifdef _DEBUG
-    m_pPinnedHeapHandleTable->RegisterCrstDebug(&m_PinnedHeapHandleTableCrst);
-#endif
+    PinnedHeapHandleTable* pTable = new PinnedHeapHandleTable(this, STATIC_OBJECT_TABLE_BUCKET_SIZE);
+    if(InterlockedCompareExchangeT<PinnedHeapHandleTable*>(&m_pPinnedHeapHandleTable, pTable, NULL) != NULL)
+    {
+        // another thread beat us to initializing the field, delete our copy
+        delete pTable;
+    }
 }
 
 
index 212a5e9..08fb580 100644 (file)
@@ -469,7 +469,7 @@ class PinnedHeapHandleBucket
 {
 public:
     // Constructor and desctructor.
-    PinnedHeapHandleBucket(PinnedHeapHandleBucket *pNext, DWORD Size, BaseDomain *pDomain);
+    PinnedHeapHandleBucket(PinnedHeapHandleBucket *pNext, PTRARRAYREF pinnedHandleArrayObj, DWORD size, BaseDomain *pDomain);
     ~PinnedHeapHandleBucket();
 
     // This returns the next bucket.
@@ -536,43 +536,25 @@ public:
     void EnumStaticGCRefs(promote_func* fn, ScanContext* sc);
 
 private:
+    void ReleaseHandlesLocked(OBJECTREF *pObjRef, DWORD nReleased);
+
     // The buckets of object handles.
+    // synchronized by m_Crst
     PinnedHeapHandleBucket *m_pHead;
 
     // We need to know the containing domain so we know where to allocate handles
     BaseDomain *m_pDomain;
 
     // The size of the PinnedHeapHandleBucket.
+    // synchronized by m_Crst
     DWORD m_NextBucketSize;
 
     // for finding and re-using embedded free items in the list
+    // these fields are synchronized by m_Crst
     PinnedHeapHandleBucket *m_pFreeSearchHint;
     DWORD m_cEmbeddedFree;
 
-#ifdef _DEBUG
-
-    // these functions are present to enforce that there is a locking mechanism in place
-    // for each PinnedHeapHandleTable even though the code itself does not do the locking
-    // you must tell the table which lock you intend to use and it will verify that it has
-    // in fact been taken before performing any operations
-
-public:
-    void RegisterCrstDebug(CrstBase *pCrst)
-    {
-        LIMITED_METHOD_CONTRACT;
-
-        // this function must be called exactly once
-        _ASSERTE(pCrst != NULL);
-        _ASSERTE(m_pCrstDebug == NULL);
-        m_pCrstDebug = pCrst;
-    }
-
-private:
-    // we will assert that this Crst is held before using the object
-    CrstBase *m_pCrstDebug;
-
-#endif
-
+    CrstExplicitInit m_Crst;
 };
 
 class PinnedHeapHandleBlockHolder;
@@ -588,7 +570,14 @@ class PinnedHeapHandleBlockHolder:public Holder<PinnedHeapHandleBlockHolder*,DoN
 public:
     FORCEINLINE PinnedHeapHandleBlockHolder(PinnedHeapHandleTable* pOwner, DWORD nCount)
     {
-        WRAPPER_NO_CONTRACT;
+        CONTRACTL
+        {
+            THROWS;
+            GC_TRIGGERS;
+            MODE_COOPERATIVE;
+        }
+        CONTRACTL_END;
+
         m_Data = pOwner->AllocateHandles(nCount);
         m_Count=nCount;
         m_pTable=pOwner;
@@ -1134,9 +1123,6 @@ protected:
     // The pinned heap handle table.
     PinnedHeapHandleTable       *m_pPinnedHeapHandleTable;
 
-    // The pinned heap handle table critical section.
-    CrstExplicitInit             m_PinnedHeapHandleTableCrst;
-
 #ifdef FEATURE_COMINTEROP
     // Information regarding the managed standard interfaces.
     MngStdInterfacesInfo        *m_pMngStdInterfacesInfo;
index 361de54..240b57b 100644 (file)
@@ -297,10 +297,6 @@ GlobalStringLiteralMap::GlobalStringLiteralMap()
         GC_TRIGGERS;
     }
     CONTRACTL_END;
-
-#ifdef _DEBUG
-    m_PinnedHeapHandleTable.RegisterCrstDebug(&m_HashTableCrstGlobal);
-#endif
 }
 
 GlobalStringLiteralMap::~GlobalStringLiteralMap()