From 84bdfa2af58206b201802eb9f02c569b53092479 Mon Sep 17 00:00:00 2001 From: PowerOfNames Date: Tue, 1 Sep 2026 21:17:07 +0200 Subject: [PATCH] Substrate: give PoolAllocator a FIFO free list Allocate popped m_FreeHandles.back() and Free pushed back, so a churning caller cycled a handful of indices and burned their generation counters to exhaustion while the rest of the pool sat untouched. Resize churn retired the first image slot after roughly 63 resizes. The free list is now a ring buffer over the same fixed-capacity index array -- head, tail and an explicit free count -- so reuse rotates through every slot and a freed handle stays detectably stale for far longer. MAX_BLOCK_COUNT is not always a power of two, so the wrap is a compare rather than a mask. BaseHandle: IsValid() no longer treats "all index bits set" as invalid. When the block count is an exact power of two the highest valid index equals the index mask, which made that slot allocatable but never freeable -- Free took the retirement branch immediately, so the index never returned to the list and the allocation count never decremented. Both Aurora pools are that shape at 1024 blocks. Validity now keys off the generation alone, which already subsumes the all-ones sentinel. A handle type must leave at least one index bit, enforced by static_assert instead of by every instance reading invalid. Slot retirement is observable: GetMaxedGenerationCount() on AllocatorBase, alongside GetCurrentAllocationCount(), which still counts retired slots. Tests: rewrote the two PoolAllocator cases that asserted LIFO reuse, moved generation exhaustion onto a small uint16_t pool so it runs in milliseconds rather than 33.5M iterations, and added coverage for rotation, burn spreading, ring wrap, retirement and the power-of-two top slot. Added the missing CreateRefFromThis cases, including the consumer shape that caught the original missing AddRef. Removed two TestAllocationHandle sections that instantiated DefineHandle<16, 0, uint16_t>, which the new static_assert rejects. 783 assertions in 29 cases, green. Sandbox verified in Debug and Release. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01HGxv2j7K5MFmFMX1iKrgcq --- Substrate/Include/Substrate/AllocatorBase.h | 1 + Substrate/Include/Substrate/BaseHandle.h | 21 +- Substrate/Include/Substrate/LinearAllocator.h | 1 + Substrate/Include/Substrate/PoolAllocator.h | 41 +++- Substrate/Include/Substrate/StackAllocator.h | 1 + SubstrateTests/Tests/TestAllocationHandle.cpp | 22 +- SubstrateTests/Tests/TestPoolAllocator.cpp | 212 ++++++++++++++++-- SubstrateTests/Tests/TestRefCounted_st.cpp | 139 ++++++++++++ 8 files changed, 379 insertions(+), 59 deletions(-) diff --git a/Substrate/Include/Substrate/AllocatorBase.h b/Substrate/Include/Substrate/AllocatorBase.h index c1234ac..ef15f08 100644 --- a/Substrate/Include/Substrate/AllocatorBase.h +++ b/Substrate/Include/Substrate/AllocatorBase.h @@ -14,6 +14,7 @@ namespace Substrate { virtual size_t GetUsedMemory() const = 0; virtual size_t GetCurrentAllocationCount() const = 0; virtual size_t GetTotalAllocationCount() const = 0; + virtual size_t GetMaxedGenerationCount() const = 0; virtual size_t GetResetCount() const = 0; #endif }; diff --git a/Substrate/Include/Substrate/BaseHandle.h b/Substrate/Include/Substrate/BaseHandle.h index fc774fd..1f7f96a 100644 --- a/Substrate/Include/Substrate/BaseHandle.h +++ b/Substrate/Include/Substrate/BaseHandle.h @@ -52,6 +52,8 @@ namespace Substrate { class BaseHandle { public: + static_assert(GenerationMask != static_cast(~static_cast(0)), "GenerationMask cannot be all 1s"); + constexpr BaseHandle() : m_Handle(0) {}; constexpr BaseHandle(THandleType index) : m_Handle(index) { @@ -76,23 +78,14 @@ namespace Substrate { } - /// Only true if the generation is not equal to the generation mask. Except when there are no generation bits, which is always a valid generation. Returns false if index == indexMask + /// Only true if the generation is not equal to the generation mask. This also covers GenerationMask = 0 -> the only invalid handle IS INVALID_HANDLE -> Index maxed out. + /// Or, generationBits maxed and index maxed, which is invalid because generation is maxed out. constexpr bool IsValid() { - //If the handle's index bits are all set to 1, the handle is invalid. - constexpr THandleType indexMask = GetIndexMask(); - if ((m_Handle & indexMask) == indexMask) - return false; - - //If the generation mask is 0, the handle is always valid. - if(GenerationMask == 0) + if constexpr(GenerationMask == 0) return true; - - //We return false if the generation mask is all bits set, because that means there is no valid index possible. - if (GenerationMask == static_cast((~static_cast(0)))) - return false; - - return (m_Handle & GenerationMask) != GenerationMask; + else + return (m_Handle & GenerationMask) != GenerationMask; } /// This is only true if the generations are equal AND the index! diff --git a/Substrate/Include/Substrate/LinearAllocator.h b/Substrate/Include/Substrate/LinearAllocator.h index 4b16241..401c67b 100644 --- a/Substrate/Include/Substrate/LinearAllocator.h +++ b/Substrate/Include/Substrate/LinearAllocator.h @@ -35,6 +35,7 @@ namespace Substrate { inline size_t GetTotalMemory() const override { return m_TotalSize; } inline size_t GetTotalAllocationCount() const override { return m_TotalAllocationCount; } inline size_t GetCurrentAllocationCount() const override { return m_CurrentAllocationCount; } + inline size_t GetMaxedGenerationCount() const override { return 0; } inline size_t GetResetCount() const override { return m_ResetCount; } #endif private: diff --git a/Substrate/Include/Substrate/PoolAllocator.h b/Substrate/Include/Substrate/PoolAllocator.h index cf5d374..02d786c 100644 --- a/Substrate/Include/Substrate/PoolAllocator.h +++ b/Substrate/Include/Substrate/PoolAllocator.h @@ -38,7 +38,7 @@ namespace Substrate { bool IsHandleValid(TResourceHandle handle); void Free(TResourceHandle handle); - const std::vector& GetFreeHandleIndices() const { return m_FreeHandles; } + const uint32_t GetFreeHandleCount() const { return m_FreeCount; } using InternalHandle = DefineHandle; const std::vector& GetHandles() const { return m_Handles; } @@ -48,6 +48,7 @@ namespace Substrate { inline size_t GetUsedMemory() const override { return m_CurrentAllocationCount * sizeof(TBlockType); } inline size_t GetCurrentAllocationCount() const override { return m_CurrentAllocationCount; } inline size_t GetTotalAllocationCount() const override { return m_TotalAllocationCount; } + inline size_t GetMaxedGenerationCount() const override { return m_MaxedGenerationCount; } inline size_t GetResetCount() const override { return 0; } #endif private: @@ -59,6 +60,10 @@ namespace Substrate { size_t m_TotalAllocationCount = 0; std::vector m_Handles; std::vector m_FreeHandles; + uint32_t m_FreeHead = 0; + uint32_t m_FreeTail = 0; + uint32_t m_FreeCount = 0; + uint32_t m_MaxedGenerationCount = 0; }; @@ -75,11 +80,15 @@ namespace Substrate { m_Handles.reserve(MAX_BLOCK_COUNT); m_FreeHandles.reserve(MAX_BLOCK_COUNT); + m_FreeCount = MAX_BLOCK_COUNT; + m_FreeHead = 0; + m_FreeTail = 0; + // Initialize handles in reverse order for better cache locality for (uint32_t i = 0; i < MAX_BLOCK_COUNT; i++) { m_Handles.push_back(InternalHandle(i)); - m_FreeHandles.push_back(MAX_BLOCK_COUNT-1-i); + m_FreeHandles.push_back(i); } } @@ -108,6 +117,10 @@ namespace Substrate { m_TotalSize = 0; m_Handles.clear(); m_FreeHandles.clear(); + m_FreeHead = 0; + m_FreeTail = 0; + //Currently blocks further usage of the Allocator + m_FreeCount = 0; } template @@ -115,17 +128,18 @@ namespace Substrate { TResourceHandle PoolAllocator::Allocate() { // Check if there are free handles - if(m_FreeHandles.size() == 0) + if(m_FreeCount == 0) throw AllocatorOutOfMemoryException("Pool allocator out of memory"); // Get the next free handle - uint32_t idx = m_FreeHandles.back(); - m_FreeHandles.pop_back(); + uint32_t idx = m_FreeHandles[m_FreeHead]; + m_FreeHead = (m_FreeHead + 1) % MAX_BLOCK_COUNT; //Calls constructor -> sets default values new(m_MemoryBlock + idx) TBlockType{}; m_CurrentAllocationCount++; m_TotalAllocationCount++; + m_FreeCount--; return m_Handles[idx].GetRaw(); } @@ -135,9 +149,9 @@ namespace Substrate { { InternalHandle handle = InternalHandle::FromRawType(resourceHandle); InternalHandle& internal = m_Handles[handle.Index()]; - if (!internal.Equals(handle)) - return false; - return true; + if (internal.IsValid() && internal.Equals(handle)) + return true; + return false; } template @@ -161,16 +175,21 @@ namespace Substrate { // Validate handle InternalHandle handle = InternalHandle::FromRawType(resourceHandle); InternalHandle& internal = m_Handles[handle.Index()]; - if (!internal.Equals(handle)) + if (!internal.Equals(handle) || !internal.IsValid()) return; // Invalid handle or handle was already freed internal = internal.IncrementGeneration(); // Check if generation is maxed out - if(!internal.IsValid()) + if (!internal.IsValid()) + { + m_MaxedGenerationCount++; return; // Cannot free handle anymore + } - m_FreeHandles.push_back(internal.Index()); + m_FreeHandles[m_FreeTail] = internal.Index(); + m_FreeCount++; + m_FreeTail = (m_FreeTail + 1) % MAX_BLOCK_COUNT; // Decrease allocation count only if handle was put back into the free list m_CurrentAllocationCount--; diff --git a/Substrate/Include/Substrate/StackAllocator.h b/Substrate/Include/Substrate/StackAllocator.h index 79b0265..5d9b6be 100644 --- a/Substrate/Include/Substrate/StackAllocator.h +++ b/Substrate/Include/Substrate/StackAllocator.h @@ -25,6 +25,7 @@ namespace Substrate { inline size_t GetTotalMemory() const override { return m_TotalSize; } inline size_t GetTotalAllocationCount() const override { return m_TotalAllocationCount; } inline size_t GetCurrentAllocationCount() const override { return m_CurrentAllocationCount; } + inline size_t GetMaxedGenerationCount() const override { return 0; } inline size_t GetResetCount() const override { return m_ResetCount; } #endif private: diff --git a/SubstrateTests/Tests/TestAllocationHandle.cpp b/SubstrateTests/Tests/TestAllocationHandle.cpp index 09a71f8..0ad1195 100644 --- a/SubstrateTests/Tests/TestAllocationHandle.cpp +++ b/SubstrateTests/Tests/TestAllocationHandle.cpp @@ -75,13 +75,11 @@ TEST_CASE("AllocationHandle Generation creation and validation", "[AllocationHan REQUIRE(handle.IsValid() == true); } - SECTION("Max value mask") - { - using TestHandle16_16_0 = Substrate::DefineHandle<16, 0, uint16_t>; - TestHandle16_16_0 handle = TestHandle16_16_0(0); - REQUIRE(handle.GetGenerationMask() == 0xFFFF); - REQUIRE(handle.IsValid() == false); - } + // "Max value mask" (DefineHandle<16, 0, uint16_t>) was removed on 2026-09-01. + // An all-ones generation mask leaves zero index bits, so the handle can address + // nothing and every instance read as invalid. That is now a static_assert in + // BaseHandle -- the instantiation itself is ill-formed, so there is no object + // left to assert against from here. Do not re-add it; it will not compile. SECTION("Overflow protection") { @@ -139,13 +137,9 @@ TEST_CASE("AllocationHandle Index handling", "[AllocationHandle][Index]") REQUIRE(handle.GetMaxIndexValue() == 0xFFF); } - SECTION("Zero index mask") - { - using TestHandle16_16_0 = Substrate::DefineHandle<16, 0, uint16_t>; - TestHandle16_16_0 handle = TestHandle16_16_0(0); - REQUIRE(handle.GetIndexMask() == 0x0000); - REQUIRE(handle.IsValid() == false); - } + // "Zero index mask" removed on 2026-09-01 for the same reason as "Max value mask" + // above: it instantiated DefineHandle<16, 0, uint16_t>, which the BaseHandle + // static_assert now rejects at compile time. SECTION("Max value index mask") { diff --git a/SubstrateTests/Tests/TestPoolAllocator.cpp b/SubstrateTests/Tests/TestPoolAllocator.cpp index 51da3fa..5863921 100644 --- a/SubstrateTests/Tests/TestPoolAllocator.cpp +++ b/SubstrateTests/Tests/TestPoolAllocator.cpp @@ -4,6 +4,9 @@ #include "Substrate/PoolAllocator.h" #include "Substrate/UtilityFunctions.h" +#include +#include + struct TestStruct // 9 bytes -> aligned to 12 bytes { @@ -19,6 +22,25 @@ constexpr size_t BlockSizeWithPadding = 12 + 4; using Allocator = Substrate::PoolAllocator; +// Raw value one generation step is worth: the generation sits above the index bits, +// so incrementing it adds (1 << INDEX_BIT_COUNT) to the raw handle. +constexpr uint32_t GenerationStep = 1u << Allocator::INDEX_BIT_COUNT; // 1 << 7 == 128 + +// A deliberately tiny pool for the generation-exhaustion cases. 36 / 12 == 3 blocks, +// so INDEX_BIT_COUNT == Log2Up(3) == 2 and GENERATION_BIT_COUNT == 16 - 2 == 14. +// That is 16383 generations per slot instead of the 33.5 million a uint32_t handle +// gives, which keeps these cases in the millisecond range. +constexpr size_t SmallAllocatorSize = 3 * TestStructSize; +using SmallAllocator = Substrate::PoolAllocator; + +// 4 blocks == an exact power of two, so the highest index (3) equals the index mask +// (INDEX_BIT_COUNT == Log2Up(4) == 2). That slot used to read as invalid purely from +// its index bits, which made it allocatable but never freeable. Aurora's pools are +// this shape at 1024 blocks, so the case is worth pinning. +constexpr size_t PowerOfTwoAllocatorSize = 4 * TestStructSize; +using PowerOfTwoAllocator = Substrate::PoolAllocator; + + TEST_CASE("Pool Allocator", "[Allocator][Pool]") { SECTION("Creation") @@ -30,8 +52,9 @@ TEST_CASE("Pool Allocator", "[Allocator][Pool]") REQUIRE(allocator.GetCurrentAllocationCount() == 0); REQUIRE(allocator.GetTotalAllocationCount() == 0); REQUIRE(allocator.GetResetCount() == 0); - REQUIRE(allocator.GetFreeHandleIndices().size() == MaxAllocations); - + REQUIRE(allocator.GetMaxedGenerationCount() == 0); + REQUIRE(allocator.GetFreeHandleCount() == MaxAllocations); + REQUIRE(Allocator::MAX_BLOCK_COUNT == MaxAllocations); REQUIRE(Allocator::INDEX_BIT_COUNT == Substrate::Utility::Log2Up(85)); //should be 7 @@ -46,6 +69,7 @@ TEST_CASE("Pool Allocator", "[Allocator][Pool]") REQUIRE(allocator.GetUsedMemory() == TestStructSize); REQUIRE(allocator.GetCurrentAllocationCount() == 1); REQUIRE(allocator.GetTotalAllocationCount() == 1); + REQUIRE(allocator.GetFreeHandleCount() == MaxAllocations - 1); } SECTION("GetPointerFromHandle") @@ -79,23 +103,92 @@ TEST_CASE("Pool Allocator", "[Allocator][Pool]") REQUIRE(allocator.GetUsedMemory() == 0); REQUIRE(allocator.GetCurrentAllocationCount() == 0); REQUIRE(allocator.GetTotalAllocationCount() == 1); + REQUIRE(allocator.GetFreeHandleCount() == MaxAllocations); } - SECTION("Free and reallocate") + SECTION("Free and reallocate hands out a different slot") { Allocator allocator; uint32_t handle1 = allocator.Allocate(); + REQUIRE(handle1 == 0); // index 0, generation 0 allocator.Free(handle1); + + // FIFO: the freed index goes to the back of the queue, so the next allocation + // takes the next untouched slot instead of handing index 0 straight back. + // Under the old LIFO free list this returned 128 (index 0, generation 1). uint32_t handle2 = allocator.Allocate(); - - REQUIRE(handle1 != handle2); // Handle should have a different generation now - REQUIRE(handle2 == 128); // Handle index should be 0 again, but the first generation bit should be incremented by 1 -> 1 << 7 == 128 + REQUIRE(handle2 == 1); REQUIRE(allocator.GetUsedMemory() == TestStructSize); REQUIRE(allocator.GetCurrentAllocationCount() == 1); REQUIRE(allocator.GetTotalAllocationCount() == 2); } + SECTION("A freed slot only returns after a full rotation") + { + Allocator allocator; + uint32_t first = allocator.Allocate(); + REQUIRE(first == 0); + allocator.Free(first); // index 0 -> back of the queue, generation 1 + + // Every other slot is handed out before index 0 comes back around. + for (size_t i = 1; i < MaxAllocations; i++) + { + uint32_t handle = allocator.Allocate(); + REQUIRE(handle == static_cast(i)); // still generation 0 + } + + // The queue now holds index 0 alone, carrying generation 1. + uint32_t reused = allocator.Allocate(); + REQUIRE(reused == GenerationStep); + REQUIRE(allocator.GetFreeHandleCount() == 0); + } + + SECTION("Generation burn spreads across every slot") + { + // This is the property the FIFO free list exists for. Under LIFO this same + // loop drove index 0 to generation 85 and left every other slot at 0. + Allocator allocator; + for (size_t i = 0; i < MaxAllocations; i++) + { + uint32_t handle = allocator.Allocate(); + REQUIRE(handle == static_cast(i)); // generation 0 on the first pass + allocator.Free(handle); + } + + // One rotation later every slot sits at generation 1, none of them higher. + for (size_t i = 0; i < MaxAllocations; i++) + { + uint32_t handle = allocator.Allocate(); + REQUIRE(handle == GenerationStep + static_cast(i)); + } + REQUIRE(allocator.GetFreeHandleCount() == 0); + } + + SECTION("Reuse order survives the ring wrapping") + { + // MAX_BLOCK_COUNT is 85 here, not a power of two, so the wrap is a plain + // compare rather than a mask. Filling the pool wraps the head; freeing the + // whole pool afterwards wraps the tail. + Allocator allocator; + std::vector live; + live.reserve(MaxAllocations); + for (size_t i = 0; i < MaxAllocations; i++) + live.push_back(allocator.Allocate()); + + REQUIRE(allocator.GetFreeHandleCount() == 0); + + for (uint32_t handle : live) + allocator.Free(handle); + + REQUIRE(allocator.GetFreeHandleCount() == MaxAllocations); + REQUIRE(allocator.GetCurrentAllocationCount() == 0); + + // Freed in index order, so they must come back in index order. + for (size_t i = 0; i < MaxAllocations; i++) + REQUIRE(allocator.Allocate() == GenerationStep + static_cast(i)); + } + SECTION("Allocate until full") { Allocator allocator; @@ -112,25 +205,104 @@ TEST_CASE("Pool Allocator", "[Allocator][Pool]") REQUIRE(allocator.GetUsedMemory() == MaxAllocations * TestStructSize); REQUIRE(allocator.GetCurrentAllocationCount() == MaxAllocations); REQUIRE(allocator.GetTotalAllocationCount() == MaxAllocations); + + // The exhaustion guard has to read the free count: m_FreeHandles is a + // fixed-capacity ring now and its size() never drops to zero. + REQUIRE(allocator.GetFreeHandleCount() == 0); REQUIRE_THROWS_AS(allocator.Allocate(), Substrate::AllocatorOutOfMemoryException); } - SECTION("Allocate and reallocate until generations are exhausted") + SECTION("The top slot of a power-of-two pool is usable") { - Allocator allocator; - constexpr size_t maxGenerations = (1 << Allocator::GENERATION_BIT_COUNT) - 1; - uint32_t firstHandle; - for (size_t gen = 0; gen < maxGenerations; gen++) + // Regression cover for GAP-030. The highest index equals the index mask here, + // so a validity check that keys off "all index bits set" wrongly condemns it: + // the slot allocates, but Free takes the retirement branch and it never + // returns to the ring. Validity has to key off the sentinel instead. + PowerOfTwoAllocator allocator; + constexpr uint16_t topIndex = PowerOfTwoAllocator::MAX_BLOCK_COUNT - 1; // 3 == index mask + + std::vector handles; + for (size_t i = 0; i < PowerOfTwoAllocator::MAX_BLOCK_COUNT; i++) + handles.push_back(allocator.Allocate()); + + REQUIRE(handles.back() == topIndex); // generation 0, index 3 + REQUIRE(allocator.GetPointerFromHandle(handles.back()) != nullptr); + REQUIRE(allocator.IsHandleValid(handles.back())); + + allocator.Free(handles.back()); + REQUIRE(allocator.GetMaxedGenerationCount() == 0); // retirement must NOT have fired + REQUIRE(allocator.GetFreeHandleCount() == 1); + REQUIRE(allocator.GetCurrentAllocationCount() == PowerOfTwoAllocator::MAX_BLOCK_COUNT - 1); + + // It comes back with its generation incremented, like any other slot. + constexpr uint16_t smallGenerationStep = 1u << PowerOfTwoAllocator::INDEX_BIT_COUNT; + REQUIRE(allocator.Allocate() == static_cast(smallGenerationStep + topIndex)); + } + + SECTION("Generation exhaustion retires a single slot") + { + // Hold every slot but one. With exactly one entry in the free list, FIFO + // hands that same slot back every cycle, so its generation burns without + // needing a full rotation per step. + SmallAllocator allocator; + constexpr size_t maxGenerations = (1u << SmallAllocator::GENERATION_BIT_COUNT) - 1; + + std::vector held; + for (size_t i = 1; i < SmallAllocator::MAX_BLOCK_COUNT; i++) + held.push_back(allocator.Allocate()); + + uint16_t cycling = allocator.Allocate(); + REQUIRE(allocator.GetFreeHandleCount() == 0); + REQUIRE(allocator.GetCurrentAllocationCount() == SmallAllocator::MAX_BLOCK_COUNT); + + // Burn it up to the last usable generation. No REQUIRE inside the loop: + // Catch2 assertions cost far more than the work being exercised. + for (size_t generation = 1; generation < maxGenerations; generation++) { - firstHandle = allocator.Allocate(); - allocator.Free(firstHandle); + allocator.Free(cycling); + cycling = allocator.Allocate(); } - REQUIRE(allocator.GetTotalAllocationCount() == maxGenerations); - REQUIRE(allocator.GetCurrentAllocationCount() == 1); // Last allocation is still active because it wont get freed again - REQUIRE(allocator.GetUsedMemory() == TestStructSize); // We dont free -> used memory should still be 1 block + REQUIRE(allocator.GetCurrentAllocationCount() == SmallAllocator::MAX_BLOCK_COUNT); - // Next allocation should use the next index, as all generations for index 0 are exhausted and it will not end up in the free list again - uint32_t handle = allocator.Allocate(); - REQUIRE(handle == 1); // Next index + // The next free saturates the generation. The slot is retired: it does not + // return to the free list, so the pool is permanently one block short. + allocator.Free(cycling); + REQUIRE(allocator.GetFreeHandleCount() == 0); + REQUIRE(allocator.GetMaxedGenerationCount() == 1); + REQUIRE_THROWS_AS(allocator.Allocate(), Substrate::AllocatorOutOfMemoryException); + + // Known behaviour, not a fault in this test: a retired slot still counts as + // allocated, so the count overstates from here on. GetMaxedGenerationCount() + // is what makes that difference explainable rather than looking like a leak. + REQUIRE(allocator.GetCurrentAllocationCount() == SmallAllocator::MAX_BLOCK_COUNT); + } + + SECTION("Retiring one slot leaves the rest of the ring usable") + { + SmallAllocator allocator; + constexpr size_t maxGenerations = (1u << SmallAllocator::GENERATION_BIT_COUNT) - 1; + + std::vector held; + for (size_t i = 1; i < SmallAllocator::MAX_BLOCK_COUNT; i++) + held.push_back(allocator.Allocate()); + + uint16_t cycling = allocator.Allocate(); + for (size_t generation = 1; generation < maxGenerations; generation++) + { + allocator.Free(cycling); + cycling = allocator.Allocate(); + } + allocator.Free(cycling); // retires that slot + + // The slots held throughout still free and reallocate normally. + for (uint16_t handle : held) + allocator.Free(handle); + + REQUIRE(allocator.GetFreeHandleCount() == SmallAllocator::MAX_BLOCK_COUNT - 1); + REQUIRE(allocator.GetMaxedGenerationCount() == 1); + for (size_t i = 0; i < SmallAllocator::MAX_BLOCK_COUNT - 1; i++) + REQUIRE(allocator.GetPointerFromHandle(allocator.Allocate()) != nullptr); + + REQUIRE_THROWS_AS(allocator.Allocate(), Substrate::AllocatorOutOfMemoryException); } -} \ No newline at end of file +} diff --git a/SubstrateTests/Tests/TestRefCounted_st.cpp b/SubstrateTests/Tests/TestRefCounted_st.cpp index e272b7d..9dcedc5 100644 --- a/SubstrateTests/Tests/TestRefCounted_st.cpp +++ b/SubstrateTests/Tests/TestRefCounted_st.cpp @@ -184,4 +184,143 @@ TEST_CASE("RefCounted testing (single threaded) - if-check", "[RefCounted][if_st REQUIRE(true); else REQUIRE(false); +} + + +// --- CreateRefFromThis ------------------------------------------------------- +// CreateRefFromThis is protected, so these need a public shim the same way +// TestClass exposes DecRefPublic() for DecRef. + +class SelfRefClass : public Substrate::RefCounted +{ +public: + inline static int s_DestructorCalled = 0; + SelfRefClass() = default; + virtual ~SelfRefClass() + { + s_DestructorCalled++; + }; + + Ref SelfRef() + { + return CreateRefFromThis(); + } +}; + +// The consumer shape that surfaced the missing AddRef: a scope-local object that +// copies the returned Ref into a member. Mirrors VulkanSubmissionScheduler holding +// a Ref built from CreateRefFromThis. +class SelfRefConsumer +{ +public: + explicit SelfRefConsumer(Ref owner) : m_Owner(owner) {} +private: + Ref m_Owner; +}; + +class SelfRefBase : public Substrate::RefCounted +{ +public: + inline static int s_DestructorCalled = 0; + SelfRefBase() = default; + virtual ~SelfRefBase() + { + s_DestructorCalled++; + }; +}; + +class SelfRefDerived : public SelfRefBase +{ +public: + SelfRefDerived() = default; + + Ref SelfRef() + { + return CreateRefFromThis(); + } +}; + + +TEST_CASE("RefCounted testing (single threaded) - CreateRefFromThis acquires a count", "[RefCounted][SelfRef_st]") +{ + SelfRefClass::s_DestructorCalled = 0; + + Ref owner = CreateRef(); + REQUIRE(owner->GetRefCount() == 1); + { + Ref self = owner->SelfRef(); + REQUIRE(self.Get() == owner.Get()); + REQUIRE(owner->GetRefCount() == 2); + } + // Back to the original count, with the subject still alive: CreateRefFromThis + // hands out a reference it acquired, not one it borrowed from its owner. + REQUIRE(SelfRefClass::s_DestructorCalled == 0); + REQUIRE(owner->GetRefCount() == 1); +} + +TEST_CASE("RefCounted testing (single threaded) - CreateRefFromThis into a member", "[RefCounted][SelfRef_st]") +{ + SelfRefClass::s_DestructorCalled = 0; + + Ref owner = CreateRef(); + REQUIRE(owner->GetRefCount() == 1); + { + SelfRefConsumer consumer(owner->SelfRef()); + REQUIRE(owner->GetRefCount() == 2); + } + // Without the AddRef inside CreateRefFromThis this net-decrements: the returned + // Ref and the consumer's member both release a single acquisition, so the object + // is destroyed here while `owner` still holds it. Assert the destructor first, + // or the count read below is itself a use-after-free. + REQUIRE(SelfRefClass::s_DestructorCalled == 0); + REQUIRE(owner->GetRefCount() == 1); +} + +TEST_CASE("RefCounted testing (single threaded) - CreateRefFromThis does not drift", "[RefCounted][SelfRef_st]") +{ + SelfRefClass::s_DestructorCalled = 0; + + Ref owner = CreateRef(); + for (int i = 0; i < 100; i++) + { + SelfRefConsumer consumer(owner->SelfRef()); + } + + // Neither leaks nor over-releases across repeated use. + REQUIRE(SelfRefClass::s_DestructorCalled == 0); + REQUIRE(owner->GetRefCount() == 1); +} + +TEST_CASE("RefCounted testing (single threaded) - CreateRefFromThis upcast counts once", "[RefCounted][SelfRef_st]") +{ + SelfRefBase::s_DestructorCalled = 0; + + Ref derived = CreateRef(); + REQUIRE(derived->GetRefCount() == 1); + { + Ref base = derived->SelfRef(); + REQUIRE(base.Get() == derived.Get()); + REQUIRE(derived->GetRefCount() == 2); // once for the upcast, not twice + } + REQUIRE(SelfRefBase::s_DestructorCalled == 0); + REQUIRE(derived->GetRefCount() == 1); +} + +TEST_CASE("RefCounted testing (single threaded) - CreateRefFromThis outliving its owner", "[RefCounted][SelfRef_st]") +{ + SelfRefClass::s_DestructorCalled = 0; + + Ref self = nullptr; + { + Ref owner = CreateRef(); + self = owner->SelfRef(); + REQUIRE(owner->GetRefCount() == 2); + } + + // The self-reference is a real owner, so it keeps the object alive on its own. + REQUIRE(SelfRefClass::s_DestructorCalled == 0); + REQUIRE(self->GetRefCount() == 1); + + self = nullptr; + REQUIRE(SelfRefClass::s_DestructorCalled == 1); // destroyed exactly once } \ No newline at end of file