From f0f8cab682a7a399c6e45b3368ca933b25dff322 Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Tue, 8 Sep 2026 14:28:07 -0400 Subject: [PATCH] [Fix] (Pipe): give the program family its birth-hook entry point and publication latch, invalidate the global-constants key whenever a create_shader_state is re-issued, and count a truncated module tail instead of asserting it --- MobileGL/MG_Impl/Pipe/ProgramEmit.h | 94 ++++++++++++++++++++--- MobileGL/MG_Test/Pipe/ProgramEmitTest.cpp | 63 ++++++++++++++- 2 files changed, 146 insertions(+), 11 deletions(-) diff --git a/MobileGL/MG_Impl/Pipe/ProgramEmit.h b/MobileGL/MG_Impl/Pipe/ProgramEmit.h index 1e4866e4..cab9302e 100644 --- a/MobileGL/MG_Impl/Pipe/ProgramEmit.h +++ b/MobileGL/MG_Impl/Pipe/ProgramEmit.h @@ -41,6 +41,7 @@ #include #include #include +#include #include #include #include @@ -49,9 +50,16 @@ namespace MobileGL::MG_Pipe { // WIRED. create/bind/delete_shader_state, set_draw_program, set_dispatch_program and // set_global_constants all have bodies, so this family contributes its bit to - // kMGPipeWiredSubsystems. See SamplerEmit.h's note for what the bit does and does not do: - // it states what this build emits for, and the EMISSION gate is the runtime - // MOBILEGL_PIPE_PUSH mask through the validate point's `wants()`, not this constant. + // kMGPipeWiredSubsystems. + // + // AND SINCE c0b THAT CONSTANT REALLY IS PART OF THE EMISSION GATE, so the note that used to + // say otherwise here was true only against the contract commit: the validate point's + // `wants()` asks the subsystem mapping, the operator's MOBILEGL_PIPE_PUSH mask, THIS + // CONSTANT and the dirty bit, and the birth hooks' `FamilyIsLive` asks the same pair one + // level in. It is also a compile-time contract - while it is non-zero PipeFill.cpp's + // `if constexpr` seam instantiates the forward to EmitShaderCso below, so a missing entry + // point is a build error here rather than at the merge. The RUNTIME A/B that switches the + // family off is still the mask. See SamplerEmit.h's twin note. inline constexpr Uint64 kMGPipeWiredProgramSubsystem = kMGPipeSubsystemPrograms; // D-H6. ~0u is the BACKENDS' "never uploaded" sentinel for a global-constants version, and @@ -221,10 +229,15 @@ namespace MobileGL::MG_Pipe { // Blob rule); Offset carries the module's staging address so a reader can see which // slots are occupied without the record pretending to declare a length it does not // own. + // + // A COUNTED REFUSAL AND NOT AN ASSERTION (D-J3). MOBILEGL_ASSERT compiles out at + // INFO, which is all three gate builds and every shipped build, so an assert here + // would leave the truncation below completely silent in exactly the builds that + // run - which is the idiom D-J3 exists to forbid. generatedSpirv cannot exceed six + // stages today, so this is a guard against a seventh; truncation is the safe + // direction and the counter is what makes it visible. const SizeT moduleCount = spirv.generatedSpirv.size(); - MOBILEGL_ASSERT(moduleCount <= 6, - "MGPProgramDesc::Spirv[] carries six modules and this program linked %u", - static_cast(moduleCount)); + if (moduleCount > 6) ++m_moduleTruncations; for (SizeT i = 0; i < moduleCount && i < 6; ++i) { m_lastDesc.Spirv[i].Seg = kMGHostSpanSegNone; m_lastDesc.Spirv[i].Offset = reinterpret_cast(spirv.generatedSpirv[i].data()); @@ -235,19 +248,60 @@ namespace MobileGL::MG_Pipe { m_lastDesc.Reflection.Size = 0; MGPipeApplyCreateShaderState(m_lastDesc, &link, &spirv); + // THE CREATE WENT OUT, so the publication latch is taken here and nowhere else + // (contract-v2 §3.1). MGPipeEmitShaderCsoDestroyAndFree reads it, and without it + // delete_shader_state can never go out - for an ordinary program or for a + // composite, both of which take that one helper. + MGPipeNoteHandlePublished(MGPipeKind::ShaderCso, handle); ++m_creates; payloadBytes += sizeof(MGPProgramDesc); + // A RE-ISSUED create_shader_state CLEARS THE APPLIER's DEFAULT UNIFORM BLOCK (wire + // W6), so the (Cso, Version) latch that suppresses set_global_constants has to go + // with it or the block is never re-sent. The case the design worries about is a + // FAILED relink of a bound program - GL keeps the previous executable and its + // uniforms running - and the general one is any future re-issue trigger that does + // not happen to move the content version, of which a recycled slot is one. + // Invalidated rather than re-emitted here, because this function has no business + // deciding when the constants go out: the next EmitGlobalConstants sees an + // unlatched key and sends them. + if (m_constantsCso == handle) { + m_constantsCso = kMGPipeNullHandle; + m_constantsVersion = kMGPipeGlobalConstantsNeverUploaded; + } + latch.RecordLive = true; latch.RecordGen = handle.Gen; latch.LinkVersion = linkVersion; return handle; } - // C-1's question for this kind - see MGPipeSamplerEmitter::RecordIsPublished for the - // full reason. A ShaderCso slot can exist with no record behind it, because a backend - // twin table mints one through MGPipeSlots().Acquire whether or not the program - // subsystem ever asked this client to emit a create. + // ---- THE CONTRACT ENTRY POINT THIS FAMILY OWES (contract-v2 §3.4) ---- + // + // PipeFill.cpp's MGPipeEmitShaderCsoCreate forwards here through the `if constexpr` + // seam keyed on kMGPipeWiredProgramSubsystem, so while that constant is non-zero this + // must exist and be spelled exactly like this. A thin wrapper on purpose: + // AcquireShaderCso above IS this family's handle rule - identity-addressed per + // ProgramObject, the composite band entered through the one door, the re-issue on the + // same handle and the publication - and a second copy of any of it here would be a + // second authority. + // + // THE HOOK HAS ALREADY APPLIED BOTH GATES (the operator's mask and the wired constant), + // so this body applies none of its own. The byte count is discarded: a birth is not a + // validate-point emission and has no payload budget to report into. + void EmitShaderCso(ProgramObject& program) { + Uint64 bytes = 0; + AcquireShaderCso(program, bytes); + } + + // The emitter's OWN record memo - "have I already published a create_shader_state at + // this slot, for this generation, at this link version". + // + // IT IS NOT WHAT THE DEATH PATH ASKS, and that changed at c0b (contract-v2 §3.1/D17): + // MGPipeEmitShaderCsoDestroyAndFree reads A's publication latch, which is one answer + // per {kind, slot, gen} that all six death helpers share. This stays because the + // VERSION-FIRST SKIP needs it - it is the same latch AcquireShaderCso consults before + // it builds a descriptor - and because a unit case reads it. // // THE COMPOSITE BAND IS INDEXED SEPARATELY, for the allocator's own reason: the band // base is 983040, so a slot-indexed vector would allocate ~983k latches for one program @@ -261,6 +315,16 @@ namespace MobileGL::MG_Pipe { return latch.RecordLive && latch.RecordGen == handle.Gen; } + // The memo's other half, and the bound-mirror clearing beside it. + // + // NO PRODUCTION CALLER TODAY, stated rather than implied: since c0b the death path + // reads the contract's latch and never asks an emitter. It is kept because the memo + // above needs a way to be told, and because everything it clears SELF-HEALS if it is + // not called - the slot's Gen moves on reuse, so `RecordGen == handle.Gen` refuses a + // stale record latch, and the three bound mirrors below hold a handle whose generation + // can never be handed out again, so the next EmitShaderState compares against a + // different handle and re-binds. Clearing them here is the cheaper answer, not the + // load-bearing one. void NoteRecordDestroyed(MGPipeHandle handle) { if (MGPipeHandleIsNull(handle)) return; Vector& table = TableOf(handle); @@ -302,11 +366,20 @@ namespace MobileGL::MG_Pipe { void ResetCounters() { m_creates = m_binds = m_drawSets = m_dispatchSets = m_constantSets = 0; + m_moduleTruncations = 0; } // ---- what a unit case reads ---- const MGPProgramDesc& LastProgramDesc() const { return m_lastDesc; } const MGPGlobalConstants& LastGlobalConstants() const { return m_lastConstants; } + // THE (Cso, Version) KEY set_global_constants is suppressed against. Exposed so a case + // can pin that a re-issued create_shader_state invalidates it - the applier clears the + // block on the re-issue (wire W6), so a latch that survived it would never re-send. + MGPipeHandle GlobalConstantsCso() const { return m_constantsCso; } + Uint32 GlobalConstantsVersion() const { return m_constantsVersion; } + // D-J3's counted refusal: programs whose linked snapshot carried more modules than + // MGPProgramDesc::Spirv[] can name, and whose tail was therefore dropped. + Uint64 TruncatedModuleCount() const { return m_moduleTruncations; } MGPipeHandle BoundCso() const { return m_boundCso; } MGPipeHandle DrawCso() const { return m_drawCso; } MGPipeHandle DispatchCso() const { return m_dispatchCso; } @@ -391,6 +464,7 @@ namespace MobileGL::MG_Pipe { Uint64 m_drawSets = 0; Uint64 m_dispatchSets = 0; Uint64 m_constantSets = 0; + Uint64 m_moduleTruncations = 0; }; inline MGPipeProgramEmitter& MGPipeProgramEmitterInstance() { diff --git a/MobileGL/MG_Test/Pipe/ProgramEmitTest.cpp b/MobileGL/MG_Test/Pipe/ProgramEmitTest.cpp index 01b073bf..e9626e50 100644 --- a/MobileGL/MG_Test/Pipe/ProgramEmitTest.cpp +++ b/MobileGL/MG_Test/Pipe/ProgramEmitTest.cpp @@ -483,7 +483,8 @@ TEST(ProgramEmit, TheProgramRecordSurvivesAMakeCurrentWhileTheThreeBindingsDoNot X(ProgramEmit, TheEmitterJoinsAndTheTrackerDoesNot) \ X(ProgramEmit, AReLinkReIssuesOnTheSameHandle) \ X(ProgramEmit, TheDrawAndDispatchProgramsAreTwoIndependentSlots) \ - X(ProgramEmit, AnUnchangedProgramEmitsNothingAtAll) + X(ProgramEmit, AnUnchangedProgramEmitsNothingAtAll) \ + X(ProgramEmit, AReIssuedCreateReSendsTheDefaultUniformBlock) #define MGL_DECLARE_PULL_SKIP(Suite, Name) \ TEST(Suite, Name) { GTEST_SKIP() << "compiled only under MOBILEGL_PIPE_PUSH"; } @@ -696,6 +697,66 @@ void main() { gl_Position = vec4(0.0); EmitVertex(); } EXPECT_EQ(Emitter().BindCount(), 1u); EXPECT_EQ(Emitter().DrawProgramSetCount(), 1u); } + + // set_global_constants is suppressed against a (ShaderCso, Version) key, and A RE-ISSUED + // create_shader_state CLEARS THE APPLIER's DEFAULT UNIFORM BLOCK (wire's W6). So the key + // has to die with the re-issue, or the block is never re-sent and the server draws the + // program with a zeroed one while the client's scratch still holds the live values. + // + // WHICH HALF OF THIS CASE IS THE DISCRIMINATOR, said plainly. The second half - "the block + // goes out again after a relink" - is also true without the fix, because + // BumpLinkObservableVersions bumps the UBO content version alongside the link version, so + // the Version half of the key moves on its own for the one re-issue trigger that exists + // today. The FIRST half is the one that goes red when the invalidation is deleted, and it + // is the property the contract actually needs: after a re-issue the emitter must hold NO + // key at all, so no future re-issue trigger - a recycled slot, a re-issue driven by + // anything that does not happen to move the content version - can leave the applier's + // cleared block latched as "already sent". + TEST(ProgramEmit, AReIssuedCreateReSendsTheDefaultUniformBlock) { + EmitterScope scope; + const GLuint name = MakeVsFsProgram(); + GL::UseProgram(name); + const SharedPtr& program = Ctx().GetProgramObject(name); + ASSERT_TRUE(program); + + ASSERT_GT(Emitter().EmitShaderState(Ctx()), 0u); + ASSERT_EQ(Emitter().CreateCount(), 1u); + // Driven to the fixed point rather than assumed to settle in one call: the first + // EmitGlobalConstants reads GetUBOContentVersion() BEFORE GetUBOSize() joins phase B, + // and that join bumps the counter, so the first key is one behind by construction. + for (int i = 0; i < 8 && Emitter().EmitGlobalConstants(Ctx()) > 0u; ++i) { + } + const Uint64 setsBefore = Emitter().GlobalConstantsSetCount(); + ASSERT_GT(setsBefore, 0u) << "this program declares a uniform, so a block must have gone out"; + const MGPipeHandle cso = Emitter().GlobalConstantsCso(); + ASSERT_FALSE(MGPipeHandleIsNull(cso)); + EXPECT_EQ(cso, Emitter().DrawCso()); + // The latch really is latched. + EXPECT_EQ(Emitter().EmitGlobalConstants(Ctx()), 0u); + EXPECT_EQ(Emitter().GlobalConstantsSetCount(), setsBefore); + + // A RELINK, which is the one thing that makes AcquireShaderCso re-issue on the same + // handle. A failed relink is the case the design worries about - GL keeps a program + // that is active for a stage running on its previous executable - and this frontend + // additionally clears the phase-B scratch in Link()'s prologue, so the populated-block + // half of that scenario is not reachable here; the re-issue is, and it is what the + // applier reacts to. + const Uint32 linkVersionBefore = program->GetLinkVersion(); + GL::LinkProgram(name); + ASSERT_NE(program->GetLinkVersion(), linkVersionBefore) << "the relink really has to move it"; + + Emitter().EmitShaderState(Ctx()); + ASSERT_EQ(Emitter().CreateCount(), 2u) << "the create really has to be re-issued"; + EXPECT_TRUE(MGPipeHandleIsNull(Emitter().GlobalConstantsCso())) + << "the applier cleared the record's default uniform block on the re-issue, so the " + "key that suppresses set_global_constants must not survive it"; + EXPECT_EQ(Emitter().GlobalConstantsVersion(), kMGPipeGlobalConstantsNeverUploaded); + + // And the block really is sent again. + for (int i = 0; i < 8 && Emitter().EmitGlobalConstants(Ctx()) > 0u; ++i) { + } + EXPECT_GT(Emitter().GlobalConstantsSetCount(), setsBefore); + } } // namespace #endif // MOBILEGL_PIPE_PUSH