From 0c55560510c507b5c33552d04da938bb51c6c01f Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Tue, 8 Sep 2026 04:11:25 -0400 Subject: [PATCH] [Fix] (Pipe, State): look the resource handle up on the content paths instead of minting it, publish the bind mask from the two draw-time emitters, pair the destroy with the create's own latch, and state the record-lifetime rule the applier now holds --- MobileGL/MG_Impl/Pipe/PipeFill.cpp | 93 ++++++++-- MobileGL/MG_Impl/Pipe/ResourceTracker.h | 166 +++++++++++++++--- MobileGL/MG_Impl/Pipe/VertexInputEmit.h | 27 +++ MobileGL/MG_Pipe/DirtySurface.def | 4 + MobileGL/MG_Pipe/PipeMutation.h | 9 +- .../GLState/BufferState/BufferObject.cpp | 9 +- MobileGL/MG_Test/Pipe/ResourceEmitTest.cpp | 3 +- 7 files changed, 273 insertions(+), 38 deletions(-) diff --git a/MobileGL/MG_Impl/Pipe/PipeFill.cpp b/MobileGL/MG_Impl/Pipe/PipeFill.cpp index 12c340fb..9a3f384b 100644 --- a/MobileGL/MG_Impl/Pipe/PipeFill.cpp +++ b/MobileGL/MG_Impl/Pipe/PipeFill.cpp @@ -564,6 +564,30 @@ namespace MobileGL::MG_Pipe { only.Kind = static_cast(MGPipeKind::Buffer); return only; } + + // THE CONTENT PATHS LOOK THE HANDLE UP, THEY DO NOT MINT IT. Acquire mutates the + // process-global slot allocator (a map insert on a miss, a free-list pop) and then + // resizes the tracker's inverse vector; D-A2 preserves the off-thread/stale-queue arm + // of Ops_SubData, so BufferObject::NotifySubData is reachable off the render thread, + // and two threads inside Acquire - or one there while ~BufferObject is in Free - is a + // torn free list and a dangling span. The mint happens ONCE, on the GL thread, in the + // BufferObject constructor (MGPipeMintResourceHandle), so on every content path the + // handle already exists and a pure lookup is not merely safe but strictly correct. + // + // A null answer therefore means a buffer whose constructor did not mint - which + // cannot happen in a push build - or a lifetime id already freed. Either way the call + // is dropped, so it is said out loud rather than passing kMGPipeNullHandle to the + // applier, which would count it as a refusal with no way back to the cause. + MGPipeHandle ContentHandleFor(const BufferObject& buffer, const char* call) { + const MGPipeHandle handle = MGPipeResourceTrackerInstance().Find(buffer); + if (MGPipeHandleIsNull(handle)) { + MGLOG_E_ONCE("MGPipe: %s on buffer %u has no resource handle - the call is dropped; a " + "push build mints one in the BufferObject constructor, so this is a lifetime " + "id that was already freed", + call, buffer.GetExternalIndex()); + } + return handle; + } } // namespace Bool MGPipeResourceSubsystemEnabled() { @@ -596,6 +620,9 @@ namespace MobileGL::MG_Pipe { // that has none. const MGPResourceDesc desc = MGPipeBuildResourceDesc(buffer, handle, bindMask, false); tracker.NoteDesc(desc, true); + // LATCHED, so the destroy is gated on whether this create actually went out rather + // than on whether a table is still registered when the object dies (D-L, m12). + tracker.NotePublished(handle); MGPipeApplyResourceCreate(desc); } @@ -619,12 +646,16 @@ namespace MobileGL::MG_Pipe { } void MGPipeEmitResourceSubData(BufferObject& buffer, SizeT offset, SizeT size) { - const MGPipeHandle handle = MGPipeResourceTrackerInstance().Acquire(buffer); + const MGPipeHandle handle = ContentHandleFor(buffer, "resource_subdata"); + if (MGPipeHandleIsNull(handle)) return; const Uint8* base = buffer.MappedData(); const Bool encodable = MGPipeForEachSubDataRecordRange(offset, size, [&](Uint64 at, Uint64 length) { MGPSubData record{}; - MGPipeBuildSubDataRecord(handle, at, length, record); + // The pre-pass inside the walk proved every piece encodable before the first + // one was emitted, so this cannot be false - but a zeroed record (null + // handle, size 0) is not the answer if that pre-pass is ever relaxed. + if (!MGPipeBuildSubDataRecord(handle, at, length, record, /*verbatimShadow=*/true)) return; MGPipeApplyResourceSubData(record, base + at); }); if (!encodable) { @@ -636,12 +667,16 @@ namespace MobileGL::MG_Pipe { } void MGPipeEmitBufferSubDataResident(BufferObject& buffer, SizeT offset, const void* bytes, SizeT size) { - const MGPipeHandle handle = MGPipeResourceTrackerInstance().Acquire(buffer); + const MGPipeHandle handle = ContentHandleFor(buffer, "buffer_subdata_resident"); + if (MGPipeHandleIsNull(handle)) return; const auto* base = static_cast(bytes); const Bool encodable = MGPipeForEachSubDataRecordRange(offset, size, [&](Uint64 at, Uint64 length) { MGPSubData record{}; - MGPipeBuildSubDataRecord(handle, at, length, record); + // NOT a verbatim level shadow: these bytes are the application's staging + // store, or the pattern FillSubData expanded locally, and neither is this + // client's untransformed shadow of the level. + if (!MGPipeBuildSubDataRecord(handle, at, length, record, /*verbatimShadow=*/false)) return; // The application's STAGING store, valid for the duration of the call only. MGPipeApplyBufferSubDataResident(record, base + (at - offset)); }); @@ -653,7 +688,8 @@ namespace MobileGL::MG_Pipe { } void MGPipeEmitResourceFlushRange(BufferObject& buffer, SizeT offset, SizeT size, Uint32 accessFlags) { - const MGPipeHandle handle = MGPipeResourceTrackerInstance().Acquire(buffer); + const MGPipeHandle handle = ContentHandleFor(buffer, "resource_flush_range"); + if (MGPipeHandleIsNull(handle)) return; MGPFlushRange record{}; record.Res = handle; record.Offset = offset; @@ -666,7 +702,8 @@ namespace MobileGL::MG_Pipe { } void MGPipeEmitResourceReadback(BufferObject& buffer) { - const MGPipeHandle handle = MGPipeResourceTrackerInstance().Acquire(buffer); + const MGPipeHandle handle = ContentHandleFor(buffer, "resource_readback"); + if (MGPipeHandleIsNull(handle)) return; MGPReadback record{}; record.Res = handle; // Whole-buffer by contract (BufferObject.h: the op pulls the backend's current @@ -678,8 +715,17 @@ namespace MobileGL::MG_Pipe { MGPipeApplyResourceReadback(record); } + // NO UnmapPersistent PRODUCER IN P3a, AND THAT IS DELIBERATE. The catalogue has the call + // and wire implemented it, but D-J forbids new behaviour and there is nothing to convert: + // BufferBackendOps has seven hooks and none of them is an unmap, and + // PipeResource::ReleasePersistentMap() (BufferObject.cpp, from RedefineStorage) tells the + // backend nothing today - it learns from the Respecify that follows. Emitting + // unmap_persistent here would therefore be a new call to a backend that has never been + // told about a release, so the client emits none and the applier's refusal counter stays + // at 0 for it. The producer lands with the phase that gives the backend an unmap hook. void* MGPipeEmitMapPersistent(BufferObject& buffer) { - const MGPipeHandle handle = MGPipeResourceTrackerInstance().Acquire(buffer); + const MGPipeHandle handle = ContentHandleFor(buffer, "map_persistent"); + if (MGPipeHandleIsNull(handle)) return nullptr; MGPipeResourceTrackerInstance().NoteMapPersistent(); // THE map-persistent-roundtrips SITE, and it counts every EMISSION - mint OR // DECLINE - because every one of them needs an answer from the resource owner. A @@ -692,11 +738,17 @@ namespace MobileGL::MG_Pipe { return MGPipeApplyMapPersistent(BufferHandleOnly(handle), buffer.GetSize(), buffer.MappedData()); } - void MGPipeEmitResourceDestroyAndFree(BufferObject& buffer) { + Bool MGPipeEmitResourceDestroyAndFree(BufferObject& buffer) { MGPipeResourceTracker& tracker = MGPipeResourceTrackerInstance(); const MGPipeHandle handle = tracker.Find(buffer); - if (MGPipeHandleIsNull(handle)) return; - if (MGPipeResourceSubsystemEnabled()) { + if (MGPipeHandleIsNull(handle)) return false; + // THE LATCHED ANSWER, not the live one (m12): create and destroy are gated at two + // different moments, and a buffer constructed while a backend's table was registered + // and destroyed after UnregisterBufferBackendOps() would otherwise free its slot with + // the applier's record still Live and the backend's twin still attached to it - on a + // slot the allocator is about to hand out again. + const Bool published = tracker.WasPublished(handle); + if (published) { tracker.NoteDestroy(); MGPipeApplyResourceDestroy(BufferHandleOnly(handle)); } @@ -707,6 +759,7 @@ namespace MobileGL::MG_Pipe { // so a double free cannot skip a generation. tracker.Retire(handle); MGPipeSlots().Free(MGPipeKind::Buffer, handle); + return published; } void MGPipeSetPoisonOmission(const char* verb, const char* field) { @@ -921,6 +974,14 @@ namespace MobileGL::MG_Pipe { // through bind_vertex_elements - and EmittedCallSuppliesTheWholeField below says // false for it, with the reason. So this bit switches the EMISSION on and // changes nothing about the fill loop. + // + // THE TWO BITS ARE NOT AN INDEPENDENT A/B IN ONE DIRECTION, and an operator turning + // them on one at a time has to know which: set_vertex_buffers and set_index_buffer + // name their buffers by {slot, gen} whether or not the resource family created a + // record for them, so bit 8 WITHOUT bit 7 sends the server handles it cannot resolve + // and every one of those calls lands in RefusedResourceCalls. Bit 7 without bit 8 is + // fine. Neither the P3a default (0x1ff, both on) nor G12's control (0x7f, both off) + // is in that arm, which is why nothing in the phase trips over it. constexpr Uint64 kMGPipeWiredSubsystems = kMGPipeSubsystemRenderState | kMGPipeSubsystemPixelPack | kMGPipeSubsystemPatchState | @@ -1419,9 +1480,15 @@ namespace MobileGL::MG_Pipe { MGPipeCsoCacheInstance().Reset(); MGPipeApplierReset(); MGPipeSetHashSuppressorInstance().InvalidateAll(); - // P3a: and the vertex-input emitter's latches, for the same reason - they say - // "this handle has already published this configuration" about an applier whose - // vertex-elements records the reset above has just dropped. + // P3a: and the vertex-input emitter's latches. NOT because the applier dropped + // its vertex-elements records - it does not, they are share-group object state + // and survive a make-current - but because the emitter's OTHER latch, the bound + // handle, mirrors the applier's BoundVertexElements, which MGPipeApplierReset + // DOES clear. Without this the bind after a make-current would be suppressed as + // unchanged and the server would draw with no vertex elements bound. Re-creating + // an unchanged configuration alongside it is a bounded over-fire; a dropped bind + // is not. The resource tracker is deliberately NOT reset here for the same + // reason its records survive: see ResourceTracker.h's ResetForTest. MGPipeVertexInputEmitterInstance().Reset(); g_residualDue = true; } diff --git a/MobileGL/MG_Impl/Pipe/ResourceTracker.h b/MobileGL/MG_Impl/Pipe/ResourceTracker.h index 31199640..b6c251db 100644 --- a/MobileGL/MG_Impl/Pipe/ResourceTracker.h +++ b/MobileGL/MG_Impl/Pipe/ResourceTracker.h @@ -177,6 +177,21 @@ namespace MobileGL::MG_Pipe { desc.StorageKind = kMGPipeResourceStorageKindBuffer; desc.BindMask = bindMask; if (storageDefined) { + // MGPResourceDesc::Width is a Uint32 and that is the CONTRACT's shape, not this + // package's, so a store of 4 GiB or more cannot be declared at all. Truncating it + // silently is the one answer that must not happen: the applier's range gate would + // then refuse the first legal write past the truncated extent as + // Fatal{ProtocolCorruption} and name a corruption that is really a narrowing here. + // So it is said out loud, once, in every build - the assertion compiles out at + // INFO, which is what all three gate builds are. + if (buffer.GetSize() > static_cast(0xFFFFFFFFull)) { + MGLOG_E_ONCE("MGPipe: buffer %u declares a store of %llu bytes, which does not fit " + "MGPResourceDesc::Width - the descriptor's extent is narrowed and every " + "write past 4 GiB will be refused by the applier's range gate", + buffer.GetExternalIndex(), + static_cast(buffer.GetSize())); + MOBILEGL_ASSERT(false, "MGPResourceDesc::Width cannot carry this buffer's size"); + } desc.Width = static_cast(buffer.GetSize()); desc.Usage = static_cast(buffer.GetUsage()); desc.StorageFlags = static_cast(buffer.GetStorageFlags()); @@ -193,11 +208,26 @@ namespace MobileGL::MG_Pipe { // coordinate and first extent, and MGPipeSetSubDataBufferRange is the ONLY spelling of // that convention. Returns false, with the record untouched, when the range does not fit // one record - which is where MGPipeForEachSubDataRecordRange comes in. - inline Bool MGPipeBuildSubDataRecord(MGPipeHandle res, Uint64 offset, Uint64 size, MGPSubData& out) { + // + // `sourceIsVerbatimLevelShadow` is the record's own question - "are these bytes an + // untransformed level shadow?" - and it is a PARAMETER because the answer differs by + // caller: resource_subdata hands over the client's own shadow at an offset into it and + // says yes; buffer_subdata_resident hands over the application's staging store, or the + // locally expanded pattern FillSubData built, and both say no. Nothing reads it on the + // buffer path today, which is exactly why it must not be a hard-coded 1 that becomes + // wrong the moment something does. + // + // Blob is FILLED, exactly: Seg is kMGHostSpanSegNone (monolith - the bytes travel beside + // the record through the entry point's companion pointer) and Size is the piece's own + // byte length, which is what the applier's ONE Blob rule holds a non-zero declaration to + // (PipeApply.cpp's SubDataBoxFault: != 0 && != MGPipeSubDataBufferSize is refused). + // Leaving it 0 would be legal too; declaring it correctly is the stronger of the two. + inline Bool MGPipeBuildSubDataRecord(MGPipeHandle res, Uint64 offset, Uint64 size, MGPSubData& out, + Bool sourceIsVerbatimLevelShadow) { out = MGPSubData{}; out.Res = res; out.Target = kMGPipeResourceTargetBuffer; - out.SourceIsVerbatimLevelShadow = 1; // the bytes ARE the client's shadow, unmodified + out.SourceIsVerbatimLevelShadow = sourceIsVerbatimLevelShadow ? 1 : 0; if (!MGPipeSetSubDataBufferRange(out, offset, size)) return false; out.Blob.Seg = kMGHostSpanSegNone; out.Blob.Size = size; @@ -298,6 +328,27 @@ namespace MobileGL::MG_Pipe { m_bySlot[slot] = Entry{}; } + // ---- D-L: was resource_create actually EMITTED for this slot? ---- + // + // The create is gated at its call site (BufferObject's constructor) and the destroy + // is gated inside MGPipeEmitResourceDestroyAndFree, so the two ask the SAME question + // at two different moments. A buffer constructed while a backend's table was + // registered and destroyed after UnregisterBufferBackendOps() would take the second + // answer, free its slot, and leave the applier's record Live - on a slot the + // allocator is about to hand out again, with the backend's twin (a driver buffer id) + // still attached to it. So the answer is LATCHED at the create and the destroy uses + // the latched one; the two are then a pair by construction rather than by the + // registration outliving every buffer. + void NotePublished(MGPipeHandle handle) { + const SizeT slot = handle.Slot; + if (slot >= m_bySlot.size()) return; + m_bySlot[slot].Published = true; + } + Bool WasPublished(MGPipeHandle handle) const { + const SizeT slot = handle.Slot; + return slot < m_bySlot.size() && m_bySlot[slot].Published; + } + // The sticky everBoundAs mask. Sticky exactly as MGPResourceDesc::ImageBindableHint's // everImageBound is: ORed, never cleared, so a buffer that was an element array once // keeps saying so. @@ -306,29 +357,53 @@ namespace MobileGL::MG_Pipe { return slot < m_bySlot.size() ? m_bySlot[slot].BindMask : Uint16{0}; } + // OR one target's bit into a handle's sticky mask, without looking at the context at + // all. This is what closes the sampling window for the two bits anything keys on: + // the vertex-input emitters resolve, at EVERY draw, exactly the attribute buffers and + // the element-slot buffer, so any buffer ever DRAWN FROM carries its ARRAY_BUFFER / + // ELEMENT_ARRAY bit for the rest of its life whether or not it happened to be bound + // at a storage op. It grows the table rather than dropping the note: it is called + // from the validate point, which is GL-thread by construction, and a slot outside the + // table is a buffer whose mint this process has not seen (a unit fixture's + // ResetForTest, in practice). void NoteBoundAs(MGPipeHandle handle, BufferTarget target) { + if (MGPipeHandleIsNull(handle)) return; const SizeT slot = handle.Slot; - if (slot >= m_bySlot.size()) return; + if (slot >= m_bySlot.size()) m_bySlot.resize(slot + 1); m_bySlot[slot].BindMask |= static_cast(MGPipeBindMaskForBufferTarget(target)); } // Accumulates into the sticky mask every target `buffer` is bound to RIGHT NOW, and // returns the accumulated value. // - // [DEVIATION, recorded in client-v1.md] D-A3 asks for the OR at every glBindBuffer / - // glBindBufferBase / glBindBufferRange / VAO element-slot bind. Those entry points - // are MG_Impl/GLImpl/Buffer/GL_Buffer.cpp's, which C.5 assigns to no package and - // C.1 does not list for this one, so the mask is accumulated by SAMPLING the - // frontend's live binding state instead - here, at every resource emission, which is - // the only place its value is read. It is still STICKY (the union over every sample - // this buffer has ever been part of), and it is exact for the GL idiom the bit - // matters for: bind, then define or update the store. What it cannot see is a bind - // that happens after the buffer's LAST storage or content operation and is never - // followed by another - the fix is one line in BindBuffer_State, and it is handed to - // the integrator rather than taken here. + // [DEVIATION, recorded in client-v2.md] D-A3 asks for the OR at every glBindBuffer / + // glBindBufferBase / glBindBufferRange / VAO element-slot bind, and C.1 points at + // MG_State/GLState/BufferState/BufferState.{h,cpp} for it - a file this package DOES + // own. The brief is wrong about where the entry points are: BufferState only VENDS + // BindingSlot& / BindingSlotRange1D&, and the .Bind() calls are + // MG_Impl/GLImpl/Buffer/GL_Buffer.cpp's (BindBuffer_State, BindBufferBase_State, + // BindBufferRange_State), which C.5 assigns to no package. So the mask is accumulated + // by SAMPLING the frontend's live binding state instead - here, at every create and + // respecify, which is where the value is PUBLISHED - and ORed into a per-slot sticky + // field that is never cleared. + // + // WHAT SAMPLING ALONE CANNOT SEE is not "a bind after the last respecify" (which the + // specified design misses too) but a TRANSIENT bind: bind an EBO, draw, unbind, then + // define it through DSA - the respecify's sample sees no binding at all, and the DSA + // idiom makes that the common case rather than a corner (TryAdoptLargeStorage's own + // comment names glNamedBufferSubData as what MC 26.3 streams with). That hole is + // closed for the two bits anything keys on by NoteBoundAs above, called from + // EmitVertexBuffers / EmitIndexBuffer at every draw. What is left unpublished is a + // buffer that is bound, never drawn from, and never re-specified afterwards; the + // remaining fix is one line in each of GL_Buffer.cpp's three *_State binders, for the + // seven bits nothing keys on yet, and it stays handed to whoever owns that file. // // The scan is skipped unless a binding-slot version moved since the last one, which - // is one Uint16 load per global target and none per binding point. + // is one Uint16 load per global target and none per binding point. It is NOT called + // from the content emitters, deliberately: it walks the whole context's binding state + // and writes the tracker, and one of those emitters (resource_subdata) is on the path + // D-A2 preserves as reachable off the render thread. Extra sampling could only widen + // a sticky union, but not at the price of a context-wide read from the wrong thread. Uint16 RefreshBindMask(GLContext& ctx, const BufferObject& buffer, MGPipeHandle handle) { const SizeT slot = handle.Slot; if (slot >= m_bySlot.size()) return 0; @@ -387,9 +462,34 @@ namespace MobileGL::MG_Pipe { void NoteDestroy() { ++m_destroys; } void NoteMapPersistent() { ++m_mapPersistents; } - // A unit fixture's per-case reset. Never called by the library: a context change - // does not invalidate a handle, because the handle is the CLIENT's identity for a - // frontend object that outlives it. + // A unit fixture's per-case reset, and the library never calls it. THE RULE, stated + // rather than left as an absence, because "nothing resets this" is not a reason: + // + // A buffer handle and the applier record it names are SHARE-GROUP OBJECT STATE. + // A GL object lives in a share group, not in a context, so a make-current changes + // neither. The applier's MGPipeApplierReset() is a make-current and deliberately + // keeps its Resources / VertexElementsCsos (PipeApply.h says so beside them); the + // ONLY things that drop a record are the object's own death signal - + // resource_destroy, which ~BufferObject raises through + // MGPipeEmitResourceDestroyAndFree, and delete_vertex_elements - and + // MGPipeApplierReleaseObjectRecords(), which is the SERVED CONTEXT's teardown and + // is deliberately wired to nothing in the monolith (there is one applier behind + // every context, so calling it on one context's destruction would drop every other + // context's records). + // + // So this tracker needs no re-publication path on a fresh context and must not have + // one: re-emitting resource_create for a record the applier still holds would move + // its Serial for nothing. What the client owes instead is the destroy - which + // ~BufferObject already emits, in the fixed emit-then-free order (D-L) - and that is + // the whole of the client's side of the record lifecycle. + // + // The vertex-input emitter's latches are the OTHER half and are genuinely per + // context: MGPipeVertexInputEmitter::Reset() is called from the FreshlyPrimed arm + // because the applier's vertex-input WORKING state (the bound handle, the window, the + // fetch shift) IS cleared there. Its vertex-elements RECORDS are not, which is why + // the emitter's Reset drops the "already published" latches but no create is lost: + // the latch is what says "re-publish", and re-publishing an unchanged configuration + // is a bounded over-fire, not a dropped write. void ResetForTest() { m_bySlot.clear(); m_bindEpoch = 0; @@ -402,6 +502,7 @@ namespace MobileGL::MG_Pipe { BufferObject* Object = nullptr; Uint32 Gen = 0; Uint16 BindMask = 0; + Bool Published = false; Uint64 BindMaskEpoch = 0; }; @@ -411,6 +512,13 @@ namespace MobileGL::MG_Pipe { // emission whose epoch differs, so it can delay a bit by one storage op and never // drop one - the same over-fire-is-free / under-fire-is-fatal direction every // shutter in Tracker.h takes. + // + // IT DOES NOT SEE THE 84x4 INDEXED BINDING POINTS, and that is sound only because + // BindBufferBase_State / BindBufferRange_State also bind the GENERIC slot for the + // same target (GL_Buffer.cpp:1531 says why), so an indexed bind always moves one of + // the versions summed here. If that ever stops being true, the CONSTANT / + // SHADER_BUFFER / ATOMIC / STREAM_OUTPUT bits start being missed silently and the + // repair is to fold GetTouchedBufferBindingPointCount into the epoch. static Uint64 BindEpoch(GLContext& ctx) { Uint64 epoch = 1; for (const auto target : MG_State::GLState::GlobalBufferTargets) { @@ -483,9 +591,27 @@ namespace MobileGL::MG_Pipe { // the three Espryt MarkGpuWritten sites mark today and the observable behaviour is // unchanged. The narrowing itself is P8/P9's. inline void MGPipeClientOnGpuWritten(MGPipeHandle res, Uint rangeCount, const MGPRange* ranges) { - (void)rangeCount; + // THE SHAPE IS A CONTRACT POINT, not a formality: the announcement is ONE range + // covering kMGPipeWholeBuffer, deliberately not ZERO ranges, because zero will mean + // "a fully narrowed set - nothing is dirty" at P8/P9. Marking the whole buffer + // written for a zero-range announcement would be the narrowing channel run backwards, + // so the shape is asserted here rather than assumed. + MOBILEGL_ASSERT(rangeCount == 1 && ranges != nullptr, + "OnGpuWritten {slot=%u, gen=%u}: P3a announces exactly one whole-buffer range, " + "not %u", + res.Slot, res.Gen, static_cast(rangeCount)); (void)ranges; - if (auto* buffer = MGPipeResourceTrackerInstance().Resolve(res)) buffer->MarkGpuWritten(); + if (rangeCount == 0) return; + auto* buffer = MGPipeResourceTrackerInstance().Resolve(res); + if (buffer == nullptr) { + // Loud, like its sibling above: a backend announcing a write against a handle + // this client cannot resolve is a dropped MarkGpuWritten, and a dropped + // MarkGpuWritten is a stale shadow read back as if it were current. + MGLOG_E_ONCE("MGPipe: OnGpuWritten for a handle {%u,%u} that resolves to no buffer", res.Slot, + res.Gen); + return; + } + buffer->MarkGpuWritten(); } // Installed once, and never over an entry a backend already claimed: these two are the diff --git a/MobileGL/MG_Impl/Pipe/VertexInputEmit.h b/MobileGL/MG_Impl/Pipe/VertexInputEmit.h index 00a35c28..72a72894 100644 --- a/MobileGL/MG_Impl/Pipe/VertexInputEmit.h +++ b/MobileGL/MG_Impl/Pipe/VertexInputEmit.h @@ -37,6 +37,7 @@ // // HEADER-ONLY, for the ownership reason Tracker.h states in full. #if MOBILEGL_PIPE_PUSH +#include #include #include #include @@ -76,6 +77,16 @@ namespace MobileGL::MG_Pipe { // workaround both key on telling the two apart. inline MGPVertexAttribWire MGPipeBuildVertexAttribWire(const MG_State::GLState::VertexAttribute& attrib, Uint32 bindingIndex) { + // ASSERT RATHER THAN ASSUME, in both directions, because the three narrowing casts + // below cross a package boundary: VertexArrayObject is another package's file and its + // 32-slot bound is its invariant, not this one's, so a BindingIndex of 256 would wrap + // to 0 and silently point every attribute at binding 0, and a negative Stride (the + // frontend field is a signed int) would arrive as a ~4 GiB unsigned distance. + MOBILEGL_ASSERT(bindingIndex < 256u, + "MGPVertexAttribWire::BindingIndex is a Uint8 and cannot carry %u", + static_cast(bindingIndex)); + MOBILEGL_ASSERT(attrib.Size >= 0 && attrib.Size <= 255, + "MGPVertexAttribWire::Size is a Uint8 and cannot carry %d", attrib.Size); MGPVertexAttribWire wire{}; wire.Offset = static_cast(attrib.Offset); wire.Stride = static_cast(attrib.Stride); @@ -200,9 +211,19 @@ namespace MobileGL::MG_Pipe { entry.Res = attrib.Buffer ? MGPipeSlots().Acquire(MGPipeKind::Buffer, attrib.Buffer->GetLifetimeId()) : kMGPipeNullHandle; + // D-A3's sticky mask, ORed HERE rather than only sampled at a storage op. + // This is the bit that survives the DSA idiom: a buffer defined through + // glNamedBuffer* may never be bound at any resource emission, but a draw + // that fetches from it resolves it right here, on the GL thread, at every + // draw. Sticky, so one draw is enough for the rest of its life. + MGPipeResourceTrackerInstance().NoteBoundAs(entry.Res, BufferTarget::Vertex); // The attribute's own byte offset lives in MGPVertexAttribWire::Offset, // so the entry's is the BINDING's, which the frontend already folded in. entry.Offset = 0; + // Signed on the frontend, unsigned on the wire, and a negative one would + // arrive as a ~4 GiB fetch distance rather than as an error. + MOBILEGL_ASSERT(attrib.Stride >= 0, "a resolved vertex stride is never negative (%d)", + attrib.Stride); entry.Stride = static_cast(attrib.Stride); entry.Divisor = static_cast(attrib.Divisor); entry.BindingIndex = static_cast(i); @@ -238,6 +259,12 @@ namespace MobileGL::MG_Pipe { if (vao) { if (const auto& bound = vao->GetIndexBufferBindingSlot().GetBoundObject()) { m_lastIndex.Res = MGPipeSlots().Acquire(MGPipeKind::Buffer, bound->GetLifetimeId()); + // The ELEMENT_ARRAY bit, and it is the one the split path keys on + // (kCapNeedsHostIndexBytes -> restart rewriting, multi-draw flattening). + // Noted at every draw for RefreshBindMask's reason: an EBO defined through + // DSA and unbound before its last respecify would otherwise never publish + // it, and getting that bit wrong is invisible in monolith. + MGPipeResourceTrackerInstance().NoteBoundAs(m_lastIndex.Res, BufferTarget::Index); } } MGPipeApplySetIndexBuffer(m_lastIndex); diff --git a/MobileGL/MG_Pipe/DirtySurface.def b/MobileGL/MG_Pipe/DirtySurface.def index abd0aeb4..ad3ca8bc 100644 --- a/MobileGL/MG_Pipe/DirtySurface.def +++ b/MobileGL/MG_Pipe/DirtySurface.def @@ -292,6 +292,10 @@ /* MarkVertexArrayForDeletion -> delete_vertex_elements, published through the */ \ /* death notice ~VertexArrayObject already raises for the VertexElementsCso */ \ /* kind; the CSO handle is minted per frontend VAO off its lifetime id. */ \ + /* THE PUBLISHER IS THE BACKEND'S (Managers.cpp's OnFrontendStateObject- */ \ + /* Destroyed consumer, package espryt), not the client's: the client mints */ \ + /* the CSO handle and emits create/bind, and the free rides with that */ \ + /* consumer. Until it lands the row states the design, not the tree. */ \ X(MarkBufferObjectForDeletion, kExplicitDestroy) \ X(MarkFramebufferObjectForDeletion, kExplicitDestroy) \ X(MarkProgramForDeletion, kUnpublishedDestroy) \ diff --git a/MobileGL/MG_Pipe/PipeMutation.h b/MobileGL/MG_Pipe/PipeMutation.h index c09cf932..29cea9cb 100644 --- a/MobileGL/MG_Pipe/PipeMutation.h +++ b/MobileGL/MG_Pipe/PipeMutation.h @@ -115,7 +115,14 @@ namespace MobileGL::MG_Pipe { void MGPipeMintResourceHandle(MG_State::GLState::BufferObject& buffer); // In this order, and it is not negotiable (D-L): the destroy resolves the handle, and // MGPipeSlotAllocator::Free erases the lifetimeId -> slot mapping it resolves through. - void MGPipeEmitResourceDestroyAndFree(MG_State::GLState::BufferObject& buffer); + // + // RETURNS whether resource_destroy was emitted, which is the LATCH taken at this buffer's + // create and not a second reading of MGPipeResourceSubsystemEnabled(). The destructor + // needs that answer to decide whether the legacy OnDestroy still owes a call: asking the + // predicate twice pairs a create emitted under one registration with a destroy gated on + // another, and either direction leaks - a live applier record on a slot about to be + // re-handed-out, or a backend object nobody releases. + Bool MGPipeEmitResourceDestroyAndFree(MG_State::GLState::BufferObject& buffer); void MGPipeEmitResourceCreate(MG_State::GLState::BufferObject& buffer); void MGPipeEmitResourceRespecify(MG_State::GLState::BufferObject& buffer); diff --git a/MobileGL/MG_State/GLState/BufferState/BufferObject.cpp b/MobileGL/MG_State/GLState/BufferState/BufferObject.cpp index 5c2a5ff2..c038550a 100644 --- a/MobileGL/MG_State/GLState/BufferState/BufferObject.cpp +++ b/MobileGL/MG_State/GLState/BufferState/BufferObject.cpp @@ -52,9 +52,12 @@ namespace MobileGL::MG_State::GLState { // call for it - no seventh NotifyStateObjectDestroyed raiser is added, because that // header exists for kinds that have no such call. The emit-then-free ORDER is fixed // inside the helper and is not negotiable. - const Bool pushedResources = MG_Pipe::MGPipeResourceSubsystemEnabled(); - MG_Pipe::MGPipeEmitResourceDestroyAndFree(*this); - if (pushedResources) return; + // The answer is the helper's LATCH - "was resource_create emitted for this buffer" - + // not a second reading of MGPipeResourceSubsystemEnabled(): a buffer constructed + // while a backend's table was registered and destroyed after it was unregistered has + // a pipe record to drop and no legacy backend object, and one constructed the other + // way round has the opposite, so the create's answer is the only one that pairs. + if (MG_Pipe::MGPipeEmitResourceDestroyAndFree(*this)) return; #endif if (m_resource.Backend() && g_bufferBackendOps && g_bufferBackendOps->OnDestroy) { g_bufferBackendOps->OnDestroy(m_resource.ReleaseBackend()); diff --git a/MobileGL/MG_Test/Pipe/ResourceEmitTest.cpp b/MobileGL/MG_Test/Pipe/ResourceEmitTest.cpp index 30226273..73319a93 100644 --- a/MobileGL/MG_Test/Pipe/ResourceEmitTest.cpp +++ b/MobileGL/MG_Test/Pipe/ResourceEmitTest.cpp @@ -1571,7 +1571,8 @@ namespace { // And every piece the walk produced has to be encodable by the record builder - // a piece the box refuses is a record the applier's bounds gate would abort on. MGPSubData record{}; - EXPECT_TRUE(MGPipeBuildSubDataRecord(MGPipeHandle{1, 1}, piece.first, piece.second, record)) + EXPECT_TRUE(MGPipeBuildSubDataRecord(MGPipeHandle{1, 1}, piece.first, piece.second, record, + /*verbatimShadow=*/true)) << "a piece the splitter produced does not fit one record"; EXPECT_EQ(MGPipeSubDataBufferOffset(record), piece.first); EXPECT_EQ(MGPipeSubDataBufferSize(record), piece.second);