From 4066f84d113b8fecc4fd08ab60ccc3bbc02bbc9a Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Sat, 19 Sep 2026 23:41:09 -0400 Subject: [PATCH] P5f fr: retire all apply allocator scope exemptions --- MobileGL/MG_Impl/Pipe/SlotAllocator.cpp | 94 +--------------------- MobileGL/MG_Impl/Pipe/SlotAllocator.h | 76 +++-------------- MobileGL/MG_Test/Wire/RemoteClientTest.cpp | 81 ++++++------------- 3 files changed, 37 insertions(+), 214 deletions(-) diff --git a/MobileGL/MG_Impl/Pipe/SlotAllocator.cpp b/MobileGL/MG_Impl/Pipe/SlotAllocator.cpp index 21cb4c0f..2c0b2a8d 100644 --- a/MobileGL/MG_Impl/Pipe/SlotAllocator.cpp +++ b/MobileGL/MG_Impl/Pipe/SlotAllocator.cpp @@ -11,9 +11,6 @@ #if MOBILEGL_BUILD_DISAGGREGATED #include -// P5e (id): the guard's exemption is keyed on whether the record being applied is BARRIERED -// (CONTRACT-P5E §4.4), and MGPipeApplierCurrentRecordIsBarriered() is where ApplyOne stamps -// that. Declarations only - this file names no applier state. #include #include #include @@ -32,28 +29,15 @@ namespace MobileGL::MG_Pipe { void MGPipeRefuseAllocatorFromApplyThread(const char* entry) { if (MG_Config::Transport == MG_Config::TransportMode::Monolith) return; if (!MG_Remote::Server::ServerLoop::OnApplyThread()) return; - // P5f: frontend-keyed registry probes are no longer an exemption. The - // separate Magma allocator debt remains backend- and barrier-keyed until - // its remaining monolith glue is removed; it cannot exempt twin-table APIs. - if (MGPipeApplierCurrentRecordIsBarriered()) { - // Ruling 12: Magma's four P7 debts, and ONLY on a DirectVulkan server. The key is - // on the backend kind rather than on the site because the scope is a class anyone - // can construct, and an Espryt probe borrowing Magma's exemption is exactly the - // hole P5e is closing. - if (MagmaP7AllocatorDebtScope::ActiveOnApplyThread() && - MG_Config::ActiveBackendType == BackendType::DirectVulkan) { - return; - } - - } + // P5f (fr): all named allocator exemptions are retired. A barrier can + // stabilize a client allocator but cannot make it server-owned memory. MGLOG_F("MGPipe: Fatal{RoleViolation, \"MGPipeSlots\"} - the apply thread called " "MGPipeSlots().%s. With an active transport the client slot allocator is " "client-only memory (CONTRACT-P5C §3.1, rule E; CONTRACT-P5E §4.4): a handle " "arrives already minted in a record, and a server that resolves or mints one " "off a frontend object's lifetime id is reading memory that will not exist on " - "its side of a real split. Only Magma's allocator debt scope admits it while the " - "record being applied is BARRIERED (barriered=%d) and, for Magma's P7 debt, " - "only on a DirectVulkan server", + "its side of a real split. No scope or backend exempts a barriered record " + "(barriered=%d)", entry, MGPipeApplierCurrentRecordIsBarriered() ? 1 : 0); std::abort(); } @@ -71,76 +55,6 @@ namespace MobileGL::MG_Pipe { std::abort(); } - namespace { - // NOT thread_local any more, and not atomic either (P5d round 3, package D). - // - // WHY THERE IS NO DATA RACE. Both depths have exactly ONE reader in the whole tree: - // MGPipeRefuseAllocatorFromApplyThread above, whose first two lines return unless the - // transport is active AND ServerLoop::OnApplyThread() is true. So the only thread whose - // depth could ever change an answer is the apply thread - and the four scope bodies - // below now increment and decrement ONLY when OnApplyThread() says so. That makes the - // apply thread the single writer and the single reader of both counters; no other - // thread touches them, and a monolith process never even reads them (the guard's first - // line returns on TransportMode::Monolith and there is no apply thread to make the - // second true). Single-writer-single-reader on one thread needs no lock, no atomic and - // no TLS. - // - // WHY IT WAS WORTH DOING. A shared-library thread_local costs an __emutls_get_address - // call per access. On the MONOLITH's GL thread that symbol is the TOP entry of the - // profile at 7.7%, and these two scopes' ctor/dtor are 32.7% of its samples - // (MGPipeFrontendKeyedRegistryScope 18.75 ctor + 13.95 dtor, the scope P5e renamed - // MagmaP7AllocatorDebtScope 8.4); on the split's apply thread emutls is 7.4%. The - // measurement predates the rename and the numbers are quoted as measured. The scope is - // constructed at ~20 - // sites in DirectGLES.cpp, several inside StateBackendObjectRegistry::HandleOf, which - // is per-draw. The cost on the GL thread is now one inlined predicate per end. - // - // The query's meaning ON THE APPLY THREAD is unchanged, which is the only meaning - // the guard reads. Off it the answer is now always false, and the query is named - // ActiveOnApplyThread() rather than Active() so that a reader who needs the other - // meaning cannot ask for it by accident: whoever wants "is a scope open on THIS - // thread" has to add it, and move the counting back, rather than read a false that - // looks like an answer. - Uint32 g_magmaP7AllocatorDebtScopeDepth = 0; - } - - MagmaP7AllocatorDebtScope::MagmaP7AllocatorDebtScope() - // Decided ONCE and remembered: the destructor must undo exactly what the constructor - // did, and re-asking the predicate would leak a count across a scope that straddled - // the apply thread's exit block. - : m_counted(MG_Remote::Server::ServerLoop::OnApplyThread()) { - if (m_counted) ++g_magmaP7AllocatorDebtScopeDepth; - } - - MagmaP7AllocatorDebtScope::~MagmaP7AllocatorDebtScope() { - if (m_counted) --g_magmaP7AllocatorDebtScopeDepth; - } - - // The DEPTH is not backend-keyed and deliberately so: the backend kind is read by the - // GUARD, once, at the moment it decides. Counting only on DirectVulkan would make the - // depth's meaning depend on a global that a test can move between the constructor and the - // destructor, which is the m_counted bug one level out. - Bool MagmaP7AllocatorDebtScope::ActiveOnApplyThread() { - return g_magmaP7AllocatorDebtScopeDepth != 0; - } - - namespace { - // Same counter, same argument, same single reader - see the block above. - Uint32 g_frontendKeyedRegistryScopeDepth = 0; - } - - MGPipeFrontendKeyedRegistryScope::MGPipeFrontendKeyedRegistryScope() - : m_counted(MG_Remote::Server::ServerLoop::OnApplyThread()) { - if (m_counted) ++g_frontendKeyedRegistryScopeDepth; - } - - MGPipeFrontendKeyedRegistryScope::~MGPipeFrontendKeyedRegistryScope() { - if (m_counted) --g_frontendKeyedRegistryScopeDepth; - } - - Bool MGPipeFrontendKeyedRegistryScope::ActiveOnApplyThread() { - return g_frontendKeyedRegistryScopeDepth != 0; - } #endif namespace { diff --git a/MobileGL/MG_Impl/Pipe/SlotAllocator.h b/MobileGL/MG_Impl/Pipe/SlotAllocator.h index 7316112e..614d1ea3 100644 --- a/MobileGL/MG_Impl/Pipe/SlotAllocator.h +++ b/MobileGL/MG_Impl/Pipe/SlotAllocator.h @@ -173,17 +173,8 @@ namespace MobileGL::MG_Pipe { // pull build's bytes do not move (G1). void MGPipeRefuseAllocatorFromApplyThread(const char* entry); - // ---- P5e (id, CONTRACT-P5E §4.4): THE CONDITION UNDER WHICH A NAMED SCOPE EXEMPTS ----- - // - // "This thread is applying a record the client is NOT parked behind." Both scopes below - // exempt a probe only while it is FALSE, because the client's wait is the whole of what - // makes a frontend read from the apply thread safe: behind a barriered record the client - // is blocked in WaitForApplied and its memory is stable; behind an unbarriered one it is - // running ahead and the same read is torn or stale BY CONSTRUCTION, which is why rule F - // has no "count it" arm and this answer does not consult MOBILEGL_IPC_STRICT_ERRORS. - // - // False in a monolith process and on any thread but the apply thread, so a GL-thread - // caller is never refused by it. + // Shared predicate for remaining field/verb guards. Registry and allocator + // guards below refuse both barriered and unbarriered transport apply. Bool MGPipeApplierIsUnbarrieredApply(); // P5f (fr): frontend-object twin lookup, minting and weak-state access are refused @@ -192,71 +183,24 @@ namespace MobileGL::MG_Pipe { // glue. This separate guard also prevents borrowing Magma's allocator exemption. void MGPipeRefuseFrontendKeyedRegistryFromApplyThread(const char* entry); - // The NAMED EXEMPTION to the rule above (CONTRACT-P5C §3.1, as amended by CONTRACT-P5E - // §4.4): the family of sites whose handle-carrying records the client does not EMIT yet. - // set_shader_buffers and set_stream_output_targets exist in the catalogue but are P4b/sb's - // to emit (SetHashSuppressor.h says so), so the buffer binding-point ensures and the - // GPU-written announcement they feed have no record handle to resolve from today. Inside - // this scope ONE read-only lifetime-id probe stays legal WHILE THE CURRENT RECORD IS - // BARRIERED; the scope is the debt's measurable, greppable form, and it retires with P7's - // server-side binding table. Every other apply-thread allocator access stays Fatal. - // - // THE DEPTH IS COUNTED ONLY ON THE APPLY THREAD (P5d round 3, package D). The counter's - // only reader is MGPipeRefuseAllocatorFromApplyThread, which returns before it looks unless - // ServerLoop::OnApplyThread() is true, so a depth kept on any OTHER thread could never - // change an answer - it was pure cost. The query is called ActiveOnApplyThread() and not - // Active() BECAUSE OF THAT (review round 3): a name that promised "is a scope open" would - // now be quietly answering "is a scope open ON THE APPLY THREAD", and the next reader to - // come along - a P4b/P7 probe deciding on the GL thread whether to emit a record - would - // read a truthful-looking false and take the wrong branch with nothing to warn it. The name - // carries the precondition so a second reader has to notice it. m_counted remembers what the constructor decided so - // the destructor undoes exactly what the constructor did; asking the predicate twice would - // leak a count for a scope that outlived the apply thread. On the GL thread (and in every - // monolith process) both ends are now one inlined predicate and a branch instead of an - // emutls call, which is where 32.7% of the monolith GL thread's __emutls_get_address - its - // top symbol at 7.7% - was going. - // - // P5e (id), ruling 12: RENAMED FROM MGPipeReverseAnnouncementScope AND KEYED ON THE - // BACKEND KIND. The rename is the finding: what is left inside it after P5e is not "the - // reverse announcement" - Espryt's half of that moved to the frontend-keyed scope with the - // rest of the registry debt - it is MAGMA'S FOUR APPLY-THREAD ALLOCATOR TOUCHES, which P7 - // retires (VulkanRenderer.cpp's two hidden-resource shutdowns and its named-blit endpoint - // resolve, plus MGPipeAnnounceBufferGpuWritten in ResourceTracker.h). Naming the scope - // after the debt rather than after one of its sites is what makes "has P7 landed yet" a - // grep; naming it after Magma is what stops a new Espryt site being wrapped in it. - // - // THE EXEMPTION IS REFUSED WHEN THE SERVER BACKEND IS NOT DirectVulkan, and that is the - // point of the key rather than a safety belt. Magma stays lockstep for the whole of P5e - // (§6.1: the DirectVulkan arm never publishes kCapRunAheadApply, so every record there is - // barriered and every probe inside this scope keeps P5C's semantics). Espryt is what P5e - // is retiring the lockstep for, so an Espryt probe must not be able to borrow Magma's - // exemption - even by wrapping itself in Magma's scope. + // P5f (fr): these historical scope names remain greppable markers around + // monolith glue until P3b/P4b remove those bodies. They hold no state and grant + // no exemption: allocator and frontend-registry access always refuse apply. class MagmaP7AllocatorDebtScope { public: - MagmaP7AllocatorDebtScope(); - ~MagmaP7AllocatorDebtScope(); + MagmaP7AllocatorDebtScope() = default; + ~MagmaP7AllocatorDebtScope() = default; MagmaP7AllocatorDebtScope(const MagmaP7AllocatorDebtScope&) = delete; MagmaP7AllocatorDebtScope& operator=(const MagmaP7AllocatorDebtScope&) = delete; - static Bool ActiveOnApplyThread(); - - private: - Bool m_counted; }; - // P5f (fr): retained as a greppable marker around monolith-only legacy registry - // calls. This scope no longer exempts transport apply from either registry or - // allocator guards, even behind a barrier. P3b/P4b can remove these markers with - // the surrounding monolith glue; adding one cannot make a new wire fallback legal. class MGPipeFrontendKeyedRegistryScope { public: - MGPipeFrontendKeyedRegistryScope(); - ~MGPipeFrontendKeyedRegistryScope(); + MGPipeFrontendKeyedRegistryScope() = default; + ~MGPipeFrontendKeyedRegistryScope() = default; MGPipeFrontendKeyedRegistryScope(const MGPipeFrontendKeyedRegistryScope&) = delete; MGPipeFrontendKeyedRegistryScope& operator=(const MGPipeFrontendKeyedRegistryScope&) = delete; - static Bool ActiveOnApplyThread(); - - private: - Bool m_counted; }; + #endif } // namespace MobileGL::MG_Pipe diff --git a/MobileGL/MG_Test/Wire/RemoteClientTest.cpp b/MobileGL/MG_Test/Wire/RemoteClientTest.cpp index 0f042147..3bd50a3e 100644 --- a/MobileGL/MG_Test/Wire/RemoteClientTest.cpp +++ b/MobileGL/MG_Test/Wire/RemoteClientTest.cpp @@ -1898,29 +1898,9 @@ MGL_BUFFER_GUARD_TEST(BufferSyncPersistentMappedRangeFromTheApplyThreadIsFatalBy MGL_BUFFER_GUARD_TEST(BufferHasDefinedContentFromTheApplyThreadIsFatalByName, 707, (void)buffer.HasDefinedContent()) #undef MGL_BUFFER_GUARD_TEST -// P5d round 3 (package D): the two NAMED EXEMPTION scopes of CONTRACT-P5C §3.1 / §5.4 stopped -// keeping their depth in a thread_local and now count ONLY while ServerLoop::OnApplyThread() is -// true (SlotAllocator.cpp). The claim that makes that legal is "the apply-thread guard is the -// counter's only reader, so a depth kept on any other thread cannot change an answer" - and -// these three cases are that claim in executable form. They are the red-once for the change: -// -// (a) drop the `if (m_counted)` from the constructor (or make it unconditional again on a -// thread that is not the apply thread) and (c) goes red - a GL-thread scope would start -// exempting the apply thread, which is exactly the leak the thread_local used to prevent; -// (b) drop the increment altogether and (a) goes red - the exemption stops exempting and -// the debt's own sites (DirectGLES' HandleOf probes) abort the server; -// (c) drop the guard's `MagmaP7AllocatorDebtScope::ActiveOnApplyThread()` / -// `FrontendKeyedRegistryScope::ActiveOnApplyThread()` rows and (b) - the control - stops -// being the only aborting arm. -// -// P5e (id), CONTRACT-P5E §4.4 REPLACED (b). The case that used to sit there - -// `AnAllocatorProbeInsideAnExemptionScopeOnTheApplyThreadIsAllowed` - asserted that a scope -// exempts UNCONDITIONALLY, which is exactly the rule §4.4 retires: an exemption is a debt the -// CLIENT'S WAIT pays for, so it holds only while the record being applied is barriered. Its -// replacement below drives the same scope with the barriered flag forced false and expects the -// abort. The green half of the old case did not need a test of its own: every Espryt -// `integration-split` entry runs the frontend-keyed scope's surviving sites on the apply thread -// behind barriered records, so a guard that stopped exempting would take the whole lane down. +// Historical P5d/P5e probes remain named for compatibility. P5f retires both +// scope exemptions, including barriered records; all allocator access on apply is +// client-memory access regardless of whether that memory happens to be stable. TEST(RemoteGuards, AnAllocatorProbeFromTheApplyThreadOutsideEveryScopeIsFatalByName) { const auto child = RunInChild([] { StartControlSession(); @@ -1935,24 +1915,8 @@ TEST(RemoteGuards, AnAllocatorProbeFromTheApplyThreadOutsideEveryScopeIsFatalByN ExpectNamedAbort(child, "Fatal{RoleViolation, \"MGPipeSlots\"}"); } -// P5e (id) RED-ONCE, CONTRACT-P5E §4.4. The exemption scope is OPEN and the probe is the one -// the debt is named for - `HandleOf`, the frontend-keyed registry's own entry - and it still -// aborts, because the record being applied is unbarriered: the client is running ahead and the -// allocator's free list and lifetimeId -> slot map are moving under the read. -// -// THE HOOK IS THE APPLIER FIELD ITSELF and that is deliberate: `CurrentRecordBarriered` is what -// PipeApplier::ApplyOne stamps from MGPipeBarriered() on every record, so forcing it here drives -// the production path rather than a test-only branch beside it. Until ra lands the wait rule -// MGPipeBarriered answers true for every record, so this is the ONLY way to reach the arm - and -// the arm has to exist before ra, because ra is what makes it reachable in production. -// -// The red: put §4.4's `if (MGPipeApplierCurrentRecordIsBarriered())` back to an unconditional -// exemption in MGPipeRefuseAllocatorFromApplyThread and this case stops aborting - red by -// expectation, which is the shape a guard case fails in. -// EVERYTHING BUT THE PROBE RUNS ON THE CLIENT THREAD, which is the buffer/texture guard cases' -// shape one block down and is load-bearing here rather than tidy: a TextureObject2D's -// constructor MINTS a resource handle, and building it on the apply thread would abort at the -// mint with the same diagnostic - a case that passes for a reason other than the one it names. +// Construct on the client and run only HandleOf on apply. The false barrier stamp +// keeps the original P5e scenario; the tests below add the old barriered loophole. TEST(RemoteGuards, AnAllocatorProbeInsideAnExemptionScopeFromTheApplyThreadIsFatalEvenAtTheOldDebtSites) { struct Probe { TestTextureTable* table; @@ -1983,18 +1947,23 @@ TEST(RemoteGuards, AnAllocatorProbeInsideAnExemptionScopeFromTheApplyThreadIsFat // P5f (fr): barriers and old named scopes no longer admit frontend identity. // Construct the object and table on the client so each death comes from the exact // probed member, not a constructor's resource_create or a missing record. -TEST(RemoteGuards, BarrieredFrontendRegistryScopeCannotExemptAllocator) { - const auto child = RunInChild([] { - StartControlSession(); - Srv::ServerLoopInstance().RunProbeOnApplyThreadForTesting(+[](void*) -> MobileGLResult { - MG_Pipe::MGPipeApplierSetCurrentRecordBarriered(true); - const MG_Pipe::MGPipeFrontendKeyedRegistryScope scope; - MG_Pipe::MGPipeRefuseAllocatorFromApplyThread("FindByLifetimeId"); - return MOBILEGL_OK; - }, nullptr); - ClientSessionInstance().Stop(); - }); - ExpectNamedAbort(child, "Fatal{RoleViolation, \"MGPipeSlots\"}"); +TEST(RemoteGuards, BarrieredLegacyScopesCannotExemptAllocator) { + for (const auto backend : {BackendType::DirectGLES, BackendType::DirectVulkan}) { + const auto child = RunInChild([backend] { + StartControlSession(); + auto selectedBackend = backend; + Srv::ServerLoopInstance().RunProbeOnApplyThreadForTesting(+[](void* self) -> MobileGLResult { + MG_Pipe::MGPipeApplierSetCurrentRecordBarriered(true); + MG_Config::ActiveBackendType = *static_cast(self); + const MG_Pipe::MGPipeFrontendKeyedRegistryScope registryScope; + const MG_Pipe::MagmaP7AllocatorDebtScope magmaScope; + MGPipeSlots().FindByLifetimeId(MGPipeKind::Texture, 99); + return MOBILEGL_OK; + }, &selectedBackend); + ClientSessionInstance().Stop(); + }); + ExpectNamedAbort(child, "Fatal{RoleViolation, \"MGPipeSlots\"}"); + } } TEST(RemoteGuards, BarrieredFrontendRegistryMembersRefuseBothLegacyScopes) { @@ -2062,11 +2031,7 @@ TEST(RemoteGuards, HandleRegistryMembersWorkOnBarrieredAndUnbarrieredApply) { EXPECT_EQ(WEXITSTATUS(child.Status), 0) << child.Log; } -// THE ONE THAT PINS THE SINGLE-WRITER ARGUMENT. The scope is open on the GL thread for the -// whole of the posted request; the apply thread's probe must still abort, because the exemption -// belongs to the thread that entered the scope and never to another one. With the depth kept in -// a plain counter that every thread incremented, this case would go green - and a real -// apply-thread violation would be silently exempted for as long as any GL thread held a scope. +// A marker held by the GL thread never affects the apply guard either. TEST(RemoteGuards, AnExemptionScopeHeldOnTheGLThreadDoesNotExemptTheApplyThread) { const auto child = RunInChild([] { StartControlSession();