[Test] (Pipe): count the map-persistent round trips over a re-specified adopted arena in both arms, now that the VAO rebind fix is in the branch

MobileGL/MG_IntegrationTest/Scenarios/LargeArenaAdoptionScenario.cpp

@@ -153,12 +153,10 @@ void main() { word = 0xC0FFEEu; }
                 glBindVertexArray(0);
                 if (m_vao != 0) glDeleteVertexArrays(1, &m_vao);
                 if (m_arena != 0) glDeleteBuffers(1, &m_arena);
-                if (m_secondArena != 0) glDeleteBuffers(1, &m_secondArena);
                 if (m_program != 0) glDeleteProgram(m_program);
                 if (m_compute != 0) glDeleteProgram(m_compute);
                 m_vao = 0;
                 m_arena = 0;
-                m_secondArena = 0;
                 m_program = 0;
                 m_compute = 0;
             }
@@ -266,9 +264,6 @@ void main() { word = 0xC0FFEEu; }
             unsigned int m_compute = 0;
             unsigned int m_vao = 0;
             unsigned int m_arena = 0;
-            // Only the counting case uses this one; see the comment there for why it does not
-            // simply re-specify m_arena.
-            unsigned int m_secondArena = 0;
             std::string m_buildLog;
         };
 
@@ -459,32 +454,30 @@ void main() { word = 0xC0FFEEu; }
 
         Gl().EndFrame(); // close the setup window, SetUp's own definition included
 
-        // One definition of a store past the 16 MiB adoption threshold, in a SECOND arena rather
+        // One definition of a store past the 16 MiB adoption threshold, taken by RE-SPECIFYING
-        // than by re-specifying SetUp's. Re-specifying an adopted store while a VAO's attributes
+        // SetUp's arena while m_vao's attributes are still pointing into it - and the attributes
-        // still read it leaves the backend VAO bound to the retired store - `dev`'s d7655247
+        // are deliberately NOT re-declared afterwards, so the draws below can only land if the
-        // ("rebind VAOs when an adopted buffer is respecified - the immediate retire path forgot
+        // backend VAO followed the new store on its own.
-        // the buffer-id generation"), which is NOT in feat/disaggregated's history. A counting
+        //
-        // case that carried that crash would be red for a reason that has nothing to do with the
+        // That is the hard shape on purpose. It was routed around in the first cut of this file
-        // counter. A fresh store is the same STORAGE DEFINITION either way, which is what
+        // because feat/disaggregated did not yet carry `dev`'s d7655247 ("rebind VAOs when an
-        // ARCHITECTURE.md:474 prices.
+        // adopted buffer is respecified - the immediate retire path forgot the buffer-id
-        glGenBuffers(1, &m_secondArena);
+        // generation") and the workload was a hard SIGSEGV inside the vertex fetch on the first
-        glBindBuffer(GL_ARRAY_BUFFER, m_secondArena);
+        // draw after the re-specification. ID-9 merged that fix (feat/disaggregated 5cb826b0) and
+        // requires it to hold in BOTH the legacy and the handle arm of the respecify/retire path,
+        // so this workload counts the path rather than avoiding it: under the
+        // ResourceSubsystemOn./Off. lanes the same body runs on both arms, and a handle arm that
+        // re-implemented the retire without the rebind is a crash here rather than a silent
+        // divergence found on device.
+        glBindBuffer(GL_ARRAY_BUFFER, m_arena);
         glBufferData(GL_ARRAY_BUFFER, kArenaBytes, nullptr, GL_DYNAMIC_DRAW);
-        glBindVertexArray(m_vao);
+        ASSERT_EQ(FirstGLError(), 0u) << "re-specifying the arena inside the counted window failed";
-        glVertexAttribPointer(0, 2, GL_FLOAT, GL_FALSE, sizeof(Vertex),
-                              reinterpret_cast<void*>(kVertexOffset));
-        glVertexAttribPointer(1, 3, GL_FLOAT, GL_FALSE, sizeof(Vertex),
-                              reinterpret_cast<void*>(kVertexOffset + 2 * sizeof(float)));
-        ASSERT_EQ(FirstGLError(), 0u) << "defining the second arena inside the counted window failed";
 
         // ... and then a frame's worth of traffic against it, of the shape the arena exists for:
         // a SubData per draw, every one of which lands in the adopted mapping and none of which
         // may acquire it again.
