Replace CoreCLR SList with NativeAOT-style design

[max-charlamb]

Note

This PR was authored with the assistance of GitHub Copilot.

Summary

Unify the CoreCLR and NativeAOT SList implementations into a single shared header at src/coreclr/inc/slist.h. NativeAOT's slist.h becomes a redirect to this shared header. The old sentinel-based SLink/SList design is replaced with the NativeAOT-style design: no sentinel node, NULL means empty, traits-based next-pointer access.

Design

SListTraits and SListMode

A single SListTraits<T, Mode, FailFastPolicy> template replaces the old DefaultSListTraits/DefaultSListTraitsWithTail classes. The SListMode enum controls which operations are permitted:

Mode	Allowed Operations	Use Case
Thin (default)	InsertHead, RemoveHead, RemoveFirst, FindAndRemove, InsertAfter	Head-only lists (CacheLine, ConnectionCookie, SyncBlock, etc.)
Tail	+ InsertTail, GetTail	Tail-tracking lists (Thread, CallCountingManager, etc.)
Interlocked	InsertHeadInterlocked only (non-atomic mutations disabled)	Lock-free add-only lists (OsModuleEntry, TypeManagerEntry)

Invalid combinations (e.g., calling InsertHead on an Interlocked list) are caught at compile time via static_assert.

Convenience Aliases

template <typename T> using SListThin = SList<T, SListTraits<T, SListMode::Thin>>;
template <typename T> using SListTail = SList<T, SListTraits<T, SListMode::Tail>>;
template <typename T> using SListInterlocked = SList<T, SListTraits<T, SListMode::Interlocked>>;

Other Features

• Pointer-to-pointer Iterator with begin()/end(), Insert/Remove at position, debug validation
• InsertHeadInterlocked via CAS (PalInterlockedCompareExchangePointer on NativeAOT, InterlockedCompareExchangeT on CoreCLR), implemented in slist.inl
• SListElem<T> wrapper for non-intrusive list usage
• m_pHead is volatile in non-DAC builds for correct iteration concurrent with interlocked inserts
• SListTailBase conditional storage — m_pTail only exists when HasTail is true

Consumer Migrations

All method names preserved: InsertHead, InsertTail, RemoveHead, FindAndRemove, GetHead, GetNext, IsEmpty.

Consumer	m_pNext type	List type
CacheLine	DPTR(CacheLine)	SList<CacheLine>
CallCountingManager	DPTR(CallCountingManager)	SListTail<CallCountingManager>
ConnectionCookie	DPTR(ConnectionCookie)	SList<ConnectionCookie>
StoredProfilerNode	DPTR(StoredProfilerNode)	SList<StoredProfilerNode>
Thread	PTR_Thread	SListTail<Thread>
SyncBlock	PTR_SyncBlock	Raw pointers (free/cleanup lists)
HandleCleanupListItem	DPTR(...)	SListTail<HandleCleanupListItem>
FailedTypeInitCleanupListItem	DPTR(...)	SListTail<FailedTypeInitCleanupListItem>
SListElem<NativeCodeVersion>	DPTR(SListElem)	SListTail<SListElem<NativeCodeVersion>>
OsModuleEntry (NativeAOT)	PTR_OsModuleEntry volatile	SListInterlocked<OsModuleEntry>
TypeManagerEntry (NativeAOT)	TypeManagerEntry* volatile	SListInterlocked<TypeManagerEntry>

DAC / cDAC Changes

• SyncBlock_1.cs, Thread_1.cs: Removed SLink offset subtraction (links now point directly to objects)
• datadescriptor.inc: Removed SLink type definition, ThreadStore.FirstThreadLink changed from TYPE(SLink) to T_POINTER
• Mock descriptors: Removed dead LinkAddress/CleanupLinkAddress properties

NativeAOT Unification

• nativeaot/Runtime/slist.h redirects to ../../inc/slist.h
• nativeaot/Runtime/slist.inl reduced to rh::std::find/count utilities + includes shared ../../inc/slist.inl
• All NativeAOT call sites updated to use unified method names (InsertHead/RemoveHead/InsertHeadInterlocked/begin/end)

[dotnet-policy-service]

Tagging subscribers to this area: @agocke
See info in area-owners.md if you want to be subscribed.

[Copilot]

Pull request overview

Replaces CoreCLR’s sentinel-based SList with a NativeAOT-style, traits-based design (no sentinel; NULL = empty) and migrates affected consumers and DAC/cDAC descriptors to point directly at the owning objects.

