From 5d4d91fe7ed6e2b4bc7fc128725fa5b9adb0ced8 Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Sun, 6 Sep 2026 10:25:25 -0400 Subject: [PATCH] [Fix] (Pipe): carry the class a vertex-attribute default was written through, and stop the applier's lossy write from being observable - set_vertex_attrib_defaults hard-coded MGPAttribValue::ValueClass to 0 for every attribute and always sent the FLOAT view's four words. A CurrentVertexAttributeValue is one value in three views and GLContext converts numerically between them, so those bytes cannot reproduce the frontend value: glVertexAttrib4f(loc, 1.5f, ..) leaves 1 in intValue and 0x3FC00000 in floatValue, and every glVertexAttribI4i/ui default was wrong too - GLContext now records which view each glVertexAttrib* write filled directly (GetCurrentVertexAttributeClass, push-only) and the payload carries that class and THAT class's own words. It is kept beside the value rather than inside it because CurrentVertexAttributeValue is mirrored into PipeInputs and compared there by a memcmp whose size assertion lives in a file this package does not own - MGPipeFillAttribValue is the flattening, in one named place, so TrackerAttribPayload can pin it: the old defect turns three of its four cases red - the applier (package A's) still memcpys the four words into all three views regardless of ValueClass, so the emitter now CHECKS: it compares the mirror the applier wrote against the frontend's value and, when they differ, copies the field itself and says so once. That closes the window the old code left wrong - a glVertexAttrib* write followed by a verb whose class does not read the field, where the residual fill does not run for it - and it stops repairing by itself the day A's applier honours the class - MGPipeVertexAttribDefaultRepairCount() makes that repair observable to a test without reading storage the fill table forbids that verb to read - the emission gate now goes through MGPipeSubsystemForDirty, the one bit-to-subsystem map, instead of a second copy of it written out by hand at the validate point; five static_asserts tie that map to the field-emitter map it has to agree with - the staging mirror is advanced only by the branch that sent dynamic bytes, with the invariant it used to rely on (BumpVersions moves both counters, RenderState.h) asserted here rather than assumed of another package's file - TrackerShippedEmitter drives MGPipeValidateForVerb itself and reads the real singletons back, so the blend-toggle and viewport shapes are pinned on the shipped emitter and not only on the unit tests' local re-implementation; the fixtures reset the applier, the cache and the tracker together, which is the only consistent state of the three - comments: the derivation probe is a one-field sample, the residual block's trip wire is half a tautology until package A's c1 lands, and the widened counter cannot see a change of exactly 65536 - all three recorded where the code is, not only in a review --- MobileGL/MG_Impl/Pipe/PipeFill.cpp | 159 +++++++++++++++++--- MobileGL/MG_Impl/Pipe/PipeFill.h | 13 ++ MobileGL/MG_Impl/Pipe/Tracker.h | 34 +++++ MobileGL/MG_State/GLState/Core.cpp | 11 ++ MobileGL/MG_State/GLState/Core.h | 39 +++++ MobileGL/MG_Test/Pipe/TrackerTest.cpp | 209 +++++++++++++++++++++++++- 6 files changed, 437 insertions(+), 28 deletions(-) diff --git a/MobileGL/MG_Impl/Pipe/PipeFill.cpp b/MobileGL/MG_Impl/Pipe/PipeFill.cpp index efb1fb71..ad032e91 100644 --- a/MobileGL/MG_Impl/Pipe/PipeFill.cpp +++ b/MobileGL/MG_Impl/Pipe/PipeFill.cpp @@ -43,6 +43,14 @@ namespace MobileGL::MG_Pipe { // path writes through them. static RenderStateParameters& RenderStateOf(PipeInputs& inputs) { return inputs.m_renderState; } static Uint32& ClearStencilOf(PipeInputs& inputs) { return inputs.m_clearStencil; } + // Read-only, and it exists for one thing: after set_vertex_attrib_defaults goes out, + // the emitter compares what the applier left here against what the frontend holds + // (EmitVertexAttribDefaults). Reading it through this door rather than through the + // accessor is deliberate - the accessor is poison-checked and this is a fill-time + // read, not a backend read. + static const PipeInputs::CurrentVertexAttributeValue* VertexAttribDefaultsOf(const PipeInputs& inputs) { + return inputs.m_currentVertexAttribute; + } static void CopyField(PipeInputs& dst, GLContext& ctx, MGPipeInputField field) { using F = MGPipeInputField; @@ -631,6 +639,27 @@ namespace MobileGL::MG_Pipe { return 0; } + // The two maps answer different questions - this one takes a field's EMITTER, the + // tracker's MGPipeSubsystemForDirty takes a dirty BIT - and they must agree, because + // the emission is gated on one and the residual-fill skip on the other. A divergence + // would push a call whose field is still pulled, or (worse) skip a field whose call + // was never emitted. Cheap to state, impossible to drift: + static_assert(SubsystemForEmitter(MGPipeFieldEmitter::BindRenderState) == + MGPipeSubsystemForDirty(MGPipeDirty::NewPipelineState), + "bind_render_state and NEW_PIPELINE_STATE must name one subsystem"); + static_assert(SubsystemForEmitter(MGPipeFieldEmitter::SetDynamicState) == + MGPipeSubsystemForDirty(MGPipeDirty::NewRenderState), + "set_dynamic_state and NEW_RENDER_STATE must name one subsystem"); + static_assert(SubsystemForEmitter(MGPipeFieldEmitter::SetPixelPackState) == + MGPipeSubsystemForDirty(MGPipeDirty::NewPixelPack), + "set_pixel_pack_state and NEW_PIXEL_PACK must name one subsystem"); + static_assert(SubsystemForEmitter(MGPipeFieldEmitter::SetPatchState) == + MGPipeSubsystemForDirty(MGPipeDirty::NewPatchState), + "set_patch_state and NEW_PATCH_STATE must name one subsystem"); + static_assert(SubsystemForEmitter(MGPipeFieldEmitter::SetVertexAttribDefaults) == + MGPipeSubsystemForDirty(MGPipeDirty::NewVertexAttribDefaults), + "set_vertex_attrib_defaults and NEW_VERTEX_ATTRIB_DEFAULTS must name one subsystem"); + // Which of those subsystems THIS BUILD actually emits for. It grows one commit at a // time, and a field whose emitter is not wired here keeps being pulled - so adding a // row to Coverage.def can never silently drop a field on the floor before the call @@ -651,13 +680,14 @@ namespace MobileGL::MG_Pipe { // // GetCurrentVertexAttribute's three views are NOT bit-identical: GLContext // CONVERTS between them (SetCurrentVertexAttributeFloat writes (Int32)value into - // intValue), while MGPipeApplySetVertexAttribDefaults memcpys one Data[4] into - // all three and ignores MGPAttribValue::ValueClass, which the wire type carries - // precisely so it does not have to. Until that applier reads ValueClass the - // carrier cannot reproduce the frontend value, so the field keeps being pulled. - // The call is still emitted: the wire shape, the payload bytes and the set-hash - // suppressor are all real, and the residual fill runs AFTER emission, so the - // mirror ends up with the frontend's value either way. + // intValue), while MGPipeApplySetVertexAttribDefaults (package A's) memcpys one + // Data[4] into all three views and ignores MGPAttribValue::ValueClass. The CLIENT + // half of that is fixed - the call now carries the class the frontend actually + // wrote and that class's own bytes - but the APPLIER still cannot reproduce the + // conversion, so this row stays SHAPE-ONLY: the field keeps being pulled, and + // retiring that pull is blocked on A teaching the applier to switch on + // ValueClass. EmitVertexAttribDefaults checks rather than trusts, and repairs the + // mirror when the applier's write does not reproduce the value. constexpr Bool EmittedCallSuppliesTheWholeField(MGPipeInputField field) { switch (field) { case MGPipeInputField::GetPixelStoreParameters: @@ -704,6 +734,13 @@ namespace MobileGL::MG_Pipe { // filler degrades to PULLING those fields instead of rendering a default, which is // the safe direction. The verify lane and RenderStateSpansTest are what say the // derivation is CORRECT; this only says it is THERE. + // + // AND IT IS A ONE-FIELD SAMPLE, deliberately: it probes m_clearStencil and nothing + // else, so a PARTIAL derivation - one that recomputes m_clearStencil and forgets, say, + // GetViewport's rounding - flips this latch to true and lets the other mirrors go + // unwritten. That is a real risk of a half-landed package A and the backstop for it is + // the verify lane (which re-reads every field at every backend read), not this probe. + // Widening the probe to all 29 would re-implement the derivation to check it. Bool ApplierDerivesRenderStateFields() { static const Bool answer = [] { static PipeInputs probe; @@ -749,6 +786,32 @@ namespace MobileGL::MG_Pipe { // 32 values, all three views - is hashed on the client and the call does not go out // when the hash has not moved. That is coalescing rule 4, and this is its one wired // consumer in P2. + // + // THE PAYLOAD AND ITS ONE MISSING HALF. A CurrentVertexAttributeValue is one value in + // three views, and GLContext CONVERTS between them numerically, so "the bytes of one + // view" is not the value: glVertexAttrib4f(loc, 1.5f, ...) leaves 1 in intValue and + // 0x3FC00000 in floatValue, and every glVertexAttribI4i/ui is a different pair again. + // MGPAttribValue carries ValueClass for exactly this reason, so the client sends the + // class the frontend actually wrote (GLContext::GetCurrentVertexAttributeClass) and + // THAT class's own four words. What is still missing is the other half: + // MGPipeApplySetVertexAttribDefaults (package A's file) memcpys the four words into + // all three views regardless of ValueClass, which cannot reproduce the conversion. + // + // The suppressing memcmp below is over the three VIEWS only, and that is not an + // oversight: the class decides how the views are REBUILT, so two writes that leave + // the three views identical rebuild identically whichever class they carried, and a + // class that moved without moving any view has nothing to publish. + // + // So the emitter CHECKS rather than assumes, the same self-healing shape as + // ApplierDerivesRenderStateFields: after the call it compares the mirror the applier + // wrote against the frontend's value, and when they differ it copies the field itself + // and says so once. That is what keeps the block correct in the window this call used + // to corrupt - a glVertexAttrib4f followed by a non-kDraw verb, where the residual + // fill does not run for this field and nothing else would have put the value back. + // The day the applier honours ValueClass the compare stops failing and the repair + // stops happening, with no edit here. + Uint64 g_attribDefaultRepairs = 0; + Uint64 EmitVertexAttribDefaults(GLContext& ctx) { MGPipeTracker& tracker = MGPipeTrackerInstance(); auto& staged = tracker.StagedAttribDefaults(); @@ -768,19 +831,37 @@ namespace MobileGL::MG_Pipe { MGPVertexAttribDefaults header{}; for (SizeT i = 0; i < kAttribs; ++i) { if (std::memcmp(&resolved[i], &staged[i], sizeof(resolved[i])) == 0) continue; - MGPAttribValue& value = tail[header.Count]; - value.Location = static_cast(i); - // ClassifyVertexAttribType resolves the float/int/uint view on the CLIENT - // (MGPipeTypes.h); the frontend keeps all three populated, so the class the - // shader input consumes is what decides which one is authoritative. - value.ValueClass = 0; - std::memcpy(value.Data, resolved[i].floatValue.data(), sizeof(value.Data)); + // The class the frontend WROTE, and that class's own bytes. Not a literal 0 + // and not ClassifyVertexAttribType's answer: that one is the SHADER's question + // ("which view does this input consume"), asked at the backend read sites, and + // it says nothing about which view holds the value the other two were + // converted from. + MGPipeFillAttribValue(static_cast(i), resolved[i], + ctx.GetCurrentVertexAttributeClass(static_cast(i)), + tail[header.Count]); header.Mask |= Uint32{1} << static_cast(i); ++header.Count; staged[i] = resolved[i]; } if (header.Count == 0) return 0; MGPipeApplySetVertexAttribDefaults(header, tail.data()); + + // Did the applier reproduce it? Byte for byte, over the attributes this call + // named - anything less would be a mirror that disagrees with the frontend in a + // window no gate looks at. + const auto* mirror = MGPipeFillAccess::VertexAttribDefaultsOf(gPipeInputs); + Bool reproduced = true; + for (SizeT i = 0; i < kAttribs && reproduced; ++i) { + if ((header.Mask & (Uint32{1} << static_cast(i))) == 0) continue; + reproduced = std::memcmp(&mirror[i], &resolved[i], sizeof(resolved[i])) == 0; + } + if (!reproduced) { + ++g_attribDefaultRepairs; + MGLOG_W_ONCE("MGPipe: MGPipeApplySetVertexAttribDefaults does not reproduce the " + "carried value on this build (it ignores MGPAttribValue::ValueClass) " + "- the client is keeping m_currentVertexAttribute authoritative"); + MGPipeFillAccess::CopyField(gPipeInputs, ctx, MGPipeInputField::GetCurrentVertexAttribute); + } return sizeof(MGPVertexAttribDefaults) + header.Count * sizeof(MGPAttribValue); } @@ -799,6 +880,15 @@ namespace MobileGL::MG_Pipe { // wire a tautology, which is exactly the failure P1's entry compare had and P2 is // paying to remove. // + // ON THIS BRANCH IT IS STILL HALF A TAUTOLOGY, and saying so is part of the honesty + // the trip wire is for: the applier compares these bits against gPipeInputs' + // capability mirror, and while MGPipeDeriveRenderStateFields is a stub that mirror is + // filled by the residual fill from the SAME IsCapabilityEnabled accessor a few lines + // below. It becomes an independent oracle the moment package A's c1 lands and the + // fill stops copying those fields. What it proves already is that the block is + // emitted, sized and suppressed - the resid= byte class and the one divergence it + // caught during development (GL_DITHER) are that evidence. + // // Emitted once per context and again whenever the capability set may have moved, // which is whenever the pipeline version moved: every SET_CAPABILITY arm calls // BumpVersions, so that shutter cannot miss one. @@ -878,14 +968,29 @@ namespace MobileGL::MG_Pipe { payloadBytes += sizeof(MGPDynamicState) + blobBytes; } - if (dirty & (MGPipeDirtyBit(MGPipeDirty::NewPipelineState) | - MGPipeDirtyBit(MGPipeDirty::NewRenderState))) { + // The staging mirror is what set_dynamic_state diffs against, so it may only be + // advanced by the branch that actually SENT dynamic bytes. Latching it whenever + // either bit fired would, if NEW_PIPELINE_STATE could ever fire alone, claim the + // server holds chunks it never received - and the chunk-level suppressor would + // then never resend them, which is a permanently stale answer with no gate on it. + // + // It cannot fire alone today because BumpVersions() moves both counters + // (RenderState.h), but that is an invariant of ANOTHER package's file. So it is + // asserted here rather than assumed, and the assignment is narrowed to the one + // bit that owns the mirror. + MOBILEGL_ASSERT((dirty & MGPipeDirtyBit(MGPipeDirty::NewPipelineState)) == 0 || + (dirty & MGPipeDirtyBit(MGPipeDirty::NewRenderState)) != 0, + "NEW_PIPELINE_STATE fired without NEW_RENDER_STATE: RenderState's " + "BumpVersions no longer moves both counters"); + if (dirty & MGPipeDirtyBit(MGPipeDirty::NewRenderState)) { tracker.Staged() = live; } return payloadBytes; } } // namespace + Uint64 MGPipeVertexAttribDefaultRepairCount() { return g_attribDefaultRepairs; } + // ---- the validate point (P2 brief D1) ---- void MGPipeValidateForVerb(MGPipeVerb verb) { PipeInputs& inputs = gPipeInputs; @@ -922,27 +1027,31 @@ namespace MobileGL::MG_Pipe { const Uint32 dirty = tracker.Update(*ctx, verbClass); // ---- step 3: emission ---- + // Every gate below goes through MGPipeSubsystemForDirty, the ONE map from a dirty bit + // to the runtime subsystem that owns it. Naming the subsystem constants here instead + // would be a second copy of that map in the only path that runs, and mis-gating a bit + // in it would pass every test the map has. const Uint64 pushMask = MG_Config::Features.PipePush; + const auto wants = [&](MGPipeDirty bit) { + const Uint64 subsystem = MGPipeSubsystemForDirty(bit); + return subsystem != 0 && (pushMask & subsystem) != 0 && + (dirty & MGPipeDirtyBit(bit)) != 0; + }; Uint64 payloadBytes = 0; - if ((pushMask & kMGPipeSubsystemRenderState) != 0 && - (dirty & (MGPipeDirtyBit(MGPipeDirty::NewPipelineState) | - MGPipeDirtyBit(MGPipeDirty::NewRenderState))) != 0) { + if (wants(MGPipeDirty::NewPipelineState) || wants(MGPipeDirty::NewRenderState)) { payloadBytes += EmitRenderState(*ctx, dirty, tracker.FreshlyPrimed()); } if (tracker.FreshlyPrimed()) { // A fresh context: what the server has is no longer what any slot last emitted. MGPipeSetHashSuppressorInstance().InvalidateAll(); } - if ((pushMask & kMGPipeSubsystemPixelPack) != 0 && - (dirty & MGPipeDirtyBit(MGPipeDirty::NewPixelPack)) != 0) { + if (wants(MGPipeDirty::NewPixelPack)) { payloadBytes += EmitPixelPackState(*ctx); } - if ((pushMask & kMGPipeSubsystemPatchState) != 0 && - (dirty & MGPipeDirtyBit(MGPipeDirty::NewPatchState)) != 0) { + if (wants(MGPipeDirty::NewPatchState)) { payloadBytes += EmitPatchState(*ctx); } - if ((pushMask & kMGPipeSubsystemVertexAttribDefaults) != 0 && - (dirty & MGPipeDirtyBit(MGPipeDirty::NewVertexAttribDefaults)) != 0) { + if (wants(MGPipeDirty::NewVertexAttribDefaults)) { payloadBytes += EmitVertexAttribDefaults(*ctx); } diff --git a/MobileGL/MG_Impl/Pipe/PipeFill.h b/MobileGL/MG_Impl/Pipe/PipeFill.h index f9be638d..9a8df9b1 100644 --- a/MobileGL/MG_Impl/Pipe/PipeFill.h +++ b/MobileGL/MG_Impl/Pipe/PipeFill.h @@ -51,6 +51,19 @@ namespace MobileGL::MG_Pipe { // Fatal{PipeVerifyBadKnob}. void MGPipeSetPoisonOmission(const char* verb, const char* field); + // PipeFill.cpp. How many times set_vertex_attrib_defaults' applier failed to reproduce + // the value the call carried, so the client wrote the mirror itself + // (EmitVertexAttribDefaults). It is the ONE observable of that repair: the window it + // covers is a verb whose class does not read m_currentVertexAttribute, where reading the + // storage to check it would be the poison violation the fill table exists to forbid. So + // TrackerShippedEmitter asserts on this counter instead, and the day package A's applier + // switches on MGPAttribValue::ValueClass the counter stops moving. + // + // Not hot-path instrumentation: it is incremented only inside the repair branch, which + // runs only when the call actually went out, which is only when an attribute default + // moved. + Uint64 MGPipeVertexAttribDefaultRepairCount(); + #if MOBILEGL_PIPE_VERIFY // PipeFill.cpp. The second arm of the comparator (P1 brief D8, ARCHITECTURE.md 13.2-2): // fills `snapshot` from the live GLContext the old way, for every field in `mask`. This diff --git a/MobileGL/MG_Impl/Pipe/Tracker.h b/MobileGL/MG_Impl/Pipe/Tracker.h index 493fea12..4ec0d6e8 100644 --- a/MobileGL/MG_Impl/Pipe/Tracker.h +++ b/MobileGL/MG_Impl/Pipe/Tracker.h @@ -149,6 +149,13 @@ namespace MobileGL::MG_Pipe { // (ARCHITECTURE.md 5.2: MG_State is not changed for this). A decrease is a wrap and adds // 65536. A wrap is harmless locally - one extra re-push, never a missed one - which is // exactly what TrackerTest.WrapAroundRePushesButNeverMisses pins. + // + // THE ONE CASE IT CANNOT SEE, stated because "never a missed push" is otherwise stronger + // than what is true: the wrap test is `now < m_last`, so a counter that advances by + // EXACTLY 65536 (or a multiple) between two walks reads as unchanged. That needs 65536 + // render-state mutations inside one verb boundary, and it is pre-existing in class - + // both backends already compare raw Uint16 versions the same way - so P2 records it + // rather than widening MG_State's counters, which ARCHITECTURE.md 5.2 rules out. class MGPipeWidenedCounter { public: Uint64 Observe(Uint16 now) { @@ -392,6 +399,33 @@ namespace MobileGL::MG_Pipe { Uint64 m_walks[kMGPipeVerbClassCount]{}; }; + // ONE attribute default, flattened onto the wire (P2 brief D10). A named function rather + // than four lines inside the emitter because this flattening is the whole correctness + // question of set_vertex_attrib_defaults: a CurrentVertexAttributeValue is one value in + // three views and GLContext converts NUMERICALLY between them, so four words alone are + // not the value - glVertexAttrib4f(loc, 1.5f, ...) leaves 1 in intValue and 0x3FC00000 in + // floatValue. MGPAttribValue::ValueClass is what makes the four words readable again, and + // TrackerAttribPayload pins that here instead of leaving it to the emitter's shape. + inline void MGPipeFillAttribValue(Uint32 location, + const MG_State::GLState::CurrentVertexAttributeValue& value, + Uint32 writtenClass, MGPAttribValue& out) { + out = MGPAttribValue{}; + out.Location = location; + out.ValueClass = static_cast(writtenClass); + static_assert(sizeof(out.Data) == sizeof(value.floatValue), "MGPAttribValue::Data is four words"); + switch (writtenClass) { + case MG_State::GLState::kVertexAttribValueClassInt: + std::memcpy(out.Data, value.intValue.data(), sizeof(out.Data)); + break; + case MG_State::GLState::kVertexAttribValueClassUint: + std::memcpy(out.Data, value.uintValue.data(), sizeof(out.Data)); + break; + default: + std::memcpy(out.Data, value.floatValue.data(), sizeof(out.Data)); + break; + } + } + // The monolith's one tracker. Under split there is one per client context; the context // identity check inside Update is what makes the single instance safe today. inline MGPipeTracker& MGPipeTrackerInstance() { diff --git a/MobileGL/MG_State/GLState/Core.cpp b/MobileGL/MG_State/GLState/Core.cpp index c814f921..3f4f28f8 100644 --- a/MobileGL/MG_State/GLState/Core.cpp +++ b/MobileGL/MG_State/GLState/Core.cpp @@ -214,6 +214,11 @@ namespace MobileGL::MG_State { current.intValue[component] = static_cast(value[component]); current.uintValue[component] = static_cast(value[component]); } +#if MOBILEGL_PIPE_PUSH + // The two views above are CONVERSIONS, not bit copies, so which one was written + // is part of the value; set_vertex_attrib_defaults carries it. + m_currentVertexAttributeClasses[index] = kVertexAttribValueClassFloat; +#endif MGP_NOTE_AGGREGATE(VertexAttribDefault); } @@ -229,6 +234,9 @@ namespace MobileGL::MG_State { current.floatValue[component] = static_cast(value[component]); current.uintValue[component] = static_cast(value[component]); } +#if MOBILEGL_PIPE_PUSH + m_currentVertexAttributeClasses[index] = kVertexAttribValueClassInt; +#endif MGP_NOTE_AGGREGATE(VertexAttribDefault); } @@ -244,6 +252,9 @@ namespace MobileGL::MG_State { current.floatValue[component] = static_cast(value[component]); current.intValue[component] = static_cast(value[component]); } +#if MOBILEGL_PIPE_PUSH + m_currentVertexAttributeClasses[index] = kVertexAttribValueClassUint; +#endif MGP_NOTE_AGGREGATE(VertexAttribDefault); } diff --git a/MobileGL/MG_State/GLState/Core.h b/MobileGL/MG_State/GLState/Core.h index a72398ac..b01310c8 100644 --- a/MobileGL/MG_State/GLState/Core.h +++ b/MobileGL/MG_State/GLState/Core.h @@ -30,10 +30,26 @@ namespace MobileGL { void Init(); namespace GLState { +#if MOBILEGL_PIPE_PUSH + // MGPAttribValue::ValueClass' encoding (MG_Pipe/MGPipeTypes.h documents the order + // "Float | Int | Uint | Double"). It lives here rather than in MG_Pipe because the + // FRONTEND is the only thing that knows which of the three views below a value was + // written through - the other two are numeric conversions of it - and MG_Pipe has + // no enum for the field yet. If package A introduces one, this becomes its alias. + inline constexpr Uint32 kVertexAttribValueClassFloat = 0; + inline constexpr Uint32 kVertexAttribValueClassInt = 1; + inline constexpr Uint32 kVertexAttribValueClassUint = 2; +#endif + struct CurrentVertexAttributeValue { Array floatValue{0.f, 0.f, 0.f, 1.f}; Array intValue{0, 0, 0, 1}; Array uintValue{0u, 0u, 0u, 1u}; + // Three scalar arrays and NOTHING ELSE. MG_Backend/MGPipe/PipeInputs.cpp + // compares this storage with one memcmp and asserts that size, so a fourth + // member here is a build break in a file P2 package B does not own. The + // written-class discriminator set_vertex_attrib_defaults needs therefore + // lives beside the array on GLContext, not inside the value. }; // Which of the three views above a shader input of a given GLSL type consumes. @@ -233,6 +249,27 @@ namespace MobileGL { Uint64 GetAnyVertexAttribDefaultGeneration() const { return m_anyVertexAttribDefaultGeneration; } + + // Which of the three views of m_currentVertexAttributes[index] the last + // glVertexAttrib* write filled DIRECTLY. The other two are NUMERIC + // conversions of it (SetCurrentVertexAttribute* below), not bit copies, so + // four words on a wire are not the value unless the class travels with them: + // glVertexAttrib4f(loc, 1.5f, ...) leaves 1 in intValue and 0x3FC00000 in + // floatValue. set_vertex_attrib_defaults carries this as MGPAttribValue's + // ValueClass so the applier can redo the conversion instead of memcpying one + // view into all three. + // + // It is kept BESIDE the array rather than inside CurrentVertexAttributeValue + // because that struct is mirrored into PipeInputs and compared there by a + // memcmp whose size assertion (MG_Backend/MGPipe/PipeInputs.cpp) is a file + // this package does not own - and because it need not be mirrored: the class + // only decides how to REBUILD the three views, so two writes that leave the + // three views identical rebuild identically whichever class they carried. + Uint32 GetCurrentVertexAttributeClass(Uint index) const { + return index < m_currentVertexAttributeClasses.size() + ? m_currentVertexAttributeClasses[index] + : kVertexAttribValueClassFloat; + } #endif // RenderState @@ -554,6 +591,8 @@ namespace MobileGL { SharedPtr m_transformFeedbackProgram; #if MOBILEGL_PIPE_PUSH Uint64 m_anyVertexAttribDefaultGeneration = 0; + // Parallel to m_currentVertexAttributes; see GetCurrentVertexAttributeClass. + Array m_currentVertexAttributeClasses{}; #endif Uint64 m_transformFeedbackGeneration = 0; // Source of the per-span ids above; never rolls back with an object switch. diff --git a/MobileGL/MG_Test/Pipe/TrackerTest.cpp b/MobileGL/MG_Test/Pipe/TrackerTest.cpp index 2b44e79c..5a3fa1b3 100644 --- a/MobileGL/MG_Test/Pipe/TrackerTest.cpp +++ b/MobileGL/MG_Test/Pipe/TrackerTest.cpp @@ -20,6 +20,8 @@ #if MOBILEGL_PIPE_PUSH #include #include +#include +#include #include #include #include @@ -27,6 +29,7 @@ #include #include +#include #include #include #endif @@ -68,7 +71,15 @@ namespace { X(TrackerWalk, AggregateGenerationCatchesABoundTextureMoving) \ X(TrackerWalk, ANaNPatchLevelEqualsItselfAndDoesNotFireForever) \ X(TrackerWalk, ThePixelPackShutterIsAByteCompareOfThePackHalfOnly) \ - X(TrackerWalk, TheFireTalliesOnlyRunWhilePipeStatsIsOn) + X(TrackerWalk, TheFireTalliesOnlyRunWhilePipeStatsIsOn) \ + X(TrackerAttribPayload, AFloatWriteCarriesTheFloatBitsAndNamesItsClass) \ + X(TrackerAttribPayload, AnIntWriteCarriesTheIntWordsAndNamesItsClass) \ + X(TrackerAttribPayload, AUintWriteCarriesTheUintWordsAndNamesItsClass) \ + X(TrackerAttribPayload, TheSameNumbersWrittenThroughADifferentClassAreADifferentValue) \ + X(TrackerShippedEmitter, ABlendToggleThroughTheValidatePointMintsTwoCsos) \ + X(TrackerShippedEmitter, TheSteadyStateThroughTheValidatePointEmitsNothing) \ + X(TrackerShippedEmitter, APushedAttributeDefaultTheApplierCannotReproduceIsRepaired) \ + X(TrackerShippedEmitter, AViewportThroughTheValidatePointMintsNoCso) #define MGL_DECLARE_PULL_SKIP(Suite, Name) \ TEST(Suite, Name) { GTEST_SKIP() << "compiled only under MOBILEGL_PIPE_PUSH"; } @@ -218,17 +229,29 @@ namespace { // one process-wide tracker, and a unit test that asserts on a shared singleton is a test // that fails when ctest runs the suite in parallel. The emission logic these reproduce is // three lines long and is the same three lines the validate point runs. + // Resetting the applier without resetting the process-wide cache and tracker would leave + // the next bind_render_state naming a CSO the applier no longer has - it asserts and + // returns, leaving m_renderState unwritten. The three are one state, so they are reset + // together, here and in TrackerShippedEmitter. + void ResetTheServerSideSingletons() { + MGPipeApplierReset(); + MGPipeCsoCacheInstance().Reset(); + MGPipeCsoCacheInstance().ResetCounters(); + MGPipeTrackerInstance().Reset(); + MGPipeSetHashSuppressorInstance().InvalidateAll(); + } + class TrackerWalk : public ::testing::Test { protected: void SetUp() override { m_previous = Move(MG_State::pGLContext); MG_State::pGLContext = MakeUnique(); m_savedPush = MG_Config::Features.PipePush; - MGPipeApplierReset(); + ResetTheServerSideSingletons(); } void TearDown() override { MG_Config::Features.PipePush = m_savedPush; - MGPipeApplierReset(); + ResetTheServerSideSingletons(); MG_State::pGLContext = Move(m_previous); } @@ -425,5 +448,185 @@ namespace { m_cache.Reset(); } + // =================================================================================== + // set_vertex_attrib_defaults' payload (P2 brief D10) + // =================================================================================== + // + // A CurrentVertexAttributeValue is ONE value in three views and GLContext converts + // numerically between them, so four words on the wire are not the value unless the class + // travels with them. These pin exactly that, because nothing else can: the emission + // happens at step 3 and the residual fill re-pulls the field at step 4, so at a kDraw + // verb a wrong payload is overwritten before any comparator or backend read sees it - + // which is how a hard-coded ValueClass of 0 survived a green verify lane. + class TrackerAttribPayload : public ::testing::Test { + protected: + void SetUp() override { + m_previous = Move(MG_State::pGLContext); + MG_State::pGLContext = MakeUnique(); + } + void TearDown() override { MG_State::pGLContext = Move(m_previous); } + + static GLContext& Ctx() { return *MG_State::pGLContext; } + + static MGPAttribValue PayloadFor(Uint location) { + MGPAttribValue value{}; + MGPipeFillAttribValue(static_cast(location), Ctx().GetCurrentVertexAttribute(location), + Ctx().GetCurrentVertexAttributeClass(location), value); + return value; + } + + static Uint32 Word(Float value) { + Uint32 bits = 0; + std::memcpy(&bits, &value, sizeof(bits)); + return bits; + } + + UniquePtr m_previous; + }; + + TEST_F(TrackerAttribPayload, AFloatWriteCarriesTheFloatBitsAndNamesItsClass) { + Ctx().SetCurrentVertexAttributeFloat(3, Array{1.5f, -2.5f, 3.0f, 4.0f}); + const MGPAttribValue value = PayloadFor(3); + EXPECT_EQ(value.Location, 3u); + EXPECT_EQ(value.ValueClass, MG_State::GLState::kVertexAttribValueClassFloat); + EXPECT_EQ(value.Data[0], Word(1.5f)); + EXPECT_EQ(value.Data[1], Word(-2.5f)); + // The defect this exists to stop: 1.5f's int VIEW is 1, and a carrier that sent the + // float bits while calling them class 0 for every attribute would be sending + // 0x3FC00000 where the frontend holds 1. + EXPECT_NE(value.Data[0], static_cast(Ctx().GetCurrentVertexAttribute(3).intValue[0])); + } + + TEST_F(TrackerAttribPayload, AnIntWriteCarriesTheIntWordsAndNamesItsClass) { + Ctx().SetCurrentVertexAttributeInt(5, Array{7, -9, 11, 13}); + const MGPAttribValue value = PayloadFor(5); + EXPECT_EQ(value.ValueClass, MG_State::GLState::kVertexAttribValueClassInt); + EXPECT_EQ(static_cast(value.Data[0]), 7); + EXPECT_EQ(static_cast(value.Data[1]), -9); + // and NOT the float view the frontend converted it into + EXPECT_NE(value.Data[0], Word(7.0f)); + } + + TEST_F(TrackerAttribPayload, AUintWriteCarriesTheUintWordsAndNamesItsClass) { + Ctx().SetCurrentVertexAttributeUint(6, Array{4000000000u, 2u, 3u, 4u}); + const MGPAttribValue value = PayloadFor(6); + EXPECT_EQ(value.ValueClass, MG_State::GLState::kVertexAttribValueClassUint); + EXPECT_EQ(value.Data[0], 4000000000u); + EXPECT_NE(value.Data[0], Word(4000000000.0f)); + } + + // The class is PER ATTRIBUTE and it is the last writer's, not the context's - a payload + // that took one attribute's class for all 32 would be the same defect as a hard-coded 0. + TEST_F(TrackerAttribPayload, TheSameNumbersWrittenThroughADifferentClassAreADifferentValue) { + Ctx().SetCurrentVertexAttributeFloat(1, Array{1.0f, 2.0f, 3.0f, 4.0f}); + Ctx().SetCurrentVertexAttributeInt(2, Array{1, 2, 3, 4}); + EXPECT_EQ(PayloadFor(1).ValueClass, MG_State::GLState::kVertexAttribValueClassFloat); + EXPECT_EQ(PayloadFor(2).ValueClass, MG_State::GLState::kVertexAttribValueClassInt); + // Same numbers, different classes, so the same four words mean different things: + // 1.0f is 0x3F800000 and the integer 1 is 0x00000001. + EXPECT_NE(PayloadFor(1).Data[0], PayloadFor(2).Data[0]); + // An attribute nobody wrote answers Float, which is what the GL default (0,0,0,1) is. + EXPECT_EQ(PayloadFor(7).ValueClass, MG_State::GLState::kVertexAttribValueClassFloat); + // and a later write of the other class moves the class of THAT attribute only + Ctx().SetCurrentVertexAttributeUint(1, Array{1u, 2u, 3u, 4u}); + EXPECT_EQ(PayloadFor(1).ValueClass, MG_State::GLState::kVertexAttribValueClassUint); + EXPECT_EQ(PayloadFor(2).ValueClass, MG_State::GLState::kVertexAttribValueClassInt); + } + + // =================================================================================== + // The SHIPPED emitter, driven through MGPipeValidateForVerb itself + // =================================================================================== + // + // TrackerWalk above reproduces step 3 against a local tracker and cache, which cannot + // fail on a defect in the validate point itself (a bit gated on the wrong subsystem, an + // emission dropped). These drive the real entry point and read the real singletons back. + // Safe because ctest runs one gtest case per process and the fixture resets all three + // pieces of server-side state on both sides of every case. + class TrackerShippedEmitter : public ::testing::Test { + protected: + void SetUp() override { + m_previous = Move(MG_State::pGLContext); + MG_State::pGLContext = MakeUnique(); + m_savedPush = MG_Config::Features.PipePush; + MG_Config::Features.PipePush = kMGPipeSubsystemsMigratedAtP2; + ResetTheServerSideSingletons(); + } + void TearDown() override { + MGPipeLeaveVerb(); + MG_Config::Features.PipePush = m_savedPush; + ResetTheServerSideSingletons(); + MG_State::pGLContext = Move(m_previous); + } + + static GLContext& Ctx() { return *MG_State::pGLContext; } + static void Draw() { MGPipeValidateForVerb(MGPipeVerb::DrawArrays); } + static const MGPipeCsoCache::Counters& Cso() { return MGPipeCsoCacheInstance().GetCounters(); } + + Uint64 m_savedPush = 0; + UniquePtr m_previous; + }; + + TEST_F(TrackerShippedEmitter, ABlendToggleThroughTheValidatePointMintsTwoCsos) { + constexpr int kToggles = 16; + Draw(); // prime: a fresh context resets the cache inside the emitter and mints once + MGPipeCsoCacheInstance().ResetCounters(); + for (int i = 0; i < kToggles; ++i) { + Ctx().SetCapability(CapabilityInput::Blend, true); + Draw(); + Ctx().SetCapability(CapabilityInput::Blend, false); + Draw(); + } + EXPECT_EQ(Cso().Mints, 1u) << "the blend-disabled state was already cached by the priming draw"; + EXPECT_EQ(Cso().Binds, static_cast(2 * kToggles)); + EXPECT_EQ(Cso().Hits, static_cast(2 * kToggles - 1)); + EXPECT_EQ(MGPipeCsoCacheInstance().Size(), 2u); + } + + TEST_F(TrackerShippedEmitter, TheSteadyStateThroughTheValidatePointEmitsNothing) { + Draw(); + // The positive half, so "nothing was emitted" cannot pass because nothing is wired: + // the first draw on a fresh context mints and binds exactly one CSO. + ASSERT_EQ(Cso().Mints, 1u) << "the priming draw emitted no create_render_state at all"; + ASSERT_EQ(Cso().Binds, 1u); + MGPipeCsoCacheInstance().ResetCounters(); + for (int i = 0; i < 8; ++i) { + Draw(); + EXPECT_EQ(MGPipeTrackerInstance().LastDirty(), 0u) << "walk " << i << " fired with nothing moved"; + } + EXPECT_EQ(Cso().Mints, 0u); + EXPECT_EQ(Cso().Binds, 0u) << "a steady-state draw bound a render-state CSO"; + } + + // The window MAJOR-2's repair covers: a glVertexAttrib* write followed by a verb whose + // class does NOT read m_currentVertexAttribute. The call still goes out (the dirty bit + // and the subsystem bit are all step 3 looks at), the applier writes four words into all + // three views because it ignores ValueClass, and nothing in step 4 puts the value back - + // so the client checks and repairs. Reading the storage here to prove it would be the + // poison violation the fill table forbids, so the repair counter is the observable. + TEST_F(TrackerShippedEmitter, APushedAttributeDefaultTheApplierCannotReproduceIsRepaired) { + Draw(); + const Uint64 before = MGPipeVertexAttribDefaultRepairCount(); + // 1.5f is the point: its int view is 1 and its bit pattern is 0x3FC00000, so the two + // cannot be the same four words whichever view the carrier picks. + Ctx().SetCurrentVertexAttributeFloat(0, Array{1.5f, 2.5f, 3.5f, 4.5f}); + MGPipeValidateForVerb(MGPipeVerb::GenerateMipmap); + EXPECT_EQ(MGPipeVertexAttribDefaultRepairCount(), before + 1) + << "the emitter accepted an applier write that cannot reproduce a converted value"; + } + + TEST_F(TrackerShippedEmitter, AViewportThroughTheValidatePointMintsNoCso) { + Draw(); + ASSERT_EQ(Cso().Mints, 1u) << "the priming draw emitted no create_render_state at all"; + MGPipeCsoCacheInstance().ResetCounters(); + for (Int i = 1; i <= 8; ++i) { + Ctx().SetViewport(IntVec4(0, 0, 64 + i, 48 + i)); + Draw(); + EXPECT_NE(MGPipeTrackerInstance().LastDirty() & MGPipeDirtyBit(MGPipeDirty::NewRenderState), 0u); + EXPECT_EQ(MGPipeTrackerInstance().LastDirty() & MGPipeDirtyBit(MGPipeDirty::NewPipelineState), 0u); + } + EXPECT_EQ(Cso().Mints, 0u); + EXPECT_EQ(Cso().Binds, 0u) << "glViewport reached the CSO cache"; + } + #endif // MOBILEGL_PIPE_PUSH } // namespace