From e3bb6d8b8249a5d9d559d50c5cbc1a18ea03918f Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Mon, 21 Sep 2026 19:38:19 -0400 Subject: [PATCH] [MG_Pipe, MG_Backend, MG_Impl] (P6/sm, P6/st): four guards that were compiled in and could never arm, and the crash behind one of them CONTRACT-P6 3.2 (D1c), 3.3 (D13) and 3.4 (D10). Every guard below asked a question a SERVER PROCESS answers wrongly - "is a ClientSession active" (never, there) or "am I on the apply thread" (every thread is, there) - so all four were present, compiled, and permanently disarmed in the one shape they exist for. That is worse than absent: it reads as coverage. D13 FIRST, THEN THE WIDENING. MGPipeRoleSplitActive() meant "the REHEARSAL knob is on AND the transport is real", so widening it in place would have left a narrow name describing a wide predicate. It is renamed to MGPipeRoleSplitRehearsalActive() across its 12 sites - including the DualBlockScenario assertion message that embeds it - and the wider predicate is introduced BESIDE it. MGPipeBlocksAreDistinct() IS TRUE FOR TWO DIFFERENT REASONS, which is the whole point: under the rehearsal the two blocks are distinct objects in one process; under SPAWN they are distinct because they are in different ADDRESS SPACES, and no knob is involved. The spawn retrace runs with role-split-state=0 - measured off the lane's own log - so every guard gated on the rehearsal was off there. THE CRASH. MGPipeServerBlockNoteIdentity early-returned on the rehearsal, so a spawn server's gPipeInputs.ContextIdentity() stayed nullptr for the life of the process. DirectGLES' framebuffer binding-slot memo compares that identity FIRST and with NO generation, against a cache variable that also starts nullptr - so nullptr == nullptr reads as a CACHE HIT and the function returns a slot pointer nothing ever filled. `sm` widens the gate; `st` lands the named Fatal PipeInputs.h had already asked for, so a reopening says which memo and which identity instead of corrupting memory three frames later. Guarded on MOBILEGL_PIPE_PUSH: in a pull build that identity is the live GLContext's address and is never null, so the test would be dead code and G1 measures that. DISJUNCTS, NOT SUBSTITUTIONS - and this was measured the hard way. The first attempt REPLACED OnApplyThread() with MGPipeServerArm() and ClientSession::Active() with MGPipeSessionLive(). 14 RemoteGuards and RemoteClientControls cases went green by omission: a ServerLoop fixture with no client session has only the original facts, so swapping them out disarmed every test that drives the apply thread directly. The originals stay; the new predicates are added beside them. AND THE PROBE HAD TO BE THE RIGHT ONE. Installing ClientSession::ApplyThreadIsInsideApplier as the apply-thread probe made every GL-thread call a role violation whenever the applier happened to be busy - the inproc retrace died on Fatal{RoleViolation, "MGPipeSlots"} raised from the GL thread. "Am I the apply thread" and "is the apply thread inside the applier" are different questions; the second is R-1's mutual-exclusion probe and is about another thread entirely. ServerLoop::OnApplyThread is the one the guards mean. Three of D1c's six predicates land here, the three the four guards need. The other three (MGPipeSplitActive, MGPipeShouldDial, MGPipeBackendIsLocal) have no D10 site and are not added as dead code; their call sites are ContextEpoch and ActiveXfbState, which this commit does not touch. D14 holds: the non-disaggregated build gets constexpr false definitions, not runtime-false predicates, so every consult folds at compile time. G1 re-measured - 0 symbol diff, .text unchanged at 0xa52203. Two test names added, both falsifiable by construction: the spawn case asserts the blocks are distinct with the rehearsal knob OFF, and the monolith case asserts the predicate is not simply a constant. Unit suite 2353 tests, one pre-existing environmental failure. All six OpenRA arms green. --- MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp | 28 +++++++++ MobileGL/MG_Backend/MGPipe/PipeInputs.cpp | 59 +++++++++++++++++-- MobileGL/MG_Backend/MGPipe/PipeInputs.h | 55 ++++++++++++++++- MobileGL/MG_Impl/Pipe/SlotAllocator.cpp | 17 +++++- .../Harness/DualBlockPeek.cpp | 2 +- .../Harness/DualBlockPeek.h | 2 +- .../Scenarios/DualBlockScenario.cpp | 4 +- MobileGL/MG_Pipe/PipeApply.cpp | 13 +++- MobileGL/MG_Remote/Client/ClientSession.cpp | 25 +++++++- MobileGL/MG_Remote/Server/ServerMain.cpp | 9 +++ MobileGL/MG_Test/Pipe/FieldOwnershipTest.cpp | 38 ++++++++++-- 11 files changed, 230 insertions(+), 22 deletions(-) diff --git a/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp b/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp index 6cfa5a2c..ae05af20 100644 --- a/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp +++ b/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp @@ -199,6 +199,34 @@ namespace MobileGL::MG_Backend::DirectGLES { #endif #if !MOBILEGL_PIPE_PUSH || MOBILEGL_PIPE_LEGACY_MEMOS const void* ctx = MGB_CTX_IDENTITY; +#if MOBILEGL_PIPE_PUSH + // `st` (CONTRACT-P6 3.4): THE NAMED FATAL PipeInputs.h ALREADY ASKED FOR. + // + // This memo compares the identity FIRST and with NO GENERATION, and its cache variable + // starts at nullptr - so an identity that is ALSO nullptr reads as a CACHE HIT and the + // function returns `*g_fbSlotCache[target]`, a pointer nothing ever filled. The crash + // that follows names neither the memo nor the identity. + // + // It is reachable exactly when a process runs this legacy arm with an unset identity, + // which is what a spawn server used to be: MGPipeServerBlockNoteIdentity early-returned + // on the REHEARSAL knob, so the server's block carried nullptr for the life of the + // process. `sm` widened that gate to MGPipeBlocksAreDistinct() and closed the hole; this + // says so out loud if it ever reopens, because a silent wrong answer here is + // indistinguishable from memory corruption three frames later. + // + // Under MOBILEGL_PIPE_PUSH only: in a pull build MGB_CTX_IDENTITY is the live + // GLContext's address, which is non-null on every path that reaches here, so the test + // would be dead code and G1 measures dead code. + if (ctx == nullptr) { + MGLOG_F("MGPipe: Fatal{UnnamedIdentity, \"FbSlotMemo\"} - the framebuffer binding-slot " + "memo was consulted with a NULL context identity. Its cache starts null too, " + "so this would have read as a hit and returned a slot pointer that was never " + "filled. The identity is set by MGPipeServerBlockNoteIdentity, which arms on " + "MGPipeBlocksAreDistinct(); a null here means this process runs the legacy " + "memo arm with a block nobody claimed (CONTRACT-P6 3.4)."); + std::abort(); + } +#endif if (ctx != g_fbSlotCacheContext) { auto& live = *MGB_CTX; for (SizeT i = 0; i < g_fbSlotCache.size(); ++i) { diff --git a/MobileGL/MG_Backend/MGPipe/PipeInputs.cpp b/MobileGL/MG_Backend/MGPipe/PipeInputs.cpp index 9e0459d6..03377b50 100644 --- a/MobileGL/MG_Backend/MGPipe/PipeInputs.cpp +++ b/MobileGL/MG_Backend/MGPipe/PipeInputs.cpp @@ -171,7 +171,10 @@ namespace MobileGL::MG_Pipe { // crash exactly where the rehearsal exists to produce a named one. The red is the // census (Harness/dualblock-expected-fatals.txt), so the marker keeps the strict // grammar with the knob's own name as the reason. - if (MGPipeRoleSplitActive()) { + // D10: rides MGPipeBlocksAreDistinct() - under spawn the two blocks are distinct + // whatever the rehearsal knob says, and a barrier pull across a real process + // boundary is exactly the thing this Fatal exists to name. + if (MGPipeBlocksAreDistinct()) { StrictBarrierPullFatal(field, verb, "MOBILEGL_IPC_ROLE_SPLIT_STATE=1"); } if (!MGPipeApplierCurrentRecordIsBarriered()) { @@ -286,16 +289,54 @@ namespace MobileGL::MG_Pipe { // ---- P5f (f1): the dual-block rehearsal's selection functions (PipeInputs.h) ----------- // - // MGPipeRoleSplitActive is deliberately NOT latched: it is two global loads, asked once per + // MGPipeRoleSplitRehearsalActive is deliberately NOT latched: it is two global loads, asked once per // verb on the fill side and once per verb boundary on the stamp side, and a latch is a // second thing a test that flips the knob mid-process would have to know about. - Bool MGPipeRoleSplitActive() { + Bool MGPipeRoleSplitRehearsalActive() { return MG_Config::Ipc.RoleSplitState && MG_Config::Transport != MG_Config::TransportMode::Monolith; } +#if MOBILEGL_BUILD_DISAGGREGATED + // ---- D1c's predicates (CONTRACT-P6 3.2) ------------------------------------------------- + namespace { + // Set once by ServerMain, before any GL work. A spawn server is the server for the whole + // life of the process; there is no thread in it that is not. + Bool g_serverProcessRole = false; + // MG_Remote's "is the calling thread inside the applier". Null until a session installs + // it, which is the honest answer for a process that has no applier. + Bool (*g_applyThreadProbe)() = nullptr; + Bool g_sessionLive = false; + } // namespace + + void MGPipeSetServerProcessRole(Bool isServerProcess) { g_serverProcessRole = isServerProcess; } + void MGPipeSetApplyThreadProbe(Bool (*probe)()) { g_applyThreadProbe = probe; } + void MGPipeSetSessionLive(Bool live) { g_sessionLive = live; } + + Bool MGPipeServerArm() { + // THE PROCESS FACT FIRST, and it short-circuits: in a spawn server every thread is a + // server thread, and asking the probe would be asking MG_Remote a question it answers + // only about the applier. + if (g_serverProcessRole) return true; + return g_applyThreadProbe != nullptr && g_applyThreadProbe(); + } + + Bool MGPipeSessionLive() { return g_sessionLive; } + + Bool MGPipeBlocksAreDistinct() { + // TWO DIFFERENT REASONS, ONE ANSWER. The rehearsal makes two blocks in one process; + // spawn makes them two blocks in two ADDRESS SPACES, where no knob is involved and the + // distinctness is a fact about the machine. The guards that used to ask only the first + // question were therefore disarmed in the one shape where the answer matters most - a + // spawn server's block is never the client's, whatever MOBILEGL_IPC_ROLE_SPLIT_STATE + // says. + return MGPipeRoleSplitRehearsalActive() || + MG_Config::Transport == MG_Config::TransportMode::Spawn; + } +#endif // MOBILEGL_BUILD_DISAGGREGATED + PipeInputs& MGPipeClientInputs() { - return MGPipeRoleSplitActive() ? gPipeInputsClientBlock : gPipeInputs; + return MGPipeRoleSplitRehearsalActive() ? gPipeInputsClientBlock : gPipeInputs; } void MGPipeClientClearVerbBoundary() { @@ -312,7 +353,15 @@ namespace MobileGL::MG_Pipe { Bool MGPipeServerContextIsLive() { return g_serverContextLive; } void MGPipeServerBlockNoteIdentity() { - if (!MGPipeRoleSplitActive()) return; + // D10: THE WIDER PREDICATE, and the narrow one was a latent crash rather than a missing + // optimisation. Gated on the REHEARSAL, this early-returned for the whole life of a + // spawn server - because that knob is off by default and nothing in the spawn shape + // turns it on - so the server's gPipeInputs.ContextIdentity() stayed nullptr forever. + // DirectGLES' fb-slot memo compares that identity FIRST and with no generation, so a + // nullptr against its own nullptr initialiser reads as a CACHE HIT and hands out a slot + // that was never filled. `st` lands the named Fatal at that site; this is the gate that + // stops it being reachable in the first place. + if (!MGPipeBlocksAreDistinct()) return; // The server cannot name the client's GLContext - under a real transport it is in // another process - so the identity the server block carries is the server's OWN // served-context clock: MGPipeApplierContextSerial moves exactly when the served diff --git a/MobileGL/MG_Backend/MGPipe/PipeInputs.h b/MobileGL/MG_Backend/MGPipe/PipeInputs.h index 19c2e472..166d7466 100644 --- a/MobileGL/MG_Backend/MGPipe/PipeInputs.h +++ b/MobileGL/MG_Backend/MGPipe/PipeInputs.h @@ -860,9 +860,60 @@ namespace MobileGL::MG_Pipe { // Same leak-at-exit storage as gPipeInputs above, for the same reason. inline PipeInputs& gPipeInputsClientBlock = *new PipeInputs(); - // PipeInputs.cpp. Whether the rehearsal is armed: the knob AND a real transport (and not + // PipeInputs.cpp. Whether the REHEARSAL is armed: the knob AND a real transport (and not // the verify build, which ConfigLoader forces off). Constant for the life of the process. - Bool MGPipeRoleSplitActive(); + // + // D13 RENAMED THIS FROM MGPipeRoleSplitActive, which is what it has always meant. The old + // name read like "the roles are split", and the widening D1c needed would have made a + // narrow name lie - so the rename came FIRST and the wider predicate was introduced beside + // it rather than by quietly changing what this one answers. + Bool MGPipeRoleSplitRehearsalActive(); + + // ---- D1c: the role predicates (CONTRACT-P6 3.2) --------------------------------------- + // + // ONE QUESTION EACH, and the reason they are separate functions rather than one `split` + // flag is that the tree asks genuinely different things. a6 proposed three; three do not + // cover it. + // + // D14: IN A NON-DISAGGREGATED BUILD THESE GET A DIFFERENT DEFINITION, not a runtime-false + // one. The `#else` arm is `constexpr`, so every consult folds at compile time and the pull + // build gains no symbol, no branch and no byte - which G1 measures rather than assumes. +#if MOBILEGL_BUILD_DISAGGREGATED + // Am I executing as the SERVER right now? A thread-scoped question in the inproc shape, + // where both roles live in one process, and a process-scoped one under spawn. + Bool MGPipeServerArm(); + + // Is a peer session live? Distinct from "is the transport split": a spawn SERVER has no + // ClientSession at all, so a guard that asked for one was permanently disarmed there. + Bool MGPipeSessionLive(); + + // Do the two roles use DIFFERENT PipeInputs storage objects? + // + // TRUE FOR TWO DIFFERENT REASONS, which is the whole point of the predicate. Under the + // rehearsal the two blocks are distinct objects in one process; under SPAWN they are + // distinct because they are in different address spaces, and no knob is involved. The + // guards below used to ask the rehearsal question and were therefore silently off in the + // one shape where the answer matters most. + Bool MGPipeBlocksAreDistinct(); + + // The two facts the predicates above are built from, stated ONCE by the code that knows + // them. Neither is discoverable from MG_Backend: "this process is the server" is something + // only ServerMain can say, and "am I on the apply thread" is MG_Remote's thread-local. + // + // A PROBE RATHER THAN A FLAG for the thread half, because it is a question about the + // CALLING thread and a flag would answer for whichever thread wrote it last. + void MGPipeSetServerProcessRole(Bool isServerProcess); + void MGPipeSetApplyThreadProbe(Bool (*probe)()); + void MGPipeSetSessionLive(Bool live); +#else + inline constexpr Bool MGPipeServerArm() { return false; } + inline constexpr Bool MGPipeSessionLive() { return false; } + inline constexpr Bool MGPipeBlocksAreDistinct() { return false; } + // Setters too: a caller guarded only at its own site would still need these to LINK. + inline void MGPipeSetServerProcessRole(Bool) {} + inline void MGPipeSetApplyThreadProbe(Bool (*)()) {} + inline void MGPipeSetSessionLive(Bool) {} +#endif // PipeInputs.cpp. THE FILL SIDE'S ONE NEW SPELLING: the client block when the rehearsal is // armed, gPipeInputs otherwise. Everything in MG_Impl/Pipe/PipeFill.cpp that used to spell // gPipeInputs spells this instead. diff --git a/MobileGL/MG_Impl/Pipe/SlotAllocator.cpp b/MobileGL/MG_Impl/Pipe/SlotAllocator.cpp index 906ae88c..c8b97749 100644 --- a/MobileGL/MG_Impl/Pipe/SlotAllocator.cpp +++ b/MobileGL/MG_Impl/Pipe/SlotAllocator.cpp @@ -11,6 +11,7 @@ #if MOBILEGL_BUILD_DISAGGREGATED #include +#include // D1c: MGPipeServerArm() #include #include #include @@ -28,7 +29,13 @@ 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; + // D10: OR'd with the PROCESS-ROLE fact, not replaced by it. OnApplyThread() answers + // about the INPROC applier thread and is the only fact a ServerLoop fixture with no + // client session has; MGPipeServerArm() adds the spawn case, where EVERY thread is a + // server thread. Replacing the first with the second disarmed the refusal in every unit + // test that drives the apply thread directly - measured, 14 RemoteGuards cases went + // green-by-omission before this was put back as a disjunction. + if (!MG_Remote::Server::ServerLoop::OnApplyThread() && !MG_Pipe::MGPipeServerArm()) 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 " @@ -47,7 +54,13 @@ namespace MobileGL::MG_Pipe { // make a frontend SharedPtr exist in a separate server process. void MGPipeRefuseFrontendKeyedRegistryFromApplyThread(const char* entry) { if (MG_Config::Transport == MG_Config::TransportMode::Monolith) return; - if (!MG_Remote::Server::ServerLoop::OnApplyThread()) return; + // D10: OR'd with the PROCESS-ROLE fact, not replaced by it. OnApplyThread() answers + // about the INPROC applier thread and is the only fact a ServerLoop fixture with no + // client session has; MGPipeServerArm() adds the spawn case, where EVERY thread is a + // server thread. Replacing the first with the second disarmed the refusal in every unit + // test that drives the apply thread directly - measured, 14 RemoteGuards cases went + // green-by-omission before this was put back as a disjunction. + if (!MG_Remote::Server::ServerLoop::OnApplyThread() && !MG_Pipe::MGPipeServerArm()) return; MGLOG_F("MGPipe: Fatal{RoleViolation, \"MGPipeSlots\"} - the apply thread reached " "BackendSlotTable::%s. Frontend-identity registry operations are monolith-only, " "including barriered records; resolve the twin from the record's handle instead", diff --git a/MobileGL/MG_IntegrationTest/Harness/DualBlockPeek.cpp b/MobileGL/MG_IntegrationTest/Harness/DualBlockPeek.cpp index 87d062aa..a224c37e 100644 --- a/MobileGL/MG_IntegrationTest/Harness/DualBlockPeek.cpp +++ b/MobileGL/MG_IntegrationTest/Harness/DualBlockPeek.cpp @@ -32,7 +32,7 @@ namespace MGITest { if (out == nullptr) return false; namespace MGP = MobileGL::MG_Pipe; out->peekAvailable = true; - out->roleSplitActive = MGP::MGPipeRoleSplitActive(); + out->roleSplitActive = MGP::MGPipeRoleSplitRehearsalActive(); MGP::PipeInputs& client = MGP::MGPipeClientInputs(); out->blocksDistinct = &client != &MGP::gPipeInputs; out->clientSerial = client.FilledState().CurrentVerbSerial; diff --git a/MobileGL/MG_IntegrationTest/Harness/DualBlockPeek.h b/MobileGL/MG_IntegrationTest/Harness/DualBlockPeek.h index 20c475e9..708a92c4 100644 --- a/MobileGL/MG_IntegrationTest/Harness/DualBlockPeek.h +++ b/MobileGL/MG_IntegrationTest/Harness/DualBlockPeek.h @@ -29,7 +29,7 @@ namespace MGITest { // False wherever the peek cannot look (see the header above). Everything else is // meaningful only when this is true. bool peekAvailable = false; - // MGPipeRoleSplitActive(): the knob AND a real transport. The control's red half turns + // MGPipeRoleSplitRehearsalActive(): the knob AND a real transport. The control's red half turns // on this answer alone. bool roleSplitActive = false; // &MGPipeClientInputs() != &gPipeInputs - the fill side and the read side are two diff --git a/MobileGL/MG_IntegrationTest/Scenarios/DualBlockScenario.cpp b/MobileGL/MG_IntegrationTest/Scenarios/DualBlockScenario.cpp index 3206232c..476564f3 100644 --- a/MobileGL/MG_IntegrationTest/Scenarios/DualBlockScenario.cpp +++ b/MobileGL/MG_IntegrationTest/Scenarios/DualBlockScenario.cpp @@ -11,7 +11,7 @@ // proves nothing if no case can tell one block from two, so this scenario asserts exactly that, // from inside the process: // -// 1. MGPipeRoleSplitActive() - the knob is armed AND the transport is real; +// 1. MGPipeRoleSplitRehearsalActive() - the knob is armed AND the transport is real; // 2. &MGPipeClientInputs() != &gPipeInputs - two distinct objects; // 3. a client verb boundary moves the CLIENT block's serial and leaves the SERVER block's // serial alone - the fill side's writes no longer reach the read side. @@ -61,7 +61,7 @@ TEST_F(DualBlockScenario, TheTwoRolesHaveDistinctBlocks) { << "the dual-block peek is compiled out: this entry is only ever registered in a " "disaggregated build, so a false here means the registration and the build disagree"; ASSERT_TRUE(before.roleSplitActive) - << "dual-block control: MGPipeRoleSplitActive() is false - MOBILEGL_IPC_ROLE_SPLIT_STATE " + << "dual-block control: MGPipeRoleSplitRehearsalActive() is false - MOBILEGL_IPC_ROLE_SPLIT_STATE " "is not armed in this process (or the transport is monolith), so the two roles share " "one PipeInputs block and this case is the knob's negative control"; EXPECT_TRUE(before.blocksDistinct) diff --git a/MobileGL/MG_Pipe/PipeApply.cpp b/MobileGL/MG_Pipe/PipeApply.cpp index d5585608..88d8bde7 100644 --- a/MobileGL/MG_Pipe/PipeApply.cpp +++ b/MobileGL/MG_Pipe/PipeApply.cpp @@ -1350,9 +1350,18 @@ namespace MobileGL::MG_Pipe { // documented bring-up exception), and a ServerLoop fixture with no client at all has // none either - there this call is the only reset that exists. Monolith keeps the // direct call, byte for byte (G1); in a pull build none of this is compiled at all. + // D10: THE MIDDLE CONJUNCT WAS ClientSession::Active() != nullptr ALONE, WHICH IS + // PERMANENTLY NULL IN A SERVER PROCESS - so this guard could not arm at all under + // spawn, in the shape where calling the reset off the apply thread is most likely and + // least recoverable. MGPipeSessionLive() is the same question asked in a way both roles + // can answer, and MGPipeServerArm() is the process-scoped half of "am I the server". + // + // BOTH ARE ADDED AS DISJUNCTS, not substituted. The originals are the only facts a + // fixture without a full session has, and swapping them out silently disarms every such + // test - measured. if (MG_Config::Transport != MG_Config::TransportMode::Monolith && - MG_Remote::Client::ClientSession::Active() != nullptr && - !MG_Remote::Server::ServerLoop::OnApplyThread()) { + (MG_Remote::Client::ClientSession::Active() != nullptr || MG_Pipe::MGPipeSessionLive()) && + !MG_Remote::Server::ServerLoop::OnApplyThread() && !MG_Pipe::MGPipeServerArm()) { MGLOG_F("MGPipe: Fatal{RoleViolation, \"g_applier\"} - MGPipeApplierReset() called " "off the apply thread with an active transport; under split the reset " "crosses as the applier_reset record (CONTRACT-P5C.md ยง5.1)"); diff --git a/MobileGL/MG_Remote/Client/ClientSession.cpp b/MobileGL/MG_Remote/Client/ClientSession.cpp index b166fc15..5657a248 100644 --- a/MobileGL/MG_Remote/Client/ClientSession.cpp +++ b/MobileGL/MG_Remote/Client/ClientSession.cpp @@ -1164,6 +1164,21 @@ namespace MobileGL::MG_Remote::Client { // them down in the mirror order (m_started = false, then g_active = nullptr). m_started = true; g_active = this; + // D1c: THE SESSION-LIVE AND APPLY-THREAD FACTS, stated where they become true. + // + // MGPipeApplierReset's layer-2 guard used to ask ClientSession::Active() != nullptr, + // which no SERVER process can ever answer yes to - so the guard was compiled in and + // permanently disarmed there. MGPipeSessionLive() is the same question asked in a way + // both roles can answer, and the probe hands MG_Backend the one thread fact it cannot + // derive: whether the CALLING thread is inside the applier. + MG_Pipe::MGPipeSetSessionLive(true); + // ServerLoop::OnApplyThread, NOT ApplyThreadIsInsideApplier - two different questions, + // and installing the wrong one turned every GL-thread call into a role violation while + // the applier happened to be busy. Measured: the inproc retrace died on + // Fatal{RoleViolation, "MGPipeSlots"} raised from the GL thread. "Am I the apply + // thread" is what the guards ask; "is the apply thread inside the applier" is R-1's + // mutual-exclusion probe and is about a DIFFERENT thread. + MG_Pipe::MGPipeSetApplyThreadProbe(&Server::ServerLoop::OnApplyThread); LogMemory("handshake"); // ---- 8. and only now the apply thread. It is package v1's ServerLoop: it names the @@ -1298,6 +1313,10 @@ namespace MobileGL::MG_Remote::Client { m_presentsSent = 0; if (g_active == this) { g_active = nullptr; + // Mirror order, and the probe goes with it: a dangling function pointer into a + // torn-down session is a worse answer than "there is no applier". + MG_Pipe::MGPipeSetSessionLive(false); + MG_Pipe::MGPipeSetApplyThreadProbe(nullptr); } // AND ONLY NOW the monolith adapters go back (codex 4): every ring an emitter would have // used is freed above, so from here a routed call - an at-exit ~BufferObject delete - runs @@ -1803,7 +1822,7 @@ namespace MobileGL::MG_Remote::Client { // its own diagnostic; the client writes a different block and waits through appliedSeq. Bool ClientSession::InBarrierWait() { return g_inBarrierWait; } Bool ClientSession::ApplyThreadIsInsideApplier() { - if (MG_Pipe::MGPipeRoleSplitActive()) return g_roleInsideApplier; + if (MG_Pipe::MGPipeRoleSplitRehearsalActive()) return g_roleInsideApplier; return g_applyThreadInsideApplier.load(std::memory_order_acquire); } @@ -1900,12 +1919,12 @@ namespace MobileGL::MG_Remote::Client { } void ClientSession::NoteApplyThreadEnteredApplier() { g_roleInsideApplier = true; - if (!MG_Pipe::MGPipeRoleSplitActive()) + if (!MG_Pipe::MGPipeRoleSplitRehearsalActive()) g_applyThreadInsideApplier.store(true, std::memory_order_release); } void ClientSession::NoteApplyThreadLeftApplier() { g_roleInsideApplier = false; - if (!MG_Pipe::MGPipeRoleSplitActive()) + if (!MG_Pipe::MGPipeRoleSplitRehearsalActive()) g_applyThreadInsideApplier.store(false, std::memory_order_release); } diff --git a/MobileGL/MG_Remote/Server/ServerMain.cpp b/MobileGL/MG_Remote/Server/ServerMain.cpp index efa3c31b..daff1f18 100644 --- a/MobileGL/MG_Remote/Server/ServerMain.cpp +++ b/MobileGL/MG_Remote/Server/ServerMain.cpp @@ -44,6 +44,7 @@ #include "SurfaceControlFrame.h" #include // MG_Config::Transport - D1 pins it to Spawn in this process +#include // D1c: MGPipeSetServerProcessRole #include #include // MG_ConfigLoader::Init - the child loads its own config (step 1.5) @@ -179,6 +180,14 @@ extern "C" __attribute__((visibility("default"))) int mobilegl_server_main(int a // is the anti-recursion axis, so Transport is free to say what is true. MobileGL::MG_ConfigLoader::Init(); MobileGL::MG_Config::Transport = MobileGL::MG_Config::TransportMode::Spawn; + // D1c/D10: THIS PROCESS IS THE SERVER, stated once and before any GL work. + // + // Four guards used to ask "am I on the apply thread" or "is a ClientSession active" - both + // questions a server PROCESS answers wrongly: it has no ClientSession at all, and every one + // of its threads is a server thread, not just the applier's. Saying it here is what arms + // them; without it they are compiled in and permanently false, which is worse than absent + // because it reads as coverage. + MobileGL::MG_Pipe::MGPipeSetServerProcessRole(true); WireLogError("MG_Remote server: pid=%d config loaded, backend=%d, Transport pinned to Spawn " "(D1: this process IS the server arm)", selfPid, static_cast(MobileGL::MG_Config::ActiveBackendType)); diff --git a/MobileGL/MG_Test/Pipe/FieldOwnershipTest.cpp b/MobileGL/MG_Test/Pipe/FieldOwnershipTest.cpp index 3ce12a6f..28d6e8a3 100644 --- a/MobileGL/MG_Test/Pipe/FieldOwnershipTest.cpp +++ b/MobileGL/MG_Test/Pipe/FieldOwnershipTest.cpp @@ -808,7 +808,7 @@ TEST_F(FieldOwnershipTest, SplitCaptureSnapshotSurvivesMakeCurrentUntilObjectRel } TEST_F(FieldOwnershipTest, RoleSplitOffFoldsTheFillSideOntoTheSharedBlock) { - EXPECT_FALSE(MGPipeRoleSplitActive()); + EXPECT_FALSE(MGPipeRoleSplitRehearsalActive()); EXPECT_EQ(&MGPipeClientInputs(), &gPipeInputs); } @@ -819,14 +819,44 @@ TEST_F(FieldOwnershipTest, RoleSplitUnderMonolithTransportIsStillOneBlock) { RoleSplitArm guard; MG_Config::Ipc.RoleSplitState = true; MG_Config::Transport = MG_Config::TransportMode::Monolith; - EXPECT_FALSE(MGPipeRoleSplitActive()); + EXPECT_FALSE(MGPipeRoleSplitRehearsalActive()); + EXPECT_EQ(&MGPipeClientInputs(), &gPipeInputs); +} + +// D1c/D10 (CONTRACT-P6 3.2, 3.4). THE PREDICATE THAT IS TRUE WITHOUT THE KNOB, and the reason +// four guards were compiled in and permanently disarmed in a spawn server. +// +// MGPipeRoleSplitRehearsalActive() answers "is the REHEARSAL armed", which needs +// MOBILEGL_IPC_ROLE_SPLIT_STATE. Under spawn the two roles are in DIFFERENT ADDRESS SPACES, so +// their PipeInputs blocks are distinct whatever that knob says - and every guard that asked the +// rehearsal question was therefore off in the one shape where it matters most. Measured on the +// real lane: the spawn retrace runs with role-split-state=0. +TEST_F(FieldOwnershipTest, SpawnMakesTheBlocksDistinctWithNoRehearsalKnobAtAll) { + RoleSplitArm guard; + MG_Config::Ipc.RoleSplitState = false; + MG_Config::Transport = MG_Config::TransportMode::Spawn; + + EXPECT_FALSE(MGPipeRoleSplitRehearsalActive()) + << "the rehearsal must stay OFF; this case is about the shape, not the knob"; + EXPECT_TRUE(MGPipeBlocksAreDistinct()) + << "a spawn server shares no storage object with its client, so any guard that asked " + "only the rehearsal question is disarmed exactly where it is needed"; +} + +// The other direction, so the predicate cannot be a constant: monolith shares one block, and +// neither the knob nor the transport alone makes it two. +TEST_F(FieldOwnershipTest, MonolithKeepsOneBlockAndTheWiderPredicateSaysSo) { + RoleSplitArm guard; + MG_Config::Ipc.RoleSplitState = false; + MG_Config::Transport = MG_Config::TransportMode::Monolith; + EXPECT_FALSE(MGPipeBlocksAreDistinct()); EXPECT_EQ(&MGPipeClientInputs(), &gPipeInputs); } TEST_F(FieldOwnershipTest, RoleSplitGivesTheFillSideADistinctBlockTheStampNeverTouches) { RoleSplitArm guard; guard.Arm(true); - ASSERT_TRUE(MGPipeRoleSplitActive()); + ASSERT_TRUE(MGPipeRoleSplitRehearsalActive()); ASSERT_NE(&MGPipeClientInputs(), &gPipeInputs); const Uint64 serverSerialBefore = gPipeInputs.FilledState().CurrentVerbSerial; @@ -1052,7 +1082,7 @@ TEST_F(FieldOwnershipTest, DualBlockDoesNotArmUnderMonolithTransport) { // A real monolith fill, not a manufactured server stamp on a monolith process. MGPipeValidateForVerb(MGPipeVerb::DrawArrays); (void)gPipeInputs.GetBoundVertexArray(); - if (MGPipeRoleSplitActive() || MGPipeResidualPullCount() != 0) ::_exit(7); + if (MGPipeRoleSplitRehearsalActive() || MGPipeResidualPullCount() != 0) ::_exit(7); }); ASSERT_TRUE(ExitedWith(r, 0)) << DescribeStatus(r) << "\n" << r.Log; EXPECT_EQ(r.Log.find("Fatal{"), std::string::npos) << r.Log;