Changes:

• Reworked src/coreclr/inc/slist.h to a traits-based intrusive list with optional tail tracking and SListElem<T> for non-intrusive usage.
• Migrated multiple CoreCLR consumers from SLink m_Link to m_pNext (DAC-safe pointer types) and updated list operations accordingly.
• Updated DAC/cDAC contracts, descriptors, and mocks so list heads/links now point directly to objects (no offset subtraction / CONTAINING_RECORD).

Reviewed changes

Copilot reviewed 22 out of 22 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
src/native/managed/cdac/tests/MockDescriptors/MockDescriptors.Thread.cs	Updates mock thread store/list wiring to use direct Thread.Address links.
src/native/managed/cdac/tests/MockDescriptors/MockDescriptors.SyncBlock.cs	Updates mock cleanup list to use direct SyncBlock.Address links.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/Thread_1.cs	Removes SLink offset logic; treats thread links as direct thread pointers.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/SyncBlock_1.cs	Removes link-offsetof subtraction; iterates cleanup list via direct pointers.
src/coreclr/vm/tieredcompilation.h	Introduces OptimizationQueue typedef using tail-enabled SList for optimization queue.
src/coreclr/vm/threads.h	Replaces SLink m_Link with PTR_Thread m_pNext; updates ThreadList and cdac offsets.
src/coreclr/vm/threads.cpp	Updates FindAndRemove usage to new boolean-return API.
src/coreclr/vm/syncblk.h	Replaces SLink usage with PTR_SyncBlock m_pNext; updates cdac offsets and cache list types.
src/coreclr/vm/syncblk.cpp	Removes offsetof(SyncBlock, m_Link) arithmetic; cleanup/free lists now store SyncBlock* directly.
src/coreclr/vm/loaderallocator.hpp	Replaces SLink with m_pNext in cleanup list items; adds tail-enabled list typedefs.
src/coreclr/vm/loaderallocator.cpp	Updates list iteration/removal to match new next-pointer layout.
src/coreclr/vm/datadescriptor/datadescriptor.inc	Removes SLink type and switches FirstThreadLink descriptor to a pointer.
src/coreclr/vm/comconnectionpoints.h	Replaces SLink with m_pNext in ConnectionCookie; updates list typedef.
src/coreclr/vm/comconnectionpoints.cpp	Removes CONTAINING_RECORD/InsertAfter usage; performs direct pointer chaining.
src/coreclr/vm/callcounting.h	Replaces SLink with m_pNext; introduces tail-enabled CallCountingManagerList.
src/coreclr/vm/callcounting.cpp	Updates static list declaration and iteration to new list type/API.
src/coreclr/vm/cachelinealloc.h	Replaces SLink with m_pNext; updates list typedefs and initialization.
src/coreclr/vm/cachelinealloc.cpp	Removes now-unneeded SList::Init() calls in allocator ctor.
src/coreclr/inc/slist.h	Core redesign: traits-based next-pointer access, optional tail tracking, SListElem<T>, and updated iterator/API.
src/coreclr/inc/profilepriv.h	Migrates StoredProfilerNode to m_pNext; updates profiler list typedef.
src/coreclr/debug/daccess/request.cpp	Updates DAC traversal of syncblock lists to use direct m_pNext pointers.
docs/design/datacontracts/SyncBlock.md	Updates docs to reflect cleanup list head now points to SyncBlock objects directly.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[jkotas]

Would it be possible to delete slist.* from under nativeaot and switch naot to this unified one?

[max-charlamb]

Yes, I wanted to make sure it doesn't break the coreclr use cases then I was going to have nativeaot redirect to this.

[max-charlamb]

We can remove this helper and access the element directly.

[max-charlamb]

Yes, I wanted to make sure it doesn't break the coreclr use cases then I was going to have nativeaot redirect to this.

[Copilot]

Pull request overview

Copilot reviewed 24 out of 24 changed files in this pull request and generated 3 comments.

[Copilot]

InsertHeadInterlocked currently calls _SLIST_IS_ALIGNED(&m_pHead, sizeof(void*)) but does not assert on the result. This loses the safety check present in the previous implementation (and turns it into a no-op). Consider changing this to _SLIST_ASSERT(_SLIST_IS_ALIGNED(&m_pHead, sizeof(void*))) so misalignment is caught in debug builds.

