diff --git a/src/coreclr/vm/syncclean.cpp b/src/coreclr/vm/syncclean.cpp index 1fa97cd589f745..7622cb6c183851 100644 --- a/src/coreclr/vm/syncclean.cpp +++ b/src/coreclr/vm/syncclean.cpp @@ -5,6 +5,7 @@ #include "common.h" #include "syncclean.hpp" +#include "threadstatics.h" #include "virtualcallstub.h" #include "threadsuspend.h" @@ -24,6 +25,8 @@ void SyncClean::CleanUp() // Give others we want to reclaim during the GC sync point a chance to do it VirtualCallStubManager::ReclaimAll(); + CleanupRetiredTLSIndices(); + #ifdef FEATURE_INTERPRETER // Reclaim dead interpreter dispatch cache entries InterpDispatchCache_ReclaimAll(); diff --git a/src/coreclr/vm/threadstatics.cpp b/src/coreclr/vm/threadstatics.cpp index 7c37a2d4ca4e79..2eae87c5ba515d 100644 --- a/src/coreclr/vm/threadstatics.cpp +++ b/src/coreclr/vm/threadstatics.cpp @@ -7,7 +7,7 @@ InFlightTLSData::InFlightTLSData(TLSIndex index) : pNext(NULL), tlsIndex(index), hTLSData(0) { } InFlightTLSData::~InFlightTLSData() { - if (!IsHandleNullUnchecked(hTLSData)) + if (hTLSData != nullptr) { DestroyTypedHandle(hTLSData); } @@ -32,6 +32,16 @@ static uint32_t g_NextNonCollectibleTlsSlot = NUMBER_OF_TLSOFFSETS_NOT_USED_IN_N static uint32_t g_directThreadLocalTLSBytesAvailable = EXTENDED_DIRECT_THREAD_LOCAL_SIZE; static CrstStatic g_TLSCrst; +static Volatile g_hasRetiredTLSIndices = false; + +// Collectible TLS indices are quarantined before they can be reused. Loader allocator +// cleanup removes the MethodTable from the active map and marks its index retired, so +// new types cannot claim the index while any thread can still contain state from the +// old owner. At the next EE synchronization point, CleanupRetiredTLSIndices clears the +// loader handle, TLS weak handle, and in-flight data for every thread. Only after that +// pass is complete is the marker changed to reusable and FindClearedIndex can return it. +static constexpr uint8_t ReusableTLSIndexMarker = 0; +static constexpr uint8_t RetiredTLSIndexMarker = 1; #endif // This can be used for out of thread access to TLS data. @@ -289,7 +299,7 @@ bool TLSIndexToMethodTableMap::FindClearedIndex(TLSIndex* pIndex) for (const auto& entry : *this) { - if (entry.IsClearedValue) + if (entry.IsClearedValue && entry.ClearedMarker == ReusableTLSIndexMarker) { *pIndex = entry.TlsIndex; return true; @@ -298,6 +308,21 @@ bool TLSIndexToMethodTableMap::FindClearedIndex(TLSIndex* pIndex) return false; } +void TLSIndexToMethodTableMap::SetClearedMarker(TLSIndex index, uint8_t marker) +{ + CONTRACTL + { + NOTHROW; + GC_NOTRIGGER; + MODE_ANY; + } + CONTRACTL_END; + + _ASSERTE(index.GetIndexOffset() < m_maxIndex); + _ASSERTE(IsClearedValue(pMap[index.GetIndexOffset()])); + VolatileStore(&pMap[index.GetIndexOffset()], (TADDR)((marker << 2) | 0x3)); +} + void InitializeThreadStaticData() { CONTRACTL @@ -459,13 +484,13 @@ void FreeThreadStaticData(Thread* pThread) for (int32_t iTlsSlot = 0; iTlsSlot < oldCollectibleTlsDataCount; ++iTlsSlot) { - if (!IsHandleNullUnchecked(pOldCollectibleTlsArrayData[iTlsSlot])) + if (pOldCollectibleTlsArrayData[iTlsSlot] != nullptr) { DestroyLongWeakHandle(pOldCollectibleTlsArrayData[iTlsSlot]); } } - delete[] (uint8_t*)pOldCollectibleTlsArrayData; + delete[] pOldCollectibleTlsArrayData; while (pOldInFlightData != NULL) { @@ -589,7 +614,7 @@ void* GetThreadLocalStaticBase(TLSIndex index) OBJECTHANDLE* pCollectibleTlsArrayData = t_ThreadStatics.pCollectibleTlsArrayData; pCollectibleTlsArrayData += index.GetIndexOffset(); OBJECTHANDLE objHandle = *pCollectibleTlsArrayData; - if (IsHandleNullUnchecked(objHandle)) + if (objHandle == nullptr) { objHandle = GetAppDomain()->CreateLongWeakHandle(NULL); *pCollectibleTlsArrayData = objHandle; @@ -608,7 +633,10 @@ void* GetThreadLocalStaticBase(TLSIndex index) if (pInFlightData->tlsIndex == index) { gcBaseAddresses.pTLSBaseAddress = dac_cast(OBJECTREFToObject(ObjectFromHandle(pInFlightData->hTLSData))); - if (pMT->IsClassInited()) + // A reused collectible index can match an entry whose weak target has been collected. + // Remove empty entries even if the new class is still initializing. Allocation below + // must use a fresh node, not a deleted entry or one that is already linked. + if (pMT->IsClassInited() || gcBaseAddresses.pTLSBaseAddress == (TADDR)NULL) { { SpinLockHolder spinLock(&t_ThreadStatics.pThread->m_TlsSpinLock); @@ -616,6 +644,7 @@ void* GetThreadLocalStaticBase(TLSIndex index) *ppOldNextPtr = pInFlightData->pNext; } delete pInFlightData; + pInFlightData = nullptr; } break; } @@ -653,12 +682,11 @@ void* GetThreadLocalStaticBase(TLSIndex index) } NewHolder pNewInFlightData = NULL; - if (!pMT->IsClassInited() && pInFlightData == NULL) + if (!pMT->IsClassInited()) { pNewInFlightData = new InFlightTLSData(index); HandleType handleType = staticIsNonCollectible ? HNDTYPE_STRONG : HNDTYPE_WEAK_LONG; pNewInFlightData->hTLSData = GetAppDomain()->CreateTypedHandle(gc.tlsEntry, handleType); - pInFlightData = pNewInFlightData; } if (isCollectible) @@ -670,21 +698,21 @@ void* GetThreadLocalStaticBase(TLSIndex index) } // After this, we cannot fail - pNewInFlightData.SuppressRelease(); { GCX_FORBID(); gcBaseAddresses.pTLSBaseAddress = (TADDR)OBJECTREFToObject(gc.tlsEntry); - if (pInFlightData == NULL) + if (pNewInFlightData == NULL) { SetTLSBaseValue(gcBaseAddresses.ppTLSBaseAddress, gcBaseAddresses.pTLSBaseAddress, staticIsNonCollectible); } else { SpinLockHolder spinLock(&t_ThreadStatics.pThread->m_TlsSpinLock); - pInFlightData->pNext = t_ThreadStatics.pInFlightData; - StoreObjectInHandle(pInFlightData->hTLSData, gc.tlsEntry); - t_ThreadStatics.pInFlightData = pInFlightData; + pNewInFlightData->pNext = t_ThreadStatics.pInFlightData; + StoreObjectInHandle(pNewInFlightData->hTLSData, gc.tlsEntry); + t_ThreadStatics.pInFlightData = pNewInFlightData; + pNewInFlightData.SuppressRelease(); } } GCPROTECT_END(); @@ -767,15 +795,27 @@ void GetTLSIndexForThreadStatic(MethodTable* pMT, bool gcStatic, TLSIndex* pInde } else { + bool allocatedNewTLSIndex = false; if (!g_pThreadStaticCollectibleTypeIndices->FindClearedIndex(&newTLSIndex)) { uint32_t tlsRawIndex = g_NextTLSSlot; newTLSIndex = TLSIndex(TLSIndexType::Collectible, tlsRawIndex); - g_NextTLSSlot += 1; + allocatedNewTLSIndex = true; } + SArray& tlsIndexList = pMT->GetLoaderAllocator()->GetTLSIndexList(); + tlsIndexList.Preallocate(tlsIndexList.GetCount() + 1); + + // Set can still fail while growing the global map, but it does so before publishing + // the MethodTable. Once Set succeeds, the reserved list capacity makes Append non-failing, + // so every published collectible index is tracked for loader allocator cleanup. g_pThreadStaticCollectibleTypeIndices->Set(newTLSIndex, pMT, gcStatic); - pMT->GetLoaderAllocator()->GetTLSIndexList().Append(newTLSIndex); + _ASSERTE(tlsIndexList.GetAllocation() > tlsIndexList.GetCount()); + tlsIndexList.Append(newTLSIndex); + if (allocatedNewTLSIndex) + { + g_NextTLSSlot += 1; + } } pIndex->VolatileStore(newTLSIndex); // Use a volatile store so that any other thread that sees the allocated index will also see the writes throughout this path. @@ -800,9 +840,109 @@ void FreeTLSIndicesForLoaderAllocator(LoaderAllocator *pLoaderAllocator) while (current != end) { - g_pThreadStaticCollectibleTypeIndices->Clear(tlsIndicesToCleanup[current], 0); + g_pThreadStaticCollectibleTypeIndices->Clear(tlsIndicesToCleanup[current], RetiredTLSIndexMarker); ++current; } + + g_hasRetiredTLSIndices.Store(true); +} + +void CleanupRetiredTLSIndices() +{ + CONTRACTL + { + NOTHROW; + GC_NOTRIGGER; + MODE_ANY; + CAN_TAKE_LOCK; + } + CONTRACTL_END; + + if (!g_hasRetiredTLSIndices.Load()) + { + return; + } + + _ASSERTE(ThreadStore::HoldingThreadStore() || IsAtProcessExit()); + + CrstHolder ch(&g_TLSCrst); + + if (!g_hasRetiredTLSIndices.Load()) + { + return; + } + + for (const TLSIndexToMethodTableMap::entry& entry : *g_pThreadStaticCollectibleTypeIndices) + { + if (!entry.IsClearedValue || entry.ClearedMarker != RetiredTLSIndexMarker) + { + continue; + } + + int32_t indexOffset = entry.TlsIndex.GetIndexOffset(); + Thread* pThread = nullptr; + while ((pThread = ThreadStore::GetAllThreadList(pThread, 0, 0)) != nullptr) + { + ThreadLocalData* pThreadLocalData = pThread->GetThreadLocalDataPtr(); + if (pThreadLocalData == nullptr && indexOffset >= pThread->cLoaderHandles) + { + continue; + } + + OBJECTHANDLE hTlsData = nullptr; + InFlightTLSData* pRemovedInFlightData = nullptr; + + { + SpinLockHolder spinLock(&pThread->m_TlsSpinLock); + + if (indexOffset < pThread->cLoaderHandles) + { + pThread->pLoaderHandles[indexOffset] = (LOADERHANDLE)nullptr; + } + + if (pThreadLocalData != nullptr) + { + if (indexOffset < pThreadLocalData->cCollectibleTlsData) + { + hTlsData = pThreadLocalData->pCollectibleTlsArrayData[indexOffset]; + pThreadLocalData->pCollectibleTlsArrayData[indexOffset] = nullptr; + } + + InFlightTLSData** ppInFlightData = &pThreadLocalData->pInFlightData; + while (*ppInFlightData != nullptr) + { + InFlightTLSData* pInFlightData = *ppInFlightData; + if (pInFlightData->tlsIndex == entry.TlsIndex) + { + *ppInFlightData = pInFlightData->pNext; + pInFlightData->pNext = pRemovedInFlightData; + pRemovedInFlightData = pInFlightData; + } + else + { + ppInFlightData = &pInFlightData->pNext; + } + } + } + } + + if (hTlsData != nullptr) + { + DestroyLongWeakHandle(hTlsData); + } + + while (pRemovedInFlightData != nullptr) + { + InFlightTLSData* pInFlightData = pRemovedInFlightData; + pRemovedInFlightData = pInFlightData->pNext; + delete pInFlightData; + } + } + + g_pThreadStaticCollectibleTypeIndices->SetClearedMarker(entry.TlsIndex, ReusableTLSIndexMarker); + } + + g_hasRetiredTLSIndices.Store(false); } static void* GetTlsIndexObjectAddress(); @@ -1210,7 +1350,7 @@ void GetThreadLocalStaticBlocksInfo(CORINFO_THREAD_STATIC_BLOCKS_INFO* pInfo) void EnumThreadMemoryRegions(ThreadLocalData *pThreadLocalData, CLRDataEnumMemoryFlags flags) { SUPPORTS_DAC; - DacEnumMemoryRegion(dac_cast(pThreadLocalData->pCollectibleTlsArrayData), pThreadLocalData->cCollectibleTlsData, flags); + DacEnumMemoryRegion(dac_cast(pThreadLocalData->pCollectibleTlsArrayData), pThreadLocalData->cCollectibleTlsData * sizeof(OBJECTHANDLE), flags); PTR_InFlightTLSData pInFlightData = pThreadLocalData->pInFlightData; while (pInFlightData != NULL) { diff --git a/src/coreclr/vm/threadstatics.h b/src/coreclr/vm/threadstatics.h index 0a77a356e33db4..9059a1f2187de0 100644 --- a/src/coreclr/vm/threadstatics.h +++ b/src/coreclr/vm/threadstatics.h @@ -148,6 +148,10 @@ class TLSIndexToMethodTableMap if (index.GetIndexOffset() < VolatileLoad(&m_maxIndex)) { TADDR rawValue = VolatileLoadWithoutBarrier(&VolatileLoad(&pMap)[index.GetIndexOffset()]); + if (IsClearedValue(rawValue)) + { + return NULL; + } return (PTR_MethodTable)UnwrapValue(rawValue); } return NULL; @@ -326,6 +330,7 @@ class TLSIndexToMethodTableMap void Set(TLSIndex index, PTR_MethodTable pMT, bool isGCStatic); bool FindClearedIndex(TLSIndex* pIndex); void Clear(TLSIndex index, uint8_t whenCleared); + void SetClearedMarker(TLSIndex index, uint8_t marker); #endif // !DACCESS_COMPILE #ifdef DACCESS_COMPILE @@ -353,6 +358,7 @@ void FreeThreadStaticData(Thread* pThread); void AssertThreadStaticDataFreed(); void GetTLSIndexForThreadStatic(MethodTable* pMT, bool gcStatic, TLSIndex* pIndex, uint32_t bytesNeeded); void FreeTLSIndicesForLoaderAllocator(LoaderAllocator *pLoaderAllocator); +void CleanupRetiredTLSIndices(); void* GetThreadLocalStaticBase(TLSIndex index); void GetThreadLocalStaticBlocksInfo (CORINFO_THREAD_STATIC_BLOCKS_INFO* pInfo); bool CanJITOptimizeTLSAccess(); diff --git a/src/coreclr/vm/threadsuspend.cpp b/src/coreclr/vm/threadsuspend.cpp index 9d2933f1a3b942..0658d099151a9f 100644 --- a/src/coreclr/vm/threadsuspend.cpp +++ b/src/coreclr/vm/threadsuspend.cpp @@ -5394,10 +5394,13 @@ void ThreadSuspend::RestartEE(BOOL SuspendSucceeded) // // SyncClean::CleanUp reclaims resources that are safe to free only - // when no threads are running managed code. Since the EE is - // suspended at this point, we know it's safe to clean up here. + // when no threads are running managed code, so it can only run after + // the EE was successfully suspended. // - SyncClean::CleanUp(); + if (SuspendSucceeded) + { + SyncClean::CleanUp(); + } #ifdef PROFILING_SUPPORTED // If a profiler is keeping track suspend events, notify it. This notification diff --git a/src/tests/Loader/CollectibleAssemblies/Statics/CollectibleTLSStaticCollection.cs b/src/tests/Loader/CollectibleAssemblies/Statics/CollectibleTLSStaticCollection.cs index 7f4df95c086e2f..7b295d989ad619 100644 --- a/src/tests/Loader/CollectibleAssemblies/Statics/CollectibleTLSStaticCollection.cs +++ b/src/tests/Loader/CollectibleAssemblies/Statics/CollectibleTLSStaticCollection.cs @@ -2,10 +2,12 @@ // The .NET Foundation licenses this file to you under the MIT license. using System; +using System.IO; using System.Reflection; using System.Reflection.Emit; using System.Runtime.CompilerServices; using System.Runtime.InteropServices; +using System.Runtime.Loader; using System.Threading; using Xunit; using TestLibrary; @@ -112,6 +114,47 @@ void ForceCollectibleTLSStaticToGoThroughThreadTermination() } + [MethodImpl(MethodImplOptions.NoInlining)] + private static WeakReference RunInCollectibleAlc(string assemblyPath, string typeName, string methodName) + { + AssemblyLoadContext context = new AssemblyLoadContext(typeName, isCollectible: true); + Assembly assembly = context.LoadFromAssemblyPath(assemblyPath); + assembly.GetType(typeName)!.GetMethod(methodName)!.Invoke(null, null); + context.Unload(); + return new WeakReference(context); + } + + [ActiveIssue("https://github.com/dotnet/runtimelab/issues/155: Collectible assemblies", typeof(Utilities), nameof(Utilities.IsNativeAot))] + [ActiveIssue("https://github.com/dotnet/runtime/issues/40394", TestRuntimes.Mono)] + [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsMultithreadingSupported))] + public static void ReuseCollectibleThreadStaticIndex() + { + string assemblyPath = Path.Combine(AppContext.BaseDirectory, "StaticsUnloaded.dll"); + WeakReference context = RunInCollectibleAlc( + assemblyPath, + nameof(ThreadStaticInFlightDataBeforeUnload), + nameof(ThreadStaticInFlightDataBeforeUnload.Touch)); + + for (int i = 0; i < 100 && context.IsAlive; i++) + { + GC.Collect(); + GC.WaitForPendingFinalizers(); + } + + // Native LoaderAllocator cleanup must finish before its TLS index can be reused. + for (int i = 0; i < 20; i++) + { + GC.Collect(); + GC.WaitForPendingFinalizers(); + Thread.Sleep(10); + } + + RunInCollectibleAlc( + assemblyPath, + nameof(ThreadStaticInFlightDataAfterUnload), + nameof(ThreadStaticInFlightDataAfterUnload.Set)); + } + [ActiveIssue("https://github.com/dotnet/runtimelab/issues/155: Collectible assemblies", typeof(Utilities), nameof(Utilities.IsNativeAot))] [ActiveIssue("https://github.com/dotnet/runtime/issues/40394", TestRuntimes.Mono)] [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsMultithreadingSupported))] diff --git a/src/tests/Loader/CollectibleAssemblies/Statics/CollectibleTLSStaticCollection.csproj b/src/tests/Loader/CollectibleAssemblies/Statics/CollectibleTLSStaticCollection.csproj index 287971ce4ffb93..cd07534404c394 100644 --- a/src/tests/Loader/CollectibleAssemblies/Statics/CollectibleTLSStaticCollection.csproj +++ b/src/tests/Loader/CollectibleAssemblies/Statics/CollectibleTLSStaticCollection.csproj @@ -5,6 +5,7 @@ true + diff --git a/src/tests/Loader/CollectibleAssemblies/Statics/StaticsUnloaded.cs b/src/tests/Loader/CollectibleAssemblies/Statics/StaticsUnloaded.cs index 447b8d9a42e2ac..593ce61cb2be98 100644 --- a/src/tests/Loader/CollectibleAssemblies/Statics/StaticsUnloaded.cs +++ b/src/tests/Loader/CollectibleAssemblies/Statics/StaticsUnloaded.cs @@ -4,6 +4,35 @@ using System; using System.Runtime.CompilerServices; +public static class ThreadStaticInFlightDataBeforeUnload +{ + [ThreadStatic] + private static object s_value; + + static ThreadStaticInFlightDataBeforeUnload() + { + s_value = new object(); + } + + [MethodImpl(MethodImplOptions.NoInlining)] + public static void Touch() + { + } +} + +public static class ThreadStaticInFlightDataAfterUnload +{ + [ThreadStatic] + private static object s_value; + + [MethodImpl(MethodImplOptions.NoInlining)] + public static void Set() + { + s_value = new object(); + GC.KeepAlive(s_value); + } +} + // Use multiple classes to trigger multiple statics allocation events public class StaticTest2 {