-        const auto vertices = QuadVertices(0.f, 1.f, 0.f);
         for (int draw = 0; draw < kDrawsInTheWindow; ++draw) {
-            glBindBuffer(GL_ARRAY_BUFFER, m_secondArena);
+            UploadQuad(0.f, 1.f, 0.f);
-            glBufferSubData(GL_ARRAY_BUFFER, kVertexOffset,
-                            GLsizeiptr(vertices.size() * sizeof(Vertex)), vertices.data());
             DrawQuad();
         }
         const auto px = CenterPixel();

MobileGL/MG_IntegrationTest/Scenarios/ResourceSubsystemControlScenario.cpp

@@ -89,19 +89,20 @@ namespace MGITest {
         // Two definitions and several draws each, so "one per definition", "one per draw" and
         // "none at all" are three different numbers.
         //
-        // TWO ARENAS, EACH DEFINED ONCE, rather than one arena defined twice, and that is a
+        // ONE ARENA DEFINED TWICE, not two arenas defined once each: the second definition
-        // deliberate detour around a bug that is not this branch's: re-specifying an adopted
+        // RE-SPECIFIES a store whose bytes the VAO's attributes are already pointing into, and
-        // store while a VAO's attributes still read it leaves the backend VAO bound to the
+        // the attributes are not re-declared afterwards. That makes this control also the place
-        // retired store, which `dev`'s d7655247 ("rebind VAOs when an adopted buffer is
+        // where the respecify/retire path is exercised on BOTH arms of the A/B, which is what
-        // respecified - the immediate retire path forgot the buffer-id generation") fixes. That
+        // ID-9 asks for: `dev`'s d7655247 ("rebind VAOs when an adopted buffer is respecified -
-        // commit is NOT in feat/disaggregated's history, and this workload reproduced it as a
+        // the immediate retire path forgot the buffer-id generation") arrived in
-        // hard SIGSEGV inside the vertex fetch on the first draw after the second definition.
+        // feat/disaggregated with the 5cb826b0 merge, and the handle arm duplicates that retire
-        // A control that carried a pre-existing `dev` crash would be red for a reason that has
+        // core, so an arm that forgot the rebind must be visible somewhere. Here it is a dead
-        // nothing to do with the subsystem bits it is measuring, so it defines a second arena
+        // draw or a fault, not a silent divergence. The first cut of this file routed around the
-        // instead - which is the same number of STORAGE DEFINITIONS, and therefore the same
+        // path because the fix was not yet in this branch's history and the workload reproduced
-        // count, by ARCHITECTURE.md:474's own wording. The finding is recorded for the
+        // as a hard SIGSEGV in the vertex fetch; that detour is what ID-9 supersedes.
-        // integrator rather than fixed here (ROADMAP.md:88: unrelated fixes do not ride the
+        //
-        // split work).
+        // The COUNT is unaffected by the change: two storage definitions either way, which is
+        // what ARCHITECTURE.md:474 prices.
         constexpr int kDefinitionsInTheWindow = 2;
         constexpr int kDrawsPerDefinition = 3;
         constexpr int kInset = 2;
@@ -154,12 +155,12 @@ void main() { oColor = vec4(vColor, 1.0); }
                 m_program = CompileProgram(kVS, kFS, &error);
                 ASSERT_NE(m_program, 0u) << error;
 
-                // The VAO only. The arenas are created and defined inside the counted window -
+                // The VAO only. The arena is created and defined inside the counted window - the
-                // the window a summary line reports is "since the previous line", so a
+                // window a summary line reports is "since the previous line", so a definition
-                // definition taken in SetUp would be counted in a window this case does not
+                // taken in SetUp would be counted in a window this case does not control - and
-                // control - and their attribute pointers are declared only once each store
+                // its attribute pointers are declared only once the store exists, because an
-                // exists, because an attribute whose offset is 16 MiB into a store that has not
+                // attribute whose offset is 16 MiB into a store that has not been defined yet is
-                // been defined yet is a range no driver has to accept.
+                // a range no driver has to accept.
                 glGenVertexArrays(1, &m_vao);
                 glBindVertexArray(m_vao);
                 RecordProperty("lane", m_lane.empty() ? "ambient" : m_lane.c_str());
@@ -170,10 +171,8 @@ void main() { oColor = vec4(vColor, 1.0); }
                 glUseProgram(0);
                 glBindVertexArray(0);
                 glBindBuffer(GL_ARRAY_BUFFER, 0);
-                for (GLuint& arena : m_arenas) {
+                if (m_arena != 0) glDeleteBuffers(1, &m_arena);
-                    if (arena != 0) glDeleteBuffers(1, &arena);
+                m_arena = 0;
-                    arena = 0;
-                }
                 if (m_vao != 0) glDeleteVertexArrays(1, &m_vao);
                 if (m_program != 0) glDeleteProgram(m_program);
             }