Suggested change

	_SLIST_IS_ALIGNED(&m_pHead, sizeof(void*));
	_SLIST_ASSERT(_SLIST_IS_ALIGNED(&m_pHead, sizeof(void*)));

[Copilot]

This comment still refers to the removed SLink type ("pointer within an SLink"), but the implementation now uses SyncBlock::m_pNext directly. Update the wording to avoid referencing SLink so the comment matches the new design.

Suggested change

	// We can't afford to use an SList<> here because we only want to burn
	// space for the minimum, which is the pointer within an SLink.
	// We can't afford extra list-node overhead here, so we use the minimum
	// space required: a single next pointer in the SyncBlock itself.

[Copilot]

Pull request overview

Copilot reviewed 27 out of 27 changed files in this pull request and generated 1 comment.

[Copilot]

ConnectionCookie::m_pNext is never initialized. Because ConnectionPoint::InsertWithLock can call CONNECTIONCOOKIELIST::InsertAfter, the list code asserts that the new item’s next pointer is NULL; with the current user-defined constructor, m_pNext will contain indeterminate data and can trip the assertion (and potentially corrupt the list in retail). Initialize m_pNext to NULL/nullptr in the ConnectionCookie constructor initializer list (or set it explicitly before inserting).

[Copilot]

Pull request overview

Copilot reviewed 30 out of 30 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

src/coreclr/vm/syncblk.h:459

• SyncBlock::m_pNext replaces the old SLink member but is never initialized in SyncBlock(DWORD indx). Newly created SyncBlocks can therefore have a garbage m_pNext value, which can break diagnostics/list traversal (e.g., DAC code in request.cpp checks/traverses m_pNext, and cDAC LinkNext now maps directly to this field). Initialize m_pNext to NULL in the constructor initializer list (and/or otherwise ensure it is cleared for in-use SyncBlocks).

  public:
    // When the SyncBlock is released (we recycle them),
    // the SyncBlockCache maintains free and cleanup lists of SyncBlocks
    // using the m_pNext pointer below.
    // Next pointer for SList linkage.
    PTR_SyncBlock  m_pNext;

  protected:
    // This is the hash code for the object. It can either have been transferred
    // from the header dword, in which case it will be limited to 26 bits, or
    // have been generated right into this member variable here, when it will
    // be a full 32 bits.

    // A 0 in this variable means no hash code has been set yet - this saves having
    // another flag to express this state, and it enables us to use a 32-bit interlocked
    // operation to set the hash code, on the other hand it means that hash codes
    // can never be 0. ObjectNative::GetHashCode in objectnative.cpp makes sure to enforce this.
    DWORD m_dwHashCode;

  public:
    SyncBlock(DWORD indx)
        : m_Lock((OBJECTHANDLE)NULL)
        , m_thinLock()
        , m_dwSyncIndex(indx)
#ifdef FEATURE_METADATA_UPDATER
        , m_pEnCInfo(PTR_NULL)
#endif // FEATURE_METADATA_UPDATER
        , m_dwHashCode(0)
    {
        LIMITED_METHOD_CONTRACT;

        m_pInteropInfo = NULL;
    }

[Copilot]

ConnectionCookie::m_pNext is newly introduced but never initialized by the ConnectionCookie(OBJECTHANDLEHolder) constructor. This can trip the _ASSERTE(*Traits::GetNextPtr(pNewItem) == NULL) in SList::InsertAfter (used in ConnectionPoint::InsertWithLock) and leaves an uninitialized pointer field on newly created cookies. Initialize m_pNext to NULL in the constructor initializer list (and/or ensure it is cleared before insertion).

[Copilot]

For Traits::HasTail == true, the iterator-based mutation APIs (SList::Insert(Iterator&, ...) / SList::Remove(Iterator&)) do not maintain m_pTail. Removing or inserting via these APIs can leave m_pTail stale, breaking subsequent InsertTail() / GetTail() semantics. Either update these APIs to preserve tail correctness (e.g., have Iterator carry a back-pointer to the owning SList and update tail when removing/inserting at the end), or disallow them for tail-tracking lists via static_assert(!Traits::HasTail, ...) and document the restriction.

Suggested change

	{ return it.Insert(pItem); }
	// Removes item pointed to by it. Returns iterator to following item.
	Iterator Remove(Iterator & it)
	{ return it.Remove(); }
	{
	static_assert(!Traits::HasTail,
	"SList::Insert(Iterator&, ...) is not supported for tail-tracking lists because it cannot maintain m_pTail.");
	return it.Insert(pItem);
	}
	// Removes item pointed to by it. Returns iterator to following item.
	Iterator Remove(Iterator & it)
	{
	static_assert(!Traits::HasTail,
	"SList::Remove(Iterator&) is not supported for tail-tracking lists because it cannot maintain m_pTail.");
	return it.Remove();
	}

