From e5603f9a4668145fc04244073c55b9020e7c68d1 Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Sun, 6 Sep 2026 10:16:15 -0400 Subject: [PATCH] [Fix] (Test, Pipe): probe every arm by content, and let the G7 control's exit status carry what it already knows - All four capability markers are now content probes over the directory the owning package owns, through one helper. The magma pair still read a single hard-coded VertexInputStateFactory.cpp while package D already keeps one of its two Features.PipeHandleAbaControl consumers in Renderer/VulkanRenderer.cpp, so one file move on D's side was a permanent AbaControl skip - the same defect the CSO probe was rewritten for. The DirectGLES probe stops asking whether SlotTables.h exists and asks for kMGPipeSubsystemEsprytSlots, the bit the arm is actually gated on. Every globbed file stays in CMAKE_CONFIGURE_DEPENDS, and the glob is CONFIGURE_DEPENDS. - Verified: with one throwaway header naming each symbol, build-push configures to "keyed on {slot, gen}" / "has an emitter" / "has a consumer" and all four MGITEST_* markers appear 24 times in the generated ctest environments; with the headers gone, all four are back to 0 and the four "will SKIP" verdicts return. The magma sim sat in Renderer/, not in the path the old probe hard-coded. - g7_negative_control.sh no longer exits 0 when the control trips for the wrong reason. A SetterConsistency that had gone red for an unrelated reason satisfied "ctest failed" and never named SetColorMask, and the integrator's D.3 reads this script's rc. The verdict is now taken after the restore and the rebuild - a broken build directory is worse than any exit status - and reported as rc 1 with the output kept, alongside the existing "did not trip" rc 1. - HandleRecycleScenario writes down what the name-recycle proxy costs: the AbaControl arm asserts corruption that needs the heap BLOCK back, sees only the NAME, and so can red an always-on integration-gpu lane for an allocator reason. That trade is deliberate - the alternative is an arm that is green on the day the reproducer stops reproducing - and the consequence is now written both in the header and at the skip that is the last thing standing between the two. --- MobileGL/MG_IntegrationTest/CMakeLists.txt | 119 ++++++++++-------- .../Scenarios/HandleRecycleScenario.cpp | 19 +++ scripts/g7_negative_control.sh | 31 ++++- 3 files changed, 114 insertions(+), 55 deletions(-) mode change 100755 => 100644 scripts/g7_negative_control.sh diff --git a/MobileGL/MG_IntegrationTest/CMakeLists.txt b/MobileGL/MG_IntegrationTest/CMakeLists.txt index 6c9c6b0e..29fd7654 100644 --- a/MobileGL/MG_IntegrationTest/CMakeLists.txt +++ b/MobileGL/MG_IntegrationTest/CMakeLists.txt @@ -375,36 +375,54 @@ endif() # So the whole block sits under the same `if (MOBILEGL_PIPE_PUSH)` as MGITEST_PIPE_PUSH_BUILD, and # HandleRecycleScenario re-checks that marker before either arm asserts, so a hand-forced # environment cannot arm an arm this build does not have either. -if (MOBILEGL_PIPE_PUSH) - file(GLOB MGL_ITEST_ESPRYT_SLOT_TABLES CONFIGURE_DEPENDS - "${MGL_ITEST_ROOT}/MobileGL/MG_Backend/DirectGLES/SlotTables.h") - if (MGL_ITEST_ESPRYT_SLOT_TABLES) - message(STATUS "Integration tests: DirectGLES is keyed on {slot, gen} (SlotTables.h present)") - list(APPEND MGL_ITEST_CAPABILITY_ENV "MGITEST_HANDLE_REKEY_DirectGLES=1") - else() - message(STATUS "Integration tests: DirectGLES has no SlotTables.h - HandleRecycle.Handles will SKIP on it") - endif() - - # The CSO counters' EMITTER, probed by content rather than by a filename. The tracker package - # owns its own file layout and may implement the tracker and the cache header-only - today it - # does (MG_Impl/Pipe/{Tracker,CsoCache}.h, no Tracker.cpp) - so a glob for `Tracker.cpp` is a - # probe for a file nobody promised to create, and it would answer "no" forever AFTER the - # package landed, leaving the CSO control skipping with a reason that had become false. What - # the control actually needs is something that emits the two counters it reads, so that is - # what is looked for: any client-side pipe source naming RenderStateCsoMints / Binds. The glob - # is CONFIGURE_DEPENDS (a new file re-runs it) and every file it finds is added to - # CMAKE_CONFIGURE_DEPENDS (an edit to one re-runs it), so neither half can go stale. - file(GLOB MGL_ITEST_PIPE_CLIENT_SOURCES CONFIGURE_DEPENDS - "${MGL_ITEST_ROOT}/MobileGL/MG_Impl/Pipe/*.h" - "${MGL_ITEST_ROOT}/MobileGL/MG_Impl/Pipe/*.cpp") - set(MGL_ITEST_CSO_EMITTER "") - foreach(mglItestPipeSource IN LISTS MGL_ITEST_PIPE_CLIENT_SOURCES) - set_property(DIRECTORY APPEND PROPERTY CMAKE_CONFIGURE_DEPENDS "${mglItestPipeSource}") - file(STRINGS "${mglItestPipeSource}" MGL_ITEST_CSO_HITS REGEX "RenderStateCso(Mints|Binds)") - if (MGL_ITEST_CSO_HITS AND NOT MGL_ITEST_CSO_EMITTER) - set(MGL_ITEST_CSO_EMITTER "${mglItestPipeSource}") +# +# ALL FOUR MARKERS ARE CONTENT PROBES, AND NONE OF THEM NAMES A FILE. A probe for a filename asks +# the wrong question: the owning package chooses its own file layout, so the moment it moves the +# code the probe answers "no" forever and the arm skips with a reason that has become false - a +# test quietly measuring nothing, which is the one outcome this whole scenario exists to prevent. +# The CSO probe was rewritten for exactly that reason once already; the magma probe still read one +# hard-coded .cpp, and package D already keeps one of its two Features.PipeHandleAbaControl +# consumers in a different file of the same directory (Renderer/VulkanRenderer.cpp), so it was one +# refactor away from a permanent AbaControl skip. So all four now ask "does any source in the +# directory the owning package owns name this symbol?", which is the thing each arm actually needs. +# +# Staleness cannot creep in from either side: the GLOB is CONFIGURE_DEPENDS (a file added or +# removed re-runs it) and every file it finds is appended to CMAKE_CONFIGURE_DEPENDS (an edit to +# one re-runs it). +function(mgl_itest_probe_for_symbol outVar directory symbolRegex) + file(GLOB_RECURSE mglItestProbeSources CONFIGURE_DEPENDS + "${directory}/*.h" "${directory}/*.hpp" "${directory}/*.cpp" "${directory}/*.c") + set(mglItestProbeHit "") + foreach(mglItestProbeSource IN LISTS mglItestProbeSources) + set_property(DIRECTORY APPEND PROPERTY CMAKE_CONFIGURE_DEPENDS "${mglItestProbeSource}") + file(STRINGS "${mglItestProbeSource}" mglItestProbeLines REGEX "${symbolRegex}") + if (mglItestProbeLines AND NOT mglItestProbeHit) + set(mglItestProbeHit "${mglItestProbeSource}") endif() endforeach() + set(${outVar} "${mglItestProbeHit}" PARENT_SCOPE) +endfunction() + +if (MOBILEGL_PIPE_PUSH) + # DirectGLES' Track H arm, probed by the subsystem bit it is gated on rather than by + # SlotTables.h existing: the bit is declared in the contract (MG_Pipe/MGPipe.h:77) and the + # backend has to name it to honour MOBILEGL_PIPE_PUSH's default mask, whatever files package C + # spreads the slot tables across. + mgl_itest_probe_for_symbol(MGL_ITEST_ESPRYT_SLOTS + "${MGL_ITEST_ROOT}/MobileGL/MG_Backend/DirectGLES" "kMGPipeSubsystemEsprytSlots") + if (MGL_ITEST_ESPRYT_SLOTS) + message(STATUS "Integration tests: DirectGLES is keyed on {slot, gen} (${MGL_ITEST_ESPRYT_SLOTS})") + list(APPEND MGL_ITEST_CAPABILITY_ENV "MGITEST_HANDLE_REKEY_DirectGLES=1") + else() + message(STATUS "Integration tests: no DirectGLES source names kMGPipeSubsystemEsprytSlots - " + "HandleRecycle.Handles will SKIP on it") + endif() + + # The CSO counters' EMITTER. The tracker package may implement the tracker and the cache + # header-only - today it does (MG_Impl/Pipe/{Tracker,CsoCache}.h, no Tracker.cpp) - so what is + # looked for is what the control actually reads: a source emitting the two counters. + mgl_itest_probe_for_symbol(MGL_ITEST_CSO_EMITTER + "${MGL_ITEST_ROOT}/MobileGL/MG_Impl/Pipe" "RenderStateCso(Mints|Binds)") if (MGL_ITEST_CSO_EMITTER) message(STATUS "Integration tests: the CSO counters have an emitter (${MGL_ITEST_CSO_EMITTER})") list(APPEND MGL_ITEST_CAPABILITY_ENV "MGITEST_PIPE_TRACKER_PRESENT=1") @@ -413,28 +431,29 @@ if (MOBILEGL_PIPE_PUSH) "CsoContentAddressing will SKIP") endif() - set(MGL_ITEST_MAGMA_VERTEX_INPUT - "${MGL_ITEST_ROOT}/MobileGL/MG_Backend/DirectVulkan/Renderer/VertexInputStateFactory.cpp") - if (EXISTS "${MGL_ITEST_MAGMA_VERTEX_INPUT}") - set_property(DIRECTORY APPEND PROPERTY CMAKE_CONFIGURE_DEPENDS "${MGL_ITEST_MAGMA_VERTEX_INPUT}") - file(STRINGS "${MGL_ITEST_MAGMA_VERTEX_INPUT}" MGL_ITEST_MAGMA_REKEY_HITS - REGEX "kMGPipeSubsystemMagmaVertexInput") - file(STRINGS "${MGL_ITEST_MAGMA_VERTEX_INPUT}" MGL_ITEST_MAGMA_ABA_HITS - REGEX "PipeHandleAbaControl") - if (MGL_ITEST_MAGMA_REKEY_HITS) - message(STATUS "Integration tests: DirectVulkan's vertex input is keyed on {slot, gen}") - list(APPEND MGL_ITEST_CAPABILITY_ENV "MGITEST_HANDLE_REKEY_DirectVulkan=1") - else() - message(STATUS "Integration tests: DirectVulkan's vertex input is not re-keyed yet - " - "HandleRecycle.Handles will SKIP on it") - endif() - if (MGL_ITEST_MAGMA_ABA_HITS) - message(STATUS "Integration tests: MOBILEGL_PIPE_HANDLE_ABA_CONTROL has a consumer") - list(APPEND MGL_ITEST_CAPABILITY_ENV "MGITEST_HANDLE_ABA_IMPLEMENTED=1") - else() - message(STATUS "Integration tests: MOBILEGL_PIPE_HANDLE_ABA_CONTROL has no consumer - " - "HandleRecycle.AbaControl will SKIP") - endif() + # DirectVulkan's Track H arm, and the ABA knob's consumer. Both over the whole backend + # directory: the re-key is subsystem 4's bit wherever package D reads it, and the knob has a + # consumer if ANY DirectVulkan source reverts a guard on it - today two do, in two files. + mgl_itest_probe_for_symbol(MGL_ITEST_MAGMA_REKEY + "${MGL_ITEST_ROOT}/MobileGL/MG_Backend/DirectVulkan" "kMGPipeSubsystemMagmaVertexInput") + if (MGL_ITEST_MAGMA_REKEY) + message(STATUS "Integration tests: DirectVulkan's vertex input is keyed on {slot, gen} " + "(${MGL_ITEST_MAGMA_REKEY})") + list(APPEND MGL_ITEST_CAPABILITY_ENV "MGITEST_HANDLE_REKEY_DirectVulkan=1") + else() + message(STATUS "Integration tests: no DirectVulkan source names kMGPipeSubsystemMagmaVertexInput - " + "HandleRecycle.Handles will SKIP on it") + endif() + + mgl_itest_probe_for_symbol(MGL_ITEST_MAGMA_ABA + "${MGL_ITEST_ROOT}/MobileGL/MG_Backend/DirectVulkan" "PipeHandleAbaControl") + if (MGL_ITEST_MAGMA_ABA) + message(STATUS "Integration tests: MOBILEGL_PIPE_HANDLE_ABA_CONTROL has a consumer " + "(${MGL_ITEST_MAGMA_ABA})") + list(APPEND MGL_ITEST_CAPABILITY_ENV "MGITEST_HANDLE_ABA_IMPLEMENTED=1") + else() + message(STATUS "Integration tests: no DirectVulkan source names PipeHandleAbaControl - " + "HandleRecycle.AbaControl will SKIP") endif() else() message(STATUS "Integration tests: pull build - HandleRecycle.{Handles,AbaControl} and " diff --git a/MobileGL/MG_IntegrationTest/Scenarios/HandleRecycleScenario.cpp b/MobileGL/MG_IntegrationTest/Scenarios/HandleRecycleScenario.cpp index 9ef37e27..cc873a28 100644 --- a/MobileGL/MG_IntegrationTest/Scenarios/HandleRecycleScenario.cpp +++ b/MobileGL/MG_IntegrationTest/Scenarios/HandleRecycleScenario.cpp @@ -40,6 +40,19 @@ // SKIPS with that reason rather than passing - the shape MG_Test/State/ObjectLifetimeIdTest.cpp // already uses for exactly this ("inconclusive, not proven"). // +// WHAT THAT PROXY COSTS THE CI LANE, WRITTEN DOWN ON PURPOSE. The name is only a proxy: the +// corruption the AbaControl arm asserts needs the freed HEAP BLOCK to be handed back, and public +// GL cannot see that. So on a run where the allocator returns the name but not the block, the two +// arms behave differently - the correctness arms (Handles, Legacy) still expect correct pixels and +// still pass, but AbaControl expects the corruption and FAILS. It does that inside +// `ctest -L integration-gpu`, a lane P2 requires green (gate G2), so this scenario can red a +// required lane for an allocator reason. That is chosen, not overlooked: an arm that skipped +// whenever it could not prove the ABA would also be green on the day the reproducer stopped +// reproducing one, and "green because nothing was tested" is precisely what this file exists to +// prevent. ObjectLifetimeIdTest makes the opposite choice because it is a unit test with no +// always-on lane behind it. If the arm ever does flake, the fix is a stronger address-reuse proxy +// - a backend counter for "a recycled slot was handed back out" - and not a looser assertion. +// // THREE ARMS, ALL ALWAYS ON (P2 brief D18). The arm is named by MGITEST_HANDLE_ARM, which is a // HARNESS marker - the library never reads it - and the CMake wiring registers one lane per arm: // @@ -472,6 +485,12 @@ void main() { oColor = texture(uTex, vUv); } ConfigureQuadVao(greenVao, greenBuffer); ASSERT_EQ(FirstGLError(), GLenum(GL_NO_ERROR)) << "building the replacement VAO left a GL error behind"; + // The skip below is the LAST thing that can save a run in which the allocator did not + // repeat itself, and it only sees half of what matters: the names. If the names come + // back but the heap blocks do not, execution continues into an assertion the + // AbaControl arm expects to see corrupted pixels from - and that arm then FAILS + // rather than skipping, in an always-on integration-gpu lane. The header says why that + // trade is taken deliberately; this is where the consequence lands. if (greenVao != redVao || greenBuffer != redBuffer) { GTEST_SKIP() << "inconclusive, not proven: the name allocator did not hand both names back " "(vao " << redVao << " -> " << greenVao << ", buffer " << redBuffer << " -> " diff --git a/scripts/g7_negative_control.sh b/scripts/g7_negative_control.sh old mode 100755 new mode 100644 index 5417747a..a551a268 --- a/scripts/g7_negative_control.sh +++ b/scripts/g7_negative_control.sh @@ -12,7 +12,9 @@ # green on a table it had stopped looking at: a walk that silently drove no setters, a hash that # stopped depending on the chunks, an assertion someone loosened. Green tells you nothing about # whether the test can still fail. This script makes it fail, for the one reason it exists to -# catch, and reports a NON-zero ctest as the pass. +# catch, and reports a NON-zero ctest THAT NAMES THE DEMOTED MEMBER'S SETTER as the pass. A red +# for any other reason is reported as inconclusive (rc 1), not as a pass: the script knows the +# difference, so its exit status has to carry it. # # THE BREAK. ColorMasks is moved out of pipeline chunk P1 into a dynamic chunk of its own, by # inserting two boundaries - at ColorMasks and at FramebufferSrgbEnabled - into the boundary @@ -39,9 +41,12 @@ # revert - WITHOUT requiring the test to exist. This is the mechanism # check, not the control; it never reports the control as passed. # -# Exit codes: 0 the control tripped (or, under --verify-patch-only, the patch compiled); -# 1 the control did NOT trip - the test stayed green on a demoted member, which is -# the finding, not an error in this script; +# Exit codes: 0 the control tripped AND named SetColorMask (or, under --verify-patch-only, the +# patch compiled); +# 1 the control did not answer: either the test stayed green on a demoted member, or +# it went red without ever naming SetColorMask, so the red cannot be attributed to +# the demotion. Both are findings about the test, not errors in this script - and +# both leave the tree restored and rebuilt; # 2 the script could not run the control at all (bad arguments, missing test, # a build that was already broken, a failed restore). set -u -o pipefail @@ -216,11 +221,19 @@ if ctest --test-dir "$BUILD_DIR" -R "$TEST_NAME" --no-tests=error --output-on-fa exit 1 fi +# A red is not yet a pass. The control's claim is "demoting ColorMasks makes the setter-consistency +# test fail AND the failure names glColorMask's setter"; a SetterConsistency that had started +# failing for an unrelated reason would satisfy the first half and none of the second, and the +# caller (the integrator's D.3 reads this script's rc) would record it as "the negative control +# passed". So the answer is remembered here and decided at the end - AFTER the restore, because +# leaving a build directory holding the broken table is worse than any exit status. +TRIPPED_FOR_THE_RIGHT_REASON=1 if grep -q 'SetColorMask' "$LOG_DIR/ctest-after.log"; then say "negative control tripped, naming SetColorMask" else + TRIPPED_FOR_THE_RIGHT_REASON=0 say "negative control tripped, but its output does not name SetColorMask - the test failed for" - say "some other reason, so read $LOG_DIR/ctest-after.log before trusting it" + say "some other reason, so this is NOT a pass. Restoring first, then reporting it." grep -m20 -E 'Failure|error|Expected|Actual' "$LOG_DIR/ctest-after.log" >&2 fi @@ -238,5 +251,13 @@ if ! ctest --test-dir "$BUILD_DIR" -R "$TEST_NAME" --no-tests=error \ exit 2 fi +if [ "$TRIPPED_FOR_THE_RIGHT_REASON" = 0 ]; then + cp -f "$LOG_DIR/ctest-after.log" ./g7-negative-control-wrong-reason.log + say "INCONCLUSIVE: $TEST_NAME went red under the demotion but its output never names" + say "SetColorMask, so the red cannot be attributed to the demoted member. The tree is restored" + say "and green again; the failing output is kept at ./g7-negative-control-wrong-reason.log." + exit 1 +fi + say "negative control tripped and the tree is green again" exit 0