diff --git a/MobileGL/MG_Backend/DirectGLES/Managers.cpp b/MobileGL/MG_Backend/DirectGLES/Managers.cpp index 200826f5..8a9429ee 100644 --- a/MobileGL/MG_Backend/DirectGLES/Managers.cpp +++ b/MobileGL/MG_Backend/DirectGLES/Managers.cpp @@ -215,6 +215,18 @@ namespace MobileGL::MG_Backend::DirectGLES { PrgramImpl::g_backendProgramObjects.DestroyByLifetimeId(lifetimeId); break; case MG_Pipe::MGPipeKind::VertexElementsCso: + // P3a C-1: this is now the SECOND path, not the only one. The client speaks the + // whole death itself (MGPipeEmitVertexElementsDestroyAndFree: delete the + // applier record, raise this notice, free the slot), because the slot is minted + // client-side on every backend and a backend that installs no death ops - which + // Magma deliberately does not - otherwise leaked the slot and the record per + // VAO for the life of the process. What is left here is the one thing only this + // side can do: drop the driver VAO the twin owns. It is raised while the handle + // still resolves, so OnFrontendObjectDestroyed's shared free (which the other + // five kinds still depend on) is simply the one that gets there first; the + // client's own Free right after it is then a no-op, because Free refuses a slot + // that is no longer live at that generation and the Gen bump rides the next + // handout rather than the free. Double release, no corruption, no abort. VertexArrayImpl::g_backendVertexArrayObjects.DestroyByLifetimeId(lifetimeId); break; default: diff --git a/MobileGL/MG_Impl/Pipe/PipeFill.cpp b/MobileGL/MG_Impl/Pipe/PipeFill.cpp index 9a3f384b..7d09c564 100644 --- a/MobileGL/MG_Impl/Pipe/PipeFill.cpp +++ b/MobileGL/MG_Impl/Pipe/PipeFill.cpp @@ -14,6 +14,9 @@ // (CMakeLists.txt appends it to SOURCE_FILES there). #include #include +// C-1: the vertex-elements CSO's death path raises the backend notice from here, between the +// applier's delete and the slot free, so that the whole order lives in one place. +#include #include #include #include @@ -762,6 +765,57 @@ namespace MobileGL::MG_Pipe { return published; } + Bool MGPipeEmitVertexElementsDestroyAndFree(Uint64 lifetimeId) { + // C-1. THE SAME SHAPE AS MGPipeEmitResourceDestroyAndFree ABOVE, and for the same + // reason: whatever mints a handle owns the death of that handle, and the mint for this + // kind is MGPipeVertexInputEmitter::EmitVertexElements - i.e. the client, on every + // backend. Espryt's StateObjectDeathOps notice used to be the only free, so under a + // backend that installs none the slot and the applier's record leaked per VAO, for + // ever. It is now the SECOND, redundant path (Managers.cpp's + // OnFrontendStateObjectDestroyed) and it must stay idempotent, which it is: the + // notice resolves through the same lifetimeId -> slot map this function frees, and + // MGPipeSlotAllocator::Free refuses a slot that is not live at that generation. + // May be the null handle: no slot is minted for a VAO that no draw ever validated with + // and no backend twin table ever looked up. That case still raises the notice below - + // see there. + const MGPipeHandle handle = + MGPipeSlots().FindByLifetimeId(MGPipeKind::VertexElementsCso, lifetimeId); + + // ASKED, NOT ASSUMED. A slot is not evidence of a record: DirectGLES mints one from + // BackendSlotTable::GetOrCreate at every VAO sync, whether or not bit 8 asked this + // client to emit a create - the shipping 0x7f A/B control arm is exactly that + // configuration. delete_vertex_elements on a handle the applier has no record for is a + // refusal, and the refusal asserts (PipeApply.cpp's ResolveVertexElements), i.e. it + // stops a verify build. + MGPipeVertexInputEmitter& emitter = MGPipeVertexInputEmitterInstance(); + const Bool published = emitter.RecordIsPublished(handle); + if (published) { + MGPHandleOnly only{}; + only.Handle = handle; + only.Kind = static_cast(MGPipeKind::VertexElementsCso); + MGPipeApplyDeleteVertexElements(only); + emitter.NoteRecordDestroyed(handle); + } + + // THE ORDER IS D-L's, WITH THE BACKEND NOTICE IN THE MIDDLE, and each of the three + // positions is load-bearing: + // * the applier's record is dropped FIRST, while nothing else can have re-handed the + // slot out, so a recycled slot cannot inherit a field; + // * the death notice is raised SECOND, because it resolves the handle through the + // allocator and a backend told after the Free below could no longer find its twin + // - which would move the leak from the client to the driver VAO. It is raised + // UNCONDITIONALLY, exactly as ~VertexArrayObject raised it before C-1: whether a + // slot exists is this client's business, and a consumer that records notices (the + // P2 e2 gate does) must not stop seeing this class announce itself; + // * the slot goes back LAST. Espryt's notice frees it too; that Free and this one + // are the same call on the same handle and the second is a no-op, because Free + // bumps no generation (the bump rides the next handout) and refuses a slot that is + // no longer live at this generation. + MG_State::GLState::NotifyStateObjectDestroyed(MGPipeKind::VertexElementsCso, lifetimeId); + if (!MGPipeHandleIsNull(handle)) MGPipeSlots().Free(MGPipeKind::VertexElementsCso, handle); + return published; + } + void MGPipeSetPoisonOmission(const char* verb, const char* field) { if (verb == nullptr || field == nullptr) { g_omission = PoisonOmission{}; diff --git a/MobileGL/MG_Impl/Pipe/VertexInputEmit.h b/MobileGL/MG_Impl/Pipe/VertexInputEmit.h index 72a72894..94ee409b 100644 --- a/MobileGL/MG_Impl/Pipe/VertexInputEmit.h +++ b/MobileGL/MG_Impl/Pipe/VertexInputEmit.h @@ -288,12 +288,61 @@ namespace MobileGL::MG_Pipe { Uint64 VertexBufferSetCount() const { return m_bufferSets; } Uint64 IndexBufferSetCount() const { return m_indexSets; } + // ---- C-1: "does the applier hold a record for exactly this handle?" ---- + // + // The CSO's death path (MGPipeEmitVertexElementsDestroyAndFree) needs that answer and + // MUST NOT GUESS IT FROM THE SLOT. A VertexElementsCso slot can exist with no record + // behind it, because a backend that keys its twins on the handle mints the slot itself + // (DirectGLES' BackendSlotTable::GetOrCreate -> MGPipeSlots().Acquire) whether or not + // bit 8 ever asked this client to emit anything - which is exactly what a + // MOBILEGL_PIPE_PUSH=0x7f lane runs. delete_vertex_elements on such a handle is a + // REFUSED call, and the applier's resolver asserts on a refusal + // (PipeApply.cpp's ResolveVertexElements), i.e. a stop in a verify build. + // + // Kept OUT of Reset(), unlike the create/bind latch beside it, and for the mirror + // image of Reset()'s own reason: "a fresh context is a fresh server" is true of the + // per-context half of this table, and object RECORDS are precisely what + // MGPipeApplierReset does not clear (PipeApply.h's two halves). This half tracks those + // records, so it lives exactly as long as they do. + Bool RecordIsPublished(MGPipeHandle handle) const { + if (MGPipeHandleIsNull(handle)) return false; + const SizeT slot = handle.Slot; + if (slot >= m_latch.size()) return false; + const Latch& latch = m_latch[slot]; + return latch.RecordLive && latch.RecordGen == handle.Gen; + } + + // The record named by `handle` is gone from the applier. Also drops the bound-handle + // memo when it named it, so the client's idea of BoundVertexElements and the applier's + // (which MGPipeApplyDeleteVertexElements just cleared for the same handle) stay in + // step rather than diverging until the next bind happens to correct it. + void NoteRecordDestroyed(MGPipeHandle handle) { + if (MGPipeHandleIsNull(handle)) return; + const SizeT slot = handle.Slot; + if (slot < m_latch.size() && m_latch[slot].RecordGen == handle.Gen) { + m_latch[slot] = Latch{}; + } + if (m_boundHandle == handle) { + m_boundHandle = kMGPipeNullHandle; + m_boundLifetimeId = 0; + } + } + // A fresh context is a fresh server: the applier's records are gone, so every latch // this emitter holds describes objects the server no longer has. Called from the // validate point's FreshlyPrimed arm beside MGPipeApplierReset and the suppressor's // InvalidateAll, for the same reason they are. + // + // The PER-CONTEXT half only - see RecordIsPublished above for why RecordLive/RecordGen + // survive. Re-creating a configuration the applier already holds is a bounded + // over-fire (MGPipeApplyCreateVertexElements starts the record over); forgetting that + // it holds one at all would leak the record and its slot at the object's death. void Reset() { - m_latch.clear(); + for (Latch& latch : m_latch) { + latch.Published = false; + latch.Gen = 0; + latch.ConfigVersion = 0; + } m_boundHandle = kMGPipeNullHandle; m_boundLifetimeId = 0; } @@ -302,9 +351,16 @@ namespace MobileGL::MG_Pipe { private: struct Latch { + // The PER-CONTEXT half: "has this emitter told THIS server about this handle's + // configuration". Cleared by Reset() at every make-current. Bool Published = false; Uint32 Gen = 0; Uint32 ConfigVersion = 0; + // The RECORD half: "does the applier hold a create_vertex_elements record at this + // slot, for this generation". Lives as long as the record does - see + // RecordIsPublished. + Bool RecordLive = false; + Uint32 RecordGen = 0; }; static MGPHandleOnly HandleOnly(MGPipeHandle handle) { @@ -345,6 +401,10 @@ namespace MobileGL::MG_Pipe { latch.Published = true; latch.Gen = handle.Gen; latch.ConfigVersion = configVersion; + // THE ONE PRODUCER of the record half: a create that reached the applier is the + // only thing that makes delete_vertex_elements a legal call for this handle. + latch.RecordLive = true; + latch.RecordGen = handle.Gen; return sizeof(MGPVertexElements) + kAttribBytes + kBindingBytes; } diff --git a/MobileGL/MG_Pipe/PipeMutation.h b/MobileGL/MG_Pipe/PipeMutation.h index 29cea9cb..7083d024 100644 --- a/MobileGL/MG_Pipe/PipeMutation.h +++ b/MobileGL/MG_Pipe/PipeMutation.h @@ -124,6 +124,22 @@ namespace MobileGL::MG_Pipe { // re-handed-out, or a backend object nobody releases. Bool MGPipeEmitResourceDestroyAndFree(MG_State::GLState::BufferObject& buffer); + // THE VERTEX-ELEMENTS CSO's DEATH, and it is BACKEND-NEUTRAL - which is the whole point. + // Before this, the only thing that ever returned a VertexElementsCso slot was DirectGLES' + // StateObjectDeathOps table; under any backend that installs none - DirectVulkan/Magma, + // which keeps its own age-reclaimed identity table on purpose - every VAO ever created + // held its slot and its ~1.3 KB applier record for the life of the process, on the shipped + // 0x1ff mask, and past 65536 slots every create_vertex_elements became a permanent + // Fatal{ProtocolCorruption}. The client mints the slot, so the client is where the death + // has to be spoken from. + // + // Takes the lifetime id and not the object for StateObjectDeathNotice.h's reason: the last + // SharedPtr has already dropped by the time this runs, and the lifetime id is what the + // slot allocator resolves the handle from. Returns whether delete_vertex_elements went + // out, i.e. whether the applier actually held a record - see the definition for why that + // is asked rather than assumed. + Bool MGPipeEmitVertexElementsDestroyAndFree(Uint64 lifetimeId); + void MGPipeEmitResourceCreate(MG_State::GLState::BufferObject& buffer); void MGPipeEmitResourceRespecify(MG_State::GLState::BufferObject& buffer); void MGPipeEmitResourceSubData(MG_State::GLState::BufferObject& buffer, SizeT offset, SizeT size); diff --git a/MobileGL/MG_State/GLState/VertexArrayState/VertexArrayObject.cpp b/MobileGL/MG_State/GLState/VertexArrayState/VertexArrayObject.cpp index 64aef896..fe9222ff 100644 --- a/MobileGL/MG_State/GLState/VertexArrayState/VertexArrayObject.cpp +++ b/MobileGL/MG_State/GLState/VertexArrayState/VertexArrayObject.cpp @@ -8,8 +8,6 @@ #include "VertexArrayObject.h" -#include - #include #include @@ -42,15 +40,24 @@ namespace MobileGL::MG_State::GLState { #if MOBILEGL_PIPE_PUSH VertexArrayObject::~VertexArrayObject() { - // P2 step e2: ANNOUNCE the death instead of leaving the backend to discover it in a - // garbage sweep. This is the last SharedPtr to this object dropping - not the - // glDelete* that only marks the name and leaves a still-bound object very much - // alive - so it is the exact moment the backend's twin, and the driver storage - // that twin owns, stop being reachable. The notice carries the lifetime id - // because the object no longer exists to be passed, and because the lifetime id - // is what the client slot allocator resolves the handle from. No-op unless a - // backend registered the ops (a pull build declares none at all). - NotifyStateObjectDestroyed(MG_Pipe::MGPipeKind::VertexElementsCso, m_lifetimeId); + // P2 step e2 / P3a C-1: ANNOUNCE the death instead of leaving the backend to discover + // it in a garbage sweep, and RETURN THE CLIENT'S OWN SLOT while doing it. This is the + // last SharedPtr to this object dropping - not the glDelete* that only marks the name + // and leaves a still-bound object very much alive - so it is the exact moment the + // backend's twin, the driver storage that twin owns, the applier's vertex-elements + // record and the {slot, gen} that names all three stop being reachable. + // + // ALL FOUR OF THOSE GO THROUGH ONE HELPER, and it is the client's rather than a + // backend's: the handle is minted by MGPipeVertexInputEmitter on every backend, so a + // death path that only exists inside a backend's death-ops table is no path at all + // under a backend that installs none - which is what DirectVulkan/Magma does on + // purpose, and what made every VAO leak a slot and a ~1.3 KB applier record for the + // life of the process on the shipped mask. The helper emits delete_vertex_elements, + // raises the notice (a no-op unless a backend registered the ops) and frees the slot, + // in that fixed order; MG_Pipe/PipeMutation.h and its definition say why each position + // is where it is. The buffer's death has exactly this shape one file over + // (BufferObject.cpp -> MGPipeEmitResourceDestroyAndFree). + MG_Pipe::MGPipeEmitVertexElementsDestroyAndFree(m_lifetimeId); } #endif