diff --git a/tools/trace_replay/CMakeLists.txt b/tools/trace_replay/CMakeLists.txt index ae1ed03b..4534e037 100644 --- a/tools/trace_replay/CMakeLists.txt +++ b/tools/trace_replay/CMakeLists.txt @@ -322,7 +322,14 @@ function(add_trace_replay_test CASE_NAME BACKEND) CROP_Y CROP_WIDTH CROP_HEIGHT - COHERENT_AS_FLUSH) + COHERENT_AS_FLUSH + # P5. VARIANT is a NAME SUFFIX and TRANSPORT is what the variant runs with; passing + # TRANSPORT without VARIANT is a configure error below, because add_test with a + # duplicate NAME is a hard CMake error and the collision is with the very case this + # entry is a second arm of. + VARIANT + TRANSPORT + IPC_SERVER_PATH) cmake_parse_arguments(TRACE_CASE "" "${oneValueArgs}" "" ${ARGN}) foreach(required TRACE_ARCHIVE GOLDEN TARGET_CALL WIDTH HEIGHT) if(NOT TRACE_CASE_${required}) @@ -347,8 +354,38 @@ function(add_trace_replay_test CASE_NAME BACKEND) if(NOT TRACE_CASE_CROP_HEIGHT) set(TRACE_CASE_CROP_HEIGHT 0) endif() + + # --- P5: the variant suffix, and why the OUTPUT DIRECTORIES have to carry it too ---------- + # + # ARCHITECTURE.md:584 asks for the SPLIT suffix because without it the entry collides with + # the same case+backend and add_test with a duplicate NAME is a hard CMake error. + # + # THE DIRECTORIES ARE THE HALF THAT IS EASY TO FORGET AND EXPENSIVE TO GET WRONG. Both arms + # of a case write output/mobilegl.log, and run_trace_case.cmake deletes TRACE_OUTPUT_DIR + # before every run - so two arms sharing one directory means the second arm's run erases the + # first arm's log while `ctest -j` may still be reading it, and, worse, the SPLIT arm's + # refusal census is then a census of whichever arm happened to finish last. That log is the + # ONLY valid census: the console sink is compiled out of these configurations, so a `ctest -V` + # transcript reports a FALSE ZERO for Fatal{ lines. P4a shipped exactly that defect once + # already, in a different place. Same argument for TRACE_ARTIFACT_DIR, which is what CI + # uploads. + set(trace_case_suffix "") + set(trace_case_output_tag "${BACKEND}") + set(trace_case_artifact_tag "actual-images") + if(TRACE_CASE_VARIANT) + set(trace_case_suffix ".${TRACE_CASE_VARIANT}") + set(trace_case_output_tag "${BACKEND}-${TRACE_CASE_VARIANT}") + set(trace_case_artifact_tag "actual-images-${TRACE_CASE_VARIANT}") + elseif(TRACE_CASE_TRANSPORT) + message(FATAL_ERROR + "add_trace_replay_test(${CASE_NAME} ${BACKEND} TRANSPORT ${TRACE_CASE_TRANSPORT}) with no " + "VARIANT: the entry would collide with the monolith arm of the same case and backend, and " + "a duplicate add_test NAME is a hard error") + endif() + set(trace_case_name "MobileGLTraceReplay.${CASE_NAME}.${BACKEND}${trace_case_suffix}") + add_test( - NAME MobileGLTraceReplay.${CASE_NAME}.${BACKEND} + NAME ${trace_case_name} COMMAND "${CMAKE_COMMAND}" -DTRACE_REPLAY_EXE=$ -DMOBILEGL_LIBRARY=${mobilegl_trace_replay_mobilegl_library} @@ -367,17 +404,30 @@ function(add_trace_replay_test CASE_NAME BACKEND) -DTRACE_CROP_WIDTH=${TRACE_CASE_CROP_WIDTH} -DTRACE_CROP_HEIGHT=${TRACE_CASE_CROP_HEIGHT} -DTRACE_COHERENT_AS_FLUSH=${TRACE_CASE_COHERENT_AS_FLUSH} - -DTRACE_OUTPUT_DIR=${CMAKE_CURRENT_BINARY_DIR}/${CASE_NAME}/${BACKEND} - -DTRACE_ARTIFACT_DIR=${CMAKE_CURRENT_BINARY_DIR}/${CASE_NAME}/actual-images + -DTRACE_TRANSPORT=${TRACE_CASE_TRANSPORT} + -DTRACE_IPC_SERVER_PATH=${TRACE_CASE_IPC_SERVER_PATH} + -DTRACE_OUTPUT_DIR=${CMAKE_CURRENT_BINARY_DIR}/${CASE_NAME}/${trace_case_output_tag} + -DTRACE_ARTIFACT_DIR=${CMAKE_CURRENT_BINARY_DIR}/${CASE_NAME}/${trace_case_artifact_tag} -P ${MOBILEGL_TRACE_ROOT}/run_trace_case.cmake) + + # The label list gains `retrace-split` on a variant arm, for the same reason + # integration-split exists: a private second label is what lets `ctest -L retrace-split + # --no-tests=error` go RED in a build that never registered the arm, instead of greenly + # running nothing. `retrace` stays so the existing whole-label selections keep describing the + # whole set. A `;` inside a property value has to be escaped. + set(trace_case_labels retrace) + if(TRACE_CASE_VARIANT) + set(trace_case_labels "retrace\;retrace-${TRACE_CASE_VARIANT}") + string(TOLOWER "${trace_case_labels}" trace_case_labels) + endif() if(BACKEND STREQUAL "DirectGLES") - set_tests_properties(MobileGLTraceReplay.${CASE_NAME}.${BACKEND} PROPERTIES + set_tests_properties(${trace_case_name} PROPERTIES ENVIRONMENT "EGL_PLATFORM=surfaceless;LIBGL_ALWAYS_SOFTWARE=1;MESA_GL_VERSION_OVERRIDE=3.3;MESA_GLSL_VERSION_OVERRIDE=330" - LABELS retrace) + LABELS "${trace_case_labels}") else() - set_tests_properties(MobileGLTraceReplay.${CASE_NAME}.${BACKEND} PROPERTIES + set_tests_properties(${trace_case_name} PROPERTIES ENVIRONMENT "LIBGL_ALWAYS_SOFTWARE=1;MESA_GL_VERSION_OVERRIDE=3.3;MESA_GLSL_VERSION_OVERRIDE=330" - LABELS retrace) + LABELS "${trace_case_labels}") endif() endfunction() @@ -386,6 +436,26 @@ function(add_trace_replay_test_for_backends CASE_NAME) add_trace_replay_test(${CASE_NAME} DirectVulkan ${ARGN}) endfunction() +# --- P5: the SPLIT arm of the retrace matrix ------------------------------------------------- +# +# One variant entry per case that trace_cases.json marks `split: true` - today that is OpenRA +# alone, which is the only target the phase gate names and (measurably) the only fixture hydrated +# locally. DirectGLES only: the split shape's first backend is Espryt, and a DirectVulkan arm +# would be measuring a server nobody has written yet. +# +# REGISTERED ONLY UNDER MOBILEGL_BUILD_DISAGGREGATED, because the arm is a statement about a +# library that compiled MG_Remote. A build without the option accepts MOBILEGL_TRANSPORT=inproc +# and silently ignores it (CONTRACT-P5 5), so an unconditional registration would be an entry +# that passes by running monolith - and run_trace_case.cmake's split block would then be the only +# thing standing between that and a green, which is one guard too few for a name that says SPLIT. +function(add_trace_replay_split_test CASE_NAME) + if(NOT MOBILEGL_BUILD_DISAGGREGATED) + return() + endif() + add_trace_replay_test(${CASE_NAME} DirectGLES VARIANT SPLIT TRANSPORT inproc + IPC_SERVER_PATH "${CMAKE_BINARY_DIR}/libMobileGLServer.so" ${ARGN}) +endfunction() + set(MOBILEGL_TRACE_CASES_CMAKE "${CMAKE_CURRENT_BINARY_DIR}/trace_cases.cmake") set_property(DIRECTORY APPEND PROPERTY CMAKE_CONFIGURE_DEPENDS "${MOBILEGL_TRACE_ROOT}/trace_cases.json") execute_process( diff --git a/tools/trace_replay/run_trace_case.cmake b/tools/trace_replay/run_trace_case.cmake index 6f0110bf..394867a8 100644 --- a/tools/trace_replay/run_trace_case.cmake +++ b/tools/trace_replay/run_trace_case.cmake @@ -28,6 +28,30 @@ if(TRACE_COHERENT_AS_FLUSH) list(APPEND coherent_as_flush_args --coherent-as-flush) endif() +# --- P5: the transport, threaded as an ENVIRONMENT rather than as a replay CLI flag ---------- +# +# MOBILEGL_TRANSPORT reaches the library through the environment, so a `cmake -P` script can set +# it and the child inherits it - exactly how the MOBILEGL_PIPE_VERIFY block at the bottom of this +# file already works, and with no C++ change anywhere. The desktop CLI has no --env option +# (trace_replay_cli.cpp:116-179 is the whole option list) and the Android path already has one +# (trace-replay-ci.sh --env -> trace_replay_core.cpp's setenv block), so an environment read +# covers both directions and a new flag would buy nothing. +# +# TWO WAYS IN, ONE ASSERTION. `-DTRACE_TRANSPORT=` is what the SPLIT ctest variant passes, so +# that entry is self-describing and needs no ritual around it; exporting MOBILEGL_TRANSPORT in +# the calling process is what ~/w7/retrace_gate.py and CI's retrace-split job do over the +# UNCHANGED case names, because the gate parses a FOREIGN reference CTestTestfile.cmake whose +# name regex cannot see a variant suffix. Setting the variable from the -D FIRST means the +# assertions below read one value however it arrived. +if(DEFINED TRACE_TRANSPORT AND NOT "${TRACE_TRANSPORT}" STREQUAL "") + set(ENV{MOBILEGL_TRANSPORT} "${TRACE_TRANSPORT}") +endif() +if(DEFINED TRACE_IPC_SERVER_PATH AND NOT "${TRACE_IPC_SERVER_PATH}" STREQUAL "") + # ARCHITECTURE.md:543. P6 consumes it; P5 carries it so that "unparsed" and + # "parsed and ignored" stop being the same observation. + set(ENV{MOBILEGL_IPC_SERVER_PATH} "${TRACE_IPC_SERVER_PATH}") +endif() + if(EXISTS "${TRACE_OUTPUT_DIR}") file(REMOVE_RECURSE "${TRACE_OUTPUT_DIR}") endif() @@ -189,3 +213,75 @@ if(DEFINED ENV{MOBILEGL_PIPE_VERIFY} AND NOT "$ENV{MOBILEGL_PIPE_VERIFY}" STREQU message(STATUS "MGPipe verify: ${pipe_verify_case} armed, zero divergences, zero unmigrated reads") endif() endif() + +# --- P5: MOBILEGL_TRANSPORT, the split arm's own assertions ---------------------------------- +# +# The same problem the verify block above solves, with the same answer and one extra reason to +# need it. A retrace that exported MOBILEGL_TRANSPORT=inproc at a library configured WITHOUT +# -DMOBILEGL_BUILD_DISAGGREGATED=ON is not merely a no-op: the variable's PARSER does not exist +# in that build at all (CONTRACT-P5 5 - putting a complaint in the unconditional part of +# ConfigLoader would move a pull-build symbol and break gate G1), so the value is accepted by the +# environment and silently ignored, every frame still matches its golden, and the case reports a +# clean pass having run monolith end to end. +# +# THE LIBRARY'S OWN LOG IS THE ONLY CHANNEL a `cmake -P` script has for the difference, and it is +# also THE ONLY VALID REFUSAL CENSUS. A `ctest -V` transcript is a FALSE ZERO for Fatal{...} +# lines: the console sink is compiled out of the configurations these lanes run, so the aborts +# reach output/mobilegl.log and nowhere else. That is why TRACE_OUTPUT_DIR and TRACE_ARTIFACT_DIR +# carry the variant - without it both arms write one output/mobilegl.log, the second run wipes +# the first, and the census silently becomes a census of one arm. P4a shipped that defect once +# already; this is the same defect in a new place, pre-empted. +# +# Three demands, all silent when MOBILEGL_TRANSPORT is unset or "monolith": +# * mobilegl.log exists - the replay wrote one, so the library was loaded and logging; +# * it carries ConfigLoader's inproc line, which is emitted ONLY by a build that compiled the +# parser AND resolved the value to InProcess. This is the falsifiable half; +# * it carries no Fatal{ at all. Under split the emit table raises +# Fatal{UnmigratedVerb, ""} for the 64 slots P5 does not implement, so a clean run of a +# reduced-path target is a run that touched none of them - and any other Fatal{ (ProtocolCorruption, +# UnmigratedPipeInput, AbiMismatch) is a real defect. The count and the distinct names are +# printed either way, because the census is the deliverable even when the run passes. +if(DEFINED ENV{MOBILEGL_TRANSPORT} AND NOT "$ENV{MOBILEGL_TRANSPORT}" STREQUAL "") + set(split_case "${TRACE_CASE_NAME} ${TRACE_BACKEND}") + if("$ENV{MOBILEGL_TRANSPORT}" STREQUAL "monolith") + message(STATUS "MGPipe split: MOBILEGL_TRANSPORT=monolith, no split assertions for ${split_case}") + elseif(NOT EXISTS "${mobilegl_log}") + message(FATAL_ERROR + "MOBILEGL_TRANSPORT=$ENV{MOBILEGL_TRANSPORT} is set for ${split_case} but the run wrote " + "no ${mobilegl_log}, so there is no evidence the transport ever resolved. A split " + "retrace with no library log cannot be counted as a split retrace.") + else() + file(READ "${mobilegl_log}" split_log) + string(FIND "${split_log}" "MOBILEGL_TRANSPORT=inproc" split_armed_at) + if(split_armed_at EQUAL -1) + message(FATAL_ERROR + "MOBILEGL_TRANSPORT=$ENV{MOBILEGL_TRANSPORT} is set for ${split_case} and the library " + "never reported resolving it. ConfigLoader::InitTransport logs one line at INFO when " + "it selects InProcess, and that line exists only in a build configured with " + "-DMOBILEGL_BUILD_DISAGGREGATED=ON - in a build without it the whole parser is " + "compiled out and the variable is accepted and ignored, which is exactly the 'the " + "split lane ran monolith and went green' failure. Check that the SPLIT runtime " + "artifact is the one at ${MOBILEGL_LIBRARY}, and that MOBILEGL_LOG_ACTIVE_LEVEL " + "admits INFO.") + 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) + message(STATUS "MGPipe split: ${split_case} transport=$ENV{MOBILEGL_TRANSPORT}, " + "Fatal{ lines in ${mobilegl_log}: ${split_fatal_count}") + if(split_fatals) + foreach(line IN LISTS split_fatals) + message(STATUS "${line}") + endforeach() + message(FATAL_ERROR + "${split_case}: ${split_fatal_count} MGPipe Fatal(s) under MOBILEGL_TRANSPORT=" + "$ENV{MOBILEGL_TRANSPORT}. Fatal{UnmigratedVerb, \"\"} is one of the 64 emit-table " + "slots P5 leaves unimplemented (R-4) - if the reduced path reached it, either the verb " + "census is wrong or this case is not on the reduced path; Fatal{ProtocolCorruption, ...} " + "is a record that crossed the wire without declaring its bytes (CONTRACT-P5 rule A); " + "Fatal{UnmigratedPipeInput, ...} is a missing row in the field-ownership table. None of " + "them is silenced here: this log is the only place they appear, because the console sink " + "is compiled out of the configurations this lane runs.") + endif() + endif() +endif() diff --git a/tools/trace_replay/trace_cases.json b/tools/trace_replay/trace_cases.json index 03ef2c0d..59bcc234 100644 --- a/tools/trace_replay/trace_cases.json +++ b/tools/trace_replay/trace_cases.json @@ -14,6 +14,7 @@ { "name": "OpenRA", "verify": true, + "split": true, "trace_archive": "openra.tgz", "trace_file": "openra.trace", "golden": "openra.0000031249.png", diff --git a/tools/trace_replay/trace_cases.py b/tools/trace_replay/trace_cases.py index 8203eac5..75f944e1 100644 --- a/tools/trace_replay/trace_cases.py +++ b/tools/trace_replay/trace_cases.py @@ -36,6 +36,23 @@ def load_trace_case_manifest(path=TRACE_CASES_JSON): raise ValueError( f"{name} is marked verify but excluded from CI, so the verify matrix would drop it" ) + # "split" opts a case into the MOBILEGL_TRANSPORT=inproc retrace subset, the same shape + # and the same reason as "verify" above: a typo has to be a loud manifest error in every + # consumer, not a subset that is quietly one case short. The split arm additionally runs + # DirectGLES ONLY - the server the arm exercises is Espryt's - so a case that excluded + # DirectGLES from CI would leave the split matrix with nothing to run. + split = merged.get("split", False) + if not isinstance(split, bool): + raise ValueError(f"split must be true or false for {name}") + if split and not merged.get("ci", True): + raise ValueError( + f"{name} is marked split but excluded from CI, so the split matrix would drop it" + ) + if split and "DirectGLES" not in ci_backends(merged): + raise ValueError( + f"{name} is marked split but does not run DirectGLES in CI; the split arm is " + f"DirectGLES-only, so the entry would be registered with no backend" + ) cases.append(merged) return {"defaults": defaults, "cases": cases} @@ -92,6 +109,16 @@ def verify_trace_cases(cases): return [case for case in cases if case.get("verify", False)] +def split_trace_cases(cases): + """The subset the split (MOBILEGL_TRANSPORT=inproc) CI mode retraces. + + P5's phase gate names exactly one: OpenRA, at SSIM >= 0.99. It is also the only fixture that + is hydrated locally, so keeping the subset explicit in the manifest is what stops a later + phase from widening the arm into an LFS fetch by accident. + """ + return [case for case in cases if case.get("split", False)] + + def ci_backends(case): backends = case.get("ci_backends") if backends is None: @@ -122,6 +149,16 @@ def github_verify_matrix(cases): return github_test_matrix(verify_trace_cases(cases)) +def github_split_matrix(cases): + """{backend, case} for the split arm. DirectGLES only - see split_trace_cases.""" + return { + "include": [ + {"backend": "DirectGLES", "case": case["name"]} + for case in split_trace_cases(cases) + ] + } + + def github_apk_matrix(cases): backends = { "DirectGLES": {"name": "DirectGLES", "gpu": "software"}, @@ -158,8 +195,8 @@ def emit_cmake(cases, fixture_root): ("CROP_HEIGHT", "crop_height", False), ("COHERENT_AS_FLUSH", "coherent_as_flush", False), ] - for case in cases: - lines.append(f"add_trace_replay_test_for_backends({cmake_quote(case['name'])}") + def emit_one(function, case): + lines.append(f"{function}({cmake_quote(case['name'])}") for cmake_key, json_key, fixture_path in keys: value = case.get(json_key) if value is None or value == "": @@ -169,6 +206,14 @@ def emit_cmake(cases, fixture_root): lines.append(f" {cmake_key} {cmake_quote(value)}") lines.append(")") lines.append("") + + for case in cases: + emit_one("add_trace_replay_test_for_backends", case) + # P5's split arm, emitted beside the monolith pair rather than in a block of its own so + # that a case and its variant always carry identical parameters. The CMake side is a + # no-op unless MOBILEGL_BUILD_DISAGGREGATED is ON. + if case.get("split", False): + emit_one("add_trace_replay_split_test", case) return "\n".join(lines) @@ -183,6 +228,7 @@ def parse_args(): "names", "github-test-matrix", "github-verify-matrix", + "github-split-matrix", "github-apk", "github-apk-matrix", "fixture-files", @@ -204,6 +250,8 @@ def main(): print(json.dumps(github_test_matrix(cases), separators=(",", ":"))) elif args.format == "github-verify-matrix": print(json.dumps(github_verify_matrix(cases), separators=(",", ":"))) + elif args.format == "github-split-matrix": + print(json.dumps(github_split_matrix(cases), separators=(",", ":"))) elif args.format == "github-apk": print(json.dumps([github_apk_case(case) for case in cases], separators=(",", ":"))) elif args.format == "github-apk-matrix":