@@ -216,23 +215,29 @@ void main() { oColor = vec4(vColor, 1.0); }
             }
 
             // ONE storage definition - the NULL-data glBufferData past the adoption threshold,
-            // which is Minecraft's arena-creation idiom and the adoption point - then the
+            // which is Minecraft's arena-creation idiom and the adoption point - then a few
-            // attribute pointers into it and a few draws. Entirely inside one frame, so one
+            // draws. Entirely inside one frame, so one summary window covers exactly this.
-            // summary window covers exactly this.
+            //
-            void DefineAnArenaAndDrawFromIt(int index, float r, float g, float b) {
+            // The attribute pointers are declared ONCE, on the first definition, and never again:
-                glGenBuffers(1, &m_arenas[static_cast<std::size_t>(index)]);
+            // definition 0 creates the store, every later index RE-SPECIFIES it under the live
-                glBindBuffer(GL_ARRAY_BUFFER, m_arenas[static_cast<std::size_t>(index)]);
+            // VAO. Re-declaring them afterwards would re-sync the VAO by hand and hide the thing
+            // the second definition is here to exercise (see kDefinitionsInTheWindow above).
+            void DefineTheArenaAndDrawFromIt(int index, float r, float g, float b) {
+                if (index == 0) glGenBuffers(1, &m_arena);
+                glBindBuffer(GL_ARRAY_BUFFER, m_arena);
                 glBufferData(GL_ARRAY_BUFFER, kArenaBytes, nullptr, GL_DYNAMIC_DRAW);
                 const std::vector<Vertex> vertices = Quad(r, g, b);
                 glBufferSubData(GL_ARRAY_BUFFER, kVertexOffset,
                                 GLsizeiptr(vertices.size() * sizeof(Vertex)), vertices.data());
-                glBindVertexArray(m_vao);
+                if (index == 0) {
-                glVertexAttribPointer(0, 2, GL_FLOAT, GL_FALSE, sizeof(Vertex),
+                    glBindVertexArray(m_vao);
-                                      reinterpret_cast<void*>(kVertexOffset));
+                    glVertexAttribPointer(0, 2, GL_FLOAT, GL_FALSE, sizeof(Vertex),
-                glVertexAttribPointer(1, 3, GL_FLOAT, GL_FALSE, sizeof(Vertex),
+                                          reinterpret_cast<void*>(kVertexOffset));
-                                      reinterpret_cast<void*>(kVertexOffset + 2 * sizeof(float)));
+                    glVertexAttribPointer(1, 3, GL_FLOAT, GL_FALSE, sizeof(Vertex),
-                glEnableVertexAttribArray(0);
+                                          reinterpret_cast<void*>(kVertexOffset + 2 * sizeof(float)));
-                glEnableVertexAttribArray(1);
+                    glEnableVertexAttribArray(0);
+                    glEnableVertexAttribArray(1);
+                }
                 glUseProgram(m_program);
                 for (int draw = 0; draw < kDrawsPerDefinition; ++draw) {
                     glDrawArrays(GL_TRIANGLES, 0, 6);
@@ -242,8 +247,7 @@ void main() { oColor = vec4(vColor, 1.0); }
             std::string m_lane;
             GLuint m_program = 0;
             GLuint m_vao = 0;
-            std::vector<GLuint> m_arenas =
+            GLuint m_arena = 0;
-                std::vector<GLuint>(static_cast<std::size_t>(kDefinitionsInTheWindow), 0u);
         };
 
         // ONE case per lane, and it is a constraint rather than a preference: this case READS the
@@ -261,9 +265,11 @@ void main() { oColor = vec4(vColor, 1.0); }
 
             ClearTo(0.0f, 0.0f, 0.0f, 1.0f);
             for (int definition = 0; definition < kDefinitionsInTheWindow; ++definition) {
-                DefineAnArenaAndDrawFromIt(definition, 0.0f, 1.0f, 0.0f);
+                DefineTheArenaAndDrawFromIt(definition, 0.0f, 1.0f, 0.0f);
                 ASSERT_EQ(FirstGLError(), GLenum(GL_NO_ERROR))
-                    << "arena definition " << definition << " left a GL error behind";
+                    << "arena definition " << definition
+                    << " left a GL error behind (definition 0 creates the store, every later one "
+                       "re-specifies it under the live VAO)";
             }
             const Image image = ReadPixels(Gl().Width(), Gl().Height());
             Gl().EndFrame(); // the swap that emits the window covering exactly the work above