From e4579ee80e2281734e4d9a4727a62f8994996b86 Mon Sep 17 00:00:00 2001 From: rereview Date: Tue, 8 Sep 2026 20:28:56 -0400 Subject: [PATCH] [Fix, Test] (Pipe): mix the program identity into the shader-image shutter's plain arm - the per-program counter is equal across programs and against no program, so a switch never moved the image window and a buffer image never reached set_shader_images (P4a seam F-2, esprytdraw SD-4) --- MobileGL/MG_Impl/Pipe/Tracker.h | 10 ++- .../Scenarios/P4aSeamAuditScenario.cpp | 79 +++++++++++++++++++ MobileGL/MG_Pipe/DirtySurface.def | 12 +-- MobileGL/MG_Test/Pipe/TrackerTest.cpp | 25 ++++++ 4 files changed, 120 insertions(+), 6 deletions(-) diff --git a/MobileGL/MG_Impl/Pipe/Tracker.h b/MobileGL/MG_Impl/Pipe/Tracker.h index b3a9ea8c..ca9bc2eb 100644 --- a/MobileGL/MG_Impl/Pipe/Tracker.h +++ b/MobileGL/MG_Impl/Pipe/Tracker.h @@ -369,7 +369,15 @@ namespace MobileGL::MG_Pipe { program->GetBlockBindingVersion()), program->GetUniformWriteSetVersion()); constants = MGPipeMixShutter(program->GetLifetimeId(), program->GetUBOContentVersion()); - programImages = program->GetImageUnitVersion(); + // THE IDENTITY IS MIXED IN (P4a fable seam F-2), exactly as the pipeline arm + // below mixes stageLinks into its half: the counter alone is a per-program + // number two programs routinely share - 0 == 0 for any pair that never moved an + // image unit through glUniform1i, and 0 == 0 against no program at all - so a + // glUseProgram between them fired nothing, set_shader_images' window stayed the + // previous program's, and a program whose only image is a BUFFER image (E's + // SD-4: nothing else moves between the bind and the dispatch) never reached the + // record at all. + programImages = MGPipeMixShutter(shader, program->GetImageUnitVersion()); opaqueUnits = MGPipeMixShutter(shader, program->GetBackendStateVersion()); } else if (const auto& pipeline = ctx.GetBoundProgramPipeline(); pipeline) { using Pipeline = MG_State::GLState::ProgramPipelineObject; diff --git a/MobileGL/MG_IntegrationTest/Scenarios/P4aSeamAuditScenario.cpp b/MobileGL/MG_IntegrationTest/Scenarios/P4aSeamAuditScenario.cpp index 605b6226..afb38c2f 100644 --- a/MobileGL/MG_IntegrationTest/Scenarios/P4aSeamAuditScenario.cpp +++ b/MobileGL/MG_IntegrationTest/Scenarios/P4aSeamAuditScenario.cpp @@ -569,5 +569,84 @@ void main() { imageStore(i1, 0, imageLoad(i0, 0) + uvec4(2u, 0u, 0u, 0u)); } EXPECT_EQ(FirstGLError(), 0u) << GLErrorName(FirstGLError()); } + // ----------------------------------------------------------------------------------- + // F-2 / SD-4: the image window follows the program, through buffer images + // ----------------------------------------------------------------------------------- + // + // Public-GL half: both dispatches store what they should (every arm passes this - the + // server's window/high-water union takes the pre-handle bind for a unit the record does + // not cover, which is exactly why the seam was silent). White-box half, on Espryt's handle + // arm: after the first dispatch set_shader_images must have arrived with a window of ONE + // unit (SD-4: on the tree the audit read a buffer image never reached the record at all - + // the null -> program transition moved nothing bit 14 read), and after the switch to the + // program naming two units the window must be TWO (F-2: the two programs' image-unit + // counters are equal, so the switch alone moved nothing either). + TEST_F(P4aSeamAuditScenario, AProgramSwitchWithEqualImageUnitCountersMovesTheImageWindow) { + if (!Ready()) return; + if (!ComputeImagesAreUsable()) GTEST_SKIP() << "no compute image units / buffer textures on this host"; + + std::string error; + const GLuint one = MakeComputeProgram(kOneBufferImageCS, &error); + ASSERT_NE(one, 0u) << error; + const GLuint two = MakeComputeProgram(kTwoBufferImagesCS, &error); + ASSERT_NE(two, 0u) << error; + + GLuint buffer0 = 0; + GLuint buffer1 = 0; + const GLuint image0 = MakeBufferTexture(&buffer0, 0u); + const GLuint image1 = MakeBufferTexture(&buffer1, 0u); + ASSERT_EQ(FirstGLError(), 0u) << "buffer texture setup left a GL error behind"; + + // Both units bound BEFORE any dispatch, so the bind generation does not move between + // the two dispatches and the only thing that changes is the program in use. + glBindImageTexture(0, image0, 0, GL_FALSE, 0, GL_READ_WRITE, GL_R32UI); + glBindImageTexture(1, image1, 0, GL_FALSE, 0, GL_READ_WRITE, GL_R32UI); + ASSERT_EQ(FirstGLError(), 0u) << "binding the buffer images left a GL error behind"; + + const bool whiteBox = SamplerHandleArmIsLive("F-2 / SD-4"); + + glUseProgram(one); + glDispatchCompute(1, 1, 1); + glMemoryBarrier(GL_ALL_BARRIER_BITS); + EXPECT_EQ(FirstGLError(), 0u) << "the first dispatch leaked a GL error"; + if (whiteBox) { + PipeShaderImageWindowPeek window{}; + ASSERT_TRUE(PeekPipeShaderImageWindow(&window)); + EXPECT_EQ(window.Start, 0u); + EXPECT_EQ(window.Count, 1u) + << "set_shader_images never arrived for a program whose only image is a BUFFER " + "image (SD-4): the null -> program transition moved nothing bit 14 read"; + } + + glUseProgram(two); + glDispatchCompute(1, 1, 1); + glMemoryBarrier(GL_ALL_BARRIER_BITS); + EXPECT_EQ(FirstGLError(), 0u) << "the second dispatch leaked a GL error"; + if (whiteBox) { + PipeShaderImageWindowPeek window{}; + ASSERT_TRUE(PeekPipeShaderImageWindow(&window)); + EXPECT_EQ(window.Start, 0u); + EXPECT_EQ(window.Count, 2u) + << "the image window did not follow the program switch: two programs with equal " + "image-unit counters, and bit 14 mixed only the counter (F-2)"; + } + + EXPECT_EQ(ReadBufferTexel0(buffer0), 7u) << "the first program's store did not land"; + EXPECT_EQ(ReadBufferTexel0(buffer1), 9u) << "the second program's store did not land"; + + glBindImageTexture(0, 0, 0, GL_FALSE, 0, GL_READ_ONLY, GL_R32UI); + glBindImageTexture(1, 0, 0, GL_FALSE, 0, GL_READ_ONLY, GL_R32UI); + glBindBuffer(GL_TEXTURE_BUFFER, 0); + glBindTexture(GL_TEXTURE_BUFFER, 0); + glUseProgram(0); + glDeleteProgram(one); + glDeleteProgram(two); + glDeleteTextures(1, &image0); + glDeleteTextures(1, &image1); + glDeleteBuffers(1, &buffer0); + glDeleteBuffers(1, &buffer1); + EXPECT_EQ(FirstGLError(), 0u) << GLErrorName(FirstGLError()); + } + } // namespace } // namespace MGITest diff --git a/MobileGL/MG_Pipe/DirtySurface.def b/MobileGL/MG_Pipe/DirtySurface.def index e2ae07b0..641ad53e 100644 --- a/MobileGL/MG_Pipe/DirtySurface.def +++ b/MobileGL/MG_Pipe/DirtySurface.def @@ -300,14 +300,15 @@ /* UseProgram is bit 6's whole subject: the shutter is */ \ /* Mix(GetCurrentProgram()->GetLifetimeId(), GetLinkVersion()) and glUseProgram is */ \ /* what moves the object it reads through. Two call sites. AND SINCE THE FABLE */ \ - /* SEAM ROUND (F-1) IT IS BIT 12's TOO: set_sampler_views is resolved for the */ \ - /* program in use, so its shutter mixes the same identity bit 6 reads, and a */ \ - /* glUseProgram alone moves both. Undecided for the same reason as bit 6 (the */ \ - /* taint below), marked the same way. */ \ + /* SEAM ROUND (F-1 / F-2) IT IS BITS 12 AND 14's TOO: set_sampler_views is */ \ + /* resolved for the program in use and set_shader_images' window is the highest */ \ + /* image unit the program in use names, so both shutters mix the same identity */ \ + /* bit 6 reads, and a glUseProgram alone moves all three. Undecided for the same */ \ + /* reason as bit 6 (the taint below), marked the same way. */ \ /* BindVertexArray is bit 5's, for the same reason one level down: the shutter mixes */ \ /* the bound VAO's identity with its configuration version, and this is the bind. */ \ /* Three call sites. */ \ - X(UseProgram, NEW_SHADER|NEW_SAMPLER_VIEWS) \ + X(UseProgram, NEW_SHADER|NEW_SAMPLER_VIEWS|NEW_SHADER_IMAGES) \ X(BindVertexArray, NEW_VERTEX_ELEMENTS) \ /* NOT NEW_SHADER, and the derivation refutes it outright rather than leaving it a */ \ /* judgement: this mutator writes m_boundProgramPipeline (plus the pipeline name table) */ \ @@ -417,6 +418,7 @@ #define MGP_DIRTY_SURFACE_UNDECIDED_LIST(X) \ X(UseProgram, NEW_SHADER) \ X(UseProgram, NEW_SAMPLER_VIEWS) \ + X(UseProgram, NEW_SHADER_IMAGES) \ X(BindVertexArray, NEW_VERTEX_ELEMENTS) // clang-format on diff --git a/MobileGL/MG_Test/Pipe/TrackerTest.cpp b/MobileGL/MG_Test/Pipe/TrackerTest.cpp index c74afafc..66952e9e 100644 --- a/MobileGL/MG_Test/Pipe/TrackerTest.cpp +++ b/MobileGL/MG_Test/Pipe/TrackerTest.cpp @@ -84,6 +84,7 @@ namespace { X(TrackerAggregates, ARenderbufferStorageDefinitionMovesTheFramebufferAggregate) \ X(TrackerWalk, AProgramSwitchAloneFiresTheSamplerViewBit) \ X(TrackerWalk, ATextureParameterAloneFiresTheSamplerViewBit) \ + X(TrackerWalk, AProgramSwitchBetweenEqualImageUnitCountersFiresTheShaderImageBit) \ X(TrackerAttribPayload, AFloatWriteCarriesTheFloatBitsAndNamesItsClass) \ X(TrackerAttribPayload, AnIntWriteCarriesTheIntWordsAndNamesItsClass) \ X(TrackerAttribPayload, AUintWriteCarriesTheUintWordsAndNamesItsClass) \ @@ -826,6 +827,30 @@ namespace { EXPECT_EQ(Walk(), 0u); } + // P4a FABLE SEAM F-2 (and E's SD-4, which is this bit through a buffer image). Bit 14's + // plain-program arm mixed GetImageUnitVersion() ALONE - a per-program counter that two + // programs routinely share, 0 == 0 for any pair that never moved an image unit through + // glUniform1i - so a glUseProgram between them fired nothing and set_shader_images' window + // stayed the previous program's. The pipeline arm already mixed stageLinks; the plain arm + // now mixes the same identity bit 6 reads. + TEST_F(TrackerWalk, AProgramSwitchBetweenEqualImageUnitCountersFiresTheShaderImageBit) { + const Uint first = Ctx().CreateProgram(); + const Uint second = Ctx().CreateProgram(); + ASSERT_EQ(Ctx().GetProgramObject(first)->GetImageUnitVersion(), + Ctx().GetProgramObject(second)->GetImageUnitVersion()) + << "the premise of this case is two programs whose image-unit counters are equal"; + Ctx().UseProgram(first); + Walk(); + ASSERT_EQ(Walk(), 0u) << "the fixture did not reach a steady state"; + + Ctx().UseProgram(second); + const Uint32 dirty = Walk(); + EXPECT_NE(dirty & MGPipeDirtyBit(MGPipeDirty::NewShaderImages), 0u) + << "set_shader_images' window is the program's and a glUseProgram alone did not " + "re-emit it (F-2)"; + EXPECT_EQ(Walk(), 0u) << "the widened shutter fires forever"; + } + // =================================================================================== // set_vertex_attrib_defaults' payload (P2 brief D10) // ===================================================================================