diff --git a/src/coreclr/vm/loaderallocator.cpp b/src/coreclr/vm/loaderallocator.cpp index c35d13d6afc77..c186a3d1b9e1b 100644 --- a/src/coreclr/vm/loaderallocator.cpp +++ b/src/coreclr/vm/loaderallocator.cpp @@ -769,7 +769,7 @@ LOADERHANDLE LoaderAllocator::AllocateHandle(OBJECTREF value) do { { - CrstHolder ch(&m_crstLoaderAllocator); + CrstHolder ch(&m_crstLoaderAllocatorHandleTable); gc.handleTable = gc.loaderAllocator->GetHandleTable(); @@ -781,7 +781,6 @@ LOADERHANDLE LoaderAllocator::AllocateHandle(OBJECTREF value) retVal = (UINT_PTR)((freeHandleIndex + 1) << 1); break; } - slotsUsed = gc.loaderAllocator->GetSlotsUsed(); if (slotsUsed > MAX_LOADERALLOCATOR_HANDLE) @@ -808,7 +807,7 @@ LOADERHANDLE LoaderAllocator::AllocateHandle(OBJECTREF value) gc.handleTable = (PTRARRAYREF)AllocateObjectArray(newSize, g_pObjectClass); { - CrstHolder ch(&m_crstLoaderAllocator); + CrstHolder ch(&m_crstLoaderAllocatorHandleTable); if (gc.loaderAllocator->GetHandleTable() == gc.handleTableOld) { @@ -886,7 +885,7 @@ void LoaderAllocator::FreeHandle(LOADERHANDLE handle) // The slot value doesn't have the low bit set, so it is an index to the handle table. // In this case, push the index of the handle to the stack of freed indexes for // reuse. - CrstHolder ch(&m_crstLoaderAllocator); + CrstHolder ch(&m_crstLoaderAllocatorHandleTable); UINT_PTR index = (((UINT_PTR)handle) >> 1) - 1; // The Push can fail due to OOM. Ignore this failure, it is better than crashing. The @@ -937,7 +936,7 @@ OBJECTREF LoaderAllocator::CompareExchangeValueInHandle(LOADERHANDLE handle, OBJ else { /* The handle table is read locklessly, be careful */ - CrstHolder ch(&m_crstLoaderAllocator); + CrstHolder ch(&m_crstLoaderAllocatorHandleTable); _ASSERTE(!ObjectHandleIsNull(m_hLoaderAllocatorObjectHandle)); @@ -983,7 +982,7 @@ void LoaderAllocator::SetHandleValue(LOADERHANDLE handle, OBJECTREF value) else { // The handle table is read locklessly, be careful - CrstHolder ch(&m_crstLoaderAllocator); + CrstHolder ch(&m_crstLoaderAllocatorHandleTable); _ASSERTE(!ObjectHandleIsNull(m_hLoaderAllocatorObjectHandle)); @@ -1065,6 +1064,7 @@ void LoaderAllocator::Init(BaseDomain *pDomain, BYTE *pExecutableHeapMemory) m_pDomain = pDomain; m_crstLoaderAllocator.Init(CrstLoaderAllocator, (CrstFlags)CRST_UNSAFE_COOPGC); + m_crstLoaderAllocatorHandleTable.Init(CrstLeafLock, (CrstFlags)CRST_UNSAFE_COOPGC); m_InteropDataCrst.Init(CrstInteropData, CRST_REENTRANCY); #ifdef FEATURE_COMINTEROP m_ComCallWrapperCrst.Init(CrstCOMCallWrapper); diff --git a/src/coreclr/vm/loaderallocator.hpp b/src/coreclr/vm/loaderallocator.hpp index 8d3fdb3fcde89..b6f5ad1445572 100644 --- a/src/coreclr/vm/loaderallocator.hpp +++ b/src/coreclr/vm/loaderallocator.hpp @@ -324,6 +324,9 @@ class LoaderAllocator // The LoaderAllocator specific string literal map. StringLiteralMap *m_pStringLiteralMap; CrstExplicitInit m_crstLoaderAllocator; + + // Protect the handle table data structures, seperated from m_crstLoaderAllocator to allow thread cleanup to use the lock + CrstExplicitInit m_crstLoaderAllocatorHandleTable; bool m_fGCPressure; bool m_fUnloaded; bool m_fTerminated; @@ -649,6 +652,15 @@ class LoaderAllocator LOADERALLOCATORREF GetExposedObject(); bool IsExposedObjectLive(); +#ifdef _DEBUG + bool HasHandleTableLock() + { + WRAPPER_NO_CONTRACT; + if (this == NULL) return true; // During initialization of the LoaderAllocator object, callers may call this with a null this pointer. + return m_crstLoaderAllocatorHandleTable.OwnedByCurrentThread(); + } +#endif + #ifndef DACCESS_COMPILE bool InsertObjectIntoFieldWithLifetimeOfCollectibleLoaderAllocator(OBJECTREF value, Object** pField); LOADERHANDLE AllocateHandle(OBJECTREF value); diff --git a/src/coreclr/vm/loaderallocator.inl b/src/coreclr/vm/loaderallocator.inl index d9f6da55df5b9..d839a4632277f 100644 --- a/src/coreclr/vm/loaderallocator.inl +++ b/src/coreclr/vm/loaderallocator.inl @@ -131,7 +131,7 @@ FORCEINLINE BOOL LoaderAllocator::GetHandleValueFastPhase2(LOADERHANDLE handle, return FALSE; LOADERALLOCATORREF loaderAllocator = dac_cast(loaderAllocatorAsObjectRef); - PTRARRAYREF handleTable = loaderAllocator->GetHandleTable(); + PTRARRAYREF handleTable = loaderAllocator->DangerousGetHandleTable(); UINT_PTR index = (((UINT_PTR)handle) >> 1) - 1; *pValue = handleTable->GetAt(index); @@ -149,7 +149,7 @@ FORCEINLINE OBJECTREF LoaderAllocator::GetHandleValueFastCannotFailType2(LOADERH /* This is lockless access to the handle table, be careful */ OBJECTREF loaderAllocatorAsObjectRef = ObjectFromHandle(m_hLoaderAllocatorObjectHandle); LOADERALLOCATORREF loaderAllocator = dac_cast(loaderAllocatorAsObjectRef); - PTRARRAYREF handleTable = loaderAllocator->GetHandleTable(); + PTRARRAYREF handleTable = loaderAllocator->DangerousGetHandleTable(); UINT_PTR index = (((UINT_PTR)handle) >> 1) - 1; return handleTable->GetAt(index); diff --git a/src/coreclr/vm/object.cpp b/src/coreclr/vm/object.cpp index a41c3acfcd4bb..2a05ca470bff1 100644 --- a/src/coreclr/vm/object.cpp +++ b/src/coreclr/vm/object.cpp @@ -1952,3 +1952,61 @@ void ExceptionObject::GetStackTrace(StackTraceArray & stackTrace, PTRARRAYREF * #endif // !defined(DACCESS_COMPILE) } + +#ifndef DACCESS_COMPILE +void LoaderAllocatorObject::SetSlotsUsed(INT32 newSlotsUsed) +{ + CONTRACTL + { + NOTHROW; + GC_NOTRIGGER; + MODE_COOPERATIVE; + PRECONDITION(m_pLoaderAllocatorScout->m_nativeLoaderAllocator->HasHandleTableLock()); + } + CONTRACTL_END; + + m_slotsUsed = newSlotsUsed; +} + +PTRARRAYREF LoaderAllocatorObject::GetHandleTable() +{ + CONTRACTL + { + NOTHROW; + GC_NOTRIGGER; + MODE_COOPERATIVE; + PRECONDITION(m_pLoaderAllocatorScout->m_nativeLoaderAllocator->HasHandleTableLock()); + } + CONTRACTL_END; + + return (PTRARRAYREF)m_pSlots; +} + +void LoaderAllocatorObject::SetHandleTable(PTRARRAYREF handleTable) +{ + CONTRACTL + { + NOTHROW; + GC_NOTRIGGER; + MODE_COOPERATIVE; + PRECONDITION(m_pLoaderAllocatorScout->m_nativeLoaderAllocator->HasHandleTableLock()); + } + CONTRACTL_END; + + SetObjectReference(&m_pSlots, (OBJECTREF)handleTable); +} + +INT32 LoaderAllocatorObject::GetSlotsUsed() +{ + CONTRACTL + { + NOTHROW; + GC_NOTRIGGER; + MODE_COOPERATIVE; + PRECONDITION(m_pLoaderAllocatorScout->m_nativeLoaderAllocator->HasHandleTableLock()); + } + CONTRACTL_END; + + return m_slotsUsed; +} +#endif // DACCESS_COMPILE diff --git a/src/coreclr/vm/object.h b/src/coreclr/vm/object.h index 59c3e300b7305..6d65b65502314 100644 --- a/src/coreclr/vm/object.h +++ b/src/coreclr/vm/object.h @@ -2081,30 +2081,22 @@ class LoaderAllocatorObject : public Object friend class CoreLibBinder; public: - PTRARRAYREF GetHandleTable() + // All uses of this api must be safe lock-free reads used only for reading from the handle table + // The normal GetHandleTable can only be called while holding the handle table lock, but + // this is for use in lock-free scenarios + PTRARRAYREF DangerousGetHandleTable() { LIMITED_METHOD_DAC_CONTRACT; - return (PTRARRAYREF)m_pSlots; - } - - void SetHandleTable(PTRARRAYREF handleTable) - { - LIMITED_METHOD_CONTRACT; - SetObjectReference(&m_pSlots, (OBJECTREF)handleTable); - } - - INT32 GetSlotsUsed() - { - LIMITED_METHOD_CONTRACT; - return m_slotsUsed; - } - - void SetSlotsUsed(INT32 newSlotsUsed) - { - LIMITED_METHOD_CONTRACT; - m_slotsUsed = newSlotsUsed; + return (PTRARRAYREF)ObjectToOBJECTREF(VolatileLoadWithoutBarrier((Object**)&m_pSlots)); } +#ifndef DACCESS_COMPILE + PTRARRAYREF GetHandleTable(); + void SetHandleTable(PTRARRAYREF handleTable); + INT32 GetSlotsUsed(); + void SetSlotsUsed(INT32 newSlotsUsed); +#endif // DACCESS_COMPILE + void SetNativeLoaderAllocator(LoaderAllocator * pLoaderAllocator) { LIMITED_METHOD_CONTRACT; diff --git a/src/tests/Loader/CollectibleAssemblies/Statics/CollectibleStatics.cs b/src/tests/Loader/CollectibleAssemblies/Statics/CollectibleStatics.cs index 6c1e5560a7e03..2b66431be0760 100644 --- a/src/tests/Loader/CollectibleAssemblies/Statics/CollectibleStatics.cs +++ b/src/tests/Loader/CollectibleAssemblies/Statics/CollectibleStatics.cs @@ -8,6 +8,7 @@ using System.Runtime.CompilerServices; using System.Runtime.InteropServices; using System.Runtime.Loader; +using System.Threading; using Xunit; public class Program @@ -65,6 +66,48 @@ public static int TestEntryPoint() if (val5Obj != obj5) return 15; + int otherThreadResult = 0; + Thread t = new ((ThreadStart)delegate { + + object obj1 = new object(); + object obj2 = new object(); + object obj3 = new object(); + object obj4 = new object(); + object obj5 = new object(); + accessor.SetStaticObject(obj1, obj2, obj3, obj4, obj5); + accessor.GetStaticObject(out object val1Obj, out object val2Obj, out object val3Obj, out object val4Obj, out object val5Obj); + if (val1Obj != obj1) + { + otherThreadResult = 111; + return; + } + if (val2Obj != obj2) + { + otherThreadResult = 112; + return; + } + if (val3Obj != obj3) + { + otherThreadResult = 113; + return; + } + if (val4Obj != obj4) + { + otherThreadResult = 114; + return; + } + if (val5Obj != obj5) + { + otherThreadResult = 115; + return; + } + }); + + t.Start(); + t.Join(); + if (otherThreadResult != 0) + return otherThreadResult; + GC.KeepAlive(accessor); return 100; }