From 7c97fcfee361a67d8446044504a47819699307b4 Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Sun, 6 Sep 2026 13:03:10 -0400 Subject: [PATCH] [Fix] (Espryt): stop the armless knob pair inside the test that needs an arm, not inside the EGL bring-up a forked pre-flight swallows - Fatal{PipeLegacyMemosDisabled} was raised from InitDisplayAndContext(), i.e. from inside eglMakeCurrent. The integration harness pre-flights that exact sequence in a forked child (MG_IntegrationTest/Harness/HeadlessGL.cpp) and reports a child that dies on a signal as "no usable GPU/display/ICD", so every scenario SKIPPED and ctest called the lane 100% passed while running nothing - on the very pair of env vars the D14/D18 A/B is driven with. ROADMAP.md:7 forbids a gate that cannot go red for the reason it exists. - The arm decision becomes a pure function of the two knobs, ClassifyEsprytSlotArm(), with three verdicts. Bring-up now calls DiagnoseEsprytSlotArm(), which names both knobs at ERROR and RETURNS; the stop stays in ResolveEsprytSlotTablesArm(), which the inline latch reaches at the first twin lookup - a scenario body, where a crash is a test failure. - A process that never looks a twin up never needs an arm and is no longer stopped by one it would not have used. That is the only behaviour this moves. - SanityTest gains an always-on case: the four knob combinations of the pure classifier, that the diagnosis does not stop, and that the stop is SIGABRT whose log line names PipeLegacyMemosDisabled, MOBILEGL_PIPE_PUSH, MOBILEGL_PIPE_LEGACY_MEMOS=0 and the bit - the message and not merely the signal, because "Subprocess aborted" alone tells an operator nothing. It skips visibly in the pull and no-legacy builds (G2 name parity). --- MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp | 19 ++- MobileGL/MG_Backend/DirectGLES/Managers.cpp | 109 ++++++++++++------ MobileGL/MG_Backend/DirectGLES/SlotTables.h | 32 ++++- MobileGL/MG_Test/SanityTest.cpp | 81 +++++++++++++ 4 files changed, 195 insertions(+), 46 deletions(-) diff --git a/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp b/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp index 571252d2..3b72d841 100644 --- a/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp +++ b/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp @@ -10223,12 +10223,19 @@ namespace MobileGL::MG_Backend::DirectGLES { DestroyEGLContext(); #if MOBILEGL_PIPE_PUSH - // Resolve the twin-table arm HERE, at backend startup, rather than leaving it to the - // first twin lookup deep inside the first draw: Fatal{PipeLegacyMemosDisabled} has to - // reach an operator who set MOBILEGL_PIPE_PUSH and MOBILEGL_PIPE_LEGACY_MEMOS into a - // combination that leaves no arm at all, including in a process that goes on to twin - // nothing. The call is idempotent and latched. - (void)EsprytSlotTablesEnabled(); + // DIAGNOSE the twin-table arm here, at backend startup, so an operator who set + // MOBILEGL_PIPE_PUSH and MOBILEGL_PIPE_LEGACY_MEMOS into a combination that leaves no + // arm at all is told so by name, in the log, before the first draw. + // + // Diagnose, and deliberately NOT resolve: resolving raises + // Fatal{PipeLegacyMemosDisabled}, and this function runs inside eglMakeCurrent, which + // the integration harness pre-flights in a FORKED CHILD + // (MG_IntegrationTest/Harness/HeadlessGL.cpp). A child that dies on a signal is reported + // to the parent as "no usable GPU/display/ICD" and every scenario in the lane is + // SKIPPED - so the stop became a green lane that ran nothing, on exactly the two env + // vars the D14/D18 A/B is driven with (ROADMAP.md:7). The stop now belongs to the first + // twin lookup, which happens in a scenario body where a crash IS a test failure. + DiagnoseEsprytSlotArm(); #endif g_Display = g_EGLFuncs.eglGetDisplay(EGL_DEFAULT_DISPLAY); diff --git a/MobileGL/MG_Backend/DirectGLES/Managers.cpp b/MobileGL/MG_Backend/DirectGLES/Managers.cpp index e49a19b7..ca1ca669 100644 --- a/MobileGL/MG_Backend/DirectGLES/Managers.cpp +++ b/MobileGL/MG_Backend/DirectGLES/Managers.cpp @@ -214,49 +214,82 @@ namespace MobileGL::MG_Backend::DirectGLES { }; } // namespace + // The one sentence that decides the arm, written once so that a test can drive every + // combination of the two knobs and so that bring-up and first-use cannot disagree. + EsprytSlotArmVerdict ClassifyEsprytSlotArm(Bool subsystemBitSet, Bool legacyMemosEnabled) { +#if MOBILEGL_PIPE_LEGACY_MEMOS + if (subsystemBitSet) return EsprytSlotArmVerdict::Handles; + return legacyMemosEnabled ? EsprytSlotArmVerdict::Legacy : EsprytSlotArmVerdict::NoArm; +#else + // The legacy arm is not compiled, so the handle arm is the only arm and neither knob + // can produce an armless configuration. + (void)subsystemBitSet; + (void)legacyMemosEnabled; + return EsprytSlotArmVerdict::Handles; +#endif + } + + EsprytSlotArmVerdict CurrentEsprytSlotArmVerdict() { + return ClassifyEsprytSlotArm( + (MG_Config::Features.PipePush & MG_Pipe::kMGPipeSubsystemEsprytSlots) != 0, + MG_Config::Features.PipeLegacyMemos); + } + + void DiagnoseEsprytSlotArm() { + if (CurrentEsprytSlotArmVerdict() != EsprytSlotArmVerdict::NoArm) return; + // Loud, named, and NOT a stop - see the comment on this function in SlotTables.h for + // why a stop raised from inside EGL bring-up is swallowed into a skipped lane. + MGLOG_E("MGPipe: PipeLegacyMemosDisabled - MOBILEGL_PIPE_PUSH leaves " + "kMGPipeSubsystemEsprytSlots (bit 5) clear and MOBILEGL_PIPE_LEGACY_MEMOS=0 " + "makes the legacy twin registry unreachable, so this context has no twin table " + "arm at all; the first twin lookup will stop the process"); + } + Bool ResolveEsprytSlotTablesArm() { // Resolved once and latched by the inline EsprytSlotTablesEnabled() in SlotTables.h: // the two arms of StateBackendObjectRegistry keep their twins in different containers, // so an answer that changed mid-run would strand every twin already built (and, for - // the driver ids those twins own, leak them). InitDisplayAndContext() forces the - // resolution at backend context creation, so the trap below fires before the first - // draw rather than on the first twin lookup - a short-lived process that never twins - // anything used to never learn its knobs left it with no arm at all. - { - const Bool bitSet = - (MG_Config::Features.PipePush & MG_Pipe::kMGPipeSubsystemEsprytSlots) != 0; -#if MOBILEGL_PIPE_LEGACY_MEMOS - if (!bitSet && !MG_Config::Features.PipeLegacyMemos) { - // The operator asked for the handle arm to be OFF and the legacy arm to be - // unreachable at the same time, which leaves no arm at all. This is a Fatal{}, - // and a Fatal{} in this codebase STOPS (MG_Impl/Pipe/PipeFill.cpp's BadKnob and - // its verify trap are both MGLOG_F + abort). Returning here instead would run - // the very arm the operator disabled and hand back a green result measured on - // it - which is exactly the lever HandleRecycleScenario's arms are selected - // with, so a mis-set A/B would be scored silently against the wrong arm - // (ARCHITECTURE.md 9.6). - MGLOG_F("MGPipe: Fatal{PipeLegacyMemosDisabled, \"kMGPipeSubsystemEsprytSlots " - "is clear but MOBILEGL_PIPE_LEGACY_MEMOS=0\"}"); - std::abort(); - } - if (bitSet) { - // The notice is only consumable on the handle arm (the legacy registry keys on - // the frontend ADDRESS, which is gone by the time a destructor speaks), so it is - // installed exactly where it can be answered. Once per process, cold. - MG_State::GLState::SetStateObjectDeathOps(&g_glesStateObjectDeathOps); - } - return bitSet; -#else - // The legacy arm is not compiled, so the handle arm is the only arm. The bit still - // decides nothing here; it is recorded so a log reader sees the mismatch. - if (!bitSet) { - MGLOG_D("MGPipe: kMGPipeSubsystemEsprytSlots is clear but this build has no " - "legacy twin registry; running the handle arm anyway"); - } - MG_State::GLState::SetStateObjectDeathOps(&g_glesStateObjectDeathOps); - return true; -#endif + // the driver ids those twins own, leak them). + // + // This runs at the FIRST TWIN LOOKUP, not at backend bring-up. That is deliberate and + // it is the fix for a lane that went green by skipping: bring-up runs inside + // eglMakeCurrent, which the integration harness pre-flights in a forked child, and a + // child that aborts is reported as "no usable GPU" and skips every scenario. Here the + // stop lands in the caller of the twin lookup - a scenario body, a sync path, a test - + // where ctest reports it as a failure. A process that never looks a twin up never needs + // an arm and is never stopped by this. + const EsprytSlotArmVerdict verdict = CurrentEsprytSlotArmVerdict(); + if (verdict == EsprytSlotArmVerdict::NoArm) { + // The operator asked for the handle arm to be OFF and the legacy arm to be + // unreachable at the same time, which leaves no arm at all. This is a Fatal{}, + // and a Fatal{} in this codebase STOPS (MG_Impl/Pipe/PipeFill.cpp's BadKnob and + // its verify trap are both MGLOG_F + abort). Returning here instead would run + // the very arm the operator disabled and hand back a green result measured on + // it - which is exactly the lever HandleRecycleScenario's arms are selected + // with, so a mis-set A/B would be scored silently against the wrong arm + // (ARCHITECTURE.md 9.6). + MGLOG_F("MGPipe: Fatal{PipeLegacyMemosDisabled, \"MOBILEGL_PIPE_PUSH leaves " + "kMGPipeSubsystemEsprytSlots (bit 5) clear and MOBILEGL_PIPE_LEGACY_MEMOS=0 " + "makes the legacy twin registry unreachable, so there is no twin table arm " + "to run\"}"); + std::abort(); } + if (verdict == EsprytSlotArmVerdict::Legacy) { + return false; + } +#if !MOBILEGL_PIPE_LEGACY_MEMOS + if ((MG_Config::Features.PipePush & MG_Pipe::kMGPipeSubsystemEsprytSlots) == 0) { + // The bit is clear but this build has no legacy twin registry to fall back to, so + // the bit decides nothing. Recorded so a log reader sees the mismatch. + MGLOG_D("MGPipe: kMGPipeSubsystemEsprytSlots is clear but this build has no " + "legacy twin registry; running the handle arm anyway"); + } +#endif + // The notice is only consumable on the handle arm (the legacy registry keys on the + // frontend ADDRESS, which is gone by the time a destructor speaks), so it is installed + // exactly where it can be answered. Once per process, cold. + MG_State::GLState::SetStateObjectDeathOps(&g_glesStateObjectDeathOps); + return true; } #endif diff --git a/MobileGL/MG_Backend/DirectGLES/SlotTables.h b/MobileGL/MG_Backend/DirectGLES/SlotTables.h index 8d381957..61400651 100644 --- a/MobileGL/MG_Backend/DirectGLES/SlotTables.h +++ b/MobileGL/MG_Backend/DirectGLES/SlotTables.h @@ -68,8 +68,36 @@ namespace MobileGL::MG_Backend::DirectGLES { #if MOBILEGL_PIPE_PUSH - // Reads the config, logs, and traps when the operator left no arm at all. Cold: called - // exactly once per process, from the latch below and from backend context creation. + // What the two knobs add up to. Split out as a PURE function of them so a test can drive + // every combination without needing a process per combination. + enum class EsprytSlotArmVerdict { + Handles, // kMGPipeSubsystemEsprytSlots is set: the {slot, gen} tables run. + Legacy, // the bit is clear and the legacy address-keyed registry is reachable. + NoArm, // the bit is clear AND MOBILEGL_PIPE_LEGACY_MEMOS=0 made the legacy arm + // unreachable, so the operator asked for a configuration with no arm at all. + }; + + EsprytSlotArmVerdict ClassifyEsprytSlotArm(Bool subsystemBitSet, Bool legacyMemosEnabled); + + // This process's verdict, read off MG_Config::Features. Latches nothing and stops nothing. + EsprytSlotArmVerdict CurrentEsprytSlotArmVerdict(); + + // Says, at backend bring-up, that the knobs leave no arm - and does NOT stop. + // + // The stop cannot live here, and that is the whole point of the split. Backend context + // creation runs inside eglMakeCurrent, and the integration harness pre-flights exactly that + // sequence in a FORKED CHILD (MG_IntegrationTest/Harness/HeadlessGL.cpp): a child that dies + // on a signal is reported as "no usable GPU/display/ICD" and every scenario in the lane is + // SKIPPED - i.e. the lane goes green having run nothing, on the very pair of env vars the + // D14/D18 A/B is driven with, which is what ROADMAP.md:7 forbids. So bring-up only + // DIAGNOSES; the stop is raised by ResolveEsprytSlotTablesArm() at the first twin lookup, + // which happens in the test body where the harness reports it as a failure. + void DiagnoseEsprytSlotArm(); + + // Reads the config, logs, installs the death-notice consumer, and STOPS when the operator + // left no arm at all. Cold: called exactly once per process, from the latch below - i.e. at + // the first twin lookup, which is the first moment an arm is actually needed. A process + // that never twins anything needs no arm and is not stopped. Bool ResolveEsprytSlotTablesArm(); // True when this process runs the {slot, gen} arm. Fixed for the life of the process: the diff --git a/MobileGL/MG_Test/SanityTest.cpp b/MobileGL/MG_Test/SanityTest.cpp index 7c095758..933b5b72 100644 --- a/MobileGL/MG_Test/SanityTest.cpp +++ b/MobileGL/MG_Test/SanityTest.cpp @@ -42,6 +42,7 @@ #include #include #include +#include #include #include @@ -3534,6 +3535,82 @@ TEST(DirectGLESSlotTable, TheTwinRegistryCasesInThisBinaryRunOnTheHandleArm) { 0ull); } +// The gate on MAJOR 1 of the round-3 review. Commit d89fb684 raised +// Fatal{PipeLegacyMemosDisabled} from inside InitDisplayAndContext(), i.e. from inside EGL +// bring-up - and the integration harness pre-flights EGL bring-up in a FORKED CHILD, converting +// any child that dies on a signal into "no usable GPU/display/ICD" and SKIPPING every scenario. +// So `MOBILEGL_PIPE_PUSH=0 MOBILEGL_PIPE_LEGACY_MEMOS=0 ctest -L integration-gpu -R DirectGLES` +// reported 100% tests passed while running nothing at all, on the exact pair of env vars the +// D14/D18 A/B is driven with. ROADMAP.md:7 forbids a gate that cannot go red for the reason it +// exists, and a lane that goes green by skipping is the worst version of that. +// +// The split this case pins: bring-up DIAGNOSES (and returns), first twin lookup STOPS. It +// checks the message and not only the signal, because an operator who is handed a bare +// "Subprocess aborted" has been told nothing about which two knobs they set. +TEST(DirectGLESSlotTable, AnArmlessKnobCombinationStopsInsteadOfSkippingTheLane) { +#if !MOBILEGL_PIPE_LEGACY_MEMOS + GTEST_SKIP() << "this build compiles no legacy twin registry, so no knob combination can " + "leave the process without an arm"; +#else + using namespace MobileGL; + namespace fs = std::filesystem; + using MG_Backend::DirectGLES::EsprytSlotArmVerdict; + + // The pure half: all four knob combinations, no process required. + EXPECT_EQ(MG_Backend::DirectGLES::ClassifyEsprytSlotArm(true, true), EsprytSlotArmVerdict::Handles); + EXPECT_EQ(MG_Backend::DirectGLES::ClassifyEsprytSlotArm(true, false), EsprytSlotArmVerdict::Handles); + EXPECT_EQ(MG_Backend::DirectGLES::ClassifyEsprytSlotArm(false, true), EsprytSlotArmVerdict::Legacy); + EXPECT_EQ(MG_Backend::DirectGLES::ClassifyEsprytSlotArm(false, false), EsprytSlotArmVerdict::NoArm); + + const Uint64 savedPush = MG_Config::Features.PipePush; + const Bool savedLegacy = MG_Config::Features.PipeLegacyMemos; + MG_Config::Features.PipePush = savedPush & ~MG_Pipe::kMGPipeSubsystemEsprytSlots; + MG_Config::Features.PipeLegacyMemos = false; + ASSERT_EQ(MG_Backend::DirectGLES::CurrentEsprytSlotArmVerdict(), EsprytSlotArmVerdict::NoArm); + + const fs::path logPath = fs::temp_directory_path() / "mobilegl-espryt-armless-knobs.log"; + fs::remove(logPath); + MG_Util::Debug::Close(); + SetEnvVar("MOBILEGL_LOG_FILE_PATH", logPath.string().c_str()); + + // Bring-up's half of the split. It must NAME the knobs and it must RETURN: this call is the + // one InitDisplayAndContext() makes, and it runs inside the harness's forked pre-flight + // child. If it ever stops again, this line takes the whole binary down and the case is red. + MG_Backend::DirectGLES::DiagnoseEsprytSlotArm(); + +#if !defined(_WIN32) + // First-use's half: the stop, raised in a forked child so it is a datum rather than the end + // of this process. In production the caller is a twin lookup inside a scenario body, where + // ctest reports the crash as a FAILING test rather than as a missing GPU. + EXPECT_EXIT((void)MG_Backend::DirectGLES::ResolveEsprytSlotTablesArm(), + ::testing::KilledBySignal(SIGABRT), ""); +#endif + + MG_Util::Debug::Close(); + UnsetEnvVar("MOBILEGL_LOG_FILE_PATH"); + MG_Config::Features.PipePush = savedPush; + MG_Config::Features.PipeLegacyMemos = savedLegacy; + + std::string contents; + { + std::ifstream logFile(logPath); + ASSERT_TRUE(logFile.good()) << "neither the diagnosis nor the fatal wrote a line an " + "operator could read"; + contents.assign(std::istreambuf_iterator(logFile), std::istreambuf_iterator()); + } + fs::remove(logPath); + + EXPECT_NE(contents.find("PipeLegacyMemosDisabled"), std::string::npos) << contents; + EXPECT_NE(contents.find("MOBILEGL_PIPE_PUSH"), std::string::npos) << contents; + EXPECT_NE(contents.find("MOBILEGL_PIPE_LEGACY_MEMOS=0"), std::string::npos) << contents; + EXPECT_NE(contents.find("kMGPipeSubsystemEsprytSlots"), std::string::npos) << contents; +#if !defined(_WIN32) + EXPECT_NE(contents.find("Fatal{"), std::string::npos) + << "the diagnosis was logged but the first-use stop was not: " << contents; +#endif +#endif // MOBILEGL_PIPE_LEGACY_MEMOS +} + #else // G2 wants the pull and the push build to list the SAME ctest entries. The twin table only // exists under MOBILEGL_PIPE_PUSH, so in the pull build each case above keeps its name and @@ -3581,4 +3658,8 @@ TEST(DirectGLESSlotTable, AProgramAndARenderbufferAnnounceTheirOwnDeath) { TEST(DirectGLESSlotTable, TheHandleArmInstallsTheDeathNoticeConsumer) { GTEST_SKIP() << "the {slot, gen} twin table is compiled only under MOBILEGL_PIPE_PUSH"; } + +TEST(DirectGLESSlotTable, AnArmlessKnobCombinationStopsInsteadOfSkippingTheLane) { + GTEST_SKIP() << "the {slot, gen} twin table is compiled only under MOBILEGL_PIPE_PUSH"; +} #endif // MOBILEGL_PIPE_PUSH