From ea38cfcb99ea148fc8b9ccf98c24a80ce9718c5b Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Tue, 8 Sep 2026 14:26:02 -0400 Subject: [PATCH] [Fix, Test] (Pipe): read the shader composite band's own counters in the composite leak case, arm the ABA flip from anywhere under a backend, match Espryt's exact refusal line and pin the bit-10-requires-bit-11 arm --- .../DirectVulkan/Renderer/MagmaPipeArms.h | 68 ++++- MobileGL/MG_IntegrationTest/CMakeLists.txt | 116 +++++++-- .../Harness/PipeSlotPeek.cpp | 22 ++ .../MG_IntegrationTest/Harness/PipeSlotPeek.h | 48 +++- .../Scenarios/HandleRecycleScenario.cpp | 198 +++++++++++--- .../ObjectSubsystemControlScenario.cpp | 244 ++++++++++++++++-- .../Scenarios/TextureUploadShapeScenario.cpp | 54 +++- 7 files changed, 635 insertions(+), 115 deletions(-) diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/MagmaPipeArms.h b/MobileGL/MG_Backend/DirectVulkan/Renderer/MagmaPipeArms.h index 79586ef4..a352cc83 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/MagmaPipeArms.h +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/MagmaPipeArms.h @@ -227,22 +227,70 @@ namespace MobileGL::MG_Backend::DirectVulkan { return MG_Config::Features.PipeHandleAbaControl; } - // The per-kind form of the answer above. `kind` is MG_Pipe::MGPipeKind. + // WHICH KINDS THIS BACKEND ACTUALLY KEYS ON {slot, gen}, and therefore which kinds the knob + // above has an identity to defeat at all. `kind` is MG_Pipe::MGPipeKind. // - // Deliberately a SWITCH over the kinds this backend mints rather than a default of "true": - // a kind added to MGPipeKind without a decision here lands in the `default` arm and is - // reported as NOT covered, which is the safe direction - an uncovered kind whose control - // asserts the correct pixels is a control that has not armed yet, while a covered-by-default - // kind whose control asserts a corruption nobody can produce is a red lane. - inline Bool MagmaPipeAbaControlCoversKind(MG_Pipe::MGPipeKind kind) { + // EXHAUSTIVE, WITH NO `default:`, for MG_IntegrationTest/Harness/PipeSlotPeek.cpp's reason: + // a kind added to MGPipeKind without a decision here must be a -Wswitch warning in this + // file rather than a row that silently inherits somebody else's answer. Being wrong in the + // "covered" direction is the expensive one - a control asserting a corruption nobody can + // produce is a permanently red always-on lane - so an undecided kind must never read true, + // and with no `default:` there is no arm for it to read true from. + // + // constexpr AND PINNED BY static_assert BELOW, which is what stops it rotting the way a + // predicate with no caller does: MagmaPipeIdentityTables mints exactly two kinds, the + // asserts say so in both directions, and the file no longer compiles if the tables and this + // statement of them ever part company. (Review F-m5: the earlier form had no caller at all + // and could not make anything red or green.) + inline constexpr Bool MagmaPipeAbaControlKindIsRekeyedHere(MG_Pipe::MGPipeKind kind) { switch (kind) { + // The two MagmaPipeIdentityTables really mints. case MG_Pipe::MGPipeKind::VertexElementsCso: case MG_Pipe::MGPipeKind::Buffer: - return MagmaPipeAbaControlDefeatsIdentity(); - default: - // P4a's six, and every other kind: not minted on this backend, so not defeatable. + return true; + // P4a's six object classes: still reached from their frontend objects on this + // backend (Magma's object paths are P7, ROADMAP.md:24), so there is no key here for + // the knob to defeat. + case MG_Pipe::MGPipeKind::Texture: + case MG_Pipe::MGPipeKind::Renderbuffer: + case MG_Pipe::MGPipeKind::Framebuffer: + case MG_Pipe::MGPipeKind::SamplerCso: + case MG_Pipe::MGPipeKind::SamplerViewCso: + case MG_Pipe::MGPipeKind::ShaderCso: + // ...and everything else this backend does not mint a handle for. + case MG_Pipe::MGPipeKind::None: + case MG_Pipe::MGPipeKind::Xfb: + case MG_Pipe::MGPipeKind::RenderStateCso: + case MG_Pipe::MGPipeKind::Fence: + case MG_Pipe::MGPipeKind::Query: + case MG_Pipe::MGPipeKind::Context: + case MG_Pipe::MGPipeKind::KindCount: return false; } + return false; + } + + static_assert(MagmaPipeAbaControlKindIsRekeyedHere(MG_Pipe::MGPipeKind::VertexElementsCso), + "MagmaPipeIdentityTables mints VertexElementsCso: the knob has an identity to " + "defeat for it"); + static_assert(MagmaPipeAbaControlKindIsRekeyedHere(MG_Pipe::MGPipeKind::Buffer), + "MagmaPipeIdentityTables mints Buffer: the knob has an identity to defeat for it"); + static_assert(!MagmaPipeAbaControlKindIsRekeyedHere(MG_Pipe::MGPipeKind::Texture) && + !MagmaPipeAbaControlKindIsRekeyedHere(MG_Pipe::MGPipeKind::Renderbuffer) && + !MagmaPipeAbaControlKindIsRekeyedHere(MG_Pipe::MGPipeKind::Framebuffer) && + !MagmaPipeAbaControlKindIsRekeyedHere(MG_Pipe::MGPipeKind::SamplerCso) && + !MagmaPipeAbaControlKindIsRekeyedHere(MG_Pipe::MGPipeKind::SamplerViewCso) && + !MagmaPipeAbaControlKindIsRekeyedHere(MG_Pipe::MGPipeKind::ShaderCso), + "P4a's six object classes are not keyed on {slot, gen} on this backend, so " + "HandleRecycleScenario's six AbaControl arms must NOT expect a corruption here. " + "Wiring one of them is what flips this assert, this predicate and that arm - and " + "MG_IntegrationTest's two-symbol probe over MG_Backend/DirectVulkan is what " + "carries the answer into the lane"); + + // The per-kind form of MagmaPipeAbaControlDefeatsIdentity(): true only where there is both a + // key to defeat here AND the operator asked for it. + inline Bool MagmaPipeAbaControlCoversKind(MG_Pipe::MGPipeKind kind) { + return MagmaPipeAbaControlKindIsRekeyedHere(kind) && MagmaPipeAbaControlDefeatsIdentity(); } // The single consumer-table entry every VAO collapses onto while the control is on. Slot diff --git a/MobileGL/MG_IntegrationTest/CMakeLists.txt b/MobileGL/MG_IntegrationTest/CMakeLists.txt index d2e295c5..50cf763f 100644 --- a/MobileGL/MG_IntegrationTest/CMakeLists.txt +++ b/MobileGL/MG_IntegrationTest/CMakeLists.txt @@ -408,7 +408,9 @@ function(mgl_itest_probe_for_symbol outVar directory symbolRegex) set(${outVar} "${mglItestProbeHit}" PARENT_SCOPE) endfunction() -# The same probe, over a CONJUNCTION: the first source in the directory that names BOTH regexes. +# The same probe, over a CONJUNCTION: BOTH regexes matched ANYWHERE UNDER the directory, not +# necessarily in the same file. outVar is set to " + " when both +# were found and to the empty string otherwise. # # P4a needs it for exactly one question and the question cannot be asked any other way. "Does # MOBILEGL_PIPE_HANDLE_ABA_CONTROL defeat the identity of P4a's OBJECT kinds on this backend?" is @@ -418,23 +420,46 @@ endfunction() # program, and every one of them would assert a corruption nothing on the tree can produce - a hard # red on an always-on integration-gpu lane. Nor is it answered by "some source names a P4a subsystem # bit", which will become true for a backend that honours the mask long before anyone wires the -# knob. What the arm actually needs is ONE SOURCE THAT DOES BOTH, and that is what this asks. +# knob. What the arm needs is BOTH FACTS TO BE TRUE OF THE BACKEND. +# +# DIRECTORY-WIDE RATHER THAN PER FILE, and that is review finding F-M5 rather than a preference. +# Requiring one file to carry both makes the arming depend on the FILE LAYOUT a later package +# chooses: a backend that wires the knob in Managers.cpp while its P4a subsystem constants live in +# SlotTables.h satisfies the question and fails the probe, the six controls keep printing wired=0 +# and asserting the correct pixels, and NOTHING fails, warns or records that the expected flip did +# not happen - the one failure mode a control whose flip is in the future has. The false-positive +# this trades against is a backend that reads the knob somewhere and names a P4a bit somewhere else +# without connecting them; that costs a red lane an engineer must look at, which is the direction +# that gets noticed. Both spellings of the answer are printed, so the configure log says which file +# supplied which half. # # Same staleness guarantees as the single-regex probe: CONFIGURE_DEPENDS on the glob, and every file # it finds appended to CMAKE_CONFIGURE_DEPENDS. function(mgl_itest_probe_for_two_symbols outVar directory symbolRegexA symbolRegexB) file(GLOB_RECURSE mglItestProbeSources CONFIGURE_DEPENDS "${directory}/*.h" "${directory}/*.hpp" "${directory}/*.cpp" "${directory}/*.c") - set(mglItestProbeHit "") + set(mglItestProbeHitA "") + set(mglItestProbeHitB "") foreach(mglItestProbeSource IN LISTS mglItestProbeSources) set_property(DIRECTORY APPEND PROPERTY CMAKE_CONFIGURE_DEPENDS "${mglItestProbeSource}") - file(STRINGS "${mglItestProbeSource}" mglItestProbeLinesA REGEX "${symbolRegexA}") - file(STRINGS "${mglItestProbeSource}" mglItestProbeLinesB REGEX "${symbolRegexB}") - if (mglItestProbeLinesA AND mglItestProbeLinesB AND NOT mglItestProbeHit) - set(mglItestProbeHit "${mglItestProbeSource}") + if (NOT mglItestProbeHitA) + file(STRINGS "${mglItestProbeSource}" mglItestProbeLinesA REGEX "${symbolRegexA}") + if (mglItestProbeLinesA) + set(mglItestProbeHitA "${mglItestProbeSource}") + endif() + endif() + if (NOT mglItestProbeHitB) + file(STRINGS "${mglItestProbeSource}" mglItestProbeLinesB REGEX "${symbolRegexB}") + if (mglItestProbeLinesB) + set(mglItestProbeHitB "${mglItestProbeSource}") + endif() endif() endforeach() - set(${outVar} "${mglItestProbeHit}" PARENT_SCOPE) + if (mglItestProbeHitA AND mglItestProbeHitB) + set(${outVar} "${mglItestProbeHitA} + ${mglItestProbeHitB}" PARENT_SCOPE) + else() + set(${outVar} "" PARENT_SCOPE) + endif() endfunction() if (MOBILEGL_PIPE_PUSH) @@ -545,10 +570,13 @@ if (MOBILEGL_PIPE_PUSH) "HandleRecycle's six P4a cases will SKIP their Handles arm on it") endif() - # ...and whether the ABA knob reaches those kinds THERE. A conjunction, for the reason - # mgl_itest_probe_for_two_symbols states: reading the knob is not the same as steering - # P4a's keys with it, and arming this marker on the weaker evidence turns six always-on - # entries into a permanent red. + # ...and whether the ABA knob reaches those kinds THERE. A conjunction over the WHOLE + # backend directory, for the reason mgl_itest_probe_for_two_symbols states: reading the + # knob is not the same as steering P4a's keys with it, so the weaker single-regex evidence + # would turn six always-on entries into a permanent red - but requiring one FILE to carry + # both halves would let a backend satisfy the question and miss the probe, and the six + # controls would then keep asserting the correct pixels with nothing recording that the + # flip was forgotten (F-M5). mgl_itest_probe_for_two_symbols(MGL_ITEST_OBJECT_ABA "${MGL_ITEST_ROOT}/MobileGL/MG_Backend/${mglItestObjectBackend}" "PipeHandleAbaControl" @@ -559,10 +587,11 @@ if (MOBILEGL_PIPE_PUSH) list(APPEND MGL_ITEST_CAPABILITY_ENV "MGITEST_HANDLE_ABA_OBJECTS_${mglItestObjectBackend}=1") else() - message(STATUS "Integration tests: no ${mglItestObjectBackend} source both reads " - "Features.PipeHandleAbaControl and names a P4a subsystem bit - " - "HandleRecycle's six P4a cases will assert the CORRECT pixels on the " - "AbaControl arm and say that it is not a control for them yet") + message(STATUS "Integration tests: MobileGL/MG_Backend/${mglItestObjectBackend} does not " + "both read Features.PipeHandleAbaControl and name a P4a subsystem bit " + "somewhere under it - HandleRecycle's six P4a cases will assert the " + "CORRECT pixels on the AbaControl arm and say that it is not a control " + "for them yet") endif() endforeach() @@ -583,6 +612,26 @@ if (MOBILEGL_PIPE_PUSH) "ObjectSubsystemControl's emission case will SKIP") endif() + # ...and the CLIENT half of the texture-upload shape, separately, because it is a different + # question with a different owner's file behind it (review F-m7). ctu= is emitted by PipeStats + # in EVERY push build whether or not anything increments it, so "the counter read zero" and + # "no client emitter exists" are the same number and TextureUploadShape cannot tell them apart + # from the summary line alone. Once package B's texture emitter lands, an emitter that stopped + # emitting would read exactly like no emitter at all and the two-sided assertion the scenario + # exists for would pass while comparing nothing. This probe is what separates them. + mgl_itest_probe_for_symbol(MGL_ITEST_CLIENT_TEXTURE_UPLOAD_EMITTER + "${MGL_ITEST_ROOT}/MobileGL/MG_Impl/Pipe" "ClientTextureUploadEmissions") + if (MGL_ITEST_CLIENT_TEXTURE_UPLOAD_EMITTER) + message(STATUS "Integration tests: the client texture-upload counter has an emitter " + "(${MGL_ITEST_CLIENT_TEXTURE_UPLOAD_EMITTER}) - TextureUploadShape's " + "two-sided assertion is live") + list(APPEND MGL_ITEST_CAPABILITY_ENV "MGITEST_PIPE_CLIENT_TEXTURE_UPLOAD_EMITTER_PRESENT=1") + else() + message(STATUS "Integration tests: no MG_Impl/Pipe source emits " + "ClientTextureUploadEmissions - TextureUploadShape records the SERVER shape " + "only and asserts that ctu= is zero") + endif() + mgl_itest_probe_for_symbol(MGL_ITEST_MAGMA_ABA "${MGL_ITEST_ROOT}/MobileGL/MG_Backend/DirectVulkan" "PipeHandleAbaControl") if (MGL_ITEST_MAGMA_ABA) @@ -1351,13 +1400,20 @@ gtest_discover_tests(MobileGLIntegrationTest # constant P3a shipped survives as P4a's A/B control, and a lane that spelled its own bit pattern # would stop being the shape that ships the first time the default moved. # -# THE THIRD LANE IS THE DEPENDENCY REFUSAL (D-K2) and it is the one entry in this family that is not -# vacuous before the emitters land: 0x9ff sets the sampler subsystem (bit 11) WITHOUT the texture -# resource subsystem (bit 10) that every MGPBoundView::Texture and MGPImageView::Res depends on, and -# the bring-up must log ONE error naming both bits and run the legacy sampler arm. That is a -# decision made from the bitmask alone, so it is assertable on a tree where nothing emits yet - and -# a half-honoured mask is invisible in the pixels by construction, which is why it needs an entry -# rather than a code comment. +# THE THIRD AND FOURTH LANES ARE THE DEPENDENCY REFUSALS (D-K2) and they are the entries in this +# family that are not vacuous before the emitters land, because both are decisions made from the +# bitmask alone. A half-honoured mask is invisible in the pixels by construction, which is why each +# needs an entry rather than a code comment. +# +# 0x9ff sets the sampler subsystem (bit 11) WITHOUT the texture resource subsystem (bit 10) that +# every MGPBoundView::Texture and MGPImageView::Res depends on. +# 0x5ff sets the texture resource subsystem (bit 10) WITHOUT the sampler subsystem (bit 11): +# D-K2's fourth row (ID-15). MGPTextureParams::BuiltinSampler is a SamplerCso handle and +# only bit 11 mints sampler CSOs, so bit 10 alone would emit a null there and the applier's +# Fatal is the next thing that happens. The brief called this pair harmless; P4a as built +# says otherwise, and the refusal belongs in the texture family's arm resolver. +# +# In both, the bring-up must log ONE error naming both bits and run the legacy arm. # # ONE CASE PER LANE, through TEST_FILTER, and it is a constraint rather than a preference: each case # READS the library's log and the log is a per-LANE resource (the library opens it fopen(path, "w"), @@ -1387,6 +1443,11 @@ mgl_itest_join_environment(MGL_ITEST_GLES_OBJECT_SUBSYSTEM_REFUSED_ENVIRONMENT "MOBILEGL_PIPE_PUSH=0x9ff" "MOBILEGL_PIPE_STATS=1" "MOBILEGL_PIPE_STATS_PERIOD=1" "MOBILEGL_LOG_FILE_PATH=${CMAKE_CURRENT_BINARY_DIR}/object-subsystem-refused-DirectGLES.log" ${MGL_ITEST_CAPABILITY_ENV} ${MGL_ITEST_COMMON_ENV}) +mgl_itest_join_environment(MGL_ITEST_GLES_OBJECT_SUBSYSTEM_REFUSED_TEXTURE_ENVIRONMENT + "MOBILEGL_BACKEND_TYPE=DirectGLES" "MGITEST_OBJECT_SUBSYSTEM_LANE=refused-texture" + "MOBILEGL_PIPE_PUSH=0x5ff" "MOBILEGL_PIPE_STATS=1" "MOBILEGL_PIPE_STATS_PERIOD=1" + "MOBILEGL_LOG_FILE_PATH=${CMAKE_CURRENT_BINARY_DIR}/object-subsystem-refused-texture-DirectGLES.log" + ${MGL_ITEST_CAPABILITY_ENV} ${MGL_ITEST_COMMON_ENV}) gtest_discover_tests(MobileGLIntegrationTest TEST_PREFIX "DirectGLES.ObjectSubsystemControl.On." @@ -1415,6 +1476,15 @@ gtest_discover_tests(MobileGLIntegrationTest TIMEOUT ${MGL_ITEST_TIMEOUT} ENVIRONMENT "${MGL_ITEST_GLES_OBJECT_SUBSYSTEM_REFUSED_ENVIRONMENT}" ) +gtest_discover_tests(MobileGLIntegrationTest + TEST_PREFIX "DirectGLES.ObjectSubsystemControl.RefusedTexture." + TEST_FILTER "ObjectSubsystemControlScenario.ATextureBitWithoutTheSamplerBitIsRefusedAndNamed" + DISCOVERY_TIMEOUT 30 + PROPERTIES + LABELS integration-gpu + TIMEOUT ${MGL_ITEST_TIMEOUT} + ENVIRONMENT "${MGL_ITEST_GLES_OBJECT_SUBSYSTEM_REFUSED_TEXTURE_ENVIRONMENT}" +) # --- The texture upload shape: RECORDED, NOT GATED in P4a (D-D4) ---------------------- # diff --git a/MobileGL/MG_IntegrationTest/Harness/PipeSlotPeek.cpp b/MobileGL/MG_IntegrationTest/Harness/PipeSlotPeek.cpp index 562a641c..3da241fd 100644 --- a/MobileGL/MG_IntegrationTest/Harness/PipeSlotPeek.cpp +++ b/MobileGL/MG_IntegrationTest/Harness/PipeSlotPeek.cpp @@ -49,12 +49,34 @@ namespace MGITest { bool PeekPipeSlotHighWater(PipeSlotKind kind, unsigned* outHighWater) { if (outHighWater == nullptr) return false; + // The ORDINARY space only, for every kind including ShaderCso (contract-v2.md 4.3). *outHighWater = static_cast(MobileGL::MG_Pipe::MGPipeSlots().HighWater(Translate(kind))); return true; } + + bool PeekPipeCompositeSlotLiveCount(unsigned* outLive) { + if (outLive == nullptr) return false; + *outLive = static_cast(MobileGL::MG_Pipe::MGPipeSlots().CompositeLiveCount()); + return true; + } + + bool PeekPipeCompositeSlotHighWater(unsigned* outHighWater) { + if (outHighWater == nullptr) return false; + *outHighWater = static_cast(MobileGL::MG_Pipe::MGPipeSlots().CompositeHighWater()); + return true; + } + + bool PeekPipeCompositeSlotBandBase(unsigned* outBandBase) { + if (outBandBase == nullptr) return false; + *outBandBase = static_cast(MobileGL::MG_Pipe::kMGPipeShaderCsoCompositeSlotBase); + return true; + } #else bool PeekPipeSlotLiveCount(PipeSlotKind, unsigned*) { return false; } bool PeekPipeSlotHighWater(PipeSlotKind, unsigned*) { return false; } + bool PeekPipeCompositeSlotLiveCount(unsigned*) { return false; } + bool PeekPipeCompositeSlotHighWater(unsigned*) { return false; } + bool PeekPipeCompositeSlotBandBase(unsigned*) { return false; } #endif } // namespace MGITest diff --git a/MobileGL/MG_IntegrationTest/Harness/PipeSlotPeek.h b/MobileGL/MG_IntegrationTest/Harness/PipeSlotPeek.h index c02ec7c1..530d36a3 100644 --- a/MobileGL/MG_IntegrationTest/Harness/PipeSlotPeek.h +++ b/MobileGL/MG_IntegrationTest/Harness/PipeSlotPeek.h @@ -45,13 +45,20 @@ namespace MGITest { SamplerCso, SamplerViewCso, // ShaderCso covers BOTH the ordinary program slots and the program-pipeline COMPOSITES - // minted out of the reserved high band (MGPipeHandles.h:86-97, D-H7). One kind, because - // that is what the allocator has: the band is a second dense table inside the same - // kind, LiveCount counts both and HighWater is one past the highest slot handed out in - // either. The composite's leak case is a separate CASE rather than a separate kind for - // that reason - what makes it its own case is that a composite's slot has TWO - // independent release paths (the pipeline cache's LRU eviction and the composite - // ProgramObject's destructor), not that it is counted anywhere else. + // minted out of the reserved high band (MGPipeHandles.h:86-107, D-H7). One kind, because + // that is what the allocator has: the band is a second dense table inside the same kind + // and LiveCount counts both. + // + // THE TWO SPACES' HIGH-WATER MARKS ARE NOT ONE NUMBER, and the correction matters here + // more than anywhere else. c0b split them (contract-v2.md 4.3): HighWater(ShaderCso) is + // now the ORDINARY space only and the band's own mark is CompositeHighWater(), because + // a merged mark is pinned at ~983k from the first composite mint onward and every "the + // high-water mark did not move over N churn rounds" assertion about ordinary programs + // would be vacuously true for the rest of the process. The composite's leak case is a + // separate CASE and reads the BAND'S OWN counters below (PeekPipeCompositeSlot*) - a + // composite's slot has TWO independent release paths (the pipeline cache's LRU eviction + // and the composite ProgramObject's destructor), and a slot that never comes back to + // the band moves neither of the ordinary numbers. ShaderCso, }; @@ -64,4 +71,31 @@ namespace MGITest { bool PeekPipeSlotLiveCount(PipeSlotKind kind, unsigned* outLive); bool PeekPipeSlotHighWater(PipeSlotKind kind, unsigned* outHighWater); + // The ShaderCso COMPOSITE BAND's own three numbers, the seventh..ninth members + // contract-v2.md 4.3 asks this header for. There is no `kind` argument because the band is + // ShaderCso's alone - AllocateComposite is the one door into it and no other kind has one. + // All three return false on the same terms as the two above, and a caller that gets false + // must SKIP. + // + // PeekPipeCompositeSlotLiveCount = MGPipeSlotAllocator::CompositeLiveCount(), the band's + // share of LiveCount(ShaderCso). + // PeekPipeCompositeSlotHighWater = CompositeHighWater() VERBATIM, i.e. one past the + // highest band slot ever handed out. It is an ABSOLUTE + // slot number and therefore starts at the band's base, + // not at zero - "no composite was ever minted" reads as + // `high water == band base`, which is what the third + // member is for. It is not returned base-relative + // because a peek whose name says HighWater and whose + // value is a delta is exactly the kind of quietly + // redefined counter this member exists to correct. + // PeekPipeCompositeSlotBandBase = kMGPipeShaderCsoCompositeSlotBase, the floor the + // other two are read against. A constant, but it + // reaches a scenario only through this header: the + // MG_Pipe headers and the GL headers are not meant to + // meet in one translation unit, which is why this + // harness exists at all. + bool PeekPipeCompositeSlotLiveCount(unsigned* outLive); + bool PeekPipeCompositeSlotHighWater(unsigned* outHighWater); + bool PeekPipeCompositeSlotBandBase(unsigned* outBandBase); + } // namespace MGITest diff --git a/MobileGL/MG_IntegrationTest/Scenarios/HandleRecycleScenario.cpp b/MobileGL/MG_IntegrationTest/Scenarios/HandleRecycleScenario.cpp index 415d9506..045f9dc7 100644 --- a/MobileGL/MG_IntegrationTest/Scenarios/HandleRecycleScenario.cpp +++ b/MobileGL/MG_IntegrationTest/Scenarios/HandleRecycleScenario.cpp @@ -173,6 +173,78 @@ namespace MGITest { bool RunningInAHandleRecycleLane() { return std::getenv(kArmMarker) != nullptr; } + // Does THIS LANE run with a live client slot allocator behind it - i.e. did it pin a + // non-zero MOBILEGL_PIPE_PUSH? + // + // The leak cases below need this and not the arm (review F-m4). The arm says which key a + // pixel assertion is about; the leak assertion is not about a key at all, it is about the + // allocator, and "the allocator has slots to leak" is exactly "the mask is not zero". The + // two questions almost coincide - DirectGLES/DirectVulkan.HandleRecycle.Legacy. and + // DirectVulkan.HandleRecycle.AbaControl. do pin MOBILEGL_PIPE_PUSH=0 - but + // DirectVulkan.HandleRecycle.AbaControlHandles. pins the shipping 0x1fff WITH a live + // allocator, and gating on `arm == Handles` declined it while telling the reader the + // lane had no allocator, which was false. Reading the lane's own pin covers all three. + // + // The ENVIRONMENT is the right place to read it from and the library's config is not: + // MG_Config is inside the library, this module links the shipping .so on Android, and the + // pin is the LANE's statement about what it configured. An entry that pinned nothing (the + // ambient ones) is not a lane and answers false - the build default may well be non-zero + // there, but an ambient entry configured no arm, no allocator expectation and no private + // log, which is the reason the whole file declines them. + bool LanePinnedALiveAllocator() { + const char* mask = std::getenv("MOBILEGL_PIPE_PUSH"); + if (mask == nullptr || mask[0] == '\0') return false; + // strtoull handles the 0x form every lane spells it in, and a value this module + // cannot parse is treated as "no pin" rather than as a non-zero mask. + char* end = nullptr; + const unsigned long long value = std::strtoull(mask, &end, 0); + return end != nullptr && *end == '\0' && value != 0ull; + } + + // ---- which of the allocator's TWO spaces a leak case measures -------------------- + // + // Every kind but ShaderCso has one space. ShaderCso has two: the ordinary program slots, + // and the reserved high band the program-pipeline COMPOSITES are minted out of through + // the allocator's one door, AllocateComposite (D-H7). c0b split their high-water marks + // (contract-v2.md 4.3) precisely so that a leak case can be written about either, and + // the composite's case has to read the BAND's - a band slot that never comes back moves + // neither of the ordinary numbers, which is the review's F-M4: the case would have + // reported green having never looked at the thing it exists for. + // + // Stated at every call site rather than defaulted, for PipeSlotPeek's `no default:` + // reason: a new leak case must say which space it is about, because the wrong answer is + // a green that asserts nothing rather than a compile error. + enum class SlotSpace { + Ordinary, + CompositeBand, + }; + + const char* SpaceSuffix(SlotSpace space) { + return space == SlotSpace::CompositeBand ? " [composite band]" : ""; + } + + bool ReadSpaceLiveCount(PipeSlotKind kind, SlotSpace space, unsigned* out) { + return space == SlotSpace::CompositeBand ? MGITest::PeekPipeCompositeSlotLiveCount(out) + : MGITest::PeekPipeSlotLiveCount(kind, out); + } + + bool ReadSpaceHighWater(PipeSlotKind kind, SlotSpace space, unsigned* out) { + return space == SlotSpace::CompositeBand ? MGITest::PeekPipeCompositeSlotHighWater(out) + : MGITest::PeekPipeSlotHighWater(kind, out); + } + + // The value a space's high-water mark has when NOTHING of it was ever handed out: 0 for + // the ordinary space, and the band's BASE for the band, because CompositeHighWater() is + // an absolute slot number. Reading this wrong is what would turn the band's "nothing was + // ever minted" skip into a silent pass on a tree that mints composites. + bool ReadSpaceHighWaterFloor(SlotSpace space, unsigned* out) { + if (space != SlotSpace::CompositeBand) { + *out = 0; + return true; + } + return MGITest::PeekPipeCompositeSlotBandBase(out); + } + const char* ArmName(Arm arm) { switch (arm) { case Arm::Handles: return "Handles"; @@ -622,28 +694,49 @@ void main() { oColor = texture(uTex, vUv); } // vacuous - which is the one of the three that catches a death path that works but // runs at the wrong time (a deferred queue, a frame-boundary sweep). // - // `maxInFlight` is how many slots of the kind one round may legitimately hold at its - // peak: 1 where the round creates one object, more where the round creates several of - // the same kind (a pipeline composite's round also creates its two stage programs, and - // all three are ShaderCsos). + // `space` says WHICH of the allocator's two spaces the three assertions are about, and + // it is the difference between an assertion and a green that reads the wrong counter: + // only ShaderCso has two, and only the composite case is about the band. + // + // `maxInFlight` is how many slots of the kind IN THAT SPACE one round may legitimately + // hold at its peak: 1 where the round creates one object of it, more where the round + // creates several. The composite round creates three ShaderCsos - two stage programs + // and the composite they are flattened into - but only ONE of the three is a band + // slot, so the band's answer is 1 and the ordinary space's would have been 3. using ChurnRound = std::function& observe)>; - void AssertChurnReturnsEverySlot(PipeSlotKind kind, const char* kindName, + void AssertChurnReturnsEverySlot(PipeSlotKind kind, SlotSpace space, const char* kindName, const char* owner, unsigned maxInFlight, const ChurnRound& round) { - if (m_arm != Arm::Handles) { - GTEST_SKIP() << "the client mints a " << kindName - << " slot only when its subsystem is on, and only the Handles arm " - "pins the shipping mask (0x1fff). The Legacy and AbaControl " - "lanes run MOBILEGL_PIPE_PUSH=0, where there is no allocator to " - "leak from."; + // THE LANE'S OWN PIN, not the arm (F-m4). The Legacy and AbaControl lanes run + // MOBILEGL_PIPE_PUSH=0 and really have no allocator to leak from; the Handles + // lanes and DirectVulkan.HandleRecycle.AbaControlHandles. all pin the shipping + // 0x1fff and do. The old gate declined the third of those while telling the + // reader it had no allocator, which was false, and left one lane's coverage on + // the table. + if (!LanePinnedALiveAllocator()) { + GTEST_SKIP() << "this entry pinned no non-zero MOBILEGL_PIPE_PUSH, so there is " + "no client slot allocator behind it to leak from: the " + "HandleRecycle.Legacy. and HandleRecycle.AbaControl. lanes pin " + "MOBILEGL_PIPE_PUSH=0 on purpose (they are about the pre-handle " + "guards), and the ambient entries configure no lane at all. The " + "lanes that carry this assertion are the two " + "HandleRecycle.Handles. ones and " + "DirectVulkan.HandleRecycle.AbaControlHandles., all of which pin " + "the shipping mask."; } unsigned probe = 0; - if (!MGITest::PeekPipeSlotLiveCount(kind, &probe)) { + if (!ReadSpaceLiveCount(kind, space, &probe)) { GTEST_SKIP() << "the client slot allocator is out of reach from this module (a " "pull build has none, and the Android link resolves no internal " "symbol), so 'could not look' would be reported as 'did not " "leak'"; } + unsigned highWaterFloor = 0; + if (!ReadSpaceHighWaterFloor(space, &highWaterFloor)) { + GTEST_SKIP() << "the composite band's base is out of reach from this module, so " + "'no composite was ever minted' cannot be told apart from 'the " + "band did not grow' and a green here would assert nothing"; + } // TWO WARM-UP ROUNDS BEFORE THE BASELINE IS TAKEN, so what is measured is growth // WITH the churn and not the one-off cost of drawing at all. The first rounds in a @@ -654,7 +747,7 @@ void main() { oColor = texture(uTex, vUv); } unsigned peakLive = 0; const std::function observe = [&]() { unsigned live = 0; - if (MGITest::PeekPipeSlotLiveCount(kind, &live) && live > peakLive) peakLive = live; + if (ReadSpaceLiveCount(kind, space, &live) && live > peakLive) peakLive = live; }; round(/*checkPixels=*/true, observe); round(/*checkPixels=*/false, observe); @@ -662,8 +755,8 @@ void main() { oColor = texture(uTex, vUv); } unsigned liveBefore = 0; unsigned highWaterBefore = 0; - ASSERT_TRUE(MGITest::PeekPipeSlotLiveCount(kind, &liveBefore)); - ASSERT_TRUE(MGITest::PeekPipeSlotHighWater(kind, &highWaterBefore)); + ASSERT_TRUE(ReadSpaceLiveCount(kind, space, &liveBefore)); + ASSERT_TRUE(ReadSpaceHighWater(kind, space, &highWaterBefore)); constexpr int kChurn = 48; for (int i = 0; i < kChurn; ++i) round(/*checkPixels=*/false, observe); @@ -671,12 +764,13 @@ void main() { oColor = texture(uTex, vUv); } unsigned liveAfter = 0; unsigned highWaterAfter = 0; - ASSERT_TRUE(MGITest::PeekPipeSlotLiveCount(kind, &liveAfter)); - ASSERT_TRUE(MGITest::PeekPipeSlotHighWater(kind, &highWaterAfter)); + ASSERT_TRUE(ReadSpaceLiveCount(kind, space, &liveAfter)); + ASSERT_TRUE(ReadSpaceHighWater(kind, space, &highWaterAfter)); std::cout << "[ HandleRecycle ] backend=" << Gl().BackendName() << " " << kindName - << " live " << liveBefore << " -> " << liveAfter << " (peak " << peakLive - << "), high water " << highWaterBefore << " -> " << highWaterAfter - << " over " << kChurn << " create/draw/destroy rounds" << std::endl; + << SpaceSuffix(space) << " live " << liveBefore << " -> " << liveAfter + << " (peak " << peakLive << "), high water " << highWaterBefore << " -> " + << highWaterAfter << " (floor " << highWaterFloor << ") over " << kChurn + << " create/draw/destroy rounds" << std::endl; // NOTHING WAS EVER MINTED, which is not "did not leak" and must not be reported as // one. On the P4a contract tree the client emits nothing for any of these kinds - @@ -685,9 +779,9 @@ void main() { oColor = texture(uTex, vUv); } // a kind it never saw. This is the same rule as the peek returning false, applied // to the other way of not being able to look, and it arms itself the moment the // owning package's emitter lands. - if (highWaterAfter == 0 && peakLive == 0 && liveAfter == 0) { + if (highWaterAfter == highWaterFloor && peakLive == 0 && liveAfter == 0) { GTEST_SKIP() << "subsystem not implemented on this tree: the client minted no " - << kindName + << kindName << SpaceSuffix(space) << " slot at all over " << (kChurn + 2) << " create/draw/destroy rounds, so there is nothing here that " "could leak and a green would assert nothing. P4a package " @@ -697,7 +791,7 @@ void main() { oColor = texture(uTex, vUv); } } EXPECT_EQ(liveAfter, liveBefore) - << kChurn << " " << kindName + << kChurn << " " << kindName << SpaceSuffix(space) << " objects were created, drawn with and destroyed and " << (liveAfter - liveBefore) << " slots never came back. Each one holds a SlotState, a lifetime-id map node " @@ -708,12 +802,12 @@ void main() { oColor = texture(uTex, vUv); } "(D-I1) rather than from a backend death table. Backend " << Gl().BackendName(); EXPECT_EQ(highWaterAfter, highWaterBefore) - << "the " << kindName + << "the " << kindName << SpaceSuffix(space) << " slot space grew with the churn instead of recycling the slot the warm-up " "rounds already handed out; the frees are not reaching the allocator's free " "list"; EXPECT_LE(peakLive > liveBefore ? peakLive - liveBefore : 0u, maxInFlight) - << "more than " << maxInFlight << " churned " << kindName + << "more than " << maxInFlight << " churned " << kindName << SpaceSuffix(space) << " object(s) were live at the allocator at once, so the deaths are arriving " "late rather than at the destructor"; } @@ -1235,11 +1329,20 @@ void main() { oColor = texture(uTex, vUv); } // The corruption IS the assertion: with the identity half of the framebuffer memo // key defeated, the replacement inherits the dead framebuffer's record and its // clear lands on the dead one's attachment. - EXPECT_NE(deadIsNotRed, 0) + // sawStale, not `deadIsNotRed != 0` (review F-m12): the weaker form is satisfied + // by a dead attachment full of GARBAGE, which is not the observation this control + // claims. sawStale is the predicate the case already computes and prints - the + // dead attachment is now the REPLACEMENT'S green, i.e. the replacement's clear + // landed there - so the assertion and the printed line say the same thing. + EXPECT_TRUE(sawStale) << "[AbaControl expects the STALE framebuffer] the DEAD framebuffer's " - "attachment is still the red it was cleared to in the warm-up, so the " - "replacement's clear went to its own attachment after all and the ABA was " - "not reproduced - the control has stopped controlling anything."; + "attachment did not come back as the replacement's green, so the " + "replacement's clear did not land on it and the ABA was not reproduced - " + "the control has stopped controlling anything. Texels that are neither the " + "warm-up red nor the replacement green are a third answer and are not a " + "reproduction either (" + << deadIsNotRed << " of " << (deadTexels.size() / 4) + << " dead texels are not red)."; } else { EXPECT_EQ(replacementIsNotGreen, 0) << "the replacement framebuffer's own attachment is not the colour it was " @@ -1807,7 +1910,8 @@ void main() { oColor = vec4(0.0, 1.0, 0.0, 1.0); } ConfigureQuadVao(vao, buffer); AssertChurnReturnsEverySlot( - PipeSlotKind::Texture, "Texture", "B (clientfb)", /*maxInFlight=*/1u, + PipeSlotKind::Texture, SlotSpace::Ordinary, "Texture", "B (clientfb)", + /*maxInFlight=*/1u, [&](bool checkPixels, const std::function& observe) { const GLuint texture = MakeSolidTexture(0, 255, 0); const Image image = DrawTexturedQuadAndRead(vao, texture); @@ -1845,7 +1949,8 @@ void main() { oColor = vec4(0.0, 1.0, 0.0, 1.0); } // DeleteSamplerState), which is precisely why it needs a case of its own: a helper // that forgot one of its three frees leaks only that kind and nothing else moves. AssertChurnReturnsEverySlot( - PipeSlotKind::SamplerViewCso, "SamplerViewCso", "C (clientsp)", /*maxInFlight=*/1u, + PipeSlotKind::SamplerViewCso, SlotSpace::Ordinary, "SamplerViewCso", "C (clientsp)", + /*maxInFlight=*/1u, [&](bool checkPixels, const std::function& observe) { const GLuint texture = MakeSolidTexture(0, 255, 0); const Image image = DrawTexturedQuadAndRead(vao, texture); @@ -1874,7 +1979,8 @@ void main() { oColor = vec4(0.0, 1.0, 0.0, 1.0); } glGenFramebuffers(1, &fbo); AssertChurnReturnsEverySlot( - PipeSlotKind::Renderbuffer, "Renderbuffer", "B (clientfb)", /*maxInFlight=*/1u, + PipeSlotKind::Renderbuffer, SlotSpace::Ordinary, "Renderbuffer", "B (clientfb)", + /*maxInFlight=*/1u, [&](bool checkPixels, const std::function& observe) { GLuint renderbuffer = 0; glGenRenderbuffers(1, &renderbuffer); @@ -1920,7 +2026,8 @@ void main() { oColor = vec4(0.0, 1.0, 0.0, 1.0); } // allocator is therefore the only observable this kind has, which makes this case the // whole of its lifetime coverage rather than a supplement to a wire assertion. AssertChurnReturnsEverySlot( - PipeSlotKind::Framebuffer, "Framebuffer", "B (clientfb)", /*maxInFlight=*/1u, + PipeSlotKind::Framebuffer, SlotSpace::Ordinary, "Framebuffer", "B (clientfb)", + /*maxInFlight=*/1u, [&](bool checkPixels, const std::function& observe) { GLuint fbo = 0; glGenFramebuffers(1, &fbo); @@ -1959,7 +2066,8 @@ void main() { oColor = vec4(0.0, 1.0, 0.0, 1.0); } // path. The LOD bias moves per round, which changes the hash and nothing else. int round = 0; AssertChurnReturnsEverySlot( - PipeSlotKind::SamplerCso, "SamplerCso", "C (clientsp)", /*maxInFlight=*/1u, + PipeSlotKind::SamplerCso, SlotSpace::Ordinary, "SamplerCso", "C (clientsp)", + /*maxInFlight=*/1u, [&](bool checkPixels, const std::function& observe) { GLuint sampler = 0; glGenSamplers(1, &sampler); @@ -2015,7 +2123,8 @@ void main() { oColor = vec4(0.0, 1.0, 0.0, 1.0); } // shader moves per round. int round = 0; AssertChurnReturnsEverySlot( - PipeSlotKind::ShaderCso, "ShaderCso", "C (clientsp)", /*maxInFlight=*/1u, + PipeSlotKind::ShaderCso, SlotSpace::Ordinary, "ShaderCso", "C (clientsp)", + /*maxInFlight=*/1u, [&](bool checkPixels, const std::function& observe) { const std::string fs = "#version 330 core\nout vec4 oColor;\nvoid main() { " "oColor = vec4(0.0, 1.0, 0.0, 1.0) + vec4(" + @@ -2079,12 +2188,19 @@ void main() { )"; int round = 0; AssertChurnReturnsEverySlot( - PipeSlotKind::ShaderCso, "ShaderCso (pipeline composites)", "C (clientsp)", - // TWO in flight: a round creates two stage programs and the composite the pipeline - // flattens them into, and the two stage programs are ordinary ShaderCsos of the - // same kind. The composite is the third, and it is the one whose two release paths - // this case is about. - /*maxInFlight=*/3u, + PipeSlotKind::ShaderCso, + // THE BAND'S OWN COUNTERS, not the ordinary ShaderCso space's (review F-M4, + // contract-v2.md 4.3/7.6). A round creates THREE ShaderCsos - the two stage + // programs and the composite the pipeline flattens them into - and the two stage + // programs are ORDINARY slots. So the ordinary space moves in this case whether + // or not a composite ever comes back, the "nothing was ever minted" skip would + // not fire, and every assertion below would have been a statement about the two + // stage programs while the band - the double-free-refusal case the band exists to + // police - went unread. + SlotSpace::CompositeBand, "ShaderCso (pipeline composites)", "C (clientsp)", + // ONE in flight, because in the BAND a round holds exactly one slot: the + // composite. (Against the ordinary space the answer would have been three.) + /*maxInFlight=*/1u, [&](bool checkPixels, const std::function& observe) { // A DIFFERENT FRAGMENT STAGE PER ROUND: the composite is keyed on // ProgramPipelineObject::ComputeDrawProgramSignature(), the per-stage diff --git a/MobileGL/MG_IntegrationTest/Scenarios/ObjectSubsystemControlScenario.cpp b/MobileGL/MG_IntegrationTest/Scenarios/ObjectSubsystemControlScenario.cpp index e934717f..e94002ef 100644 --- a/MobileGL/MG_IntegrationTest/Scenarios/ObjectSubsystemControlScenario.cpp +++ b/MobileGL/MG_IntegrationTest/Scenarios/ObjectSubsystemControlScenario.cpp @@ -48,6 +48,20 @@ // that the refusal is NAMED and that the run then produces the same pixels as any other // lane: a refusal that half-ran, or that aborted, would both be failures here. // +// refused-texture (MOBILEGL_PIPE_PUSH=0x5ff = bits 0..8 plus bit 10, texture resources, WITHOUT +// bit 11) +// D-K2's FOURTH row (ID-15), and the direction the brief originally called harmless. +// MGPTextureParams::BuiltinSampler is a SamplerCso HANDLE and only bit 11 mints sampler +// CSOs, so with bit 10 alone every set_texture_params would carry a null there and the +// applier's Fatal{ProtocolCorruption} is the next thing that happens. Same two assertions +// as the lane above, with the two bits' roles swapped. +// +// both refusal lanes +// "NAMED" means ONE LINE of the library's log, at ERROR severity, that says it REFUSED and +// names both bits. Not a substring anywhere in the file: the word "sampler" appears in +// almost any log the sampler path writes to, and an assertion that cannot go red for its +// stated reason is worse than no assertion (review F-M6). +// // every lane // THE PIXELS MUST NOT MOVE. The workload draws one solid-colour quad through a texture, an // explicit sampler object and a user framebuffer, and every lane must read back that colour. @@ -72,6 +86,7 @@ #include #include #include +#include #include #include @@ -97,6 +112,12 @@ namespace MGITest { constexpr const char* kLaneOn = "on"; constexpr const char* kLaneOff = "off"; constexpr const char* kLaneRefused = "refused"; + // D-K2's FOURTH row (ID-15): bit 10 without bit 11. 0x5ff is 0x1ff plus bit 10. + constexpr const char* kLaneRefusedTexture = "refused-texture"; + + bool LaneIsARefusalLane(const std::string& lane) { + return lane == kLaneRefused || lane == kLaneRefusedTexture; + } constexpr int kInset = 2; constexpr int kTextureSize = 4; @@ -134,6 +155,70 @@ void main() { oColor = texture(uTex, vUv); } return lane != nullptr ? std::string(lane) : std::string(); } + // ---- reading the refusal out of the library's own log ------------------------------ + // + // THE UNIT IS A LINE, AND THE LINE HAS TO BE THE REFUSAL (review F-M6). The first cut of + // this asked whether the WHOLE FILE contained a lowercase "sampler" and whether it + // contained "texture resource", anywhere, in any order, at any severity. Both are true of + // almost any log the moment the sampler path says anything at all, so the assertion could + // not go red for the reason it claims and the one P4a control that is not vacuous before + // the emitters land would have been vacuous too. + // + // What is matched instead is one line that is ALL of: + // * at ERROR severity - the library writes "[