diff --git a/MobileGL/ConfigLoader.cpp b/MobileGL/ConfigLoader.cpp index a9686a22..5ea497ba 100644 --- a/MobileGL/ConfigLoader.cpp +++ b/MobileGL/ConfigLoader.cpp @@ -336,10 +336,32 @@ namespace MobileGL::MG_ConfigLoader { } // The three P6 forms. Recognised precisely, so the diagnostic can say "not yet" // rather than "unknown", which are different bugs on the operator's side. - if (lowered == "spawn" || lowered.compare(0, 5, "unix:") == 0 || - lowered.compare(0, 5, "pipe:") == 0) { - MGLOG_E("Config: MOBILEGL_TRANSPORT='%s' names a transport P6 implements and P5 does " - "not; staying on monolith. This run is NOT a split run.", + // P6 `sm`/`cp`: spawn is IMPLEMENTED. It launches a server process on a + // rendezvous of its own and connects to it - the two processes are + // independent, so this is `Dial = Connect` with a launcher attached + // rather than the fork-coupled shape ARCHITECTURE.md:488 described. + if (lowered == "spawn") { + MG_Config::Transport = MG_Config::TransportMode::Spawn; + // THE SAME SHAPE AS THE inproc LINE ABOVE, AND FOR THE SAME REASON. + // run_trace_case.cmake asserts on a distinctive sentence from this + // function rather than on `MOBILEGL_TRANSPORT=spawn` alone, because + // ConfigLoader logs an `Accepted env variable: KEY=VALUE` line for + // every MOBILEGL_* in EVERY build including the pull one - so the + // bare KEY=VALUE is satisfied by a monolith library that parsed + // nothing (review M-5). A spawn arm with no line of its own could + // only have been asserted with the inproc marker, which spawn never + // emits: the arm would have been red for a missing sentence rather + // than green for a working transport. + MGLOG_I("Config: MOBILEGL_TRANSPORT=spawn - the MGPipe record stream crosses a real " + "ring to an apply thread in ANOTHER PROCESS"); + return; + } + if (lowered.compare(0, 5, "unix:") == 0 || lowered.compare(0, 5, "pipe:") == 0) { + // unix: and pipe: remain P6+ - connecting to an endpoint SOMEBODY + // ELSE is listening on needs the server lifecycle to be somebody + // else's too, which is P12's. Named, never a silent degrade. + MGLOG_E("Config: MOBILEGL_TRANSPORT='%s' names a transport P6 does not implement yet; " + "staying on monolith. This run is NOT a split run.", value.c_str()); MG_Config::Transport = MG_Config::TransportMode::Monolith; return; diff --git a/MobileGL/MG_Backend/Init.cpp b/MobileGL/MG_Backend/Init.cpp index 71937ee1..70413581 100644 --- a/MobileGL/MG_Backend/Init.cpp +++ b/MobileGL/MG_Backend/Init.cpp @@ -226,6 +226,26 @@ namespace MobileGL::MG_Backend { // the client half here. Bool InitSplitRoles() { using namespace MobileGL::MG_Remote; + + // SPAWN: THE SERVER ROLE IS NOT IN THIS PROCESS. Steps 1 and 2 - + // the backend and the two CallMask halves - happen in the server, + // which ran InitServerRoleForSpawn before it accepted us. Running + // them here as well would build a second BackendObject in the one + // process that must not have one, and would publish a capability + // mask computed from the WRONG backend's function table. + if (MG_Config::Transport == MG_Config::TransportMode::Spawn) { + const MobileGLResult started = Client::ClientSessionInstance().StartSpawned(); + if (started != MOBILEGL_OK) { + MGLOG_E("MG_Remote: the spawn session failed to start (rc=%d); MobileGL will " + "NOT fall back to monolith - a lane named split that ran monolith is " + "the one failure this phase is built to make impossible", + static_cast(started)); + return false; + } + pActiveBackendObject = MakeUnique(); + return true; + } + if (!InitServerRoleCommon()) { return false; } @@ -308,6 +328,19 @@ namespace MobileGL::MG_Backend { MGLOG_W("Failed to initialize MobileGL backend libraries for the remote object"); return; } + // P6: BOTH CHECKS BELOW ARE STATEMENTS ABOUT THE SERVER'S PROCESS, and under + // spawn that is not this one. Their own wording says so - "a mask is a + // statement about THIS BACKEND", "the table the SERVER'S BACKEND registered at + // step 1" - and in a spawn client there is no backend and never was a table, so + // MGPipeGetResourceOps() is null by construction and the first check fires on a + // correct session. It did, on the first spawn retrace ever run. + // + // The checks are NOT weakened: the server runs the step-2 one itself, inside + // InitServerRoleForSpawn, against its own freshly registered table - which is the + // process where the question has an answer. What is lost under spawn is the + // step-5 re-run's ability to catch a table REPLACED between accept and here, and + // that is a question about one address space; it cannot be asked across two. + if (MG_Config::Transport != MG_Config::TransportMode::Spawn) { // m-6, re-worded per review v2 N-8. The honesty cross-check runs a SECOND time, now // that step 4's pActiveBackendObject (the client's BackendObject_Remote) exists and // its Initialize() has run inside InitSpecificBackendLibs. What the re-run CAN catch @@ -329,6 +362,7 @@ namespace MobileGL::MG_Backend { static_cast(g_resourceOpsAtStep2)); std::abort(); } + } LogBackendInfo(); return; } diff --git a/MobileGL/MG_Remote/Client/ClientSession.cpp b/MobileGL/MG_Remote/Client/ClientSession.cpp index 0b8d952d..54a4262c 100644 --- a/MobileGL/MG_Remote/Client/ClientSession.cpp +++ b/MobileGL/MG_Remote/Client/ClientSession.cpp @@ -57,6 +57,9 @@ namespace MobileGL::MG_Remote::Client { // is gone is a different fact from a peer that is slow, and only the // doorbell death latch may say which. constexpr Uint32 kRemoteControlTimeoutMs = 5000; + // The server has to create a backend before it binds, and a cold + // software rasteriser is not fast. Bounded, and a NAMED refusal at the end. + constexpr Uint32 kSpawnConnectTimeoutMs = 20000; // Teardown's drain. Also bounded, and for the same reason - table 3's order is // "publish and wait for the server to drain and acknowledge", and a wait with no // deadline there turns a lost record into a hung process exit. @@ -614,6 +617,55 @@ namespace MobileGL::MG_Remote::Client { /*startApplyThreadHere=*/true); } + MobileGLResult ClientSession::StartSpawned() { + if (m_started) { + return MOBILEGL_ERR_INVALID_ARGUMENT; + } + // A rendezvous nobody else can collide with. Independent processes need + // a NAME; making it per-process-per-instant is what stops two runs on + // one machine - which `ctest -j` produces by construction - finding each + // other's server. + const std::string endpoint = std::string("/tmp/mgl-") + + std::to_string(static_cast(::getpid())) + "-" + + std::to_string(reinterpret_cast(this)) + + ".sock"; + + const MobileGLResult launched = + Server::LaunchServer(std::string(MG_Config::Ipc.ServerPath.c_str()), endpoint, + &m_spawned); + if (launched != MOBILEGL_OK) { + return launched; + } + + std::unique_ptr socket; + // The bounded connect retry is what tolerates the server still binding - + // and it is the CLIENT's, not the launcher's. An exhausted budget is a + // NAMED refusal; there is no monolith fallback from here. + const MobileGLResult connected = + Transport::SocketTransport::ConnectTo(endpoint, kSpawnConnectTimeoutMs, socket); + if (connected != MOBILEGL_OK) { + int exitCode = 0; + Server::ReapServer(m_spawned, 2000, &exitCode); + MGLOG_E("MG_Remote client: could not reach the server we launched at \"%s\" " + "(rc=%d, server exit=%d)", + endpoint.c_str(), static_cast(connected), exitCode); + return connected; + } + // THE ONE FACT A MONOLITH OR inproc RUN CANNOT PRODUCE: a pid that is + // not ours. The ConfigLoader marker proves the VALUE resolved; this + // proves the SERVER ROLE LEFT THIS PROCESS, which is the whole claim the + // SPAWN arm of the retrace matrix makes. run_trace_case.cmake requires + // BOTH, so a spawn arm cannot go green on a transport that resolved and + // then quietly ran the server role locally. + const MobileGLResult started = StartOverSocket(std::move(socket)); + if (started == MOBILEGL_OK) { + MGLOG_I("MG_Remote client: spawn ARMED - the server role runs in pid %d, reached at " + "\"%s\"", + m_spawned.pid, endpoint.c_str()); + } + return started; + } + MobileGLResult ClientSession::StartOverSocket( std::unique_ptr transport) { if (m_started) { @@ -702,8 +754,9 @@ namespace MobileGL::MG_Remote::Client { // The slot travels in the sideband rather than being inferred from arrival // order, so a reordered or dropped offer is a NAMED mismatch instead of two // segments quietly swapped. - // Six offers: four segments, then the two bells (slots 4 and 5). - int fds[6] = {-1, -1, -1, -1, -1, -1}; + // Seven offers: four segments, then THREE bell descriptors (slots 4..6). + // Three because each side parks on one and rings another; see ServerMain. + int fds[7] = {-1, -1, -1, -1, -1, -1, -1}; { struct Sideband { Uint32 slot; @@ -717,7 +770,7 @@ namespace MobileGL::MG_Remote::Client { } } }; - for (int received = 0; received < 6; ++received) { + for (int received = 0; received < 7; ++received) { Sideband sideband{}; Uint64 sidebandSize = 0; int fd = -1; @@ -742,7 +795,7 @@ namespace MobileGL::MG_Remote::Client { } std::memcpy(&sideband, scratch.data(), sizeof(Sideband)); const bool isSegment = sideband.slot < 4; - if (sideband.slot >= 6 || fds[sideband.slot] >= 0 || + if (sideband.slot >= 7 || fds[sideband.slot] >= 0 || (isSegment && sideband.bytes != announced[sideband.slot])) { MGLOG_E("MG_Remote client: segment offer names slot %u with %llu bytes; " "Welcome announced a different size or the slot is already filled", @@ -770,8 +823,14 @@ namespace MobileGL::MG_Remote::Client { // WHICH BELL IS WHICH IS THE SESSION'S KNOWLEDGE, exactly as under // inproc: slot 4 is the pair end whose WRITE wakes the server, slot 5 // is the one this side POLLS. - m_socketPeerBell = MakeUnique(fds[4], 1, true); - m_socketSelfBell = MakeUnique(fds[5], 1, true); + // PARK ON ONE END, RING THE OTHER - the same split the server makes, + // and for the same reason: a single-fd bell rings the PEER, so it + // cannot wake its own waiter. + // fds[4] ring the SERVER (ring-only, we never park on it) + // fds[5] our park end + // fds[6] ring OURSELVES + m_socketPeerBell = MakeUnique(-1, fds[4], 1, true); + m_socketSelfBell = MakeUnique(fds[5], fds[6], 1, true); } // `cp`: from here the twelve EGL forwarders cross the socket instead of @@ -1116,6 +1175,25 @@ namespace MobileGL::MG_Remote::Client { // process that no longer has a session. During the whole span above, the raised flag made // any routed call abort by name instead. ReinstallMonolithAfterTeardown(); + // P6: the server we launched. Closing the transport is what it sees as + // EOF and EOF is its whole exit condition, so the order is transport + // first, reap second - and the reap is BOUNDED, because a server that + // will not go is a different fact from one that is slow and only the + // exit code can say which. + if (m_spawned.pid >= 0) { + if (m_socketTransport) { + m_socketTransport->Shutdown(); + } + int exitCode = 0; + if (Server::ReapServer(m_spawned, 3000, &exitCode) != MOBILEGL_OK) { + MGLOG_W("MG_Remote client: the spawned server (pid %d) did not exit within 3 s", + m_spawned.pid); + } else if (exitCode != 0) { + MGLOG_W("MG_Remote client: the spawned server exited %d", exitCode); + } + } + Server::ServerLoopInstance().SetRemoteControlSink(nullptr, nullptr); + } Wire::PipeWireEncoder& ClientSession::Encoder() { return m_encoder; } diff --git a/MobileGL/MG_Remote/Client/ClientSession.h b/MobileGL/MG_Remote/Client/ClientSession.h index f823fb96..194df5df 100644 --- a/MobileGL/MG_Remote/Client/ClientSession.h +++ b/MobileGL/MG_Remote/Client/ClientSession.h @@ -38,6 +38,7 @@ #include #include +#include "../Server/ServerSpawn.h" #include "../Server/SurfaceControlFrame.h" #include "../Transport/Doorbell.h" #include "../Transport/EventRing.h" @@ -108,6 +109,12 @@ namespace MobileGL::MG_Remote::Client { // RUNNING server process. The two bells arrive over it, with the four // segments, as SCM_RIGHTS offers - nothing here is inherited, because // the two processes were started independently. + // P6: launch a server process and connect to it. The two are + // INDEPENDENT - the launcher hands over a rendezvous name and nothing + // else - so this is the same code path a client would use against a + // server that was already running when it started. + MobileGLResult StartSpawned(); + MobileGLResult StartOverSocket(std::unique_ptr transport); MobileGLResult StartOverTransportPair(std::unique_ptr clientEnd, @@ -391,6 +398,9 @@ namespace MobileGL::MG_Remote::Client { std::mutex m_remoteControlMutex; Uint64 m_remoteControlSeq = 0; + // The server we launched, if we launched one. Reaped in Stop(). + Server::LaunchedServer m_spawned; + std::unique_ptr m_socketTransport; std::unique_ptr m_socketSelfBell; std::unique_ptr m_socketPeerBell; diff --git a/MobileGL/MG_Remote/Protocol/SurfaceOpCodec.cpp b/MobileGL/MG_Remote/Protocol/SurfaceOpCodec.cpp index 1d0bbd0a..a3aa2847 100644 --- a/MobileGL/MG_Remote/Protocol/SurfaceOpCodec.cpp +++ b/MobileGL/MG_Remote/Protocol/SurfaceOpCodec.cpp @@ -47,9 +47,13 @@ namespace MobileGL::MG_Remote { static_cast(::MobileGL::Wire::SurfaceOpKind::ReleaseResources)); static_assert(static_cast(SurfaceControlOp::SetWindowHandle) == static_cast(::MobileGL::Wire::SurfaceOpKind::SetWindowHandle)); + // cp's one schema addition, on the same append-only terms. + static_assert(static_cast(SurfaceControlOp::InitCapabilities) == + static_cast(::MobileGL::Wire::SurfaceOpKind::InitCapabilities)); static_assert(static_cast(::MobileGL::Wire::SurfaceOpKind::SetSwapInterval) == 8 && static_cast(::MobileGL::Wire::SurfaceOpKind::ReleaseResources) == 9 && - static_cast(::MobileGL::Wire::SurfaceOpKind::SetWindowHandle) == 10); + static_cast(::MobileGL::Wire::SurfaceOpKind::SetWindowHandle) == 10 && + static_cast(::MobileGL::Wire::SurfaceOpKind::InitCapabilities) == 11); static_assert(static_cast(::MobileGL::Wire::WindowKind::MetalLayer) == 6); bool OpNeedsAWindowBackend(SurfaceControlOp op) { @@ -80,8 +84,12 @@ namespace MobileGL::MG_Remote { } bool SurfaceControlOpForWireKind(::MobileGL::Wire::SurfaceOpKind kind, SurfaceControlOp* out) { + // THE UPPER BOUND IS THE LAST APPENDED KIND, and it moves with every + // append or the new tag decodes as out-of-range - which reads as + // Fatal{ProtocolCorruption} at ServerApplyWireSurfaceOp rather than as + // the missing row it is. if (::flatbuffers::IsOutRange(kind, ::MobileGL::Wire::SurfaceOpKind::InitializeDisplay, - ::MobileGL::Wire::SurfaceOpKind::SetWindowHandle)) { + ::MobileGL::Wire::SurfaceOpKind::InitCapabilities)) { return false; } const SurfaceControlOp op = static_cast(static_cast(kind)); diff --git a/MobileGL/MG_Remote/Protocol/generated/protocol_generated.h b/MobileGL/MG_Remote/Protocol/generated/protocol_generated.h index 87b6e287..a209a7d1 100644 --- a/MobileGL/MG_Remote/Protocol/generated/protocol_generated.h +++ b/MobileGL/MG_Remote/Protocol/generated/protocol_generated.h @@ -125,11 +125,12 @@ enum class SurfaceOpKind : uint8_t { SetSwapInterval = 8, ReleaseResources = 9, SetWindowHandle = 10, + InitCapabilities = 11, MIN = None, - MAX = SetWindowHandle + MAX = InitCapabilities }; -inline const SurfaceOpKind (&EnumValuesSurfaceOpKind())[11] { +inline const SurfaceOpKind (&EnumValuesSurfaceOpKind())[12] { static const SurfaceOpKind values[] = { SurfaceOpKind::None, SurfaceOpKind::InitializeDisplay, @@ -141,13 +142,14 @@ inline const SurfaceOpKind (&EnumValuesSurfaceOpKind())[11] { SurfaceOpKind::ReleaseCurrent, SurfaceOpKind::SetSwapInterval, SurfaceOpKind::ReleaseResources, - SurfaceOpKind::SetWindowHandle + SurfaceOpKind::SetWindowHandle, + SurfaceOpKind::InitCapabilities }; return values; } inline const char * const *EnumNamesSurfaceOpKind() { - static const char * const names[12] = { + static const char * const names[13] = { "None", "InitializeDisplay", "CreateWindowSurface", @@ -159,13 +161,14 @@ inline const char * const *EnumNamesSurfaceOpKind() { "SetSwapInterval", "ReleaseResources", "SetWindowHandle", + "InitCapabilities", nullptr }; return names; } inline const char *EnumNameSurfaceOpKind(SurfaceOpKind e) { - if (::flatbuffers::IsOutRange(e, SurfaceOpKind::None, SurfaceOpKind::SetWindowHandle)) return ""; + if (::flatbuffers::IsOutRange(e, SurfaceOpKind::None, SurfaceOpKind::InitCapabilities)) return ""; const size_t index = static_cast(e); return EnumNamesSurfaceOpKind()[index]; } diff --git a/MobileGL/MG_Remote/Protocol/protocol.fbs b/MobileGL/MG_Remote/Protocol/protocol.fbs index 9fb8d2bc..fe5429dc 100644 --- a/MobileGL/MG_Remote/Protocol/protocol.fbs +++ b/MobileGL/MG_Remote/Protocol/protocol.fbs @@ -176,6 +176,12 @@ enum SurfaceOpKind : ubyte { SetSwapInterval = 8, ReleaseResources = 9, SetWindowHandle = 10, + // P6 (package cp) appended this one. The draft held that InitCapabilities needs + // no wire form because "the CapsSnapshot answers it" - but a CapsSnapshot is the + // ANSWER and this is the REQUEST, and a spawn client has a live caller for it + // (Client/BackendObject_Remote.cpp). Without the tag the codec refused it by + // name and every spawned eglMakeCurrent failed with InprocOnlyOpOnTheWire. + InitCapabilities = 11, } enum WindowKind : ubyte { diff --git a/MobileGL/MG_Remote/Server/ServerLoop.cpp b/MobileGL/MG_Remote/Server/ServerLoop.cpp index 615e788b..a021c9ef 100644 --- a/MobileGL/MG_Remote/Server/ServerLoop.cpp +++ b/MobileGL/MG_Remote/Server/ServerLoop.cpp @@ -1030,7 +1030,7 @@ namespace MobileGL::MG_Remote::Server { backend->SetWindowHandle(UnpackWindowHandle(frame)); frame.ok = true; return MOBILEGL_OK; - case SurfaceControlOp::InitCapabilitiesInprocOnly: + case SurfaceControlOp::InitCapabilities: // Inproc-only (f0-egl §4.2): on the wire the ANSWER to this op is the CapsSnapshot // frame itself, so there is no SurfaceOp kind for it. The frame channel still has to // carry it inproc because InitCapabilities runs GL queries and therefore belongs to @@ -1172,7 +1172,7 @@ namespace MobileGL::MG_Remote::Server { Bool ServerInitCapabilities() { SurfaceControlFrame frame; - frame.kind = SurfaceControlOp::InitCapabilitiesInprocOnly; + frame.kind = SurfaceControlOp::InitCapabilities; const MobileGLResult rc = ServerLoopInstance().RunSurfaceControlFrame(frame); return rc == MOBILEGL_OK && frame.ok; } diff --git a/MobileGL/MG_Remote/Server/ServerMain.cpp b/MobileGL/MG_Remote/Server/ServerMain.cpp index c79350f5..efa3c31b 100644 --- a/MobileGL/MG_Remote/Server/ServerMain.cpp +++ b/MobileGL/MG_Remote/Server/ServerMain.cpp @@ -43,7 +43,9 @@ #include "ServerSession.h" #include "SurfaceControlFrame.h" +#include // MG_Config::Transport - D1 pins it to Spawn in this process #include +#include // MG_ConfigLoader::Init - the child loads its own config (step 1.5) #include #include @@ -147,6 +149,40 @@ extern "C" __attribute__((visibility("default"))) int mobilegl_server_main(int a } WireLogError("MG_Remote server: pid=%d listening on \"%s\"", selfPid, endpoint.c_str()); + // ---- 1.5. CONFIG, AND THEN D1. + // + // TWO SEPARATE DEFECTS LIVED HERE, and both were silent. + // + // (a) NOTHING LOADED THE CONFIG AT ALL. mobilegl_server_main calls + // InitServerRoleForSpawn directly rather than MobileGL::Initialize, and + // MG_ConfigLoader::Init is a step of the latter - so every MG_Config + // value in this process was its STATIC DEFAULT. MOBILEGL_BACKEND_TYPE + // survives the launcher's scrub on purpose, and it was being read by + // nobody: a DirectVulkan session would have brought up a DirectGLES + // server and then disagreed with its client about which backend the + // capability mask describes. + // + // (b) THE CHILD RESOLVED Transport == Monolith. CONTRACT-P6 §3.1 (D1) says + // it must read as Spawn, and a6 measured the cost of getting it wrong: + // 226 live lines test `Transport != Monolith` to select the SERVER arm, + // 148 of them in MG_Backend. Monolith flips all of them to frontend glue + // in the one process that has no frontend. Observed, exactly as + // predicted: TextureImpl::UnitTexturesByHandle() answered false, Clear + // took the legacy arm that dereferences borrowed FRONTEND slot pointers, + // and the apply thread took a SIGSEGV inside SyncNeccessaryTextures on + // the first Clear record of the OpenRA retrace. + // + // IT IS SET HERE AND NOT THROUGH THE ENVIRONMENT because the environment is + // where it must NOT appear: LooksLikeAClientEnvironment above exits 65 if + // MOBILEGL_TRANSPORT survived into this process, and that check is + // anti-recursion catch (b). D1's own answer is that Dial - not Transport - + // 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; + 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)); + // ---- 2. the backend, through the SAME entry point the inproc server role // uses (MG_Backend/ServerRole.h). No EGL yet: the context is created on the // apply thread when the client's first eglMakeCurrent crosses. @@ -188,8 +224,21 @@ extern "C" __attribute__((visibility("default"))) int mobilegl_server_main(int a std::fflush(nullptr); ::_exit(74); } - SocketDoorbell selfBell(serverBell[0], /*code=*/1, /*ownsFd=*/true); - SocketDoorbell peerBell(clientBell[0], /*code=*/1, /*ownsFd=*/true); + // PARK AND NOTIFY ON DIFFERENT DESCRIPTORS, and this is not a refinement - + // the single-fd form CANNOT WAKE ITSELF. send() on one end of a socketpair + // is delivered to the OTHER end, so a one-fd bell built on serverBell[0] + // writes to serverBell[1] and polls serverBell[0]: correct for waking a + // PEER, useless for waking a thread in this process. The server needs + // exactly that, because the control pump posts into the apply thread's + // mailbox and then has to ring the bell that thread is parked on. Measured + // symptom when it was wrong: every eglInitialize crossed, landed in the + // mailbox, and timed out at 5 s because nothing ever woke mgl-srv-apply. + SocketDoorbell selfBell(/*parkFd=*/serverBell[0], /*notifyFd=*/serverBell[1], /*code=*/1, + /*ownsFds=*/true); + // RINGS ONLY. The client parks on clientBell[0]; writing clientBell[1] is + // what reaches it. parkFd = -1 says in the type that this side never waits. + SocketDoorbell peerBell(/*parkFd=*/-1, /*notifyFd=*/clientBell[1], /*code=*/1, + /*ownsFds=*/true); Server::ServerSession& session = Server::ServerSessionInstance(); session.SetExternalDoorbells(&selfBell, &peerBell); @@ -237,10 +286,15 @@ extern "C" __attribute__((visibility("default"))) int mobilegl_server_main(int a ::_exit(69); } } - // Slots 4 and 5 are the bells. The client gets the OTHER end of each pair: - // it writes to the server's bell and polls its own. - const int bellFds[2] = {serverBell[1], clientBell[1]}; - for (std::uint32_t index = 0; index < 2; ++index) { + // SLOTS 4, 5 AND 6 ARE THE BELLS - three, not two, and the third is the + // one the two-fd form above forces: + // 4 serverBell[1] the client's handle to RING THE SERVER + // 5 clientBell[0] the client's PARK end + // 6 clientBell[1] the client's handle to ring ITSELF, for the same + // reason the server needs serverBell[1] + // Sharing DUPs, so the descriptors these bells own stay valid here. + const int bellFds[3] = {serverBell[1], clientBell[0], clientBell[1]}; + for (std::uint32_t index = 0; index < 3; ++index) { Sideband sideband{4 + index, 0}; if (control->ShareFd(bellFds[index], MobileGLByteSpan{&sideband, sizeof(sideband)}) != MOBILEGL_OK) { @@ -250,8 +304,9 @@ extern "C" __attribute__((visibility("default"))) int mobilegl_server_main(int a ::_exit(69); } } - ::close(serverBell[1]); - ::close(clientBell[1]); + // clientBell[0] is the CLIENT's park end and no bell here owns it; the + // other three belong to selfBell and peerBell and are closed with them. + ::close(clientBell[0]); } // ---- 7. the apply thread. It holds the native context for its whole life, @@ -264,7 +319,7 @@ extern "C" __attribute__((visibility("default"))) int mobilegl_server_main(int a } WireLogError("MG_Remote server: pid=%d transport=spawn role=server ready " - "(backend up, six descriptors handed over, apply thread running)", + "(backend up, seven descriptors handed over, apply thread running)", selfPid); // ---- 8. the control pump. EOF is the whole exit condition: no timeout may diff --git a/MobileGL/MG_Remote/Server/SurfaceControlFrame.cpp b/MobileGL/MG_Remote/Server/SurfaceControlFrame.cpp index 4bcfd882..01bba314 100644 --- a/MobileGL/MG_Remote/Server/SurfaceControlFrame.cpp +++ b/MobileGL/MG_Remote/Server/SurfaceControlFrame.cpp @@ -23,7 +23,7 @@ namespace MobileGL::MG_Remote::Server { case SurfaceControlOp::SetSwapInterval: return "SetSwapInterval"; case SurfaceControlOp::ReleaseResources: return "ReleaseResources"; case SurfaceControlOp::SetWindowHandle: return "SetWindowHandle"; - case SurfaceControlOp::InitCapabilitiesInprocOnly: return "InitCapabilitiesInprocOnly"; + case SurfaceControlOp::InitCapabilities: return "InitCapabilities"; case SurfaceControlOp::SwapBuffersInprocOnly: return "SwapBuffersInprocOnly"; case SurfaceControlOp::InitWindowSurfaceInprocOnly: return "InitWindowSurfaceInprocOnly"; case SurfaceControlOp::ProbeForTesting: return "ProbeForTesting"; @@ -43,6 +43,10 @@ namespace MobileGL::MG_Remote::Server { case SurfaceControlOp::SetSwapInterval: case SurfaceControlOp::ReleaseResources: case SurfaceControlOp::SetWindowHandle: + // cp: the request half of InitCapabilities. Its ANSWER is a CapsSnapshot + // frame, which is why this row was missing; the request still has to + // cross, because under spawn the apply thread is in another process. + case SurfaceControlOp::InitCapabilities: return true; default: return false; diff --git a/MobileGL/MG_Remote/Server/SurfaceControlFrame.h b/MobileGL/MG_Remote/Server/SurfaceControlFrame.h index 7aef9b82..42d33d9e 100644 --- a/MobileGL/MG_Remote/Server/SurfaceControlFrame.h +++ b/MobileGL/MG_Remote/Server/SurfaceControlFrame.h @@ -55,12 +55,18 @@ namespace MobileGL::MG_Remote::Server { SetSwapInterval = 8, ReleaseResources = 9, SetWindowHandle = 10, + // P6 (cp) MOVED THIS ONE ONTO THE WIRE, and the old name was a lie the + // moment spawn existed. f0-egl reasoned that the wire's ANSWER to + // InitCapabilities is the CapsSnapshot frame, which is true and beside + // the point: the REQUEST still has to reach the apply thread, and under + // spawn that thread is in another process. Measured before the fix - + // every spawned eglMakeCurrent died at "surface op kind 11 cannot cross + // the wire (InprocOnlyOpOnTheWire)", one call after the backend came up. + InitCapabilities = 11, // INPROC-ONLY kinds: legal inside this process's frame channel, NEVER encodable onto the // wire (SurfaceOpCodec refuses them by name). f0-egl's census found no production caller - // for the two dead forwarders and a CapsSnapshot answer for the third; they ride the same - // frame channel because the function-pointer mailbox is GONE, not because they are wire - // ops. - InitCapabilitiesInprocOnly = 11, // the wire's answer is the CapsSnapshot frame itself + // for the two dead forwarders; they ride the same frame channel because the + // function-pointer mailbox is GONE, not because they are wire ops. SwapBuffersInprocOnly = 12, // present travels as a record (the class-B Present emitter) InitWindowSurfaceInprocOnly = 13,// a client-side no-op; kept for the inproc test lane ProbeForTesting = 14, // MG_Test's arbitrary-work seam through the same channel diff --git a/MobileGL/MG_Remote/Transport/Doorbell.cpp b/MobileGL/MG_Remote/Transport/Doorbell.cpp index 8ebb36dd..606f7f29 100644 --- a/MobileGL/MG_Remote/Transport/Doorbell.cpp +++ b/MobileGL/MG_Remote/Transport/Doorbell.cpp @@ -212,7 +212,7 @@ namespace MobileGL::MG_Remote::Transport { // ----------------------------------------------------------------------- SocketDoorbell::SocketDoorbell(int fd, std::uint8_t code, bool ownsFd) - : m_fd(fd), m_code(code), m_ownsFd(ownsFd) { + : m_fd(fd), m_notifyFd(fd), m_code(code), m_ownsFd(ownsFd) { #if defined(SO_NOSIGPIPE) // The per-socket form of MSG_NOSIGNAL, on the platforms that lack the per-call one: // a Notify to a hung-up peer must come back as EPIPE, not as a fatal signal. @@ -223,19 +223,44 @@ namespace MobileGL::MG_Remote::Transport { #endif } + // The two-fd form: park on one descriptor, ring another. See the header for + // why the server needs it and the cross-process form cannot provide it. + SocketDoorbell::SocketDoorbell(int parkFd, int notifyFd, std::uint8_t code, bool ownsFds) + : m_fd(parkFd), m_notifyFd(notifyFd), m_code(code), m_ownsFd(ownsFds) { +#if defined(SO_NOSIGPIPE) + for (int fd : {m_fd, m_notifyFd}) { + if (fd >= 0) { + const int one = 1; + (void)::setsockopt(fd, SOL_SOCKET, SO_NOSIGPIPE, &one, sizeof(one)); + } + } +#endif + } + SocketDoorbell::~SocketDoorbell() { - if (m_ownsFd && m_fd >= 0) { + if (!m_ownsFd) { + return; + } + if (m_fd >= 0) { ::close(m_fd); } + // Only when they are different: the single-fd form has m_notifyFd == m_fd + // and closing it twice is a double close, which on a busy process closes + // somebody else's descriptor rather than failing. + if (m_notifyFd >= 0 && m_notifyFd != m_fd) { + ::close(m_notifyFd); + } } void SocketDoorbell::Notify() { - if (m_fd < 0) { + // The NOTIFY descriptor, not the park one: a ring-only bell has no park + // fd at all and must still be able to ring. + if (m_notifyFd < 0) { return; } const std::uint8_t byte = m_code; for (;;) { - const ssize_t written = ::send(m_fd, &byte, 1, MSG_DONTWAIT | MSG_NOSIGNAL); + const ssize_t written = ::send(m_notifyFd, &byte, 1, MSG_DONTWAIT | MSG_NOSIGNAL); if (written == 1) { return; } @@ -259,6 +284,11 @@ namespace MobileGL::MG_Remote::Transport { } bool SocketDoorbell::Park(std::uint32_t timeoutMs) { + // A ring-only bell was never a waiter. Saying so immediately is the + // honest answer; blocking forever on -1 would be a hang with no cause. + if (m_fd < 0) { + return false; + } if (m_fd < 0 || m_dead) { return false; } diff --git a/MobileGL/MG_Remote/Transport/Doorbell.h b/MobileGL/MG_Remote/Transport/Doorbell.h index 1f31552b..43f1dc1b 100644 --- a/MobileGL/MG_Remote/Transport/Doorbell.h +++ b/MobileGL/MG_Remote/Transport/Doorbell.h @@ -397,6 +397,23 @@ namespace MobileGL::MG_Remote::Transport { // When `ownsFd` the descriptor is closed with this object. `code` is // the byte written by Notify. SocketDoorbell(int fd, std::uint8_t code, bool ownsFd); + + // P6: PARK AND NOTIFY ON DIFFERENT DESCRIPTORS. + // + // The single-fd form above is the cross-process one: send() on this end + // is delivered to the PEER's end, so the peer's own SocketDoorbell is + // what receives it. That is correct, and it is also why the single-fd + // form cannot wake ITSELF - and the server needs exactly that, because + // its control pump posts into the apply thread's mailbox and must ring + // the bell that thread is parked on. Under inproc CondVarDoorbell has no + // such split: notify and wait are the same object. + // + // So: `parkFd` is polled, `notifyFd` is written. For a socketpair where + // this side holds both ends they are [0] and [1], and a peer that must + // also be able to ring it gets a DUP of [1]. `parkFd` may be -1 for a + // bell this side only ever RINGS - Park then returns immediately, which + // is the honest answer for an object that was never a waiter. + SocketDoorbell(int parkFd, int notifyFd, std::uint8_t code, bool ownsFds); ~SocketDoorbell() override; void Notify() override; @@ -413,7 +430,8 @@ namespace MobileGL::MG_Remote::Transport { // afterwards. std::uint64_t Drain(); - int m_fd; + int m_fd; // polled + int m_notifyFd; // written; equals m_fd in the single-fd (cross-process) form std::uint8_t m_code; bool m_ownsFd; bool m_dead = false; diff --git a/MobileGL/MG_Test/Wire/SurfaceControlFrameTest.cpp b/MobileGL/MG_Test/Wire/SurfaceControlFrameTest.cpp index 5f935d5c..528d171c 100644 --- a/MobileGL/MG_Test/Wire/SurfaceControlFrameTest.cpp +++ b/MobileGL/MG_Test/Wire/SurfaceControlFrameTest.cpp @@ -137,8 +137,11 @@ TEST(SurfaceControlFrameTest, ExactlyTheTenFramedOpsHaveWireKindsAndTheValuesAre EXPECT_EQ(static_cast(::MobileGL::Wire::SurfaceOpKind::SetWindowHandle), 10u); // The inproc-only kinds refuse the wire by name, both directions. + // InitCapabilities WAS in this list and cp took it out; the eleventh op has a + // test of its own below rather than a line here, because this test's name is + // a statement about ten and G14 does not let a name be traded for another. const SurfaceControlOp inprocOnly[] = { - SurfaceControlOp::None, SurfaceControlOp::InitCapabilitiesInprocOnly, + SurfaceControlOp::None, SurfaceControlOp::SwapBuffersInprocOnly, SurfaceControlOp::InitWindowSurfaceInprocOnly, SurfaceControlOp::ProbeForTesting, }; @@ -153,6 +156,37 @@ TEST(SurfaceControlFrameTest, ExactlyTheTenFramedOpsHaveWireKindsAndTheValuesAre << "a wire kind the schema does not define must not decode into an op"; } +TEST(SurfaceControlFrameTest, TheEleventhFramedOpIsInitCapabilitiesAndItCrossesTheWire) { + // cp. The op above this one in the enum, SetWindowHandle, was fc's last + // append; this is P6's. It gets a test rather than a row in the ten-op table + // because the failure it guards is specific and was REAL: with no wire kind + // the codec answered InprocOnlyOpOnTheWire, BackendObject_Remote's + // InitCapabilities call failed, MakeEGLCurrent reported "InitCapabilities + // failed", and the OpenRA spawn retrace died one call after the server's + // backend had come up green. Nothing about that read as a missing enum row. + ::MobileGL::Wire::SurfaceOpKind kind; + ASSERT_TRUE(WireKindForSurfaceControlOp(SurfaceControlOp::InitCapabilities, &kind)) + << "InitCapabilities lost its wire kind; a spawn client cannot ask for caps"; + EXPECT_EQ(static_cast(kind), 11u) << "wire ABI: append-only, and 11 is taken"; + EXPECT_EQ(static_cast(SurfaceControlOp::InitCapabilities), 11u); + + SurfaceControlOp back; + ASSERT_TRUE(SurfaceControlOpForWireKind(kind, &back)) + << "the decode range's upper bound did not move with the append, so the new tag " + "reads as out-of-range - which surfaces as Fatal{ProtocolCorruption}, not as the " + "missing row it is"; + EXPECT_EQ(back, SurfaceControlOp::InitCapabilities); + + // And it really encodes. WireKindForSurfaceControlOp answering yes is not the + // same fact as the encoder accepting the frame: the refusal the client hit + // came out of EncodeSurfaceOpFrame. + flatbuffers::FlatBufferBuilder builder(256); + SurfaceControlFrame frame; + frame.kind = SurfaceControlOp::InitCapabilities; + frame.seq = 7; + EXPECT_EQ(EncodeSurfaceOpFrame(frame, &builder), SurfaceWireError::None); +} + TEST(SurfaceControlFrameTest, WindowBackendAndWireWindowKindMapExplicitlyBothWays) { using MG_Backend::WindowBackend; const struct { @@ -269,8 +303,6 @@ TEST(SurfaceControlFrameTest, MalformedAndInprocOnlyFramesRefuseToEncode) { EXPECT_EQ(EncodeSurfaceOpFrame(frame, &builder), SurfaceWireError::InprocOnlyOpOnTheWire); frame.kind = SurfaceControlOp::SwapBuffersInprocOnly; EXPECT_EQ(EncodeSurfaceOpFrame(frame, &builder), SurfaceWireError::InprocOnlyOpOnTheWire); - frame.kind = SurfaceControlOp::InitCapabilitiesInprocOnly; - EXPECT_EQ(EncodeSurfaceOpFrame(frame, &builder), SurfaceWireError::InprocOnlyOpOnTheWire); frame.kind = SurfaceControlOp::InitWindowSurfaceInprocOnly; EXPECT_EQ(EncodeSurfaceOpFrame(frame, &builder), SurfaceWireError::InprocOnlyOpOnTheWire); diff --git a/MobileGL/MG_Util/Debug/Log.cpp b/MobileGL/MG_Util/Debug/Log.cpp index c0801875..7b90a488 100644 --- a/MobileGL/MG_Util/Debug/Log.cpp +++ b/MobileGL/MG_Util/Debug/Log.cpp @@ -8,6 +8,8 @@ #include "../../Includes.h" +#include // std::strcmp, for the spawned-server role check in InitFile + namespace MobileGL { namespace MG_Util::Debug { static FILE* s_logFile = nullptr; @@ -70,7 +72,32 @@ namespace MobileGL { logPath = MOBILEGL_LOG_FILE_PATH; } if (logPath && *logPath) { - s_logFile = std::fopen(logPath, "w"); + // "a", NOT "w", WHENEVER A SECOND PROCESS SHARES THIS PATH. + // + // Under transport=spawn the server is a separate process + // writing the same MOBILEGL_LOG_FILE_PATH, and two "w" + // handles each carry their OWN file offset: both start at 0 + // and overwrite each other's bytes. The result is not + // interleaved, it is CORRUPT - observed as half-lines like + // `ote client: CapsMirror generation 1 adopted` while + // diagnosing the spawn retrace, and as the server's whole + // startup block vanishing under the client's later writes. + // O_APPEND makes each write atomic at the end of the file, + // which is what makes the log a usable census across both + // processes - and run_trace_case.cmake's Fatal{ count reads + // exactly this file. + // + // THE CLIENT STILL TRUNCATES ONCE, so a fresh run does not + // read as a growing pile of old ones. The server never does: + // it is always started by a client that has already opened + // this path, and a truncate from the far side would delete + // the very startup lines a failed session needs. + const char* role = std::getenv("MOBILEGL_IPC_ROLE"); + const bool isSpawnedServer = role != nullptr && std::strcmp(role, "server") == 0; + if (!isSpawnedServer) { + if (FILE* truncate = std::fopen(logPath, "w")) std::fclose(truncate); + } + s_logFile = std::fopen(logPath, "a"); } } #endif diff --git a/tools/trace_replay/CMakeLists.txt b/tools/trace_replay/CMakeLists.txt index 81229cb2..d5a6fae0 100644 --- a/tools/trace_replay/CMakeLists.txt +++ b/tools/trace_replay/CMakeLists.txt @@ -465,6 +465,22 @@ function(add_trace_replay_split_test CASE_NAME) endif() add_trace_replay_test(${CASE_NAME} DirectGLES VARIANT SPLIT TRANSPORT inproc IPC_SERVER_PATH "${CMAKE_BINARY_DIR}/libMobileGLServer.so" ${ARGN}) + + # --- P6: the SPAWN arm, beside the inproc one rather than instead of it ------------------ + # + # TWO ARMS, NOT ONE RENAMED. The inproc arm is the CONTROL: it is the same + # library, the same codec, the same applier and the same picture, with the + # only difference being which process the server role runs in. A spawn arm + # that had replaced it would have left "did the transport break it?" and + # "did the phase break it?" answerable only together. + # + # G14 is satisfied by construction - a name is added, none is removed - and + # the VARIANT gives it its own output and artifact directories, which is the + # half the comment above says is easy to forget: both arms write + # output/mobilegl.log, and that log is the ONLY valid refusal census because + # the console sink is compiled out of these configurations. + add_trace_replay_test(${CASE_NAME} DirectGLES VARIANT SPAWN TRANSPORT spawn + IPC_SERVER_PATH "${CMAKE_BINARY_DIR}/libMobileGLServer.so" ${ARGN}) endfunction() set(MOBILEGL_TRACE_CASES_CMAKE "${CMAKE_CURRENT_BINARY_DIR}/trace_cases.cmake") diff --git a/tools/trace_replay/run_trace_case.cmake b/tools/trace_replay/run_trace_case.cmake index f684c8e3..358de6cd 100644 --- a/tools/trace_replay/run_trace_case.cmake +++ b/tools/trace_replay/run_trace_case.cmake @@ -262,7 +262,34 @@ if(DEFINED ENV{MOBILEGL_TRANSPORT} AND NOT "$ENV{MOBILEGL_TRANSPORT}" STREQUAL " # DEBUG to debug a split failure. The sentence below exists only in # ConfigLoader::InitTransport's InProcess arm, which exists only under # MOBILEGL_BUILD_DISAGGREGATED. - set(split_expected_marker "MOBILEGL_TRANSPORT=inproc - the MGPipe record stream") + # + # ONE ROW PER TRANSPORT THIS RUNNER ADMITS. A transport with no row is a + # FATAL below rather than a fall-through, because the fall-through is the + # exact accident this whole block exists to prevent: an arm that named a + # transport nobody asserted on and went green having proved nothing. + # + # THE SPAWN ROW CARRIES A SECOND SENTENCE, and it is the stronger half. + # The ConfigLoader marker proves the VALUE RESOLVED - it is a statement + # about a parser. `spawn ARMED - the server role runs in pid N` is + # written by ClientSession::StartSpawned only after the connect and the + # handshake succeeded, and it carries a pid that is not this process's. + # inproc resolves its own marker while running the server role on a + # thread HERE; nothing on that path can write this one. + set(split_expected_second "") + if("$ENV{MOBILEGL_TRANSPORT}" STREQUAL "inproc") + set(split_expected_marker "MOBILEGL_TRANSPORT=inproc - the MGPipe record stream") + elseif("$ENV{MOBILEGL_TRANSPORT}" STREQUAL "spawn") + set(split_expected_marker "MOBILEGL_TRANSPORT=spawn - the MGPipe record stream") + set(split_expected_second "spawn ARMED - the server role runs in pid ") + else() + message(FATAL_ERROR + "MOBILEGL_TRANSPORT=$ENV{MOBILEGL_TRANSPORT} is set for ${split_case} and this " + "runner has no marker for it, so the run could not be checked. ConfigLoader " + "recognises monolith|inproc|spawn|unix:|pipe: and REFUSES the last " + "two BY NAME, staying on monolith - a run that asked for one of those would " + "have replayed monolith. Add a row above when the transport lands; do not let " + "an unasserted transport reach a green.") + endif() string(FIND "${split_log}" "${split_expected_marker}" split_armed_at) if(split_armed_at EQUAL -1) # Say which transport was actually asked for. ConfigLoader REFUSES spawn / unix: / @@ -270,11 +297,14 @@ if(DEFINED ENV{MOBILEGL_TRANSPORT} AND NOT "$ENV{MOBILEGL_TRANSPORT}" STREQUAL " # has a different diagnosis from one that set inproc against a monolith library, and # the old message named `inproc` either way. set(split_refusal "") - if(NOT "$ENV{MOBILEGL_TRANSPORT}" STREQUAL "inproc") + if("$ENV{MOBILEGL_TRANSPORT}" STREQUAL "spawn") set(split_refusal - " NOTE: this run asked for '$ENV{MOBILEGL_TRANSPORT}', which P5 does not " - "implement - ConfigLoader recognises spawn / unix: / pipe: and REFUSES them by " - "name, staying on monolith. Only 'inproc' can resolve in P5.") + " NOTE: this run asked for 'spawn'. The server is a SEPARATE PROCESS here, so " + "the two ways to get this far with no marker are a library that has no spawn " + "arm (pre-P6) and a launcher that never resolved the transport at all. " + "MOBILEGL_IPC_SERVER_PATH must point at a libMobileGLServer.so built from this " + "same tree; LaunchServer REFUSES an unresolvable image BY NAME and never falls " + "back to running the server role locally.") endif() message(FATAL_ERROR "MOBILEGL_TRANSPORT=$ENV{MOBILEGL_TRANSPORT} is set for ${split_case} and the library " @@ -288,6 +318,23 @@ if(DEFINED ENV{MOBILEGL_TRANSPORT} AND NOT "$ENV{MOBILEGL_TRANSPORT}" STREQUAL " "admits INFO (at WARN or above the line is compiled out and this reds for no " "defect).${split_refusal}") endif() + # ... and the half that a same-process run cannot satisfy. Checked + # SEPARATELY from the marker above so the two failures read differently: + # no marker means the transport never resolved, while marker-but-no-pid + # means it resolved and the session never came up across the boundary. + if(NOT "${split_expected_second}" STREQUAL "") + string(FIND "${split_log}" "${split_expected_second}" split_second_at) + if(split_second_at EQUAL -1) + message(FATAL_ERROR + "${split_case}: ${mobilegl_log} reports MOBILEGL_TRANSPORT=" + "$ENV{MOBILEGL_TRANSPORT} resolved, but carries no " + "\"${split_expected_second}\". That sentence is written by " + "ClientSession::StartSpawned AFTER the launch, the connect and the handshake " + "all succeeded, and the pid in it is the SERVER's - it is the only evidence " + "in this log that the server role left this process. Without it the run " + "resolved a transport and then replayed something else.") + endif() + endif() # The refusal census. Recorded on every split run, pass or fail. file(STRINGS "${mobilegl_log}" split_fatals REGEX "Fatal\\{") list(LENGTH split_fatals split_fatal_count)