[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.
This commit is contained in:
2026-09-21 19:38:19 -04:00
parent 90f9a79058
commit e3bb6d8b82
11 changed files with 230 additions and 22 deletions
@@ -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) {