From 47c68f2cb50106d18ac2914fbd8aac9334d4d127 Mon Sep 17 00:00:00 2001 From: Aaron Robinson Date: Wed, 21 Oct 2026 10:51:01 -0700 Subject: [PATCH 01/13] Extend SHash traversal and capacity reuse Add index-based traversal, DAC backing-memory enumeration, and optional retained-capacity clearing. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ce4b0890-6559-4b90-b46f-11179e70f973 --- src/coreclr/inc/shash.h | 12 +++++++++-- src/coreclr/inc/shash.inl | 44 +++++++++++++++++++++++++++++++++------ 2 files changed, 48 insertions(+), 8 deletions(-) diff --git a/src/coreclr/inc/shash.h b/src/coreclr/inc/shash.h index 8621acff0674c6..05db8047f39515 100644 --- a/src/coreclr/inc/shash.h +++ b/src/coreclr/inc/shash.h @@ -231,15 +231,23 @@ class EMPTY_BASES SHash : public TRAITS void RemovePtr(element_t * element); - // Remove all elements in the hashtable + // Remove all elements, optionally retaining the backing array for reuse. - void RemoveAll(); + void RemoveAll(bool retainCapacity = false); // Begin and End pointers for iteration over entire table. Iterator Begin() const; Iterator End() const; + // Index-based traversal does not retain a pointer into reallocatable storage. + // The caller must exclude additions while enumerating. + const element_t *GetNext(count_t &index) const; + +#ifdef DACCESS_COMPILE + void EnumMemoryRegions(); +#endif + // Begin and End pointers for iteration over all elements with a given key. KeyIterator Begin(key_t key) const; diff --git a/src/coreclr/inc/shash.inl b/src/coreclr/inc/shash.inl index eb1129bb58bb4a..b0fc4ea20d0446 100644 --- a/src/coreclr/inc/shash.inl +++ b/src/coreclr/inc/shash.inl @@ -52,6 +52,31 @@ SHash::~SHash() delete [] m_table; } +template +const typename SHash::element_t *SHash::GetNext(count_t &index) const +{ + LIMITED_METHOD_DAC_CONTRACT; + + Iterator iterator(this, FALSE); + iterator.m_index = index; + iterator.First(); + if (iterator.m_index >= m_tableSize) + return nullptr; + + index = iterator.m_index + 1; + return &*iterator; +} + +#ifdef DACCESS_COMPILE +template +void SHash::EnumMemoryRegions() +{ + SUPPORTS_DAC; + if (m_table != nullptr) + DacEnumMemoryRegion(dac_cast(m_table), m_tableSize * sizeof(element_t)); +} +#endif + template typename SHash::count_t SHash::GetCount() const { @@ -282,7 +307,7 @@ void SHash::RemovePtr(element_t * p) } template -void SHash::RemoveAll() +void SHash::RemoveAll(bool retainCapacity) { CONTRACTL { @@ -300,13 +325,20 @@ void SHash::RemoveAll() } } - delete [] m_table; - - m_table = NULL; - m_tableSize = 0; + if (retainCapacity) + { + for (count_t i = 0; i < m_tableSize; i++) + m_table[i] = TRAITS::Null(); + } + else + { + delete [] m_table; + m_table = NULL; + m_tableSize = 0; + m_tableMax = 0; + } m_tableCount = 0; m_tableOccupied = 0; - m_tableMax = 0; } template From 94003f08c5deba072cdb5187b34b67535b9f842a Mon Sep 17 00:00:00 2001 From: Aaron Robinson Date: Wed, 21 Oct 2026 10:51:17 -0700 Subject: [PATCH 02/13] Migrate debugger hash tables to SHash Preserve COM enumeration ownership, patch-slot indices, and pending-evaluation ordering and OOM handling. Validated ICorDebugEnum cloning/reset/skip, ICorDebugBreakpointEnum and ICorDebugStepperEnum enumeration, and ICorDebugEval::NewString. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ce4b0890-6559-4b90-b46f-11179e70f973 --- src/coreclr/debug/di/hash.cpp | 387 +++++------------- src/coreclr/debug/di/rsclass.cpp | 2 +- src/coreclr/debug/di/rspriv.h | 135 ++---- src/coreclr/debug/di/rspriv.inl | 15 +- src/coreclr/debug/ee/controller.cpp | 182 ++++---- src/coreclr/debug/ee/controller.h | 177 ++++---- src/coreclr/debug/ee/debugger.cpp | 178 ++++---- src/coreclr/debug/ee/debugger.h | 310 ++++++-------- src/coreclr/debug/ee/debuggermodule.cpp | 63 +-- src/coreclr/debug/ee/functioninfo.cpp | 74 +--- .../exceptionstacktrace.cs | 48 +++ 11 files changed, 657 insertions(+), 914 deletions(-) diff --git a/src/coreclr/debug/di/hash.cpp b/src/coreclr/debug/di/hash.cpp index 911e0742814bb2..6634e3efcc63de 100644 --- a/src/coreclr/debug/di/hash.cpp +++ b/src/coreclr/debug/di/hash.cpp @@ -14,90 +14,54 @@ CordbHashTable::~CordbHashTable() { - HASHFIND find; - - for (CordbHashEntry *entry = (CordbHashEntry *) FindFirstEntry(&find); - entry != NULL; - entry = (CordbHashEntry *) FindNextEntry(&find)) - entry->pBase->InternalRelease(); + for (SHash::Iterator iterator = m_table.Begin(), end = m_table.End(); + iterator != end; iterator++) + (*iterator)->InternalRelease(); } HRESULT CordbHashTable::UnsafeAddBase(CordbBase *pBase) { AssertIsProtected(); - DbgIncChangeCount(); - - HRESULT hr = S_OK; - - if (!m_initialized) - { - HRESULT res = NewInit(m_iBuckets, sizeof(CordbHashEntry), 0xffff); - - if (res != S_OK) - { - return res; - } - m_initialized = true; - } - - CordbHashEntry *entry = (CordbHashEntry *) Add(HASH((ULONG_PTR)pBase->m_id)); + if (!m_table.AddNoThrow(pBase)) + return E_OUTOFMEMORY; - if (entry == NULL) - { - hr = E_FAIL; - } - else - { - entry->pBase = pBase; - m_count++; - pBase->InternalAddRef(); - } - return hr; + pBase->InternalAddRef(); + return S_OK; } bool CordbHashTable::IsInitialized() { - return m_initialized; + return m_table.GetCapacity() != 0; } CordbBase *CordbHashTable::UnsafeGetBase(ULONG_PTR id, BOOL fFab) { AssertIsProtected(); - CordbHashEntry *entry = NULL; - - if (!m_initialized) - return (NULL); - - entry = (CordbHashEntry *) Find(HASH((ULONG_PTR)id), KEY(id)); - return (entry ? entry->pBase : NULL); + return m_table.Lookup(id); } HRESULT CordbHashTable::UnsafeSwapBase(CordbBase *pOldBase, CordbBase *pNewBase) { - if (!m_initialized) - return E_FAIL; - AssertIsProtected(); - DbgIncChangeCount(); ULONG_PTR id = (ULONG_PTR)pOldBase->m_id; - CordbHashEntry *entry - = (CordbHashEntry *) Find(HASH((ULONG_PTR)id), KEY(id)); + CordbBase *const *entry = m_table.LookupPtr(id); if (entry == NULL) { return E_FAIL; } - _ASSERTE(entry->pBase == pOldBase); - entry->pBase = pNewBase; + _ASSERTE(*entry == pOldBase); + _ASSERTE(pOldBase->m_id == pNewBase->m_id); + pNewBase->InternalAddRef(); + m_table.ReplacePtr(entry, pNewBase); // release the hash table's reference to the old base and transfer it // to the new one. pOldBase->InternalRelease(); - pNewBase->InternalAddRef(); return S_OK; } @@ -107,24 +71,13 @@ CordbBase *CordbHashTable::UnsafeRemoveBase(ULONG_PTR id) { AssertIsProtected(); - if (!m_initialized) - return NULL; - - DbgIncChangeCount(); - - - CordbHashEntry *entry - = (CordbHashEntry *) Find(HASH((ULONG_PTR)id), KEY(id)); - - if (entry == NULL) + CordbBase *base = m_table.Lookup(id); + if (base == NULL) { return NULL; } - CordbBase *base = entry->pBase; - - Delete(HASH((ULONG_PTR)id), (HASHENTRY *) entry); - m_count--; + m_table.Remove(id); base->InternalRelease(); return base; @@ -138,12 +91,8 @@ CordbBase *CordbHashTable::UnsafeFindFirst(HASHFIND *find) CordbBase *CordbHashTable::UnsafeUnlockedFindFirst(HASHFIND *find) { - CordbHashEntry *entry = (CordbHashEntry *) FindFirstEntry(find); - - if (entry == NULL) - return NULL; - else - return entry->pBase; + find->iNext = 0; + return UnsafeUnlockedFindNext(find); } CordbBase *CordbHashTable::UnsafeFindNext(HASHFIND *find) @@ -154,12 +103,10 @@ CordbBase *CordbHashTable::UnsafeFindNext(HASHFIND *find) CordbBase *CordbHashTable::UnsafeUnlockedFindNext(HASHFIND *find) { - CordbHashEntry *entry = (CordbHashEntry *) FindNextEntry(find); - - if (entry == NULL) - return NULL; - else - return entry->pBase; + COUNT_T index = find->iNext; + CordbBase *const *entry = m_table.GetNext(index); + find->iNext = index; + return entry == nullptr ? nullptr : *entry; } /* ------------------------------------------------------------------------- * @@ -170,20 +117,15 @@ CordbBase *CordbHashTable::UnsafeUnlockedFindNext(HASHFIND *find) // Use the BuildOrThrow method to instantiate. CordbHashTableEnum::CordbHashTableEnum( CordbBase * pOwnerObj, NeuterList * pOwnerList, - CordbHashTable *table, REFIID guid ) : CordbBase(pOwnerObj->GetProcess(), 0, enumCordbHashTableEnum), m_pOwnerObj(pOwnerObj), m_pOwnerNeuterList(pOwnerList), - m_table(table), - m_started(false), - m_done(false), m_guid(guid), - m_iCurElt(0), - m_count(0), - m_fCountInit(FALSE) + m_iCurElt(0) { + m_items.EnableAutoClear(); } //--------------------------------------------------------------------------------------- @@ -204,9 +146,26 @@ void CordbHashTableEnum::BuildOrThrow( const GUID &id, RSInitHolder * pHolder) { - CordbHashTableEnum * pEnum = new CordbHashTableEnum(pOwnerObj, pOwnerList, pTable, id); + CordbProcess *pProcess = pOwnerObj->GetProcess(); + RSLock *pLock = pProcess == nullptr ? nullptr : pProcess->GetProcessLock(); + RSLockHolder lockHolder(pLock, FALSE); + if (pLock != nullptr) + lockHolder.Acquire(); + + CordbHashTableEnum * pEnum = new CordbHashTableEnum(pOwnerObj, pOwnerList, id); pHolder->Assign(pEnum); + pEnum->m_items.AllocOrThrow(pTable->GetCount()); + HASHFIND find; + ULONG index = 0; + for (CordbBase *item = pTable->UnsafeFindFirst(&find); + item != nullptr; + item = pTable->UnsafeFindNext(&find)) + { + pEnum->m_items.Assign(index++, item); + } + _ASSERTE(index == pEnum->m_items.Length()); + // If no neuter-list supplied, then our owner is manually managing us. // It also means we can't be cloned. if (pOwnerList != NULL) @@ -214,9 +173,6 @@ void CordbHashTableEnum::BuildOrThrow( pOwnerList->Add(pOwnerObj->GetProcess(), pEnum); } -#ifdef _DEBUG - pEnum->m_DbgChangeCount = pEnum->m_table->GetChangeCount(); -#endif } // Only for cloning. @@ -225,20 +181,15 @@ CordbHashTableEnum::CordbHashTableEnum(CordbHashTableEnum *cloneSrc) : CordbBase(cloneSrc->m_pOwnerObj->GetProcess(), 0, enumCordbHashTableEnum), m_pOwnerObj(cloneSrc->m_pOwnerObj), m_pOwnerNeuterList(cloneSrc->m_pOwnerNeuterList), - m_started(cloneSrc->m_started), - m_done(cloneSrc->m_done), - m_hashfind(cloneSrc->m_hashfind), m_guid(cloneSrc->m_guid), - m_iCurElt(cloneSrc->m_iCurElt), - m_count(cloneSrc->m_count), - m_fCountInit(cloneSrc->m_fCountInit) + m_iCurElt(cloneSrc->m_iCurElt) { + m_items.EnableAutoClear(); + // We can get cloned at any time, so our owner can't manually control us, // so we need explicit access to a neuter list. _ASSERTE(m_pOwnerNeuterList != NULL); - m_table = cloneSrc->m_table; - HRESULT hr = S_OK; EX_TRY { @@ -259,23 +210,19 @@ CordbHashTableEnum::CordbHashTableEnum(CordbHashTableEnum *cloneSrc) EX_CATCH_HRESULT(hr); SetUnrecoverableIfFailed(GetProcess(), hr); -#ifdef _DEBUG - m_DbgChangeCount = cloneSrc->m_DbgChangeCount; -#endif } CordbHashTableEnum::~CordbHashTableEnum() { _ASSERTE(this->IsNeutered()); - _ASSERTE(m_table == NULL); _ASSERTE(m_pOwnerObj == NULL); _ASSERTE(m_pOwnerNeuterList == NULL); } void CordbHashTableEnum::Neuter() { - m_table = NULL; + m_items.Clear(); m_pOwnerObj = NULL; m_pOwnerNeuterList = NULL; @@ -287,8 +234,7 @@ HRESULT CordbHashTableEnum::Reset() { HRESULT hr = S_OK; - m_started = false; - m_done = false; + m_iCurElt = 0; return hr; } @@ -304,36 +250,34 @@ HRESULT CordbHashTableEnum::Clone(ICorDebugEnum **ppEnum) HRESULT hr; hr = S_OK; - CordbHashTableEnum *e = NULL; - CordbProcess * pProc = GetProcess(); + *ppEnum = nullptr; - if (pProc != NULL) - { - // @todo - this is really ugly. This macro sets up stack-based dtor cleanup - // objects, and so it has to be in the same block of code as the code - // it protectes. Eg, we couldn't say: 'if (...) { ATT_ } { common code }' - // because the ATT_ stack based object would get destructed before the 'common code' - // was executed. - ATT_REQUIRE_STOPPED_MAY_FAIL(pProc); - e = new (nothrow) CordbHashTableEnum(this); - } - else - { - e = new (nothrow) CordbHashTableEnum(this); - - } - - if (e == NULL) + EX_TRY { - (*ppEnum) = NULL; - hr = E_OUTOFMEMORY; - goto LExit; + auto cloneSnapshot = [&]() + { + RSInitHolder clone; + clone.Assign(new CordbHashTableEnum(this)); + clone->m_items.AllocOrThrow(m_items.Length()); + for (ULONG index = 0; index < m_items.Length(); index++) + clone->m_items.Assign(index, m_items[index]); + IfFailThrow(clone->QueryInterface(m_guid, reinterpret_cast(ppEnum))); + clone.ClearAndMarkDontNeuter(); + }; + + if (pProc != NULL) + { + ATT_REQUIRE_STOPPED_MAY_FAIL(pProc); + cloneSnapshot(); + } + else + { + cloneSnapshot(); + } } + EX_CATCH_HRESULT(hr); - e->QueryInterface(m_guid, (void **) ppEnum); - -LExit: return hr; } @@ -345,57 +289,7 @@ HRESULT CordbHashTableEnum::GetCount(ULONG *pcelt) VALIDATE_POINTER_TO_OBJECT(pcelt, ULONG *); - *pcelt = m_table->GetCount(); - - return S_OK; -} - -HRESULT CordbHashTableEnum::PrepForEnum(CordbBase **pBase) -{ - HRESULT hr = S_OK; - - if (!m_started) - { - (*pBase) = m_table->UnsafeUnlockedFindFirst(&m_hashfind); - m_started = true; - } - else - { - (*pBase) = m_table->UnsafeUnlockedFindNext(&m_hashfind); - } - - return hr; -} - -HRESULT CordbHashTableEnum::SetupModuleEnum(void) -{ - return S_OK; -} - - -HRESULT CordbHashTableEnum::AdvancePreAssign(CordbBase **pBase) -{ - HRESULT hr = S_OK; - return hr; -} - -HRESULT CordbHashTableEnum::AdvancePostAssign(CordbBase **pBase, - CordbBase **b, - CordbBase **bEnd) -{ - CordbBase *base; - - if (pBase == NULL) - pBase = &base; - - // If we're looping like normal, or we're in skip - if ( ((b < bEnd) || ((b ==bEnd)&&(b==NULL))) - ) - { - (*pBase) = m_table->UnsafeUnlockedFindNext(&m_hashfind); - if (*pBase == NULL) - m_done = true; - } + *pcelt = m_items.Length(); return S_OK; } @@ -432,79 +326,50 @@ HRESULT CordbHashTableEnum::Next(ULONG celt, } HRESULT hr = S_OK; - CordbBase *base = NULL; CordbBase **b = bases; CordbBase **bEnd = bases + celt; - hr = PrepForEnum(&base); - if (FAILED(hr)) + while (b < bEnd && m_iCurElt < m_items.Length()) { - goto LError; - } - - while (b < bEnd && !m_done) - { - hr = AdvancePreAssign(&base); - if (FAILED(hr)) + CordbBase *base = m_items[m_iCurElt++]; + if (m_guid == IID_ICorDebugProcessEnum) { - goto LError; + *b = (CordbBase*)(ICorDebugProcess*)(CordbProcess*)base; } - - if (base == NULL) + else if (m_guid == IID_ICorDebugBreakpointEnum) + { + *b = (CordbBase*)(ICorDebugBreakpoint*)(CordbBreakpoint*)base; + } + else if (m_guid == IID_ICorDebugStepperEnum) + { + *b = (CordbBase*)(ICorDebugStepper*)(CordbStepper*)base; + } + else if (m_guid == IID_ICorDebugModuleEnum) { - m_done = true; + *b = (CordbBase*)(ICorDebugModule*)(CordbModule*)base; + } + else if (m_guid == IID_ICorDebugThreadEnum) + { + *b = (CordbBase*)(ICorDebugThread*)(CordbThread*)base; + } + else if (m_guid == IID_ICorDebugAppDomainEnum) + { + *b = (CordbBase*)(ICorDebugAppDomain*)(CordbAppDomain*)base; + } + else if (m_guid == IID_ICorDebugAssemblyEnum) + { + *b = (CordbBase*)(ICorDebugAssembly*)(CordbAssembly*)base; } else { - if (m_guid == IID_ICorDebugProcessEnum) - { - *b = (CordbBase*)(ICorDebugProcess*)(CordbProcess*)base; - } - else if (m_guid == IID_ICorDebugBreakpointEnum) - { - *b = (CordbBase*)(ICorDebugBreakpoint*)(CordbBreakpoint*)base; - } - else if (m_guid == IID_ICorDebugStepperEnum) - { - *b = (CordbBase*)(ICorDebugStepper*)(CordbStepper*)base; - } - else if (m_guid == IID_ICorDebugModuleEnum) - { - *b = (CordbBase*)(ICorDebugModule*)(CordbModule*)base; - } - else if (m_guid == IID_ICorDebugThreadEnum) - { - *b = (CordbBase*)(ICorDebugThread*)(CordbThread*)base; - } - else if (m_guid == IID_ICorDebugAppDomainEnum) - { - *b = (CordbBase*)(ICorDebugAppDomain*)(CordbAppDomain*)base; - } - else if (m_guid == IID_ICorDebugAssemblyEnum) - { - *b = (CordbBase*)(ICorDebugAssembly*)(CordbAssembly*)base; - } - else - { - *b = (CordbBase*)(IUnknown*)base; - } - - if (*b) - { - // 'b' is not a valid CordbBase ptr. - base->ExternalAddRef(); - b++; - } - - hr = AdvancePostAssign(&base, b, bEnd); - if (FAILED(hr)) - { - goto LError; - } + *b = (CordbBase*)(IUnknown*)base; } + + // 'b' is not a valid CordbBase ptr. + base->ExternalAddRef(); + b++; } -LError: // // If celt == 1, then the pceltFetched parameter is optional. // @@ -517,7 +382,7 @@ HRESULT CordbHashTableEnum::Next(ULONG celt, // If we reached the end of the enumeration, but not the end // of the number of requested items, we return S_FALSE. // - if (!FAILED(hr) && m_done && (b != bEnd)) + if (b != bEnd) { hr = S_FALSE; } @@ -531,36 +396,15 @@ HRESULT CordbHashTableEnum::Skip(ULONG celt) FAIL_IF_NEUTERED(this); AssertValid(); - HRESULT hr = S_OK; - - CordbBase *base; - - if (celt > 0) + ULONG remaining = m_items.Length() - m_iCurElt; + if (celt > remaining) { - if (!m_started) - { - base = m_table->UnsafeUnlockedFindFirst(&m_hashfind); - - if (base == NULL) - m_done = true; - else - celt--; - - m_started = true; - } - - while (celt > 0 && !m_done) - { - base = m_table->UnsafeUnlockedFindNext(&m_hashfind); - - if (base == NULL) - m_done = true; - else - celt--; - } + m_iCurElt = m_items.Length(); + return S_OK; } - return hr; + m_iCurElt += celt; + return S_OK; } HRESULT CordbHashTableEnum::QueryInterface(REFIID id, void **pInterface) @@ -607,16 +451,7 @@ HRESULT CordbHashTableEnum::QueryInterface(REFIID id, void **pInterface) #ifdef _DEBUG void CordbHashTableEnum::AssertValid() { - // @todo - Our behavior is undefined when enumerating a collection that changes underneath us. - // We'd love to just call this situation illegal, but clients could have very reasonably taken a dependency - // on it. Various APIs (eg, ICDStepper::Deactivate) may remove items from the hash. - // So we need to figure out what the behavior is here, spec it, and then enforce that and enable the - // strongest asserts possible there. - // - // Specifically, we cannot check that the hash hasn't change from underneath us: - // CONSISTENCY_CHECK_MSGF(m_DbgChangeCount == m_table->GetChangeCount(), - // ("Underlying hashtable has changed while enumerating.\nOriginal stamp=%d\nNew stamp=%d\nThis enum=0x%p", - // m_DbgChangeCount, m_table->GetChangeCount(), this)); + _ASSERTE(m_iCurElt <= m_items.Length()); } diff --git a/src/coreclr/debug/di/rsclass.cpp b/src/coreclr/debug/di/rsclass.cpp index 8e30f5f759982c..0efeb0f9dba560 100644 --- a/src/coreclr/debug/di/rsclass.cpp +++ b/src/coreclr/debug/di/rsclass.cpp @@ -46,7 +46,7 @@ CordbClass::CordbClass(CordbModule *m, mdTypeDef classMetadataToken) HANDLED: CordbModule* m_module; // Assigned w/o AddRef() FieldData *m_fields; // Deleted in ~CordbClass - CordbHangingFieldTable m_hangingFieldsStatic; // by value, ~CHashTableAndData frees + CordbHangingFieldTable m_hangingFieldsStatic; // by value, owns its storage */ diff --git a/src/coreclr/debug/di/rspriv.h b/src/coreclr/debug/di/rspriv.h index 9e68d3c1abd5ea..372c93d78d9ff7 100644 --- a/src/coreclr/debug/di/rspriv.h +++ b/src/coreclr/debug/di/rspriv.h @@ -1709,45 +1709,35 @@ typedef CordbEnumerator, // explicitly associate w/ an optional RSLock and assert that lock is held on access. // ---------------------------------------------------------------------------- -struct CordbHashEntry +class EMPTY_BASES CordbHashTraits : public NonDacAwareSHashTraits> { - FREEHASHENTRY entry; - CordbBase *pBase; +public: + typedef ULONG_PTR key_t; + static key_t GetKey(CordbBase *const &element) { return element->m_id; } + static BOOL Equals(key_t left, key_t right) { return left == right; } + static count_t Hash(key_t key) + { + count_t hash = static_cast(key); +#ifdef HOST_64BIT + hash = HashCOUNT_T(hash, static_cast(key >> 32)); +#endif + return hash; + } }; -class CordbHashTable : private CHashTableAndData +class CordbHashTable { private: - bool m_initialized; - SIZE_T m_count; - - BOOL Cmp(SIZE_T k1, const HASHENTRY * pc2) - { - LIMITED_METHOD_CONTRACT; - - return ((ULONG_PTR)k1) != (reinterpret_cast(pc2))->pBase->m_id; - } - - ULONG HASH(ULONG_PTR id) - { - return (ULONG)(id); - } - - SIZE_T KEY(UINT_PTR id) - { - return (SIZE_T)id; - } + SHash m_table; public: bool IsInitialized(); #ifndef DACCESS_COMPILE - CordbHashTable(ULONG size) - : CHashTableAndData(size), m_initialized(false), m_count(0) + CordbHashTable(ULONG) { #ifdef _DEBUG m_pDbgLock = NULL; - m_dbgChangeCount = 0; #endif } virtual ~CordbHashTable(); @@ -1759,19 +1749,13 @@ class CordbHashTable : private CHashTableAndData { m_pDbgLock = pLock; } - int GetChangeCount() { return m_dbgChangeCount; } private: void AssertIsProtected(); - // Increment the Change count. This can be used to check if the hashtable changes while being enumerated. - void DbgIncChangeCount() { m_dbgChangeCount++; } - - int m_dbgChangeCount; RSLock * m_pDbgLock; #else // RSLock association is a no-op on free builds. void AssertIsProtected() { }; - void DbgIncChangeCount() { }; #endif // _DEBUG public: @@ -1781,7 +1765,7 @@ class CordbHashTable : private CHashTableAndData ULONG32 GetCount() { - return ((ULONG32)m_count); + return m_table.GetCount(); } // These operators are unsafe b/c they have no typesafety. @@ -1855,7 +1839,6 @@ public ICorDebugAssemblyEnum CordbHashTableEnum( CordbBase * pOwnerObj, NeuterList * pOwnerList, - CordbHashTable *table, const GUID &id); public: @@ -2007,39 +1990,16 @@ public ICorDebugAssemblyEnum NeuterList * m_pOwnerNeuterList; - CordbHashTable *m_table; - bool m_started; - bool m_done; - HASHFIND m_hashfind; + RSPtrArray m_items; REFIID m_guid; ULONG m_iCurElt; - ULONG m_count; - BOOL m_fCountInit; #ifdef _DEBUG - // timestampt of hashtable when we start enumerating it. Useful for detecting if the table - // changes underneath us. - int m_DbgChangeCount; void AssertValid(); #else void AssertValid() { } #endif -private: - //These factor code between Next & Skip - HRESULT PrepForEnum(CordbBase **pBase); - - // Note that the set of types advanced by Pre & by Post are disjoint, and - // that the union of these two sets are all possible types enuerated by - // the CordbHashTableEnum. - HRESULT AdvancePreAssign(CordbBase **pBase); - HRESULT AdvancePostAssign(CordbBase **pBase, - CordbBase **b, - CordbBase **bEnd); - - // This factors some code that initializes the module enumerator. - HRESULT SetupModuleEnum(); - }; @@ -3975,7 +3935,6 @@ class CordbProcess : CORDB_ADDRESS m_minPatchAddr; //smallest patch in table CORDB_ADDRESS m_maxPatchAddr; - // @todo port : if slots of CHashTable change, so should these #define DPT_TERMINATING_INDEX (UINT32_MAX) // Index into m_pPatchTable of the first patch (first used entry). ULONG m_iFirstPatch; @@ -4433,69 +4392,57 @@ class CordbModule : public CordbBase, }; -struct CordbHangingField +class EMPTY_BASES CordbHangingFieldTraits : public NonDacAwareSHashTraits> { - FREEHASHENTRY entry; - FieldData data; +public: + typedef mdFieldDef key_t; + static key_t GetKey(const FieldData &element) { return element.m_fldMetadataToken; } + static BOOL Equals(key_t left, key_t right) { return left == right; } + static count_t Hash(key_t key) { return key; } + static FieldData Null() { return {}; } + static FieldData Deleted() + { + FieldData element = {}; + element.m_fldMetadataToken = static_cast(-1); + return element; + } + static bool IsNull(const FieldData &element) { return element.m_fldMetadataToken == 0; } + static bool IsDeleted(const FieldData &element) { return element.m_fldMetadataToken == static_cast(-1); } }; // A hashtable for storing EnC hanging field information // FieldData.m_fldMetadataToken is the key -class CordbHangingFieldTable : public CHashTableAndData +class CordbHangingFieldTable { - private: - - BOOL Cmp(SIZE_T k1, const HASHENTRY *pc2) - { - LIMITED_METHOD_CONTRACT; - return (ULONG)(UINT_PTR)(k1) != - (reinterpret_cast(pc2))->data.m_fldMetadataToken; - } - - ULONG HASH(mdFieldDef fldToken) - { - LIMITED_METHOD_CONTRACT; - return fldToken; - } - - SIZE_T KEY(mdFieldDef fldToken) - { - return (SIZE_T)fldToken; - } + SHash m_table; public: #ifndef DACCESS_COMPILE - CordbHangingFieldTable() : CHashTableAndData(11) + void Clear() { - NewInit(11, sizeof(CordbHangingField), 11); + m_table.RemoveAll(); } FieldData * AddFieldInfo(FieldData * pInfo) { _ASSERTE(pInfo != NULL); - CordbHangingField *pEntry = (CordbHangingField *)Add(HASH(pInfo->m_fldMetadataToken)); - pEntry->data = *pInfo; // copy everything over - - // Return a pointer to the data - return &(pEntry->data); + m_table.Add(*pInfo); + return GetFieldInfo(pInfo->m_fldMetadataToken); } void RemoveFieldInfo(mdFieldDef fldToken) { - CordbHangingField *entry = (CordbHangingField*)Find(HASH(fldToken), KEY(fldToken)); - _ASSERTE(entry != NULL); - Delete(HASH(fldToken), (HASHENTRY*)entry); + m_table.Remove(fldToken); } #endif // #ifndef DACCESS_COMPILE FieldData * GetFieldInfo(mdFieldDef fldToken) { - CordbHangingField * entry = (CordbHangingField *)Find(HASH(fldToken), KEY(fldToken)); - return (entry!=NULL?&(entry->data):NULL); + return const_cast(m_table.LookupPtr(fldToken)); } }; diff --git a/src/coreclr/debug/di/rspriv.inl b/src/coreclr/debug/di/rspriv.inl index 755de8d2bc7a64..c7641b06fc5cf2 100644 --- a/src/coreclr/debug/di/rspriv.inl +++ b/src/coreclr/debug/di/rspriv.inl @@ -92,7 +92,10 @@ RSInitHolder::~RSInitHolder() if (m_pObject != NULL) { CordbProcess * pProcess = m_pObject->GetProcess(); - RSLockHolder lockHolder(pProcess->GetProcessLock()); + RSLock *pLock = pProcess == nullptr ? nullptr : pProcess->GetProcessLock(); + RSLockHolder lockHolder(pLock, FALSE); + if (pLock != nullptr) + lockHolder.Acquire(); m_pObject->Neuter(); @@ -666,16 +669,15 @@ inline void CordbSafeHashTable::TransferToArray(RSPtrArray * pArray) pArray->AllocOrThrow(count); + T *pCordbBase = FindFirst(&find); while(idx < count) { - T * pCordbBase = FindFirst(&find); _ASSERTE(pCordbBase != NULL); pArray->Assign(idx, pCordbBase); idx++; - // We're removing while iterating the collection. - // But we reset the iteration each time by calling FindFirst. RemoveBase((ULONG_PTR)pCordbBase->m_id); // this will call release, adjust GetCount() + pCordbBase = FindNext(&find); } // Assert is at end. @@ -701,16 +703,15 @@ inline void CordbSafeHashTable::NeuterAndClear(RSLock * pLock) UINT32 count = GetCount(); UINT32 idx = 0; + T *pCordbBase = FindFirst(&find); while(idx < count) { - T * pCordbBase = FindFirst(&find); _ASSERTE(pCordbBase != NULL); pCordbBase->Neuter(); idx++; - // We're removing while iterating the collection. - // But we reset the iteration each time by calling FindFirst. RemoveBase((ULONG_PTR)pCordbBase->m_id); // this will call release, adjust GetCount() + pCordbBase = FindNext(&find); } // Assert is at end. diff --git a/src/coreclr/debug/ee/controller.cpp b/src/coreclr/debug/ee/controller.cpp index 589b513883a2c6..7f9fb35ace1ddf 100644 --- a/src/coreclr/debug/ee/controller.cpp +++ b/src/coreclr/debug/ee/controller.cpp @@ -276,7 +276,87 @@ SharedPatchBypassBuffer* DebuggerControllerPatch::GetOrCreateSharedPatchBypassBu #endif // !FEATURE_EMULATE_SINGLESTEP #endif // !DACCESS_COMPILE -// @todo - remove all this splicing trash +#ifndef DACCESS_COMPILE +DebuggerPatchTable::~DebuggerPatchTable() +{ + if (m_pcEntries != 0) + CNewZeroData::Free(reinterpret_cast(m_pcEntries), 0); +} + +void DebuggerPatchTable::EnsureList(DebuggerPatchKey key) +{ + if (m_patchLists.LookupPtr(key) == nullptr) + { + DebuggerPatchListEntry entry = {}; + entry.key = key; + entry.first = DPT_INVALID_SLOT; + m_patchLists.Add(entry); + } +} + +DebuggerControllerPatch *DebuggerPatchTable::AddPatch(DebuggerPatchKey key) +{ + if (m_iFree == DPT_INVALID_SLOT) + { + S_UINT32 count = S_UINT32(m_iEntries) + S_UINT32(m_iEntries / 2) + S_UINT32(17); + S_UINT32 size = count * S_UINT32(sizeof(DebuggerControllerPatch)); + if (count.IsOverflow() || size.IsOverflow() || size.Value() > INT_MAX) + ThrowOutOfMemory(); + + BYTE *memory = CNewZeroData::Alloc(static_cast(size.Value()), 0); + if (memory == nullptr) + ThrowOutOfMemory(); + + if (m_pcEntries != 0) + { + memcpy(memory, reinterpret_cast(m_pcEntries), m_iEntries * sizeof(DebuggerControllerPatch)); + CNewZeroData::Free(reinterpret_cast(m_pcEntries), 0); + } + m_pcEntries = reinterpret_cast(memory); + for (ULONG index = m_iEntries; index < count.Value(); index++) + { + DebuggerControllerPatch *patch = reinterpret_cast(EntryPtr(index)); + patch->entry.iFree = index + 1; + } + reinterpret_cast(EntryPtr(count.Value() - 1))->entry.iFree = DPT_INVALID_SLOT; + m_iFree = m_iEntries; + m_iEntries = count.Value(); + } + + // Allocate the index before taking a slot so OOM leaves the table unchanged. + EnsureList(key); + const DebuggerPatchListEntry *list = m_patchLists.LookupPtr(key); + ULONG index = m_iFree; + DebuggerControllerPatch *patch = reinterpret_cast(EntryPtr(index)); + m_iFree = patch->entry.iFree; + memset(patch, 0, sizeof(*patch)); + patch->entry.iPrev = DPT_INVALID_SLOT; + patch->entry.iNext = list->first; + if (list->first != DPT_INVALID_SLOT) + EntryPtr(list->first)->iPrev = index; + + DebuggerPatchListEntry updated = *list; + updated.first = index; + m_patchLists.ReplacePtr(list, updated); + return patch; +} + +void DebuggerPatchTable::LinkPatch(DebuggerControllerPatch *patch) +{ + const DebuggerPatchListEntry *list = m_patchLists.LookupPtr(Key(patch)); + _ASSERTE(list != nullptr); + ULONG index = ItemIndex(reinterpret_cast(patch)); + patch->entry.iPrev = DPT_INVALID_SLOT; + patch->entry.iNext = list->first; + if (list->first != DPT_INVALID_SLOT) + EntryPtr(list->first)->iPrev = index; + + DebuggerPatchListEntry updated = *list; + updated.first = index; + m_patchLists.ReplacePtr(list, updated); +} +#endif // DACCESS_COMPILE + // This Sort/Splice stuff just reorders the patches within a particular chain such // that when we iterate through by calling GetPatch() and GetNextPatch(DebuggerControllerPatch), // we'll get patches in increasing order of DebuggerControllerTypes. @@ -288,7 +368,7 @@ void DebuggerPatchTable::SortPatchIntoPatchList(DebuggerControllerPatch **ppPatc LOG((LF_CORDB, LL_EVERYTHING, "DPT::SPIPL ppPatch: %p, pPatch: %p \n", ppPatch, (*ppPatch))); #ifdef _DEBUG DebuggerControllerPatch *patchFirst - = (DebuggerControllerPatch *) Find(Hash((*ppPatch)), Key((*ppPatch))); + = FindPatch(Key(*ppPatch)); _ASSERTE(patchFirst == (*ppPatch)); _ASSERTE((*ppPatch)->controller->GetDCType() != DEBUGGER_CONTROLLER_STATIC); #endif //_DEBUG @@ -301,14 +381,6 @@ void DebuggerPatchTable::SortPatchIntoPatchList(DebuggerControllerPatch **ppPatc return; } - // If we decide to reorder the list, we'll need to keep the element - // indexed by the hash function as the (sorted)first item. Everything else - // chains off this element, can thus stay put. - // Thus, either the element we just added is already sorted, or else we'll - // have to move it elsewhere in the list, meaning that we'll have to swap - // the second item & the new item, so that the index points to the proper - // first item in the list. - //use Cur ptr for case where patch gets appended to list DebuggerControllerPatch *patchCur = patchNext; @@ -348,26 +420,25 @@ void DebuggerPatchTable::SortPatchIntoPatchList(DebuggerControllerPatch **ppPatc // the patch back somewhere else. void DebuggerPatchTable::SpliceOutOfList(DebuggerControllerPatch *patch) { - // We need to get iHash, the index of the ptr within - // m_piBuckets, ie it's entry in the hashtable. - ULONG iHash = Hash(patch) % m_iBuckets; - ULONG iElement = m_piBuckets[iHash]; - DebuggerControllerPatch *patchFirst - = (DebuggerControllerPatch *) EntryPtr(iElement); + DebuggerPatchKey key = Key(patch); + const DebuggerPatchListEntry *list = m_patchLists.LookupPtr(key); + _ASSERTE(list != nullptr); // Fix up pointers to chain - if (patchFirst == patch) + if (patch->entry.iPrev == DPT_INVALID_SLOT) { // The first patch shouldn't have anything behind it. _ASSERTE(patch->entry.iPrev == DPT_INVALID_SLOT); - if (patch->entry.iNext != DPT_INVALID_SLOT) + if (patch->entry.iNext == DPT_INVALID_SLOT) { - m_piBuckets[iHash] = patch->entry.iNext; + m_patchLists.Remove(key); } else { - m_piBuckets[iHash] = DPT_INVALID_SLOT; + DebuggerPatchListEntry updated = *list; + updated.first = patch->entry.iNext; + m_patchLists.ReplacePtr(list, updated); } } @@ -378,7 +449,7 @@ void DebuggerPatchTable::SpliceOutOfList(DebuggerControllerPatch *patch) if (patch->entry.iPrev != DPT_INVALID_SLOT) { - EntryPtr(patch->entry.iNext)->iNext = patch->entry.iNext; + EntryPtr(patch->entry.iPrev)->iNext = patch->entry.iNext; } patch->entry.iNext = DPT_INVALID_SLOT; @@ -634,13 +705,8 @@ DebuggerControllerPatch *DebuggerPatchTable::AddPatchForMethodDef(DebuggerContro LOG( (LF_CORDB,LL_INFO10000,"DPT:APFMD 0x%x with dji %p, %s offset 0x%zx controller:%p AD:%p\n", md, dji, (offsetIsIL ? "IL" : "native"), offset, controller, pAppDomain)); - DebuggerFunctionKey key; - - key.module = module; - key.md = md; - // Get a new uninitialized patch object - DebuggerControllerPatch *patch = (DebuggerControllerPatch *) Add(HashKey(&key)); + DebuggerControllerPatch *patch = AddPatch({0, dac_cast(module), md}); if (patch == NULL) ThrowOutOfMemory(); @@ -747,7 +813,7 @@ DebuggerControllerPatch *DebuggerPatchTable::AddPatchForAddress(DebuggerControll // get new uninitialized patch object DebuggerControllerPatch *patch = - (DebuggerControllerPatch *) Add(HashAddress(address)); + AddPatch({dac_cast(address), 0, 0}); if (patch == NULL) { @@ -826,11 +892,12 @@ void DebuggerPatchTable::BindPatch(DebuggerControllerPatch *patch, CORDB_ADDRESS //zeroing out the opcode field (see lengthy comment above) // Since the patch is double-hashed based off Address, if we change the address, // we must remove and reinsert the patch. - CHashTable::Delete(HashKey(&patch->key), ItemIndex((HASHENTRY*)patch)); + EnsureList({dac_cast(address), 0, 0}); + SpliceOutOfList(patch); patch->address = address; - CHashTable::Add(HashAddress(address), ItemIndex((HASHENTRY*)patch)); + LinkPatch(patch); SortPatchIntoPatchList(&patch); @@ -861,13 +928,12 @@ void DebuggerPatchTable::UnbindPatch(DebuggerControllerPatch *patch) // Update it's index entry in the table to use it's unbound key // Since the patch is double-hashed based off Address, if we change the address, // we must remove and reinsert the patch. - CHashTable::Delete( HashAddress(patch->address), - ItemIndex((HASHENTRY*)patch)); + EnsureList({0, dac_cast(patch->key.module), patch->key.md}); + SpliceOutOfList(patch); patch->address = NULL; // we're no longer bound to this address - CHashTable::Add( HashKey(&patch->key), - ItemIndex((HASHENTRY*)patch)); + LinkPatch(patch); _ASSERTE(!patch->IsBound() ); @@ -881,47 +947,17 @@ void DebuggerPatchTable::RemovePatch(DebuggerControllerPatch *patch) patch->DoCleanup(); #endif // !FEATURE_EMULATE_SINGLESTEP - // - // Because of the implementation of CHashTable, we can safely - // delete elements while iterating through the table. This - // behavior is relied upon - do not change to a different - // implementation without considering this fact. - // - Delete(Hash(patch), (HASHENTRY *) patch); + SpliceOutOfList(patch); + patch->refCount = 0; + patch->entry.iFree = m_iFree; + m_iFree = ItemIndex(reinterpret_cast(patch)); } DebuggerControllerPatch *DebuggerPatchTable::GetNextPatch(DebuggerControllerPatch *prev) { - ULONG iNext; - HASHENTRY *psEntry; - - // Start at the next entry in the chain. - // @todo - note that: EntryPtr(ItemIndex(x)) == x - iNext = EntryPtr(ItemIndex((HASHENTRY*)prev))->iNext; - - // Search until we hit the end. - while (iNext != UINT32_MAX) - { - // Compare the keys. - psEntry = EntryPtr(iNext); - - // Careful here... we can hash the entries in this table - // by two types of keys. In this type of search, the type - // of the second key (psEntry) does not necessarily - // indicate the type of the first key (prev), so we have - // to check for sure. - DebuggerControllerPatch *pc2 = (DebuggerControllerPatch*)psEntry; - - if (((pc2->address == NULL) && (prev->address == NULL)) || - ((pc2->address != NULL) && (prev->address != NULL))) - if (!Cmp(Key(prev), psEntry)) - return pc2; - - // Advance to the next item in the chain. - iNext = psEntry->iNext; - } - - return NULL; + ULONG next = prev->entry.iNext; + return next == DPT_INVALID_SLOT ? nullptr + : dac_cast(EntryPtr(next)); } // Count how many patches are in the table. @@ -933,9 +969,7 @@ int DebuggerPatchTable::GetNumberOfPatches() if ((TADDR)NULL != m_pcEntries) { DebuggerControllerPatch *dcp; - ULONG i = 0; - - while (i++ +struct DebuggerPatchKey +{ + TADDR address; + TADDR module; + mdMethodDef token; +}; + +struct DebuggerPatchListEntry : DebuggerHeapHashEntry +{ + DebuggerPatchKey key; + ULONG first; +}; + +class EMPTY_BASES DebuggerPatchListTraits : public DefaultSHashTraits +{ +public: + typedef DebuggerPatchKey key_t; + static key_t GetKey(const element_t &element) { return element.key; } + static BOOL Equals(key_t left, key_t right) + { + return left.address == right.address && left.module == right.module && left.token == right.token; + } + static count_t Hash(key_t key) + { + TADDR value = key.address != 0 ? key.address : key.module; + return HashCOUNT_T(key.token, DebuggerHashPointer(value)); + } + static element_t Null() { return {}; } + static element_t Deleted() + { + element_t element = {}; + element.key.address = static_cast(-1); + return element; + } + static bool IsNull(const element_t &element) { return element.key.address == 0 && element.key.module == 0; } + static bool IsDeleted(const element_t &element) { return element.key.address == static_cast(-1); } +}; + +class DebuggerPatchTable { VPTR_BASE_CONCRETE_VTABLE_CLASS(DebuggerPatchTable); friend struct ::cdac_data; public: +#ifndef DACCESS_COMPILE + virtual ~DebuggerPatchTable(); +#else virtual ~DebuggerPatchTable() = default; +#endif friend class DebuggerRCThread; private: //incremented so that we can get DPT-wide unique PIDs. // pid = Patch ID. SIZE_T m_patchId; - // Given a patch, retrieves the correct key. The return value of this function is passed to Cmp(), Find(), etc. - SIZE_T Key(DebuggerControllerPatch *patch) + // Slots retain their indices when the SHash index grows. Readers in another process + // consume only this zero-initialized, contiguous storage and its published capacity. + TADDR m_pcEntries; + ULONG m_iEntries; + ULONG m_iFree; + SHash m_patchLists; + + static DebuggerPatchKey Key(DebuggerControllerPatch *patch) { LIMITED_METHOD_DAC_CONTRACT; - - // Most clients of CHashTable pass a host pointer as the key. However, the key really could be - // anything. In our case, the key can either be a host pointer of type DebuggerFunctionKey or - // the address of the patch. - if (patch->address == NULL) - { - return (SIZE_T)(&patch->key); - } - else - { - return (SIZE_T)(dac_cast(patch->address)); - } + return patch->address == nullptr + ? DebuggerPatchKey{0, dac_cast(patch->key.module), patch->key.md} + : DebuggerPatchKey{dac_cast(patch->address), 0, 0}; } - // Given two DebuggerControllerPatches, tells - // whether they are equal or not. Does this by comparing the correct - // key. - // BYTE* pc1: If pc2 is hashed by address, - // pc1 is an address. If - // pc2 is hashed by DebuggerFunctionKey, - // pc1 is a DebuggerFunctionKey - //Returns true if the two patches are equal, false otherwise - BOOL Cmp(SIZE_T k1, const HASHENTRY * pc2) + DebuggerControllerPatch *FindPatch(DebuggerPatchKey key) { LIMITED_METHOD_DAC_CONTRACT; - - DebuggerControllerPatch * pPatch2 = dac_cast(const_cast(pc2)); - - if (pPatch2->address == NULL) - { - // k1 is a host pointer of type DebuggerFunctionKey. - DebuggerFunctionKey * pKey1 = reinterpret_cast(k1); - - return ((pKey1->module != pPatch2->key.module) || (pKey1->md != pPatch2->key.md)); - } - else - { - return ((SIZE_T)(dac_cast(pPatch2->address)) != k1); - } + const DebuggerPatchListEntry *entry = m_patchLists.LookupPtr(key); + return entry == nullptr || entry->first == DPT_INVALID_SLOT ? nullptr + : dac_cast(EntryPtr(entry->first)); } - //Computes a hash value based on an address - ULONG HashAddress(PTR_CORDB_ADDRESS_TYPE address) + HASHENTRY *EntryPtr(ULONG index) { LIMITED_METHOD_DAC_CONTRACT; - return (ULONG)(SIZE_T)(dac_cast(address)); + return dac_cast(m_pcEntries + index * sizeof(DebuggerControllerPatch)); } - //Computes a hash value based on a DebuggerFunctionKey - ULONG HashKey(DebuggerFunctionKey * pKey) + ULONG ItemIndex(HASHENTRY *entry) { - SUPPORTS_DAC; - return HashPtr(pKey->md, pKey->module); + LIMITED_METHOD_DAC_CONTRACT; + return static_cast((dac_cast(entry) - m_pcEntries) / sizeof(DebuggerControllerPatch)); } - //Computes a hash value from a patch, using the address field - // if the patch is hashed by address, using the DebuggerFunctionKey - // otherwise - ULONG Hash(DebuggerControllerPatch * pPatch) - { - SUPPORTS_DAC; - - if (pPatch->address == NULL) - return HashKey(&(pPatch->key)); - else - return HashAddress(pPatch->address); - } +#ifndef DACCESS_COMPILE + DebuggerControllerPatch *AddPatch(DebuggerPatchKey key); + void EnsureList(DebuggerPatchKey key); + void LinkPatch(DebuggerControllerPatch *patch); +#endif //Public Members public: enum { @@ -756,7 +758,8 @@ class DebuggerPatchTable : private CHashTableAndData #ifndef DACCESS_COMPILE - DebuggerPatchTable() : CHashTableAndData(DPT_HASH_BUCKETS) { } + DebuggerPatchTable() : m_patchId(DCP_PATCHID_FIRST_VALID), m_pcEntries(0), + m_iEntries(0), m_iFree(DPT_INVALID_SLOT) { } HRESULT Init() { @@ -764,8 +767,7 @@ class DebuggerPatchTable : private CHashTableAndData m_patchId = DCP_PATCHID_FIRST_VALID; - SUPPRESS_ALLOCATION_ASSERTS_IN_THIS_SCOPE; - return NewInit(17, sizeof(DebuggerControllerPatch), 101); + return S_OK; } // Assuming that the chain of patches (as defined by all the @@ -818,16 +820,12 @@ class DebuggerPatchTable : private CHashTableAndData // We can hopefully remove them. static SIZE_T GetOffsetOfEntries() { - // assert that we the offsets of these fields in the base class is - // the same as the offset of this field in this class. - _ASSERTE((void*)(DebuggerPatchTable*)NULL == (void*)(CHashTableAndData*)NULL); - return helper_GetOffsetOfEntries(); + return offsetof(DebuggerPatchTable, m_pcEntries); } static SIZE_T GetOffsetOfCount() { - _ASSERTE((void*)(DebuggerPatchTable*)NULL == (void*)(CHashTableAndData*)NULL); - return helper_GetOffsetOfCount(); + return offsetof(DebuggerPatchTable, m_iEntries); } // GetPatch find the first patch in the hash table @@ -839,12 +837,7 @@ class DebuggerPatchTable : private CHashTableAndData // this Module,mdMethodDef pair DebuggerControllerPatch *GetPatch(Module *module, mdToken md) { - DebuggerFunctionKey key; - - key.module = module; - key.md = md; - - return reinterpret_cast(Find(HashKey(&key), (SIZE_T)&key)); + return FindPatch({0, dac_cast(module), md}); } #endif // #ifndef DACCESS_COMPILE @@ -856,10 +849,7 @@ class DebuggerPatchTable : private CHashTableAndData SUPPORTS_DAC; ARM_ONLY(_ASSERTE(dac_cast(address) & THUMB_CODE)); - DebuggerControllerPatch * pPatch = - dac_cast(Find(HashAddress(address), (SIZE_T)(dac_cast(address)))); - - return pPatch; + return FindPatch({dac_cast(address), 0, 0}); } DebuggerControllerPatch *GetNextPatch(DebuggerControllerPatch *prev); @@ -867,9 +857,8 @@ class DebuggerPatchTable : private CHashTableAndData // Find the first patch in the patch table, and store // index info in info. Along with GetNextPatch, this can // iterate through the whole patch table. Note that since the - // hashtable operates via iterating through all the contents - // of all the buckets, if you add an entry while iterating - // through the table, you may or may not iterate across + // table operates by iterating over its slots. If you add an entry while iterating + // through the table, you may or may not iterate across // the new entries. You will iterate through all the entries // that were present at the beginning of the run. You // safely delete anything you've already iterated by, anything @@ -878,7 +867,8 @@ class DebuggerPatchTable : private CHashTableAndData { SUPPORTS_DAC; - return dac_cast(FindFirstEntry(pInfo)); + pInfo->iNext = 0; + return GetNextPatch(pInfo); } // Along with GetFirstPatch, this can iterate through @@ -888,7 +878,14 @@ class DebuggerPatchTable : private CHashTableAndData { SUPPORTS_DAC; - return dac_cast(FindNextEntry(pInfo)); + while (pInfo->iNext < m_iEntries) + { + DebuggerControllerPatch *patch = + dac_cast(EntryPtr(pInfo->iNext++)); + if (!patch->IsFree()) + return patch; + } + return nullptr; } // Used by DebuggerController to translate an index diff --git a/src/coreclr/debug/ee/debugger.cpp b/src/coreclr/debug/ee/debugger.cpp index 745b9bdbcb2cdf..a5f40c875ed11b 100644 --- a/src/coreclr/debug/ee/debugger.cpp +++ b/src/coreclr/debug/ee/debugger.cpp @@ -65,6 +65,26 @@ InteropSafe interopsafe; #ifndef DACCESS_COMPILE +void *DebuggerHeapHashEntry::operator new[](size_t size, const std::nothrow_t &) noexcept +{ + if (size > INT_MAX) + return nullptr; + return CNewZeroData::Alloc(static_cast(size), 0); +} + +void *DebuggerHeapHashEntry::operator new[](size_t size) +{ + void *memory = operator new[](size, std::nothrow); + if (memory == nullptr) + ThrowOutOfMemory(); + return memory; +} + +void DebuggerHeapHashEntry::operator delete[](void *memory) +{ + CNewZeroData::Free(static_cast(memory), 0); +} + DebuggerRCThread *g_pRCThread = NULL; // Do some compile time checking on the events in DbgIpcEventTypes.h @@ -1062,7 +1082,63 @@ HRESULT Debugger::CheckInitModuleTable() return S_OK; } -// Checks if the m_pModules table has been allocated, and if not does so. +#ifndef DACCESS_COMPILE +HRESULT DebuggerPendingFuncEvalTable::AddPendingEval(Thread *pThread, DebuggerEval *pDE) +{ + CONTRACTL + { + NOTHROW; + GC_NOTRIGGER; + } + CONTRACTL_END; + + _ASSERTE(pThread != nullptr && pDE != nullptr); + _ASSERTE(pDE->m_nextPendingEval == nullptr); + + DebuggerPendingFuncEval *entry = GetPendingEval(pThread); + if (entry != nullptr) + { + // Keep newest-first order independently of SHash rehashing. + pDE->m_nextPendingEval = entry->pDE; + entry->pDE = pDE; + return S_OK; + } + + DebuggerPendingFuncEval newEntry = {}; + newEntry.pThread = pThread; + newEntry.pDE = pDE; + return m_table.AddNoThrow(newEntry) ? S_OK : E_OUTOFMEMORY; +} + +void DebuggerPendingFuncEvalTable::RemovePendingEval(Thread *pThread) +{ + CONTRACTL + { + NOTHROW; + GC_NOTRIGGER; + } + CONTRACTL_END; + + _ASSERTE(pThread != nullptr); + + DebuggerPendingFuncEval *entry = GetPendingEval(pThread); + if (entry != nullptr) + { + DebuggerEval *pDE = entry->pDE; + if (pDE->m_nextPendingEval != nullptr) + { + entry->pDE = pDE->m_nextPendingEval; + } + else + { + m_table.RemovePtr(entry); + } + pDE->m_nextPendingEval = nullptr; + } +} +#endif // DACCESS_COMPILE + +// Checks if the pending-evaluation table has been allocated, and if not does so. HRESULT Debugger::CheckInitPendingFuncEvalTable() { CONTRACTL @@ -1102,83 +1178,17 @@ ULONG DebuggerMethodInfoTable::CheckDmiTable(void) { LIMITED_METHOD_CONTRACT; - ULONG cApparent = 0; - ULONG cOfficial = 0; - - if (NULL != m_pcEntries) + HASHFIND find; + ULONG count = 0; + for (DebuggerMethodInfoEntry *entry = m_table.FindFirstEntry(&find); + entry != nullptr; + entry = m_table.FindNextEntry(&find)) { - DebuggerMethodInfoEntry *dcp; - int i = 0; - while (i++ pFD != 0 && - dcp->pFD != (MethodDesc*)0xcdcdcdcd && - dcp->mi != NULL) - { - cApparent++; - - _ASSERTE( dcp->pFD == dcp->mi->m_fd ); - LOG((LF_CORDB, LL_INFO1000, "DMIT::CDT:Entry:0x%p mi:0x%p\nPrevs:\n", - dcp, dcp->mi)); - DebuggerMethodInfo *dmi = dcp->mi->m_prevMethodInfo; - - while(dmi != NULL) - { - LOG((LF_CORDB, LL_INFO1000, "\t0x%p\n", dmi)); - dmi = dmi->m_prevMethodInfo; - } - dmi = dcp->mi->m_nextMethodInfo; - - LOG((LF_CORDB, LL_INFO1000, "Nexts:\n", dmi)); - while(dmi != NULL) - { - LOG((LF_CORDB, LL_INFO1000, "\t0x%p\n", dmi)); - dmi = dmi->m_nextMethodInfo; - } - - LOG((LF_CORDB, LL_INFO1000, "DMIT::CDT:DONE\n", - dcp, dcp->mi)); - } - } - - if (m_piBuckets == 0) - { - LOG((LF_CORDB, LL_INFO1000, "DMIT::CDT: The table is officially empty!\n")); - return cOfficial; - } - - LOG((LF_CORDB, LL_INFO1000, "DMIT::CDT:Looking for official entries:\n")); - - ULONG iNext = m_piBuckets[0]; - ULONG iBucket = 1; - HASHENTRY *psEntry = NULL; - while (TRUE) - { - while (iNext != UINT32_MAX) - { - cOfficial++; - - psEntry = EntryPtr(iNext); - dcp = ((DebuggerMethodInfoEntry *)psEntry); - - LOG((LF_CORDB, LL_INFO1000, "\tEntry:0x%p mi:0x%p @idx:0x%x @bucket:0x%x\n", - dcp, dcp->mi, iNext, iBucket)); - - iNext = psEntry->iNext; - } - - // Advance to the next bucket. - if (iBucket < m_iBuckets) - iNext = m_piBuckets[iBucket++]; - else - break; - } - - LOG((LF_CORDB, LL_INFO1000, "DMIT::CDT:Finished official entries: ****************")); + _ASSERTE(entry->key.pModule != nullptr); + count++; } - - return cOfficial; + _ASSERTE(count == m_table.GetCount()); + return count; } #endif // _DEBUG_DMI_TABLE @@ -1225,6 +1235,7 @@ DebuggerEval::DebuggerEval(CONTEXT * pContext, DebuggerIPCE_FuncEvalInfo * pEval } m_thread = pEvalInfo->vmThreadToken.GetRawPtr(); + m_nextPendingEval = nullptr; m_evalType = pEvalInfo->funcEvalType; m_methodToken = pEvalInfo->funcMetadataToken; m_classToken = pEvalInfo->funcClassMetadataToken; @@ -14144,12 +14155,9 @@ HRESULT Debugger::FuncEvalSetup(DebuggerIPCE_FuncEvalInfo *pEvalInfo, if (pDE->m_argData == NULL) { - DeleteInteropSafeExecutable(pDE); + DeleteInteropSafe(pDE); return E_OUTOFMEMORY; } - - // Pass back the address of the argument data area so the right side can write to it for us. - *argDataArea = pDE->m_argData; } // Hijacked evals rewrite the thread's native context to enter FuncEvalHijack when execution resumes. @@ -14199,7 +14207,7 @@ HRESULT Debugger::FuncEvalSetup(DebuggerIPCE_FuncEvalInfo *pEvalInfo, if (FAILED(hr)) { - DeleteInteropSafeExecutable(pDE); // Note this runs the destructor for DebuggerEval, which releases its internal buffers + DeleteInteropSafe(pDE); return hr; } @@ -14207,7 +14215,12 @@ HRESULT Debugger::FuncEvalSetup(DebuggerIPCE_FuncEvalInfo *pEvalInfo, // the process continues. Interpreter evals run from the INTOP_BREAKPOINT handler after // the debugger callback returns — no context modification and no IncThreadsAtUnsafePlaces // needed because the stack remains walkable. - GetPendingEvals()->AddPendingEval(pDE->m_thread, pDE); + hr = GetPendingEvals()->AddPendingEval(pDE->m_thread, pDE); + if (FAILED(hr)) + { + DeleteInteropSafe(pDE); + return hr; + } LOG((LF_CORDB, LL_INFO1000, "D::FES: Non-hijack func eval setup for pDE:%p on thread %p (fInException=%d)\n", pDE, pThread, fInException)); @@ -14216,6 +14229,7 @@ HRESULT Debugger::FuncEvalSetup(DebuggerIPCE_FuncEvalInfo *pEvalInfo, // Return that all went well. Tracing the stack at this point should not show that the func eval is setup, but it // will show a wrong IP, so it shouldn't be done. + *argDataArea = pDE->m_argData; *debuggerEvalKey = pDE; LOG((LF_CORDB, LL_INFO100000, "D:FES for pDE:%p evalType:%d on thread %p, id=0x%x\n", diff --git a/src/coreclr/debug/ee/debugger.h b/src/coreclr/debug/ee/debugger.h index f2461a16864c74..09bba3f501bce4 100644 --- a/src/coreclr/debug/ee/debugger.h +++ b/src/coreclr/debug/ee/debugger.h @@ -455,9 +455,19 @@ class DebuggerModule * Hash to hold pending func evals by thread id * ------------------------------------------------------------------------ */ -struct DebuggerPendingFuncEval +// Array allocation for SHash elements must not take a heap lock held by a suspended thread. +class DebuggerHeapHashEntry +{ +public: +#ifndef DACCESS_COMPILE + static void *operator new[](size_t size); + static void *operator new[](size_t size, const std::nothrow_t &) noexcept; + static void operator delete[](void *memory); +#endif +}; + +struct DebuggerPendingFuncEval : DebuggerHeapHashEntry { - FREEHASHENTRY entry; PTR_Thread pThread; PTR_DebuggerEval pDE; }; @@ -2938,8 +2948,7 @@ void ApcActivationCallbackStubEnd(); }; -// CNewZeroData is the allocator used by the all the hash tables that the helper thread could possibly alter. It uses -// the interop safe allocator. +// Zero-initialized allocations on the debugger's interop-safe heap. class CNewZeroData { public: @@ -2985,183 +2994,102 @@ class CNewZeroData pHeap->Free(pPtr); } - static BYTE *Grow(BYTE *&pPtr, int iCurSize) - { - CONTRACTL - { - NOTHROW; - GC_NOTRIGGER; - PRECONDITION(g_pDebugger != NULL); - } - CONTRACTL_END; - - void *p; - - DebuggerHeap* pHeap = g_pDebugger->GetInteropSafeHeap_NoThrow(); - _ASSERTE(pHeap != NULL); // should already exist - - _ASSERTE( iCurSize >= 0 ); - S_UINT32 iNewSize = S_UINT32( iCurSize ) + S_UINT32( GrowSize(iCurSize) ); - if( iNewSize.IsOverflow() ) - { - return NULL; - } - p = pHeap->Realloc(pPtr, iNewSize.Value(), iCurSize); - if (p == NULL) - { - return NULL; - } - - memset((BYTE*)p+iCurSize, 0, GrowSize(iCurSize)); - return (pPtr = (BYTE *)p); - } - - // A hashtable may recycle memory. We need to zero it out again. - static void Clean(BYTE * pData, int iSize) - { - LIMITED_METHOD_CONTRACT; - - memset(pData, 0, iSize); - } #endif // DACCESS_COMPILE - - static int RoundSize(int iSize) - { - LIMITED_METHOD_CONTRACT; - - return (iSize); - } - static int GrowSize(int iCurSize) - { - LIMITED_METHOD_CONTRACT; - int newSize = (3 * iCurSize) / 2; - return (newSize < 256) ? 256 : newSize; - } }; -class DebuggerPendingFuncEvalTable : private CHashTableAndData +inline COUNT_T DebuggerHashPointer(TADDR pointer) { - public: - virtual ~DebuggerPendingFuncEvalTable() = default; - - private: - - BOOL Cmp(SIZE_T k1, const HASHENTRY * pc2) - { - LIMITED_METHOD_DAC_CONTRACT; + LIMITED_METHOD_DAC_CONTRACT; -#if defined(DACCESS_COMPILE) - // This function hasn't been tested yet in the DAC build. Make sure the DACization is correct. - DacNotImpl(); -#endif // DACCESS_COMPILE - - Thread * pThread1 = reinterpret_cast(k1); - Thread * pThread2 = dac_cast(const_cast(pc2))->pThread; - - return (pThread1 != pThread2); - } + COUNT_T hash = static_cast(pointer); +#ifdef HOST_64BIT + hash = HashCOUNT_T(hash, static_cast(pointer >> 32)); +#endif + return hash; +} - ULONG HASH(Thread* pThread) +template +class DebuggerSHash : public SHash +{ +public: + typename TRAITS::element_t *FindFirstEntry(HASHFIND *find) { - LIMITED_METHOD_CONTRACT; - return (ULONG)((SIZE_T)pThread); // only use low 32-bits if 64-bit + find->iNext = 0; + return FindNextEntry(find); } - - SIZE_T KEY(Thread * pThread) + typename TRAITS::element_t *FindNextEntry(HASHFIND *find) { - LIMITED_METHOD_CONTRACT; - return (SIZE_T)pThread; + typename TRAITS::count_t index = find->iNext; + const typename TRAITS::element_t *entry = this->GetNext(index); + find->iNext = index; + return const_cast(entry); } +}; - public: +class EMPTY_BASES DebuggerPendingFuncEvalTraits : public DefaultSHashTraits +{ +public: + typedef PTR_Thread key_t; + static key_t GetKey(const element_t &element) { return element.pThread; } + static BOOL Equals(key_t left, key_t right) { return left == right; } + static count_t Hash(key_t key) { return DebuggerHashPointer(dac_cast(key)); } + static element_t Null() { return {}; } + static element_t Deleted() + { + element_t element = {}; + element.pThread = dac_cast(static_cast(-1)); + return element; + } + static bool IsNull(const element_t &element) { return element.pThread == nullptr; } + static bool IsDeleted(const element_t &element) { return element.pThread == dac_cast(static_cast(-1)); } +}; +class DebuggerPendingFuncEvalTable +{ + SHash m_table; +public: #ifndef DACCESS_COMPILE - DebuggerPendingFuncEvalTable() : CHashTableAndData(11) - { - WRAPPER_NO_CONTRACT; - - SUPPRESS_ALLOCATION_ASSERTS_IN_THIS_SCOPE; - NewInit(11, sizeof(DebuggerPendingFuncEval), 11); - } - - void AddPendingEval(Thread *pThread, DebuggerEval *pDE) - { - WRAPPER_NO_CONTRACT; - - _ASSERTE((pThread != NULL) && (pDE != NULL)); - - DebuggerPendingFuncEval *pfe = (DebuggerPendingFuncEval*)Add(HASH(pThread)); - pfe->pThread = pThread; - pfe->pDE = pDE; - } - - void RemovePendingEval(Thread* pThread) - { - WRAPPER_NO_CONTRACT; - - _ASSERTE(pThread != NULL); - - DebuggerPendingFuncEval *entry = (DebuggerPendingFuncEval*)Find(HASH(pThread), KEY(pThread)); - Delete(HASH(pThread), (HASHENTRY*)entry); - } - + HRESULT AddPendingEval(Thread *pThread, DebuggerEval *pDE); + void RemovePendingEval(Thread *pThread); #endif // #ifndef DACCESS_COMPILE DebuggerPendingFuncEval *GetPendingEval(Thread* pThread) { WRAPPER_NO_CONTRACT; - DebuggerPendingFuncEval *entry = (DebuggerPendingFuncEval*)Find(HASH(pThread), KEY(pThread)); - return entry; + return const_cast(m_table.LookupPtr(dac_cast(pThread))); } }; -struct DebuggerModuleEntry +struct DebuggerModuleEntry : DebuggerHeapHashEntry { - FREEHASHENTRY entry; PTR_DebuggerModule module; }; typedef DPTR(struct DebuggerModuleEntry) PTR_DebuggerModuleEntry; -class DebuggerModuleTable : private CHashTableAndData +class EMPTY_BASES DebuggerModuleTraits : public DefaultSHashTraits { -#ifdef DACCESS_COMPILE - public: - virtual ~DebuggerModuleTable() = default; -#endif - - private: - - BOOL Cmp(SIZE_T k1, const HASHENTRY * pc2) - { - LIMITED_METHOD_DAC_CONTRACT; - -#if defined(DACCESS_COMPILE) - // This function hasn't been tested yet in the DAC build. Make sure the DACization is correct. - DacNotImpl(); -#endif // DACCESS_COMPILE - - Module * pModule1 = reinterpret_cast(k1); - Module * pModule2 = - dac_cast(const_cast(pc2))->module->GetRuntimeModule(); - - return (pModule1 != pModule2); - } - - ULONG HASH(Module* module) - { - LIMITED_METHOD_CONTRACT; - return (ULONG)((SIZE_T)module); // only use low 32-bits if 64-bit - } - - SIZE_T KEY(Module * pModule) - { - LIMITED_METHOD_CONTRACT; - return (SIZE_T)pModule; - } +public: + typedef PTR_Module key_t; + static key_t GetKey(const element_t &element) { return dac_cast(element.module->GetRuntimeModule()); } + static BOOL Equals(key_t left, key_t right) { return left == right; } + static count_t Hash(key_t key) { return DebuggerHashPointer(dac_cast(key)); } + static element_t Null() { return {}; } + static element_t Deleted() + { + element_t element = {}; + element.module = dac_cast(static_cast(-1)); + return element; + } + static bool IsNull(const element_t &element) { return element.module == nullptr; } + static bool IsDeleted(const element_t &element) { return element.module == dac_cast(static_cast(-1)); } +}; +class DebuggerModuleTable +{ + DebuggerSHash m_table; #ifdef _DEBUG bool ThreadHoldsLock(); #endif @@ -3170,8 +3098,7 @@ class DebuggerModuleTable : private CHashTableAndData #ifndef DACCESS_COMPILE - DebuggerModuleTable(); - virtual ~DebuggerModuleTable(); + ~DebuggerModuleTable(); void AddModule(DebuggerModule *module); @@ -3201,13 +3128,11 @@ struct DebuggerMethodInfoKey } ; // struct DebuggerMethodInfoEntry: Entry for the JIT info hash table. -// FREEHASHENTRY entry: Needed for use by the hash table // DebuggerMethodInfo * ji: The actual DebuggerMethodInfo to // hash. Note that DMI's will be hashed by MethodDesc. typedef DPTR(struct DebuggerMethodInfoEntry) PTR_DebuggerMethodInfoEntry; -struct DebuggerMethodInfoEntry +struct DebuggerMethodInfoEntry : DebuggerHeapHashEntry { - FREEHASHENTRY entry; DebuggerMethodInfoKey key; SIZE_T nVersion; SIZE_T nVersionLastRemapped; @@ -3226,7 +3151,28 @@ struct DebuggerMethodInfoEntry // INVARIANT: There is only one DebuggerMethodInfo per method // in the table. Note that DMI's will be hashed by MethodDesc. // -class DebuggerMethodInfoTable : private CHashTableAndData +class EMPTY_BASES DebuggerMethodInfoTraits : public DefaultSHashTraits +{ +public: + typedef DebuggerMethodInfoKey key_t; + static key_t GetKey(const element_t &element) { return element.key; } + static BOOL Equals(key_t left, key_t right) + { + return left.pModule == right.pModule && left.token == right.token; + } + static count_t Hash(key_t key) { return HashCOUNT_T(key.token, DebuggerHashPointer(dac_cast(key.pModule))); } + static element_t Null() { return {}; } + static element_t Deleted() + { + element_t element = {}; + element.key.pModule = dac_cast(static_cast(-1)); + return element; + } + static bool IsNull(const element_t &element) { return element.key.pModule == nullptr; } + static bool IsDeleted(const element_t &element) { return element.key.pModule == dac_cast(static_cast(-1)); } +}; + +class DebuggerMethodInfoTable { VPTR_BASE_CONCRETE_VTABLE_CLASS(DebuggerMethodInfoTable); @@ -3234,32 +3180,7 @@ class DebuggerMethodInfoTable : private CHashTableAndData virtual ~DebuggerMethodInfoTable() = default; private: - BOOL Cmp(SIZE_T k1, const HASHENTRY * pc2) - { - LIMITED_METHOD_DAC_CONTRACT; - - // This is the inverse of the KEY() function. - DebuggerMethodInfoKey * pDjik = reinterpret_cast(k1); - - DebuggerMethodInfoEntry * pDjie = dac_cast(const_cast(pc2)); - - return (pDjik->pModule != pDjie->key.pModule) || - (pDjik->token != pDjie->key.token); - } - - ULONG HASH(DebuggerMethodInfoKey* pDjik) - { - LIMITED_METHOD_DAC_CONTRACT; - return HashPtr( pDjik->token, pDjik->pModule ); - } - - SIZE_T KEY(DebuggerMethodInfoKey * pDjik) - { - // This is casting a host pointer to a SIZE_T. So that key is restricted to the host address space. - // This key is just passed to Cmp(), which will cast it back to a DebuggerMethodInfoKey*. - LIMITED_METHOD_DAC_CONTRACT; - return (SIZE_T)pDjik; - } + DebuggerSHash m_table; //#define _DEBUG_DMI_TABLE @@ -3281,8 +3202,6 @@ class DebuggerMethodInfoTable : private CHashTableAndData #ifndef DACCESS_COMPILE - DebuggerMethodInfoTable(); - HRESULT AddMethodInfo(Module *pModule, mdMethodDef token, DebuggerMethodInfo *mi); @@ -3387,6 +3306,7 @@ class DebuggerEval T_CONTEXT m_context; Thread *m_thread; + DebuggerEval *m_nextPendingEval; DebuggerIPCE_FuncEvalType m_evalType; mdMethodDef m_methodToken; mdTypeDef m_classToken; @@ -3458,21 +3378,21 @@ class DebuggerEval DeleteInteropSafe(m_externalMemoryOwner); } - // Clean up any temporary buffers used to send the argument type information. These were allocated - // in response to a GET_BUFFER message. - DebuggerIPCE_FuncEvalArgData *argData = GetArgData(); - for (unsigned int i = 0; i < m_argCount; i++) + if (m_argData) { - if (argData[i].fullArgType != (CORDB_ADDRESS)0) + // Clean up any temporary buffers used to send the argument type information. These were allocated + // in response to a GET_BUFFER message. + DebuggerIPCE_FuncEvalArgData *argData = GetArgData(); + for (unsigned int i = 0; i < m_argCount; i++) { - _ASSERTE(g_pDebugger != NULL); - g_pDebugger->ReleaseRemoteBuffer((BYTE*)CORDB_ADDRESS_TO_PTR(argData[i].fullArgType), true); + if (argData[i].fullArgType != (CORDB_ADDRESS)0) + { + _ASSERTE(g_pDebugger != NULL); + g_pDebugger->ReleaseRemoteBuffer((BYTE*)CORDB_ADDRESS_TO_PTR(argData[i].fullArgType), true); + } } - } - // Clean up the array of argument information. This was allocated as part of Func Eval setup. - if (m_argData) - { + // Clean up the array of argument information. This was allocated as part of Func Eval setup. DeleteInteropSafe(m_argData); } diff --git a/src/coreclr/debug/ee/debuggermodule.cpp b/src/coreclr/debug/ee/debuggermodule.cpp index f8f495aa85adf3..ee59d278f8ca0e 100644 --- a/src/coreclr/debug/ee/debuggermodule.cpp +++ b/src/coreclr/debug/ee/debuggermodule.cpp @@ -30,14 +30,6 @@ void DebuggerModule::SetCanChangeJitFlags(bool fCanChangeJitFlags) #ifndef DACCESS_COMPILE -DebuggerModuleTable::DebuggerModuleTable() : CHashTableAndData(101) -{ - WRAPPER_NO_CONTRACT; - - SUPPRESS_ALLOCATION_ASSERTS_IN_THIS_SCOPE; - NewInit(101, sizeof(DebuggerModuleEntry), 101); -} - DebuggerModuleTable::~DebuggerModuleTable() { WRAPPER_NO_CONTRACT; @@ -69,21 +61,19 @@ void DebuggerModuleTable::Clear() HASHFIND hf; DebuggerModuleEntry *pDME; - pDME = (DebuggerModuleEntry *) FindFirstEntry(&hf); + pDME = m_table.FindFirstEntry(&hf); while (pDME) { DebuggerModule *pDM = pDME->module; - Module *pEEM = pDM->GetRuntimeModule(); - TRACE_FREE(pDME->module); + m_table.Remove(pDM->GetRuntimeModule()); DeleteInteropSafe(pDM); - Delete(HASH(pEEM), (HASHENTRY *) pDME); - pDME = (DebuggerModuleEntry *) FindFirstEntry(&hf); + pDME = m_table.FindNextEntry(&hf); } - CHashTableAndData::Clear(); + m_table.RemoveAll(); } void DebuggerModuleTable::AddModule(DebuggerModule *pModule) @@ -102,13 +92,9 @@ void DebuggerModuleTable::AddModule(DebuggerModule *pModule) LOG((LF_CORDB, LL_EVERYTHING, "DMT::AM: DebuggerMod:%p Module:%p\n", pModule, pModule->GetRuntimeModule())); - DebuggerModuleEntry * pEntry = (DebuggerModuleEntry *) Add(HASH(pModule->GetRuntimeModule())); - if (pEntry == NULL) - { - ThrowOutOfMemory(); - } - - pEntry->module = pModule; + DebuggerModuleEntry entry = {}; + entry.module = pModule; + m_table.Add(entry); } //----------------------------------------------------------------------------- @@ -129,25 +115,17 @@ void DebuggerModuleTable::RemoveModule(Module* pModule) _ASSERTE(ThreadHoldsLock()); _ASSERTE(pModule != NULL); - HASHFIND hf; - - for (DebuggerModuleEntry *pDME = (DebuggerModuleEntry *) FindFirstEntry(&hf); - pDME != NULL; - pDME = (DebuggerModuleEntry*) FindNextEntry(&hf)) + const DebuggerModuleEntry *entry = m_table.LookupPtr(pModule); + if (entry != nullptr) { - DebuggerModule *pDM = pDME->module; - Module* pRuntimeModule = pDM->GetRuntimeModule(); - - if (pRuntimeModule == pModule) - { - LOG((LF_CORDB, LL_INFO1000, "DMT::RM Removing DebuggerMod:%p - Module:%p Asm:%p\n", - pDM, pModule, pDM->GetAssembly())); - TRACE_FREE(pDM); - DeleteInteropSafe(pDM); - Delete(HASH(pRuntimeModule), (HASHENTRY *) pDME); - _ASSERTE(GetModule(pModule) == NULL); - return; - } + DebuggerModule *pDM = entry->module; + LOG((LF_CORDB, LL_INFO1000, "DMT::RM Removing DebuggerMod:%p - Module:%p Asm:%p\n", + pDM, pModule, pDM->GetAssembly())); + TRACE_FREE(pDM); + m_table.Remove(pModule); + DeleteInteropSafe(pDM); + _ASSERTE(GetModule(pModule) == NULL); + return; } LOG((LF_CORDB, LL_INFO1000, "DMT::RM No debugger module found for Module:%p\n", pModule)); @@ -168,8 +146,7 @@ DebuggerModule *DebuggerModuleTable::GetModule(Module* module) _ASSERTE(module != NULL); _ASSERTE(ThreadHoldsLock()); - DebuggerModuleEntry *entry - = (DebuggerModuleEntry *) Find(HASH(module), KEY(module)); + const DebuggerModuleEntry *entry = m_table.LookupPtr(dac_cast(module)); if (entry == NULL) return NULL; else @@ -187,7 +164,7 @@ DebuggerModule *DebuggerModuleTable::GetFirstModule(HASHFIND *info) _ASSERTE(ThreadHoldsLock()); - DebuggerModuleEntry *entry = (DebuggerModuleEntry *) FindFirstEntry(info); + DebuggerModuleEntry *entry = m_table.FindFirstEntry(info); if (entry == NULL) return NULL; else @@ -205,7 +182,7 @@ DebuggerModule *DebuggerModuleTable::GetNextModule(HASHFIND *info) _ASSERTE(ThreadHoldsLock()); - DebuggerModuleEntry *entry = (DebuggerModuleEntry *) FindNextEntry(info); + DebuggerModuleEntry *entry = m_table.FindNextEntry(info); if (entry == NULL) return NULL; else diff --git a/src/coreclr/debug/ee/functioninfo.cpp b/src/coreclr/debug/ee/functioninfo.cpp index 6c0ac028cb6c91..a31f03c7d56c92 100644 --- a/src/coreclr/debug/ee/functioninfo.cpp +++ b/src/coreclr/debug/ee/functioninfo.cpp @@ -2095,25 +2095,6 @@ DebuggerJitInfo* DebuggerMethodInfo::GetLatestJitInfo_NoCreate() } -DebuggerMethodInfoTable::DebuggerMethodInfoTable() : CHashTableAndData(101) -{ - CONTRACTL - { - WRAPPER(THROWS); - GC_NOTRIGGER; - - } - CONTRACTL_END; - - SUPPRESS_ALLOCATION_ASSERTS_IN_THIS_SCOPE; - HRESULT hr = NewInit(101, sizeof(DebuggerMethodInfoEntry), 101); - - if (FAILED(hr)) - { - ThrowWin32(hr); - } -} - HRESULT DebuggerMethodInfoTable::AddMethodInfo(Module *pModule, mdMethodDef token, DebuggerMethodInfo *mi) @@ -2144,18 +2125,10 @@ HRESULT DebuggerMethodInfoTable::AddMethodInfo(Module *pModule, dmik.pModule = pModule; dmik.token = token; - DebuggerMethodInfoEntry *dmie = - (DebuggerMethodInfoEntry *) Add(HASH(&dmik)); - - if (dmie == NULL) - { - ThrowOutOfMemory(); - return E_OUTOFMEMORY; - } - - dmie->key.pModule = pModule; - dmie->key.token = token; - dmie->mi = mi; + DebuggerMethodInfoEntry entry = {}; + entry.key = dmik; + entry.mi = mi; + m_table.Add(entry); LOG((LF_CORDB, LL_INFO1000, "DMIT::AMI: mod:%p tok:0x%08x\n", pModule, token)); @@ -2187,7 +2160,7 @@ HRESULT DebuggerMethodInfoTable::OverwriteMethodInfo(Module *pModule, dmik.token = token; DebuggerMethodInfoEntry *entry - = (DebuggerMethodInfoEntry *) Find(HASH(&dmik), KEY(&dmik)); + = const_cast(m_table.LookupPtr(dmik)); if (entry != NULL) { if ( (fOnlyIfNull && entry->mi == NULL) @@ -2221,7 +2194,7 @@ void DebuggerMethodInfoTable::ClearMethodsOfModule(Module *pModule) HASHFIND info; DebuggerMethodInfoEntry *entry - = (DebuggerMethodInfoEntry *) FindFirstEntry(&info); + = m_table.FindFirstEntry(&info); while(entry != NULL) { Module *pMod = entry->key.pModule ; @@ -2234,7 +2207,7 @@ void DebuggerMethodInfoTable::ClearMethodsOfModule(Module *pModule) DeleteEntryDMI(entry); } - Delete(HASH(&(entry->key)), (HASHENTRY*)entry); + m_table.Remove(entry->key); } else { @@ -2259,7 +2232,7 @@ void DebuggerMethodInfoTable::ClearMethodsOfModule(Module *pModule) } } - entry = (DebuggerMethodInfoEntry *) FindNextEntry(&info); + entry = m_table.FindNextEntry(&info); } } @@ -2321,7 +2294,7 @@ DebuggerMethodInfo *DebuggerMethodInfoTable::GetMethodInfo(Module *pModule, mdMe dmik.pModule = dac_cast(pModule); dmik.token = token; - DebuggerMethodInfoEntry *entry = dac_cast(Find(HASH(&dmik), KEY(&dmik))); + const DebuggerMethodInfoEntry *entry = m_table.LookupPtr(dmik); if (entry == NULL ) { @@ -2349,8 +2322,7 @@ DebuggerMethodInfo *DebuggerMethodInfoTable::GetFirstMethodInfo(HASHFIND *info) _ASSERTE(g_pDebugger->HasDebuggerDataLock()); - DebuggerMethodInfoEntry *entry = PTR_DebuggerMethodInfoEntry - (PTR_HOST_TO_TADDR(FindFirstEntry(info))); + DebuggerMethodInfoEntry *entry = m_table.FindFirstEntry(info); if (entry == NULL) return NULL; else @@ -2372,8 +2344,7 @@ DebuggerMethodInfo *DebuggerMethodInfoTable::GetNextMethodInfo(HASHFIND *info) _ASSERTE(g_pDebugger->HasDebuggerDataLock()); - DebuggerMethodInfoEntry *entry = PTR_DebuggerMethodInfoEntry - (PTR_HOST_TO_TADDR(FindNextEntry(info))); + DebuggerMethodInfoEntry *entry = m_table.FindNextEntry(info); // We may have incremented the version number // for methods that never got JITted, so we should @@ -2381,8 +2352,7 @@ DebuggerMethodInfo *DebuggerMethodInfoTable::GetNextMethodInfo(HASHFIND *info) while (entry != NULL && entry->mi == NULL) { - entry = PTR_DebuggerMethodInfoEntry - (PTR_HOST_TO_TADDR(FindNextEntry(info))); + entry = m_table.FindNextEntry(info); } if (entry == NULL) @@ -2401,9 +2371,7 @@ DebuggerMethodInfoEntry::EnumMemoryRegions(CLRDataEnumMemoryFlags flags) { SUPPORTS_DAC; - // This structure is in an array in the hash - // so the 'this' is implicitly enumerated by the - // array enum in CHashTable. + // The containing SHash enumerates the entry storage. // For a MiniDumpNormal, what is needed for modules is already enumerated elsewhere. // Don't waste time doing it here an extra time. Also, this will add many MB extra into the dump. @@ -2412,10 +2380,11 @@ DebuggerMethodInfoEntry::EnumMemoryRegions(CLRDataEnumMemoryFlags flags) key.pModule->EnumMemoryRegions(flags, true); } - while (mi.IsValid()) + PTR_DebuggerMethodInfo methodInfo = mi; + while (methodInfo.IsValid()) { - mi->EnumMemoryRegions(flags); - mi = mi->m_prevMethodInfo; + methodInfo->EnumMemoryRegions(flags); + methodInfo = methodInfo->m_prevMethodInfo; } } @@ -2452,12 +2421,13 @@ void DebuggerMethodInfoTable::EnumMemoryRegions(CLRDataEnumMemoryFlags flags) WRAPPER_NO_CONTRACT; DAC_ENUM_VTHIS(); - CHashTableAndData::EnumMemoryRegions(flags); + m_table.EnumMemoryRegions(); - for (ULONG i = 0; i < m_iEntries; i++) + HASHFIND find; + for (DebuggerMethodInfoEntry *entry = m_table.FindFirstEntry(&find); + entry != nullptr; + entry = m_table.FindNextEntry(&find)) { - DebuggerMethodInfoEntry* entry = - PTR_DebuggerMethodInfoEntry(PTR_HOST_TO_TADDR(EntryPtr(i))); entry->EnumMemoryRegions(flags); } } diff --git a/src/tests/baseservices/exceptions/exceptionstacktrace/exceptionstacktrace.cs b/src/tests/baseservices/exceptions/exceptionstacktrace/exceptionstacktrace.cs index e6d5789a4fc47f..53473a567941a1 100644 --- a/src/tests/baseservices/exceptions/exceptionstacktrace/exceptionstacktrace.cs +++ b/src/tests/baseservices/exceptions/exceptionstacktrace/exceptionstacktrace.cs @@ -17,6 +17,54 @@ public static class Program static Exception theException = new Exception("My exception"); static int recursion; + [ConditionalTheory(typeof(Utilities), nameof(Utilities.IsReflectionEmitSupported))] + [InlineData(17)] + [InlineData(257)] + public static void StackTracesAfterCollectibleMethodTableGrowth(int methodCount) + { + for (int generation = 0; generation < 3; generation++) + { + WeakReference type = CreateAndTraceCollectibleMethods(methodCount); + for (int attempt = 0; type.IsAlive && attempt < 10; attempt++) + { + GC.Collect(); + GC.WaitForPendingFinalizers(); + GC.Collect(); + } + + Assert.False(type.IsAlive); + } + } + + [MethodImpl(MethodImplOptions.NoInlining)] + private static WeakReference CreateAndTraceCollectibleMethods(int methodCount) + { + AssemblyBuilder assembly = AssemblyBuilder.DefineDynamicAssembly( + new AssemblyName("CollectibleStackTraces"), AssemblyBuilderAccess.RunAndCollect); + TypeBuilder builder = assembly.DefineDynamicModule("Methods").DefineType("ThrowingMethods"); + ConstructorInfo constructor = typeof(InvalidOperationException).GetConstructor(Type.EmptyTypes); + for (int index = 0; index < methodCount; index++) + { + MethodBuilder method = builder.DefineMethod( + "Throw" + index, MethodAttributes.Public | MethodAttributes.Static, typeof(void), Type.EmptyTypes); + method.SetImplementationFlags(MethodImplAttributes.NoInlining); + ILGenerator il = method.GetILGenerator(); + il.Emit(OpCodes.Newobj, constructor); + il.Emit(OpCodes.Throw); + } + + Type type = builder.CreateType(); + for (int index = 0; index < methodCount; index++) + { + string name = "Throw" + index; + Action action = type.GetMethod(name).CreateDelegate(); + InvalidOperationException exception = Assert.Throws(action); + Assert.Contains("ThrowingMethods." + name + "()", exception.ToString()); + } + + return new WeakReference(type); + } + private static void VerifyStackTrace(Exception e, string[] templateStackTrace) { string[] stackTrace = e.StackTrace.Split(Environment.NewLine, StringSplitOptions.RemoveEmptyEntries); From c05a3339d3f245e6538fde0d43a181f48141f2f3 Mon Sep 17 00:00:00 2001 From: Aaron Robinson Date: Wed, 21 Oct 2026 10:51:36 -0700 Subject: [PATCH 03/13] Replace final CHashTable consumer and remove legacy tables Use SHash for profiler allocation counts and remove CHashTable, CHashTableAndData, and their unused allocators. Validated ICorProfilerCallback::ObjectsAllocatedByClass and allocation-statistics suppression. Full x64 Checked CoreCLR and Release libraries/host build passed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ce4b0890-6559-4b90-b46f-11179e70f973 --- src/coreclr/inc/utilcode.h | 542 ------------------ src/coreclr/utilcode/collections.cpp | 298 ---------- src/coreclr/vm/eetoprofinterfaceimpl.cpp | 160 +----- src/coreclr/vm/eetoprofinterfaceimpl.h | 37 +- .../profiler/native/gcprofiler/gcprofiler.cpp | 10 + 5 files changed, 51 insertions(+), 996 deletions(-) diff --git a/src/coreclr/inc/utilcode.h b/src/coreclr/inc/utilcode.h index 159aac8908e729..ddd516e2ef7aff 100644 --- a/src/coreclr/inc/utilcode.h +++ b/src/coreclr/inc/utilcode.h @@ -1364,548 +1364,6 @@ struct HASHFIND }; -//***************************************************************************** -// IMPORTANT: This data structure is deprecated, please do not add any new uses. -// The hashtable implementation that should be used instead is code:SHash. -// If code:SHash does not work for you, talk to mailto:clrdeag. -//***************************************************************************** -// This is a class that implements a chain and bucket hash table. -// -// The data is actually supplied as an array of structures by the user of this class. -// This allows the buckets to use small indices to point to the chain, instead of pointers. -// -// Each entry in the array contains a HASHENTRY structure immediately -// followed by the key used to hash the structure. -// -// The HASHENTRY part of every structure is used to implement the chain of -// entries in a single bucket. -// -// This implementation does not support rehashing the buckets if the table grows -// to big. -// @TODO: Fix this by adding an abstract function Hash() which must be implemented -// by all clients. -// -//***************************************************************************** -class CHashTable -{ - friend class DebuggerRCThread; //RCthread actually needs access to - //fields of derrived class DebuggerPatchTable - -protected: - TADDR m_pcEntries; // Pointer to the array of structs. - ULONG m_iEntrySize; // Size of the structs. - - ULONG m_iBuckets; // # of chains we are hashing into. - PTR_ULONG m_piBuckets; // Ptr to the array of bucket chains. - - INDEBUG(unsigned m_maxSearch;) // For evaluating perf characteristics - - HASHENTRY *EntryPtr(ULONG iEntry) - { - LIMITED_METHOD_DAC_CONTRACT; - return (PTR_HASHENTRY(m_pcEntries + (iEntry * (size_t)m_iEntrySize))); - } - - ULONG ItemIndex(HASHENTRY *p) - { - SUPPORTS_DAC; - LIMITED_METHOD_CONTRACT; - return (ULONG)((dac_cast(p) - m_pcEntries) / m_iEntrySize); - } - - -public: - - CHashTable( - ULONG iBuckets) : // # of chains we are hashing into. - m_pcEntries((TADDR)NULL), - m_iBuckets(iBuckets) - { - LIMITED_METHOD_CONTRACT; - - m_piBuckets = NULL; - - INDEBUG(m_maxSearch = 0;) - } - - CHashTable() : // # of chains we are hashing into. - m_pcEntries((TADDR)NULL), - m_iBuckets(5) - { - LIMITED_METHOD_CONTRACT; - - m_piBuckets = NULL; - - INDEBUG(m_maxSearch = 0;) - } - -#ifndef DACCESS_COMPILE - - ~CHashTable() - { - LIMITED_METHOD_CONTRACT; - if (m_piBuckets != NULL) - { - delete [] m_piBuckets; - m_piBuckets = NULL; - } - } - -//***************************************************************************** -// This is the second part of construction where we do all of the work that -// can fail. We also take the array of structs here because the calling class -// presumably needs to allocate it in its NewInit. -//***************************************************************************** - HRESULT NewInit( // Return status. - BYTE *pcEntries, // Array of structs we are managing. - ULONG iEntrySize); // Size of the entries. - -//***************************************************************************** -// This can be called to change the pointer to the table that the hash table -// is managing. You might call this if (for example) you realloc the size -// of the table and its pointer is different. -//***************************************************************************** - void SetTable( - BYTE *pcEntries) // Array of structs we are managing. - { - LIMITED_METHOD_CONTRACT; - m_pcEntries = (TADDR)pcEntries; - } - -//***************************************************************************** -// Clear the hash table as if there were nothing in it. -//***************************************************************************** - void Clear() - { - LIMITED_METHOD_CONTRACT; - _ASSERTE(m_piBuckets != NULL); - memset(m_piBuckets, 0xff, m_iBuckets * sizeof(ULONG)); - } - -//***************************************************************************** -// Add the struct at the specified index in m_pcEntries to the hash chains. -//***************************************************************************** - BYTE *Add( // New entry. - ULONG iHash, // Hash value of entry to add. - ULONG iIndex); // Index of struct in m_pcEntries. - -//***************************************************************************** -// Delete the struct at the specified index in m_pcEntries from the hash chains. -//***************************************************************************** - void Delete( - ULONG iHash, // Hash value of entry to delete. - ULONG iIndex); // Index of struct in m_pcEntries. - - void Delete( - ULONG iHash, // Hash value of entry to delete. - HASHENTRY *psEntry); // The struct to delete. - -//***************************************************************************** -// The item at the specified index has been moved, update the previous and -// next item. -//***************************************************************************** - void Move( - ULONG iHash, // Hash value for the item. - ULONG iNew); // New location. - -#endif // #ifndef DACCESS_COMPILE - -//***************************************************************************** -// Return a boolean indicating whether or not this hash table has been inited. -//***************************************************************************** - int IsInited() - { - LIMITED_METHOD_CONTRACT; - return (m_piBuckets != NULL); - } - -//***************************************************************************** -// Search the hash table for an entry with the specified key value. -//***************************************************************************** - BYTE *Find( // Index of struct in m_pcEntries. - ULONG iHash, // Hash value of the item. - SIZE_T key); // The key to match. - -//***************************************************************************** -// Search the hash table for the next entry with the specified key value. -//***************************************************************************** - ULONG FindNext( // Index of struct in m_pcEntries. - SIZE_T key, // The key to match. - ULONG iIndex); // Index of previous match. - -//***************************************************************************** -// Returns the first entry in the first hash bucket and inits the search -// struct. Use the FindNextEntry function to continue walking the list. The -// return order is not guaranteed. -//***************************************************************************** - BYTE *FindFirstEntry( // First entry found, or 0. - HASHFIND *psSrch) // Search object. - { - WRAPPER_NO_CONTRACT; - if (m_piBuckets == nullptr) - return (0); - psSrch->iBucket = 1; - psSrch->iNext = m_piBuckets[0]; - return (FindNextEntry(psSrch)); - } - -//***************************************************************************** -// Returns the next entry in the list. -//***************************************************************************** - BYTE *FindNextEntry( // The next entry, or0 for end of list. - HASHFIND *psSrch); // Search object. - -#ifdef DACCESS_COMPILE - void EnumMemoryRegions(CLRDataEnumMemoryFlags flags, - ULONG numEntries); -#endif - -protected: - virtual BOOL Cmp(SIZE_T key1, const HASHENTRY * pc2) = 0; -}; - - -class CNewData -{ -public: - static BYTE *Alloc(int iSize, int iMaxSize) - { - WRAPPER_NO_CONTRACT; - return (new BYTE[iSize]); - } - static void Free(BYTE *pPtr, int iSize) - { - LIMITED_METHOD_CONTRACT; - delete [] pPtr; - } - static BYTE *Grow(BYTE *&pPtr, int iCurSize) - { - WRAPPER_NO_CONTRACT; - BYTE *p; - S_SIZE_T newSize = S_SIZE_T(iCurSize) + S_SIZE_T(GrowSize(iCurSize)); - //check for overflow - if(newSize.IsOverflow()) - p = NULL; - else - p = new (nothrow) BYTE[newSize.Value()]; - if (p == 0) return (0); - memcpy (p, pPtr, iCurSize); - delete [] pPtr; - pPtr = p; - return pPtr; - } - static void Clean(BYTE * pData, int iSize) - { - } - static int RoundSize(int iSize) - { - LIMITED_METHOD_CONTRACT; - return (iSize); - } - static int GrowSize(int iCurSize) - { - LIMITED_METHOD_CONTRACT; - int newSize = (3 * iCurSize) / 2; - return (newSize < 256) ? 256 : newSize; - } -}; - -class CNewDataNoThrow -{ -public: - static BYTE *Alloc(int iSize, int iMaxSize) - { - WRAPPER_NO_CONTRACT; - return (new (nothrow) BYTE[iSize]); - } - static void Free(BYTE *pPtr, int iSize) - { - LIMITED_METHOD_CONTRACT; - delete [] pPtr; - } - static BYTE *Grow(BYTE *&pPtr, int iCurSize) - { - WRAPPER_NO_CONTRACT; - BYTE *p; - S_SIZE_T newSize = S_SIZE_T(iCurSize) + S_SIZE_T(GrowSize(iCurSize)); - //check for overflow - if(newSize.IsOverflow()) - p = NULL; - else - p = new (nothrow) BYTE[newSize.Value()]; - if (p == 0) return (0); - memcpy (p, pPtr, iCurSize); - delete [] pPtr; - pPtr = p; - return pPtr; - } - static void Clean(BYTE * pData, int iSize) - { - } - static int RoundSize(int iSize) - { - LIMITED_METHOD_CONTRACT; - return (iSize); - } - static int GrowSize(int iCurSize) - { - LIMITED_METHOD_CONTRACT; - int newSize = (3 * iCurSize) / 2; - return (newSize < 256) ? 256 : newSize; - } -}; - - -//***************************************************************************** -// IMPORTANT: This data structure is deprecated, please do not add any new uses. -// The hashtable implementation that should be used instead is code:SHash. -// If code:SHash does not work for you, talk to mailto:clrdeag. -//***************************************************************************** -// CHashTable expects the data to be in a single array - this is provided by -// CHashTableAndData. -// The array is allocated using the MemMgr type. CNewData and -// CNewDataNoThrow can be used for this. -//***************************************************************************** -template -class CHashTableAndData : public CHashTable -{ -public: - DAC_ALIGNAS(CHashTable) - ULONG m_iFree; // Index into m_pcEntries[] of next available slot - ULONG m_iEntries; // size of m_pcEntries[] - -public: - - CHashTableAndData() : - CHashTable() - { - LIMITED_METHOD_CONTRACT; - } - - CHashTableAndData( - ULONG iBuckets) : // # of chains we are hashing into. - CHashTable(iBuckets) - { - LIMITED_METHOD_CONTRACT; - } - -#ifndef DACCESS_COMPILE - - ~CHashTableAndData() - { - WRAPPER_NO_CONTRACT; - if (m_pcEntries != (TADDR)NULL) - MemMgr::Free((BYTE*)m_pcEntries, MemMgr::RoundSize(m_iEntries * m_iEntrySize)); - } - -//***************************************************************************** -// This is the second part of construction where we do all of the work that -// can fail. We also take the array of structs here because the calling class -// presumably needs to allocate it in its NewInit. -//***************************************************************************** - HRESULT NewInit( // Return status. - ULONG iEntries, // # of entries. - ULONG iEntrySize, // Size of the entries. - int iMaxSize); // Max size of data. - -//***************************************************************************** -// Clear the hash table as if there were nothing in it. -//***************************************************************************** - void Clear() - { - WRAPPER_NO_CONTRACT; - m_iFree = 0; - InitFreeChain(0, m_iEntries); - CHashTable::Clear(); - } - -//***************************************************************************** -// Grabs a slot for the new entry to be added. -// The caller should fill in the non-HASHENTRY part of the returned slot -//***************************************************************************** - BYTE *Add( - ULONG iHash) // Hash value of entry to add. - { - WRAPPER_NO_CONTRACT; - FREEHASHENTRY *psEntry; - - // Make the table bigger if necessary. - if (m_iFree == UINT32_MAX && !Grow()) - return (NULL); - - // Add the first entry from the free list to the hash chain. - psEntry = (FREEHASHENTRY *) CHashTable::Add(iHash, m_iFree); - m_iFree = psEntry->iFree; - - // If we're recycling memory, give our memory-allocator a chance to re-init it. - - // Each entry is prefixed with a header - we don't want to trash that. - SIZE_T cbHeader = sizeof(FREEHASHENTRY); - MemMgr::Clean((BYTE*) psEntry + cbHeader, (int) (m_iEntrySize - cbHeader)); - - return ((BYTE *) psEntry); - } - -//***************************************************************************** -// Delete the struct at the specified index in m_pcEntries from the hash chains. -//***************************************************************************** - void Delete( - ULONG iHash, // Hash value of entry to delete. - ULONG iIndex) // Index of struct in m_pcEntries. - { - WRAPPER_NO_CONTRACT; - CHashTable::Delete(iHash, iIndex); - ((FREEHASHENTRY *) EntryPtr(iIndex))->iFree = m_iFree; - m_iFree = iIndex; - } - - void Delete( - ULONG iHash, // Hash value of entry to delete. - HASHENTRY *psEntry) // The struct to delete. - { - WRAPPER_NO_CONTRACT; - CHashTable::Delete(iHash, psEntry); - ((FREEHASHENTRY *) psEntry)->iFree = m_iFree; - m_iFree = ItemIndex(psEntry); - } - -#endif // #ifndef DACCESS_COMPILE - - // This is a sad legacy workaround. The debugger's patch table (implemented as this - // class) is shared across process. We publish the runtime offsets of - // some key fields. Since those fields are private, we have to provide - // accessors here. So if you're not using these functions, don't start. - // We can hopefully remove them. - // Note that we can't just make RCThread a friend of this class (we tried - // originally) because the inheritance chain has a private modifier, - // so DebuggerPatchTable::m_pcEntries is illegal. - static SIZE_T helper_GetOffsetOfEntries() - { - LIMITED_METHOD_CONTRACT; - return offsetof(CHashTableAndData, m_pcEntries); - } - - static SIZE_T helper_GetOffsetOfCount() - { - LIMITED_METHOD_CONTRACT; - return offsetof(CHashTableAndData, m_iEntries); - } - -#ifdef DACCESS_COMPILE - void EnumMemoryRegions(CLRDataEnumMemoryFlags flags) - { - SUPPORTS_DAC; - CHashTable::EnumMemoryRegions(flags, m_iEntries); - } -#endif - -private: - void InitFreeChain(ULONG iStart,ULONG iEnd); - int Grow(); -}; - -#ifndef DACCESS_COMPILE - -//***************************************************************************** -// This is the second part of construction where we do all of the work that -// can fail. We also take the array of structs here because the calling class -// presumably needs to allocate it in its NewInit. -//***************************************************************************** -template -HRESULT CHashTableAndData::NewInit(// Return status. - ULONG iEntries, // # of entries. - ULONG iEntrySize, // Size of the entries. - int iMaxSize) // Max size of data. -{ - WRAPPER_NO_CONTRACT; - BYTE *pcEntries; - HRESULT hr; - - - // note that this function can throw because it depends on the ::Alloc - - // Allocate the memory for the entries. - if ((pcEntries = MemMgr::Alloc(MemMgr::RoundSize(iEntries * iEntrySize), - MemMgr::RoundSize(iMaxSize))) == 0) - return (E_OUTOFMEMORY); - m_iEntries = iEntries; - - // Init the base table. - if (FAILED(hr = CHashTable::NewInit(pcEntries, iEntrySize))) - MemMgr::Free(pcEntries, MemMgr::RoundSize(iEntries * iEntrySize)); - else - { - // Init the free chain. - m_iFree = 0; - InitFreeChain(0, iEntries); - } - return (hr); -} - -//***************************************************************************** -// Initialize a range of records such that they are linked together to be put -// on the free chain. -//***************************************************************************** -template -void CHashTableAndData::InitFreeChain( - ULONG iStart, // Index to start initializing. - ULONG iEnd) // Index to stop initializing -{ - LIMITED_METHOD_CONTRACT; - BYTE* pcPtr; - _ASSERTE(iEnd > iStart); - - pcPtr = (BYTE*)m_pcEntries + iStart * (size_t)m_iEntrySize; - for (++iStart; iStart < iEnd; ++iStart) - { - ((FREEHASHENTRY *) pcPtr)->iFree = iStart; - pcPtr += m_iEntrySize; - } - ((FREEHASHENTRY *) pcPtr)->iFree = UINT32_MAX; -} - -//***************************************************************************** -// Attempt to increase the amount of space available for the record heap. -//***************************************************************************** -template -int CHashTableAndData::Grow() // 1 if successful, 0 if not. -{ - WRAPPER_NO_CONTRACT; - int iCurSize; // Current size in bytes. - int iEntries; // New # of entries. - - _ASSERTE(m_pcEntries != (TADDR)NULL); - _ASSERTE(m_iFree == UINT32_MAX); - - // Compute the current size and new # of entries. - S_UINT32 iTotEntrySize = S_UINT32(m_iEntries) * S_UINT32(m_iEntrySize); - if( iTotEntrySize.IsOverflow() ) - { - _ASSERTE( !"CHashTableAndData overflow!" ); - return (0); - } - iCurSize = MemMgr::RoundSize( iTotEntrySize.Value() ); - iEntries = (iCurSize + MemMgr::GrowSize(iCurSize)) / m_iEntrySize; - - if ( (iEntries < 0) || ((ULONG)iEntries <= m_iEntries) ) - { - _ASSERTE( !"CHashTableAndData overflow!" ); - return (0); - } - - // Try to expand the array. - if (MemMgr::Grow(*(BYTE**)&m_pcEntries, iCurSize) == 0) - return (0); - - // Init the newly allocated space. - InitFreeChain(m_iEntries, iEntries); - m_iFree = m_iEntries; - m_iEntries = iEntries; - return (1); -} - -#endif // #ifndef DACCESS_COMPILE - //***************************************************************************** //***************************************************************************** diff --git a/src/coreclr/utilcode/collections.cpp b/src/coreclr/utilcode/collections.cpp index e6a165efbb6a3d..a6560a555a3b64 100644 --- a/src/coreclr/utilcode/collections.cpp +++ b/src/coreclr/utilcode/collections.cpp @@ -11,304 +11,6 @@ #include "utilcode.h" #include "ex.h" -// -// -// CHashTable -// -// - -#ifndef DACCESS_COMPILE - -//***************************************************************************** -// This is the second part of construction where we do all of the work that -// can fail. We also take the array of structs here because the calling class -// presumably needs to allocate it in its NewInit. -//***************************************************************************** -HRESULT CHashTable::NewInit( // Return status. - BYTE *pcEntries, // Array of structs we are managing. - ULONG iEntrySize) // Size of the entries. -{ - CONTRACTL - { - NOTHROW; - GC_NOTRIGGER; - } - CONTRACTL_END; - - _ASSERTE(iEntrySize >= sizeof(FREEHASHENTRY)); - - // Allocate the bucket chain array and init it. - if ((m_piBuckets = new (nothrow) ULONG [m_iBuckets]) == NULL) - return (OutOfMemory()); - memset(m_piBuckets, 0xff, m_iBuckets * sizeof(ULONG)); - - // Save the array of structs we are managing. - m_pcEntries = (TADDR)pcEntries; - m_iEntrySize = iEntrySize; - return (S_OK); -} - -//***************************************************************************** -// Add the struct at the specified index in m_pcEntries to the hash chains. -//***************************************************************************** -BYTE *CHashTable::Add( // New entry. - ULONG iHash, // Hash value of entry to add. - ULONG iIndex) // Index of struct in m_pcEntries. -{ - CONTRACTL - { - NOTHROW; - GC_NOTRIGGER; - } - CONTRACTL_END; - - HASHENTRY *psEntry; // The struct we are adding. - - // Get a pointer to the entry we are adding. - psEntry = EntryPtr(iIndex); - - // Compute the hash value for the entry. - iHash %= m_iBuckets; - - _ASSERTE(m_piBuckets[iHash] != iIndex && - (m_piBuckets[iHash] == UINT32_MAX || EntryPtr(m_piBuckets[iHash])->iPrev != iIndex)); - - // Setup this entry. - psEntry->iPrev = UINT32_MAX; - psEntry->iNext = m_piBuckets[iHash]; - - // Link it into the hash chain. - if (m_piBuckets[iHash] != UINT32_MAX) - EntryPtr(m_piBuckets[iHash])->iPrev = iIndex; - m_piBuckets[iHash] = iIndex; - return ((BYTE *) psEntry); -} - -//***************************************************************************** -// Delete the struct at the specified index in m_pcEntries from the hash chains. -//***************************************************************************** -void CHashTable::Delete( - ULONG iHash, // Hash value of entry to delete. - ULONG iIndex) // Index of struct in m_pcEntries. -{ - WRAPPER_NO_CONTRACT; - - HASHENTRY *psEntry; // Struct to delete. - - // Get a pointer to the entry we are deleting. - psEntry = EntryPtr(iIndex); - Delete(iHash, psEntry); -} - -//***************************************************************************** -// Delete the struct at the specified index in m_pcEntries from the hash chains. -//***************************************************************************** -void CHashTable::Delete( - ULONG iHash, // Hash value of entry to delete. - HASHENTRY *psEntry) // The struct to delete. -{ - CONTRACTL - { - NOTHROW; - GC_NOTRIGGER; - } - CONTRACTL_END; - - // Compute the hash value for the entry. - iHash %= m_iBuckets; - - _ASSERTE(psEntry->iPrev != psEntry->iNext || psEntry->iPrev == UINT32_MAX); - - // Fix the predecessor. - if (psEntry->iPrev == UINT32_MAX) - m_piBuckets[iHash] = psEntry->iNext; - else - EntryPtr(psEntry->iPrev)->iNext = psEntry->iNext; - - // Fix the successor. - if (psEntry->iNext != UINT32_MAX) - EntryPtr(psEntry->iNext)->iPrev = psEntry->iPrev; -} - -//***************************************************************************** -// The item at the specified index has been moved, update the previous and -// next item. -//***************************************************************************** -void CHashTable::Move( - ULONG iHash, // Hash value for the item. - ULONG iNew) // New location. -{ - CONTRACTL - { - NOTHROW; - GC_NOTRIGGER; - } - CONTRACTL_END; - - HASHENTRY *psEntry; // The struct we are deleting. - - psEntry = EntryPtr(iNew); - _ASSERTE(psEntry->iPrev != iNew && psEntry->iNext != iNew); - - if (psEntry->iPrev != UINT32_MAX) - EntryPtr(psEntry->iPrev)->iNext = iNew; - else - m_piBuckets[iHash % m_iBuckets] = iNew; - if (psEntry->iNext != UINT32_MAX) - EntryPtr(psEntry->iNext)->iPrev = iNew; -} - -#endif // !DACCESS_COMPILE - -//***************************************************************************** -// Search the hash table for an entry with the specified key value. -//***************************************************************************** -BYTE *CHashTable::Find( // Index of struct in m_pcEntries. - ULONG iHash, // Hash value of the item. - SIZE_T key) // The key to match. -{ - CONTRACTL - { - NOTHROW; - GC_NOTRIGGER; - SUPPORTS_DAC; - } - CONTRACTL_END; - - ULONG iNext; // Used to traverse the chains. - HASHENTRY *psEntry; // Used to traverse the chains. - - // Start at the top of the chain. - iNext = m_piBuckets[iHash % m_iBuckets]; - - // Search until we hit the end. - -#ifdef _DEBUG - unsigned count = 0; -#endif - - while (iNext != UINT32_MAX) - { - // Compare the keys. - psEntry = EntryPtr(iNext); - -#ifdef _DEBUG - count++; -#endif - if (!Cmp(key, psEntry)) - { -#ifdef _DEBUG - if (count > m_maxSearch) - m_maxSearch = count; -#endif - - return ((BYTE *) psEntry); - } - - // Advance to the next item in the chain. - iNext = psEntry->iNext; - } - - // We couldn't find it. - return (0); -} - -//***************************************************************************** -// Search the hash table for the next entry with the specified key value. -//***************************************************************************** -ULONG CHashTable::FindNext( // Index of struct in m_pcEntries. - SIZE_T key, // The key to match. - ULONG iIndex) // Index of previous match. -{ - CONTRACTL - { - NOTHROW; - GC_NOTRIGGER; - } - CONTRACTL_END; - - ULONG iNext; // Used to traverse the chains. - HASHENTRY *psEntry; // Used to traverse the chains. - - // Start at the next entry in the chain. - iNext = EntryPtr(iIndex)->iNext; - - // Search until we hit the end. - while (iNext != UINT32_MAX) - { - // Compare the keys. - psEntry = EntryPtr(iNext); - if (!Cmp(key, psEntry)) - return (iNext); - - // Advance to the next item in the chain. - iNext = psEntry->iNext; - } - - // We couldn't find it. - return (UINT32_MAX); -} - -//***************************************************************************** -// Returns the next entry in the list. -//***************************************************************************** -BYTE *CHashTable::FindNextEntry( // The next entry, or0 for end of list. - HASHFIND *psSrch) // Search object. -{ - CONTRACTL - { - NOTHROW; - GC_NOTRIGGER; - SUPPORTS_DAC; - } - CONTRACTL_END; - - HASHENTRY *psEntry; // Used to traverse the chains. - - for (;;) - { - // See if we already have one to use and if so, use it. - if (psSrch->iNext != UINT32_MAX) - { - psEntry = EntryPtr(psSrch->iNext); -#if DACCESS_COMPILE - // If there is a simple infinite loop in the linked list - // If more complex forms of infinite loops are present, this code may need to be adjusted to handle an arbitrary cycle. - if (psEntry->iNext == psSrch->iNext) - return NULL; -#endif - psSrch->iNext = psEntry->iNext; - return ((BYTE *) psEntry); - } - - // Advance to the next bucket. - if (psSrch->iBucket < m_iBuckets) - psSrch->iNext = m_piBuckets[psSrch->iBucket++]; - else - break; - } - - // There were no more entries to be found. - return (0); -} - -#ifdef DACCESS_COMPILE - -void -CHashTable::EnumMemoryRegions(CLRDataEnumMemoryFlags flags, - ULONG numEntries) -{ - SUPPORTS_DAC; - - // This class may be embedded so do not enum 'this'. - DacEnumMemoryRegion(m_pcEntries, - (ULONG)numEntries * m_iEntrySize); - DacEnumMemoryRegion(dac_cast(m_piBuckets), - (ULONG)m_iBuckets * sizeof(ULONG)); -} - -#endif // #ifdef DACCESS_COMPILE - // // // CClosedHashBase diff --git a/src/coreclr/vm/eetoprofinterfaceimpl.cpp b/src/coreclr/vm/eetoprofinterfaceimpl.cpp index df0a0e6723ffa2..692f4d405c6669 100644 --- a/src/coreclr/vm/eetoprofinterfaceimpl.cpp +++ b/src/coreclr/vm/eetoprofinterfaceimpl.cpp @@ -377,46 +377,6 @@ static HRESULT CoCreateProfiler( } -//--------------------------------------------------------------------------------------- -// -// Implementation of CHashTableImpl functions. This class a simple implementation of -// CHashTable to provide a very simple implementation of the Cmp pure virtual function -// - -EEToProfInterfaceImpl::CHashTableImpl::CHashTableImpl(ULONG iBuckets) - : CHashTable(iBuckets) -{ - WRAPPER_NO_CONTRACT; -} - -EEToProfInterfaceImpl::CHashTableImpl::~CHashTableImpl() -{ - WRAPPER_NO_CONTRACT; -} - -//--------------------------------------------------------------------------------------- -// -// Comparison function for hash table of ClassIDs -// -// Arguments: -// pc1 - hash key to compare -// pc2 - hash value to compare -// -// Return Value: -// TRUE if the key & value refer to the same ClassID; otherwise FALSE -// - -BOOL EEToProfInterfaceImpl::CHashTableImpl::Cmp(SIZE_T k1, const HASHENTRY * pc2) -{ - LIMITED_METHOD_CONTRACT; - - ClassID key = (ClassID) k1; - ClassID val = ((CLASSHASHENTRY *)pc2)->m_clsId; - - return (key != val); -} - - //--------------------------------------------------------------------------------------- // // EEToProfInterfaceImpl ctor just sets initial values @@ -985,11 +945,6 @@ EEToProfInterfaceImpl::~EEToProfInterfaceImpl() _ASSERTE((UINT_PTR)m_pSavedAllocDataBlock != 0xFFFFFFFF); #endif - _ASSERTE(m_pSavedAllocDataBlock->pHashTable != NULL); - // Get rid of the hash table - if (m_pSavedAllocDataBlock->pHashTable) - delete m_pSavedAllocDataBlock->pHashTable; - // Get rid of the two arrays used to hold class<->numinstance info if (m_pSavedAllocDataBlock->cLength != 0) { @@ -1000,12 +955,6 @@ EEToProfInterfaceImpl::~EEToProfInterfaceImpl() delete [] m_pSavedAllocDataBlock->arrcObjects; } - // Get rid of the hash array used by the hash table - if (m_pSavedAllocDataBlock->arrHash) - { - delete [] m_pSavedAllocDataBlock->arrHash; - } - delete m_pSavedAllocDataBlock; m_pSavedAllocDataBlock = NULL; } @@ -1408,11 +1357,6 @@ HRESULT EEToProfInterfaceImpl::EndMovedReferences(void * pHeapId) } -#define HASH_ARRAY_SIZE_INITIAL 1024 -#define HASH_ARRAY_SIZE_INC 256 -#define HASH_NUM_BUCKETS 32 -#define HASH(x) ( (ULONG) ((SIZE_T)x) ) // A simple hash function - //--------------------------------------------------------------------------------------- // // Callback used by the GC when walking the heap (via AllocByClassHelper in @@ -1473,60 +1417,19 @@ HRESULT EEToProfInterfaceImpl::AllocByClass(ObjectID objId, ClassID clsId, void if (pData == NULL) return (E_OUTOFMEMORY); - // Create a new hash table - pData->pHashTable = new (nothrow) CHashTableImpl(HASH_NUM_BUCKETS); - if (!pData->pHashTable) - { - delete pData; - return (E_OUTOFMEMORY); - } - - // Get the memory for the array that the hash table is going to use - pData->arrHash = new (nothrow) CLASSHASHENTRY[HASH_ARRAY_SIZE_INITIAL]; - if (pData->arrHash == NULL) - { - delete pData->pHashTable; - delete pData; - return (E_OUTOFMEMORY); - } - - // Save the number of elements in the array - pData->cHash = HASH_ARRAY_SIZE_INITIAL; - - // Now initialize the hash table - HRESULT hr = pData->pHashTable->NewInit((BYTE *)pData->arrHash, sizeof(CLASSHASHENTRY)); - if (hr == E_OUTOFMEMORY) - { - delete [] pData->arrHash; - delete pData->pHashTable; - delete pData; - return (E_OUTOFMEMORY); - } - _ASSERTE(pData->pHashTable->IsInited()); - - // Null some entries - pData->arrClsId = NULL; - pData->arrcObjects = NULL; - pData->cLength = 0; - // Hold on to the structure m_pSavedAllocDataBlock = pData; } - // Got some memory and hash table to store entries, yay! - *((size_t *)pHeapId) = (size_t)pData; - // Initialize the data - pData->iHash = 0; - pData->pHashTable->Clear(); + pData->hashTable.RemoveAll(true); } - _ASSERTE(pData->iHash <= pData->cHash); _ASSERTE(dwProcId == GetCurrentProcessId()); // Lookup to see if this class already has an entry - CLASSHASHENTRY * pEntry = - reinterpret_cast(pData->pHashTable->Find(HASH(clsId), (SIZE_T)clsId)); + AllocByClassEntry *pEntry = + const_cast(pData->hashTable.LookupPtr(clsId)); // If this class has already been encountered, just increment the counter. if (pEntry) @@ -1535,34 +1438,13 @@ HRESULT EEToProfInterfaceImpl::AllocByClass(ObjectID objId, ClassID clsId, void // Otherwise, need to add this one as a new entry in the hash table else { - // If we're full, we need to realloc - if (pData->iHash == pData->cHash) - { - // Try to realloc the memory - CLASSHASHENTRY *tmp = new (nothrow) CLASSHASHENTRY[pData->cHash + HASH_ARRAY_SIZE_INC]; - if (!tmp) - { - return (E_OUTOFMEMORY); - } - - _ASSERTE(pData->arrHash); - memcpy (tmp, pData->arrHash, pData->cHash*sizeof(CLASSHASHENTRY)); - delete [] pData->arrHash; - pData->arrHash = tmp; - // Tell the hash table that the memory location of the array has changed - pData->pHashTable->SetTable((BYTE *)pData->arrHash); - - // Save the new size of the array - pData->cHash += HASH_ARRAY_SIZE_INC; - } - - // Now add the new entry - CLASSHASHENTRY *pNewEntry = (CLASSHASHENTRY *) pData->pHashTable->Add(HASH(clsId), pData->iHash++); - - pNewEntry->m_clsId = clsId; - pNewEntry->m_count = 1; + AllocByClassEntry entry = { clsId, 1 }; + if (!pData->hashTable.AddNoThrow(entry)) + return E_OUTOFMEMORY; } + *((size_t *)pHeapId) = (size_t)pData; + // Indicate success return (S_OK); } @@ -5370,10 +5252,11 @@ HRESULT EEToProfInterfaceImpl::NotifyAllocByClass(AllocByClassData *pData) "**PROF: ObjectsAllocatedByClass.\n")); _ASSERTE(pData != NULL); - _ASSERTE(pData->iHash > 0); + ULONG classCount = pData->hashTable.GetCount(); + _ASSERTE(classCount > 0); // If the arrays are not long enough, get rid of them. - if (pData->cLength != 0 && pData->iHash > pData->cLength) + if (pData->cLength != 0 && classCount > pData->cLength) { _ASSERTE(pData->arrClsId != NULL && pData->arrcObjects != NULL); delete [] pData->arrClsId; @@ -5384,13 +5267,13 @@ HRESULT EEToProfInterfaceImpl::NotifyAllocByClass(AllocByClassData *pData) // If there are no arrays, must allocate them. if (pData->cLength == 0) { - pData->arrClsId = new (nothrow) ClassID[pData->iHash]; + pData->arrClsId = new (nothrow) ClassID[classCount]; if (pData->arrClsId == NULL) { return E_OUTOFMEMORY; } - pData->arrcObjects = new (nothrow) ULONG[pData->iHash]; + pData->arrcObjects = new (nothrow) ULONG[classCount]; if (pData->arrcObjects == NULL) { delete [] pData->arrClsId; @@ -5400,34 +5283,31 @@ HRESULT EEToProfInterfaceImpl::NotifyAllocByClass(AllocByClassData *pData) } // Indicate that the memory was successfully allocated - pData->cLength = pData->iHash; + pData->cLength = classCount; } // Now copy all the data - HASHFIND hFind; - CLASSHASHENTRY * pCur = (CLASSHASHENTRY *) pData->pHashTable->FindFirstEntry(&hFind); size_t iCur = 0; // current index for arrays - while (pCur != NULL) + for (AllocByClassTable::Iterator entry = pData->hashTable.Begin(); entry != pData->hashTable.End(); entry++) { - _ASSERTE(iCur < pData->iHash); + _ASSERTE(iCur < classCount); - pData->arrClsId[iCur] = pCur->m_clsId; - pData->arrcObjects[iCur] = (DWORD) pCur->m_count; + pData->arrClsId[iCur] = entry->m_clsId; + pData->arrcObjects[iCur] = (DWORD) entry->m_count; // Move to the next entry iCur++; - pCur = (CLASSHASHENTRY *) pData->pHashTable->FindNextEntry(&hFind); } - _ASSERTE(iCur == pData->iHash); + _ASSERTE(iCur == classCount); // Now communicate the results to the profiler { // All callbacks are really NOTHROW, but that's enforced partially by the profiler, // whose try/catch blocks aren't visible to the contract system PERMANENT_CONTRACT_VIOLATION(ThrowsViolation, ReasonProfilerCallout); - return m_pCallback2->ObjectsAllocatedByClass((ULONG)pData->iHash, pData->arrClsId, pData->arrcObjects); + return m_pCallback2->ObjectsAllocatedByClass(classCount, pData->arrClsId, pData->arrcObjects); } } diff --git a/src/coreclr/vm/eetoprofinterfaceimpl.h b/src/coreclr/vm/eetoprofinterfaceimpl.h index 89a76cfe853d83..b97ea59b7df0c7 100644 --- a/src/coreclr/vm/eetoprofinterfaceimpl.h +++ b/src/coreclr/vm/eetoprofinterfaceimpl.h @@ -491,35 +491,40 @@ class EEToProfInterfaceImpl // // This is for a hashing of ClassID values - struct CLASSHASHENTRY : HASHENTRY + struct AllocByClassEntry { ClassID m_clsId; // The class ID (also the key) size_t m_count; // How many of this class have been counted }; - // This is a simple implementation of CHashTable to provide a very simple - // implementation of the Cmp pure virtual function - class CHashTableImpl : public CHashTable + class AllocByClassTraits : public NoRemoveSHashTraits>> { public: - CHashTableImpl(ULONG iBuckets); - virtual ~CHashTableImpl(); - - protected: - virtual BOOL Cmp(SIZE_T k1, const HASHENTRY * pc2); + typedef ClassID key_t; + static key_t GetKey(const element_t &entry) { return entry.m_clsId; } + static BOOL Equals(key_t left, key_t right) { return left == right; } + static count_t Hash(key_t key) + { + count_t hash = static_cast(key); +#ifdef HOST_64BIT + hash = HashCOUNT_T(hash, static_cast(key >> 32)); +#endif + return hash; + } + static element_t Null() { return {}; } + static bool IsNull(const element_t &entry) { return entry.m_clsId == 0; } }; + typedef SHash AllocByClassTable; + // This contains the data for storing allocation information // in terms of numbers of objects sorted by class. struct AllocByClassData { - CHashTableImpl * pHashTable; // The hash table - CLASSHASHENTRY * arrHash; // Array that the hashtable uses for linking - ULONG cHash; // The total number of elements in arrHash - ULONG iHash; // Next empty entry in the hash array - ClassID * arrClsId; // Array of ClassIDs for the call to ObjectsAllocatedByClass - ULONG * arrcObjects; // Array of counts for the call to ObjectsAllocatedByClass - size_t cLength; // Length of the above two parallel arrays + AllocByClassTable hashTable; + ClassID * arrClsId = nullptr; // Array of ClassIDs for ObjectsAllocatedByClass + ULONG * arrcObjects = nullptr; // Array of counts for ObjectsAllocatedByClass + size_t cLength = 0; // Length of the above two parallel arrays }; static const UINT kcReferencesMax = 512; diff --git a/src/tests/profiler/native/gcprofiler/gcprofiler.cpp b/src/tests/profiler/native/gcprofiler/gcprofiler.cpp index 45d810bbf21f03..55dd1cd31627f2 100644 --- a/src/tests/profiler/native/gcprofiler/gcprofiler.cpp +++ b/src/tests/profiler/native/gcprofiler/gcprofiler.cpp @@ -104,6 +104,16 @@ HRESULT GCProfiler::ObjectsAllocatedByClass(ULONG cClassCount, ClassID classIds[ printf("GCProfiler::ObjectsAllocatedByClass: FAIL: Expected ObjectsAllocatedByClass Calls == GCStart. AllocatedByClassCalls=%d, GCStart=%d\n", (int)_allocatedByClassCalls, (int)_gcStarts); } + std::unordered_set classes; + for (ULONG i = 0; i < cClassCount; i++) + { + if (classIds[i] == 0 || cObjects[i] == 0 || !classes.insert(classIds[i]).second) + { + _failures++; + printf("GCProfiler::ObjectsAllocatedByClass: FAIL: Invalid or duplicate class entry at %u\n", i); + } + } + return S_OK; } From d25ba1193e66efee9c62d16aa1a6984f6e0f8a5c Mon Sep 17 00:00:00 2001 From: Aaron Robinson Date: Wed, 21 Oct 2026 11:10:56 -0700 Subject: [PATCH 04/13] Pair debugger nothrow array allocation with deallocation Release interop-safe array storage when element construction fails. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ce4b0890-6559-4b90-b46f-11179e70f973 --- src/coreclr/debug/ee/debugger.cpp | 5 +++++ src/coreclr/debug/ee/debugger.h | 1 + 2 files changed, 6 insertions(+) diff --git a/src/coreclr/debug/ee/debugger.cpp b/src/coreclr/debug/ee/debugger.cpp index a5f40c875ed11b..abaa90ecf3358f 100644 --- a/src/coreclr/debug/ee/debugger.cpp +++ b/src/coreclr/debug/ee/debugger.cpp @@ -85,6 +85,11 @@ void DebuggerHeapHashEntry::operator delete[](void *memory) CNewZeroData::Free(static_cast(memory), 0); } +void DebuggerHeapHashEntry::operator delete[](void *memory, const std::nothrow_t &) noexcept +{ + operator delete[](memory); +} + DebuggerRCThread *g_pRCThread = NULL; // Do some compile time checking on the events in DbgIpcEventTypes.h diff --git a/src/coreclr/debug/ee/debugger.h b/src/coreclr/debug/ee/debugger.h index 09bba3f501bce4..6a96f5bb7d16c3 100644 --- a/src/coreclr/debug/ee/debugger.h +++ b/src/coreclr/debug/ee/debugger.h @@ -463,6 +463,7 @@ class DebuggerHeapHashEntry static void *operator new[](size_t size); static void *operator new[](size_t size, const std::nothrow_t &) noexcept; static void operator delete[](void *memory); + static void operator delete[](void *memory, const std::nothrow_t &) noexcept; #endif }; From dda590a62ea18d285dca7ab309bafafe4c640e4e Mon Sep 17 00:00:00 2001 From: Aaron Robinson Date: Wed, 21 Oct 2026 11:10:57 -0700 Subject: [PATCH 05/13] Assert debugger patch arena representation requirements Enforce trivial copyability and hash-entry layout for byte-managed patch storage. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ce4b0890-6559-4b90-b46f-11179e70f973 --- src/coreclr/debug/ee/controller.cpp | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/coreclr/debug/ee/controller.cpp b/src/coreclr/debug/ee/controller.cpp index 7f9fb35ace1ddf..11c840e8297b59 100644 --- a/src/coreclr/debug/ee/controller.cpp +++ b/src/coreclr/debug/ee/controller.cpp @@ -18,6 +18,8 @@ #include "../../vm/methoditer.h" #include "../../vm/tailcallhelp.h" +#include + const char *GetTType( TraceType tt); #define IsSingleStep(exception) ((exception) == EXCEPTION_SINGLE_STEP) @@ -277,6 +279,11 @@ SharedPatchBypassBuffer* DebuggerControllerPatch::GetOrCreateSharedPatchBypassBu #endif // !DACCESS_COMPILE #ifndef DACCESS_COMPILE +static_assert(std::is_trivially_copyable::value); +static_assert(offsetof(DebuggerControllerPatch, entry) == 0); +static_assert(offsetof(FREEHASHENTRY, iPrev) == offsetof(HASHENTRY, iPrev)); +static_assert(offsetof(FREEHASHENTRY, iNext) == offsetof(HASHENTRY, iNext)); + DebuggerPatchTable::~DebuggerPatchTable() { if (m_pcEntries != 0) From e0b54eed660ff9272133b6d04efd6c7e6f2642ee Mon Sep 17 00:00:00 2001 From: Aaron Robinson Date: Wed, 21 Oct 2026 12:20:39 -0700 Subject: [PATCH 06/13] Add callback-based SHash updates Remove const casts from debugger and profiler table access. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ce4b0890-6559-4b90-b46f-11179e70f973 --- src/coreclr/debug/di/divalue.cpp | 4 +- src/coreclr/debug/di/rsclass.cpp | 20 ++++---- src/coreclr/debug/di/rspriv.h | 18 +++---- src/coreclr/debug/di/rstype.cpp | 4 +- src/coreclr/debug/ee/debugger.cpp | 33 +++++++------ src/coreclr/debug/ee/debugger.h | 14 +++--- src/coreclr/debug/ee/debuggermodule.cpp | 6 +-- src/coreclr/debug/ee/functioninfo.cpp | 57 +++++++++++----------- src/coreclr/debug/inc/dacdbistructures.h | 18 +++---- src/coreclr/debug/inc/dacdbistructures.inl | 8 +-- src/coreclr/inc/shash.h | 10 +++- src/coreclr/inc/shash.inl | 26 +++++++++- src/coreclr/vm/eetoprofinterfaceimpl.cpp | 14 ++---- 13 files changed, 130 insertions(+), 102 deletions(-) diff --git a/src/coreclr/debug/di/divalue.cpp b/src/coreclr/debug/di/divalue.cpp index 46a549880899f3..cf86f8ec884813 100644 --- a/src/coreclr/debug/di/divalue.cpp +++ b/src/coreclr/debug/di/divalue.cpp @@ -2093,7 +2093,7 @@ HRESULT CordbObjectValue::GetFieldValueForType(ICorDebugType * pType, ThrowHR(E_INVALIDARG); } - FieldData * pFieldData; + const FieldData * pFieldData; #ifdef _DEBUG pFieldData = NULL; @@ -3130,7 +3130,7 @@ HRESULT CordbVCObjectValue::GetFieldValueForType(ICorDebugType * pType, pCordbType = static_cast (pType); } - FieldData * pFieldData; + const FieldData * pFieldData; #ifdef _DEBUG pFieldData = NULL; diff --git a/src/coreclr/debug/di/rsclass.cpp b/src/coreclr/debug/di/rsclass.cpp index 0efeb0f9dba560..80e103a2527069 100644 --- a/src/coreclr/debug/di/rsclass.cpp +++ b/src/coreclr/debug/di/rsclass.cpp @@ -154,7 +154,7 @@ HRESULT CordbClass::GetStaticFieldValue(mdFieldDef fieldDef, // Lookup the field given its metadata token. - FieldData *pFieldData; + const FieldData *pFieldData; hr = GetFieldInfo(fieldDef, &pFieldData); @@ -220,7 +220,7 @@ HRESULT CordbClass::GetStaticFieldValue(mdFieldDef fieldDef, // Else some other failure. /* static */ HRESULT CordbClass::GetStaticFieldValue2(CordbModule * pModule, - FieldData * pFieldData, + const FieldData * pFieldData, BOOL fEnCHangingField, const Instantiation * pInst, ICorDebugFrame * pFrame, @@ -847,7 +847,7 @@ BOOL CordbClass::GotUnallocatedStatic(DacDbiArrayList * pFieldList) * */ HRESULT FieldData::GetFieldSignature(CordbModule *pModule, - SigParser *pSigParser) + SigParser *pSigParser) const { CONTRACTL { @@ -989,14 +989,14 @@ void CordbClass::InitEnCFieldInfo(EnCHangingFieldInfo * pEncField, // fieldToken - token for the EnC field // output: pointer to an initialized instance of FieldData that has been added to the appropriate table // in our cache -FieldData * CordbClass::GetEnCFieldFromDac(BOOL fStatic, +const FieldData * CordbClass::GetEnCFieldFromDac(BOOL fStatic, CordbObjectValue * pObject, mdFieldDef fieldToken) { EnCHangingFieldInfo encField; mdTypeDef metadataToken; - FieldData fieldData, - * pInfo = NULL; + FieldData fieldData; + const FieldData * pInfo = NULL; CordbProcess * pProcess = GetModule()->GetProcess(); _ASSERTE(pProcess != NULL); @@ -1046,7 +1046,7 @@ FieldData * CordbClass::GetEnCFieldFromDac(BOOL fStatic, // S_OK on success, failure code otherwise. //----------------------------------------------------------------------------- HRESULT CordbClass::GetEnCHangingField(mdFieldDef fldToken, - FieldData **ppFieldData, + const FieldData **ppFieldData, CordbObjectValue * pObject) { FAIL_IF_NEUTERED(this); @@ -1065,7 +1065,7 @@ HRESULT CordbClass::GetEnCHangingField(mdFieldDef fldToken, BOOL fStatic = (pObject == NULL); // Look for cached field information - FieldData *pInfo = NULL; + const FieldData *pInfo = NULL; if (fStatic) { // Static fields should _NOT_ be cleared, since they stick around. Thus @@ -1114,7 +1114,7 @@ HRESULT CordbClass::GetEnCHangingField(mdFieldDef fldToken, // Returns: // S_OK on success. else failure. //----------------------------------------------------------------------------- -HRESULT CordbClass::GetFieldInfo(mdFieldDef fldToken, FieldData **ppFieldData) +HRESULT CordbClass::GetFieldInfo(mdFieldDef fldToken, const FieldData **ppFieldData) { INTERNAL_SYNC_API_ENTRY(GetProcess()); @@ -1150,7 +1150,7 @@ HRESULT CordbClass::SearchFieldInfo( DacDbiArrayList * pFieldList, mdTypeDef classToken, mdFieldDef fldToken, - FieldData **ppFieldData + const FieldData **ppFieldData ) { unsigned int i; diff --git a/src/coreclr/debug/di/rspriv.h b/src/coreclr/debug/di/rspriv.h index 372c93d78d9ff7..4cda465185d6ab 100644 --- a/src/coreclr/debug/di/rspriv.h +++ b/src/coreclr/debug/di/rspriv.h @@ -4425,7 +4425,7 @@ class CordbHangingFieldTable m_table.RemoveAll(); } - FieldData * AddFieldInfo(FieldData * pInfo) + const FieldData * AddFieldInfo(const FieldData * pInfo) { _ASSERTE(pInfo != NULL); @@ -4440,9 +4440,9 @@ class CordbHangingFieldTable #endif // #ifndef DACCESS_COMPILE - FieldData * GetFieldInfo(mdFieldDef fldToken) + const FieldData * GetFieldInfo(mdFieldDef fldToken) const { - return const_cast(m_table.LookupPtr(fldToken)); + return m_table.LookupPtr(fldToken); } }; @@ -4712,7 +4712,7 @@ class CordbType : public CordbBase, public ICorDebugType, public ICorDebugType2 // These are available after Init() has been called.... HRESULT GetUnboxedObjectSize(ULONG32 *res); - HRESULT GetFieldInfo(mdFieldDef fldToken, FieldData ** ppFieldData); + HRESULT GetFieldInfo(mdFieldDef fldToken, const FieldData ** ppFieldData); CordbAppDomain *GetAppDomain() { return m_appdomain; } @@ -4942,10 +4942,10 @@ class CordbClass : public CordbBase, public ICorDebugClass, public ICorDebugClas DacDbiArrayList * pFieldList, mdTypeDef classToken, mdFieldDef fldToken, - FieldData ** ppFieldData); + const FieldData ** ppFieldData); static HRESULT GetStaticFieldValue2(CordbModule * pModule, - FieldData * pFieldData, + const FieldData * pFieldData, BOOL fEnCHangingField, const Instantiation * pInst, ICorDebugFrame * pFrame, @@ -4957,12 +4957,12 @@ class CordbClass : public CordbBase, public ICorDebugClass, public ICorDebugClas // Get information about a field that was added by EnC HRESULT GetEnCHangingField(mdFieldDef fldToken, - FieldData ** ppFieldData, + const FieldData ** ppFieldData, CordbObjectValue * pObject); private: // Get information via the DAC about a field added with Edit and Continue. - FieldData * GetEnCFieldFromDac(BOOL fStatic, + const FieldData * GetEnCFieldFromDac(BOOL fStatic, CordbObjectValue * pObject, mdFieldDef fieldToken); @@ -4976,7 +4976,7 @@ class CordbClass : public CordbBase, public ICorDebugClass, public ICorDebugClas public: - HRESULT GetFieldInfo(mdFieldDef fldToken, FieldData ** ppFieldData); + HRESULT GetFieldInfo(mdFieldDef fldToken, const FieldData ** ppFieldData); // If you want to force the init to happen even if we think the class // is up to date, set fForceInit to TRUE diff --git a/src/coreclr/debug/di/rstype.cpp b/src/coreclr/debug/di/rstype.cpp index b5585105ea754f..1d9377eeade661 100644 --- a/src/coreclr/debug/di/rstype.cpp +++ b/src/coreclr/debug/di/rstype.cpp @@ -78,7 +78,7 @@ HRESULT CordbType::GetStaticFieldValue(mdFieldDef fieldDef, IfFailThrow(hr); // Lookup the field given its metadata token. - FieldData * pFieldData; + const FieldData * pFieldData; hr = GetFieldInfo(fieldDef, &pFieldData); @@ -2488,7 +2488,7 @@ HRESULT CordbType::GetTypeID(COR_TYPEID *pId) // S_OK on success. CORDBG_E_ENC_HANGING_FIELD for EnC fields (common case) // Other errors on failure case. //----------------------------------------------------------------------------- -HRESULT CordbType::GetFieldInfo(mdFieldDef fldToken, FieldData ** ppFieldData) +HRESULT CordbType::GetFieldInfo(mdFieldDef fldToken, const FieldData ** ppFieldData) { INTERNAL_SYNC_API_ENTRY(GetProcess()); // HRESULT hr = S_OK; diff --git a/src/coreclr/debug/ee/debugger.cpp b/src/coreclr/debug/ee/debugger.cpp index abaa90ecf3358f..47def51321c184 100644 --- a/src/coreclr/debug/ee/debugger.cpp +++ b/src/coreclr/debug/ee/debugger.cpp @@ -1100,12 +1100,13 @@ HRESULT DebuggerPendingFuncEvalTable::AddPendingEval(Thread *pThread, DebuggerEv _ASSERTE(pThread != nullptr && pDE != nullptr); _ASSERTE(pDE->m_nextPendingEval == nullptr); - DebuggerPendingFuncEval *entry = GetPendingEval(pThread); - if (entry != nullptr) + if (m_table.Update(pThread, [pDE](DebuggerPendingFuncEval &entry) { // Keep newest-first order independently of SHash rehashing. - pDE->m_nextPendingEval = entry->pDE; - entry->pDE = pDE; + pDE->m_nextPendingEval = entry.pDE; + entry.pDE = pDE; + })) + { return S_OK; } @@ -1126,20 +1127,22 @@ void DebuggerPendingFuncEvalTable::RemovePendingEval(Thread *pThread) _ASSERTE(pThread != nullptr); - DebuggerPendingFuncEval *entry = GetPendingEval(pThread); - if (entry != nullptr) + DebuggerEval *pDE = nullptr; + if (!m_table.Update(pThread, [&pDE](DebuggerPendingFuncEval &entry) { - DebuggerEval *pDE = entry->pDE; + pDE = entry.pDE; if (pDE->m_nextPendingEval != nullptr) { - entry->pDE = pDE->m_nextPendingEval; - } - else - { - m_table.RemovePtr(entry); + entry.pDE = pDE->m_nextPendingEval; } - pDE->m_nextPendingEval = nullptr; + })) + { + return; } + + if (pDE->m_nextPendingEval == nullptr) + m_table.Remove(pThread); + pDE->m_nextPendingEval = nullptr; } #endif // DACCESS_COMPILE @@ -1185,7 +1188,7 @@ ULONG DebuggerMethodInfoTable::CheckDmiTable(void) HASHFIND find; ULONG count = 0; - for (DebuggerMethodInfoEntry *entry = m_table.FindFirstEntry(&find); + for (const DebuggerMethodInfoEntry *entry = m_table.FindFirstEntry(&find); entry != nullptr; entry = m_table.FindNextEntry(&find)) { @@ -7576,7 +7579,7 @@ void Debugger::ProcessAnyPendingEvals(Thread *pThread) // FuncEvalComplete event, so if the user asks for another func eval then there will be a new pending eval when we // loop and check again. // - DebuggerPendingFuncEval *pfe; + const DebuggerPendingFuncEval *pfe; while (GetPendingEvals() != NULL && (pfe = GetPendingEvals()->GetPendingEval(pThread)) != NULL) { diff --git a/src/coreclr/debug/ee/debugger.h b/src/coreclr/debug/ee/debugger.h index 6a96f5bb7d16c3..2376051030dab4 100644 --- a/src/coreclr/debug/ee/debugger.h +++ b/src/coreclr/debug/ee/debugger.h @@ -3013,18 +3013,18 @@ template class DebuggerSHash : public SHash { public: - typename TRAITS::element_t *FindFirstEntry(HASHFIND *find) + const typename TRAITS::element_t *FindFirstEntry(HASHFIND *find) const { find->iNext = 0; return FindNextEntry(find); } - typename TRAITS::element_t *FindNextEntry(HASHFIND *find) + const typename TRAITS::element_t *FindNextEntry(HASHFIND *find) const { typename TRAITS::count_t index = find->iNext; const typename TRAITS::element_t *entry = this->GetNext(index); find->iNext = index; - return const_cast(entry); + return entry; } }; @@ -3055,11 +3055,11 @@ class DebuggerPendingFuncEvalTable void RemovePendingEval(Thread *pThread); #endif // #ifndef DACCESS_COMPILE - DebuggerPendingFuncEval *GetPendingEval(Thread* pThread) + const DebuggerPendingFuncEval *GetPendingEval(Thread* pThread) const { WRAPPER_NO_CONTRACT; - return const_cast(m_table.LookupPtr(dac_cast(pThread))); + return m_table.LookupPtr(dac_cast(pThread)); } }; @@ -3140,7 +3140,7 @@ struct DebuggerMethodInfoEntry : DebuggerHeapHashEntry PTR_DebuggerMethodInfo mi; #ifdef DACCESS_COMPILE - void EnumMemoryRegions(CLRDataEnumMemoryFlags flags); + void EnumMemoryRegions(CLRDataEnumMemoryFlags flags) const; #endif }; @@ -3218,7 +3218,7 @@ class DebuggerMethodInfoTable // {Module*,mdMethodDef} // (b) Perf: don't waste the memory! void ClearMethodsOfModule(Module *pModule); - void DeleteEntryDMI(DebuggerMethodInfoEntry *entry); + void DeleteEntryDMI(const DebuggerMethodInfoEntry *entry); #endif // #ifndef DACCESS_COMPILE diff --git a/src/coreclr/debug/ee/debuggermodule.cpp b/src/coreclr/debug/ee/debuggermodule.cpp index ee59d278f8ca0e..0895a7aca26d63 100644 --- a/src/coreclr/debug/ee/debuggermodule.cpp +++ b/src/coreclr/debug/ee/debuggermodule.cpp @@ -59,7 +59,7 @@ void DebuggerModuleTable::Clear() _ASSERTE(ThreadHoldsLock()); HASHFIND hf; - DebuggerModuleEntry *pDME; + const DebuggerModuleEntry *pDME; pDME = m_table.FindFirstEntry(&hf); @@ -164,7 +164,7 @@ DebuggerModule *DebuggerModuleTable::GetFirstModule(HASHFIND *info) _ASSERTE(ThreadHoldsLock()); - DebuggerModuleEntry *entry = m_table.FindFirstEntry(info); + const DebuggerModuleEntry *entry = m_table.FindFirstEntry(info); if (entry == NULL) return NULL; else @@ -182,7 +182,7 @@ DebuggerModule *DebuggerModuleTable::GetNextModule(HASHFIND *info) _ASSERTE(ThreadHoldsLock()); - DebuggerModuleEntry *entry = m_table.FindNextEntry(info); + const DebuggerModuleEntry *entry = m_table.FindNextEntry(info); if (entry == NULL) return NULL; else diff --git a/src/coreclr/debug/ee/functioninfo.cpp b/src/coreclr/debug/ee/functioninfo.cpp index a31f03c7d56c92..1cdf472b831e7b 100644 --- a/src/coreclr/debug/ee/functioninfo.cpp +++ b/src/coreclr/debug/ee/functioninfo.cpp @@ -2159,22 +2159,21 @@ HRESULT DebuggerMethodInfoTable::OverwriteMethodInfo(Module *pModule, dmik.pModule = pModule; dmik.token = token; - DebuggerMethodInfoEntry *entry - = const_cast(m_table.LookupPtr(dmik)); - if (entry != NULL) + HRESULT hr = E_FAIL; + m_table.Update(dmik, [&](DebuggerMethodInfoEntry &entry) { - if ( (fOnlyIfNull && entry->mi == NULL) + if ( (fOnlyIfNull && entry.mi == NULL) || !fOnlyIfNull) { - entry->mi = mi; + entry.mi = mi; LOG((LF_CORDB, LL_INFO1000, "DMIT::OMI: mod:%p tok:0x%08x remap nVer:0x%zx\n", - pModule, token, entry->nVersionLastRemapped)); - return S_OK; + pModule, token, entry.nVersionLastRemapped)); + hr = S_OK; } - } + }); - return E_FAIL; + return hr; } // pModule is being destroyed - remove any entries that belong to it. Why? @@ -2193,7 +2192,7 @@ void DebuggerMethodInfoTable::ClearMethodsOfModule(Module *pModule) HASHFIND info; - DebuggerMethodInfoEntry *entry + const DebuggerMethodInfoEntry *entry = m_table.FindFirstEntry(&info); while(entry != NULL) { @@ -2236,7 +2235,7 @@ void DebuggerMethodInfoTable::ClearMethodsOfModule(Module *pModule) } } -void DebuggerMethodInfoTable::DeleteEntryDMI(DebuggerMethodInfoEntry *entry) +void DebuggerMethodInfoTable::DeleteEntryDMI(const DebuggerMethodInfoEntry *entry) { CONTRACTL { @@ -2248,12 +2247,16 @@ void DebuggerMethodInfoTable::DeleteEntryDMI(DebuggerMethodInfoEntry *entry) } CONTRACTL_END; - DebuggerMethodInfo *dmiPrev = entry->mi->m_prevMethodInfo; - TRACE_FREE(entry->mi); - DeleteInteropSafe(entry->mi); - entry->mi = dmiPrev; - if ( dmiPrev != NULL ) - dmiPrev->m_nextMethodInfo = NULL; + bool updated = m_table.Update(entry->key, [](DebuggerMethodInfoEntry &element) + { + DebuggerMethodInfo *dmiPrev = element.mi->m_prevMethodInfo; + TRACE_FREE(element.mi); + DeleteInteropSafe(element.mi); + element.mi = dmiPrev; + if ( dmiPrev != NULL ) + dmiPrev->m_nextMethodInfo = NULL; + }); + _ASSERTE(updated); } #endif // #ifndef DACCESS_COMPILE @@ -2322,13 +2325,11 @@ DebuggerMethodInfo *DebuggerMethodInfoTable::GetFirstMethodInfo(HASHFIND *info) _ASSERTE(g_pDebugger->HasDebuggerDataLock()); - DebuggerMethodInfoEntry *entry = m_table.FindFirstEntry(info); + const DebuggerMethodInfoEntry *entry = m_table.FindFirstEntry(info); if (entry == NULL) return NULL; - else - { - return entry->mi; - } + + return entry->mi; } DebuggerMethodInfo *DebuggerMethodInfoTable::GetNextMethodInfo(HASHFIND *info) @@ -2344,7 +2345,7 @@ DebuggerMethodInfo *DebuggerMethodInfoTable::GetNextMethodInfo(HASHFIND *info) _ASSERTE(g_pDebugger->HasDebuggerDataLock()); - DebuggerMethodInfoEntry *entry = m_table.FindNextEntry(info); + const DebuggerMethodInfoEntry *entry = m_table.FindNextEntry(info); // We may have incremented the version number // for methods that never got JITted, so we should @@ -2357,17 +2358,15 @@ DebuggerMethodInfo *DebuggerMethodInfoTable::GetNextMethodInfo(HASHFIND *info) if (entry == NULL) return NULL; - else - { - return entry->mi; - } + + return entry->mi; } #ifdef DACCESS_COMPILE void -DebuggerMethodInfoEntry::EnumMemoryRegions(CLRDataEnumMemoryFlags flags) +DebuggerMethodInfoEntry::EnumMemoryRegions(CLRDataEnumMemoryFlags flags) const { SUPPORTS_DAC; @@ -2424,7 +2423,7 @@ void DebuggerMethodInfoTable::EnumMemoryRegions(CLRDataEnumMemoryFlags flags) m_table.EnumMemoryRegions(); HASHFIND find; - for (DebuggerMethodInfoEntry *entry = m_table.FindFirstEntry(&find); + for (const DebuggerMethodInfoEntry *entry = m_table.FindFirstEntry(&find); entry != nullptr; entry = m_table.FindNextEntry(&find)) { diff --git a/src/coreclr/debug/inc/dacdbistructures.h b/src/coreclr/debug/inc/dacdbistructures.h index 621fa84503e113..8b860e3256dc94 100644 --- a/src/coreclr/debug/inc/dacdbistructures.h +++ b/src/coreclr/debug/inc/dacdbistructures.h @@ -653,17 +653,17 @@ class MSLAYOUT FieldData // initialize various fields of an instance of FieldData from information in a FieldDesc void Initialize(BOOL fIsStatic, BOOL fIsPrimitive, mdFieldDef mdToken); #else - HRESULT GetFieldSignature(class CordbModule * pModule, /*OUT*/ SigParser * pSigParser); + HRESULT GetFieldSignature(class CordbModule * pModule, /*OUT*/ SigParser * pSigParser) const; #endif // clear various fields for a new instance of FieldData void ClearFields(); // Make sure it's okay to get or set an instance field offset. - BOOL OkToGetOrSetInstanceOffset(); + BOOL OkToGetOrSetInstanceOffset() const; // Make sure it's okay to get or set a static field address. - BOOL OkToGetOrSetStaticAddress(); + BOOL OkToGetOrSetStaticAddress() const; // If this is an instance field, store its offset void SetInstanceOffset( ULONG64 offset ); @@ -674,11 +674,11 @@ class MSLAYOUT FieldData // If this is an instance field, return its offset // Note that this offset is always a real offset (possibly larger than 22 bits), which isn't // necessarily the same as the overloaded FieldDesc.dwOffset field. - CORDB_ADDRESS GetInstanceOffset(); + CORDB_ADDRESS GetInstanceOffset() const; // If this is a "normal" static, get its absolute address // TLS and context-specific statics are "special". - CORDB_ADDRESS GetStaticAddress(); + CORDB_ADDRESS GetStaticAddress() const; // // Data members @@ -704,10 +704,10 @@ class MSLAYOUT FieldData // the object. CORDB_ADDRESS m_pFldStaticAddress; // The absolute target address of a static field - PCCOR_SIGNATURE m_fldSignatureCache; // This is passed across as null. It is a RS-only cache, and SHOULD - // NEVER BE ACCESSED DIRECTLY! - ULONG m_fldSignatureCacheSize; // This is passed across as 0. It is a RS-only cache, and SHOULD - // NEVER BE ACCESSED DIRECTLY! + mutable PCCOR_SIGNATURE m_fldSignatureCache; // This is passed across as null. It is a RS-only cache, and SHOULD + // NEVER BE ACCESSED DIRECTLY! + mutable ULONG m_fldSignatureCacheSize; // This is passed across as 0. It is a RS-only cache, and SHOULD + // NEVER BE ACCESSED DIRECTLY! public: VMPTR_FieldDesc m_vmFieldDesc; diff --git a/src/coreclr/debug/inc/dacdbistructures.inl b/src/coreclr/debug/inc/dacdbistructures.inl index 5ee266f329d166..d075c88eb2d7fb 100644 --- a/src/coreclr/debug/inc/dacdbistructures.inl +++ b/src/coreclr/debug/inc/dacdbistructures.inl @@ -643,7 +643,7 @@ void FieldData::ClearFields() } inline -BOOL FieldData::OkToGetOrSetInstanceOffset() +BOOL FieldData::OkToGetOrSetInstanceOffset() const { return (!m_fFldIsStatic && !m_fFldIsRVA && !m_fFldIsTLS && m_fFldStorageAvailable && (m_pFldStaticAddress == (CORDB_ADDRESS)NULL)); @@ -662,7 +662,7 @@ void FieldData::SetInstanceOffset(ULONG64 offset) } inline -BOOL FieldData::OkToGetOrSetStaticAddress() +BOOL FieldData::OkToGetOrSetStaticAddress() const { return (m_fFldIsStatic && !m_fFldIsTLS && m_fFldStorageAvailable && (m_fldInstanceOffset == 0)); @@ -681,7 +681,7 @@ void FieldData::SetStaticAddress(CORDB_ADDRESS addr) // Get the offset of a field inline -CORDB_ADDRESS FieldData::GetInstanceOffset() +CORDB_ADDRESS FieldData::GetInstanceOffset() const { _ASSERTE(!m_fFldIsStatic); _ASSERTE(!m_fFldIsRVA); @@ -693,7 +693,7 @@ CORDB_ADDRESS FieldData::GetInstanceOffset() // Get the static address for a field inline -CORDB_ADDRESS FieldData::GetStaticAddress() +CORDB_ADDRESS FieldData::GetStaticAddress() const { _ASSERTE(m_fFldIsStatic); _ASSERTE(!m_fFldIsTLS); diff --git a/src/coreclr/inc/shash.h b/src/coreclr/inc/shash.h index 05db8047f39515..0ddc217cbd9a3e 100644 --- a/src/coreclr/inc/shash.h +++ b/src/coreclr/inc/shash.h @@ -22,6 +22,7 @@ // (@todo: this can be remedied by delaying destruction of old tables during reallocation, e.g. during GC) // - Remove operations may be asynchronous with Lookup/Add, unless elements are also deallocated. (In which // case full synchronization is required) +// - Update operations must be synchronized with other accesses to the updated element. // Common "gotchas": // - The Add method never replaces an element. The new element will be added even if an element with the same @@ -198,6 +199,13 @@ class EMPTY_BASES SHash : public TRAITS const element_t* LookupPtr(key_t key) const; + // Update the first matching element in place, returning false if no match exists. + // The callback must preserve the key and sentinel state, and must not structurally modify the table. + // It must not retain a mutable pointer or reference to the element. + // Callback exceptions propagate; changes already made are not rolled back. + template + bool Update(key_t key, Functor &&functor); + // Pointer-based flavor to replace an existing element (allows efficient access to tables of structures) void ReplacePtr(const element_t *elementPtr, const element_t &newElement, bool invokeCleanupAction = true); @@ -341,7 +349,7 @@ class EMPTY_BASES SHash : public TRAITS // Utility function to find the first element with the given key in // the hash table. - const element_t* Lookup(PTR_element_t table, count_t tableSize, key_t key) const; + element_t* Lookup(PTR_element_t table, count_t tableSize, key_t key) const; // Utility function to remove the first element with the given key // in the hash table. diff --git a/src/coreclr/inc/shash.inl b/src/coreclr/inc/shash.inl index b0fc4ea20d0446..fa1359f3eff471 100644 --- a/src/coreclr/inc/shash.inl +++ b/src/coreclr/inc/shash.inl @@ -123,6 +123,30 @@ const typename SHash::element_t * SHash::LookupPtr(key_t key) co return Lookup(m_table, m_tableSize, key); } +template +template +bool SHash::Update(key_t key, Functor &&functor) +{ + WRAPPER_NO_CONTRACT; + + element_t *element = Lookup(m_table, m_tableSize, key); + if (element == nullptr) + return false; + +#ifdef _DEBUG + PTR_element_t table = m_table; + count_t tableSize = m_tableSize; + count_t count = m_tableCount; +#endif // _DEBUG + + functor(*element); + + _ASSERTE(m_table == table && m_tableSize == tableSize && m_tableCount == count); + _ASSERTE(!TRAITS::IsNull(*element) && !TRAITS::IsDeleted(*element)); + _ASSERTE(TRAITS::Equals(TRAITS::GetKey(*element), key)); + return true; +} + template void SHash::ReplacePtr(const element_t *elementPtr, const element_t &newElement, bool invokeCleanupAction) { @@ -667,7 +691,7 @@ SHash::DeleteOldTable(element_t * oldTable) } template -const typename SHash::element_t * SHash::Lookup(PTR_element_t table, count_t tableSize, key_t key) const +typename SHash::element_t * SHash::Lookup(PTR_element_t table, count_t tableSize, key_t key) const { CONTRACTL { diff --git a/src/coreclr/vm/eetoprofinterfaceimpl.cpp b/src/coreclr/vm/eetoprofinterfaceimpl.cpp index 692f4d405c6669..c48fd7198c8caa 100644 --- a/src/coreclr/vm/eetoprofinterfaceimpl.cpp +++ b/src/coreclr/vm/eetoprofinterfaceimpl.cpp @@ -1427,16 +1427,10 @@ HRESULT EEToProfInterfaceImpl::AllocByClass(ObjectID objId, ClassID clsId, void _ASSERTE(dwProcId == GetCurrentProcessId()); - // Lookup to see if this class already has an entry - AllocByClassEntry *pEntry = - const_cast(pData->hashTable.LookupPtr(clsId)); - - // If this class has already been encountered, just increment the counter. - if (pEntry) - pEntry->m_count++; - - // Otherwise, need to add this one as a new entry in the hash table - else + if (!pData->hashTable.Update(clsId, [](AllocByClassEntry &entry) + { + entry.m_count++; + })) { AllocByClassEntry entry = { clsId, 1 }; if (!pData->hashTable.AddNoThrow(entry)) From ae96ea5ac9031432325fdc48fb690f749a91d3e0 Mon Sep 17 00:00:00 2001 From: Aaron Robinson Date: Wed, 21 Oct 2026 13:22:45 -0700 Subject: [PATCH 07/13] Restore method-info table default constructor on Unix Unix VPTR macros declare an int constructor and suppress the implicit default constructor. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ce4b0890-6559-4b90-b46f-11179e70f973 --- src/coreclr/debug/ee/debugger.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/coreclr/debug/ee/debugger.h b/src/coreclr/debug/ee/debugger.h index 2376051030dab4..dfc0943ab06c2b 100644 --- a/src/coreclr/debug/ee/debugger.h +++ b/src/coreclr/debug/ee/debugger.h @@ -3203,6 +3203,8 @@ class DebuggerMethodInfoTable #ifndef DACCESS_COMPILE + DebuggerMethodInfoTable() = default; + HRESULT AddMethodInfo(Module *pModule, mdMethodDef token, DebuggerMethodInfo *mi); From 6954f59697249398276ccfe8239447e5a9d9f0f1 Mon Sep 17 00:00:00 2001 From: Aaron Robinson Date: Wed, 21 Oct 2026 13:22:46 -0700 Subject: [PATCH 08/13] Remove unused Cordb hash-table constructor sizes Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ce4b0890-6559-4b90-b46f-11179e70f973 --- src/coreclr/debug/di/module.cpp | 6 +++--- src/coreclr/debug/di/process.cpp | 6 +++--- src/coreclr/debug/di/rsappdomain.cpp | 8 ++++---- src/coreclr/debug/di/rsfunction.cpp | 2 +- src/coreclr/debug/di/rsmain.cpp | 2 +- src/coreclr/debug/di/rspriv.h | 6 ++---- src/coreclr/debug/di/rstype.cpp | 6 +++--- 7 files changed, 17 insertions(+), 19 deletions(-) diff --git a/src/coreclr/debug/di/module.cpp b/src/coreclr/debug/di/module.cpp index 4c5c08c5f69835..47c8cbf94b2c0c 100644 --- a/src/coreclr/debug/di/module.cpp +++ b/src/coreclr/debug/di/module.cpp @@ -30,13 +30,13 @@ CordbModule::CordbModule( : CordbBase(pProcess, VmPtrToCookie(vmModule), enumCordbModule), m_pAssembly(0), m_pAppDomain(0), - m_classes(11), - m_functions(101), + m_classes(), + m_functions(), m_vmAssembly(vmAssembly), m_vmModule(vmModule), m_EnCCount(0), m_fForceMetaDataSerialize(FALSE), - m_nativeCodeTable(101) + m_nativeCodeTable() { _ASSERTE(pProcess->GetProcessLock()->HasLock()); diff --git a/src/coreclr/debug/di/process.cpp b/src/coreclr/debug/di/process.cpp index f5359a8873f6d0..8726b9fec3daf8 100644 --- a/src/coreclr/debug/di/process.cpp +++ b/src/coreclr/debug/di/process.cpp @@ -840,13 +840,13 @@ CordbProcess::CordbProcess(ULONG64 clrInstanceId, m_synchronized(false), m_syncCompleteReceived(false), m_pShim(pShim), - m_userThreads(11), + m_userThreads(), m_oddSync(false), #ifdef FEATURE_INTEROP_DEBUGGING - m_unmanagedThreads(11), + m_unmanagedThreads(), #endif m_pAppDomain(NULL), - m_steppers(11), + m_steppers(), m_continueCounter(1), m_flushCounter(0), m_leftSideEventAvailable(NULL), diff --git a/src/coreclr/debug/di/rsappdomain.cpp b/src/coreclr/debug/di/rsappdomain.cpp index 2b07c1649afd93..69514035ea3748 100644 --- a/src/coreclr/debug/di/rsappdomain.cpp +++ b/src/coreclr/debug/di/rsappdomain.cpp @@ -42,10 +42,10 @@ CordbAppDomain::CordbAppDomain(CordbProcess * pProcess, VMPTR_AppDomain vmAppDomain) : CordbBase(pProcess, LsPtrToCookie(vmAppDomain.ToLsPtr()), enumCordbAppDomain), m_AppDomainId(0), - m_breakpoints(17), - m_sharedtypes(3), - m_modules(17), - m_assemblies(9), + m_breakpoints(), + m_sharedtypes(), + m_modules(), + m_assemblies(), m_vmAppDomain(vmAppDomain) { // This may throw out of the Ctor on error. diff --git a/src/coreclr/debug/di/rsfunction.cpp b/src/coreclr/debug/di/rsfunction.cpp index 92c73b9aba6847..f18450402bd41e 100644 --- a/src/coreclr/debug/di/rsfunction.cpp +++ b/src/coreclr/debug/di/rsfunction.cpp @@ -44,7 +44,7 @@ CordbFunction::CordbFunction(CordbModule * m, m_argCountCached(0), m_fIsStaticCached(FALSE) #ifdef FEATURE_CODE_VERSIONING - , m_reJitILCodes(1) + , m_reJitILCodes() #endif // FEATURE_CODE_VERSIONING { m_methodSigParserCached = SigParser(NULL, 0); diff --git a/src/coreclr/debug/di/rsmain.cpp b/src/coreclr/debug/di/rsmain.cpp index b0199c36356183..9a31c42b41ec72 100644 --- a/src/coreclr/debug/di/rsmain.cpp +++ b/src/coreclr/debug/di/rsmain.cpp @@ -927,7 +927,7 @@ Cordb::Cordb(CorDebugInterfaceVersion iDebuggerVersion) Cordb::Cordb(CorDebugInterfaceVersion iDebuggerVersion, const ProcessDescriptor& pd, LPCWSTR dacModulePath) : CordbBase(NULL, 0, enumCordb), - m_processes(11), + m_processes(), m_initialized(false), m_debuggerSpecifiedVersion(iDebuggerVersion), m_pd(pd), diff --git a/src/coreclr/debug/di/rspriv.h b/src/coreclr/debug/di/rspriv.h index 4cda465185d6ab..dfe222ae283d8b 100644 --- a/src/coreclr/debug/di/rspriv.h +++ b/src/coreclr/debug/di/rspriv.h @@ -1734,7 +1734,7 @@ class CordbHashTable bool IsInitialized(); #ifndef DACCESS_COMPILE - CordbHashTable(ULONG) + CordbHashTable() { #ifdef _DEBUG m_pDbgLock = NULL; @@ -1795,9 +1795,7 @@ class CordbSafeHashTable : public CordbHashTable { public: #ifndef DACCESS_COMPILE - CordbSafeHashTable(ULONG size) : CordbHashTable(size) - { - } + CordbSafeHashTable() = default; #endif // Typesafe wrappers HRESULT AddBase(T * pBase) { return UnsafeAddBase(pBase); } diff --git a/src/coreclr/debug/di/rstype.cpp b/src/coreclr/debug/di/rstype.cpp index 1d9377eeade661..3320304a42ae9a 100644 --- a/src/coreclr/debug/di/rstype.cpp +++ b/src/coreclr/debug/di/rstype.cpp @@ -123,7 +123,7 @@ CordbType::CordbType(CordbAppDomain *appdomain, CorElementType et, unsigned int m_appdomain(appdomain), m_pClass(NULL), m_rank(rank), - m_spinetypes(2), + m_spinetypes(), m_objectSize(0), m_fieldInfoNeedsInit(TRUE) { @@ -152,7 +152,7 @@ CordbType::CordbType(CordbAppDomain *appdomain, CorElementType et, CordbClass *c m_appdomain(appdomain), m_pClass(cls), m_rank(0), - m_spinetypes(2), + m_spinetypes(), m_objectSize(0), m_fieldInfoNeedsInit(TRUE) { @@ -180,7 +180,7 @@ CordbType::CordbType(CordbType *tycon, CordbType *tyarg) m_appdomain(tycon->m_appdomain), m_pClass(tycon->m_pClass), m_rank(tycon->m_rank), - m_spinetypes(2), + m_spinetypes(), m_objectSize(0), m_fieldInfoNeedsInit(TRUE) // tyarg is added as part of instantiation -see below... From 3acceefb469698335e948202ae67251bc31c1e43 Mon Sep 17 00:00:00 2001 From: Aaron Robinson Date: Wed, 21 Oct 2026 15:25:39 -0700 Subject: [PATCH 09/13] Use native SHash iteration and destructor cleanup Remove indexed traversal and debugger adapters. Release Cordb entries through the traits cleanup hook. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ce4b0890-6559-4b90-b46f-11179e70f973 --- src/coreclr/debug/di/hash.cpp | 36 ++--------- src/coreclr/debug/di/process.cpp | 80 ++++++++++--------------- src/coreclr/debug/di/rsappdomain.cpp | 24 +++----- src/coreclr/debug/di/rsmain.cpp | 11 +--- src/coreclr/debug/di/rspriv.h | 18 +++--- src/coreclr/debug/di/rspriv.inl | 33 +++------- src/coreclr/debug/ee/debugger.cpp | 44 ++++++-------- src/coreclr/debug/ee/debugger.h | 34 +++-------- src/coreclr/debug/ee/debuggermodule.cpp | 29 +++------ src/coreclr/debug/ee/functioninfo.cpp | 46 +++----------- src/coreclr/inc/shash.h | 4 -- src/coreclr/inc/shash.inl | 15 ----- 12 files changed, 105 insertions(+), 269 deletions(-) diff --git a/src/coreclr/debug/di/hash.cpp b/src/coreclr/debug/di/hash.cpp index 6634e3efcc63de..5144bcbede5196 100644 --- a/src/coreclr/debug/di/hash.cpp +++ b/src/coreclr/debug/di/hash.cpp @@ -12,13 +12,6 @@ * Hash Table class * ------------------------------------------------------------------------- */ -CordbHashTable::~CordbHashTable() -{ - for (SHash::Iterator iterator = m_table.Begin(), end = m_table.End(); - iterator != end; iterator++) - (*iterator)->InternalRelease(); -} - HRESULT CordbHashTable::UnsafeAddBase(CordbBase *pBase) { AssertIsProtected(); @@ -83,30 +76,16 @@ CordbBase *CordbHashTable::UnsafeRemoveBase(ULONG_PTR id) return base; } -CordbBase *CordbHashTable::UnsafeFindFirst(HASHFIND *find) +CordbHashTable::Iterator CordbHashTable::Begin() { AssertIsProtected(); - return UnsafeUnlockedFindFirst(find); + return m_table.Begin(); } -CordbBase *CordbHashTable::UnsafeUnlockedFindFirst(HASHFIND *find) -{ - find->iNext = 0; - return UnsafeUnlockedFindNext(find); -} - -CordbBase *CordbHashTable::UnsafeFindNext(HASHFIND *find) +CordbHashTable::Iterator CordbHashTable::End() { AssertIsProtected(); - return UnsafeUnlockedFindNext(find); -} - -CordbBase *CordbHashTable::UnsafeUnlockedFindNext(HASHFIND *find) -{ - COUNT_T index = find->iNext; - CordbBase *const *entry = m_table.GetNext(index); - find->iNext = index; - return entry == nullptr ? nullptr : *entry; + return m_table.End(); } /* ------------------------------------------------------------------------- * @@ -156,13 +135,10 @@ void CordbHashTableEnum::BuildOrThrow( pHolder->Assign(pEnum); pEnum->m_items.AllocOrThrow(pTable->GetCount()); - HASHFIND find; ULONG index = 0; - for (CordbBase *item = pTable->UnsafeFindFirst(&find); - item != nullptr; - item = pTable->UnsafeFindNext(&find)) + for (CordbHashTable::Iterator iterator = pTable->Begin(), end = pTable->End(); iterator != end; iterator++) { - pEnum->m_items.Assign(index++, item); + pEnum->m_items.Assign(index++, *iterator); } _ASSERTE(index == pEnum->m_items.Length()); diff --git a/src/coreclr/debug/di/process.cpp b/src/coreclr/debug/di/process.cpp index 8726b9fec3daf8..cf04c3e1b7e818 100644 --- a/src/coreclr/debug/di/process.cpp +++ b/src/coreclr/debug/di/process.cpp @@ -353,15 +353,14 @@ IMDInternalImport * CordbProcess::LookupMetaData(VMPTR_PEAssembly vmPEAssembly) { INTERNAL_DAC_CALLBACK(this); - HASHFIND hashFindModule; IMDInternalImport * pMDII = NULL; if (m_pAppDomain != NULL) { - for (CordbModule * pModule = m_pAppDomain->m_modules.FindFirst(&hashFindModule); - pModule != NULL; - pModule = m_pAppDomain->m_modules.FindNext(&hashFindModule)) + for (CordbHashTable::Iterator iterator = m_pAppDomain->m_modules.Begin(), end = m_pAppDomain->m_modules.End(); + iterator != end; iterator++) { + CordbModule *pModule = static_cast(*iterator); if (pModule->GetPEFile() == vmPEAssembly) { pMDII = NULL; @@ -385,10 +384,10 @@ IMDInternalImport * CordbProcess::LookupMetaData(VMPTR_PEAssembly vmPEAssembly) { m_pAppDomain->PrepopulateModules(); - for (CordbModule * pModule = m_pAppDomain->m_modules.FindFirst(&hashFindModule); - pModule != NULL; - pModule = m_pAppDomain->m_modules.FindNext(&hashFindModule)) + for (CordbHashTable::Iterator iterator = m_pAppDomain->m_modules.Begin(), end = m_pAppDomain->m_modules.End(); + iterator != end; iterator++) { + CordbModule *pModule = static_cast(*iterator); if (pModule->GetPEFile() == vmPEAssembly) { pMDII = NULL; @@ -1939,11 +1938,9 @@ CordbThread * CordbProcess::TryLookupOrCreateThreadByVolatileOSId(DWORD dwThread // CordbThread * CordbProcess::TryLookupThreadByVolatileOSId(DWORD dwThreadId) { - HASHFIND find; - for (CordbThread * pThread = m_userThreads.FindFirst(&find); - pThread != NULL; - pThread = m_userThreads.FindNext(&find)) + for (CordbHashTable::Iterator iterator = m_userThreads.Begin(), end = m_userThreads.End(); iterator != end; iterator++) { + CordbThread *pThread = static_cast(*iterator); _ASSERTE(pThread != NULL); // Get the OS tid. This returns 0 if the thread is switched out. @@ -3548,14 +3545,10 @@ void CordbProcess::MarkAllThreadsDirty() INTERNAL_API_ENTRY(this); _ASSERTE(ThreadHoldsProcessLock()); - CordbThread * pThread; - HASHFIND find; - // We don't need to prepopulate here (to collect LS state) because we're just updating RS state. - for (pThread = m_userThreads.FindFirst(&find); - pThread != NULL; - pThread = m_userThreads.FindNext(&find)) + for (CordbHashTable::Iterator iterator = m_userThreads.Begin(), end = m_userThreads.End(); iterator != end; iterator++) { + CordbThread *pThread = static_cast(*iterator); _ASSERTE(pThread != NULL); pThread->MarkStackFramesDirty(); } @@ -5695,14 +5688,11 @@ HRESULT CordbProcess::SetAllThreadsDebugState(CorDebugThreadState state, if (SUCCEEDED(hr)) { RSLockHolder lockHolder(GetProcessLock()); - HASHFIND hashFind; - CordbThread * pThread; - // We don't need to prepopulate here (to collect LS state) because we're just updating RS state. - for (pThread = m_userThreads.FindFirst(&hashFind); - pThread != NULL; - pThread = m_userThreads.FindNext(&hashFind)) + for (CordbHashTable::Iterator iterator = m_userThreads.Begin(), end = m_userThreads.End(); + iterator != end; iterator++) { + CordbThread *pThread = static_cast(*iterator); if (pThread != pCordbExceptThread) { pThread->m_debugState = state; @@ -7479,11 +7469,10 @@ HRESULT CordbProcess::SuspendUnmanagedThreads() _ASSERTE(ThreadHoldsProcessLock()); // Iterate over all unmanaged threads... - CordbUnmanagedThread* ut; - HASHFIND find; - - for (ut = m_unmanagedThreads.FindFirst(&find); ut != NULL; ut = m_unmanagedThreads.FindNext(&find)) + for (CordbHashTable::Iterator iterator = m_unmanagedThreads.Begin(), end = m_unmanagedThreads.End(); + iterator != end; iterator++) { + CordbUnmanagedThread *ut = static_cast(*iterator); // Don't suspend any thread in a can't stop region. This includes cooperative mode threads & preemptive // threads that haven't pushed a NativeTransitionFrame. The ultimate problem here is that a thread @@ -7527,11 +7516,10 @@ HRESULT CordbProcess::ResumeUnmanagedThreads() FAIL_IF_NEUTERED(this); // Iterate over all unmanaged threads... - CordbUnmanagedThread* ut; - HASHFIND find; - - for (ut = m_unmanagedThreads.FindFirst(&find); ut != NULL; ut = m_unmanagedThreads.FindNext(&find)) + for (CordbHashTable::Iterator iterator = m_unmanagedThreads.Begin(), end = m_unmanagedThreads.End(); + iterator != end; iterator++) { + CordbUnmanagedThread *ut = static_cast(*iterator); // Only resume those unmanaged threads that were suspended by us. if (ut->IsSuspended()) { @@ -8247,16 +8235,14 @@ HRESULT CordbProcess::GetThreadForTaskID(TASKID taskId, ICorDebugThread2 ** ppTh PrepopulateThreadsOrThrow(); // now find the ICorDebugThread corresponding to it - CordbThread * pThread; - HASHFIND hashFind; - - - for (pThread = m_userThreads.FindFirst(&hashFind); - pThread != NULL; - pThread = m_userThreads.FindNext(&hashFind)) + CordbThread *pThread = nullptr; + for (CordbHashTable::Iterator iterator = m_userThreads.Begin(), end = m_userThreads.End(); + iterator != end; iterator++) { - if (pThread->GetTaskID() == taskId) + CordbThread *pCandidate = static_cast(*iterator); + if (pCandidate->GetTaskID() == taskId) { + pThread = pCandidate; break; } } @@ -9642,11 +9628,10 @@ void CordbRCEventThread::ThreadProc() waitCount = 1; CordbSafeHashTable * pHashTable = m_cordb->GetProcessList(); - HASHFIND hashFind; - CordbProcess * pProcess; - - for (pProcess = pHashTable->FindFirst(&hashFind); pProcess != NULL; pProcess = pHashTable->FindNext(&hashFind)) + for (CordbHashTable::Iterator iterator = pHashTable->Begin(), end = pHashTable->End(); + iterator != end; iterator++) { + CordbProcess *pProcess = static_cast(*iterator); _ASSERTE(waitCount < MAXIMUM_WAIT_OBJECTS); if( waitCount >= MAXIMUM_WAIT_OBJECTS ) @@ -9682,7 +9667,7 @@ void CordbRCEventThread::ThreadProc() while(i < waitCount) { - pProcess = rgProcessSet[i]; + CordbProcess *pProcess = rgProcessSet[i]; // Take the process lock so we can check the queue safely pProcess->Lock(); @@ -14384,15 +14369,14 @@ HRESULT CordbProcess::IsReadyForDetach() } // Check for any outstanding EnC modules. - HASHFIND foundModule; - CordbModule * pModule = m_pAppDomain->m_modules.FindFirst(&foundModule); - while (pModule != NULL) + for (CordbHashTable::Iterator iterator = m_pAppDomain->m_modules.Begin(), end = m_pAppDomain->m_modules.End(); + iterator != end; iterator++) { + CordbModule *pModule = static_cast(*iterator); if (pModule->m_EnCCount > 0) { return CORDBG_E_DETACH_FAILED_ON_ENC; } - pModule = m_pAppDomain->m_modules.FindNext(&foundModule); } } diff --git a/src/coreclr/debug/di/rsappdomain.cpp b/src/coreclr/debug/di/rsappdomain.cpp index 69514035ea3748..ba15debd4295a9 100644 --- a/src/coreclr/debug/di/rsappdomain.cpp +++ b/src/coreclr/debug/di/rsappdomain.cpp @@ -108,13 +108,10 @@ void CordbAppDomain::Neuter() // Disconnect any active breakpoints // { - CordbBreakpoint* entry; - HASHFIND find; - - for (entry = m_breakpoints.FindFirst(&find); - entry != NULL; - entry = m_breakpoints.FindNext(&find)) + for (CordbHashTable::Iterator iterator = m_breakpoints.Begin(), end = m_breakpoints.End(); + iterator != end; iterator++) { + CordbBreakpoint *entry = static_cast(*iterator); entry->Disconnect(); } } @@ -534,15 +531,11 @@ CordbModule * CordbAppDomain::GetModuleFromMetaDataInterface(IUnknown *pIMetaDat hr = pImport->GetScopeProps(NULL, 0, 0, &matchMVID); IfFailThrow(hr); - CordbModule* pModule; - HASHFIND findmodule; - PrepopulateModules(); - for (pModule = m_modules.FindFirst(&findmodule); - pModule != NULL; - pModule = m_modules.FindNext(&findmodule)) + for (CordbHashTable::Iterator iterator = m_modules.Begin(), end = m_modules.End(); iterator != end; iterator++) { + CordbModule *pModule = static_cast(*iterator); IMetaDataImport * pImportCurrent = pModule->GetMetaDataImporter(); // throws _ASSERTE(pImportCurrent != NULL); @@ -848,12 +841,9 @@ void CordbAppDomain::PrepopulateModules() PrepopulateAssembliesOrThrow(); - HASHFIND hashfind; - - for (CordbAssembly * pAssembly = m_assemblies.FindFirst(&hashfind); - pAssembly != NULL; - pAssembly = m_assemblies.FindNext(&hashfind)) + for (CordbHashTable::Iterator iterator = m_assemblies.Begin(), end = m_assemblies.End(); iterator != end; iterator++) { + CordbAssembly *pAssembly = static_cast(*iterator); VMPTR_Assembly vmAssembly = pAssembly->GetAssemblyPtr(); VMPTR_Module vmModule = VMPTR_Module::NullPtr(); BOOL isModuleLoaded = FALSE; diff --git a/src/coreclr/debug/di/rsmain.cpp b/src/coreclr/debug/di/rsmain.cpp index 9a31c42b41ec72..a629d477307cde 100644 --- a/src/coreclr/debug/di/rsmain.cpp +++ b/src/coreclr/debug/di/rsmain.cpp @@ -1056,13 +1056,9 @@ HRESULT Cordb::Terminate() { RSLockHolder ch(&m_processListMutex); - HASHFIND hfDT; - CordbProcess * pProcess; - - for (pProcess= (CordbProcess*) m_processes.FindFirst(&hfDT); - pProcess != NULL; - pProcess = (CordbProcess*) m_processes.FindNext(&hfDT)) + for (CordbHashTable::Iterator iterator = m_processes.Begin(), end = m_processes.End(); iterator != end; iterator++) { + CordbProcess *pProcess = static_cast(*iterator); _ASSERTE(pProcess->IsSafeToSendEvents() || pProcess->m_unrecoverableError); if (pProcess->IsSafeToSendEvents() && !pProcess->m_unrecoverableError) { @@ -1108,8 +1104,7 @@ HRESULT Cordb::Terminate() #ifdef _DEBUG { - HASHFIND find; - _ASSERTE(m_processes.FindFirst(&find) == NULL); // should be emptied by neuter + _ASSERTE(m_processes.GetCount() == 0); // should be emptied by neuter } #endif //_DEBUG diff --git a/src/coreclr/debug/di/rspriv.h b/src/coreclr/debug/di/rspriv.h index dfe222ae283d8b..81d457812cf821 100644 --- a/src/coreclr/debug/di/rspriv.h +++ b/src/coreclr/debug/di/rspriv.h @@ -1713,6 +1713,8 @@ class EMPTY_BASES CordbHashTraits : public NonDacAwareSHashTraitsInternalRelease(); } static key_t GetKey(CordbBase *const &element) { return element->m_id; } static BOOL Equals(key_t left, key_t right) { return left == right; } static count_t Hash(key_t key) @@ -1731,6 +1733,8 @@ class CordbHashTable SHash m_table; public: + typedef SHash::Iterator Iterator; + bool IsInitialized(); #ifndef DACCESS_COMPILE @@ -1740,7 +1744,7 @@ class CordbHashTable m_pDbgLock = NULL; #endif } - virtual ~CordbHashTable(); + virtual ~CordbHashTable() = default; #ifdef _DEBUG // CordbHashTables may be protected by a lock. For debug-builds, we can associate @@ -1775,13 +1779,8 @@ class CordbHashTable CordbBase *UnsafeGetBase(ULONG_PTR id, BOOL fFab = TRUE); CordbBase *UnsafeRemoveBase(ULONG_PTR id); - CordbBase *UnsafeFindFirst(HASHFIND *find); - CordbBase *UnsafeFindNext(HASHFIND *find); - - // Unlocked versions don't assert that the lock us held. - CordbBase *UnsafeUnlockedFindFirst(HASHFIND *find); - CordbBase *UnsafeUnlockedFindNext(HASHFIND *find); - + Iterator Begin(); + Iterator End(); }; @@ -1813,9 +1812,6 @@ class CordbSafeHashTable : public CordbHashTable T* RemoveBase(ULONG_PTR id) { return static_cast(UnsafeRemoveBase(id)); } - T* FindFirst(HASHFIND *find) { return static_cast(UnsafeFindFirst(find)); } - T* FindNext(HASHFIND *find) { return static_cast(UnsafeFindNext(find)); } - // Neuter all items and clear void NeuterAndClear(RSLock * pLock); diff --git a/src/coreclr/debug/di/rspriv.inl b/src/coreclr/debug/di/rspriv.inl index c7641b06fc5cf2..42f118e0b9a344 100644 --- a/src/coreclr/debug/di/rspriv.inl +++ b/src/coreclr/debug/di/rspriv.inl @@ -633,19 +633,14 @@ inline void CordbSafeHashTable::CopyToArray(RSPtrArray * pArray) pArray->AllocOrThrow(count); - HASHFIND find; UINT32 idx = 0; - T * pCordbBase = FindFirst(&find); - while(idx < count) + for (Iterator iterator = Begin(), end = End(); iterator != end; iterator++) { - pArray->Assign(idx, pCordbBase); - idx++; - pCordbBase = FindNext(&find); + pArray->Assign(idx++, static_cast(*iterator)); } - // Assert is at end. - _ASSERTE(pCordbBase == NULL); + _ASSERTE(idx == count); } // Empty the contents of the hash to an array. Array gets ownersship. @@ -663,24 +658,21 @@ inline void CordbSafeHashTable::TransferToArray(RSPtrArray * pArray) { // Assumes caller has necessary locks - HASHFIND find; UINT32 count = GetCount(); UINT32 idx = 0; pArray->AllocOrThrow(count); - T *pCordbBase = FindFirst(&find); - while(idx < count) + for (Iterator iterator = Begin(), end = End(); iterator != end; iterator++) { - _ASSERTE(pCordbBase != NULL); - pArray->Assign(idx, pCordbBase); + T *pCordbBase = static_cast(*iterator); + pArray->Assign(idx++, pCordbBase); - idx++; RemoveBase((ULONG_PTR)pCordbBase->m_id); // this will call release, adjust GetCount() - pCordbBase = FindNext(&find); } // Assert is at end. + _ASSERTE(idx == count); _ASSERTE(GetCount() == 0); } @@ -699,19 +691,12 @@ inline void CordbSafeHashTable::NeuterAndClear(RSLock * pLock) { _ASSERTE(pLock->HasLock()); - HASHFIND find; - UINT32 count = GetCount(); - UINT32 idx = 0; - - T *pCordbBase = FindFirst(&find); - while(idx < count) + for (Iterator iterator = Begin(), end = End(); iterator != end; iterator++) { - _ASSERTE(pCordbBase != NULL); + T *pCordbBase = static_cast(*iterator); pCordbBase->Neuter(); - idx++; RemoveBase((ULONG_PTR)pCordbBase->m_id); // this will call release, adjust GetCount() - pCordbBase = FindNext(&find); } // Assert is at end. diff --git a/src/coreclr/debug/ee/debugger.cpp b/src/coreclr/debug/ee/debugger.cpp index 47def51321c184..f6b08fbacf9fa3 100644 --- a/src/coreclr/debug/ee/debugger.cpp +++ b/src/coreclr/debug/ee/debugger.cpp @@ -1186,13 +1186,10 @@ ULONG DebuggerMethodInfoTable::CheckDmiTable(void) { LIMITED_METHOD_CONTRACT; - HASHFIND find; ULONG count = 0; - for (const DebuggerMethodInfoEntry *entry = m_table.FindFirstEntry(&find); - entry != nullptr; - entry = m_table.FindNextEntry(&find)) + for (Iterator iterator = m_table.Begin(), end = m_table.End(); iterator != end; iterator++) { - _ASSERTE(entry->key.pModule != nullptr); + _ASSERTE(iterator->key.pModule != nullptr); count++; } _ASSERTE(count == m_table.GetCount()); @@ -5533,11 +5530,10 @@ void Debugger::UpdateAllModuleJMCFlag(bool fStatus) // and so there is nothing to update. if (m_pModules != NULL) { - HASHFIND f; - for (DebuggerModule * m = m_pModules->GetFirstModule(&f); - m != NULL; - m = m_pModules->GetNextModule(&f)) + for (DebuggerModuleTable::Iterator iterator = m_pModules->Begin(), end = m_pModules->End(); + iterator != end; iterator++) { + DebuggerModule *m = iterator->module; // the primary module may get called multiple times, but that's ok. UpdateModuleJMCFlag(m->GetRuntimeModule(), fStatus); } // end for all modules. @@ -5604,13 +5600,11 @@ void Debugger::SetModuleDefaultJMCStatus(Module * pRuntimeModule, bool fStatus) if (pTable != NULL) { Debugger::DebuggerDataLockHolder debuggerDataLockHolder(g_pDebugger); - HASHFIND info; - - for (DebuggerMethodInfo *dmi = pTable->GetFirstMethodInfo(&info); - dmi != NULL; - dmi = pTable->GetNextMethodInfo(&info)) + for (DebuggerMethodInfoTable::Iterator iterator = pTable->Begin(), end = pTable->End(); + iterator != end; iterator++) { - if (dmi->GetRuntimeModule() == pRuntimeModule) + DebuggerMethodInfo *dmi = iterator->mi; + if (dmi != nullptr && dmi->GetRuntimeModule() == pRuntimeModule) { // This DMI is in this module, so update its status dmi->SetJMCStatus(fStatus); @@ -9420,13 +9414,11 @@ void Debugger::UnloadModule(Module* pRuntimeModule) DebuggerMethodInfoTable * pTable = GetMethodInfoTable(); if (pTable != NULL) { - HASHFIND info; - - for (DebuggerMethodInfo *dmi = pTable->GetFirstMethodInfo(&info); - dmi != NULL; - dmi = pTable->GetNextMethodInfo(&info)) + for (DebuggerMethodInfoTable::Iterator iterator = pTable->Begin(), end = pTable->End(); + iterator != end; iterator++) { - if (dmi->m_module == pRuntimeModule) + DebuggerMethodInfo *dmi = iterator->mi; + if (dmi != nullptr && dmi->m_module == pRuntimeModule) { dmi->SetJMCStatus(false); } @@ -10675,14 +10667,14 @@ bool Debugger::HandleIPCEvent(DebuggerIPCEvent * pEvent) if (pMethodInfoTable != NULL) { - HASHFIND hashFind; DebuggerDataLockHolder debuggerDataLockHolder(this); - for (DebuggerMethodInfo * pMethodInfo = pMethodInfoTable->GetFirstMethodInfo(&hashFind); - pMethodInfo != NULL; - pMethodInfo = pMethodInfoTable->GetNextMethodInfo(&hashFind)) + for (DebuggerMethodInfoTable::Iterator iterator = pMethodInfoTable->Begin(), end = pMethodInfoTable->End(); + iterator != end; iterator++) { - pMethodInfo->SetJMCStatus(false); + DebuggerMethodInfo *pMethodInfo = iterator->mi; + if (pMethodInfo != nullptr) + pMethodInfo->SetJMCStatus(false); } } LOG((LF_CORDB, LL_EVERYTHING, "Done clearing JMC methods!\n")); diff --git a/src/coreclr/debug/ee/debugger.h b/src/coreclr/debug/ee/debugger.h index dfc0943ab06c2b..818353126c3132 100644 --- a/src/coreclr/debug/ee/debugger.h +++ b/src/coreclr/debug/ee/debugger.h @@ -3009,25 +3009,6 @@ inline COUNT_T DebuggerHashPointer(TADDR pointer) return hash; } -template -class DebuggerSHash : public SHash -{ -public: - const typename TRAITS::element_t *FindFirstEntry(HASHFIND *find) const - { - find->iNext = 0; - return FindNextEntry(find); - } - - const typename TRAITS::element_t *FindNextEntry(HASHFIND *find) const - { - typename TRAITS::count_t index = find->iNext; - const typename TRAITS::element_t *entry = this->GetNext(index); - find->iNext = index; - return entry; - } -}; - class EMPTY_BASES DebuggerPendingFuncEvalTraits : public DefaultSHashTraits { public: @@ -3090,12 +3071,13 @@ class EMPTY_BASES DebuggerModuleTraits : public DefaultSHashTraits m_table; + SHash m_table; #ifdef _DEBUG bool ThreadHoldsLock(); #endif public: + typedef SHash::Iterator Iterator; #ifndef DACCESS_COMPILE @@ -3111,9 +3093,8 @@ class DebuggerModuleTable DebuggerModule *GetModule(Module* module); - // We should never look for a NULL Module * - DebuggerModule *GetFirstModule(HASHFIND *info); - DebuggerModule *GetNextModule(HASHFIND *info); + Iterator Begin(); + Iterator End(); }; // struct DebuggerMethodInfoKey: Key for each of the method info hash table entries. @@ -3181,7 +3162,7 @@ class DebuggerMethodInfoTable virtual ~DebuggerMethodInfoTable() = default; private: - DebuggerSHash m_table; + SHash m_table; //#define _DEBUG_DMI_TABLE @@ -3200,6 +3181,7 @@ class DebuggerMethodInfoTable #endif // _DEBUG_DMI_TABLE public: + typedef SHash::Iterator Iterator; #ifndef DACCESS_COMPILE @@ -3225,8 +3207,8 @@ class DebuggerMethodInfoTable #endif // #ifndef DACCESS_COMPILE DebuggerMethodInfo *GetMethodInfo(Module *pModule, mdMethodDef token); - DebuggerMethodInfo *GetFirstMethodInfo(HASHFIND *info); - DebuggerMethodInfo *GetNextMethodInfo(HASHFIND *info); + Iterator Begin(); + Iterator End(); #ifdef DACCESS_COMPILE void EnumMemoryRegions(CLRDataEnumMemoryFlags flags); diff --git a/src/coreclr/debug/ee/debuggermodule.cpp b/src/coreclr/debug/ee/debuggermodule.cpp index 0895a7aca26d63..042a0627e448c7 100644 --- a/src/coreclr/debug/ee/debuggermodule.cpp +++ b/src/coreclr/debug/ee/debuggermodule.cpp @@ -58,19 +58,12 @@ void DebuggerModuleTable::Clear() _ASSERTE(ThreadHoldsLock()); - HASHFIND hf; - const DebuggerModuleEntry *pDME; - - pDME = m_table.FindFirstEntry(&hf); - - while (pDME) + for (Iterator iterator = m_table.Begin(), end = m_table.End(); iterator != end; iterator++) { - DebuggerModule *pDM = pDME->module; - TRACE_FREE(pDME->module); + DebuggerModule *pDM = iterator->module; + TRACE_FREE(pDM); m_table.Remove(pDM->GetRuntimeModule()); DeleteInteropSafe(pDM); - - pDME = m_table.FindNextEntry(&hf); } m_table.RemoveAll(); @@ -153,7 +146,7 @@ DebuggerModule *DebuggerModuleTable::GetModule(Module* module) return entry->module; } -DebuggerModule *DebuggerModuleTable::GetFirstModule(HASHFIND *info) +DebuggerModuleTable::Iterator DebuggerModuleTable::Begin() { CONTRACTL { @@ -164,14 +157,10 @@ DebuggerModule *DebuggerModuleTable::GetFirstModule(HASHFIND *info) _ASSERTE(ThreadHoldsLock()); - const DebuggerModuleEntry *entry = m_table.FindFirstEntry(info); - if (entry == NULL) - return NULL; - else - return entry->module; + return m_table.Begin(); } -DebuggerModule *DebuggerModuleTable::GetNextModule(HASHFIND *info) +DebuggerModuleTable::Iterator DebuggerModuleTable::End() { CONTRACTL { @@ -182,9 +171,5 @@ DebuggerModule *DebuggerModuleTable::GetNextModule(HASHFIND *info) _ASSERTE(ThreadHoldsLock()); - const DebuggerModuleEntry *entry = m_table.FindNextEntry(info); - if (entry == NULL) - return NULL; - else - return entry->module; + return m_table.End(); } diff --git a/src/coreclr/debug/ee/functioninfo.cpp b/src/coreclr/debug/ee/functioninfo.cpp index 1cdf472b831e7b..eba8280791968b 100644 --- a/src/coreclr/debug/ee/functioninfo.cpp +++ b/src/coreclr/debug/ee/functioninfo.cpp @@ -2190,12 +2190,9 @@ void DebuggerMethodInfoTable::ClearMethodsOfModule(Module *pModule) LOG((LF_CORDB, LL_INFO1000000, "CMOM:mod:0x%p (%s)\n", pModule ,pModule->GetDebugName())); - HASHFIND info; - - const DebuggerMethodInfoEntry *entry - = m_table.FindFirstEntry(&info); - while(entry != NULL) + for (Iterator iterator = m_table.Begin(), end = m_table.End(); iterator != end; iterator++) { + const DebuggerMethodInfoEntry *entry = &*iterator; Module *pMod = entry->key.pModule ; if (pMod == pModule) { @@ -2230,8 +2227,6 @@ void DebuggerMethodInfoTable::ClearMethodsOfModule(Module *pModule) dmi = dmi->m_prevMethodInfo; } } - - entry = m_table.FindNextEntry(&info); } } @@ -2312,54 +2307,32 @@ DebuggerMethodInfo *DebuggerMethodInfoTable::GetMethodInfo(Module *pModule, mdMe } -DebuggerMethodInfo *DebuggerMethodInfoTable::GetFirstMethodInfo(HASHFIND *info) +DebuggerMethodInfoTable::Iterator DebuggerMethodInfoTable::Begin() { CONTRACTL { NOTHROW; GC_NOTRIGGER; - - PRECONDITION(CheckPointer(info)); } CONTRACTL_END; _ASSERTE(g_pDebugger->HasDebuggerDataLock()); - const DebuggerMethodInfoEntry *entry = m_table.FindFirstEntry(info); - if (entry == NULL) - return NULL; - - return entry->mi; + return m_table.Begin(); } -DebuggerMethodInfo *DebuggerMethodInfoTable::GetNextMethodInfo(HASHFIND *info) +DebuggerMethodInfoTable::Iterator DebuggerMethodInfoTable::End() { CONTRACTL { NOTHROW; GC_NOTRIGGER; - - PRECONDITION(CheckPointer(info)); } CONTRACTL_END; _ASSERTE(g_pDebugger->HasDebuggerDataLock()); - const DebuggerMethodInfoEntry *entry = m_table.FindNextEntry(info); - - // We may have incremented the version number - // for methods that never got JITted, so we should - // pretend like they don't exist here. - while (entry != NULL && - entry->mi == NULL) - { - entry = m_table.FindNextEntry(info); - } - - if (entry == NULL) - return NULL; - - return entry->mi; + return m_table.End(); } @@ -2422,12 +2395,9 @@ void DebuggerMethodInfoTable::EnumMemoryRegions(CLRDataEnumMemoryFlags flags) DAC_ENUM_VTHIS(); m_table.EnumMemoryRegions(); - HASHFIND find; - for (const DebuggerMethodInfoEntry *entry = m_table.FindFirstEntry(&find); - entry != nullptr; - entry = m_table.FindNextEntry(&find)) + for (Iterator iterator = m_table.Begin(), end = m_table.End(); iterator != end; iterator++) { - entry->EnumMemoryRegions(flags); + iterator->EnumMemoryRegions(flags); } } #endif // #ifdef DACCESS_COMPILE diff --git a/src/coreclr/inc/shash.h b/src/coreclr/inc/shash.h index 0ddc217cbd9a3e..336db252b9bb4c 100644 --- a/src/coreclr/inc/shash.h +++ b/src/coreclr/inc/shash.h @@ -248,10 +248,6 @@ class EMPTY_BASES SHash : public TRAITS Iterator Begin() const; Iterator End() const; - // Index-based traversal does not retain a pointer into reallocatable storage. - // The caller must exclude additions while enumerating. - const element_t *GetNext(count_t &index) const; - #ifdef DACCESS_COMPILE void EnumMemoryRegions(); #endif diff --git a/src/coreclr/inc/shash.inl b/src/coreclr/inc/shash.inl index fa1359f3eff471..faffd39b8bbfa3 100644 --- a/src/coreclr/inc/shash.inl +++ b/src/coreclr/inc/shash.inl @@ -52,21 +52,6 @@ SHash::~SHash() delete [] m_table; } -template -const typename SHash::element_t *SHash::GetNext(count_t &index) const -{ - LIMITED_METHOD_DAC_CONTRACT; - - Iterator iterator(this, FALSE); - iterator.m_index = index; - iterator.First(); - if (iterator.m_index >= m_tableSize) - return nullptr; - - index = iterator.m_index + 1; - return &*iterator; -} - #ifdef DACCESS_COMPILE template void SHash::EnumMemoryRegions() From e3b2e02ea2e5fe543f0a7f9df5ae40a59a29e724 Mon Sep 17 00:00:00 2001 From: Aaron Robinson Date: Wed, 21 Oct 2026 16:54:34 -0700 Subject: [PATCH 10/13] Use ReplacePtr for SHash updates Remove the callback API and reuse existing slots for replacement. Split trait-controlled cleanup from explicit cleanup overrides, and mark in-place updates as non-cleaning. Validated Checked x64 runtime/DAC/DBI, native EE/DBI checks, and live ICorDebug scenarios. The cleanup-override check fails before the change and passes afterward. Optimized counter code is unchanged by the overload split. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ce4b0890-6559-4b90-b46f-11179e70f973 --- src/coreclr/debug/ee/controller.cpp | 6 ++-- src/coreclr/debug/ee/debugger.cpp | 31 +++++++++--------- src/coreclr/debug/ee/functioninfo.cpp | 40 +++++++++++------------- src/coreclr/inc/shash.h | 20 +++++------- src/coreclr/inc/shash.inl | 24 +++----------- src/coreclr/vm/eetoprofinterfaceimpl.cpp | 14 ++++++--- 6 files changed, 59 insertions(+), 76 deletions(-) diff --git a/src/coreclr/debug/ee/controller.cpp b/src/coreclr/debug/ee/controller.cpp index 11c840e8297b59..81f513d11b4364 100644 --- a/src/coreclr/debug/ee/controller.cpp +++ b/src/coreclr/debug/ee/controller.cpp @@ -344,7 +344,7 @@ DebuggerControllerPatch *DebuggerPatchTable::AddPatch(DebuggerPatchKey key) DebuggerPatchListEntry updated = *list; updated.first = index; - m_patchLists.ReplacePtr(list, updated); + m_patchLists.ReplacePtr(list, updated, false /* invokeCleanupAction */); return patch; } @@ -360,7 +360,7 @@ void DebuggerPatchTable::LinkPatch(DebuggerControllerPatch *patch) DebuggerPatchListEntry updated = *list; updated.first = index; - m_patchLists.ReplacePtr(list, updated); + m_patchLists.ReplacePtr(list, updated, false /* invokeCleanupAction */); } #endif // DACCESS_COMPILE @@ -445,7 +445,7 @@ void DebuggerPatchTable::SpliceOutOfList(DebuggerControllerPatch *patch) { DebuggerPatchListEntry updated = *list; updated.first = patch->entry.iNext; - m_patchLists.ReplacePtr(list, updated); + m_patchLists.ReplacePtr(list, updated, false /* invokeCleanupAction */); } } diff --git a/src/coreclr/debug/ee/debugger.cpp b/src/coreclr/debug/ee/debugger.cpp index f6b08fbacf9fa3..de417158e2c275 100644 --- a/src/coreclr/debug/ee/debugger.cpp +++ b/src/coreclr/debug/ee/debugger.cpp @@ -1100,13 +1100,14 @@ HRESULT DebuggerPendingFuncEvalTable::AddPendingEval(Thread *pThread, DebuggerEv _ASSERTE(pThread != nullptr && pDE != nullptr); _ASSERTE(pDE->m_nextPendingEval == nullptr); - if (m_table.Update(pThread, [pDE](DebuggerPendingFuncEval &entry) + const DebuggerPendingFuncEval *entry = m_table.LookupPtr(pThread); + if (entry != nullptr) { + DebuggerPendingFuncEval updated = *entry; // Keep newest-first order independently of SHash rehashing. - pDE->m_nextPendingEval = entry.pDE; - entry.pDE = pDE; - })) - { + pDE->m_nextPendingEval = entry->pDE; + updated.pDE = pDE; + m_table.ReplacePtr(entry, updated, false /* invokeCleanupAction */); return S_OK; } @@ -1127,21 +1128,23 @@ void DebuggerPendingFuncEvalTable::RemovePendingEval(Thread *pThread) _ASSERTE(pThread != nullptr); - DebuggerEval *pDE = nullptr; - if (!m_table.Update(pThread, [&pDE](DebuggerPendingFuncEval &entry) - { - pDE = entry.pDE; - if (pDE->m_nextPendingEval != nullptr) - { - entry.pDE = pDE->m_nextPendingEval; - } - })) + const DebuggerPendingFuncEval *entry = m_table.LookupPtr(pThread); + if (entry == nullptr) { return; } + DebuggerEval *pDE = entry->pDE; if (pDE->m_nextPendingEval == nullptr) + { m_table.Remove(pThread); + } + else + { + DebuggerPendingFuncEval updated = *entry; + updated.pDE = pDE->m_nextPendingEval; + m_table.ReplacePtr(entry, updated, false /* invokeCleanupAction */); + } pDE->m_nextPendingEval = nullptr; } #endif // DACCESS_COMPILE diff --git a/src/coreclr/debug/ee/functioninfo.cpp b/src/coreclr/debug/ee/functioninfo.cpp index eba8280791968b..39d352b1b53eb1 100644 --- a/src/coreclr/debug/ee/functioninfo.cpp +++ b/src/coreclr/debug/ee/functioninfo.cpp @@ -2159,21 +2159,19 @@ HRESULT DebuggerMethodInfoTable::OverwriteMethodInfo(Module *pModule, dmik.pModule = pModule; dmik.token = token; - HRESULT hr = E_FAIL; - m_table.Update(dmik, [&](DebuggerMethodInfoEntry &entry) + const DebuggerMethodInfoEntry *entry = m_table.LookupPtr(dmik); + if (entry == nullptr || (fOnlyIfNull && entry->mi != nullptr)) { - if ( (fOnlyIfNull && entry.mi == NULL) - || !fOnlyIfNull) - { - entry.mi = mi; + return E_FAIL; + } - LOG((LF_CORDB, LL_INFO1000, "DMIT::OMI: mod:%p tok:0x%08x remap nVer:0x%zx\n", - pModule, token, entry.nVersionLastRemapped)); - hr = S_OK; - } - }); + DebuggerMethodInfoEntry updated = *entry; + updated.mi = mi; + m_table.ReplacePtr(entry, updated, false /* invokeCleanupAction */); - return hr; + LOG((LF_CORDB, LL_INFO1000, "DMIT::OMI: mod:%p tok:0x%08x remap nVer:0x%zx\n", + pModule, token, updated.nVersionLastRemapped)); + return S_OK; } // pModule is being destroyed - remove any entries that belong to it. Why? @@ -2242,16 +2240,14 @@ void DebuggerMethodInfoTable::DeleteEntryDMI(const DebuggerMethodInfoEntry *entr } CONTRACTL_END; - bool updated = m_table.Update(entry->key, [](DebuggerMethodInfoEntry &element) - { - DebuggerMethodInfo *dmiPrev = element.mi->m_prevMethodInfo; - TRACE_FREE(element.mi); - DeleteInteropSafe(element.mi); - element.mi = dmiPrev; - if ( dmiPrev != NULL ) - dmiPrev->m_nextMethodInfo = NULL; - }); - _ASSERTE(updated); + DebuggerMethodInfoEntry updated = *entry; + DebuggerMethodInfo *dmiPrev = updated.mi->m_prevMethodInfo; + TRACE_FREE(updated.mi); + DeleteInteropSafe(updated.mi); + updated.mi = dmiPrev; + m_table.ReplacePtr(entry, updated, false /* invokeCleanupAction */); + if (dmiPrev != nullptr) + dmiPrev->m_nextMethodInfo = nullptr; } #endif // #ifndef DACCESS_COMPILE diff --git a/src/coreclr/inc/shash.h b/src/coreclr/inc/shash.h index 336db252b9bb4c..b4155580a2da38 100644 --- a/src/coreclr/inc/shash.h +++ b/src/coreclr/inc/shash.h @@ -22,7 +22,7 @@ // (@todo: this can be remedied by delaying destruction of old tables during reallocation, e.g. during GC) // - Remove operations may be asynchronous with Lookup/Add, unless elements are also deallocated. (In which // case full synchronization is required) -// - Update operations must be synchronized with other accesses to the updated element. +// - Replacing elements must be synchronized with other accesses to those elements. // Common "gotchas": // - The Add method never replaces an element. The new element will be added even if an element with the same @@ -79,8 +79,8 @@ // static void OnDestructPerEntryCleanupAction(ELEMENT& e) Called on every element when in hashtable destructor. // s_DestructPerEntryCleanupAction must be set to true if implemented. // static void OnRemovePerEntryCleanupAction(ELEMENT& e) Called when an element is removed from the hashtable, including when -// the hashtable is destructed. s_RemovePerEntryCleanupAction must be set to true -// if implemented. +// the hashtable is destructed. Set s_RemovePerEntryCleanupAction to true for +// automatic cleanup. ReplacePtr can explicitly override this policy. // // s_growth_factor_numerator // s_growth_factor_denominator Factor to grow allocation (numerator/denominator). @@ -199,16 +199,12 @@ class EMPTY_BASES SHash : public TRAITS const element_t* LookupPtr(key_t key) const; - // Update the first matching element in place, returning false if no match exists. - // The callback must preserve the key and sentinel state, and must not structurally modify the table. - // It must not retain a mutable pointer or reference to the element. - // Callback exceptions propagate; changes already made are not rolled back. - template - bool Update(key_t key, Functor &&functor); - // Pointer-based flavor to replace an existing element (allows efficient access to tables of structures) - void ReplacePtr(const element_t *elementPtr, const element_t &newElement, bool invokeCleanupAction = true); + void ReplacePtr(const element_t *elementPtr, const element_t &newElement); + + // The explicit bool overrides the traits' removal-cleanup policy for this replacement. + void ReplacePtr(const element_t *elementPtr, const element_t &newElement, bool invokeCleanupAction); // Add an element to the hash table. This will never replace an element; multiple // elements may be stored with the same key. @@ -345,7 +341,7 @@ class EMPTY_BASES SHash : public TRAITS // Utility function to find the first element with the given key in // the hash table. - element_t* Lookup(PTR_element_t table, count_t tableSize, key_t key) const; + const element_t* Lookup(PTR_element_t table, count_t tableSize, key_t key) const; // Utility function to remove the first element with the given key // in the hash table. diff --git a/src/coreclr/inc/shash.inl b/src/coreclr/inc/shash.inl index faffd39b8bbfa3..a131917ba74697 100644 --- a/src/coreclr/inc/shash.inl +++ b/src/coreclr/inc/shash.inl @@ -109,27 +109,11 @@ const typename SHash::element_t * SHash::LookupPtr(key_t key) co } template -template -bool SHash::Update(key_t key, Functor &&functor) +void SHash::ReplacePtr(const element_t *elementPtr, const element_t &newElement) { WRAPPER_NO_CONTRACT; - element_t *element = Lookup(m_table, m_tableSize, key); - if (element == nullptr) - return false; - -#ifdef _DEBUG - PTR_element_t table = m_table; - count_t tableSize = m_tableSize; - count_t count = m_tableCount; -#endif // _DEBUG - - functor(*element); - - _ASSERTE(m_table == table && m_tableSize == tableSize && m_tableCount == count); - _ASSERTE(!TRAITS::IsNull(*element) && !TRAITS::IsDeleted(*element)); - _ASSERTE(TRAITS::Equals(TRAITS::GetKey(*element), key)); - return true; + ReplacePtr(elementPtr, newElement, TRAITS::s_RemovePerEntryCleanupAction); } template @@ -150,7 +134,7 @@ void SHash::ReplacePtr(const element_t *elementPtr, const element_t &new } CONTRACTL_END; - if (TRAITS::s_RemovePerEntryCleanupAction && invokeCleanupAction) + if (invokeCleanupAction) { TRAITS::OnRemovePerEntryCleanupAction(*elementPtr); } @@ -676,7 +660,7 @@ SHash::DeleteOldTable(element_t * oldTable) } template -typename SHash::element_t * SHash::Lookup(PTR_element_t table, count_t tableSize, key_t key) const +const typename SHash::element_t * SHash::Lookup(PTR_element_t table, count_t tableSize, key_t key) const { CONTRACTL { diff --git a/src/coreclr/vm/eetoprofinterfaceimpl.cpp b/src/coreclr/vm/eetoprofinterfaceimpl.cpp index c48fd7198c8caa..8803987b83d6e3 100644 --- a/src/coreclr/vm/eetoprofinterfaceimpl.cpp +++ b/src/coreclr/vm/eetoprofinterfaceimpl.cpp @@ -1427,13 +1427,17 @@ HRESULT EEToProfInterfaceImpl::AllocByClass(ObjectID objId, ClassID clsId, void _ASSERTE(dwProcId == GetCurrentProcessId()); - if (!pData->hashTable.Update(clsId, [](AllocByClassEntry &entry) + const AllocByClassEntry *entry = pData->hashTable.LookupPtr(clsId); + if (entry != nullptr) { - entry.m_count++; - })) + AllocByClassEntry updated = *entry; + updated.m_count++; + pData->hashTable.ReplacePtr(entry, updated, false /* invokeCleanupAction */); + } + else { - AllocByClassEntry entry = { clsId, 1 }; - if (!pData->hashTable.AddNoThrow(entry)) + AllocByClassEntry newEntry = { clsId, 1 }; + if (!pData->hashTable.AddNoThrow(newEntry)) return E_OUTOFMEMORY; } From 555b7a94f345ccf82344182800a591ecd9697a5b Mon Sep 17 00:00:00 2001 From: Aaron Robinson Date: Thu, 22 Oct 2026 10:07:52 -0700 Subject: [PATCH 11/13] Use callback updates for SHash mutations Keep LookupPtr read-only and replace ReplacePtr and lookup-entry mutation casts with Update. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ce4b0890-6559-4b90-b46f-11179e70f973 --- src/coreclr/debug/daccess/daccess.cpp | 28 +++---- src/coreclr/debug/di/hash.cpp | 15 ++-- src/coreclr/debug/ee/controller.cpp | 44 +++++------ src/coreclr/debug/ee/debugger.cpp | 36 ++++----- src/coreclr/debug/ee/debugger.h | 2 +- src/coreclr/debug/ee/functioninfo.cpp | 49 ++++++------ src/coreclr/inc/shash.h | 20 ++--- src/coreclr/inc/shash.inl | 42 ++++------ .../nativeaot/Runtime/GcStressControl.cpp | 23 +++--- src/coreclr/nativeaot/Runtime/shash.h | 11 ++- src/coreclr/nativeaot/Runtime/shash.inl | 25 +++++- src/coreclr/vm/assemblybinder.cpp | 78 +++++++++---------- src/coreclr/vm/callcounting.cpp | 8 +- src/coreclr/vm/crossloaderallocatorhash.inl | 28 +++++-- src/coreclr/vm/eetoprofinterfaceimpl.cpp | 9 +-- src/coreclr/vm/inlinetracking.cpp | 8 +- 16 files changed, 220 insertions(+), 206 deletions(-) diff --git a/src/coreclr/debug/daccess/daccess.cpp b/src/coreclr/debug/daccess/daccess.cpp index c0e25f52e3a639..42196705413479 100644 --- a/src/coreclr/debug/daccess/daccess.cpp +++ b/src/coreclr/debug/daccess/daccess.cpp @@ -1487,21 +1487,19 @@ DAC_INSTANCE* DacInstanceManager::Add(DAC_INSTANCE* inst) { _ASSERTE(inst != NULL); - const KeyValuePair* pEntry = m_hash.LookupPtr(inst->addr); - if (pEntry != NULL) - { - DAC_INSTANCE* existing = pEntry->Value(); + if (!m_hash.Update(inst->addr, [&](KeyValuePair &entry) + { + DAC_INSTANCE* existing = entry.Value(); - //This is necessary to preserve the semantics of Supersede, however, it - //is more or less dead code. - inst->next = existing; + //This is necessary to preserve the semantics of Supersede, however, it + //is more or less dead code. + inst->next = existing; - //verify descending order - _ASSERTE(inst->size >= existing->size); + //verify descending order + _ASSERTE(inst->size >= existing->size); - m_hash.ReplacePtr(pEntry, KeyValuePair(inst->addr, inst)); - } - else + entry = KeyValuePair(inst->addr, inst); + })) { inst->next = NULL; m_hash.Add(KeyValuePair(inst->addr, inst)); @@ -1747,7 +1745,11 @@ DacInstanceManager::Supersede(DAC_INSTANCE* inst) // Removing the head of the chain. if (inst->next != NULL) { - m_hash.ReplacePtr(pEntry, KeyValuePair(inst->addr, inst->next)); + bool updated = m_hash.Update(inst->addr, [&](KeyValuePair &entry) + { + entry = KeyValuePair(inst->addr, inst->next); + }); + _ASSERTE(updated); } else { diff --git a/src/coreclr/debug/di/hash.cpp b/src/coreclr/debug/di/hash.cpp index 5144bcbede5196..d42e05e03e560b 100644 --- a/src/coreclr/debug/di/hash.cpp +++ b/src/coreclr/debug/di/hash.cpp @@ -40,18 +40,17 @@ HRESULT CordbHashTable::UnsafeSwapBase(CordbBase *pOldBase, CordbBase *pNewBase) ULONG_PTR id = (ULONG_PTR)pOldBase->m_id; - CordbBase *const *entry = m_table.LookupPtr(id); - - if (entry == NULL) + if (!m_table.Update(id, [&](CordbBase *&entry) + { + _ASSERTE(entry == pOldBase); + _ASSERTE(pOldBase->m_id == pNewBase->m_id); + pNewBase->InternalAddRef(); + entry = pNewBase; + })) { return E_FAIL; } - _ASSERTE(*entry == pOldBase); - _ASSERTE(pOldBase->m_id == pNewBase->m_id); - pNewBase->InternalAddRef(); - m_table.ReplacePtr(entry, pNewBase); - // release the hash table's reference to the old base and transfer it // to the new one. pOldBase->InternalRelease(); diff --git a/src/coreclr/debug/ee/controller.cpp b/src/coreclr/debug/ee/controller.cpp index 81f513d11b4364..a5d596deaf5303 100644 --- a/src/coreclr/debug/ee/controller.cpp +++ b/src/coreclr/debug/ee/controller.cpp @@ -332,35 +332,34 @@ DebuggerControllerPatch *DebuggerPatchTable::AddPatch(DebuggerPatchKey key) // Allocate the index before taking a slot so OOM leaves the table unchanged. EnsureList(key); - const DebuggerPatchListEntry *list = m_patchLists.LookupPtr(key); ULONG index = m_iFree; DebuggerControllerPatch *patch = reinterpret_cast(EntryPtr(index)); m_iFree = patch->entry.iFree; memset(patch, 0, sizeof(*patch)); patch->entry.iPrev = DPT_INVALID_SLOT; - patch->entry.iNext = list->first; - if (list->first != DPT_INVALID_SLOT) - EntryPtr(list->first)->iPrev = index; - - DebuggerPatchListEntry updated = *list; - updated.first = index; - m_patchLists.ReplacePtr(list, updated, false /* invokeCleanupAction */); + bool updated = m_patchLists.Update(key, [&](DebuggerPatchListEntry &list) + { + patch->entry.iNext = list.first; + if (list.first != DPT_INVALID_SLOT) + EntryPtr(list.first)->iPrev = index; + list.first = index; + }); + _ASSERTE(updated); return patch; } void DebuggerPatchTable::LinkPatch(DebuggerControllerPatch *patch) { - const DebuggerPatchListEntry *list = m_patchLists.LookupPtr(Key(patch)); - _ASSERTE(list != nullptr); ULONG index = ItemIndex(reinterpret_cast(patch)); patch->entry.iPrev = DPT_INVALID_SLOT; - patch->entry.iNext = list->first; - if (list->first != DPT_INVALID_SLOT) - EntryPtr(list->first)->iPrev = index; - - DebuggerPatchListEntry updated = *list; - updated.first = index; - m_patchLists.ReplacePtr(list, updated, false /* invokeCleanupAction */); + bool updated = m_patchLists.Update(Key(patch), [&](DebuggerPatchListEntry &list) + { + patch->entry.iNext = list.first; + if (list.first != DPT_INVALID_SLOT) + EntryPtr(list.first)->iPrev = index; + list.first = index; + }); + _ASSERTE(updated); } #endif // DACCESS_COMPILE @@ -428,8 +427,7 @@ void DebuggerPatchTable::SortPatchIntoPatchList(DebuggerControllerPatch **ppPatc void DebuggerPatchTable::SpliceOutOfList(DebuggerControllerPatch *patch) { DebuggerPatchKey key = Key(patch); - const DebuggerPatchListEntry *list = m_patchLists.LookupPtr(key); - _ASSERTE(list != nullptr); + _ASSERTE(m_patchLists.LookupPtr(key) != nullptr); // Fix up pointers to chain if (patch->entry.iPrev == DPT_INVALID_SLOT) @@ -443,9 +441,11 @@ void DebuggerPatchTable::SpliceOutOfList(DebuggerControllerPatch *patch) } else { - DebuggerPatchListEntry updated = *list; - updated.first = patch->entry.iNext; - m_patchLists.ReplacePtr(list, updated, false /* invokeCleanupAction */); + bool updated = m_patchLists.Update(key, [&](DebuggerPatchListEntry &entry) + { + entry.first = patch->entry.iNext; + }); + _ASSERTE(updated); } } diff --git a/src/coreclr/debug/ee/debugger.cpp b/src/coreclr/debug/ee/debugger.cpp index de417158e2c275..d10e15791a0659 100644 --- a/src/coreclr/debug/ee/debugger.cpp +++ b/src/coreclr/debug/ee/debugger.cpp @@ -1100,14 +1100,13 @@ HRESULT DebuggerPendingFuncEvalTable::AddPendingEval(Thread *pThread, DebuggerEv _ASSERTE(pThread != nullptr && pDE != nullptr); _ASSERTE(pDE->m_nextPendingEval == nullptr); - const DebuggerPendingFuncEval *entry = m_table.LookupPtr(pThread); - if (entry != nullptr) - { - DebuggerPendingFuncEval updated = *entry; - // Keep newest-first order independently of SHash rehashing. - pDE->m_nextPendingEval = entry->pDE; - updated.pDE = pDE; - m_table.ReplacePtr(entry, updated, false /* invokeCleanupAction */); + if (m_table.Update(pThread, [&](DebuggerPendingFuncEval &entry) + { + // Keep newest-first order independently of SHash rehashing. + pDE->m_nextPendingEval = entry.pDE; + entry.pDE = pDE; + })) + { return S_OK; } @@ -1128,23 +1127,24 @@ void DebuggerPendingFuncEvalTable::RemovePendingEval(Thread *pThread) _ASSERTE(pThread != nullptr); - const DebuggerPendingFuncEval *entry = m_table.LookupPtr(pThread); - if (entry == nullptr) + DebuggerEval *pDE = nullptr; + bool removeEntry = false; + if (!m_table.Update(pThread, [&](DebuggerPendingFuncEval &entry) + { + pDE = entry.pDE; + if (pDE->m_nextPendingEval == nullptr) + removeEntry = true; + else + entry.pDE = pDE->m_nextPendingEval; + })) { return; } - DebuggerEval *pDE = entry->pDE; - if (pDE->m_nextPendingEval == nullptr) + if (removeEntry) { m_table.Remove(pThread); } - else - { - DebuggerPendingFuncEval updated = *entry; - updated.pDE = pDE->m_nextPendingEval; - m_table.ReplacePtr(entry, updated, false /* invokeCleanupAction */); - } pDE->m_nextPendingEval = nullptr; } #endif // DACCESS_COMPILE diff --git a/src/coreclr/debug/ee/debugger.h b/src/coreclr/debug/ee/debugger.h index 818353126c3132..d50ee2bb56301c 100644 --- a/src/coreclr/debug/ee/debugger.h +++ b/src/coreclr/debug/ee/debugger.h @@ -3202,7 +3202,7 @@ class DebuggerMethodInfoTable // {Module*,mdMethodDef} // (b) Perf: don't waste the memory! void ClearMethodsOfModule(Module *pModule); - void DeleteEntryDMI(const DebuggerMethodInfoEntry *entry); + void DeleteEntryDMI(DebuggerMethodInfoEntry &entry); #endif // #ifndef DACCESS_COMPILE diff --git a/src/coreclr/debug/ee/functioninfo.cpp b/src/coreclr/debug/ee/functioninfo.cpp index 39d352b1b53eb1..383731117c1619 100644 --- a/src/coreclr/debug/ee/functioninfo.cpp +++ b/src/coreclr/debug/ee/functioninfo.cpp @@ -2159,19 +2159,18 @@ HRESULT DebuggerMethodInfoTable::OverwriteMethodInfo(Module *pModule, dmik.pModule = pModule; dmik.token = token; - const DebuggerMethodInfoEntry *entry = m_table.LookupPtr(dmik); - if (entry == nullptr || (fOnlyIfNull && entry->mi != nullptr)) - { - return E_FAIL; - } - - DebuggerMethodInfoEntry updated = *entry; - updated.mi = mi; - m_table.ReplacePtr(entry, updated, false /* invokeCleanupAction */); - - LOG((LF_CORDB, LL_INFO1000, "DMIT::OMI: mod:%p tok:0x%08x remap nVer:0x%zx\n", - pModule, token, updated.nVersionLastRemapped)); - return S_OK; + HRESULT hr = E_FAIL; + m_table.Update(dmik, [&](DebuggerMethodInfoEntry &entry) + { + if (fOnlyIfNull && entry.mi != nullptr) + return; + + entry.mi = mi; + hr = S_OK; + LOG((LF_CORDB, LL_INFO1000, "DMIT::OMI: mod:%p tok:0x%08x remap nVer:0x%zx\n", + pModule, token, entry.nVersionLastRemapped)); + }); + return hr; } // pModule is being destroyed - remove any entries that belong to it. Why? @@ -2194,12 +2193,16 @@ void DebuggerMethodInfoTable::ClearMethodsOfModule(Module *pModule) Module *pMod = entry->key.pModule ; if (pMod == pModule) { - // This method actually got mitted, at least + // This method actually got emitted, at least // once - remove all version info. - while(entry->mi != NULL) + bool updated = m_table.Update(entry->key, [&](DebuggerMethodInfoEntry ¤t) { - DeleteEntryDMI(entry); - } + while (current.mi != NULL) + { + DeleteEntryDMI(current); + } + }); + _ASSERTE(updated); m_table.Remove(entry->key); } @@ -2228,7 +2231,7 @@ void DebuggerMethodInfoTable::ClearMethodsOfModule(Module *pModule) } } -void DebuggerMethodInfoTable::DeleteEntryDMI(const DebuggerMethodInfoEntry *entry) +void DebuggerMethodInfoTable::DeleteEntryDMI(DebuggerMethodInfoEntry &entry) { CONTRACTL { @@ -2240,12 +2243,10 @@ void DebuggerMethodInfoTable::DeleteEntryDMI(const DebuggerMethodInfoEntry *entr } CONTRACTL_END; - DebuggerMethodInfoEntry updated = *entry; - DebuggerMethodInfo *dmiPrev = updated.mi->m_prevMethodInfo; - TRACE_FREE(updated.mi); - DeleteInteropSafe(updated.mi); - updated.mi = dmiPrev; - m_table.ReplacePtr(entry, updated, false /* invokeCleanupAction */); + DebuggerMethodInfo *dmiPrev = entry.mi->m_prevMethodInfo; + TRACE_FREE(entry.mi); + DeleteInteropSafe(entry.mi); + entry.mi = dmiPrev; if (dmiPrev != nullptr) dmiPrev->m_nextMethodInfo = nullptr; } diff --git a/src/coreclr/inc/shash.h b/src/coreclr/inc/shash.h index b4155580a2da38..9870f9319e4285 100644 --- a/src/coreclr/inc/shash.h +++ b/src/coreclr/inc/shash.h @@ -80,7 +80,7 @@ // s_DestructPerEntryCleanupAction must be set to true if implemented. // static void OnRemovePerEntryCleanupAction(ELEMENT& e) Called when an element is removed from the hashtable, including when // the hashtable is destructed. Set s_RemovePerEntryCleanupAction to true for -// automatic cleanup. ReplacePtr can explicitly override this policy. +// automatic cleanup. Update does not invoke this action. // // s_growth_factor_numerator // s_growth_factor_denominator Factor to grow allocation (numerator/denominator). @@ -195,16 +195,18 @@ class EMPTY_BASES SHash : public TRAITS element_t Lookup(key_t key) const; - // Pointer-based flavor of Lookup (allows efficient access to tables of structures) + // Read-only pointer-based lookup. Use Update to modify an existing element. const element_t* LookupPtr(key_t key) const; - // Pointer-based flavor to replace an existing element (allows efficient access to tables of structures) - - void ReplacePtr(const element_t *elementPtr, const element_t &newElement); - - // The explicit bool overrides the traits' removal-cleanup policy for this replacement. - void ReplacePtr(const element_t *elementPtr, const element_t &newElement, bool invokeCleanupAction); + // Update the first matching element in place, returning false if no match exists. + // The callback must preserve the key and sentinel state, and must not structurally modify the table. + // It must not retain a mutable pointer or reference to the element. + // Callers must synchronize updates with other access to the affected element. + // No removal cleanup is performed; the callback manages ownership of any replaced value. + // Callback exceptions propagate; changes already made are not rolled back. + template + bool Update(key_t key, Functor &&functor); // Add an element to the hash table. This will never replace an element; multiple // elements may be stored with the same key. @@ -341,7 +343,7 @@ class EMPTY_BASES SHash : public TRAITS // Utility function to find the first element with the given key in // the hash table. - const element_t* Lookup(PTR_element_t table, count_t tableSize, key_t key) const; + element_t* Lookup(PTR_element_t table, count_t tableSize, key_t key) const; // Utility function to remove the first element with the given key // in the hash table. diff --git a/src/coreclr/inc/shash.inl b/src/coreclr/inc/shash.inl index a131917ba74697..fe7d118d1c17c0 100644 --- a/src/coreclr/inc/shash.inl +++ b/src/coreclr/inc/shash.inl @@ -109,37 +109,27 @@ const typename SHash::element_t * SHash::LookupPtr(key_t key) co } template -void SHash::ReplacePtr(const element_t *elementPtr, const element_t &newElement) +template +bool SHash::Update(key_t key, Functor &&functor) { WRAPPER_NO_CONTRACT; - ReplacePtr(elementPtr, newElement, TRAITS::s_RemovePerEntryCleanupAction); -} + element_t *element = Lookup(m_table, m_tableSize, key); + if (element == nullptr) + return false; -template -void SHash::ReplacePtr(const element_t *elementPtr, const element_t &newElement, bool invokeCleanupAction) -{ - CONTRACTL - { - NOTHROW; - GC_NOTRIGGER; - INSTANCE_CHECK; - PRECONDITION(m_table <= elementPtr); - PRECONDITION(elementPtr < m_table + m_tableSize); - PRECONDITION(!TRAITS::IsNull(*elementPtr)); - PRECONDITION(!TRAITS::IsDeleted(*elementPtr)); - PRECONDITION(!TRAITS::IsNull(newElement)); - PRECONDITION(!TRAITS::IsDeleted(newElement)); - PRECONDITION(TRAITS::Equals(TRAITS::GetKey(newElement), TRAITS::GetKey(*elementPtr))); - } - CONTRACTL_END; +#ifdef _DEBUG + PTR_element_t table = m_table; + count_t tableSize = m_tableSize; + count_t count = m_tableCount; +#endif // _DEBUG - if (invokeCleanupAction) - { - TRAITS::OnRemovePerEntryCleanupAction(*elementPtr); - } + functor(*element); - *const_cast(elementPtr) = newElement; + _ASSERTE(m_table == table && m_tableSize == tableSize && m_tableCount == count); + _ASSERTE(!TRAITS::IsNull(*element) && !TRAITS::IsDeleted(*element)); + _ASSERTE(TRAITS::Equals(TRAITS::GetKey(*element), key)); + return true; } template @@ -660,7 +650,7 @@ SHash::DeleteOldTable(element_t * oldTable) } template -const typename SHash::element_t * SHash::Lookup(PTR_element_t table, count_t tableSize, key_t key) const +typename SHash::element_t * SHash::Lookup(PTR_element_t table, count_t tableSize, key_t key) const { CONTRACTL { diff --git a/src/coreclr/nativeaot/Runtime/GcStressControl.cpp b/src/coreclr/nativeaot/Runtime/GcStressControl.cpp index ec1922eaab0c48..c729e6626fc360 100644 --- a/src/coreclr/nativeaot/Runtime/GcStressControl.cpp +++ b/src/coreclr/nativeaot/Runtime/GcStressControl.cpp @@ -95,25 +95,21 @@ class GcStressControl CrstHolder lh(&s_lock); - const CallsiteCountEntry * pEntry = s_callsites.LookupPtr(CallsiteIP); - size_t hits; - - if (pEntry == NULL) - { - hits = 1; - CallsiteCountEntry e = {CallsiteIP, 1, 1, ht}; - s_callsites.AddOrReplace(e); - } - else + bool found = s_callsites.Update(CallsiteIP, [&](CallsiteCountEntry &entry) { - hits = ++(const_cast(pEntry)->countHit); + entry.countHit++; if (bForceGC) { - ++(const_cast(pEntry)->countForced); + entry.countForced++; } + }); + if (!found) + { + CallsiteCountEntry e = {CallsiteIP, 1, 1, ht}; + s_callsites.AddOrReplace(e); } - return pEntry == NULL; + return !found; } static bool GcStressTriggerFirstHit(uintptr_t CallsiteIP, HijackType ht) @@ -177,4 +173,3 @@ bool ShouldHijackForGcStress(uintptr_t CallsiteIP, HijackType ht) #endif // FEATURE_GC_STRESS & !DACCESS_COMPILE - diff --git a/src/coreclr/nativeaot/Runtime/shash.h b/src/coreclr/nativeaot/Runtime/shash.h index 27b90157250a4f..c36deda34f4fa7 100644 --- a/src/coreclr/nativeaot/Runtime/shash.h +++ b/src/coreclr/nativeaot/Runtime/shash.h @@ -151,10 +151,17 @@ class SHash : public TRAITS element_t Lookup(key_t key) const; - // Pointer-based flavor of Lookup (allows efficient access to tables of structures) + // Read-only pointer-based lookup. Use Update to modify an existing element. const element_t* LookupPtr(key_t key) const; + // Update the first matching element in place, returning false if no match exists. + // The callback must preserve the key and sentinel state, and must not structurally modify the table. + // It must not retain a mutable pointer or reference to the element. + // Callers must synchronize updates with other access to the affected element. + template + bool Update(key_t key, Functor &&functor); + // Add an element to the hash table. This will never replace an element; multiple // elements may be stored with the same key. // @@ -252,7 +259,7 @@ class SHash : public TRAITS // Utility function to find the first element with the given key in // the hash table. - static const element_t* Lookup(PTR_element_t table, count_t tableSize, key_t key); + static element_t* Lookup(PTR_element_t table, count_t tableSize, key_t key); // Utility function to remove the first element with the given key // in the hash table. diff --git a/src/coreclr/nativeaot/Runtime/shash.inl b/src/coreclr/nativeaot/Runtime/shash.inl index 8e2d5af49b593f..6bf17972163057 100644 --- a/src/coreclr/nativeaot/Runtime/shash.inl +++ b/src/coreclr/nativeaot/Runtime/shash.inl @@ -48,6 +48,28 @@ const typename SHash< TRAITS>::element_t* SHash::LookupPtr(key_t key) co return Lookup(m_table, m_tableSize, key); } +template +template +bool SHash::Update(key_t key, Functor &&functor) +{ + element_t *element = Lookup(m_table, m_tableSize, key); + if (element == nullptr) + return false; + +#ifdef _DEBUG + PTR_element_t table = m_table; + count_t tableSize = m_tableSize; + count_t count = m_tableCount; +#endif // _DEBUG + + functor(*element); + + ASSERT(m_table == table && m_tableSize == tableSize && m_tableCount == count); + ASSERT(!TRAITS::IsNull(*element) && !TRAITS::IsDeleted(*element)); + ASSERT(TRAITS::Equals(TRAITS::GetKey(*element), key)); + return true; +} + template bool SHash::Add(const element_t &element) { @@ -255,7 +277,7 @@ bool SHash::Reallocate(count_t newTableSize) } template -const typename SHash::element_t * SHash::Lookup(PTR_element_t table, count_t tableSize, key_t key) +typename SHash::element_t * SHash::Lookup(PTR_element_t table, count_t tableSize, key_t key) { if (tableSize == 0) return NULL; @@ -467,4 +489,3 @@ typename SHash::count_t SHash::NextPrime(typename SHash: // restore "Conditional expression is constant" warning to default value #pragma warning(default:4127) - diff --git a/src/coreclr/vm/assemblybinder.cpp b/src/coreclr/vm/assemblybinder.cpp index 26ebe890fe3404..6591db69283de5 100644 --- a/src/coreclr/vm/assemblybinder.cpp +++ b/src/coreclr/vm/assemblybinder.cpp @@ -82,29 +82,27 @@ void AssemblyBinder::DeclareDependencyOnMvid(LPCUTF8 simpleName, GUID mvid, bool addAllLoadedModules = true; } - SimpleNameToExpectedMVIDAndRequiringAssembly* foundElem = (SimpleNameToExpectedMVIDAndRequiringAssembly*)m_assemblySimpleNameMvidCheckHash.LookupPtr(simpleName); - if (foundElem == NULL) - { - SimpleNameToExpectedMVIDAndRequiringAssembly newElem(simpleName, mvid, compositeComponent, imageName); - m_assemblySimpleNameMvidCheckHash.Add(newElem); - } - else - { - // Elem already exists. Determine if the existing elem is another one with the same mvid, in which case just record that a dependency is in play. - // If the existing elem has a different mvid, fail. - if (IsEqualGUID(mvid, foundElem->Mvid)) + if (!m_assemblySimpleNameMvidCheckHash.Update(simpleName, [&](SimpleNameToExpectedMVIDAndRequiringAssembly &entry) { - // Mvid matches exactly. - if (foundElem->AssemblyRequirementName == NULL) + // Elem already exists. Determine if the existing elem is another one with the same mvid, in which case just record that a dependency is in play. + // If the existing elem has a different mvid, fail. + if (IsEqualGUID(mvid, entry.Mvid)) { - foundElem->AssemblyRequirementName = imageName; - foundElem->CompositeComponent = compositeComponent; + // Mvid matches exactly. + if (entry.AssemblyRequirementName == NULL) + { + entry.AssemblyRequirementName = imageName; + entry.CompositeComponent = compositeComponent; + } } - } - else - { - MvidMismatchFatalError(foundElem->Mvid, mvid, simpleName, compositeComponent, imageName); - } + else + { + MvidMismatchFatalError(entry.Mvid, mvid, simpleName, compositeComponent, imageName); + } + })) + { + SimpleNameToExpectedMVIDAndRequiringAssembly newElem(simpleName, mvid, compositeComponent, imageName); + m_assemblySimpleNameMvidCheckHash.Add(newElem); } if (addAllLoadedModules) @@ -127,31 +125,29 @@ void AssemblyBinder::DeclareLoadedAssembly(Assembly* loadedAssembly) LPCUTF8 simpleName = loadedAssembly->GetSimpleName(); - SimpleNameToExpectedMVIDAndRequiringAssembly* foundElem = (SimpleNameToExpectedMVIDAndRequiringAssembly*)m_assemblySimpleNameMvidCheckHash.LookupPtr(simpleName); - if (foundElem == NULL) + if (!m_assemblySimpleNameMvidCheckHash.Update(simpleName, [&](SimpleNameToExpectedMVIDAndRequiringAssembly &entry) + { + // Elem already exists. Determine if the existing elem is another one with the same mvid, in which case do nothing. Everything is fine here. + // If the existing elem has a different mvid, but isn't a dependency on exact mvid elem, then set the mvid to all 0. + // If the existing elem has a different mvid, and is a dependency on exact mvid elem, then we've hit a fatal error. + if (IsEqualGUID(mvid, entry.Mvid)) + { + // Mvid matches exactly. + } + else if (entry.AssemblyRequirementName == NULL) + { + // Another loaded assembly, set the stored Mvid to all zeroes to indicate that it isn't a unique mvid + memset(&entry.Mvid, 0, sizeof(GUID)); + } + else + { + MvidMismatchFatalError(mvid, entry.Mvid, simpleName, entry.CompositeComponent, entry.AssemblyRequirementName); + } + })) { SimpleNameToExpectedMVIDAndRequiringAssembly newElem(simpleName, mvid, false, NULL); m_assemblySimpleNameMvidCheckHash.Add(newElem); } - else - { - // Elem already exists. Determine if the existing elem is another one with the same mvid, in which case do nothing. Everything is fine here. - // If the existing elem has a different mvid, but isn't a dependency on exact mvid elem, then set the mvid to all 0. - // If the existing elem has a different mvid, and is a dependency on exact mvid elem, then we've hit a fatal error. - if (IsEqualGUID(mvid, foundElem->Mvid)) - { - // Mvid matches exactly. - } - else if (foundElem->AssemblyRequirementName == NULL) - { - // Another loaded assembly, set the stored Mvid to all zeroes to indicate that it isn't a unique mvid - memset(&foundElem->Mvid, 0, sizeof(GUID)); - } - else - { - MvidMismatchFatalError(mvid, foundElem->Mvid, simpleName, foundElem->CompositeComponent, foundElem->AssemblyRequirementName); - } - } } #endif // FEATURE_READYTORUN diff --git a/src/coreclr/vm/callcounting.cpp b/src/coreclr/vm/callcounting.cpp index ada6cf272930ad..09947dc99b50a1 100644 --- a/src/coreclr/vm/callcounting.cpp +++ b/src/coreclr/vm/callcounting.cpp @@ -1018,11 +1018,11 @@ void CallCountingManager::DeleteAllCallCountingStubs() // Currently, tier 0 is the last code version that is counted, and the method is typically not counted anymore. // Remove the forwarder stub if one exists, a new one will be created if necessary, for example, if a profiler adds // an IL code version for the method. - Precode *const *forwarderStubPtr = - methodDescForwarderStubHash.LookupPtr(callCountingInfo->GetCodeVersion().GetMethodDesc()); - if (forwarderStubPtr != nullptr) + MethodDesc *methodDesc = callCountingInfo->GetCodeVersion().GetMethodDesc(); + MethodDescForwarderStubHash::KeyIterator forwarderStub = methodDescForwarderStubHash.Begin(methodDesc); + if (forwarderStub != methodDescForwarderStubHash.End(methodDesc)) { - methodDescForwarderStubHash.RemovePtr(const_cast(forwarderStubPtr)); + methodDescForwarderStubHash.Remove(forwarderStub); } callCountingInfoByCodeVersionHash.Remove(it); diff --git a/src/coreclr/vm/crossloaderallocatorhash.inl b/src/coreclr/vm/crossloaderallocatorhash.inl index 35344cfba9ce72..0b468a1c509755 100644 --- a/src/coreclr/vm/crossloaderallocatorhash.inl +++ b/src/coreclr/vm/crossloaderallocatorhash.inl @@ -428,7 +428,12 @@ void CrossLoaderAllocatorHash::Add(TKey key, TValue value, LoaderAllocat else { hashKeyEntry = keyValueStore; - keyToTrackersHash.ReplacePtr(hashKeyEntryPtr, hashKeyEntry); + bool updated = keyToTrackersHash.Update(key, [&](KeyValueStoreOrLAHashKeyToTrackers *&entry) + { + KeyToValuesHashTraits::OnRemovePerEntryCleanupAction(entry); + entry = hashKeyEntry; + }); + _ASSERTE(updated); } keyValueStoreHolder.SuppressRelease(); @@ -447,7 +452,11 @@ void CrossLoaderAllocatorHash::Add(TKey key, TValue value, LoaderAllocat // LAHashKeyToTrackers, so replace the element without cleanup. hashKeyToTrackers = new LAHashKeyToTrackers(keyValueStore); hashKeyEntry = hashKeyToTrackers; - keyToTrackersHash.ReplacePtr(hashKeyEntryPtr, hashKeyEntry, false /* invokeCleanupAction */); + bool updated = keyToTrackersHash.Update(key, [&](KeyValueStoreOrLAHashKeyToTrackers *&entry) + { + entry = hashKeyEntry; + }); + _ASSERTE(updated); } // Must add it to the cross LA structure @@ -462,7 +471,12 @@ void CrossLoaderAllocatorHash::Add(TKey key, TValue value, LoaderAllocat if (KeyToValuesHashTraits::AddToValuesInHeapMemory(&keyValueStore, keyValueStoreHolder, key, value)) { - keyToValuePerLAHash->ReplacePtr(hashKeyEntryInPerLAHashPtr, keyValueStore); + bool updated = keyToValuePerLAHash->Update(key, [&](KeyValueStoreOrLAHashKeyToTrackers *&entry) + { + KeyToValuesHashTraits::OnRemovePerEntryCleanupAction(entry); + entry = keyValueStore; + }); + _ASSERTE(updated); keyValueStoreHolder.SuppressRelease(); } } @@ -656,14 +670,14 @@ void CrossLoaderAllocatorHash::RemoveAll(TKey key) _ASSERTE(TRAITS::GetLoaderAllocator(key) == m_pLoaderAllocator); KeyToValuesHash &keyToTrackersHash = m_keyToDependentTrackersHash; - KeyValueStoreOrLAHashKeyToTrackers *const *hashKeyEntryPtr = keyToTrackersHash.LookupPtr(key); - if (hashKeyEntryPtr == NULL) + typename KeyToValuesHash::KeyIterator hashKeyEntryIterator = keyToTrackersHash.Begin(key); + if (hashKeyEntryIterator == keyToTrackersHash.End(key)) { return; } // We have an entry in the hashtable for the key/dependenthandle. - KeyValueStoreOrLAHashKeyToTrackers *hashKeyEntry = *hashKeyEntryPtr; + KeyValueStoreOrLAHashKeyToTrackers *hashKeyEntry = *hashKeyEntryIterator; LAHashKeyToTrackers *hashKeyToTrackers = NULL; KeyValueStore *keyValueStore; @@ -705,7 +719,7 @@ void CrossLoaderAllocatorHash::RemoveAll(TKey key) } // Remove entry from key to tracker hash - keyToTrackersHash.RemovePtr(const_cast(hashKeyEntryPtr)); + keyToTrackersHash.Remove(hashKeyEntryIterator); } #endif // !DACCESS_COMPILE diff --git a/src/coreclr/vm/eetoprofinterfaceimpl.cpp b/src/coreclr/vm/eetoprofinterfaceimpl.cpp index 8803987b83d6e3..85680528c9e741 100644 --- a/src/coreclr/vm/eetoprofinterfaceimpl.cpp +++ b/src/coreclr/vm/eetoprofinterfaceimpl.cpp @@ -1427,14 +1427,7 @@ HRESULT EEToProfInterfaceImpl::AllocByClass(ObjectID objId, ClassID clsId, void _ASSERTE(dwProcId == GetCurrentProcessId()); - const AllocByClassEntry *entry = pData->hashTable.LookupPtr(clsId); - if (entry != nullptr) - { - AllocByClassEntry updated = *entry; - updated.m_count++; - pData->hashTable.ReplacePtr(entry, updated, false /* invokeCleanupAction */); - } - else + if (!pData->hashTable.Update(clsId, [](AllocByClassEntry &entry) { entry.m_count++; })) { AllocByClassEntry newEntry = { clsId, 1 }; if (!pData->hashTable.AddNoThrow(newEntry)) diff --git a/src/coreclr/vm/inlinetracking.cpp b/src/coreclr/vm/inlinetracking.cpp index 4b8e8a9467798c..4fd0e8b9de1e94 100644 --- a/src/coreclr/vm/inlinetracking.cpp +++ b/src/coreclr/vm/inlinetracking.cpp @@ -144,13 +144,7 @@ void InlineTrackingMap::AddInlining(MethodDesc *inliner, MethodDesc *inlinee) } CrstHolder lock(&m_mapCrst); - InlineTrackingEntry *existingEntry = const_cast(LookupPtr(inlineeMnM)); - if (existingEntry) - { - // We saw this inlinee before, just add one more inliner - existingEntry->Add(inliner); - } - else + if (!Update(inlineeMnM, [&](InlineTrackingEntry &entry) { entry.Add(inliner); })) { // We haven't seen this inlinee before, create a new record in the hashtable // and add a first inliner to it. From 1fe605a81bb22859b06bfd91e62ba0d911e2c2bb Mon Sep 17 00:00:00 2001 From: Aaron Robinson Date: Fri, 23 Oct 2026 11:48:33 -0700 Subject: [PATCH 12/13] Remove remaining legacy debugger hash types Use typed patch slots and value initialization; clarify borrowed lookup lifetimes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ce4b0890-6559-4b90-b46f-11179e70f973 --- src/coreclr/debug/daccess/dacfn.cpp | 6 +- src/coreclr/debug/di/rsclass.cpp | 2 +- src/coreclr/debug/ee/controller.cpp | 104 ++++++++++++-------------- src/coreclr/debug/ee/controller.h | 42 +++++------ src/coreclr/debug/ee/debugger.cpp | 6 +- src/coreclr/inc/shash.h | 3 +- src/coreclr/inc/utilcode.h | 28 ------- src/coreclr/nativeaot/Runtime/shash.h | 3 +- 8 files changed, 81 insertions(+), 113 deletions(-) diff --git a/src/coreclr/debug/daccess/dacfn.cpp b/src/coreclr/debug/daccess/dacfn.cpp index 822d99080a6386..d31729d074a4b8 100644 --- a/src/coreclr/debug/daccess/dacfn.cpp +++ b/src/coreclr/debug/daccess/dacfn.cpp @@ -1397,10 +1397,10 @@ void DacPatchCache::Populate() // Clear any stale entries from previous failed population attempts m_entries.Clear(); - HASHFIND info; + ULONG patchIndex; DebuggerPatchTable * pTable = DebuggerController::GetPatchTable(); - DebuggerControllerPatch * pPatch = pTable->GetFirstPatch(&info); + DebuggerControllerPatch * pPatch = pTable->GetFirstPatch(&patchIndex); while (pPatch != NULL) { @@ -1414,7 +1414,7 @@ void DacPatchCache::Populate() m_entries.Append(entry); } - pPatch = pTable->GetNextPatch(&info); + pPatch = pTable->GetNextPatch(&patchIndex); } m_isPopulated = true; diff --git a/src/coreclr/debug/di/rsclass.cpp b/src/coreclr/debug/di/rsclass.cpp index 80e103a2527069..78020d982194ed 100644 --- a/src/coreclr/debug/di/rsclass.cpp +++ b/src/coreclr/debug/di/rsclass.cpp @@ -1007,7 +1007,7 @@ const FieldData * CordbClass::GetEnCFieldFromDac(BOOL fStatic, IfFailThrow(pProcess->GetDAC()->GetEnCHangingFieldInfo(&encField, &fieldData)); _ASSERTE((fStatic != 0) == fieldData.m_fFldIsStatic); - // Save the field results in our cache and get a stable pointer to the data + // Cache the field data and return a borrowed pointer that may be invalidated by subsequent cache mutations. if (fStatic) { pInfo = m_hangingFieldsStatic.AddFieldInfo(&fieldData); diff --git a/src/coreclr/debug/ee/controller.cpp b/src/coreclr/debug/ee/controller.cpp index a5d596deaf5303..0b82e2bc1a7a7d 100644 --- a/src/coreclr/debug/ee/controller.cpp +++ b/src/coreclr/debug/ee/controller.cpp @@ -280,9 +280,6 @@ SharedPatchBypassBuffer* DebuggerControllerPatch::GetOrCreateSharedPatchBypassBu #ifndef DACCESS_COMPILE static_assert(std::is_trivially_copyable::value); -static_assert(offsetof(DebuggerControllerPatch, entry) == 0); -static_assert(offsetof(FREEHASHENTRY, iPrev) == offsetof(HASHENTRY, iPrev)); -static_assert(offsetof(FREEHASHENTRY, iNext) == offsetof(HASHENTRY, iNext)); DebuggerPatchTable::~DebuggerPatchTable() { @@ -322,10 +319,10 @@ DebuggerControllerPatch *DebuggerPatchTable::AddPatch(DebuggerPatchKey key) m_pcEntries = reinterpret_cast(memory); for (ULONG index = m_iEntries; index < count.Value(); index++) { - DebuggerControllerPatch *patch = reinterpret_cast(EntryPtr(index)); - patch->entry.iFree = index + 1; + DebuggerControllerPatch *patch = EntryPtr(index); + patch->iFree = index + 1; } - reinterpret_cast(EntryPtr(count.Value() - 1))->entry.iFree = DPT_INVALID_SLOT; + EntryPtr(count.Value() - 1)->iFree = DPT_INVALID_SLOT; m_iFree = m_iEntries; m_iEntries = count.Value(); } @@ -333,13 +330,13 @@ DebuggerControllerPatch *DebuggerPatchTable::AddPatch(DebuggerPatchKey key) // Allocate the index before taking a slot so OOM leaves the table unchanged. EnsureList(key); ULONG index = m_iFree; - DebuggerControllerPatch *patch = reinterpret_cast(EntryPtr(index)); - m_iFree = patch->entry.iFree; - memset(patch, 0, sizeof(*patch)); - patch->entry.iPrev = DPT_INVALID_SLOT; + DebuggerControllerPatch *patch = EntryPtr(index); + m_iFree = patch->iFree; + *patch = {}; + patch->iPrev = DPT_INVALID_SLOT; bool updated = m_patchLists.Update(key, [&](DebuggerPatchListEntry &list) { - patch->entry.iNext = list.first; + patch->iNext = list.first; if (list.first != DPT_INVALID_SLOT) EntryPtr(list.first)->iPrev = index; list.first = index; @@ -350,11 +347,11 @@ DebuggerControllerPatch *DebuggerPatchTable::AddPatch(DebuggerPatchKey key) void DebuggerPatchTable::LinkPatch(DebuggerControllerPatch *patch) { - ULONG index = ItemIndex(reinterpret_cast(patch)); - patch->entry.iPrev = DPT_INVALID_SLOT; + ULONG index = ItemIndex(patch); + patch->iPrev = DPT_INVALID_SLOT; bool updated = m_patchLists.Update(Key(patch), [&](DebuggerPatchListEntry &list) { - patch->entry.iNext = list.first; + patch->iNext = list.first; if (list.first != DPT_INVALID_SLOT) EntryPtr(list.first)->iPrev = index; list.first = index; @@ -430,12 +427,12 @@ void DebuggerPatchTable::SpliceOutOfList(DebuggerControllerPatch *patch) _ASSERTE(m_patchLists.LookupPtr(key) != nullptr); // Fix up pointers to chain - if (patch->entry.iPrev == DPT_INVALID_SLOT) + if (patch->iPrev == DPT_INVALID_SLOT) { // The first patch shouldn't have anything behind it. - _ASSERTE(patch->entry.iPrev == DPT_INVALID_SLOT); + _ASSERTE(patch->iPrev == DPT_INVALID_SLOT); - if (patch->entry.iNext == DPT_INVALID_SLOT) + if (patch->iNext == DPT_INVALID_SLOT) { m_patchLists.Remove(key); } @@ -443,39 +440,39 @@ void DebuggerPatchTable::SpliceOutOfList(DebuggerControllerPatch *patch) { bool updated = m_patchLists.Update(key, [&](DebuggerPatchListEntry &entry) { - entry.first = patch->entry.iNext; + entry.first = patch->iNext; }); _ASSERTE(updated); } } - if (patch->entry.iNext != DPT_INVALID_SLOT) + if (patch->iNext != DPT_INVALID_SLOT) { - EntryPtr(patch->entry.iNext)->iPrev = patch->entry.iPrev; + EntryPtr(patch->iNext)->iPrev = patch->iPrev; } - if (patch->entry.iPrev != DPT_INVALID_SLOT) + if (patch->iPrev != DPT_INVALID_SLOT) { - EntryPtr(patch->entry.iPrev)->iNext = patch->entry.iNext; + EntryPtr(patch->iPrev)->iNext = patch->iNext; } - patch->entry.iNext = DPT_INVALID_SLOT; - patch->entry.iPrev = DPT_INVALID_SLOT; + patch->iNext = DPT_INVALID_SLOT; + patch->iPrev = DPT_INVALID_SLOT; } void DebuggerPatchTable::SpliceInBackOf(DebuggerControllerPatch *patchAppend, DebuggerControllerPatch *patchEnd) { - ULONG iAppend = ItemIndex((HASHENTRY*)patchAppend); - ULONG iEnd = ItemIndex((HASHENTRY*)patchEnd); + ULONG iAppend = ItemIndex(patchAppend); + ULONG iEnd = ItemIndex(patchEnd); - patchAppend->entry.iPrev = iEnd; - patchAppend->entry.iNext = patchEnd->entry.iNext; + patchAppend->iPrev = iEnd; + patchAppend->iNext = patchEnd->iNext; - if (patchAppend->entry.iNext != DPT_INVALID_SLOT) - EntryPtr(patchAppend->entry.iNext)->iPrev = iAppend; + if (patchAppend->iNext != DPT_INVALID_SLOT) + EntryPtr(patchAppend->iNext)->iPrev = iAppend; - patchEnd->entry.iNext = iAppend; + patchEnd->iNext = iAppend; } #endif @@ -956,15 +953,15 @@ void DebuggerPatchTable::RemovePatch(DebuggerControllerPatch *patch) SpliceOutOfList(patch); patch->refCount = 0; - patch->entry.iFree = m_iFree; - m_iFree = ItemIndex(reinterpret_cast(patch)); + patch->iFree = m_iFree; + m_iFree = ItemIndex(patch); } DebuggerControllerPatch *DebuggerPatchTable::GetNextPatch(DebuggerControllerPatch *prev) { - ULONG next = prev->entry.iNext; + ULONG next = prev->iNext; return next == DPT_INVALID_SLOT ? nullptr - : dac_cast(EntryPtr(next)); + : EntryPtr(next); } // Count how many patches are in the table. @@ -1291,10 +1288,10 @@ void DebuggerController::DisableAll() // if (!IsAtProcessExit()) { - HASHFIND f; - for (DebuggerControllerPatch *patch = g_patches->GetFirstPatch(&f); + ULONG patchIndex; + for (DebuggerControllerPatch *patch = g_patches->GetFirstPatch(&patchIndex); patch != NULL; - patch = g_patches->GetNextPatch(&f)) + patch = g_patches->GetNextPatch(&patchIndex)) { if (patch->controller == this) { @@ -2173,7 +2170,7 @@ BOOL DebuggerController::AddILPatch(AppDomain * pAppDomain, Module *module, // We have to keep the index here instead of the pointer. The loop below adds more patches, // which may cause the patch table to grow and move. - ULONG primaryIndex = g_patches->GetItemIndex((HASHENTRY*)primary); + ULONG primaryIndex = g_patches->GetItemIndex(primary); // Iterate through every existing NativeCodeBlob (with the same EnC version). // This includes generics + prejitted code. @@ -2212,7 +2209,7 @@ BOOL DebuggerController::AddILPatch(AppDomain * pAppDomain, Module *module, { fVersionMatch = TRUE; - primary = (DebuggerControllerPatch *)g_patches->GetEntryPtr(primaryIndex); + primary = g_patches->GetEntryPtr(primaryIndex); // If we're missing JIT info for any then // we won't have applied the bp to every instantiation. That should probably be reported @@ -2398,10 +2395,10 @@ void DebuggerController::RemovePatchesFromModule(Module *pModule, AppDomain *pAp // First find all patches of interest DebuggerController::ControllerLockHolder ch; - HASHFIND f; - for (DebuggerControllerPatch *patch = g_patches->GetFirstPatch(&f); + ULONG patchIndex; + for (DebuggerControllerPatch *patch = g_patches->GetFirstPatch(&patchIndex); patch != NULL; - patch = g_patches->GetNextPatch(&f)) + patch = g_patches->GetNextPatch(&patchIndex)) { // Skip patches not in the specified domain if ((pAppDomain != NULL) && (patch->pAppDomain != pAppDomain)) @@ -2456,10 +2453,10 @@ bool DebuggerController::ModuleHasPatches( Module* pModule ) } // First find all patches of interest - HASHFIND f; - for (DebuggerControllerPatch *patch = g_patches->GetFirstPatch(&f); + ULONG patchIndex; + for (DebuggerControllerPatch *patch = g_patches->GetFirstPatch(&patchIndex); patch != NULL; - patch = g_patches->GetNextPatch(&f)) + patch = g_patches->GetNextPatch(&patchIndex)) { // // This mirrors logic in code:DebuggerController::RemovePatchesFromModule @@ -2906,11 +2903,11 @@ DPOSS_ACTION DebuggerController::ScanForTriggers(CORDB_ADDRESS_TYPE *address, // Annoyingly, TriggerPatch may add patches, which may cause // the patch table to move, which may, in turn, invalidate // the patch (and patchNext) pointers. Store indices, instead. - iEvent = g_patches->GetItemIndex( (HASHENTRY *)patch ); + iEvent = g_patches->GetItemIndex(patch); if (patchNext != NULL) { - iEventNext = g_patches->GetItemIndex((HASHENTRY *)patchNext); + iEventNext = g_patches->GetItemIndex(patchNext); } if (MatchPatch(thread, context, patch)) @@ -2945,16 +2942,14 @@ DPOSS_ACTION DebuggerController::ScanForTriggers(CORDB_ADDRESS_TYPE *address, tpr == TPR_TRIGGER_ONLY_THIS_AND_LOOP) { // Make sure we've still got a valid pointer. - patch = (DebuggerControllerPatch *) - DebuggerController::g_patches->GetEntryPtr( iEvent ); + patch = DebuggerController::g_patches->GetEntryPtr(iEvent); pDcq->dcqEnqueue(patch->controller, TRUE); // @todo Return value } // Make sure we've got a valid pointer in case TriggerPatch // returned false but still caused the table to move. - patch = (DebuggerControllerPatch *) - g_patches->GetEntryPtr( iEvent ); + patch = g_patches->GetEntryPtr(iEvent); // A patch can be deleted as a result of it's being triggered. // The actual deletion of the patch is delayed until after the @@ -2965,7 +2960,7 @@ DPOSS_ACTION DebuggerController::ScanForTriggers(CORDB_ADDRESS_TYPE *address, // "deleted" flag didn't get set when we iterate the patches in DisableAll(). patchNext = g_patches->GetNextPatch(patch); if (patchNext != NULL) - iEventNext = g_patches->GetItemIndex((HASHENTRY *)patchNext); + iEventNext = g_patches->GetItemIndex(patchNext); // Note that ReleasePatch() actually removes the patch if its ref count // reaches 0 after the release. @@ -2986,8 +2981,7 @@ DPOSS_ACTION DebuggerController::ScanForTriggers(CORDB_ADDRESS_TYPE *address, } else if (patchNext != NULL) { - patch = (DebuggerControllerPatch *) - g_patches->GetEntryPtr(iEventNext); + patch = g_patches->GetEntryPtr(iEventNext); } else { diff --git a/src/coreclr/debug/ee/controller.h b/src/coreclr/debug/ee/controller.h index 3e6ffa04d4f11d..34233c35236a4c 100644 --- a/src/coreclr/debug/ee/controller.h +++ b/src/coreclr/debug/ee/controller.h @@ -363,8 +363,7 @@ enum DebuggerPatchKind { PATCH_KIND_IL_PRIMARY, PATCH_KIND_IL_REPLICA, PATCH_KIN // this should contain all the info that's needed over the course of a // patch's lifetime. // -// FREEHASHENTRY entry: Three ULONGs, this is required -// by the underlying hashtable implementation +// ULONG iPrev, iNext, iFree: Slot indices for the patch chains and free list. // DWORD opcode: A nonzero opcode && address field means that // the patch has been applied to something. This may not be a full // opcode, it is possible it is a partial opcode. @@ -419,7 +418,9 @@ struct DebuggerControllerPatch friend class DebuggerPatchTable; friend class DebuggerController; - FREEHASHENTRY entry; + ULONG iPrev; + ULONG iNext; + ULONG iFree; DebuggerController *controller; DebuggerFunctionKey key; SIZE_T offset; @@ -729,19 +730,19 @@ class DebuggerPatchTable LIMITED_METHOD_DAC_CONTRACT; const DebuggerPatchListEntry *entry = m_patchLists.LookupPtr(key); return entry == nullptr || entry->first == DPT_INVALID_SLOT ? nullptr - : dac_cast(EntryPtr(entry->first)); + : EntryPtr(entry->first); } - HASHENTRY *EntryPtr(ULONG index) + DebuggerControllerPatch *EntryPtr(ULONG index) { LIMITED_METHOD_DAC_CONTRACT; - return dac_cast(m_pcEntries + index * sizeof(DebuggerControllerPatch)); + return dac_cast(m_pcEntries + index * sizeof(DebuggerControllerPatch)); } - ULONG ItemIndex(HASHENTRY *entry) + ULONG ItemIndex(DebuggerControllerPatch *patch) { LIMITED_METHOD_DAC_CONTRACT; - return static_cast((dac_cast(entry) - m_pcEntries) / sizeof(DebuggerControllerPatch)); + return static_cast((dac_cast(patch) - m_pcEntries) / sizeof(DebuggerControllerPatch)); } #ifndef DACCESS_COMPILE @@ -855,33 +856,32 @@ class DebuggerPatchTable DebuggerControllerPatch *GetNextPatch(DebuggerControllerPatch *prev); // Find the first patch in the patch table, and store - // index info in info. Along with GetNextPatch, this can - // iterate through the whole patch table. Note that since the - // table operates by iterating over its slots. If you add an entry while iterating + // the next slot index in pIndex. Along with GetNextPatch, this can + // iterate through the whole patch table by scanning its slots. + // If you add an entry while iterating // through the table, you may or may not iterate across // the new entries. You will iterate through all the entries // that were present at the beginning of the run. You // safely delete anything you've already iterated by, anything // else is kinda risky. - DebuggerControllerPatch * GetFirstPatch(HASHFIND * pInfo) + DebuggerControllerPatch *GetFirstPatch(ULONG *pIndex) { SUPPORTS_DAC; - pInfo->iNext = 0; - return GetNextPatch(pInfo); + *pIndex = 0; + return GetNextPatch(pIndex); } // Along with GetFirstPatch, this can iterate through // the whole patch table. See GetFirstPatch for more info // on the rules of iterating through the table. - DebuggerControllerPatch * GetNextPatch(HASHFIND * pInfo) + DebuggerControllerPatch *GetNextPatch(ULONG *pIndex) { SUPPORTS_DAC; - while (pInfo->iNext < m_iEntries) + while (*pIndex < m_iEntries) { - DebuggerControllerPatch *patch = - dac_cast(EntryPtr(pInfo->iNext++)); + DebuggerControllerPatch *patch = EntryPtr((*pIndex)++); if (!patch->IsFree()) return patch; } @@ -890,7 +890,7 @@ class DebuggerPatchTable // Used by DebuggerController to translate an index // of a patch into a direct pointer. - inline HASHENTRY * GetEntryPtr(ULONG iEntry) + inline DebuggerControllerPatch *GetEntryPtr(ULONG iEntry) { SUPPORTS_DAC; @@ -899,11 +899,11 @@ class DebuggerPatchTable // Used by DebuggerController to grab indices of patches // rather than holding direct pointers to them. - inline ULONG GetItemIndex(HASHENTRY * p) + inline ULONG GetItemIndex(DebuggerControllerPatch *patch) { SUPPORTS_DAC; - return ItemIndex(p); + return ItemIndex(patch); } // Count how many patches are in the table. diff --git a/src/coreclr/debug/ee/debugger.cpp b/src/coreclr/debug/ee/debugger.cpp index d10e15791a0659..5d2fe628cbe23b 100644 --- a/src/coreclr/debug/ee/debugger.cpp +++ b/src/coreclr/debug/ee/debugger.cpp @@ -4651,7 +4651,7 @@ HRESULT Debugger::MapAndBindFunctionPatches(DebuggerJitInfo *djiNew, CONTRACTL_END; HRESULT hr = S_OK; - HASHFIND hf; + ULONG patchIndex; SIZE_T *pidTableEntry = NULL; SIZE_T pidInCaseTableMoves; Module *pModule = g_pEEInterface->MethodDescGetModule(fd); @@ -4676,9 +4676,9 @@ HRESULT Debugger::MapAndBindFunctionPatches(DebuggerJitInfo *djiNew, DebuggerPatchTable *pPatchTable = DebuggerController::GetPatchTable(); GetBPMappingDuplicates()->Clear(); //dups are tracked per-version - for (DebuggerControllerPatch *dcp = pPatchTable->GetFirstPatch(&hf); + for (DebuggerControllerPatch *dcp = pPatchTable->GetFirstPatch(&patchIndex); dcp != NULL; - dcp = pPatchTable->GetNextPatch( &hf )) + dcp = pPatchTable->GetNextPatch(&patchIndex)) { LOG((LF_CORDB, LL_INFO10000, "D::MABFP: Got patch %p, patchId:0x%zx\n", dcp, dcp->patchId)); diff --git a/src/coreclr/inc/shash.h b/src/coreclr/inc/shash.h index 9870f9319e4285..6cdb51e620d896 100644 --- a/src/coreclr/inc/shash.h +++ b/src/coreclr/inc/shash.h @@ -196,7 +196,8 @@ class EMPTY_BASES SHash : public TRAITS element_t Lookup(key_t key) const; // Read-only pointer-based lookup. Use Update to modify an existing element. - + // The returned pointer refers to table storage and is invalidated by reallocation, + // removal of the element, or clearing the table. const element_t* LookupPtr(key_t key) const; // Update the first matching element in place, returning false if no match exists. diff --git a/src/coreclr/inc/utilcode.h b/src/coreclr/inc/utilcode.h index ddd516e2ef7aff..33c46d83653bc5 100644 --- a/src/coreclr/inc/utilcode.h +++ b/src/coreclr/inc/utilcode.h @@ -1336,34 +1336,6 @@ template class CBinarySearch } }; -//***************************************************************************** -// The information that the hash table implementation stores at the beginning -// of every record that can be but in the hash table. -//***************************************************************************** -typedef DPTR(struct HASHENTRY) PTR_HASHENTRY; -struct HASHENTRY -{ - ULONG iPrev; // Previous bucket in the chain. - ULONG iNext; // Next bucket in the chain. -}; - -typedef DPTR(struct FREEHASHENTRY) PTR_FREEHASHENTRY; -struct FREEHASHENTRY : HASHENTRY -{ - ULONG iFree; -}; - -//***************************************************************************** -// Used by the FindFirst/FindNextEntry functions. These api's allow you to -// do a sequential scan of all entries. -//***************************************************************************** -struct HASHFIND -{ - ULONG iBucket; // The next bucket to look in. - ULONG iNext; -}; - - //***************************************************************************** //***************************************************************************** diff --git a/src/coreclr/nativeaot/Runtime/shash.h b/src/coreclr/nativeaot/Runtime/shash.h index c36deda34f4fa7..1cb09082d67d25 100644 --- a/src/coreclr/nativeaot/Runtime/shash.h +++ b/src/coreclr/nativeaot/Runtime/shash.h @@ -152,7 +152,8 @@ class SHash : public TRAITS element_t Lookup(key_t key) const; // Read-only pointer-based lookup. Use Update to modify an existing element. - + // The returned pointer refers to table storage and is invalidated by reallocation, + // removal of the element, or clearing the table. const element_t* LookupPtr(key_t key) const; // Update the first matching element in place, returning false if no match exists. From 48b9dd9a6a641ea442ce0c254c92db1905515b53 Mon Sep 17 00:00:00 2001 From: Aaron Robinson Date: Fri, 23 Oct 2026 11:49:17 -0700 Subject: [PATCH 13/13] Use Add for new GC stress callsites Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ce4b0890-6559-4b90-b46f-11179e70f973 --- src/coreclr/nativeaot/Runtime/GcStressControl.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/nativeaot/Runtime/GcStressControl.cpp b/src/coreclr/nativeaot/Runtime/GcStressControl.cpp index c729e6626fc360..25e968f043113d 100644 --- a/src/coreclr/nativeaot/Runtime/GcStressControl.cpp +++ b/src/coreclr/nativeaot/Runtime/GcStressControl.cpp @@ -106,7 +106,7 @@ class GcStressControl if (!found) { CallsiteCountEntry e = {CallsiteIP, 1, 1, ht}; - s_callsites.AddOrReplace(e); + s_callsites.Add(e); } return !found;