[Copilot]

Pull request overview

Copilot reviewed 31 out of 31 changed files in this pull request and generated 2 comments.

[Copilot]

SyncBlockCache is documented as having "No constructors/destructors - global instance" (see below in this type), but adding SList<SyncBlock> members makes SyncBlockCache non-trivially constructible due to SList's constructor. This can introduce a global constructor for g_SyncBlockCacheInstance and violates the stated constraint. Consider keeping these as raw pointers (as before) or introducing a trivially-initializable SList variant that can be explicitly initialized from SyncBlockCache::Init() without requiring static initialization.

Suggested change

	SList<SyncBlock> m_CleanupBlockList; // list of sync blocks that need cleanup
	SList<SyncBlock> m_FreeBlockList; // list of free sync blocks
	SyncBlock *m_CleanupBlockList; // head of sync blocks that need cleanup
	SyncBlock *m_FreeBlockList; // head of free sync blocks

[Copilot]

SList::InsertAfter is available for tail-tracking lists (SListMode::Tail), but because it is static it cannot update the list's cached tail pointer. Calling this on a tail list (especially inserting after the current tail) would leave m_pTail stale and break subsequent InsertTail/GetTail behavior. Either disallow InsertAfter when Traits::HasTail (static_assert), or make it an instance method that updates the tail when needed.

[Copilot]

Pull request overview

Copilot reviewed 30 out of 30 changed files in this pull request and generated 1 comment.

[Copilot]

slist.h includes "cdacdata.h", but there is no src/coreclr/inc/cdacdata.h; it currently lives at src/coreclr/vm/cdacdata.h. This will compile in CoreCLR builds only if the VM include directory is on the include path, and is likely to break NativeAOT (which includes this header via a relative path but does not add src/coreclr/vm to include directories). Prefer including the header via a stable relative path (e.g., ../vm/cdacdata.h) or forward-declaring template<typename T> struct cdac_data; to avoid the include-path dependency.

Suggested change

	#include "cdacdata.h"
	#include "../vm/cdacdata.h"

[Copilot]

Pull request overview

Copilot reviewed 31 out of 31 changed files in this pull request and generated 1 comment.

[Copilot]

Comment mentions DefaultSListTraitsWithTail<T> for tail insertion, but the new API surface provides SListTail<T> / SListTraits<T, SListMode::Tail> (and there is no DefaultSListTraitsWithTail in this header). Updating the comment would avoid misleading future callers.

Suggested change

	// field. Use DefaultSListTraitsWithTail<T> for O(1) tail insertion.
	// field. Use SListTraits<T, SListMode::Tail> for O(1) tail insertion.

[github-actions]

🤖 Copilot Code Review — PR #126949

Note

This review was generated by Copilot (Claude Opus 4.6), with additional findings synthesized from a Claude Sonnet 4.5 sub-agent review.

Holistic Assessment

Motivation: Justified — unifying the SList data structure between CoreCLR and NativeAOT reduces duplication and the SListMode enum with static_assert enforcement is a clear improvement for compile-time safety.

Approach: Sound — the traits-based design with SListMode::Thin/Tail/Interlocked is cleaner than the inheritance-based DefaultSListTraits/DefaultSListTraitsWithTail approach. The convenience aliases (SListThin<T>, SListTail<T>, SListInterlocked<T>) make the API ergonomic. Removing method aliases and unifying to lowercase begin()/end() simplifies the API surface.

Summary: ⚠️ Needs Changes. One debug assertion issue with InsertAfter and an uninitialized m_pNext field, plus a stale comment referencing a deleted type. The core design and volatile/CAS changes are correct.

---

Detailed Findings

❌ InsertAfter assertion will fire on uninitialized m_pNext — slist.h:408, comconnectionpoints.h:38-69

The newly added assertion in InsertAfter:

_ASSERTE(*Traits::GetNextPtr(pNewItem) == NULL);

checks that pNewItem->m_pNext is NULL before insertion. However, ConnectionCookie's constructor does not initialize m_pNext:

