Skip to content

Commit

Permalink
Detect class inited status more correctly in prestub/jitinterface (#1…
Browse files Browse the repository at this point in the history 
…04253)

* Fix #104247 by providing a new helper function which means what the old IsClassInited helper meant
- AND a very subtle race condition where we need to set the IsClassInited flag bit on dynamic statics BEFORE setting the MethodTable level one
  • Loading branch information
@davidwrighton
davidwrighton authored Jul 3, 2024
1 parent 2ea6ae5 commit 015b7ed
Show file tree
Hide file tree
Showing 8 changed files with 97 additions and 50 deletions.
6 changes: 3 additions & 3 deletions src/coreclr/vm/appdomain.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -672,7 +672,7 @@ void AppDomain::SetNativeDllSearchDirectories(LPCWSTR wszNativeDllSearchDirector
}
}

OBJECTREF* BaseDomain::AllocateObjRefPtrsInLargeTable(int nRequested, DynamicStaticsInfo* pStaticsInfo, MethodTable *pMTToFillWithStaticBoxes)
OBJECTREF* BaseDomain::AllocateObjRefPtrsInLargeTable(int nRequested, DynamicStaticsInfo* pStaticsInfo, MethodTable *pMTToFillWithStaticBoxes, bool isClassInitdeByUpdatingStaticPointer)
{
CONTRACTL
{
Expand Down Expand Up @@ -707,7 +707,7 @@ OBJECTREF* BaseDomain::AllocateObjRefPtrsInLargeTable(int nRequested, DynamicSta
if (pStaticsInfo)
{
// race with other threads that might be doing the same concurrent allocation
if (!pStaticsInfo->InterlockedUpdateStaticsPointer(/*isGCPointer*/ true, (TADDR)result))
if (!pStaticsInfo->InterlockedUpdateStaticsPointer(/*isGCPointer*/ true, (TADDR)result, isClassInitdeByUpdatingStaticPointer))
{
// we lost the race, release our handles and use the handles from the
// winning thread
Expand Down Expand Up @@ -2930,7 +2930,7 @@ void AppDomain::SetupSharedStatics()

FieldDesc * pEmptyStringFD = CoreLibBinder::GetField(FIELD__STRING__EMPTY);
OBJECTREF* pEmptyStringHandle = (OBJECTREF*)
((TADDR)g_pStringClass->GetDynamicStaticsInfo()->m_pGCStatics+pEmptyStringFD->GetOffset());
((TADDR)g_pStringClass->GetDynamicStaticsInfo()->GetGCStaticsPointer()+pEmptyStringFD->GetOffset());
SetObjectReference( pEmptyStringHandle, StringObject::GetEmptyString());
}

Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/vm/appdomain.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -522,7 +522,7 @@ class BaseDomain
// Statics and reflection info (Types, MemberInfo,..) are stored this way
// If pStaticsInfo != 0, allocation will only take place if GC statics in the DynamicStaticsInfo are NULL (and the allocation
// will be properly serialized)
OBJECTREF *AllocateObjRefPtrsInLargeTable(int nRequested, DynamicStaticsInfo* pStaticsInfo = NULL, MethodTable *pMTToFillWithStaticBoxes = NULL);
OBJECTREF *AllocateObjRefPtrsInLargeTable(int nRequested, DynamicStaticsInfo* pStaticsInfo = NULL, MethodTable *pMTToFillWithStaticBoxes = NULL, bool isClassInitdeByUpdatingStaticPointer = false);

//****************************************************************************************
// Handles
Expand Down
15 changes: 6 additions & 9 deletions src/coreclr/vm/jitinterface.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1210,10 +1210,9 @@ CorInfoHelpFunc CEEInfo::getSharedStaticsHelper(FieldDesc * pField, MethodTable
{
STANDARD_VM_CONTRACT;

pFieldMT->AttemptToPreinit();
bool GCStatic = (pField->GetFieldType() == ELEMENT_TYPE_CLASS ||
pField->GetFieldType() == ELEMENT_TYPE_VALUETYPE);
bool noCtor = pFieldMT->IsClassInited();
bool noCtor = pFieldMT->IsClassInitedOrPreinited();
bool threadStatic = pField->IsThreadStatic();
bool isInexactMT = pFieldMT->IsSharedByGenericInstantiations();
bool isCollectible = pFieldMT->Collectible();
Expand Down Expand Up @@ -1451,7 +1450,7 @@ void CEEInfo::getFieldInfo (CORINFO_RESOLVED_TOKEN * pResolvedToken,
}

// We are not going through a helper. The constructor has to be triggered explicitly.
if (!pFieldMT->IsClassInited())
if (!pFieldMT->IsClassInitedOrPreinited())
fieldFlags |= CORINFO_FLG_FIELD_INITCLASS;
}
else
Expand Down Expand Up @@ -1536,10 +1535,9 @@ void CEEInfo::getFieldInfo (CORINFO_RESOLVED_TOKEN * pResolvedToken,
// Allocate space for the local class if necessary, but don't trigger
// class construction.
pFieldMT->EnsureStaticDataAllocated();
pFieldMT->AttemptToPreinit();

// We are not going through a helper. The constructor has to be triggered explicitly.
if (!pFieldMT->IsClassInited())
if (!pFieldMT->IsClassInitedOrPreinited())
fieldFlags |= CORINFO_FLG_FIELD_INITCLASS;

GCX_COOP();
Expand Down Expand Up @@ -3884,8 +3882,7 @@ CorInfoInitClassResult CEEInfo::initClass(

MethodTable *pTypeToInitMT = typeToInitTH.AsMethodTable();

pTypeToInitMT->AttemptToPreinit();
if (pTypeToInitMT->IsClassInited())
if (pTypeToInitMT->IsClassInitedOrPreinited())
{
// If the type is initialized there really is nothing to do.
result = CORINFO_INITCLASS_INITIALIZED;
Expand Down Expand Up @@ -11726,7 +11723,7 @@ bool CEEInfo::getStaticFieldContent(CORINFO_FIELD_HANDLE fieldHnd, uint8_t* buff
// class construction.
pEnclosingMT->EnsureStaticDataAllocated();

if (!field->IsThreadStatic() && pEnclosingMT->IsClassInited() && IsFdInitOnly(field->GetAttributes()))
if (!field->IsThreadStatic() && pEnclosingMT->IsClassInitedOrPreinited() && IsFdInitOnly(field->GetAttributes()))
{
if (field->IsObjRef())
{
Expand Down Expand Up @@ -11914,7 +11911,7 @@ CORINFO_CLASS_HANDLE CEEJitInfo::getStaticFieldCurrentClass(CORINFO_FIELD_HANDLE
VALIDATEOBJECTREF(fieldObj);

// Check for initialization before looking at the value
isClassInitialized = !!pEnclosingMT->IsClassInited();
isClassInitialized = !!pEnclosingMT->IsClassInitedOrPreinited();

if (fieldObj != NULL)
{
Expand Down
12 changes: 6 additions & 6 deletions src/coreclr/vm/loaderallocator.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2254,7 +2254,7 @@ PTR_OnStackReplacementManager LoaderAllocator::GetOnStackReplacementManager()
#endif // FEATURE_ON_STACK_REPLACEMENT

#ifndef DACCESS_COMPILE
void LoaderAllocator::AllocateBytesForStaticVariables(DynamicStaticsInfo* pStaticsInfo, uint32_t cbMem)
void LoaderAllocator::AllocateBytesForStaticVariables(DynamicStaticsInfo* pStaticsInfo, uint32_t cbMem, bool isClassInitedByUpdatingStaticPointer)
{
CONTRACTL
{
Expand Down Expand Up @@ -2294,7 +2294,7 @@ void LoaderAllocator::AllocateBytesForStaticVariables(DynamicStaticsInfo* pStati
WeakInteriorHandleHolder weakHandleHolder = GetAppDomain()->CreateWeakInteriorHandle(ptrArray, &pStaticsInfo->m_pNonGCStatics);
RegisterHandleForCleanupLocked(weakHandleHolder.GetValue());
weakHandleHolder.SuppressRelease();
bool didUpdateStaticsPointer = pStaticsInfo->InterlockedUpdateStaticsPointer(/* isGCPointer */false, (TADDR)ptrArray->GetDataPtr());
bool didUpdateStaticsPointer = pStaticsInfo->InterlockedUpdateStaticsPointer(/* isGCPointer */false, (TADDR)ptrArray->GetDataPtr(), isClassInitedByUpdatingStaticPointer);
_ASSERTE(didUpdateStaticsPointer);
}
}
Expand Down Expand Up @@ -2324,11 +2324,11 @@ void LoaderAllocator::AllocateBytesForStaticVariables(DynamicStaticsInfo* pStati
pbMem = (uint8_t*)ALIGN_UP(pbMem, 8);
}
#endif
pStaticsInfo->InterlockedUpdateStaticsPointer(/* isGCPointer */false, (TADDR)pbMem);
pStaticsInfo->InterlockedUpdateStaticsPointer(/* isGCPointer */false, (TADDR)pbMem, isClassInitedByUpdatingStaticPointer);
}
}

void LoaderAllocator::AllocateGCHandlesBytesForStaticVariables(DynamicStaticsInfo* pStaticsInfo, uint32_t cSlots, MethodTable* pMTToFillWithStaticBoxes)
void LoaderAllocator::AllocateGCHandlesBytesForStaticVariables(DynamicStaticsInfo* pStaticsInfo, uint32_t cSlots, MethodTable* pMTToFillWithStaticBoxes, bool isClassInitedByUpdatingStaticPointer)
{
CONTRACTL
{
Expand Down Expand Up @@ -2374,15 +2374,15 @@ void LoaderAllocator::AllocateGCHandlesBytesForStaticVariables(DynamicStaticsInf
WeakInteriorHandleHolder weakHandleHolder = GetAppDomain()->CreateWeakInteriorHandle(ptrArray, &pStaticsInfo->m_pGCStatics);
RegisterHandleForCleanupLocked(weakHandleHolder.GetValue());
weakHandleHolder.SuppressRelease();
bool didUpdateStaticsPointer = pStaticsInfo->InterlockedUpdateStaticsPointer(/* isGCPointer */true, (TADDR)ptrArray->GetDataPtr());
bool didUpdateStaticsPointer = pStaticsInfo->InterlockedUpdateStaticsPointer(/* isGCPointer */true, (TADDR)ptrArray->GetDataPtr(), isClassInitedByUpdatingStaticPointer);
_ASSERTE(didUpdateStaticsPointer);
}
}
GCPROTECT_END();
}
else
{
GetDomain()->AllocateObjRefPtrsInLargeTable(cSlots, pStaticsInfo, pMTToFillWithStaticBoxes);
GetDomain()->AllocateObjRefPtrsInLargeTable(cSlots, pStaticsInfo, pMTToFillWithStaticBoxes, isClassInitedByUpdatingStaticPointer);
}
}
#endif // !DACCESS_COMPILE
Expand Down
4 changes: 2 additions & 2 deletions src/coreclr/vm/loaderallocator.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -761,8 +761,8 @@ class LoaderAllocator
LIMITED_METHOD_CONTRACT;
return m_nGCCount;
}
void AllocateBytesForStaticVariables(DynamicStaticsInfo* pStaticsInfo, uint32_t cbMem);
void AllocateGCHandlesBytesForStaticVariables(DynamicStaticsInfo* pStaticsInfo, uint32_t cSlots, MethodTable* pMTWithStaticBoxes);
void AllocateBytesForStaticVariables(DynamicStaticsInfo* pStaticsInfo, uint32_t cbMem, bool isClassInitedByUpdatingStaticPointer);
void AllocateGCHandlesBytesForStaticVariables(DynamicStaticsInfo* pStaticsInfo, uint32_t cSlots, MethodTable* pMTWithStaticBoxes, bool isClassInitedByUpdatingStaticPointer);

static BOOL Destroy(QCall::LoaderAllocatorHandle pLoaderAllocator);

Expand Down
59 changes: 40 additions & 19 deletions src/coreclr/vm/methodtable.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3782,20 +3782,42 @@ void MethodTable::EnsureStaticDataAllocated()
CONTRACTL_END;

PTR_MethodTableAuxiliaryData pAuxiliaryData = GetAuxiliaryDataForWrite();
if (!pAuxiliaryData->IsStaticDataAllocated() && IsDynamicStatics())
if (!pAuxiliaryData->IsStaticDataAllocated())
{
DynamicStaticsInfo *pDynamicStaticsInfo = GetDynamicStaticsInfo();
// Allocate space for normal statics if we might have them
if (pDynamicStaticsInfo->GetNonGCStaticsPointer() == NULL)
GetLoaderAllocator()->AllocateBytesForStaticVariables(pDynamicStaticsInfo, GetClass()->GetNonGCRegularStaticFieldBytes());
bool isInitedIfStaticDataAllocated = IsInitedIfStaticDataAllocated();
if (IsDynamicStatics() && !IsSharedByGenericInstantiations())
{
DynamicStaticsInfo *pDynamicStaticsInfo = GetDynamicStaticsInfo();
// Allocate space for normal statics if we might have them
if (pDynamicStaticsInfo->GetNonGCStaticsPointer() == NULL)
GetLoaderAllocator()->AllocateBytesForStaticVariables(pDynamicStaticsInfo, GetClass()->GetNonGCRegularStaticFieldBytes(), isInitedIfStaticDataAllocated);

if (pDynamicStaticsInfo->GetGCStaticsPointer() == NULL)
GetLoaderAllocator()->AllocateGCHandlesBytesForStaticVariables(pDynamicStaticsInfo, GetClass()->GetNumHandleRegularStatics(), this->HasBoxedRegularStatics() ? this : NULL);
if (pDynamicStaticsInfo->GetGCStaticsPointer() == NULL)
GetLoaderAllocator()->AllocateGCHandlesBytesForStaticVariables(pDynamicStaticsInfo, GetClass()->GetNumHandleRegularStatics(), this->HasBoxedRegularStatics() ? this : NULL, isInitedIfStaticDataAllocated);
}
pAuxiliaryData->SetIsStaticDataAllocated(isInitedIfStaticDataAllocated);
}
pAuxiliaryData->SetIsStaticDataAllocated();
}

void MethodTable::AttemptToPreinit()
bool MethodTable::IsClassInitedOrPreinited()
{
CONTRACTL
{
THROWS;
GC_TRIGGERS;
INJECT_FAULT(COMPlusThrowOM());
}
CONTRACTL_END;

bool initResult;
if (GetAuxiliaryData()->IsClassInitedOrPreinitedDecided(&initResult))
return initResult;

EnsureStaticDataAllocated();
return IsClassInited();
}

bool MethodTable::IsInitedIfStaticDataAllocated()
{
CONTRACTL
{
Expand All @@ -3806,33 +3828,32 @@ void MethodTable::AttemptToPreinit()
CONTRACTL_END;

if (IsClassInited())
return;
{
return true;
}

if (HasClassConstructor())
{
// If there is a class constructor, then the class cannot be preinitted.
return;
return false;
}

if (GetClass()->GetNonGCRegularStaticFieldBytes() == 0 && GetClass()->GetNumHandleRegularStatics() == 0)
{
// If there are static fields that are not thread statics, then the class is preinitted.
SetClassInited();
return;
// If there aren't static fields that are not thread statics, then the class is preinitted.
return true;
}

// At this point, we are looking at a class that has no class constructor, but does have static fields

if (IsSharedByGenericInstantiations())
{
// If we don't know the exact type, we can't pre-allocate the fields
return;
// If we don't know the exact type, we can't pre-init the the fields
return false;
}

// All this class needs to be initialized is to allocate the memory for the static fields. Do so, and mark the type as initialized
EnsureStaticDataAllocated();
SetClassInited();
return;
return true;
}

void MethodTable::EnsureTlsIndexAllocated()
Expand Down
44 changes: 37 additions & 7 deletions src/coreclr/vm/methodtable.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -337,7 +337,7 @@ struct MethodTableAuxiliaryData
enum_flag_DependenciesLoaded = 0x0080, // class and all dependencies loaded up to CLASS_LOADED_BUT_NOT_VERIFIED

enum_flag_IsInitError = 0x0100,
enum_flag_IsStaticDataAllocated = 0x0200,
enum_flag_IsStaticDataAllocated = 0x0200, // When this is set, if the class can be marked as initialized without any further code execution it will be.
// unum_unused = 0x0400,
enum_flag_IsTlsIndexAllocated = 0x0800,
enum_flag_MayHaveOpenInterfaceInInterfaceMap = 0x1000,
Expand Down Expand Up @@ -447,9 +447,19 @@ struct MethodTableAuxiliaryData

inline BOOL IsClassInited() const
{
LIMITED_METHOD_DAC_CONTRACT;
return VolatileLoad(&m_dwFlags) & enum_flag_Initialized;
}

inline bool IsClassInitedOrPreinitedDecided(bool *initResult) const
{
LIMITED_METHOD_DAC_CONTRACT;

DWORD dwFlags = VolatileLoad(&m_dwFlags);
*initResult = m_dwFlags & enum_flag_Initialized;
return (dwFlags & (enum_flag_IsStaticDataAllocated|enum_flag_Initialized)) != 0;
}

#ifndef DACCESS_COMPILE
inline void SetClassInited()
{
Expand All @@ -465,10 +475,10 @@ struct MethodTableAuxiliaryData
}

#ifndef DACCESS_COMPILE
inline void SetIsStaticDataAllocated()
inline void SetIsStaticDataAllocated(bool markAsInitedToo)
{
LIMITED_METHOD_CONTRACT;
InterlockedOr((LONG*)&m_dwFlags, (LONG)enum_flag_IsStaticDataAllocated);
InterlockedOr((LONG*)&m_dwFlags, markAsInitedToo ? (LONG)(enum_flag_IsStaticDataAllocated|enum_flag_Initialized) : (LONG)enum_flag_IsStaticDataAllocated);
}
#endif

Expand Down Expand Up @@ -567,7 +577,7 @@ struct DynamicStaticsInfo
bool GetIsInitedAndNonGCStaticsPointerIfInited(PTR_BYTE *ptrResult) { TADDR staticsVal = VolatileLoadWithoutBarrier(&m_pNonGCStatics); *ptrResult = dac_cast<PTR_BYTE>(staticsVal); return !(staticsVal & ISCLASSNOTINITED); }

// This function sets the pointer portion of a statics pointer. It returns false if the statics value was already set.
bool InterlockedUpdateStaticsPointer(bool isGC, TADDR newVal)
bool InterlockedUpdateStaticsPointer(bool isGC, TADDR newVal, bool isClassInitedByUpdatingStaticPointer)
{
TADDR oldVal;
TADDR oldValFromInterlockedOp;
Expand All @@ -583,7 +593,14 @@ struct DynamicStaticsInfo
return false;
}

oldValFromInterlockedOp = InterlockedCompareExchangeT(pAddr, newVal | oldVal, oldVal);
if (isClassInitedByUpdatingStaticPointer)
{
oldValFromInterlockedOp = InterlockedCompareExchangeT(pAddr, newVal, oldVal);
}
else
{
oldValFromInterlockedOp = InterlockedCompareExchangeT(pAddr, newVal | oldVal, oldVal);
}
} while(oldValFromInterlockedOp != oldVal);
return true;
}
Expand Down Expand Up @@ -1042,18 +1059,31 @@ class MethodTable
#ifndef DACCESS_COMPILE
void SetClassInited()
{
GetAuxiliaryDataForWrite()->SetClassInited();
// This must be before setting the MethodTable level flag, as otherwise there is a race condition where
// the MethodTable flag is set, which would allows the JIT to generate a call to a helper which assumes
// the DynamicStaticInfo level flag is set.
// The other race in the other direction is not a concern, as it can only cause allows reads/write from the static
// fields, which are effectively inited in any case once we reach this point.
if (IsDynamicStatics())
{
GetDynamicStaticsInfo()->SetClassInited();
}
GetAuxiliaryDataForWrite()->SetClassInited();
}

void AttemptToPreinit();
private:
bool IsInitedIfStaticDataAllocated();
public:
// Is the MethodTable current initialized, and/or can the runtime initialize the MethodTable
// without running any user code. (This function may allocate memory, and may throw OutOfMemory)
bool IsClassInitedOrPreinited();
#endif

// Is the MethodTable current known to be initialized
// If you want to know if it is initialized and allocation/throwing is permitted, call IsClassInitedOrPreinited instead
BOOL IsClassInited()
{
LIMITED_METHOD_DAC_CONTRACT;
return GetAuxiliaryDataForWrite()->IsClassInited();
}

Expand Down
5 changes: 2 additions & 3 deletions src/coreclr/vm/prestub.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3545,9 +3545,8 @@ static PCODE getHelperForStaticBase(Module * pModule, CORCOMPILE_FIXUP_BLOB_KIND
{
STANDARD_VM_CONTRACT;

pMT->AttemptToPreinit();
bool GCStatic = (kind == ENCODE_STATIC_BASE_GC_HELPER || kind == ENCODE_THREAD_STATIC_BASE_GC_HELPER);
bool noCtor = pMT->IsClassInited();
bool noCtor = pMT->IsClassInitedOrPreinited();
bool threadStatic = (kind == ENCODE_THREAD_STATIC_BASE_NONGC_HELPER || kind == ENCODE_THREAD_STATIC_BASE_GC_HELPER);

CorInfoHelpFunc helper;
Expand Down Expand Up @@ -3916,7 +3915,7 @@ PCODE DynamicHelperFixup(TransitionBlock * pTransitionBlock, TADDR * pCell, DWOR
else
{
// Delay the creation of the helper until the type is initialized
if (pMT->IsClassInited())
if (pMT->IsClassInitedOrPreinited())
pHelper = getHelperForInitializedStatic(pModule, (CORCOMPILE_FIXUP_BLOB_KIND)kind, pMT, pFD);
}
}
Expand Down

0 comments on commit 015b7ed

Please sign in to comment.