Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix thread static cleanup paths #107438

Merged
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Fix thread static cleanup paths
- Do not destroy GC handles while holding the spin lock
- Free the pLoaderHandle array when the thread is terminated
  • Loading branch information
davidwrighton committed Sep 6, 2024
commit 99a8812f1f3e56f544f281ee45ea594557c123fb
65 changes: 45 additions & 20 deletions src/coreclr/vm/threadstatics.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -405,6 +405,13 @@ void FreeLoaderAllocatorHandlesForTLSData(Thread *pThread)
}
}
}

pThread->cLoaderHandles = -1; // Sentinel value indicating that there are no LoaderHandles and the thread is permanently dead.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Set the sentinel before we start cleaning the entries?

if (pThread->pLoaderHandles != NULL)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: This should be always true since cLoaderHandles > 0 here. Also, the delete can be called outside the lock.

{
delete[] pThread->pLoaderHandles;
pThread->pLoaderHandles = NULL;
}
}
}

Expand All @@ -431,34 +438,48 @@ void FreeThreadStaticData(Thread* pThread)
}
CONTRACTL_END;

SpinLockHolder spinLock(&pThread->m_TlsSpinLock);
InFlightTLSData* pOldInFlightData = nullptr;

ThreadLocalData *pThreadLocalData = &t_ThreadStatics;
int32_t oldCollectibleTlsDataCount = 0;
DPTR(OBJECTHANDLE) pOldCollectibleTlsArrayData = nullptr;

for (int32_t iTlsSlot = 0; iTlsSlot < pThreadLocalData->cCollectibleTlsData; ++iTlsSlot)
{
if (!IsHandleNullUnchecked(pThreadLocalData->pCollectibleTlsArrayData[iTlsSlot]))
SpinLockHolder spinLock(&pThread->m_TlsSpinLock);

ThreadLocalData *pThreadLocalData = &t_ThreadStatics;

pOldCollectibleTlsArrayData = pThreadLocalData->pCollectibleTlsArrayData;
oldCollectibleTlsDataCount = pThreadLocalData->cCollectibleTlsData;

pThreadLocalData->pCollectibleTlsArrayData = 0;
pThreadLocalData->cCollectibleTlsData = 0;
pThreadLocalData->pNonCollectibleTlsArrayData = 0;
pThreadLocalData->cNonCollectibleTlsData = 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
pThreadLocalData->pCollectibleTlsArrayData = 0;
pThreadLocalData->cCollectibleTlsData = 0;
pThreadLocalData->pNonCollectibleTlsArrayData = 0;
pThreadLocalData->cNonCollectibleTlsData = 0;
pThreadLocalData->pCollectibleTlsArrayData = NULL;
pThreadLocalData->cCollectibleTlsData = 0;
pThreadLocalData->pNonCollectibleTlsArrayData = NULL;
pThreadLocalData->cNonCollectibleTlsData = 0;


pOldInFlightData = pThreadLocalData->pInFlightData;
_ASSERTE(pThreadLocalData->pThread == pThread);
pThreadLocalData->pThread = NULL;
}

for (int32_t iTlsSlot = 0; iTlsSlot < oldCollectibleTlsDataCount; ++iTlsSlot)
{
if (!IsHandleNullUnchecked(pOldCollectibleTlsArrayData[iTlsSlot]))
{
DestroyLongWeakHandle(pThreadLocalData->pCollectibleTlsArrayData[iTlsSlot]);
DestroyLongWeakHandle(pOldCollectibleTlsArrayData[iTlsSlot]);
}
}

delete[] (uint8_t*)pThreadLocalData->pCollectibleTlsArrayData;

pThreadLocalData->pCollectibleTlsArrayData = 0;
pThreadLocalData->cCollectibleTlsData = 0;
pThreadLocalData->pNonCollectibleTlsArrayData = 0;
pThreadLocalData->cNonCollectibleTlsData = 0;
if (pOldCollectibleTlsArrayData != NULL)
{
delete[] (uint8_t*)pOldCollectibleTlsArrayData;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (pOldCollectibleTlsArrayData != NULL)
{
delete[] (uint8_t*)pOldCollectibleTlsArrayData;
}
delete[] (uint8_t*)pOldCollectibleTlsArrayData;


while (pThreadLocalData->pInFlightData != NULL)
while (pOldInFlightData != NULL)
{
InFlightTLSData* pInFlightData = pThreadLocalData->pInFlightData;
pThreadLocalData->pInFlightData = pInFlightData->pNext;
InFlightTLSData* pInFlightData = pOldInFlightData;
pOldInFlightData = pInFlightData->pNext;
delete pInFlightData;
}

_ASSERTE(pThreadLocalData->pThread == pThread);
pThreadLocalData->pThread = NULL;
}

void SetTLSBaseValue(TADDR *ppTLSBaseAddress, TADDR pTLSBaseAddress, bool useGCBarrierInsteadOfHandleStore)
Expand Down Expand Up @@ -553,6 +574,8 @@ void* GetThreadLocalStaticBase(TLSIndex index)
delete[] pOldArray;
}

_ASSERTE(t_ThreadStatics.pThread->cLoaderHandles != -1); // Check sentinel value indicating that there are no LoaderHandles, the thread has gone through termination and is permanently dead.

if (isCollectible && t_ThreadStatics.pThread->cLoaderHandles <= index.GetIndexOffset())
{
// Grow the underlying TLS array
Expand Down Expand Up @@ -594,9 +617,11 @@ void* GetThreadLocalStaticBase(TLSIndex index)
gcBaseAddresses.pTLSBaseAddress = dac_cast<TADDR>(OBJECTREFToObject(ObjectFromHandle(pInFlightData->hTLSData)));
if (pMT->IsClassInited())
{
SpinLockHolder spinLock(&t_ThreadStatics.pThread->m_TlsSpinLock);
SetTLSBaseValue(gcBaseAddresses.ppTLSBaseAddress, gcBaseAddresses.pTLSBaseAddress, staticIsNonCollectible);
*ppOldNextPtr = pInFlightData->pNext;
{
SpinLockHolder spinLock(&t_ThreadStatics.pThread->m_TlsSpinLock);
SetTLSBaseValue(gcBaseAddresses.ppTLSBaseAddress, gcBaseAddresses.pTLSBaseAddress, staticIsNonCollectible);
*ppOldNextPtr = pInFlightData->pNext;
}
delete pInFlightData;
}
break;
Expand Down
Loading