Skip to content

Commit

Permalink
Fix debugger funceval deadlock (dotnet#72179)
Browse files Browse the repository at this point in the history 
Fixes dotnet#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.
  • Loading branch information
@noahfalk
noahfalk authored Jul 22, 2022
1 parent 5097317 commit 463efbb
Show file tree
Hide file tree
Showing 5 changed files with 216 additions and 284 deletions.
16 changes: 8 additions & 8 deletions src/coreclr/inc/CrstTypes.def
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -185,7 +185,7 @@ Crst DelegateToFPtrHash
End

Crst DomainLocalBlock
AcquiredBefore AppDomainHandleTable IbcProfile LoaderHeap SystemDomainDelayedUnloadList UniqueStack
AcquiredBefore PinnedHeapHandleTable IbcProfile LoaderHeap SystemDomainDelayedUnloadList UniqueStack
End

Crst DynamicIL
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -305,7 +305,7 @@ Crst LeafLock
End

Crst LoaderAllocator
AcquiredBefore AppDomainHandleTable HandleTable UniqueStack ThreadStore
AcquiredBefore PinnedHeapHandleTable HandleTable UniqueStack ThreadStore
AcquiredAfter DomainLocalBlock
End

Expand Down Expand Up @@ -334,7 +334,7 @@ Crst Module
End

Crst ModuleFixup
AcquiredBefore AppDomainHandleTable GlobalStrLiteralMap IbcProfile SyncBlockCache
AcquiredBefore PinnedHeapHandleTable GlobalStrLiteralMap IbcProfile SyncBlockCache
End

Crst ModuleLookupTable
Expand All @@ -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
Expand Down
166 changes: 83 additions & 83 deletions src/coreclr/inc/crsttypes.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,85 +16,85 @@
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,
Expand Down Expand Up @@ -146,7 +146,6 @@ enum CrstType
int g_rgCrstLevelMap[] =
{
10, // CrstAppDomainCache
14, // CrstAppDomainHandleTable
3, // CrstArgBasedStubCache
0, // CrstAssemblyList
12, // CrstAssemblyLoader
Expand Down Expand Up @@ -186,7 +185,7 @@ int g_rgCrstLevelMap[] =
7, // CrstFuncPtrStubs
10, // CrstFusionAppCtx
10, // CrstGCCover
13, // CrstGlobalStrLiteralMap
15, // CrstGlobalStrLiteralMap
1, // CrstHandleTable
0, // CrstIbcProfile
8, // CrstIJWFixupData
Expand All @@ -212,7 +211,7 @@ int g_rgCrstLevelMap[] =
3, // CrstManagedObjectWrapperMap
10, // CrstMethodDescBackpatchInfoTracker
5, // CrstModule
15, // CrstModuleFixup
16, // CrstModuleFixup
4, // CrstModuleLookupTable
0, // CrstMulticoreJitHash
13, // CrstMulticoreJitManager
Expand All @@ -225,6 +224,7 @@ int g_rgCrstLevelMap[] =
19, // CrstPendingTypeLoadEntry
4, // CrstPgoData
0, // CrstPinnedByrefValidation
14, // CrstPinnedHeapHandleTable
0, // CrstProfilerGCRefDataFreeList
13, // CrstProfilingAPIStatus
4, // CrstRCWCache
Expand Down Expand Up @@ -270,7 +270,6 @@ int g_rgCrstLevelMap[] =
LPCSTR g_rgCrstNameMap[] =
{
"CrstAppDomainCache",
"CrstAppDomainHandleTable",
"CrstArgBasedStubCache",
"CrstAssemblyList",
"CrstAssemblyLoader",
Expand Down Expand Up @@ -349,6 +348,7 @@ LPCSTR g_rgCrstNameMap[] =
"CrstPendingTypeLoadEntry",
"CrstPgoData",
"CrstPinnedByrefValidation",
"CrstPinnedHeapHandleTable",
"CrstProfilerGCRefDataFreeList",
"CrstProfilingAPIStatus",
"CrstRCWCache",
Expand Down
Loading

0 comments on commit 463efbb

Please sign in to comment.