From 94c130d639fef24cffa2eb49eacef7033c21504c Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Tue, 22 Sep 2026 00:37:39 -0400 Subject: [PATCH] [MG_Util, MG_IntegrationTest, CI] (P6): the readers the role split left behind The per-role log split renamed BOTH halves so that an un-updated reader would fail loudly instead of silently reading half a session. It worked: CI found four readers this way. This is them. - MG_Util/Debug/Log.h: the role vocabulary now exists in the PULL build too, header-only. It was wholly inside #if MOBILEGL_BUILD_DISAGGREGATED, so PipeInputsTest - which builds in the verify flavour - could not name ReadRoleLogs and took the whole build-linux-verify job down at compile time. Inline rather than out-of-line because of G1: nothing in the library calls these, so an inline definition emits code into the readers and NOTHING into libMobileGL.so. Verified: pull symbols 30570 -> 30570, 0 added / 0 removed, .text still 0xa52203, and zero role-log symbols in the pull library. - PipeStatsWindow.h: the harness copy of the naming rule is now flavour-aware. It derived `.client` in BOTH flavours, so in the verify build - one role, MOBILEGL_LOG_FILE_PATH written unchanged - it opened a file that never existed. The copy has to stay a copy: MG_Util's RoleLogPath is `t` (local) in the shipping .so this module links, so calling it does not link. Added LogMark/MarkLaneLog/ReadLaneLogSince: one offset PER ROLE, because a single scalar over the concatenation slides by whatever the other role wrote. - The four arming scenarios: they asserted on lines the BACKEND emits, and under inproc the backend runs on the apply thread - the server role - so the line lands in the server's log. Reading only the client's half reported the emulation unarmed, i.e. accused the product of a defect the reader invented. Four copies of the same private helper pair now delegate to the harness. - scripts/ci/magma_runahead_checks.py: it unlinked and read the base name as if it were a file, so the negative arm saw an empty log and could not prove the knob took. It now imports split_log_paths' derivation - the authoritative one - rather than becoming a third copy. Local verification (WSL, lk-split + c6-pull): - the 4 CI-red entries: 10/10 PASS under inproc, 10/10 under monolith - unit role-log readers: 510/510 PASS - magma_runahead_checks negative: run_ahead_0 EXPECTED_RED, restored GREEN - the 5 affected TUs compile clean with MOBILEGL_BUILD_DISAGGREGATED=0 - G1: 0/0/0/0, .text 0xa52203 Red-once is the CI run itself (35677991453), which failed for each of these reasons before the change. --- .../Harness/PipeStatsWindow.h | 83 +++++++++++++++++-- .../Scenarios/PipeVerifyArmingScenario.cpp | 31 ++++--- .../Scenarios/PointSizeDemotionScenario.cpp | 28 +++---- .../PrimitivesGeneratedNoXfbScenario.cpp | 28 +++---- .../Scenarios/UnlocatedIoBlockScenario.cpp | 28 +++---- MobileGL/MG_Util/Debug/Log.h | 44 ++++++++++ scripts/ci/magma_runahead_checks.py | 22 ++++- 7 files changed, 191 insertions(+), 73 deletions(-) diff --git a/MobileGL/MG_IntegrationTest/Harness/PipeStatsWindow.h b/MobileGL/MG_IntegrationTest/Harness/PipeStatsWindow.h index 6d94c1a6..d5cc9f8c 100644 --- a/MobileGL/MG_IntegrationTest/Harness/PipeStatsWindow.h +++ b/MobileGL/MG_IntegrationTest/Harness/PipeStatsWindow.h @@ -29,6 +29,7 @@ #pragma once +#include #include #include #include @@ -41,13 +42,20 @@ namespace MGITest::PipeStatsWindow { // ROLE - `.client` and `.server` - because under inproc both roles are // threads of one process and a single file made every per-side assertion a search. // - // THE SUFFIX IS DERIVED HERE AND NOWHERE ELSE, matching MG_Util/Debug/Log.cpp's - // RoleLogPathFor. Two copies of a naming rule is one copy too many: the one that is not - // updated opens a file that does not exist and reports "the library never logged", which - // reads as a product failure rather than as a stale path. + // THE RULE IS COPIED HERE AND IT HAS TO BE, which is worth stating because the obvious fix is + // to call the library's own MG_Util::Debug::RoleLogPath and delete this. That does not link: + // this module links the SHIPPING libMobileGL.so, built -fvisibility=hidden, and the role-path + // helpers are `t` (local) in it - only MG_Test, which links the static archive, may call them. + // + // SO THE COPY IS FLAVOUR-AWARE INSTEAD. That is the part the first copy got wrong: it derived + // `.client` in BOTH flavours, while the pull build has one role and writes + // MOBILEGL_LOG_FILE_PATH unchanged. In the verify lane it therefore opened a file that never + // existed and every reader reported "the library never logged" - a product failure that had + // not happened. Keep this `#if` and Log.cpp's RoleLogPath in step. inline std::string RoleLogPath(const char* roleSuffix) { const char* base = std::getenv("MOBILEGL_LOG_FILE_PATH"); if (base == nullptr || *base == '\0') return {}; +#if MOBILEGL_BUILD_DISAGGREGATED std::string path(base); const std::string::size_type slash = path.find_last_of("/\\"); const std::string::size_type dot = path.find_last_of('.'); @@ -55,15 +63,27 @@ namespace MGITest::PipeStatsWindow { return path + "." + roleSuffix; } return path.substr(0, dot) + "." + roleSuffix + path.substr(dot); +#else + (void)roleSuffix; + return std::string(base); +#endif } - // The lane's private CLIENT log path, or empty when the lane configured none. Every existing - // caller wants this one: they read markers the CLIENT emits - the transport resolution line, - // PipeStats summaries, the arming diagnostics. + // The lane's private CLIENT log path, or empty when the lane configured none. In the pull + // build this IS the whole log. inline std::string LibraryLogPath() { return RoleLogPath("client"); } - // The server role's half, for a caller that wants a line only the applier writes. - inline std::string ServerLibraryLogPath() { return RoleLogPath("server"); } + // The server role's half, or EMPTY when this build has no separate server half. Empty rather + // than "the same path again": every caller below concatenates the two, and a pull build that + // named one file twice would show each line twice - which reads as a doubled counter or a + // repeated diagnostic rather than as a path mistake. + inline std::string ServerLibraryLogPath() { +#if MOBILEGL_BUILD_DISAGGREGATED + return RoleLogPath("server"); +#else + return {}; +#endif + } inline std::string ReadWholeFile(const std::string& path) { if (path.empty()) return {}; @@ -72,6 +92,51 @@ namespace MGITest::PipeStatsWindow { return std::string((std::istreambuf_iterator(file)), std::istreambuf_iterator()); } + inline std::string ReadFileSince(const std::string& path, std::uintmax_t offset) { + if (path.empty()) return {}; + std::ifstream file(path, std::ios::binary); + if (!file.good()) return {}; + file.seekg(static_cast(offset)); + return std::string((std::istreambuf_iterator(file)), std::istreambuf_iterator()); + } + + // A "the log is this long right now" snapshot, ONE OFFSET PER ROLE. + // + // The scenarios that assert on arming take a mark before the workload and read what was + // appended after it, so that only bytes this case caused can satisfy - or refute - the claim. + // The roles are separate files, so this cannot be one scalar: a single offset applied to the + // concatenation would slide by however much the OTHER role happened to write, and the read + // would start mid-line in the wrong file. + struct LogMark { + std::uintmax_t client = 0; + std::uintmax_t server = 0; + }; + + inline std::uintmax_t FileSizeOrZero(const std::string& path) { + if (path.empty()) return 0; + std::ifstream file(path, std::ios::binary | std::ios::ate); + if (!file.good()) return 0; + const std::streamoff size = file.tellg(); + return size < 0 ? 0 : static_cast(size); + } + + inline LogMark MarkLaneLog() { + return LogMark{FileSizeOrZero(LibraryLogPath()), FileSizeOrZero(ServerLibraryLogPath())}; + } + + // BOTH ROLES, and for the arming diagnostics the SERVER's is the one that matters. Those lines + // are emitted by the BACKEND - "demoted to an ordinary varying", the reroute and emulation + // banners - and under inproc the backend runs on the apply thread, which is the server role. + // Reading only the client's half finds nothing and reports "the emulation is not armed", + // which is the most expensive possible way to be wrong: it accuses the product of a defect + // that the reader itself invented. + inline std::string ReadLaneLogSince(const LogMark& mark) { + return ReadFileSince(LibraryLogPath(), mark.client) + + ReadFileSince(ServerLibraryLogPath(), mark.server); + } + + inline std::string ReadLaneLog() { return ReadLaneLogSince(LogMark{}); } + // The last summary line in the log, verbatim. `found` is false when the library never emitted // one, which is a different failure from "the counter read zero" and has to be reported as // one: it means the stats channel never reached the process, not that the workload did diff --git a/MobileGL/MG_IntegrationTest/Scenarios/PipeVerifyArmingScenario.cpp b/MobileGL/MG_IntegrationTest/Scenarios/PipeVerifyArmingScenario.cpp index ed834b65..d2df7877 100644 --- a/MobileGL/MG_IntegrationTest/Scenarios/PipeVerifyArmingScenario.cpp +++ b/MobileGL/MG_IntegrationTest/Scenarios/PipeVerifyArmingScenario.cpp @@ -111,24 +111,23 @@ void main() { o_color = vec4(0.25, 0.5, 0.75, 1.0); } return std::filesystem::path(MGITest::PipeStatsWindow::LibraryLogPath()); } - static std::uintmax_t LibraryLogSize() { - std::error_code ec; - const std::filesystem::path path = LibraryLogPath(); - if (path.empty()) return 0; - const std::uintmax_t size = std::filesystem::file_size(path, ec); - return ec ? 0 : size; + // ONE MARK PER ROLE. The library writes a log per role, so "how long is the log + // right now" is two numbers; a single scalar applied to the concatenation would slide + // by whatever the other role wrote in between and start the read mid-line. + static MGITest::PipeStatsWindow::LogMark LibraryLogMark() { + return MGITest::PipeStatsWindow::MarkLaneLog(); } - static std::string LibraryLogSince(std::uintmax_t offset) { - const std::filesystem::path path = LibraryLogPath(); - if (path.empty()) return {}; - std::ifstream file(path, std::ios::binary); - if (!file.good()) return {}; - file.seekg(static_cast(offset)); - return std::string((std::istreambuf_iterator(file)), std::istreambuf_iterator()); + // BOTH ROLES. The arming diagnostics this case looks for are emitted by the + // BACKEND, and under inproc the backend runs on the apply thread - the server role - + // so the line lands in the server's log. Reading only the client's found nothing and + // reported the emulation unarmed, which accused the product of a defect the reader + // had invented. + static std::string LibraryLogSince(const MGITest::PipeStatsWindow::LogMark& mark) { + return MGITest::PipeStatsWindow::ReadLaneLogSince(mark); } - static std::string LibraryLog() { return LibraryLogSince(0); } + static std::string LibraryLog() { return MGITest::PipeStatsWindow::ReadLaneLog(); } // One frame that crosses several verb boundaries: a clear (kClear), a draw (kDraw) and // a readback (kReadback). Three of the nine fill classes, so an entry compare that only @@ -201,7 +200,7 @@ void main() { o_color = vec4(0.25, 0.5, 0.75, 1.0); } "backwards; the VerifyCorrupted. lane owns that half"; } - const std::uintmax_t before = LibraryLogSize(); + const MGITest::PipeStatsWindow::LogMark before = LibraryLogMark(); ASSERT_NO_FATAL_FAILURE(DrawOneFrame()); EXPECT_EQ(FirstGLError(), 0u); @@ -252,7 +251,7 @@ void main() { o_color = vec4(0.25, 0.5, 0.75, 1.0); } } const std::string knob = std::getenv("MOBILEGL_PIPE_VERIFY_CORRUPT"); - const std::uintmax_t before = LibraryLogSize(); + const MGITest::PipeStatsWindow::LogMark before = LibraryLogMark(); ASSERT_NO_FATAL_FAILURE(DrawOneFrame()); const std::string appended = LibraryLogSince(before); diff --git a/MobileGL/MG_IntegrationTest/Scenarios/PointSizeDemotionScenario.cpp b/MobileGL/MG_IntegrationTest/Scenarios/PointSizeDemotionScenario.cpp index f1c31c98..9e247b95 100644 --- a/MobileGL/MG_IntegrationTest/Scenarios/PointSizeDemotionScenario.cpp +++ b/MobileGL/MG_IntegrationTest/Scenarios/PointSizeDemotionScenario.cpp @@ -311,22 +311,20 @@ void main() return std::filesystem::path(MGITest::PipeStatsWindow::LibraryLogPath()); } - static std::uintmax_t LibraryLogSize() { - std::error_code ec; - const std::filesystem::path path = LibraryLogPath(); - if (path.empty()) return 0; - const std::uintmax_t size = std::filesystem::file_size(path, ec); - return ec ? 0 : size; + // ONE MARK PER ROLE. The library writes a log per role, so "how long is the log + // right now" is two numbers; a single scalar applied to the concatenation would slide + // by whatever the other role wrote in between and start the read mid-line. + static MGITest::PipeStatsWindow::LogMark LibraryLogMark() { + return MGITest::PipeStatsWindow::MarkLaneLog(); } - static std::string LibraryLogSince(std::uintmax_t offset) { - const std::filesystem::path path = LibraryLogPath(); - if (path.empty()) return {}; - std::ifstream file(path, std::ios::binary); - if (!file.good()) return {}; - file.seekg(static_cast(offset)); - return std::string((std::istreambuf_iterator(file)), - std::istreambuf_iterator()); + // BOTH ROLES. The arming diagnostics this case looks for are emitted by the + // BACKEND, and under inproc the backend runs on the apply thread - the server role - + // so the line lands in the server's log. Reading only the client's found nothing and + // reported the emulation unarmed, which accused the product of a defect the reader + // had invented. + static std::string LibraryLogSince(const MGITest::PipeStatsWindow::LogMark& mark) { + return MGITest::PipeStatsWindow::ReadLaneLogSince(mark); } std::string m_buildLog; @@ -494,7 +492,7 @@ void main() // Taken BEFORE the program is built, so the line this looks for can only be one // this process wrote. - const std::uintmax_t before = LibraryLogSize(); + const MGITest::PipeStatsWindow::LogMark before = LibraryLogMark(); const GLuint program = BuildCaptureProgram({{GL_VERTEX_SHADER, kPointVertexSource}, {GL_GEOMETRY_SHADER, kPointGeometrySource}, diff --git a/MobileGL/MG_IntegrationTest/Scenarios/PrimitivesGeneratedNoXfbScenario.cpp b/MobileGL/MG_IntegrationTest/Scenarios/PrimitivesGeneratedNoXfbScenario.cpp index b13e2bac..ddb6f104 100644 --- a/MobileGL/MG_IntegrationTest/Scenarios/PrimitivesGeneratedNoXfbScenario.cpp +++ b/MobileGL/MG_IntegrationTest/Scenarios/PrimitivesGeneratedNoXfbScenario.cpp @@ -256,22 +256,20 @@ void main() { return std::filesystem::path(MGITest::PipeStatsWindow::LibraryLogPath()); } - static std::uintmax_t LibraryLogSize() { - std::error_code ec; - const std::filesystem::path path = LibraryLogPath(); - if (path.empty()) return 0; - const std::uintmax_t size = std::filesystem::file_size(path, ec); - return ec ? 0 : size; + // ONE MARK PER ROLE. The library writes a log per role, so "how long is the log + // right now" is two numbers; a single scalar applied to the concatenation would slide + // by whatever the other role wrote in between and start the read mid-line. + static MGITest::PipeStatsWindow::LogMark LibraryLogMark() { + return MGITest::PipeStatsWindow::MarkLaneLog(); } - static std::string LibraryLogSince(std::uintmax_t offset) { - const std::filesystem::path path = LibraryLogPath(); - if (path.empty()) return {}; - std::ifstream file(path, std::ios::binary); - if (!file.good()) return {}; - file.seekg(static_cast(offset)); - return std::string((std::istreambuf_iterator(file)), - std::istreambuf_iterator()); + // BOTH ROLES. The arming diagnostics this case looks for are emitted by the + // BACKEND, and under inproc the backend runs on the apply thread - the server role - + // so the line lands in the server's log. Reading only the client's found nothing and + // reported the emulation unarmed, which accused the product of a defect the reader + // had invented. + static std::string LibraryLogSince(const MGITest::PipeStatsWindow::LogMark& mark) { + return MGITest::PipeStatsWindow::ReadLaneLogSince(mark); } GLuint m_vao = 0; @@ -536,7 +534,7 @@ void main() { // Taken BEFORE the draw, so the line this looks for can only be one this // process wrote for this span. The latch fires on the FIRST rerouted // draw, which is inside the query below. - const std::uintmax_t before = LibraryLogSize(); + const MGITest::PipeStatsWindow::LogMark before = LibraryLogMark(); const GLuint generated = QueryGenerated([]() { glDrawArrays(GL_TRIANGLES, 0, 3); }); EXPECT_EQ(DrainGLErrors(), 0u); EXPECT_EQ(generated, 1u) << "the pinned-on lane did not even count correctly"; diff --git a/MobileGL/MG_IntegrationTest/Scenarios/UnlocatedIoBlockScenario.cpp b/MobileGL/MG_IntegrationTest/Scenarios/UnlocatedIoBlockScenario.cpp index 252a8a40..b23c438e 100644 --- a/MobileGL/MG_IntegrationTest/Scenarios/UnlocatedIoBlockScenario.cpp +++ b/MobileGL/MG_IntegrationTest/Scenarios/UnlocatedIoBlockScenario.cpp @@ -364,22 +364,20 @@ void main() // is searched from here forward, because the file is APPENDED to by every process // in the lane and a line left behind by an earlier one would otherwise satisfy the // assertion without this process having done anything at all. - static std::uintmax_t LibraryLogSize() { - std::error_code ec; - const std::filesystem::path path = LibraryLogPath(); - if (path.empty()) return 0; - const std::uintmax_t size = std::filesystem::file_size(path, ec); - return ec ? 0 : size; + // ONE MARK PER ROLE. The library writes a log per role, so "how long is the log + // right now" is two numbers; a single scalar applied to the concatenation would slide + // by whatever the other role wrote in between and start the read mid-line. + static MGITest::PipeStatsWindow::LogMark LibraryLogMark() { + return MGITest::PipeStatsWindow::MarkLaneLog(); } - static std::string LibraryLogSince(std::uintmax_t offset) { - const std::filesystem::path path = LibraryLogPath(); - if (path.empty()) return {}; - std::ifstream file(path, std::ios::binary); - if (!file.good()) return {}; - file.seekg(static_cast(offset)); - return std::string((std::istreambuf_iterator(file)), - std::istreambuf_iterator()); + // BOTH ROLES. The arming diagnostics this case looks for are emitted by the + // BACKEND, and under inproc the backend runs on the apply thread - the server role - + // so the line lands in the server's log. Reading only the client's found nothing and + // reported the emulation unarmed, which accused the product of a defect the reader + // had invented. + static std::string LibraryLogSince(const MGITest::PipeStatsWindow::LogMark& mark) { + return MGITest::PipeStatsWindow::ReadLaneLogSince(mark); } static GLenum FirstGLError() { @@ -500,7 +498,7 @@ void main() // Taken BEFORE the program is built, so the line this looks for can only be one // this process wrote. The latch means it is emitted at the FIRST stage of the // FIRST affected program, which is inside the build below. - const std::uintmax_t before = LibraryLogSize(); + const MGITest::PipeStatsWindow::LogMark before = LibraryLogMark(); const GLuint program = BuildPipeline(kDistinctTessEvalSource, kDistinctGeometrySource); if (program == 0) { diff --git a/MobileGL/MG_Util/Debug/Log.h b/MobileGL/MG_Util/Debug/Log.h index 47bd79ab..758e6883 100644 --- a/MobileGL/MG_Util/Debug/Log.h +++ b/MobileGL/MG_Util/Debug/Log.h @@ -122,6 +122,7 @@ {} #endif +#include #include namespace MobileGL { @@ -173,6 +174,49 @@ namespace MobileGL { // these readers exist to check. std::string ReadRoleLogs(const char* basePath); void TruncateRoleLogs(const char* basePath); +#else + // THE SAME VOCABULARY IN THE PULL BUILD, where there is exactly one log because there + // is exactly one role. A reader that asks for "what this run logged" has to compile + // and mean the right thing in BOTH flavours; the alternative is an `#if` at every + // call site, and the call site that forgets one is the one that silently reads + // nothing. (It was not hypothetical: PipeInputsTest built only in the verify flavour + // and took the whole `build-linux-verify` job down with it.) + // + // HEADER-ONLY ON PURPOSE, FOR G1. The pull library's symbol set and `.text` must stay + // byte-identical to the baseline, so these may not become library symbols. Nothing + // inside the library calls them - only tests and harnesses do - so an inline + // definition emits code into those readers and nothing at all into libMobileGL.so. + enum class LogRole { Client, Server }; + + // No roles to separate, so nothing to record. Present so that a harness may say which + // role a thread is without asking which build it is in. + inline void SetThreadLogRole(LogRole) {} + + // One role, one file: the pull build writes MOBILEGL_LOG_FILE_PATH unchanged. + inline std::string RoleLogPath(const char* basePath, LogRole) { + return basePath == nullptr ? std::string() : std::string(basePath); + } + + inline std::string ReadRoleLogs(const char* basePath) { + std::string all; + if (basePath == nullptr) return all; + if (FILE* file = std::fopen(basePath, "rb")) { + char chunk[4096]; + std::size_t got = 0; + while ((got = std::fread(chunk, 1, sizeof(chunk), file)) > 0) { + all.append(chunk, got); + } + std::fclose(file); + } + return all; + } + + inline void TruncateRoleLogs(const char* basePath) { + if (basePath == nullptr) return; + if (FILE* file = std::fopen(basePath, "w")) { + std::fclose(file); + } + } #endif } // namespace Debug } // namespace MG_Util diff --git a/scripts/ci/magma_runahead_checks.py b/scripts/ci/magma_runahead_checks.py index 772b7d23..85d98544 100644 --- a/scripts/ci/magma_runahead_checks.py +++ b/scripts/ci/magma_runahead_checks.py @@ -19,6 +19,15 @@ import sys import time import xml.etree.ElementTree as ET +# P6: MOBILEGL_LOG_FILE_PATH IS A BASE NAME and the library writes one file per ROLE, so no file +# of the literal name exists. The derivation is IMPORTED, not copied: a third copy of the rule is +# a third thing that can fall behind, and the one that does reads an empty log and reports "the +# runtime knob did not take" - a product failure that never happened. (It did happen: this check +# went red on the role split for exactly that reason.) +sys.path.insert(0, str(Path(__file__).resolve().parents[2] / "MobileGL" / "MG_IntegrationTest" + / "Harness")) +from split_log_paths import any_role_file, read_role_logs, role_paths # noqa: E402 + CASES = ( "QueuedBufferVersionsAndDeletedNameReuseKeepTheirPixels", "QueuedProgramRebindsKeepEachUniformSnapshot", @@ -98,7 +107,10 @@ def run_case(test, build, directory, overrides): private = (cwd / private).resolve() if not private.is_absolute() else private.resolve() if not private.is_relative_to(build): raise RuntimeError(f"refusing to reset a private log outside the selected build: {private}") - private.unlink(missing_ok=True) + # Every role's file, because a stale server half would otherwise survive into this arm and + # its lines would read as evidence about the knob this run set. + for role_path in role_paths(private): + Path(role_path).unlink(missing_ok=True) xml = directory / "gtest.xml" command = [arg for arg in command if not arg.startswith("--gtest_output=")] command.append(f"--gtest_output=xml:{xml}") @@ -119,10 +131,14 @@ def run_case(test, build, directory, overrides): except subprocess.TimeoutExpired: rc = 124 output.write("\nRUNNER_TIMEOUT: process killed after discovered CTest timeout\n") - private_text = private.read_text(encoding="utf-8", errors="replace") if private.is_file() else "" + # BOTH ROLES, concatenated into the one evidence file this run archives. `Config: IPC` is the + # client's line and the queue-lead diagnostics are the applier's, so either half alone would + # answer one of the two questions the negative arm asks and be silent on the other. + private_text = read_role_logs(private) (directory / "private.log").write_text(private_text, encoding="utf-8") result = {"ctest_name": test["name"], "returncode": rc, - "seconds": round(time.monotonic()-started, 3), "private_log_present": private.is_file()} + "seconds": round(time.monotonic()-started, 3), + "private_log_present": any_role_file(private)} write_json(directory / "process.json", result) return result