diff --git a/MobileGL/MG_Impl/Pipe/PipeFill.cpp b/MobileGL/MG_Impl/Pipe/PipeFill.cpp index 07f1cea7..e3ec15db 100644 --- a/MobileGL/MG_Impl/Pipe/PipeFill.cpp +++ b/MobileGL/MG_Impl/Pipe/PipeFill.cpp @@ -52,9 +52,9 @@ // P5c gt (CONTRACT-P5C §6 layer 2, audit A1): the client-side gPipeInputs check consults // InBarrierWait() and ApplyThreadIsInsideApplier() - both live here. #include -// P5c ev (CONTRACT-P5C §4.2): RecordError's transport arm posts kEventGlError through the -// server session, and InvalidateCompileEnv's forward is deleted with an active transport. -#include +// P5f fv: backend errors use the same owned reverse callback table as other events. +// The producer, rather than this client-side translation unit, knows the server session. +#include #endif #include @@ -2182,7 +2182,6 @@ namespace MobileGL::MG_Pipe { } void PipeInputs::RecordError(ErrorCode code, UniquePtr info) { - MGP_STICKY_FORWARD_PULL(RecordError); #if MOBILEGL_BUILD_DISAGGREGATED if (MG_Config::Transport != MG_Config::TransportMode::Monolith) { // The error queue is CLIENT state and the apply thread may not write it (R4). @@ -2192,11 +2191,16 @@ namespace MobileGL::MG_Pipe { // program order of error-then-read but not cross-verb interleaving - the // accepted P5c shape, stated in the contract rather than discovered in P6. const String message = info != nullptr ? info->toString() : String{}; - MG_Remote::Server::ServerSessionInstance().PostGlError(static_cast(code), - message.c_str()); + if (gMGPipeCallbacks.OnGlError == nullptr) { + MGLOG_F("MGPipe: Fatal{RoleViolation, \"OnGlError.callback-missing\"} - " + "a transport backend error has no reverse-channel owner"); + std::abort(); + } + gMGPipeCallbacks.OnGlError(static_cast(code), message.c_str()); return; } #endif + MGP_STICKY_FORWARD_PULL(RecordError); auto* ctx = LiveContext(); if (ctx == nullptr) { MGLOG_E_ONCE("PipeInputs::RecordError: no live context, dropping error %d", static_cast(code)); diff --git a/MobileGL/MG_Impl/Pipe/ResourceTracker.h b/MobileGL/MG_Impl/Pipe/ResourceTracker.h index d6870ea1..28ca4305 100644 --- a/MobileGL/MG_Impl/Pipe/ResourceTracker.h +++ b/MobileGL/MG_Impl/Pipe/ResourceTracker.h @@ -693,42 +693,26 @@ namespace MobileGL::MG_Pipe { install(gMGPipeCallbacks.OnGpuWritten, &MGPipeClientOnGpuWritten, "OnGpuWritten"); } - // The GPU-write announcement for a buffer the backend holds only a FRONTEND POINTER to - - // Magma's barrier-pulled binding-point reads (UniformManager / VulkanRenderer), whose - // direct bufferObject->MarkGpuWritten() was the apply thread poking client memory (R2). - // Routed through the reverse channel exactly as DirectGLES' MarkBufferGpuWritten routes - // its own sites: ONE whole-buffer range, stated rather than implied (zero ranges is the - // shape a fully narrowed announcement will legitimately have at P8/P9, and the two must - // not be the same record). The handle comes from the client allocator's lifetime-id - // probe - the mint is unconditional at the BufferObject constructor - // (MGPipeMintResourceHandle), so a live buffer always has one. - // - // The fallback is the monolith shape, preserved byte for byte: no reverse channel - // installed (a pull-arm build, or a process with no session) pokes the object directly, - // which is exactly what these sites did before. A MISSING HANDLE with a live channel is - // loud rather than a silent drop, the same choice MarkBufferGpuWritten makes. + // Legacy-object entry for the monolith backend. Transport consumers carry handles + // and call OnGpuWritten directly; P5f fm rejects Magma's P7 buffer consumers before + // reaching here. Keep a named guard at this boundary so a future caller cannot turn + // a missing callback/handle into a write through a client object. inline void MGPipeAnnounceBufferGpuWritten( const SharedPtr& bufferObject) { if (bufferObject == nullptr) return; +#if MOBILEGL_BUILD_DISAGGREGATED + if (MG_Config::Transport != MG_Config::TransportMode::Monolith) { + MGLOG_F("MGPipe: Fatal{RoleViolation, \"gpu-written-legacy-object\"} - " + "a transport producer must announce a record handle, never a client BufferObject"); + std::abort(); + } +#endif if (gMGPipeCallbacks.OnGpuWritten == nullptr) { bufferObject->MarkGpuWritten(); return; } - const MGPipeHandle res = [&]() { -#if MOBILEGL_BUILD_DISAGGREGATED - // CONTRACT-P5C §3.1's named exemption, Magma's half: the binding records that - // would carry this buffer's handle are sb's to emit and Magma's server-side - // binding table is P7's, so until then the probe runs inside the scope - the - // debt's named, greppable form rather than a silent guard removal. - // - // P5e (id), ruling 12: the FOURTH of Magma's apply-thread allocator debts and the - // last user of the scope P5e renamed. Espryt never reaches this arm under a - // transport - MarkBufferGpuWrittenByHandle carries the handle from the record side - // (sb/fb) - so the DirectVulkan key on the exemption costs it nothing. - const MagmaP7AllocatorDebtScope magmaP7AllocatorDebt; -#endif - return MGPipeSlots().FindByLifetimeId(MGPipeKind::Buffer, bufferObject->GetLifetimeId()); - }(); + const MGPipeHandle res = + MGPipeSlots().FindByLifetimeId(MGPipeKind::Buffer, bufferObject->GetLifetimeId()); if (MGPipeHandleIsNull(res)) { MGLOG_E_ONCE("MGPipe: no handle for the GPU-write announcement of buffer %u - the " "reverse channel is installed but the mint is missing, so the mark " diff --git a/MobileGL/MG_Pipe/MGPipeCallbacks.h b/MobileGL/MG_Pipe/MGPipeCallbacks.h index f379acea..0ebe2db4 100644 --- a/MobileGL/MG_Pipe/MGPipeCallbacks.h +++ b/MobileGL/MG_Pipe/MGPipeCallbacks.h @@ -25,8 +25,9 @@ // an optimization (section 7.4). namespace MobileGL::MG_Pipe { struct MGPipeCallbacks { - // A driver-detected GL error that only the server could have seen. - void (*OnGlError)(Uint32 code); + // A driver-detected GL error that only the server could have seen. The text is + // borrowed for this synchronous call; a transport producer copies it before return. + void (*OnGlError)(Uint32 code, const char* message); // Ranges of a resource the GPU wrote; retires MarkGpuWritten. void (*OnGpuWritten)(MGPipeHandle res, Uint rangeCount, const MGPRange* ranges); void (*OnBufferWriteback)(MGPipeHandle res, Uint64 offset, MGPBlobRef bytes); diff --git a/MobileGL/MG_Remote/Server/ServerSession.cpp b/MobileGL/MG_Remote/Server/ServerSession.cpp index f8734b58..e1a66c9d 100644 --- a/MobileGL/MG_Remote/Server/ServerSession.cpp +++ b/MobileGL/MG_Remote/Server/ServerSession.cpp @@ -159,7 +159,7 @@ namespace MobileGL::MG_Remote::Server { // ---- P5c ev: the reverse channel's PRODUCER callbacks (CONTRACT-P5C §4.1) -------- // - // With an active transport the three reverse entries of gMGPipeCallbacks belong to + // With an active transport the four reverse entries of gMGPipeCallbacks belong to // the SERVER session, never to the client: the backend calls them with exactly the // arguments it always passed (a writeback blobref whose Offset IS the backend's own // mapped pointer, a range array, a surface info), and what the callback does with @@ -329,6 +329,17 @@ namespace MobileGL::MG_Remote::Server { session.PublishEvents(); } + // P5f fv: OnGlError now has the same owner and lifetime as the other producers. + // PostGlError copies message bytes synchronously into the existing FIFO event ring. + void ServerOnGlError(Uint32 code, const char* message) { + auto* session = ServerSession::Active(); + if (session == nullptr) { + MGLOG_F("MGPipe: Fatal{RoleViolation, \"OnGlError.session-missing\"}"); + std::abort(); + } + session->PostGlError(code, message); + } + // One installer for both roles' tables, so the check exists in exactly one spelling: // writing over an entry somebody else claimed is Fatal{RoleViolation, // "callback-double-install"} - MGPipeCallbacks' "never over an entry a backend @@ -339,7 +350,7 @@ namespace MobileGL::MG_Remote::Server { if (entry != nullptr && entry != producer) { MGLOG_F("MGPipe: Fatal{RoleViolation, \"callback-double-install\"} - a reverse " "MGPipeCallbacks entry is already claimed by a different function. With " - "an active transport the three reverse entries are the server " + "an active transport the four reverse entries are the server " "session's producers; the client installs them under monolith only"); std::abort(); } @@ -529,12 +540,13 @@ namespace MobileGL::MG_Remote::Server { m_accepted = true; g_active = this; - // ---- P5c ev: the three reverse-channel producers are THIS session's (CONTRACT-P5C + // ---- P5c ev: the four reverse-channel producers are THIS session's (CONTRACT-P5C // §4.1), installed before the apply thread can apply a record that produces one and // uninstalled at Close after the join. Under monolith these entries are the // client's (MGPipeInstallClientResourceCallbacks, monolith-only now); a session is // by definition not monolith, so finding one of them claimed here is the double // installation the check exists to name. + InstallReverseCallback(MG_Pipe::gMGPipeCallbacks.OnGlError, &ServerOnGlError); InstallReverseCallback(MG_Pipe::gMGPipeCallbacks.OnBufferWriteback, &ServerOnBufferWriteback); InstallReverseCallback(MG_Pipe::gMGPipeCallbacks.OnGpuWritten, &ServerOnGpuWritten); @@ -621,6 +633,9 @@ namespace MobileGL::MG_Remote::Server { Wire::SegmentTable::UninstallProcessResolver(); // The reverse-channel producers go with the session that owns them - release // only the entries that are still OURS, never one a later owner installed. + if (MG_Pipe::gMGPipeCallbacks.OnGlError == &ServerOnGlError) { + MG_Pipe::gMGPipeCallbacks.OnGlError = nullptr; + } if (MG_Pipe::gMGPipeCallbacks.OnBufferWriteback == &ServerOnBufferWriteback) { MG_Pipe::gMGPipeCallbacks.OnBufferWriteback = nullptr; } diff --git a/MobileGL/MG_Test/Wire/ServerLoopTest.cpp b/MobileGL/MG_Test/Wire/ServerLoopTest.cpp index 4d062139..bdcd0e24 100644 --- a/MobileGL/MG_Test/Wire/ServerLoopTest.cpp +++ b/MobileGL/MG_Test/Wire/ServerLoopTest.cpp @@ -37,7 +37,9 @@ #include #include #include +#include #include +#include #include #include #include @@ -54,6 +56,7 @@ #include #include #include +#include #include #include @@ -2262,6 +2265,195 @@ TEST(ServerLoopTest, AnIndirectDrawRecordReachesTheSinkWithItsBlockIntact) { fixture.Stop(); } +namespace { + Uint32 g_fvErrorCode = 0; + String g_fvErrorMessage; + Uint32 g_fvErrorCalls = 0; + + void CaptureFvError(Uint32 code, const char* message) { + g_fvErrorCode = code; + g_fvErrorMessage = message ? message : ""; + ++g_fvErrorCalls; + } + + struct FvGlobals { + MG_Pipe::MGPipeCallbacks callbacks = MG_Pipe::gMGPipeCallbacks; + Bool strict = MG_Config::Ipc.StrictErrors; + Bool split = MG_Config::Ipc.RoleSplitState; + ~FvGlobals() { + MG_Pipe::MGPipeServerClearVerbBoundary(); + MG_Pipe::gMGPipeCallbacks = callbacks; + MG_Config::Ipc.StrictErrors = strict; + MG_Config::Ipc.RoleSplitState = split; + } + }; + + struct FvSession : ServerFixture { + ~FvSession() { Stop(); } + }; +} + +TEST(P5fReverseChannel, GlErrorsUseTheOwnedCallbackWithoutAResidualPull) { + FvGlobals restore; + MG_Config::Ipc.StrictErrors = true; + MG_Config::Ipc.RoleSplitState = true; + MG_Pipe::gMGPipeCallbacks.OnGlError = &CaptureFvError; + g_fvErrorCalls = 0; + MG_Pipe::MGPipeResetResidualPullCountForTesting(); + MG_Pipe::MGPipeServerStampVerbBoundary(MG_Pipe::MGPipeVerb::Clear); + MG_Pipe::gPipeInputs.RecordError(ErrorCode::InvalidOperation, + MakeUnique("fv", "draw", "driver message")); + EXPECT_EQ(g_fvErrorCalls, 1u); + EXPECT_EQ(g_fvErrorCode, static_cast(ErrorCode::InvalidOperation)); + EXPECT_EQ(g_fvErrorMessage, "[fv] [draw] driver message"); + EXPECT_EQ(MG_Pipe::MGPipeResidualPullCount(), 0u); +} + +TEST(P5fReverseChannel, AcceptCloseAndReacceptOwnTheGlErrorCallback) { + FvGlobals restore; + MG_Pipe::gMGPipeCallbacks.OnGlError = nullptr; + FvSession fixture; + ASSERT_TRUE(fixture.Handshake()); + const auto producer = MG_Pipe::gMGPipeCallbacks.OnGlError; + ASSERT_NE(producer, nullptr); + EXPECT_EQ(fixture.session->Accept(*fixture.serverTransport), MOBILEGL_ERR_INVALID_ARGUMENT); + EXPECT_EQ(MG_Pipe::gMGPipeCallbacks.OnGlError, producer); + fixture.Stop(); + EXPECT_EQ(MG_Pipe::gMGPipeCallbacks.OnGlError, nullptr); + ASSERT_TRUE(fixture.Handshake()); + EXPECT_EQ(MG_Pipe::gMGPipeCallbacks.OnGlError, producer); + fixture.Stop(); + EXPECT_EQ(MG_Pipe::gMGPipeCallbacks.OnGlError, nullptr); +} + +TEST(P5fReverseChannel, CloseDoesNotEraseAnotherGlErrorOwner) { + FvGlobals restore; + MG_Pipe::gMGPipeCallbacks.OnGlError = nullptr; + FvSession fixture; + ASSERT_TRUE(fixture.Handshake()); + MG_Pipe::gMGPipeCallbacks.OnGlError = &CaptureFvError; + fixture.Stop(); + EXPECT_EQ(MG_Pipe::gMGPipeCallbacks.OnGlError, &CaptureFvError); +} + +#if !defined(_WIN32) +TEST(P5fReverseChannel, GlErrorCallbackRejectsDoubleInstallation) { + EXPECT_EXIT({ + MG_Pipe::gMGPipeCallbacks.OnGlError = &CaptureFvError; + FvSession fixture; + (void)fixture.Handshake(); + std::_Exit(9); + }, ::testing::KilledBySignal(SIGABRT), ""); + EXPECT_NE(ReadLog().find("callback-double-install"), std::string::npos); +} + +TEST(P5fReverseChannel, RecordErrorWithoutCallbackIsNamedFatal) { + EXPECT_EXIT({ + MG_Config::Ipc.StrictErrors = true; + MG_Config::Ipc.RoleSplitState = true; + MG_Pipe::gMGPipeCallbacks.OnGlError = nullptr; + MG_Pipe::gPipeInputs.RecordError(ErrorCode::InvalidOperation, nullptr); + std::_Exit(9); + }, ::testing::KilledBySignal(SIGABRT), ""); + EXPECT_NE(ReadLog().find("OnGlError.callback-missing"), std::string::npos); +} + +TEST(P5fReverseChannel, LegacyGpuWrittenNeverReadsAFrontendObjectInTransport) { + for (Bool installed : {false, true}) { + EXPECT_EXIT({ + MG_Pipe::gMGPipeCallbacks.OnGpuWritten = installed ? + +[](MG_Pipe::MGPipeHandle, Uint, const MG_Pipe::MGPRange*) {} : nullptr; + // Deliberately unreadable object: a guard after a lifetime-id probe or + // either MarkGpuWritten fallback would SIGSEGV instead of the named abort. + SharedPtr foreign( + reinterpret_cast(std::uintptr_t{1}), + [](MG_State::GLState::BufferObject*) {}); + MG_Pipe::MGPipeAnnounceBufferGpuWritten(foreign); + std::_Exit(9); + }, ::testing::KilledBySignal(SIGABRT), ""); + } + EXPECT_NE(ReadLog().find("gpu-written-legacy-object"), std::string::npos); +} +#endif + +TEST(P5fReverseChannel, GlErrorMessagesShareFifoWithOtherReverseEvents) { + FvGlobals restore; + MG_Config::Ipc.StrictErrors = true; + MG_Config::Ipc.RoleSplitState = true; + MG_Pipe::gMGPipeCallbacks.OnGlError = nullptr; + FvSession fixture; + ASSERT_TRUE(fixture.Handshake()); + ASSERT_TRUE(fixture.StartLoop()); + ASSERT_EQ(Server::ServerLoopInstance().RunProbeOnApplyThreadForTesting( + +[](void*) -> MobileGLResult { + MG_Pipe::MGPipeServerStampVerbBoundary(MG_Pipe::MGPipeVerb::Clear); + MG_Pipe::gPipeInputs.RecordError(ErrorCode::InvalidOperation, + MakeUnique("fv", "driver error")); + const MG_Pipe::MGPRange whole{0, MG_Pipe::kMGPipeWholeBuffer}; + MG_Pipe::gMGPipeCallbacks.OnGpuWritten({17, 3}, 1, &whole); + Uint8 bytes[] = {11, 22, 33, 44}; + MG_Pipe::gMGPipeCallbacks.OnBufferWriteback({17, 3}, 24, + MG_Pipe::MGPBlobRef{reinterpret_cast(bytes), sizeof(bytes), + MG_Pipe::kMGHostSpanSegNone, 0}); + std::memset(bytes, 0xDD, sizeof(bytes)); + String longMessage(Transport::kEventGlErrorMaxMessageBytes * 2, 'Q'); + MG_Pipe::gMGPipeCallbacks.OnGlError(static_cast(ErrorCode::InvalidValue), + longMessage.c_str()); + std::fill(longMessage.begin(), longMessage.end(), '?'); + MG_Pipe::gMGPipeCallbacks.OnGlError(static_cast(ErrorCode::InvalidEnum), nullptr); + MG_Pipe::MGPipeServerClearVerbBoundary(); + return MOBILEGL_OK; + }, nullptr), MOBILEGL_OK); + + Transport::EventRingConsumer events(fixture.clientSegments.EventControl(), + fixture.clientSegments.CmdControl(), fixture.clientSegments.EventRingBase(), + fixture.clientSegments.EventRingCapacity(), fixture.clientSegments.EventSegmentBase()); + ASSERT_TRUE(events.Valid()); + const Uint16 kinds[] = {Transport::kEventGlError, Transport::kEventGpuWritten, + Transport::kEventBufferWriteback, Transport::kEventGlError, Transport::kEventGlError}; + for (SizeT i = 0; i < 5; ++i) { + Transport::RingRecordView event; + ASSERT_TRUE(events.Pop(event)) << i; + EXPECT_EQ(event.kind, kinds[i]) << i; + if (event.kind == Transport::kEventGlError) { + const auto* head = static_cast(event.payload); + ASSERT_GE(event.payloadSize, sizeof(*head) + head->MessageBytes); + ASSERT_GT(head->MessageBytes, 0u); + const auto* text = reinterpret_cast(head + 1); + EXPECT_EQ(text[head->MessageBytes - 1], '\0'); + const String message(text, head->MessageBytes - 1); + if (i == 0) { + EXPECT_EQ(head->Code, static_cast(ErrorCode::InvalidOperation)); + EXPECT_EQ(message, "[fv] driver error"); + } else if (i == 3) { + EXPECT_EQ(head->Code, static_cast(ErrorCode::InvalidValue)); + EXPECT_EQ(head->MessageBytes, Transport::kEventGlErrorMaxMessageBytes); + EXPECT_EQ(message, String(Transport::kEventGlErrorMaxMessageBytes - 1, 'Q')); + } else { + EXPECT_EQ(head->Code, static_cast(ErrorCode::InvalidEnum)); + EXPECT_EQ(head->MessageBytes, 1u); + EXPECT_TRUE(message.empty()); + } + } else if (event.kind == Transport::kEventGpuWritten) { + const auto* head = static_cast(event.payload); + EXPECT_EQ(head->Resource.Slot, 17u); + EXPECT_EQ(head->Resource.Gen, 3u); + EXPECT_EQ(head->RangeCount, 1u); + const auto* range = reinterpret_cast(head + 1); + EXPECT_EQ(range->Size, MG_Pipe::kMGPipeWholeBuffer); + } else { + const auto* head = static_cast(event.payload); + EXPECT_EQ(head->Offset, 24u); + EXPECT_EQ(head->Size, 4u); + const Uint8 expected[] = {11, 22, 33, 44}; + EXPECT_EQ(std::memcmp(head + 1, expected, sizeof(expected)), 0); + } + } + Transport::RingRecordView extra; + EXPECT_FALSE(events.Pop(extra)); + events.Drained(); +} + int main(int argc, char** argv) { // Before anything logs: MG_Util::Debug::InitFile() reads the variable once, on the first // write, and caches the FILE*. The name carries this process's pid, because