[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

@@ -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,

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
             }
         };
 

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);

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

MobileGL/MG_Test/SanityTest.cpp

@@ -34,6 +34,7 @@
 #include <MG_Util/ShaderTranspiler/ShaderSourceProcessor.h>
 #include <MG_Util/Debug/Log.h>
 #include <MG_Util/Types.h>
+#include <set>
 
 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<MobileGL::Uint64> 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<MobileGL::Uint32> erasedKeys;
+    std::set<MobileGL::Uint32> 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) {

MobileGL/MG_Util/Types.h

@@ -57,13 +57,21 @@ namespace MobileGL {
     using Array = std::array<T, N>;
     // 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<Key, T> with the key exposed mutably, so `it->first =`
     // compiles and silently corrupts the table - the one sharp edge this map has