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