ConnectionCookie(OBJECTHANDLEHolder hndEventProvObj) : m_hndEventProvObj(std::move(hndEventProvObj))
{  // m_pNext is NOT in the initializer list — indeterminate value for pointer type

In comconnectionpoints.cpp:702, a freshly allocated ConnectionCookie is passed to InsertAfter via the mid-list insertion path of InsertWithLock, where m_pNext has never been set. In debug non-DAC builds, DPTR(ConnectionCookie) is ConnectionCookie* (raw pointer), so m_pNext will have an indeterminate value (likely 0xCDCDCDCD from the debug heap), and the assertion will fire.

Fix: Add m_pNext initialization to ConnectionCookie's constructor:

ConnectionCookie(OBJECTHANDLEHolder hndEventProvObj)
    : m_pNext(NULL), m_hndEventProvObj(std::move(hndEventProvObj))

(Note: The InsertHead path is unaffected because it overwrites m_pNext before use.)

---

💡 Stale comment references deleted type — slist.h:93

// field. Use DefaultSListTraitsWithTail<T> for O(1) tail insertion.

DefaultSListTraitsWithTail<T> was removed in this PR. Should reference the new alias:

// field. Use SListTail<T> for O(1) tail insertion.

---

✅ Volatile correctness — slist.h:111-115, slist.inl:23

Making m_pHead volatile in non-DAC builds is correct. The old code used *reinterpret_cast<T * volatile *>(&m_pHead) only inside InsertHeadInterlocked; the new approach applies the volatile qualifier to the field itself, which is both safer (prevents compiler caching during iteration alongside concurrent interlocked insertions) and cleaner. The InsertHeadInterlocked CAS loop in slist.inl reads m_pHead directly (now a volatile read), stores it in pItem->m_pNext, then copies to expected for the CAS comparison. This is functionally correct — if another thread modifies m_pHead between the read and the CAS, the CAS will fail and the loop retries.

The volatile applies to all SList instances (not just interlocked ones). While this prevents some compiler optimizations for non-concurrent lists, the performance impact is negligible for a linked-list head pointer, and the simpler design avoids conditional-volatile complexity.

---

✅ NativeAOT _ASSERTE availability — slist.h:17-21, rhassert.h:39-41

Verified that every NativeAOT compilation unit that includes slist.h includes rhassert.h first (either directly or transitively via CommonMacros.h or gcenv.h). The #ifndef _ASSERTE guard in rhassert.h:39 ensures stable definition. The removal of the redundant _ASSERTE alias from slist.h's FEATURE_NATIVEAOT block is safe. Similarly, #ifndef guards were removed from LIMITED_METHOD_CONTRACT etc., which is fine since these macros are never defined elsewhere in NativeAOT.

---

✅ cDAC GetThreadFromLink removal — Thread_1.cs:42,97

Correct simplification. The removed helper was a no-op identity function (return threadLink;). Since m_pNext now points directly to Thread objects (no sentinel node indirection), threadStore.FirstThreadLink and thread.LinkNext provide the correct thread pointer directly. Verified via cdac_data<ThreadStore>::FirstThreadLink = offsetof(ThreadStore, m_ThreadList) + offsetof(ThreadList, m_pHead) and cdac_data<Thread>::Link = offsetof(Thread, m_pNext).

---

✅ SyncBlock m_pNext initialization — syncblk.h:454

Adding m_pNext(PTR_NULL) to the constructor initializer list is correct for C++ code (where explicit initialization is required, unlike managed code where CA1805 applies). This prevents use-before-initialization in code paths that check m_pNext before the SyncBlock is added to a free list.

---

✅ loaderallocator.cpp iteration change — loaderallocator.cpp:1966

Using HandleCleanupList::GetNext(item) instead of item->m_pNext is correct and preferable — it uses the traits accessor rather than directly accessing the field, maintaining the abstraction layer.

---

✅ static_assert enforcement design

The compile-time enforcement of mode-specific operations is well-designed:

• InsertHead, RemoveHead, InsertTail, RemoveFirst, FindAndRemove, InsertAfter all block SListMode::Interlocked
• InsertHeadInterlocked requires SListMode::Interlocked
• InsertTail/GetTail require HasTail
• InsertHeadInterlocked blocks HasTail

This prevents incorrect mixing of interlocked and non-interlocked operations at compile time.

Generated by Code Review for issue #126949 · ◷

[max-charlamb]

don't need comment, can be removed.

[max-charlamb]

same as above, can be removed.

[max-charlamb]

nit: please fix comment spacing