diff --git a/MobileGL/MG_Impl/Pipe/Tracker.h b/MobileGL/MG_Impl/Pipe/Tracker.h index 283115c9..b3a9ea8c 100644 --- a/MobileGL/MG_Impl/Pipe/Tracker.h +++ b/MobileGL/MG_Impl/Pipe/Tracker.h @@ -349,6 +349,18 @@ namespace MobileGL::MG_Pipe { Uint64 bindings = 0; Uint64 constants = 0; Uint64 programImages = 0; + // THE PROGRAM INPUT OF THE PROGRAM-RESOLVED VIEW SET (P4a fable seam F-1). + // set_sampler_views is resolved for the program in use (SamplerEmit.h: the sampler + // uniform's TYPE picks which of a unit's targets is the view) and the emitter + // memoises that resolution on (lifetime id, link version, backend state version). A + // shutter that read only the texture generations therefore missed a glUseProgram: + // `glBindTexture x N; glUseProgram(P1); draw; glUseProgram(P2); draw` moved nothing + // bit 12 read, so the view set stayed P1's - and E's record epoch, keyed on the two + // set serials, then never rebuilt the texture sync list for P2 either. This value is + // that memo key, and bit 12 mixes it in below: over-firing costs one re-resolution + // the set-hash suppressor absorbs, under-firing left the record describing the + // previous program's units. + Uint64 opaqueUnits = 0; if (program) { shader = MGPipeMixShutter(program->GetLifetimeId(), program->GetLinkVersion()); bindings = MGPipeMixShutter( @@ -358,6 +370,7 @@ namespace MobileGL::MG_Pipe { program->GetUniformWriteSetVersion()); constants = MGPipeMixShutter(program->GetLifetimeId(), program->GetUBOContentVersion()); programImages = program->GetImageUnitVersion(); + opaqueUnits = MGPipeMixShutter(shader, program->GetBackendStateVersion()); } else if (const auto& pipeline = ctx.GetBoundProgramPipeline(); pipeline) { using Pipeline = MG_State::GLState::ProgramPipelineObject; // THE FIELDS ARE READ DIRECTLY RATHER THAN THROUGH THE TWO FUNCTIONS THAT @@ -401,6 +414,10 @@ namespace MobileGL::MG_Pipe { Uint64 stageLinks = static_cast(ctx.GetBoundProgramPipelineName()); Uint64 stageState = 0; Uint64 stageImages = 0; + // The per-stage sampler/image unit assignments alone (glUniform1i on a stage + // program's sampler moves its backend state version and reaches the composite + // through the uniform mirror), for bit 12's program input below. + Uint64 stageOpaque = 0; for (SizeT stage = 0; stage < Pipeline::kGraphicsStageCount; ++stage) { const auto& staged = pipeline->GetStageProgram(static_cast(stage)); if (!staged) continue; @@ -412,12 +429,14 @@ namespace MobileGL::MG_Pipe { staged->GetBlockBindingVersion())), staged->GetUniformWriteSetVersion()); stageImages = MGPipeMixShutter(stageImages, staged->GetImageUnitVersion()); + stageOpaque = MGPipeMixShutter(stageOpaque, staged->GetBackendStateVersion()); } shader = stageLinks; stageState = MGPipeMixShutter(stageLinks, stageState); bindings = MGPipeMixShutter(stageState, stageImages); constants = stageState; programImages = MGPipeMixShutter(stageLinks, stageImages); + opaqueUnits = MGPipeMixShutter(stageLinks, stageOpaque); } now[Index(MGPipeDirty::NewShader)] = shader; now[Index(MGPipeDirty::NewShaderBindings)] = bindings; @@ -491,8 +510,16 @@ namespace MobileGL::MG_Pipe { ctx.GetFramebufferBindingSlot(FramebufferTarget::Draw).GetVersion())), m_readFramebufferBind.Observe( ctx.GetFramebufferBindingSlot(FramebufferTarget::Read).GetVersion())); - now[Index(MGPipeDirty::NewSamplerViews)] = - MGPipeMixShutter(textureContent, ctx.GetTextureBindGeneration()); + // Bit 12 reads FOUR things (F-1): the two texture aggregates, the bind generation + // and the program input computed above. The params aggregate is here because + // SamplerEmit.h drops a unit's view to null when SamplesAsIncompleteTexture says so, + // and that predicate reads the effective sampler's filters - a glTexParameteri( + // MIN_FILTER) that completes a texture fired bit 13 and not this one, so the entry + // stayed null. The program input is here because the set is resolved FOR THE + // PROGRAM IN USE, and a glUseProgram alone moved nothing this shutter read. + now[Index(MGPipeDirty::NewSamplerViews)] = MGPipeMixShutter( + MGPipeMixShutter(MGPipeMixShutter(textureContent, textureParams), ctx.GetTextureBindGeneration()), + opaqueUnits); // Bit 13, WIDENED AT P4a FOR BIT 11's REASON and found the same way. glBindSampler // moves NEITHER half of what this used to read: GL_Sampler.cpp's BindSampler_State // goes through NoteTextureUnitTouched and TextureUnit::SetSamplerObject, and both diff --git a/MobileGL/MG_IntegrationTest/Scenarios/P4aSeamAuditScenario.cpp b/MobileGL/MG_IntegrationTest/Scenarios/P4aSeamAuditScenario.cpp index 04df4db1..605b6226 100644 --- a/MobileGL/MG_IntegrationTest/Scenarios/P4aSeamAuditScenario.cpp +++ b/MobileGL/MG_IntegrationTest/Scenarios/P4aSeamAuditScenario.cpp @@ -471,5 +471,103 @@ void main() { imageStore(i1, 0, imageLoad(i0, 0) + uvec4(2u, 0u, 0u, 0u)); } EXPECT_EQ(FirstGLError(), 0u) << GLErrorName(FirstGLError()); } + // ----------------------------------------------------------------------------------- + // F-1 / F-1b: a program switch re-resolves the view set, and the texture sync list with it + // ----------------------------------------------------------------------------------- + // + // The sequence the audit named, and every step of it is ordinary: two programs sampling two + // different units, a texture bound to a unit's EMPTY 2D slot - the unit was already touched + // through another target, so the high-water mark does not move - while a program that does + // not sample it is in use, then the switch to the one that does. Nothing between the two + // draws touches a parameter, a level or a populated slot, which is exactly what leaves the + // record epoch - and the program-independent texture sync list keyed on it - unmoved on + // the tree the audit read: the second program sampled an unbound unit and drew black. + TEST_F(P4aSeamAuditScenario, ATextureBoundToAnEmptySlotUnderOneProgramIsSampledByTheNext) { + if (!Ready()) return; + + std::string error; + const GLuint first = CompileProgram(kQuadVS, kFetchFS, &error); + ASSERT_NE(first, 0u) << error; + const GLuint second = CompileProgram(kQuadVS, kFetchFS, &error); + ASSERT_NE(second, 0u) << error; + glUseProgram(first); + glUniform1i(glGetUniformLocation(first, "uTex"), 0); + glUseProgram(second); + glUniform1i(glGetUniformLocation(second, "uTex"), 1); + glUseProgram(0); + + // Every texture exists, complete, with its parameters set, BEFORE the first draw: a + // parameter or a level defined between the two draws would move the sampling-resolution + // generation and rescue the list by accident. + const GLuint red = MakeSolidTexture2D(255, 0, 0); + const GLuint green = MakeSolidTexture2D(0, 255, 0); + GLuint touch3D = 0; + glGenTextures(1, &touch3D); + glBindTexture(GL_TEXTURE_3D, touch3D); + const std::uint8_t blue[2 * 2 * 2 * 4] = {0, 0, 255, 255, 0, 0, 255, 255, 0, 0, 255, 255, 0, 0, 255, 255, + 0, 0, 255, 255, 0, 0, 255, 255, 0, 0, 255, 255, 0, 0, 255, 255}; + glTexImage3D(GL_TEXTURE_3D, 0, GL_RGBA8, 2, 2, 2, 0, GL_RGBA, GL_UNSIGNED_BYTE, blue); + glTexParameteri(GL_TEXTURE_3D, GL_TEXTURE_MIN_FILTER, GL_NEAREST); + glTexParameteri(GL_TEXTURE_3D, GL_TEXTURE_MAG_FILTER, GL_NEAREST); + glBindTexture(GL_TEXTURE_3D, 0); + glBindTexture(GL_TEXTURE_2D, 0); + ASSERT_EQ(FirstGLError(), 0u) << "texture setup left a GL error behind"; + + ColorFbo target = MakeColorFbo(kSize, kSize); + ASSERT_NE(target.fbo, 0u); + BindFbo(target); + glBindVertexArray(m_vao); + + // Unit 1 is TOUCHED through its 3D slot; its 2D slot stays empty. Unit 0 holds red. + // The first program is in use BEFORE the first verb (the clear), so the very first + // view set that goes out is already resolved for it - measured: with no program in + // use at the clear the first set is [null, null], and the bind below then re-resolves + // to [red, null], a DIFFERENT set that moves the serial and rescues the case by + // accident. + glActiveTexture(GL_TEXTURE1); + glBindTexture(GL_TEXTURE_3D, touch3D); + glActiveTexture(GL_TEXTURE0); + glBindTexture(GL_TEXTURE_2D, red); + glUseProgram(first); + ClearTo(0.0f, 0.0f, 1.0f, 1.0f); + DrawQuad(); + + // THE BIND ONTO THE EMPTY SLOT, under a program that does not sample unit 1, and a + // draw with THAT program so the bind's own re-resolution of the view set happens under + // it (the bind generation fires bit 12 at the next verb; a switch inside the same verb + // gap would let that fire resolve under the second program by accident) ... + glActiveTexture(GL_TEXTURE1); + glBindTexture(GL_TEXTURE_2D, green); + glActiveTexture(GL_TEXTURE0); + DrawQuad(); + // ... and THE SWITCH to the one that does sample it. No other state moves. + glUseProgram(second); + DrawQuad(); + EXPECT_EQ(FirstGLError(), 0u) << "the two draws left a GL error behind"; + + const Image image = ReadPixels(kSize, kSize); + ASSERT_FALSE(image.Empty()); + EXPECT_TRUE(RegionIsMostly(image, kInset, kSize - 1 - kInset, kInset, kSize - 1 - kInset, "green", 0.0, + "the draw after the program switch")) + << "black means the second program sampled an unbound unit: the texture bound to the " + "empty slot was never synced because the view set - and E's record epoch with it - " + "did not move on the program switch (F-1 / F-1b); red means the first program's " + "set was still in force"; + + glActiveTexture(GL_TEXTURE1); + glBindTexture(GL_TEXTURE_2D, 0); + glBindTexture(GL_TEXTURE_3D, 0); + glActiveTexture(GL_TEXTURE0); + glBindTexture(GL_TEXTURE_2D, 0); + glUseProgram(0); + DestroyColorFbo(target); + glDeleteTextures(1, &red); + glDeleteTextures(1, &green); + glDeleteTextures(1, &touch3D); + glDeleteProgram(first); + glDeleteProgram(second); + EXPECT_EQ(FirstGLError(), 0u) << GLErrorName(FirstGLError()); + } + } // namespace } // namespace MGITest diff --git a/MobileGL/MG_Pipe/DirtySurface.def b/MobileGL/MG_Pipe/DirtySurface.def index e5c0c69c..e2ae07b0 100644 --- a/MobileGL/MG_Pipe/DirtySurface.def +++ b/MobileGL/MG_Pipe/DirtySurface.def @@ -299,11 +299,15 @@ /* begins with one of the ten words the pattern matched. */ \ /* 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. */ \ + /* 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. */ \ /* 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) \ + X(UseProgram, NEW_SHADER|NEW_SAMPLER_VIEWS) \ 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) */ \ @@ -387,11 +391,12 @@ // row nor outlive its reason. // // IT WAS EMPTY UNTIL P4a, and it stops being empty for a reason that is a property of the -// SCANNER rather than of the two rows. Both entries below are bit answers that are plainly -// true - glUseProgram is what moves the object bit 6's shutter reads through, and -// glBindVertexArray is what moves the object bit 5's shutter reads through - and the write -// analysis cannot say so, because each of them reaches, BY NAME, a body that writes a member -// with no m_ prefix: +// SCANNER rather than of the rows. Every entry below is a bit answer that is plainly true - +// glUseProgram is what moves the object bits 6, 12 and 14's shutters read through (the +// program in use; bits 12 and 14 since the fable seam round, F-1 / F-2), and glBindVertexArray +// is what moves the object bit 5's shutter reads through - and the write analysis cannot say +// so, because each of the two mutators reaches, BY NAME, a body that writes a member with no +// m_ prefix: // // UseProgram -> DestroyProgramSlot() writes `attachedShaders` // BindVertexArray -> a call spelled `Bind(` resolves to every body of that name, one of @@ -403,13 +408,15 @@ // never be claimed about code the script could not read. Widening the taint rule to ignore // non-m_ writes would weaken the one mechanism that catches a genuine under-fire, so the rows // are MARKED, with the tool's own reason, rather than the tool being made more permissive. -// Control 9c is what proves a marked row still needs the mark, and control 18 is what fails -// the moment either of these becomes decidable and the mark outlives its reason. +// Control 21 is what proves every marked row still needs its mark (it reads this list, so a +// row that gains a bit here is counted rather than assumed), and control 18 is what fails the +// moment any of them becomes decidable and the mark outlives its reason. // // The ten mutators that reach a tainted body (--check prints the count) all carry a prose -// answer, which no derivation checks; these two are the first that carry a bit answer. +// answer, which no derivation checks; these two mutators are the first that carry a bit answer. #define MGP_DIRTY_SURFACE_UNDECIDED_LIST(X) \ X(UseProgram, NEW_SHADER) \ + X(UseProgram, NEW_SAMPLER_VIEWS) \ 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 dbd3a5a3..c74afafc 100644 --- a/MobileGL/MG_Test/Pipe/TrackerTest.cpp +++ b/MobileGL/MG_Test/Pipe/TrackerTest.cpp @@ -82,6 +82,8 @@ namespace { X(TrackerWalk, UseProgramZeroLeavesTheBoundPipelineDrivingTheProgramBits) \ X(TrackerAggregates, ATextureStorageDefinitionMovesTheFramebufferAggregateToo) \ X(TrackerAggregates, ARenderbufferStorageDefinitionMovesTheFramebufferAggregate) \ + X(TrackerWalk, AProgramSwitchAloneFiresTheSamplerViewBit) \ + X(TrackerWalk, ATextureParameterAloneFiresTheSamplerViewBit) \ X(TrackerAttribPayload, AFloatWriteCarriesTheFloatBitsAndNamesItsClass) \ X(TrackerAttribPayload, AnIntWriteCarriesTheIntWordsAndNamesItsClass) \ X(TrackerAttribPayload, AUintWriteCarriesTheUintWordsAndNamesItsClass) \ @@ -786,6 +788,44 @@ namespace { EXPECT_NE(dirty & MGPipeDirtyBit(MGPipeDirty::NewGlobalConstants), 0u); } + // P4a FABLE SEAM F-1. set_sampler_views is resolved for the PROGRAM IN USE (the sampler + // uniform's type picks which of a unit's targets is the view), and bit 12's shutter read + // only the texture-content aggregate and the bind generation - so `glUseProgram(P1); draw; + // glUseProgram(P2); draw` never re-emitted the set and the record went on describing P1's + // units. A program switch alone, with no bind and no texture change, has to fire it. + TEST_F(TrackerWalk, AProgramSwitchAloneFiresTheSamplerViewBit) { + const Uint first = Ctx().CreateProgram(); + const Uint second = Ctx().CreateProgram(); + 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::NewSamplerViews), 0u) + << "the view set is resolved for the program in use and a glUseProgram alone did not " + "re-emit it (F-1)"; + EXPECT_EQ(Walk(), 0u) << "the widened shutter fires forever"; + } + + // The other input F-1 added: the params aggregate. SamplerEmit.h drops a unit's view to + // null when SamplesAsIncompleteTexture says so, and that predicate reads the effective + // sampler's filters and the level range - a glTexParameteri that completes a texture fired + // bit 13 and left the view entry null. + TEST_F(TrackerWalk, ATextureParameterAloneFiresTheSamplerViewBit) { + const auto& tex = Ctx().CreateTextureObject(1, TextureTarget::Texture2D); + ASSERT_TRUE(tex != nullptr); + Walk(); + ASSERT_EQ(Walk(), 0u) << "the fixture did not reach a steady state"; + + tex->SetMaxLevel(4); + const Uint32 dirty = Walk(); + EXPECT_NE(dirty & MGPipeDirtyBit(MGPipeDirty::NewSamplerViews), 0u) + << "completeness is a view-set input and a parameter change did not re-resolve it"; + EXPECT_NE(dirty & MGPipeDirtyBit(MGPipeDirty::NewSamplers), 0u); + EXPECT_EQ(Walk(), 0u); + } + // =================================================================================== // set_vertex_attrib_defaults' payload (P2 brief D10) // =================================================================================== diff --git a/scripts/gen_pipe_dirty_surface.py b/scripts/gen_pipe_dirty_surface.py index 2b0c296f..e1d0d6c9 100644 --- a/scripts/gen_pipe_dirty_surface.py +++ b/scripts/gen_pipe_dirty_surface.py @@ -1811,17 +1811,21 @@ def self_test(scanned, bits, publishers, movers, moved, outside=None, undecided_ "20%s (the %s row is STALE the moment the scan stops finding it)" % ("abcd"[index], name)) - # 21. THE TWO UNDECIDED MARKS ARE STILL LOAD-BEARING. Dropping them has to make --check - # refuse both rows as unmarked UNDECIDED - which is what says the marks are covering a - # real blind spot rather than a verdict the analysis could give today. Control 18 is - # the other direction: a mark the derivation DOES decide is itself a problem, so - # neither of these can outlive its reason. + # 21. EVERY UNDECIDED MARK IS STILL LOAD-BEARING. Dropping them all has to make --check + # refuse EVERY marked (mutator, bit) as an unmarked UNDECIDED, and nothing else - which + # is what says each mark is covering a real blind spot rather than a verdict the + # analysis could give today. Control 18 is the other direction: a mark the derivation + # DOES decide is itself a problem, so no mark can outlive its reason. Read from the + # file's own list rather than spelled here, so a row that gains a bit - UseProgram + # gained NEW_SAMPLER_VIEWS and NEW_SHADER_IMAGES at the P4a fable seam round - cannot + # silently turn this control into one that counts the wrong number. problems, _, _, undecided_rows = object_class_problems(real, bits, movers, moved, outside, {}) - tripped(any(p.startswith("UNDECIDED answer NEW_SHADER for UseProgram") for p in problems) - and any(p.startswith("UNDECIDED answer NEW_VERTEX_ELEMENTS for BindVertexArray") - for p in problems) - and len(undecided_rows) == 2, - "21 (the two P4a undecided marks are still needed)") + marked_pairs = sorted((mutator, bit) for mutator, marks in real_marks.items() for bit in marks) + tripped(marked_pairs + and all(any(p.startswith("UNDECIDED answer %s for %s" % (bit, mutator)) for p in problems) + for mutator, bit in marked_pairs) + and len(undecided_rows) == len(marked_pairs), + "21 (every P4a undecided mark - %d of them - is still needed)" % len(marked_pairs)) # THE POSITIVE CONTROLS. (a) The row that was wrong in round 3: SetPixelStoreParam writes # NEW_PIXEL_PACK's shutter member sixteen times, through a token-pasting macro; it has