From 5682f429d0c877ae5d50ad723b76253b182c98f6 Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Wed, 16 Sep 2026 09:11:47 -0400 Subject: [PATCH] [Fix, Test] (MG_Remote/Client, MG_Impl/Pipe): c1 round 3 - route the five by-address delete sites (B1); observe the client arm (B3); readback requires Status=OK and the exact extent and refuses a pack-PBO read by name (M2/M8/codex11/ID-57); one ERROR->Fatal{ReplyError} rule across the two escapes and an honest ShutDown decline (M4/codex5); InitialBytesNotCarried is role-aware (M5); a routed call during teardown refuses by name not runs the applier (codex4); caps adopted on every make-current (codex12); set_dynamic_state carries an optional blob; PACK_SKIP_IMAGES ignored for a 2-D read (codex6); one tight-size function for production and test (M3/codex10); redcheck 9/9 red each on its own string (M6) --- MobileGL/MG_Impl/Pipe/PipeFill.cpp | 46 ++++-- .../MG_Remote/Client/BackendObject_Remote.cpp | 34 +++- MobileGL/MG_Remote/Client/ClientSession.cpp | 40 +++-- MobileGL/MG_Remote/Client/ClientSession.h | 8 +- MobileGL/MG_Remote/Client/EmitTables.cpp | 108 +++++++++++-- MobileGL/MG_Remote/Client/EmitTables.h | 7 + MobileGL/MG_Remote/Client/WireTables.cpp | 145 ++++++++++++++--- MobileGL/MG_Remote/Client/WireTables.h | 38 ++++- MobileGL/MG_Test/Wire/RemoteClientTest.cpp | 148 ++++++++++++++++++ 9 files changed, 513 insertions(+), 61 deletions(-) diff --git a/MobileGL/MG_Impl/Pipe/PipeFill.cpp b/MobileGL/MG_Impl/Pipe/PipeFill.cpp index d4fe5ec4..7a58544c 100644 --- a/MobileGL/MG_Impl/Pipe/PipeFill.cpp +++ b/MobileGL/MG_Impl/Pipe/PipeFill.cpp @@ -743,7 +743,18 @@ namespace MobileGL::MG_Pipe { // SPLIT-ONLY, and that is load-bearing for G2: under monolith the applier reads // `initialBytes` directly and a second upload would be a real behaviour change in the // arm the split arm is measured against. - if (MG_Config::Transport != MG_Config::TransportMode::Monolith && initialBytes != nullptr) { + // + // ROLE-AWARE (M5). Under inproc the apply thread reaches this very emitter when the + // server's own backend respecifies a buffer (c1-v2 §4 R-17.3: that is where + // Fatal{BarrierTimeout, "ResourceRespecify"} from mgl-srv-apply came from). On that + // thread this branch would emit CLIENT wire records - which the server role does not + // produce, so ClientWireRecordsEmitted() would not move and the self-check below would + // abort the SERVER by name; it would also run the respecify(nullptr)+follow-up shape, + // giving the server role a path monolith does not have. So the server role takes the + // ELSE below, exactly as monolith does. RunsAsTheServerRole() is v1's + // ServerLoop::OnApplyThread(), false on the GL thread that owns this fill. + if (MG_Config::Transport != MG_Config::TransportMode::Monolith && + !MG_Remote::Client::RunsAsTheServerRole() && initialBytes != nullptr) { MGPipeRouteResourceRespecify(desc, nullptr); // COUNTED, NOT ASSUMED, and this is the only thing that can gate the follow-up at // all. Dropping the walk below leaves a respecify that went out with nullptr and @@ -859,8 +870,11 @@ namespace MobileGL::MG_Pipe { // // EXACTLY THAT RANGE, not the whole buffer: the flush's own [offset, size) is what // the ladder rewrites, and staging more would be the coverage WIDENING ID-37 forbids. - // Split-only, for the respecify's G2 reason. - if (MG_Config::Transport != MG_Config::TransportMode::Monolith && size != 0) { + // Split-only, for the respecify's G2 reason - and ROLE-AWARE for M5's reason, the twin of + // the respecify branch above: the apply thread flushing the server's own buffer emits no + // client wire records, so it takes the plain route below rather than this split follow-up. + if (MG_Config::Transport != MG_Config::TransportMode::Monolith && + !MG_Remote::Client::RunsAsTheServerRole() && size != 0) { MGPipeEmitResourceSubData(buffer, offset, size); } #endif @@ -1510,9 +1524,21 @@ namespace MobileGL::MG_Pipe { // Step 1, shared: the wire delete goes out FIRST and only for a PUBLISHED handle, and // the latch is cleared with it so a second death path - a composite's two, a backend's // redundant notice - cannot emit a second delete for a record that is already gone. - Bool EmitDeleteIfPublished(MGPipeKind kind, MGPipeHandle handle, void (*apply)(const MGPHandleOnly&)) { + // + // `route` IS A MGPipeRoute AND NEVER A MGPipeApply (B1). R-17 converted the + // 40 direct applier CALLS to route calls by renaming `MGPipeApply(` -> but these + // five sites take the entry point BY ADDRESS, `&MGPipeApply`, so the call-expression + // rename missed them and four routed rows (DeleteSamplerView, DeleteShaderState, + // DeleteSamplerState, ResourceDestroy for textures/renderbuffers) still ran the applier + // synchronously on the GL thread under split - two writers on g_applier with the barrier + // not consulted, and under spawn a silent no-op that leaks every one of those objects. + // The parameter type is the route's, which is byte-identical to the applier's + // (const MGPHandleOnly&, void return), so the fix is `&MGPipeRoute` at the five call + // sites; the grep gate scripts/../p5-c1 redcheck refuses any `&MGPipeApply` under MG_Impl/. + Bool EmitDeleteIfPublished(MGPipeKind kind, MGPipeHandle handle, + void (*route)(const MGPHandleOnly&)) { if (!MGPipeHandleIsPublished(kind, handle)) return false; - apply(HandleOnly(kind, handle)); + route(HandleOnly(kind, handle)); MGPipeNoteHandleUnpublished(kind, handle); return true; } @@ -1533,7 +1559,7 @@ namespace MobileGL::MG_Pipe { const MGPipeHandle handle = MGPipeSlots().FindByLifetimeId(MGPipeKind::SamplerViewCso, lifetimeId); const Bool published = - EmitDeleteIfPublished(MGPipeKind::SamplerViewCso, handle, &MGPipeApplyDeleteSamplerView); + EmitDeleteIfPublished(MGPipeKind::SamplerViewCso, handle, &MGPipeRouteDeleteSamplerView); ForwardWhenWired( MGPipeSamplerEmitterInstance(), [&](auto& emitter) { emitter.NoteRecordDestroyed(handle); }); // THE NOTICE IS RAISED FOR THIS KIND TOO, and the reason it once was not is wrong: @@ -1554,7 +1580,7 @@ namespace MobileGL::MG_Pipe { Bool MGPipeEmitTextureDestroyAndFree(Uint64 lifetimeId) { const MGPipeHandle handle = MGPipeSlots().FindByLifetimeId(MGPipeKind::Texture, lifetimeId); const Bool published = - EmitDeleteIfPublished(MGPipeKind::Texture, handle, &MGPipeApplyResourceDestroy); + EmitDeleteIfPublished(MGPipeKind::Texture, handle, &MGPipeRouteResourceDestroy); // The emitter retires its entry while the handle still resolves (C-2): the drain list // drops the dead texture's levels, the raw pointer goes, the built-in sampler's cache // reference is given back, the latches and the sticky mask are cleared. @@ -1595,7 +1621,7 @@ namespace MobileGL::MG_Pipe { const MGPipeHandle handle = MGPipeSlots().FindByLifetimeId(MGPipeKind::Renderbuffer, lifetimeId); const Bool published = - EmitDeleteIfPublished(MGPipeKind::Renderbuffer, handle, &MGPipeApplyResourceDestroy); + EmitDeleteIfPublished(MGPipeKind::Renderbuffer, handle, &MGPipeRouteResourceDestroy); ForwardWhenWired( MGPipeTextureEmitterInstance(), [&](auto& emitter) { emitter.NoteRenderbufferDied(handle); }); NotifyAndFree(MGPipeKind::Renderbuffer, lifetimeId, handle); @@ -1632,7 +1658,7 @@ namespace MobileGL::MG_Pipe { const MGPipeHandle handle = MGPipeSlots().FindByLifetimeId(MGPipeKind::SamplerCso, lifetimeId); const Bool published = - EmitDeleteIfPublished(MGPipeKind::SamplerCso, handle, &MGPipeApplyDeleteSamplerState); + EmitDeleteIfPublished(MGPipeKind::SamplerCso, handle, &MGPipeRouteDeleteSamplerState); // NOTHING TO RETIRE IN AN EMITTER FOR THIS KIND, stated rather than implied: a sampler // CSO is content-addressed and belongs to a value, so no emitter keeps an entry under // a SamplerObject's handle - the cache's entries are keyed by value and reference @@ -1653,7 +1679,7 @@ namespace MobileGL::MG_Pipe { const MGPipeHandle handle = MGPipeSlots().FindByLifetimeId(MGPipeKind::ShaderCso, lifetimeId); const Bool published = - EmitDeleteIfPublished(MGPipeKind::ShaderCso, handle, &MGPipeApplyDeleteShaderState); + EmitDeleteIfPublished(MGPipeKind::ShaderCso, handle, &MGPipeRouteDeleteShaderState); ForwardWhenWired( MGPipeProgramEmitterInstance(), [&](auto& emitter) { emitter.NoteRecordDestroyed(handle); }); NotifyAndFree(MGPipeKind::ShaderCso, lifetimeId, handle); diff --git a/MobileGL/MG_Remote/Client/BackendObject_Remote.cpp b/MobileGL/MG_Remote/Client/BackendObject_Remote.cpp index f8b616c6..44d09bb7 100644 --- a/MobileGL/MG_Remote/Client/BackendObject_Remote.cpp +++ b/MobileGL/MG_Remote/Client/BackendObject_Remote.cpp @@ -210,7 +210,25 @@ namespace MobileGL::MG_Remote::Client { // InitCapabilities' answer comes from a snapshot the server can only publish once its // own InitCapabilities has run - and ServerMakeEGLCurrent is what publishes it. if (!Server::ServerMakeEGLCurrent(dpy, draw, read, ctx)) return false; - return MG_Backend::BackendObject::MakeEGLCurrent(dpy, draw, read, ctx); + if (!MG_Backend::BackendObject::MakeEGLCurrent(dpy, draw, read, ctx)) return false; + + // R-12 ARM (a) ON EVERY SUCCESSFUL MAKE-CURRENT (codex 12). ServerMakeEGLCurrent above + // republishes the caps snapshot on every call (ServerLoop.cpp:613-628), but the base + // class only runs InitCapabilities - the one place that pumps and refreshes - on the + // FIRST make-current per surface (BackendObject.cpp:341-347). A repeated make-current + // onto an already-initialised surface therefore left the client mirror one generation + // behind while unpumped snapshots accumulated, so a cap getter or a shader compile before + // the next Present read the prior mirror. Adopting here closes that: "a second snapshot + // arrival IS the invalidation" (R-12) now holds AT the make-current that caused it. It is + // idempotent - on the first make-current InitCapabilities already pumped, so this adopts + // 0 - and a release-current (draw/ctx cleared) publishes nothing and is skipped. + if (draw != EGL_NO_SURFACE && ctx != EGL_NO_CONTEXT) { + if (ClientSession* session = ClientSession::Active()) { + session->PumpControlPlane(); + RefreshFormatCapabilities(); + } + } + return true; } Bool BackendObject_Remote::SwapEGLBuffers(EGLDisplay dpy, EGLSurface draw) { @@ -247,4 +265,18 @@ namespace MobileGL::MG_Remote::Client { MG_Backend::BackendObject::ReleaseEGLResources(); } + // NO strong CreateRemoteBackendObject() lives here, and the reason is a link fact, not an + // oversight. v1's Init.cpp calls MG_Remote::Client::CreateRemoteBackendObject() and ships a + // __attribute__((weak)) placeholder for it beside ServerLoop that aborts by name; its comment + // expects "c1's strong definition [to] displace it at link time". A strong definition here + // does NOT: libMobileGL is linked from a static archive, ServerLoop.o (weak) is already in + // the link and satisfies Init's reference, and nothing else references this TU's + // CreateRemoteBackendObject - so BackendObject_Remote.o is never pulled to override it, and + // the weak's abort fires (measured: readelf shows one local symbol, the log shows + // Fatal{UnimplementedRemoteBackendObject}). The integrator's Init.cpp hunk works because it + // constructs BackendObject_Remote DIRECTLY (MakeUnique), which both + // references this object - forcing its TU into the link - and bypasses the weak symbol. So + // the merge-time construction stays v1's Init.cpp edit (or a --whole-archive / forced + // reference the integrator adds); see c1-v3.md. + } // namespace MobileGL::MG_Remote::Client diff --git a/MobileGL/MG_Remote/Client/ClientSession.cpp b/MobileGL/MG_Remote/Client/ClientSession.cpp index 843ff968..24b918aa 100644 --- a/MobileGL/MG_Remote/Client/ClientSession.cpp +++ b/MobileGL/MG_Remote/Client/ClientSession.cpp @@ -576,12 +576,17 @@ namespace MobileGL::MG_Remote::Client { } void ClientSession::Stop() { - // FIRST, BEFORE ANYTHING ELSE GOES AWAY (R-17). Every later step here frees something - // an emitter dereferences - the rings, the segments, the transports - so a GL call - // that arrives during teardown must already be looking at the monolith arm. Putting - // the monolith adapters back rather than nulling the rows is deliberate: a null row is - // the pre-migration state and would be an undiagnosed crash, and the adapter is a - // correct answer for a process that no longer has a session. + // FIRST, BEFORE ANYTHING ELSE GOES AWAY (R-17 / codex 4). Every later step here frees + // something an emitter dereferences - the rings, the segments, the transports - so a + // routed GL-thread call that arrives during teardown must not run the applier on the + // caller and must not reach a half-freed ring. Round 2 reinstalled the monolith adapters + // HERE, which is running the applier on the caller - the forbidden path table 3 draws. + // Uninstall now RAISES A FLAG and leaves the wire rows in place; the next routed call + // aborts by name (Fatal{ClientTablesUninstalled}) inside RequireSession before it touches + // anything. The monolith adapters go back only at the END of teardown + // (ReinstallMonolithAfterTeardown), for the at-exit ~BufferObject deletes that reach a + // process with no session at all - and by then the rings are gone, so the applier a + // monolith adapter runs is a defined no-op rather than a use-after-free. UninstallClientWireTables(); if (!m_started) { // Start's own failure paths land here with a half-built session. FIVE of them are @@ -599,6 +604,9 @@ namespace MobileGL::MG_Remote::Client { m_clientTransport.reset(); m_serverTransport.reset(); m_transport = nullptr; + // The rings are gone; put the monolith adapters back for a process that will make no + // more routed calls except, possibly, at-exit deletes (codex 4). + ReinstallMonolithAfterTeardown(); return; } @@ -655,6 +663,12 @@ namespace MobileGL::MG_Remote::Client { if (g_active == this) { g_active = 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 + // the applier exactly as it does under monolith, which is the correct answer for a + // process that no longer has a session. During the whole span above, the raised flag made + // any routed call abort by name instead. + ReinstallMonolithAfterTeardown(); } Wire::PipeWireEncoder& ClientSession::Encoder() { return m_encoder; } @@ -673,8 +687,9 @@ namespace MobileGL::MG_Remote::Client { // "always accept" is ID-39's 66 lost DirectVulkan uploads with a wire in between. Uint64 ClientSession::EmitAndWait(MG_Pipe::MGPWireOp op, const void* payload, Uint64 payloadBytes, const void* varTail, Uint64 varTailBytes, void* replyOut, - Uint64 replyBytes, Int32* statusOut) { + Uint64 replyBytes, Int32* statusOut, Uint64* replySizeOut) { if (statusOut != nullptr) *statusOut = Wire::ReplySink::kStatusError; + if (replySizeOut != nullptr) *replySizeOut = 0; if (!m_started) { MGLOG_F("MGPipe: Fatal{NoClientSession, \"%s\"} - EmitAndWait on a session that has " "not started. There is no fall-through: a record that could not be emitted " @@ -735,9 +750,15 @@ namespace MobileGL::MG_Remote::Client { if (wait == Transport::SessionWait::ShutDown) { // The doorbell died: the server went away. The only thing that returns from a // kWaitForever park, and therefore the only way a client blocked in the barrier - // survives a server that is gone. Not a Fatal - teardown legitimately reaches here. + // survives a server that is gone. It is teardown, not a server fault - so the answer + // handed back is DECLINED, not the ERROR the status was pre-set to (M4): a + // reply-owning row that saw ERROR here would abort Fatal{ReplyError} on a shutting- + // down session, which is what the "teardown legitimately reaches here" comment + // promised would NOT happen. DECLINED is honest - "the verb did not happen" - and the + // acceptance rows already treat it as `false` / nullptr without aborting. + if (statusOut != nullptr) *statusOut = Wire::ReplySink::kStatusDeclined; MGLOG_E("MG_Remote client: the barrier for %s (seq %llu) woke on a dead doorbell; the " - "server is gone and this verb did not happen", + "server is gone and this verb did not happen (reported as DECLINED, not ERROR)", Wire::WireOpName(op), static_cast(seq)); return seq; } @@ -770,6 +791,7 @@ namespace MobileGL::MG_Remote::Client { std::abort(); } if (statusOut != nullptr) *statusOut = status; + if (replySizeOut != nullptr) *replySizeOut = replySize; if (status == Wire::ReplySink::kStatusError) { MGLOG_E("MG_Remote client: %s (seq %llu) answered ERROR", Wire::WireOpName(op), static_cast(seq)); diff --git a/MobileGL/MG_Remote/Client/ClientSession.h b/MobileGL/MG_Remote/Client/ClientSession.h index 5671d5b7..2f971f9c 100644 --- a/MobileGL/MG_Remote/Client/ClientSession.h +++ b/MobileGL/MG_Remote/Client/ClientSession.h @@ -96,11 +96,17 @@ namespace MobileGL::MG_Remote::Client { // name where a kReplySlot answer lands; pass {nullptr, 0} for a call that has none. // Returns the record's seq, which is also its reply-slot id. // + // `replySizeOut` (optional) receives the answer's OWN byte count as the server stamped + // it - which is not always `replyBytes`: a short OK reply stamps fewer, and a DECLINE or + // ERROR stamps 0. ReadPixels is the one caller that must know, because scattering a + // reply that arrived short would spray stale bytes as pixels (M2 / codex 11); it reads + // this and refuses `replySize != DstSize` by name rather than trust the copy. + // // Waiting is spin(MOBILEGL_IPC_SPIN_US) then park, through Doorbell::Wait, with // producerParked set before blocking - the shape Doorbell.h:121 already implements. Uint64 EmitAndWait(MG_Pipe::MGPWireOp op, const void* payload, Uint64 payloadBytes, const void* varTail, Uint64 varTailBytes, void* replyOut, - Uint64 replyBytes, Int32* statusOut); + Uint64 replyBytes, Int32* statusOut, Uint64* replySizeOut = nullptr); // MOBILEGL_IPC_VERB_BARRIER. False is the R-1 negative control and is EXPECTED to be // red; it must be run once and the way it goes red recorded. diff --git a/MobileGL/MG_Remote/Client/EmitTables.cpp b/MobileGL/MG_Remote/Client/EmitTables.cpp index 53c3d4bf..39fba630 100644 --- a/MobileGL/MG_Remote/Client/EmitTables.cpp +++ b/MobileGL/MG_Remote/Client/EmitTables.cpp @@ -32,6 +32,7 @@ #include #include +#include #include #include @@ -96,7 +97,11 @@ namespace MobileGL::MG_Remote::Client { // last clause is what the control checks with a sentinel. Bool ReadbackPackStateIsTight(GLsizei width, Uint64 bytesPerPixel, const PixelStoreParameters& pack) { - if (pack.SkipRows != 0 || pack.SkipPixels != 0 || pack.SkipImages != 0) return false; + // SKIP_IMAGES (and IMAGE_HEIGHT) are NOT consulted: glReadPixels is a 2-D read and GL + // ignores the image-level pack parameters for it, exactly as the monolith conversion + // path does at DirectGLES.cpp:10905 (honorPackImageParams=false). A non-zero + // SkipImages therefore does not make the layout non-tight (codex 6). + if (pack.SkipRows != 0 || pack.SkipPixels != 0) return false; if (pack.RowLength != 0 && pack.RowLength != width) return false; const Uint64 alignment = pack.Alignment > 0 ? static_cast(pack.Alignment) : 1ull; const Uint64 rowBytes = static_cast(width) * bytesPerPixel; @@ -265,9 +270,62 @@ namespace MobileGL::MG_Remote::Client { ReadbackBytesPerPixel(format, type); } + // M2 / codex 11: the reply the server posted is COMPLETE and OK. A short OK reply, and a + // DECLINED or ERROR reply with a zero payload, both leave the destination full of stale + // bytes; scattering or returning it is the silently truncated picture ID-47's own comment + // says an SSIM comparison cannot see, arriving through the status field rather than + // through truncation. `expected` is the record's own DstSize (CONTRACT-P5 row 23: the + // reply's exact extent). The predicate is at namespace scope (below) and exposed for the + // control, so R-16's "drive the production predicate" holds rather than a second copy of + // the rule in the test. + + // The same predicate at the call site, with the named Fatal each failure mode owns. + // status > expected cannot reach here: EmitAndWait already aborts Fatal{ReplyTooLarge} on + // an oversize reply, so the only failures left are a wrong status or a SHORT one. + void RequireReadbackReplyComplete(Int32 status, Uint64 replySize, Uint64 expected) { + if (status == Wire::ReplySink::kStatusError) { + MGLOG_F("MGPipe: Fatal{ReplyError, \"ReadPixels\"} - the readback answered ERROR; " + "the destination is left untouched rather than filled with stale bytes"); + std::abort(); + } + if (status == Wire::ReplySink::kStatusDeclined) { + MGLOG_F("MGPipe: Fatal{ReadbackDeclined, \"ReadPixels\"} - the server has no " + "GL.ReadPixels and DECLINED; a decline is a real answer for an acceptance " + "row (R-5) but a blocking readback has no pixels to return, so it is a " + "Fatal here rather than a buffer of stale bytes"); + std::abort(); + } + if (replySize != expected) { + MGLOG_F("MGPipe: Fatal{ReadbackReplyShort, \"ReadPixels %llu < %llu\"} - the OK " + "reply carried fewer bytes than the read's own DstSize (CONTRACT-P5 row " + "23's exact extent); the missing rows would otherwise be scattered as " + "whatever the destination held", + static_cast(replySize), + static_cast(expected)); + std::abort(); + } + } + void EmitReadPixels(GLint x, GLint y, GLsizei width, GLsizei height, GLenum format, GLenum type, void* pixels) { ClientSession& session = RequireSession("ReadPixels"); + + // ID-57 / M8: A PACK-PBO DESTINATION IS REFUSED BY NAME, BEFORE ANY EMISSION AND + // BEFORE `pixels` IS TOUCHED. With GL_PIXEL_PACK_BUFFER bound, the frontend permits + // `pixels` to be a byte OFFSET into that buffer, not an address (GL_Framebuffer.cpp: + // 3055 aligns it to the type size, one byte for UNSIGNED_BYTE) - and this emitter has + // no PBO branch: it sets DstOffset = 0, hands the offset to EmitAndWait as a host + // buffer, and the reply is memcpy'd to CPU address . Under monolith the + // backend maps the PBO and writes the reply into the buffer (unchanged). The real + // split form - the server writes the reply into the buffer resource and the client + // marks it GPU-written (b1's MarkReadPixelsPackBuffer becoming the producer contract + // §3 names) - is a P6 ROADMAP item. In P5 it is class C's shape (R-4), refused here. + if (MG_State::pGLContext != nullptr && + MG_State::pGLContext->GetBufferBindingSlot(::MobileGL::BufferTarget::PixelPack) + .GetBoundObject()) { + UnmigratedVerbFatal("ReadPixels+PACK_BUFFER"); + } + BeforeReadOnlyVerb(); // THE PBO HALF IS b1's DESIGN AND b1 ALREADY WIRED ITS MARK, at @@ -275,10 +333,16 @@ namespace MobileGL::MG_Remote::Client { // ReadPixels_Backend itself. So this emitter deliberately does NOT call // MarkReadPixelsPackBuffer(): a second call there would be the "wire it twice" // shape, and the per-row counter b1's unit cases assert on would then count one - // read as two. + // read as two. (In P5 the refusal above means no PBO read reaches here at all; the + // note stays for the P6 form.) if (width <= 0 || height <= 0) return; const Uint64 bytesPerPixel = ReadbackBytesPerPixel(format, type); - const Uint64 tight = static_cast(width) * static_cast(height) * bytesPerPixel; + // ONE tight-size function for production AND the control (M3 / codex 10a). The first + // cut computed this inline here while the test drove TightReadbackByteCount, so a + // `+16` on the production line stayed green - the test observed a different number. + // Now the number the server allocates and the number the test asserts come from the + // same body. + const Uint64 tight = TightReadbackBytes(width, height, format, type); MG_Pipe::MGPReadbackInfo info{}; info.Res = MG_Pipe::kMGPipeNullHandle; // "the bound read surface answers" @@ -312,6 +376,7 @@ namespace MobileGL::MG_Remote::Client { } Int32 status = 0; + Uint64 replySize = 0; if (ReadbackPackStateIsTight(width, bytesPerPixel, pack)) { // THE COMMON CASE, AND IT KEEPS THE ZERO-COPY. A neutral pack state means the // destination layout IS the tight layout, so the reply lands straight in the @@ -319,7 +384,11 @@ namespace MobileGL::MG_Remote::Client { // SAME bytes, not a second rule: ScatterTightReadback below is a memcpy of the // whole run in exactly this case, and the control drives that function. session.EmitAndWait(MG_Pipe::MGPWireOp::ReadPixels, &info, sizeof(info), nullptr, 0, - pixels, tight, &status); + pixels, tight, &status, &replySize); + // M2 / codex 11: an OK reply that arrived short, or a DECLINE/ERROR, must not be + // handed back as pixels. The Fatal aborts before the application reads the buffer, + // so the bytes EmitAndWait already copied into `pixels` are never observed. + RequireReadbackReplyComplete(status, replySize, tight); return; } @@ -328,7 +397,10 @@ namespace MobileGL::MG_Remote::Client { // level out: nothing here outlives the call. Vector bounce(static_cast(tight)); session.EmitAndWait(MG_Pipe::MGPWireOp::ReadPixels, &info, sizeof(info), nullptr, 0, - bounce.data(), tight, &status); + bounce.data(), tight, &status, &replySize); + // BEFORE THE SCATTER, so a short or non-OK reply never reaches the application's + // pointer at all (the bounce is the only thing that held the partial bytes). + RequireReadbackReplyComplete(status, replySize, tight); ScatterTightReadbackIntoPackState(bounce.data(), pixels, width, height, bytesPerPixel, pack); } @@ -590,6 +662,12 @@ namespace MobileGL::MG_Remote::Client { return TightReadbackBytes(width, height, format, type); } + // M2 / codex 11's predicate at namespace scope: the one RequireReadbackReplyComplete decides + // on and the one the control drives. 0=OK / 1=DECLINED / 2=ERROR. + Bool ReadbackReplyIsComplete(Int32 status, Uint64 replySize, Uint64 expected) { + return status == Wire::ReplySink::kStatusOk && replySize == expected; + } + // ID-49's scatter. Exported for the same reason as the refusal above: the control drives // THIS, which is what the emitter calls, rather than a second copy of 8.4.4's arithmetic. void ScatterTightReadbackIntoPackState(const void* tight, void* destination, GLsizei width, @@ -601,15 +679,21 @@ namespace MobileGL::MG_Remote::Client { const Uint64 alignment = pack.Alignment > 0 ? static_cast(pack.Alignment) : 1ull; const Uint64 strideBytes = ((rowPixels * bytesPerPixel + alignment - 1) / alignment) * alignment; + // m6: the client re-derives GL 4.6 8.4.4's pack layout, so it honours GL_PACK_ROW_LENGTH + // VERBATIM - including the ill-formed 0 < ROW_LENGTH < width, where the row stride is + // narrower than a written row and consecutive rows overlap. GL leaves that case to the + // implementation; the client reproduces exactly what the monolith backend's own scatter + // would do with the same state rather than clamping, so the two arms stay byte-identical. + // The fast path (ReadbackPackStateIsTight) already rejects any ROW_LENGTH != width, so + // this only runs on the scatter path the application asked for. const Uint64 writtenPerRow = static_cast(width) * bytesPerPixel; - // SKIP_IMAGES is in the parameter set and is meaningless for a 2D read, so it is - // applied as GL defines it (whole images of ROW_LENGTH x IMAGE_HEIGHT) rather than - // ignored - ignoring a non-zero one would silently write over the application's first - // image. - const Uint64 imageRows = - pack.ImageHeight > 0 ? static_cast(pack.ImageHeight) : static_cast(height); + // SKIP_IMAGES and IMAGE_HEIGHT ARE IGNORED (codex 6). glReadPixels is a 2-D read; GL + // does not apply the image-level pack parameters to it, and the monolith conversion path + // says so explicitly with honorPackImageParams=false (DirectGLES.cpp:10905). Applying + // SKIP_IMAGES here shifted a read with SKIP_IMAGES=1 by a whole image and overran an + // application buffer sized for exactly `height` rows. Only SKIP_ROWS and SKIP_PIXELS - + // the 2-D skips - offset the first written byte. auto* out = static_cast(destination) + - static_cast(pack.SkipImages) * imageRows * strideBytes + static_cast(pack.SkipRows) * strideBytes + static_cast(pack.SkipPixels) * bytesPerPixel; const auto* in = static_cast(tight); diff --git a/MobileGL/MG_Remote/Client/EmitTables.h b/MobileGL/MG_Remote/Client/EmitTables.h index 1a7719eb..cb791e46 100644 --- a/MobileGL/MG_Remote/Client/EmitTables.h +++ b/MobileGL/MG_Remote/Client/EmitTables.h @@ -147,4 +147,11 @@ namespace MobileGL::MG_Remote::Client { Bool ReadbackPackStateIsTightForTest(GLsizei width, Uint64 bytesPerPixel, const PixelStoreParameters& pack); + // M2 / codex 11. True exactly when the readback reply is OK and carries the read's own exact + // extent (CONTRACT-P5 row 23). EmitReadPixels calls this and Fatals by name when it is false + // - a short OK reply, or a DECLINE/ERROR with a zero payload, is refused rather than scattered + // as pixels. Exposed so the control drives the production predicate (R-16), not a copy: pass + // 0=OK / 1=DECLINED / 2=ERROR as `status`. + Bool ReadbackReplyIsComplete(Int32 status, Uint64 replySize, Uint64 expected); + } // namespace MobileGL::MG_Remote::Client diff --git a/MobileGL/MG_Remote/Client/WireTables.cpp b/MobileGL/MG_Remote/Client/WireTables.cpp index 820959a5..256e2f67 100644 --- a/MobileGL/MG_Remote/Client/WireTables.cpp +++ b/MobileGL/MG_Remote/Client/WireTables.cpp @@ -41,6 +41,7 @@ #include #include +#include #include #include @@ -48,29 +49,60 @@ namespace MobileGL::MG_Remote::Client { using MG_Pipe::MGPWireOp; - namespace { + // TABLE 3's ROLE SPLIT, AS A RUNTIME CHECK. gMGPipeScreen / gMGPipeContext are PROCESS + // globals and under `inproc` the server role is a thread in this same process, so the + // apply thread running the server's own backend - the EGL bring-up, InitCapabilities, + // the applier - reaches these very emitters. A record published there would be waited + // for by the thread that is supposed to apply it: `Fatal{BarrierTimeout, + // "ResourceRespecify"}` from `mgl-srv-apply`, thirty seconds into bring-up, which is + // exactly how this was found. + // + // THE ANSWER IS NOT "SUPPRESS THE RECORD" - it is "run the server's own code", because + // on that thread this process IS the server and the applier is one call away. It is + // the same thing PipeWireCodec does on the decode side, where every arm calls + // MGPipeApply* directly and never goes through a table. + // + // Under `spawn` (P6) the predicate is constantly false in the client process and + // constantly true in the server's, so this costs one atomic load and changes nothing. + // + // NOT in the anonymous namespace, because M5 needs it from MG_Impl/Pipe/PipeFill.cpp too + // (its split-only respecify/flush branches must not run on the apply thread). Declared in + // WireTables.h. + Bool RunsAsTheServerRole() { return Server::ServerLoop::OnApplyThread(); } - // TABLE 3's ROLE SPLIT, AS A RUNTIME CHECK. gMGPipeScreen / gMGPipeContext are PROCESS - // globals and under `inproc` the server role is a thread in this same process, so the - // apply thread running the server's own backend - the EGL bring-up, InitCapabilities, - // the applier - reaches these very emitters. A record published there would be waited - // for by the thread that is supposed to apply it: `Fatal{BarrierTimeout, - // "ResourceRespecify"}` from `mgl-srv-apply`, thirty seconds into bring-up, which is - // exactly how this was found. - // - // THE ANSWER IS NOT "SUPPRESS THE RECORD" - it is "run the server's own code", because - // on that thread this process IS the server and the applier is one call away. It is - // the same thing PipeWireCodec does on the decode side, where every arm calls - // MGPipeApply* directly and never goes through a table. - // - // Under `spawn` (P6) the predicate is constantly false in the client process and - // constantly true in the server's, so this costs one atomic load and changes nothing. - Bool RunsAsTheServerRole() { return Server::ServerLoop::OnApplyThread(); } + namespace { Uint64 g_emitted = 0; Uint64 g_declined = 0; + // TEARDOWN REFUSAL (codex 4). ClientSession::Stop marks the routed tables uninstalled + // BEFORE it frees the rings, the segments and the transport - so the window between then + // and the moment the monolith adapters go back is one in which a routed GL-thread call + // must not run the applier on the caller (the forbidden path table 3 draws) and must not + // reach a half-freed ring. Round 2 reinstalled the monolith adapters at the top of Stop, + // which is exactly running the applier on the caller; a routed mutation after + // UninstallClientWireTables() moved no wire ordinal and never refused (the cross-family + // verifier reproduced it). So Uninstall now RAISES THIS FLAG and leaves the wire rows in + // place; RequireSession below reads it and refuses by name before it touches anything. + // Atomic because the apply thread's own role check races a GL-thread teardown. + std::atomic g_clientTablesUninstalled{false}; + ClientSession& RequireSession(const char* row) { + // BEFORE the session lookup and before any ring access. A routed call that arrives + // once Stop has begun tearing the session down is refused by name rather than run + // on the caller's thread - the applier is server-exclusive (table 3), and a wire + // emit into a ring being freed is a use-after-free. The monolith adapters are put + // back only as the LAST step of teardown, for the at-exit ~BufferObject deletes that + // legitimately reach a process with no session (see ReinstallMonolithAfterTeardown). + if (g_clientTablesUninstalled.load(std::memory_order_acquire)) { + MGLOG_F("MGPipe: Fatal{ClientTablesUninstalled, \"%s\"} - a routed call reached the " + "client wire tables while ClientSession::Stop was tearing the session " + "down. R-4/R-17: the applier is server-exclusive and the rings this emit " + "would use are being freed, so the call is refused by name rather than run " + "on the caller", + row); + std::abort(); + } ClientSession* session = ClientSession::Active(); if (session == nullptr) { MGLOG_F("MGPipe: Fatal{NoClientSession, \"%s\"} - the client wire tables are " @@ -173,7 +205,6 @@ namespace MobileGL::MG_Remote::Client { MGP_WIRE_BLOB(CreateRenderState, MGPRenderStateDesc, Blob) MGP_WIRE_BLOB(CreateVertexElements, MGPVertexElements, Blob) MGP_WIRE_BLOB(CreateSamplerState, MGPSamplerDesc, Parameters) - MGP_WIRE_BLOB(SetDynamicState, MGPDynamicState, Blob) MGP_WIRE_BLOB(SetGlobalConstants, MGPGlobalConstants, Blob) // -- context, variable tail --------------------------------------------------- @@ -189,6 +220,33 @@ namespace MobileGL::MG_Remote::Client { // -- the rows that fit none of the three shapes -------------------------------- + // set_dynamic_state. AN OPTIONAL BLOB, NOT A MANDATORY ONE (round-2 regression, the + // census's Fatal{BlobMissing, "SetDynamicState"}). EmitRenderState (PipeFill.cpp:2405- + // 2421) sends a set_dynamic_state whenever the RenderState VERSION moved, and when the + // chunk-level suppressor finds NO dynamic chunk changed it sends the 32-byte header with + // ChunkMask == 0 and blobByteCount == 0 - a version-only update, which the comment there + // calls out as deliberate. The generic MGP_WIRE_BLOB wrapper stages through StageRequired, + // which Fatals on a zero count, so a legal header-only update aborted the process with a + // blob-shape complaint that had nothing to do with the actual state of the pipe. The + // DECODER already handles it: PipeWireCodec.cpp:1571-1587 takes the `ChunkMask == 0` arm + // (CheckBlobIsHonest, no blob resolved) and applies the header alone. So the emitter is + // the only side that was wrong; it stages OPTIONALLY, exactly like the two sub-data rows. + // A non-empty mask still stages (blobByteCount > 0), so the reduced-path scenarios, whose + // first draw is freshly primed with every chunk, are byte-for-byte unchanged. + void Wire_SetDynamicState(const MG_Pipe::MGPDynamicState* payload, const void* blobBytes, + Uint64 blobByteCount) { + if (RunsAsTheServerRole()) { + MG_Pipe::MGPipeMonolithContext().SetDynamicState(payload, blobBytes, blobByteCount); + return; + } + ClientSession& session = RequireSession("SetDynamicState"); + MG_Pipe::MGPDynamicState record = *payload; + record.Blob = StageOptional(session, blobBytes, blobByteCount); + session.EmitAndWait(MGPWireOp::SetDynamicState, &record, sizeof(record), nullptr, 0, + nullptr, 0, nullptr); + ++g_emitted; + } + // set_residual_value_state. CONTRACT-P5 table 1 row 6: the applier takes a frontend // `ResidualValueBlock&` and `MGPResidualValueState` is never instantiated on the live // path, so the encoder invents BOTH the record fill and the blob fill. The block IS @@ -338,6 +396,17 @@ namespace MobileGL::MG_Remote::Client { 0, nullptr, 0, &status); (void)seq; ++g_emitted; + // ERROR IS NOT A DECLINE (M4 / codex 5 / R-5). status is 0 OK / 1 DECLINED / 2 ERROR; + // an escape may not fold 2 into `false`, which is what a bare `return status == 0` + // did - a transport fault then read as "the server said no". Every reply-owning row + // - the generated acceptance rows through MGPipeTakeReplyBool, and now these two + // escapes - answers ERROR with the same named Fatal. + if (status == 2) { + MGLOG_F("MGPipe: Fatal{ReplyError, \"resource_respecify\"} - the row answered " + "ERROR, which is not an acceptance answer; folding it into accepted or " + "refused would make a transport fault look like a resource decision"); + std::abort(); + } if (status == 1) ++g_declined; return status == 0; } @@ -376,6 +445,15 @@ namespace MobileGL::MG_Remote::Client { session.EmitAndWait(MGPWireOp::MapPersistent, handle, sizeof(*handle), nullptr, 0, nullptr, 0, &status); ++g_emitted; + // ERROR IS NOT A DECLINE (M4 / codex 5 / R-5), the same rule the respecify escape and + // the generated acceptance rows obey: status 2 is a transport fault, and returning + // nullptr for it would make it indistinguishable from R-6's legitimate decline. + if (status == 2) { + MGLOG_F("MGPipe: Fatal{ReplyError, \"map_persistent\"} - the row answered ERROR, " + "which is not an acceptance answer; a transport fault is not a resource " + "decision and may not be folded into the decline R-6 predicts"); + std::abort(); + } if (status == 1) ++g_declined; // NOT "always nullptr": the answer is READ. R-6 says the server declines, and the // day it stops declining this returns what it actually said rather than what the @@ -436,6 +514,12 @@ namespace MobileGL::MG_Remote::Client { void InstallClientWireTables() { using namespace MG_Pipe; + // A fresh install means the routed tables are live again: a Start after a previous + // session's Stop clears the teardown-refusal flag so its own routed calls are not + // refused. (Stop already puts the monolith adapters back and clears the flag; this is + // the belt to that braces.) + g_clientTablesUninstalled.store(false, std::memory_order_release); + gMGPipeScreen.ResourceCreate = &Wire_ResourceCreate; gMGPipeScreen.ResourceDestroy = &Wire_ResourceDestroy; gMGPipeScreen.UnmapPersistent = &Wire_UnmapPersistent; @@ -480,11 +564,28 @@ namespace MobileGL::MG_Remote::Client { } void UninstallClientWireTables() { - // PUTS THE MONOLITH ARM BACK rather than nulling the rows. A null row is the - // pre-migration state and would be an immediate crash with no diagnostic at whatever - // GL call raced the teardown; the monolith adapter is at least a correct answer for a - // process that no longer has a session. + // MARK THE ROUTED TABLES UNINSTALLED (codex 4). It does NOT reinstall the monolith + // adapters, and that is the whole fix: round 2 reinstalled them here, at the TOP of + // Stop, so a routed GL-thread call arriving during teardown ran the applier on the + // caller - the forbidden path - and moved no wire ordinal, with no refusal. Raising the + // flag leaves the Wire_* rows in place; the next routed call reaches RequireSession, + // reads the flag and aborts by name (Fatal{ClientTablesUninstalled}) before it touches a + // ring that Stop is about to free. The monolith adapters are put back only once teardown + // is complete, by ReinstallMonolithAfterTeardown, for the at-exit deletes that reach a + // process with no session at all. Idempotent: safe to call when nothing was installed. + g_clientTablesUninstalled.store(true, std::memory_order_release); + } + + void ReinstallMonolithAfterTeardown() { + // THE LAST STEP OF ClientSession::Stop, after the rings, segments and transport are gone + // and the apply thread has joined. Now a routed call can only be a process that no + // longer has a session - the canonical case is ~BufferObject running from an exit + // handler (ID-8) - and the monolith adapter, which runs the applier synchronously, is + // the correct answer for it, exactly as it is in a pure-monolith build. Clearing the + // flag re-enables the (now monolith) rows. A null row here would be an undiagnosed crash + // at whatever GL call an exit handler makes; the applier is a defined no-op-or-apply. MG_Pipe::MGPipeInstallMonolithTables(); + g_clientTablesUninstalled.store(false, std::memory_order_release); } Uint64 ClientWireRecordsEmitted() { return g_emitted; } diff --git a/MobileGL/MG_Remote/Client/WireTables.h b/MobileGL/MG_Remote/Client/WireTables.h index d87c3eca..90345dbe 100644 --- a/MobileGL/MG_Remote/Client/WireTables.h +++ b/MobileGL/MG_Remote/Client/WireTables.h @@ -43,20 +43,46 @@ namespace MobileGL::MG_Remote::Client { // gMGPipeRouteEscapes and records the arm. Idempotent. void InstallClientWireTables(); - // Puts the monolith adapters back. Idempotent; safe to call when nothing was installed. + // Marks the routed tables uninstalled so a routed call refuses by name + // (Fatal{ClientTablesUninstalled, ""}) rather than running the applier on the caller + // (codex 4). It does NOT restore the monolith adapters - that is ReinstallMonolithAfterTeardown + // below, run only once the session's rings are freed. Idempotent. void UninstallClientWireTables(); - // How many records the thirty-seven emitters have published. This is t1's FOURTH arming - // fact - "the client encoder's record ordinal actually moving during the case" - counted - // at the only place that can count it, and it is deliberately NOT the encoder's EmitSeq: - // EmitSeq moves for the five class-B verbs too, so a lane that armed on it would arm on a - // Clear and call the resource path proven. + // The LAST step of ClientSession::Stop: after the rings, segments and transport are gone, + // puts the monolith adapters back and clears the refusal flag, so the at-exit ~BufferObject + // deletes that reach a process with no session run the applier as they do under monolith. + void ReinstallMonolithAfterTeardown(); + + // How many records the thirty-seven emitters have published. It counts the ROUTED rows + // only - a resource_create, a set_vertex_buffers, a create_shader_state - and never the + // five class-B verbs, so it is the one number that says "the resource/CSO/state path really + // ran" as opposed to "a Clear crossed". + // + // WHAT IT IS NOT (M7, corrected). This is NOT what arms the split lane. WireTables.h round 2 + // claimed it was "t1's FOURTH arming fact", and that was wrong: the harness reads the + // encoder's EmitSeq (Harness/SplitRuntimePeek.cpp:50, Harness/ScenarioFixture.h:85), which + // moves for the class-B verbs too. So an armed Clear-only lane is green on EmitSeq while this + // counter stays 0. Making the harness read THIS instead is t1's file (SplitRuntimePeek), so + // the honest statement is the one here: c1 counts the routed ordinal at the only place that + // can, PipeFill's InitialBytesNotCarried self-check reads it (PipeFill.cpp:755), and the + // lane's own arming remains EmitSeq until t1 re-points it. c1-v3.md M7 has the full note. Uint64 ClientWireRecordsEmitted(); // How many of those were refused by the server, by acceptance row. Counted rather than // inferred, R-8's rule one level out. Uint64 ClientWireRecordsDeclined(); + // TRUE ON THE SERVER ROLE's OWN THREAD (the apply thread), false everywhere else. It is + // v1's ServerLoop::OnApplyThread(), exposed here because it is table 3's role split made + // into one predicate and TWO packages read it: the wire emitters below (a routed call that + // finds itself on the apply thread runs the monolith adapter, because on that thread this + // process IS the server), and MG_Impl/Pipe/PipeFill.cpp's split-only respecify/flush + // branches (M5: those branches emit CLIENT wire records and must not run on the server's + // apply thread, which produces none - the InitialBytesNotCarried self-check would abort the + // server otherwise). Declared here so PipeFill does not have to include a server header. + Bool RunsAsTheServerRole(); + } // namespace MobileGL::MG_Remote::Client #endif // MOBILEGL_BUILD_DISAGGREGATED diff --git a/MobileGL/MG_Test/Wire/RemoteClientTest.cpp b/MobileGL/MG_Test/Wire/RemoteClientTest.cpp index 1bd97a17..fd2c7b33 100644 --- a/MobileGL/MG_Test/Wire/RemoteClientTest.cpp +++ b/MobileGL/MG_Test/Wire/RemoteClientTest.cpp @@ -37,6 +37,7 @@ #include #include #include +#include #if !defined(_WIN32) #include @@ -521,6 +522,39 @@ TEST(RemoteReadback, TheTightRowsAreScatteredWhereThePackStateSaysAndTheGapsAreL "reply carried"; } +TEST(RemoteReadback, PackSkipImagesIsIgnoredForATwoDimensionalRead) { + // codex 6: SKIP_IMAGES (and IMAGE_HEIGHT) are image-level pack parameters and GL ignores + // them for glReadPixels, a 2-D read - the monolith conversion path says so with + // honorPackImageParams=false (DirectGLES.cpp:10905). The first cut applied SKIP_IMAGES as a + // whole-image offset: a 4x3 RGBA8 read with SKIP_IMAGES=1 wrote bytes 48..95 of a 96-byte + // destination sized for one image, overrunning it. With the fix the reply lands at bytes + // 0..47 and the second image's worth of bytes keeps its sentinel. + constexpr Uint64 kBpp = 4; + constexpr GLsizei kW = 4; + constexpr GLsizei kH = 3; + constexpr Uint8 kSentinel = 0xEE; + + PixelStoreParameters pack{}; + pack.SkipImages = 1; // the parameter under test + pack.ImageHeight = kH; // and its companion; both must be ignored + + std::vector tight(static_cast(kW) * kH * kBpp); + for (size_t i = 0; i < tight.size(); ++i) tight[i] = static_cast(i + 1); + + std::vector destination(96, kSentinel); + ScatterTightReadbackIntoPackState(tight.data(), destination.data(), kW, kH, kBpp, pack); + + for (size_t i = 0; i < tight.size(); ++i) + EXPECT_EQ(destination[i], tight[i]) << "byte " << i << " should hold the reply at offset 0"; + for (size_t i = tight.size(); i < destination.size(); ++i) + EXPECT_EQ(destination[i], kSentinel) + << "byte " << i << " is past the read's own extent and SKIP_IMAGES must not have moved " + "the write there"; + // And the fast path takes it: an otherwise-neutral read with only SKIP_IMAGES set is tight. + EXPECT_TRUE(ReadbackPackStateIsTightForTest(kW, kBpp, pack)) + << "SKIP_IMAGES alone must not force the bounce path for a 2-D read"; +} + TEST(RemoteReadback, TheFastPathIsTakenExactlyWhenTheScatterWouldChangeNothing) { // EmitReadPixels reads the reply STRAIGHT into the application's pointer when // ReadbackPackStateIsTight says so, and pays for a bounce buffer otherwise. That is only @@ -644,6 +678,120 @@ TEST(PipeRouting, AnErrorStatusIsNotFoldedIntoAcceptedOrRefused) { } #endif // MGTEST_HAVE_FORK +// ===================================================================================== +// B3 / codex 9: the CLIENT arm's 37 rows are observed, not just the monolith install +// ===================================================================================== + +namespace { + // How many function-pointer cells of `a` differ from `b`, walked as a block of void* - + // the structs ARE their function pointers (PipeCatalogueTest static_asserts that shape). A + // routed row that stayed on the monolith adapter reads EQUAL and is not counted, which is + // exactly the defect this measures. + template + SizeT CountDifferingCells(const T& a, const T& b) { + const void* const* pa = reinterpret_cast(&a); + const void* const* pb = reinterpret_cast(&b); + SizeT n = 0; + for (SizeT i = 0; i < sizeof(T) / sizeof(void*); ++i) + if (pa[i] != pb[i]) ++n; + return n; + } +} // namespace + +TEST(PipeRouting, TheInstalledClientArmIsWireAndNotMonolithAndEveryRoutedRowMoved) { + // WHAT B3 SAYS IS MISSING. PipeCatalogueTest installs and COUNTS the monolith table, so a + // client row that was never overwritten stays non-null and still counts - deleting one + // `gMGPipe*.X = &Wire_X` assignment left every gate green (the cross-family verifier + // reproduced it: catalogue 32/32, all lanes green). The only thing that catches it is + // observing that the CLIENT install actually MOVED each routed row off the monolith adapter. + // + // The monolith adapters are kept beside the installed tables (MGPipeMonolith*()), so the + // client install differs from them at exactly the routed rows and nowhere else. This reads + // the pointers back rather than constructing them (R-16): a deleted assignment reads equal. + InstallClientWireTables(); + EXPECT_EQ(static_cast(MGPipeInstalledArm()), static_cast(MGPipeRouteArm::kClientWire)) + << "InstallClientWireTables did not record the client-wire arm"; + + const SizeT movedScreen = CountDifferingCells(gMGPipeScreen, MGPipeMonolithScreen()); + const SizeT movedContext = CountDifferingCells(gMGPipeContext, MGPipeMonolithContext()); + EXPECT_EQ(movedScreen + movedContext, 33u) + << "exactly the 33 generated routed rows must differ from the monolith adapters; " + << movedScreen + movedContext + << " did, so a row was left on the monolith adapter (it would run the applier on the GL " + "thread under split) or an unrouted row was overwritten"; + + const SizeT movedEscapes = CountDifferingCells(gMGPipeRouteEscapes, MGPipeMonolithEscapes()); + EXPECT_EQ(movedEscapes, 4u) + << "the four escape routes must move off the monolith escapes too"; + + // Restore the monolith arm for the sibling cases that assert it (and for a clean binary). + MGPipeInstallMonolithTables(); + EXPECT_EQ(static_cast(MGPipeInstalledArm()), static_cast(MGPipeRouteArm::kMonolith)); +} + +#if MGTEST_HAVE_FORK +TEST(PipeRouting, AClientWireRowWithNoSessionRefusesByNameRatherThanApplying) { + // THE RUNTIME HALF of B3, and it distinguishes a Wire_* row from the monolith adapter by + // BEHAVIOUR: with the client tables installed and no session, a routed call reaches + // RequireSession and aborts Fatal{NoClientSession}. The monolith adapter (the deleted- + // assignment state) would instead run MGPipeApply* and NOT abort with that string - so the + // control goes red the moment a row falls back to monolith. + const ChildResult child = RunInChild([] { + InstallClientWireTables(); + MGPHandleOnly handle{}; + handle.Handle = MGPipeHandle{1, 0}; + handle.Kind = static_cast(MGPipeKind::Renderbuffer); + gMGPipeScreen.ResourceDestroy(&handle); // Wire_ResourceDestroy, no session + }); + EXPECT_TRUE(DiedOfAbort(child)) << DescribeStatus(child) << "\n" << child.Log; + EXPECT_NE(child.Log.find("Fatal{NoClientSession, \"ResourceDestroy\"}"), std::string::npos) + << "the installed row did not refuse by name; it may be the monolith adapter (B3)\n" + << child.Log; +} + +TEST(PipeRouting, ARoutedCallDuringTeardownRefusesByNameNotRunsTheApplier) { + // codex 4: UninstallClientWireTables marks the tables uninstalled; a routed call in that + // window must abort by name rather than run the applier on the caller. Reverting Uninstall + // to reinstall the monolith adapters (round 2's behaviour) makes this call run the applier + // and NOT abort with this string - the red-once. + const ChildResult child = RunInChild([] { + InstallClientWireTables(); + UninstallClientWireTables(); + MGPHandleOnly handle{}; + handle.Handle = MGPipeHandle{1, 0}; + handle.Kind = static_cast(MGPipeKind::Renderbuffer); + gMGPipeScreen.ResourceDestroy(&handle); + }); + EXPECT_TRUE(DiedOfAbort(child)) << DescribeStatus(child) << "\n" << child.Log; + EXPECT_NE(child.Log.find("Fatal{ClientTablesUninstalled, \"ResourceDestroy\"}"), std::string::npos) + << "a routed call after UninstallClientWireTables ran the applier on the caller instead " + "of refusing by name (codex 4)\n" + << child.Log; +} +#endif // MGTEST_HAVE_FORK + +// ===================================================================================== +// M2 / codex 11: a short or non-OK reply is refused, never scattered as pixels +// ===================================================================================== + +TEST(RemoteReadback, AReplyIsScatteredOnlyWhenItIsOkAndExactlyTheReadsExtent) { + // EmitReadPixels decides on ReadbackReplyIsComplete before it scatters or returns (the Fatal + // wording each mode owns is at the call site). Driving the production predicate directly: + // only a full OK reply is complete; a short OK reply, and a DECLINE or ERROR with a zero + // payload, are not - and those are the shapes that would otherwise spray stale destination + // bytes as pixels. `tight` is the read's own DstSize (CONTRACT-P5 row 23). + constexpr Int32 kOk = 0, kDeclined = 1, kError = 2; + const Uint64 tight = TightReadbackByteCount(4, 3, 0x1908, 0x1401); // 48 + EXPECT_TRUE(ReadbackReplyIsComplete(kOk, tight, tight)) << "a full OK reply is the only one scattered"; + EXPECT_FALSE(ReadbackReplyIsComplete(kOk, tight - 16, tight)) + << "a SHORT OK reply (one row missing) must not be scattered - the missing rows would be " + "whatever the destination held"; + EXPECT_FALSE(ReadbackReplyIsComplete(kDeclined, 0, tight)) + << "a DECLINED reply carries no pixels"; + EXPECT_FALSE(ReadbackReplyIsComplete(kError, 0, tight)) << "an ERROR reply carries no pixels"; + EXPECT_FALSE(ReadbackReplyIsComplete(kOk, 0, tight)) << "an OK reply of zero bytes is not the extent"; +} + int main(int argc, char** argv) { namespace fs = std::filesystem; const fs::path path =