From e71d715e1ae072ba10fe9162d982bd89755fb2bc Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Wed, 12 Aug 2026 00:08:24 -0400 Subject: [PATCH] [Fix, Test] (MG_Backend/DirectVulkan, MG_Util, MG_Test): VkProgramObject move dropped stageSpirvDigests - a robin-hood swap would pair one program's modules with another's digests; sanity tests assert key identity, not iteration counts --- MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp | 3 +- .../DirectVulkan/Renderer/ProgramFactory.h | 10 +++++ .../Renderer/VkRenderPassManager.h | 28 +++++++------ .../GLState/BufferState/BufferState.cpp | 2 +- MobileGL/MG_Test/SanityTest.cpp | 39 +++++++++++++++++-- MobileGL/MG_Util/Types.h | 22 +++++++---- 6 files changed, 80 insertions(+), 24 deletions(-) diff --git a/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp b/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp index f06a1706..a47406b5 100644 --- a/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp +++ b/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp @@ -5175,7 +5175,8 @@ namespace MobileGL::MG_Backend::DirectGLES { GLsizei srcWidth, GLsizei srcHeight, GLsizei srcDepth) { // BY VALUE, not by reference. SyncTextureObjectToBackend hands back a reference to a // slot inside the backend texture registry, and the second call mutates that very map: - // GetOrCreate indexes it (an insert rehashes and moves every entry), and Find drops any + // GetOrCreate indexes it (an insert relocates entries - by rehashing, and also by + // robin-hood displacement well under the load factor), and Find drops any // entry whose state object has expired - which, with the map open-addressed and erasing // by shifting the probe cluster backwards, relocates entries other than the erased one. // Either way a reference taken by the first call is stale by the time the second returns, diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/ProgramFactory.h b/MobileGL/MG_Backend/DirectVulkan/Renderer/ProgramFactory.h index c34fda0b..249bbc38 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/ProgramFactory.h +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/ProgramFactory.h @@ -129,6 +129,14 @@ namespace MobileGL::MG_Backend::DirectVulkan { hash = other.hash; stages = std::move(other.stages); modules = std::move(other.modules); + // Must travel with `modules`: these digests name the SPIR-V those exact + // shader modules were built from, and the pipeline-failure diagnostics + // print the two together. Leaving it behind used to merely lose the + // digests on a rehash; now that the cache is a robin-hood table, insertion + // SWAPS two entries, and a field that no move touches stays behind in the + // slot - pairing one program's modules with another program's digests, so + // a pipeline failure would be reported against the wrong SPIR-V. + stageSpirvDigests = std::move(other.stageSpirvDigests); descriptorSetLayout = other.descriptorSetLayout; pipelineLayout = other.pipelineLayout; bindingKinds = std::move(other.bindingKinds); @@ -178,6 +186,7 @@ namespace MobileGL::MG_Backend::DirectVulkan { hash = other.hash; stages = std::move(other.stages); modules = std::move(other.modules); + stageSpirvDigests = std::move(other.stageSpirvDigests); // travels with `modules` - see the move ctor descriptorSetLayout = other.descriptorSetLayout; pipelineLayout = other.pipelineLayout; bindingKinds = std::move(other.bindingKinds); @@ -244,6 +253,7 @@ namespace MobileGL::MG_Backend::DirectVulkan { } modules.clear(); stages.clear(); + stageSpirvDigests.clear(); // the modules they describe are gone } }; diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/VkRenderPassManager.h b/MobileGL/MG_Backend/DirectVulkan/Renderer/VkRenderPassManager.h index 24978eb3..531a46be 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/VkRenderPassManager.h +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/VkRenderPassManager.h @@ -101,18 +101,24 @@ namespace MobileGL::MG_Backend::DirectVulkan { std::swap(layers, that.layers); std::swap(lastUsedFrame, that.lastUsedFrame); } - // Move ASSIGNMENT, not just construction. The destructor below suppresses the - // implicit one, which left the type move-constructible but not move-assignable - - // and therefore not swappable, which std::swap(pair&, pair&) requires. That was - // invisible while UnorderedMap only ever move-CONSTRUCTED an element into a fresh - // slot. ska::flat_hash_map probes robin-hood: inserting swaps the entry being - // placed against the one already sitting in the slot whenever it has travelled - // further from its desired position, so the mapped type has to be swappable or the - // whole table fails to instantiate. + // Move ASSIGNMENT, not just construction. The move constructor above and the + // destructor below each independently suppress the implicit one, which left the + // type move-constructible but not move-assignable - and therefore not swappable, + // which std::swap(pair&, pair&) requires. That was invisible while UnorderedMap + // only ever move-CONSTRUCTED an element into a fresh slot. ska::flat_hash_map + // probes robin-hood: inserting swaps the entry being placed against the one + // already sitting in the slot whenever it has travelled further from its desired + // position, so the mapped type has to be swappable or the table fails to + // instantiate at all. // - // Swap-based, exactly like the move constructor: our old handles land in `that` - // and are destroyed when it dies, so ownership stays with a single object and no - // VkRenderPass/VkFramebuffer is leaked or double-destroyed. + // SWAP SEMANTICS, exactly like the move constructor: this does not release the + // destination's handles, it parks them in `that`, which destroys them when it + // dies. That is correct for the only caller - std::swap, whose temporary expires + // immediately - and it is what keeps the three-move sequence from destroying a + // live render pass. It is NOT correct for a hand-written `a = std::move(b)` where + // `a` held live handles and `b` outlives the statement: those handles would then + // survive until `b` dies. There is no such caller; add a destroy-then-steal + // assignment before writing one. RenderPassEntry& operator=(RenderPassEntry&& that) noexcept { if (this != &that) { std::swap(hash, that.hash); diff --git a/MobileGL/MG_State/GLState/BufferState/BufferState.cpp b/MobileGL/MG_State/GLState/BufferState/BufferState.cpp index cd965d70..e43ee75c 100644 --- a/MobileGL/MG_State/GLState/BufferState/BufferState.cpp +++ b/MobileGL/MG_State/GLState/BufferState/BufferState.cpp @@ -68,7 +68,7 @@ namespace MobileGL::MG_State::GLState { } } // Erase through the iterator already in hand: erase(key) would repeat the - // find() from line 55, and the successor scan that once made key-based + // find() above, and the successor scan that once made key-based // erase the cheaper of the two no longer happens here - erase(iterator) // hands back an unconverted proxy, and the scan is what converting it // would cost. The unbind loops above touch only the binding arrays, so diff --git a/MobileGL/MG_Test/SanityTest.cpp b/MobileGL/MG_Test/SanityTest.cpp index 6a3be004..41b72bcb 100644 --- a/MobileGL/MG_Test/SanityTest.cpp +++ b/MobileGL/MG_Test/SanityTest.cpp @@ -34,6 +34,7 @@ #include #include #include +#include namespace { class DynamicParameterBackend final : public MobileGL::MG_Backend::BackendObject { @@ -2018,13 +2019,26 @@ TEST(UnorderedMapSanity, EraseWhileIteratingVisitsEveryElementExactlyOnce) { } ASSERT_EQ(map.size(), kCount); + // Record WHICH keys the sweep hands back, not just how many. A count alone cannot + // tell a correct sweep from one that visits some element twice and misses another, + // which is exactly the shape a backward-shift bug takes: the shift rewrites the + // probe cluster, so a defect duplicates or strands elements rather than changing + // the tally. + std::set visitedKeys; MobileGL::SizeT visited = 0; for (auto it = map.begin(); it != map.end();) { + const MobileGL::Uint64 key = it->first; + EXPECT_TRUE(visitedKeys.insert(key).second) << "key " << key << " was visited twice"; it = map.erase(it); ++visited; - ASSERT_LE(visited, kCount); // old code: runaway past end / skipped entries + ASSERT_LE(visited, kCount); // runaway past end / skipped entries } EXPECT_EQ(visited, kCount); + EXPECT_EQ(visitedKeys.size(), kCount); + for (MobileGL::Uint64 key = 0; key < kCount; ++key) { + EXPECT_TRUE(visitedKeys.count(key * 0x9e3779b97f4a7c15ull) != 0) + << "key " << key << " was never visited by the sweep"; + } EXPECT_EQ(map.size(), 0u); } @@ -2037,19 +2051,36 @@ TEST(UnorderedMapSanity, EraseReturnsTheSuccessorElement) { // Erasing every other visited element must still visit all 64 exactly once: // the iterator returned by erase names the very next element, not one past it. MobileGL::SizeT visited = 0; - MobileGL::SizeT erased = 0; + std::set erasedKeys; + std::set keptKeys; for (auto it = map.begin(); it != map.end();) { ++visited; + const MobileGL::Uint32 key = it->first; if ((visited & 1) != 0) { + erasedKeys.insert(key); it = map.erase(it); - ++erased; } else { + keptKeys.insert(key); ++it; } ASSERT_LE(visited, 64u); } EXPECT_EQ(visited, 64u); - EXPECT_EQ(map.size(), 64u - erased); + EXPECT_EQ(erasedKeys.size() + keptKeys.size(), 64u); + EXPECT_EQ(map.size(), keptKeys.size()); + + // The interleaved erases rewrite probe clusters underneath the cursor, so the real + // question is not how many elements the loop counted but whether the table still + // resolves every key correctly afterwards. A stranded element stays in size() but + // stops being findable; a duplicated one answers for a key it does not own. + for (const MobileGL::Uint32 key : keptKeys) { + const auto found = map.find(key); + ASSERT_NE(found, map.end()) << "surviving key " << key << " is no longer findable"; + EXPECT_EQ(found->second, key) << "key " << key << " resolves to the wrong value"; + } + for (const MobileGL::Uint32 key : erasedKeys) { + EXPECT_EQ(map.find(key), map.end()) << "erased key " << key << " is still findable"; + } } TEST(UnorderedMapSanity, ErasingTheOnlyElementReturnsEnd) { diff --git a/MobileGL/MG_Util/Types.h b/MobileGL/MG_Util/Types.h index e5cbbcbe..40a9568f 100644 --- a/MobileGL/MG_Util/Types.h +++ b/MobileGL/MG_Util/Types.h @@ -57,13 +57,21 @@ namespace MobileGL { using Array = std::array; // ska::flat_hash_map, the same table MobileGlues settled on, at the same commit. // - // Open addressing with robin-hood probing, so a rehash moves the elements: any - // insert, emplace, operator[], reserve or rehash invalidates every iterator, - // reference and pointer into the map. Erase does too, and less obviously - - // deletion shifts the rest of the probe cluster backwards, so erasing one key - // can move a DIFFERENT key's element. Where a mapped value's address has to - // outlive later mutation, the map holds a UniquePtr/SharedPtr and the pointee - // stays put; those sites say so where they are declared. + // Open addressing with robin-hood probing. Any insert, emplace, operator[], + // reserve or rehash invalidates every iterator, reference and pointer into the + // map - and NOT only by rehashing: robin-hood insertion swaps the entry being + // placed against the occupant whenever it has travelled further from its desired + // position, so an insert well under the load factor still relocates entries. + // Erase relocates too, and less obviously - deletion shifts the rest of the probe + // cluster backwards, so erasing one key can move a DIFFERENT key's element. + // Where a mapped value's address has to outlive later mutation, the map holds a + // UniquePtr/SharedPtr and the pointee stays put; those sites say so where they + // are declared. + // + // Erase destroys the mapped value BEFORE it repairs the probe cluster, so a + // mapped-value destructor that re-enters the same map sees a hole in the middle + // of a chain and a stale size: a re-entrant find() misses every key past the hole. + // Nothing does that today; do not be the first without checking. // // Its value_type is pair with the key exposed mutably, so `it->first =` // compiles and silently corrupts the table - the one sharp edge this map has