From 80d205c401748c413ff24d5fc7a037205b7eae48 Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Tue, 29 Sep 2026 04:07:35 -0400 Subject: [PATCH] [Refactor] (Remote, P10 B): the emit table's SetSwapInterval slot is the server forwarder - SetSwapInterval_Unmigrated and the BackendObject_Remote override deleted, class C empty (69 emitted / 0 unmigrated), and a unit control that the slot crosses as one control frame --- .../MG_Remote/Client/BackendObject_Remote.cpp | 14 ++-- .../MG_Remote/Client/BackendObject_Remote.h | 13 ++-- MobileGL/MG_Remote/Client/EmitTables.cpp | 31 ++++++--- MobileGL/MG_Remote/Client/EmitTables.h | 5 +- MobileGL/MG_Test/Wire/RemoteClientTest.cpp | 68 +++++++++++-------- MobileGL/MG_Test/Wire/ServerLoopTest.cpp | 27 ++++++++ 6 files changed, 101 insertions(+), 57 deletions(-) diff --git a/MobileGL/MG_Remote/Client/BackendObject_Remote.cpp b/MobileGL/MG_Remote/Client/BackendObject_Remote.cpp index 4f805fad..c09e8578 100644 --- a/MobileGL/MG_Remote/Client/BackendObject_Remote.cpp +++ b/MobileGL/MG_Remote/Client/BackendObject_Remote.cpp @@ -32,7 +32,8 @@ namespace MobileGL::MG_Remote::Client { // ---- the EGL seam ------------------------------------------------------------------ // - // THE NINE EGL VIRTUALS CALL v1's TWELVE FORWARDERS AND NOTHING ELSE. c1 round 1 built + // THE EGL VIRTUALS CALL v1's TWELVE FORWARDERS AND NOTHING ELSE (the swap interval's + // since P10 through the emit table's slot, not an override here). c1 round 1 built // its own trampolines over ServerLoop's control channel and ServerLoop::Backend(), // which ran the right driver call on the right thread and was still wrong, because // three of the twelve do MORE than forward: @@ -403,15 +404,8 @@ namespace MobileGL::MG_Remote::Client { return MG_Backend::BackendObject::SwapEGLBuffers(dpy, draw); } - void BackendObject_Remote::SetEGLSwapInterval(Int interval) { - // OVERRIDDEN BECAUSE THE BASE WOULD FATAL. BackendObject.cpp:402 null-checks - // GetBackendFunctions().SetSwapInterval and calls it when non-null - one of the 41 - // null checks R-4 turns into "always supported" - and SetSwapInterval is class C, so - // the base implementation would abort on every eglSwapInterval. The answer is the - // caps-mirror-read rule's general shape: the question "can the presentation path take - // an interval" belongs to the server, so it is asked of the server. - Server::ServerSetEGLSwapInterval(interval); - } + // No SetEGLSwapInterval here any more (P10 B): the base class calls the emit table's slot, + // which is now Server::ServerSetEGLSwapInterval itself (EmitTables.cpp). void BackendObject_Remote::ReleaseEGLSurface(EGLSurface surface) { // P12: a released server-owned surface stops taking the server window's geometry. diff --git a/MobileGL/MG_Remote/Client/BackendObject_Remote.h b/MobileGL/MG_Remote/Client/BackendObject_Remote.h index ed252a5d..1af3d405 100644 --- a/MobileGL/MG_Remote/Client/BackendObject_Remote.h +++ b/MobileGL/MG_Remote/Client/BackendObject_Remote.h @@ -40,11 +40,11 @@ // function-pointer mailbox before it), and then lets the // base class keep the client-side books. // -// SetEGLSwapInterval IS THE ONE THAT MUST NOT REACH THE TABLE. The base implementation -// null-checks GetBackendFunctions().SetSwapInterval (BackendObject.cpp:402) - one of the 41 -// null checks R-4 turns into "always supported" - and SetSwapInterval is class C, so the base -// implementation would Fatal on every eglSwapInterval. It is overridden to forward instead, -// which is the caps-mirror-read rule's shape for a slot whose answer is "ask the server". +// SetEGLSwapInterval IS NOT OVERRIDDEN (P10 B). It used to be, because the base implementation +// calls GetBackendFunctions().SetSwapInterval (BackendObject.cpp:402) and that slot was the last +// class-C Fatal in the emit table. The slot is now the forwarder the override called +// (Server::ServerSetEGLSwapInterval, EmitTables.cpp), so the base's route IS the forward and a +// second spelling of it here would only be a second place for the two to drift apart. #pragma once #include @@ -68,14 +68,13 @@ namespace MobileGL::MG_Remote::Client { const MG_Backend::DynamicBackendParameters& GetDynamicParameters() const override; BackendType GetBackendType() const override; - // ---- the nine EGL lifecycle virtuals --------------------------------------------- + // ---- the EGL lifecycle virtuals (SetEGLSwapInterval is the base's: see above) ----- Bool InitializeEGLDisplay(EGLDisplay dpy, EGLint* major, EGLint* minor) override; Bool CreateEGLWindowSurface(EGLSurface surface, const MG_Backend::WindowHandle& handle) override; Bool ResizeEGLWindowSurface(EGLSurface surface, Uint32 width, Uint32 height) override; Bool CreateEGLPbufferSurface(EGLSurface surface, EGLint width, EGLint height) override; Bool MakeEGLCurrent(EGLDisplay dpy, EGLSurface draw, EGLSurface read, EGLContext ctx) override; Bool SwapEGLBuffers(EGLDisplay dpy, EGLSurface draw) override; - void SetEGLSwapInterval(Int interval) override; void ReleaseEGLSurface(EGLSurface surface) override; void ReleaseEGLResources() override; diff --git a/MobileGL/MG_Remote/Client/EmitTables.cpp b/MobileGL/MG_Remote/Client/EmitTables.cpp index daa493e8..9e5cff23 100644 --- a/MobileGL/MG_Remote/Client/EmitTables.cpp +++ b/MobileGL/MG_Remote/Client/EmitTables.cpp @@ -34,6 +34,8 @@ #include "GpuWritePending.h" #include "PersistentMapTracker.h" +#include "../Server/ServerLoop.h" + #include #include #include @@ -1807,8 +1809,8 @@ namespace MobileGL::MG_Remote::Client { // per-package ownership assertions below then still hold, the totals stay arithmetic, // and a slot that changes class without changing the arithmetic is a build break. The // X-macro shape is kept so the DEFINITION and the ASSIGNMENT cannot drift apart. Three - // slots carry a body the macro cannot (DispatchCompute, DispatchComputeIndirect, - // SetSwapInterval) and are written out by hand below. + // slots carried a body the macro cannot (DispatchCompute, DispatchComputeIndirect, + // SetSwapInterval) and were written out by hand below; all three are class B now. // // d1 indexed / instanced / multi-draw / indirect draws -> draw_vbo (59), its // kDrawIsIndirect tail and its kDrawHasUserIndices span @@ -1849,7 +1851,8 @@ namespace MobileGL::MG_Remote::Client { #define MGR_UNMIGRATED_F1_SLOTS(X) - // The wave-3 tail. SetSwapInterval is hand-written below (it is not a GL.* slot). + // The wave-3 tail. Empty since P10 B retired SetSwapInterval (it was not a GL.* slot and + // was never a row here). #define MGR_UNMIGRATED_TAIL_SLOTS(X) // The non-void ones, kept apart only because the macro body differs: a [[noreturn]] @@ -1880,8 +1883,13 @@ namespace MobileGL::MG_Remote::Client { // hook before their Fatal so that the package flipping them would inherit a call site // that was already correct, and EmitDispatchCompute / EmitDispatchComputeIndirect above // are that inheritance - same two calls, same order, the record where the Fatal was. - - void SetSwapInterval_Unmigrated(Int) { UnmigratedVerbFatal("SetSwapInterval"); } + // + // THE LAST HAND-WRITTEN STUB, SetSwapInterval_Unmigrated, IS GONE TOO (P10 B), and class C + // is empty. It was never reachable: eglSwapInterval reaches the table only through + // BackendObject::SetEGLSwapInterval, and BackendObject_Remote overrode that to call + // Server::ServerSetEGLSwapInterval - a SurfaceControlFrame on the control channel, which + // crosses processes. The slot now IS that forwarder (assigned in the class-B block), the + // override is deleted, and there is one route from eglSwapInterval to the server. // The counts, as arithmetic. MGR_COUNT_ONE expands to `+ 1` per row. #define MGR_COUNT_ONE(Name, Ret, Sig) +1 @@ -1890,9 +1898,8 @@ namespace MobileGL::MG_Remote::Client { constexpr Uint32 kUnmigratedI1 = 0 MGR_UNMIGRATED_I1_SLOTS(MGR_COUNT_ONE); constexpr Uint32 kUnmigratedT2 = 0 MGR_UNMIGRATED_T2_SLOTS(MGR_COUNT_ONE); constexpr Uint32 kUnmigratedF1 = 0 MGR_UNMIGRATED_F1_SLOTS(MGR_COUNT_ONE); - // + SetSwapInterval, written out by hand. constexpr Uint32 kUnmigratedTail = - 0 MGR_UNMIGRATED_TAIL_SLOTS(MGR_COUNT_ONE) MGR_UNMIGRATED_TAIL_VALUE_SLOTS(MGR_COUNT_ONE) + 1; + 0 MGR_UNMIGRATED_TAIL_SLOTS(MGR_COUNT_ONE) MGR_UNMIGRATED_TAIL_VALUE_SLOTS(MGR_COUNT_ONE); #undef MGR_COUNT_ONE constexpr Uint32 kUnmigratedSlots = kUnmigratedD1 + kUnmigratedI1 + kUnmigratedT2 + kUnmigratedF1 + kUnmigratedTail; @@ -1903,7 +1910,9 @@ namespace MobileGL::MG_Remote::Client { constexpr Uint32 kEmittedSlotsI1 = 7; constexpr Uint32 kEmittedSlotsT2 = 7; constexpr Uint32 kEmittedSlotsF1 = 11; - constexpr Uint32 kEmittedSlotsTail = 3; // BlitNamedFramebuffer, both texture readbacks + // BlitNamedFramebuffer, both texture readbacks, and SetSwapInterval (P10 B) - the one + // class-B slot that crosses as a surface control frame rather than a SEG_CMD record. + constexpr Uint32 kEmittedSlotsTail = 4; constexpr Uint32 kEmittedSlotsSync = 5; constexpr Uint32 kEmittedSlotsQueries = 11; constexpr Uint32 kEmittedSlots = @@ -1938,7 +1947,6 @@ namespace MobileGL::MG_Remote::Client { MGR_UNMIGRATED_GL_SLOTS(MGR_ASSIGN_UNMIGRATED) MGR_UNMIGRATED_GL_VALUE_SLOTS(MGR_ASSIGN_UNMIGRATED) #undef MGR_ASSIGN_UNMIGRATED - table.SetSwapInterval = &SetSwapInterval_Unmigrated; table.GL.FenceSync = &EmitFenceSync; table.GL.ClientWaitSync = &EmitClientWaitSync; @@ -1996,6 +2004,11 @@ namespace MobileGL::MG_Remote::Client { table.GL.BlitFramebuffer = &EmitBlitFramebuffer; table.GL.BlitNamedFramebuffer = &EmitBlitNamedFramebuffer; table.Present = &EmitPresent; + // P10 B: the swap interval is the server's presentation-path question, so the slot + // forwards it there as a SurfaceControlFrame (ServerLoop.cpp's SetSwapInterval arm), + // across processes on spawn and tcp - the forwarder BackendObject_Remote's deleted + // SetEGLSwapInterval override used to call. + table.SetSwapInterval = &Server::ServerSetEGLSwapInterval; // ---- class B, P5b t2. Assigned AFTER the class-C block above, which is what makes // the flip a single-line change per slot: the Fatal thunk is overwritten, and a slot // whose row is removed from MGR_UNMIGRATED_T2_SLOTS but not assigned here would be diff --git a/MobileGL/MG_Remote/Client/EmitTables.h b/MobileGL/MG_Remote/Client/EmitTables.h index 7e5c40d2..cac4a8b2 100644 --- a/MobileGL/MG_Remote/Client/EmitTables.h +++ b/MobileGL/MG_Remote/Client/EmitTables.h @@ -38,7 +38,10 @@ // Present has ZERO MG_Impl call sites: it is reached through EGLImpl.cpp:178 -> // BackendObject.cpp:396, so mirroring GLImpl will not find it. // C. Fatal{UnmigratedVerb} - the remaining 64, SetSwapInterval and GetGpuTimestampNs among -// them. +// them. (That was P5's partition. P5b..P10 moved every one of them to B; the last, +// SetSwapInterval, is a forward to the server's control channel since P10, and class C is +// empty. UnmigratedVerbFatal below remains the named refusal of a class-B emitter's +// unrepresentable SHAPE - "+".) // // AND THE RULE R-4 WOULD OTHERWISE BREAK. 41 of the 69 slots are null-checked at their call // site, and several of those checks are CAPABILITY PROBES, not safety checks - BeginOcclusionQuery diff --git a/MobileGL/MG_Test/Wire/RemoteClientTest.cpp b/MobileGL/MG_Test/Wire/RemoteClientTest.cpp index 13681605..d3b2aabf 100644 --- a/MobileGL/MG_Test/Wire/RemoteClientTest.cpp +++ b/MobileGL/MG_Test/Wire/RemoteClientTest.cpp @@ -192,10 +192,11 @@ namespace { // ===================================================================================== TEST(RemoteEmitTable, TheThreeClassesPartitionAllSeventyOneSlots) { - // P5 baseline five + f1 eleven + i1 seven + t2 six emitted slots. + // P5 baseline five + f1 eleven + i1 seven + t2 six emitted slots, ... and since P10 B the + // swap interval: class C is EMPTY, every slot but the two local answers reaches the server. EXPECT_EQ(LocallyAnsweredSlotCount(), 2u); - EXPECT_EQ(ImplementedVerbCount(), 68u); - EXPECT_EQ(UnmigratedSlotCount(), 1u); + EXPECT_EQ(ImplementedVerbCount(), 69u); + EXPECT_EQ(UnmigratedSlotCount(), 0u); EXPECT_EQ(LocallyAnsweredSlotCount() + ImplementedVerbCount() + UnmigratedSlotCount(), kRemoteEmitSlotCount); } @@ -333,27 +334,32 @@ TEST(RemoteEmitTable, TheFiveEmittersAreTheOnesTheCensusMeasured) { } #if MGTEST_HAVE_FORK -TEST(RemoteEmitTable, AnUnmigratedSlotAbortsAndNamesItself) { +TEST(RemoteEmitTable, AnUnmigratedShapeRefusalAbortsAndNamesItself) { // THE DEATH TEST ON THE UnmigratedVerbFatal ARM. It asserts the exact wording, not merely // that the child died: a control that trips on any abort is satisfied by the wrong abort, // which is one of the three shapes R-16 was written after. - // GetTexImage is the wave-3 tail (CONTRACT-P5B.md ยง7): no P5b package flips it, so this - // case keeps its subject across the four P5b landings. (It was DrawElements until d1 made - // that a class-B emitter.) - const ChildResult r = RunInChild([] { RemoteEmitTable().SetSwapInterval(1); }); + // NO SLOT IS CLASS C ANY MORE: P10 B retired SetSwapInterval, the last one and this case's + // subject until then. The funnel is still the named refusal of a class-B emitter's + // unrepresentable SHAPE (RefuseDrawByName, EmitF1Clear's default arm), and every refusal + // left on it is spelled "+" - so that is what is driven here, through the + // funnel itself, because every emitter that reaches it needs a session and a context first. + const ChildResult r = + RunInChild([] { UnmigratedVerbFatal("MultiDrawElementsBaseVertex+CLIENT_INDICES"); }); ASSERT_TRUE(DiedOfAbort(r)) << DescribeStatus(r) << "\n" << r.Log; - EXPECT_NE(r.Log.find("Fatal{UnmigratedVerb, \"SetSwapInterval\"}"), std::string::npos) << r.Log; + EXPECT_NE(r.Log.find("Fatal{UnmigratedVerb, \"MultiDrawElementsBaseVertex+CLIENT_INDICES\"}"), + std::string::npos) + << r.Log; } -TEST(RemoteEmitTable, EachUnmigratedSlotNamesItsOwnSlot) { +TEST(RemoteEmitTable, EachUnmigratedShapeRefusalNamesItsOwnShape) { // The half the case above cannot state on its own: that the name in the message is the - // slot's and not a constant. Two different slots, two different names - both from the - // wave-3 tail, for the reason the case above gives. - const ChildResult r = RunInChild([] { RemoteEmitTable().SetSwapInterval(1); }); + // caller's and not a constant. Two different shapes, two different names. + const ChildResult r = RunInChild([] { UnmigratedVerbFatal("DrawArraysIndirect+CLIENT_COMMANDS"); }); ASSERT_TRUE(DiedOfAbort(r)) << DescribeStatus(r) << "\n" << r.Log; - EXPECT_NE(r.Log.find("Fatal{UnmigratedVerb, \"SetSwapInterval\"}"), std::string::npos) << r.Log; - EXPECT_EQ(r.Log.find("GetTexImage"), std::string::npos) - << "the Fatal message names a slot other than the one that was called:\n" + EXPECT_NE(r.Log.find("Fatal{UnmigratedVerb, \"DrawArraysIndirect+CLIENT_COMMANDS\"}"), std::string::npos) + << r.Log; + EXPECT_EQ(r.Log.find("MultiDrawElementsBaseVertex"), std::string::npos) + << "the Fatal message names a shape other than the one that was refused:\n" << r.Log; } @@ -373,13 +379,20 @@ TEST(RemoteEmitTable, AFlippedDrawSlotDemandsASessionRatherThanNamingItselfUnmig << r.Log; } -TEST(RemoteEmitTable, SetSwapIntervalIsClassCAndSaysSo) { +TEST(RemoteEmitTable, SetSwapIntervalIsTheServerForwarderAndNotAFatal) { // The slot the verb census found by NOT mirroring GLImpl: SetSwapInterval has zero MG_Impl - // call sites and is reached only through the EGL path, so a table built from the 89 GLImpl - // sites would have left it null. + // call sites and is reached only through the EGL path (BackendObject::SetEGLSwapInterval), + // so a table built from the 89 GLImpl sites would have left it null. It was the last class-C + // slot; since P10 B it IS the control-frame forwarder the old BackendObject_Remote override + // called. With no apply loop in this child the forwarder answers NOT_INITIALIZED and returns + // (ServerLoop.cpp, PostSurfaceControlFrameWithCallerLock), so the child EXITS - the P5 stub + // died Fatal{UnmigratedVerb, "SetSwapInterval"} right here. That the frame then reaches a + // running server is ServerLoopTest's TheEmitTableSwapIntervalSlotCrossesAsOneDispatchedFrame. + EXPECT_EQ(reinterpret_cast(RemoteEmitTable().SetSwapInterval), + reinterpret_cast(&MobileGL::MG_Remote::Server::ServerSetEGLSwapInterval)); const ChildResult r = RunInChild([] { RemoteEmitTable().SetSwapInterval(1); }); - ASSERT_TRUE(DiedOfAbort(r)) << DescribeStatus(r) << "\n" << r.Log; - EXPECT_NE(r.Log.find("Fatal{UnmigratedVerb, \"SetSwapInterval\"}"), std::string::npos) << r.Log; + ASSERT_TRUE(WIFEXITED(r.Status) && WEXITSTATUS(r.Status) == 0) << DescribeStatus(r) << "\n" << r.Log; + EXPECT_EQ(r.Log.find("Fatal{UnmigratedVerb"), std::string::npos) << r.Log; } #endif // MGTEST_HAVE_FORK @@ -389,13 +402,11 @@ TEST(RemoteEmitTable, SetSwapIntervalIsClassCAndSaysSo) { TEST(RemoteEmitTable, TheSevenI1SlotsAreClassBAndAreNotTheFatalThunk) { // The census's five measured slots plus the two companions that share their rows. Named // rather than counted, so a table that flipped a DIFFERENT seven is red here and not only - // in the arithmetic. The comparison is against a slot that is still class C: a flipped slot - // and an unflipped one cannot be the same pointer, which is what a forgotten class-B - // assignment would look like (class C is assigned FIRST in BuildRemoteEmitTable precisely so - // that the mistake is loud rather than null). + // in the arithmetic. This case used to compare each slot against a slot that was still + // class C; since P10 B there is none left to compare against, so the class is decided + // behaviourally by EachI1SlotReachesRequireSessionUnderItsOwnName below and this case keeps + // the pointer facts that need no fork. const MG_Backend::GlobalBackendFunctionsTable& table = RemoteEmitTable(); - const void* fatal = reinterpret_cast(table.SetSwapInterval); - ASSERT_NE(fatal, nullptr); const void* const i1[] = { reinterpret_cast(table.GL.BindImageTexture), reinterpret_cast(table.GL.DispatchCompute), @@ -411,9 +422,6 @@ TEST(RemoteEmitTable, TheSevenI1SlotsAreClassBAndAreNotTheFatalThunk) { "ShaderStorageBlockBinding"}; for (SizeT i = 0; i < sizeof(i1) / sizeof(i1[0]); ++i) { EXPECT_NE(i1[i], nullptr) << kNames[i] << " is null"; - EXPECT_NE(i1[i], fatal) << kNames[i] - << " still points at an UnmigratedVerbFatal thunk; i1 flipped it " - "to class B"; } // The two barrier slots and the two dispatch slots share a WIRE ROW but not an emitter: the // discriminant (ByRegion / IsIndirect) is set by the emitter, so one thunk for both would diff --git a/MobileGL/MG_Test/Wire/ServerLoopTest.cpp b/MobileGL/MG_Test/Wire/ServerLoopTest.cpp index 3ac3016e..536e60e1 100644 --- a/MobileGL/MG_Test/Wire/ServerLoopTest.cpp +++ b/MobileGL/MG_Test/Wire/ServerLoopTest.cpp @@ -45,6 +45,7 @@ #include #include #include +#include #include #include #include @@ -1199,6 +1200,32 @@ TEST(ServerLoopTest, AVoidForwarderCrossesAsOneDispatchedFrame) { fixture.Stop(); } +// P10 B: THE EMIT TABLE'S SetSwapInterval SLOT REACHES THE SERVER. It was the last class-C slot +// (Fatal{UnmigratedVerb, "SetSwapInterval"}) and is now the forwarder above, which is what +// BackendObject::SetEGLSwapInterval calls for every eglSwapInterval on the client. The case goes +// through the TABLE, not the forwarder by name, and requires the frame to have crossed: the same +// dispatch counter as the case above. Red once by pointing the slot at a no-op lambda in +// BuildRemoteEmitTable - the counter stays 0 - and with the P5 stub restored the call aborts +// with Fatal{UnmigratedVerb, "SetSwapInterval"}. +TEST(ServerLoopTest, TheEmitTableSwapIntervalSlotCrossesAsOneDispatchedFrame) { + ServerFixture fixture; + ASSERT_TRUE(fixture.Handshake()); + ASSERT_TRUE(fixture.StartLoop()); + Server::ServerLoop& loop = Server::ServerLoopInstance(); + ASSERT_TRUE(fixture.WaitUntilTrulyParked()); + ASSERT_EQ(loop.ControlFramesDispatched(), 0u); + + const auto slot = MobileGL::MG_Remote::Client::RemoteEmitTable().SetSwapInterval; + ASSERT_NE(slot, nullptr); + slot(1); + + EXPECT_EQ(loop.ControlFramesDispatched(), 1u) + << "the emit table's SetSwapInterval slot did not put exactly one control frame on the " + "server's channel; eglSwapInterval under split would not reach the server"; + + fixture.Stop(); +} + // P5d round 3, package D: THE IDENTITY MUST DIE WITH THE THREAD, and this is the case that says // so. OnApplyThread() is no longer `m_running && id == mine` - it is one relaxed load of // Server::Detail::g_applyThreadKey compared against the caller's own thread pointer - so there