From 55d2af9bd1c9097189781838f19ad259afa103ee Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Tue, 8 Sep 2026 00:16:00 -0400 Subject: [PATCH] [Fix, Test] (Magma, MG_IntegrationTest): make the handle-ABA negative control construct its own collision and defeat the {slot, gen} generation - MOBILEGL_PIPE_HANDLE_ABA_CONTROL asserted the corruption and saw correct pixels, so DirectVulkan.HandleRecycle.AbaControl.*AVertexArray* was RED in an always-on integration-gpu lane while every guard it was supposed to be defeating stood. Two measured reasons, neither of them the {slot, gen} re-key: (1) D18 spelled the control as "hash the raw BufferObject* instead of its lifetime id, and skip the vaoLifetimeId compare", which only collides if the allocator hands the freed block back - it does not. glGen* recycles the NAME, but a VertexArrayObject is 3920 bytes, past glibc's tcache, so its chunk goes to the unsorted bin and is split by the next allocation the replacement path makes; four create/delete cycles in one run gave four addresses ~1 MiB apart, and the BufferObject behaves the same. (2) The reproducer put a frame boundary between the arming draw and the recycled draw, and the only memo that carries a GPU slice rather than a layout - ResolvedVertexBindings - declines across frames by design, so no key collision whatsoever could have shown up in pixels. - The control no longer asks the allocator for the collision: on both arms it replaces the object identity in DirectVulkan's vertex-input keys with a constant, which is the strongest form of "the block came back" and is deterministic. Three sites, all behind one question (MagmaPipeAbaControlDefeatsIdentity): the buffer identity leaves VertexInputStateFactory::ComputeHash, VertexInputStateFactory::MemosFor claims one entry without its Owner compare, and VulkanRenderer::LookupVaoDrawMemo hands one entry back uncleared ahead of both arms. - That is what makes the control cover the key P2 SHIPS. Under MOBILEGL_PIPE_PUSH=0 the handle arm is not executed at all, so the old control said nothing about the generation in {slot, gen} - the whole of what makes the re-keyed memos ABA-safe. A second lane, DirectVulkan.HandleRecycle.AbaControlHandles., runs the handle arm with the knob and asserts the same corruption; D18's lane is kept verbatim beside it for the pre-handle arm. - The reproducer's two draws now share a frame, and both buffers are realised before the window, so a moved slice epoch cannot mask the ABA behind a gate that is not about identity. Nothing else is relaxed: the frame serial, the slice epochs and the host-map check stay in force, so a green arm still means "a replacement object was handed its predecessor's resolved vertex bindings because the identity halves of the keys were defeated". - ExpectPixelsFor now prints what it OBSERVED (STALE/FRESH/NEITHER) next to what the arm expected, on every arm and whether or not the case passes. - Knob-off is unchanged and the pull build is untouched: every new branch is #if MOBILEGL_PIPE_PUSH, and symbol_report.py --threshold 0 against the pre-P2 baseline still reports 0 added / 0 removed / 0 renamed and the same four resized symbols (RenderState::RenderState, SetCapability, IsCapabilityEnabled, _GLOBAL__sub_I_DirectGLES.cpp). --- MobileGL/Config.h | 17 +- .../DirectVulkan/Renderer/MagmaPipeArms.h | 51 ++++++ .../Renderer/VertexInputStateFactory.cpp | 32 +++- .../DirectVulkan/Renderer/VulkanRenderer.cpp | 17 ++ MobileGL/MG_IntegrationTest/CMakeLists.txt | 29 +++- .../Scenarios/HandleRecycleScenario.cpp | 154 ++++++++++++------ 6 files changed, 231 insertions(+), 69 deletions(-) diff --git a/MobileGL/Config.h b/MobileGL/Config.h index 279193f3..1bc17800 100644 --- a/MobileGL/Config.h +++ b/MobileGL/Config.h @@ -357,12 +357,17 @@ namespace MobileGL::MG_Config { // Fatal{UnmigratedPipeInput} (negative control B). Unknown name is // Fatal{PipeVerifyBadKnob}. String PipePoisonOmit; - // MOBILEGL_PIPE_HANDLE_ABA_CONTROL (negative control C, P2 brief D18): defeat the - // two guards the {slot, gen} re-key replaces - hash the raw BufferObject* instead - // of its lifetime id, and skip the VAO lifetime-id compare - so - // HandleRecycleScenario.AbaControl reproduces the ABA and asserts the WRONG pixels. - // That is what proves the reproducer still reproduces. Under MOBILEGL_PIPE_PUSH - // only, so it cannot exist in a shipping pull build. + // MOBILEGL_PIPE_HANDLE_ABA_CONTROL (negative control C, P2 brief D18): replace the + // OBJECT IDENTITY in every DirectVulkan vertex-input memo key with a constant, on + // whichever arm the run is on - the pre-handle (address, lifetime id) pair AND the + // handle arm's {slot, gen} generation - so a replacement object inherits its dead + // predecessor's resolved vertex bindings and HandleRecycleScenario.AbaControl asserts + // the WRONG pixels. That is what proves the reproducer still reproduces. D18 wrote + // this as "hash the raw BufferObject* instead of its lifetime id"; measured, the heap + // block is never handed back, so that spelling collided with nothing and the control + // went vacuous - see MagmaPipeArms.h's MagmaPipeAbaControlDefeatsIdentity for the + // measurement and for what the control still leaves standing. Under + // MOBILEGL_PIPE_PUSH only, so it cannot exist in a shipping pull build. Bool PipeHandleAbaControl = false; #endif // MOBILEGL_PIPE_STATS: dump the boundary counters (bytes, calls, roundtrips, diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/MagmaPipeArms.h b/MobileGL/MG_Backend/DirectVulkan/Renderer/MagmaPipeArms.h index f9591b46..e1a54486 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/MagmaPipeArms.h +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/MagmaPipeArms.h @@ -130,6 +130,57 @@ namespace MobileGL::MG_Backend::DirectVulkan { #endif } + // --------------------------------------------------------------------------------- + // Negative control C (P2 brief D18): MOBILEGL_PIPE_HANDLE_ABA_CONTROL + // --------------------------------------------------------------------------------- + // + // "Is the object-identity half of every vertex-input memo key deliberately defeated in + // this run?" - the ONE question the control's sites ask, for the same reason + // MagmaPipeTrackHArmIsHandles exists: three sites deciding separately could disagree, + // and a control that defeats two of three guards proves nothing. + // + // WHAT IT DEFEATS, AND WHY IT IS SPELLED AS "REPLACE THE IDENTITY WITH A CONSTANT" + // RATHER THAN "USE THE HEAP ADDRESS". + // + // D18 wrote the control as "hash attr.Buffer.get() instead of GetLifetimeId(), and skip + // the vaoLifetimeId compare", on the theory that a deleted object's replacement lands at + // the freed heap block and so reproduces the key. Measured, it does not: in + // HandleRecycleScenario the GL NAMES come back (glGen* hands the deleted name straight + // out) but the C++ heap blocks do not - a VertexArrayObject is 3920 bytes, too large for + // glibc's tcache, so its chunk goes to the unsorted bin and is split by the very next + // allocation the replacement path makes. Four create/delete cycles in one run produced + // four distinct addresses, ~1 MiB apart. With no address reuse there is nothing for + // "hash the address" to collide with: the replacement hashes differently, indexes a + // different memo slot, and inherits nothing - so the arm asserted stale pixels and saw + // fresh ones, which is a FAILING negative control that had stopped controlling anything. + // + // So the control no longer asks the allocator for the collision; it manufactures it. On + // both arms the object identity is replaced by a constant, which is the strongest form of + // "the allocator handed the block back" and is deterministic. That covers strictly more + // than D18's spelling, and in particular it covers the arm P2 SHIPS: on the handle arm + // the constant defeats the GENERATION in {slot, gen}, which is the whole of what makes + // the re-keyed memos ABA-safe. Defeating only the retired lifetime-id/address guards + // would leave the shipped key untested, which is exactly the vacuity this control exists + // to catch. + // + // Everything the control does NOT defeat is as load-bearing as what it does. It never + // touches a guard that is not an IDENTITY guard: the resolved-bindings memo's frame + // serial, its slice-epoch compares and its host-map check all stay in force, so a green + // AbaControl arm still means "a replacement object was handed its dead predecessor's + // resolved vertex bindings because the identity halves of the keys were defeated", not + // "every safety net was switched off until something broke". + // + // Off by default (Config.h), set only by the HandleRecycle AbaControl ctest lanes, and + // #if MOBILEGL_PIPE_PUSH throughout, so no shipping pull build can even parse it. + inline Bool MagmaPipeAbaControlDefeatsIdentity() { + return MG_Config::Features.PipeHandleAbaControl; + } + + // The single consumer-table entry every VAO collapses onto while the control is on. Slot + // 0 is a real, ordinary entry of both tables (MagmaPipeSlotIndex maps the first allocatable + // handle onto it), so nothing about the tables changes shape for the control's sake. + inline constexpr Uint32 kMagmaPipeAbaControlSlotIndex = 0; + // --------------------------------------------------------------------------------- // The {slot, gen} mint // --------------------------------------------------------------------------------- diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/VertexInputStateFactory.cpp b/MobileGL/MG_Backend/DirectVulkan/Renderer/VertexInputStateFactory.cpp index c2ac4a36..02013f47 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/VertexInputStateFactory.cpp +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/VertexInputStateFactory.cpp @@ -63,14 +63,23 @@ namespace MobileGL::MG_Backend::DirectVulkan { const MG_Pipe::MGPipeHandle handle = m_identity->HandleOf(MG_Pipe::MGPipeKind::Buffer, attr.Buffer->GetLifetimeId()); bufferKey = static_cast(handle.Slot) | (static_cast(handle.Gen) << 32); - } else if (MG_Config::Features.PipeHandleAbaControl) { - // Negative control C (P2 brief D18), and it applies to the PRE-HANDLE arm - // on purpose: hash the raw BufferObject* the way this did before the - // lifetime-id fix, so HandleRecycleScenario.AbaControl can reproduce the - // ABA and assert the WRONG pixels. That arm is what proves the reproducer - // still reproduces; if the allocator stops handing the address back, it - // fails instead of passing for the wrong reason. - bufferKey = static_cast(reinterpret_cast(attr.Buffer.get())); + } + if (MagmaPipeAbaControlDefeatsIdentity()) { + // Negative control C (P2 brief D18), on WHICHEVER arm this run is on - the + // pre-handle lifetime id and the handle's {slot, gen} are the same guard + // wearing two hats, and a control that defeated only the retired one would + // say nothing about the key P2 ships. + // + // The identity is replaced by a constant rather than by the raw + // BufferObject*, because the address is not recycled in practice and so + // never collides (see MagmaPipeAbaControlDefeatsIdentity). Zero is what a + // key with NO buffer identity in it looks like - the exact defect this + // hash was fixed for: "the hash is what TryBindResolvedVertexBindings + // accepts as proof that a memoised binding still reads the buffer it was + // resolved from", and with the identity gone it accepts a binding resolved + // from a different buffer. HandleRecycleScenario.AbaControl then draws a + // replacement VAO and gets its dead predecessor's vertex data. + bufferKey = 0; } } #endif @@ -89,6 +98,13 @@ namespace MobileGL::MG_Backend::DirectVulkan { // does this, and no two live VAOs can share an entry however large the working set is. // There is no probe in front of it because the mint itself is one - a one-entry memo // hit for every acquisition after this draw's first, and a hash probe otherwise. + if (MagmaPipeAbaControlDefeatsIdentity()) { + // Negative control C: one entry for every VAO, claimed without the Owner compare, + // which is precisely "the slot was recycled and Gen did not move". The replacement + // therefore inherits the dead VAO's content hash and its resolved-entry pointer - + // the two facts the generation is the only thing protecting. + return m_vaoMemos[kMagmaPipeAbaControlSlotIndex]; + } VaoBackendMemos& memos = m_vaoMemos[MagmaPipeSlotIndex(handle)]; if (!(memos.Owner == handle)) { // A slot whose Gen moved because the identity table recycled it for a different diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp b/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp index 24bbab5b..39589401 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp @@ -3650,6 +3650,23 @@ void main() { m_vaoDrawMemoTable.resize(kVaoDrawMemoSlotCount); } #if MOBILEGL_PIPE_PUSH + if (MagmaPipeAbaControlDefeatsIdentity()) { + // Negative control C (P2 brief D18), ahead of BOTH arms because it defeats the + // identity half of both keys at once: the legacy arm's (address, lifetime id) pair + // and the handle arm's {slot, gen}. Every VAO lands on one entry and the entry is + // handed back without an identity compare and WITHOUT being cleared - which is + // exactly what this table would do if a replacement object reproduced its dead + // predecessor's address, or reused its slot without the generation moving. + // + // Nothing else about the entry is relaxed: whether the resolved bindings it holds + // are then USED is still decided by TryBindResolvedVertexBindings' frame serial, + // content hash, active-attribute mask and slice epochs. That is what keeps the arm + // an assertion about identity rather than about the memo as a whole. + VaoDrawMemo& aliased = m_vaoDrawMemoTable[kMagmaPipeAbaControlSlotIndex]; + aliased.vaoKey = vao; + aliased.vaoLifetimeId = vao->GetLifetimeId(); + return &aliased; + } // ---- P2 D12.4, the handle arm ---- // // The slot PICKS the entry, and the handle DECIDES whether the entry is this VAO's - diff --git a/MobileGL/MG_IntegrationTest/CMakeLists.txt b/MobileGL/MG_IntegrationTest/CMakeLists.txt index 29fd7654..40c2b40b 100644 --- a/MobileGL/MG_IntegrationTest/CMakeLists.txt +++ b/MobileGL/MG_IntegrationTest/CMakeLists.txt @@ -824,10 +824,18 @@ gtest_discover_tests(MobileGLIntegrationTest # block: a ctest ENVIRONMENT property REPLACES the job environment for the names it lists, so an # entry naming only its own knobs would lose the EGL vendor and Vulkan ICD pinning. # -# The AbaControl arm is DirectVulkan only. The knob reverts two DirectVulkan guards -# (VertexInputStateFactory::ComputeHash's key and LookupVaoDrawMemo's lifetimeId compare); it -# steers nothing on DirectGLES, and a lane that configured it there would be a permanent skip -# claiming to be a control. +# The AbaControl arm is DirectVulkan only. The knob defeats the object-identity half of +# DirectVulkan's vertex-input memo keys (VertexInputStateFactory::ComputeHash, its per-VAO memo +# table, and LookupVaoDrawMemo); it steers nothing on DirectGLES, and a lane that configured it +# there would be a permanent skip claiming to be a control. +# +# It gets TWO lanes, because there are two arms and the control has to cover the one P2 SHIPS. +# `AbaControl` is D18's lane verbatim (MOBILEGL_PIPE_PUSH=0, the pre-handle arm) and defeats the +# lifetime-id/address guards; `AbaControlHandles` runs the handle arm (MOBILEGL_PIPE_LEGACY_MEMOS=0, +# the default push mask) and defeats the {slot, gen} GENERATION, which is what makes the re-keyed +# memos ABA-safe. With only the first lane the control says nothing at all about the re-key: the +# handle arm is not executed under MOBILEGL_PIPE_PUSH=0, so every guard it would have to defeat is +# in another branch. # # The two PUSH-ONLY knobs of those arms are set only in a push build, and the lane NAMES are # unaffected by that (an ENVIRONMENT property is not part of a test's name, so G2 still sees the @@ -860,6 +868,10 @@ mgl_itest_join_environment(MGL_ITEST_VULKAN_HANDLE_ABA_ENVIRONMENT "MOBILEGL_BACKEND_TYPE=DirectVulkan" "MGITEST_HANDLE_ARM=aba" "MOBILEGL_PIPE_PUSH=0" ${MGL_ITEST_ABA_ARM_KNOBS} ${MGL_ITEST_CAPABILITY_ENV} ${MGL_ITEST_VULKAN_ENV}) +mgl_itest_join_environment(MGL_ITEST_VULKAN_HANDLE_ABA_HANDLES_ENVIRONMENT + "MOBILEGL_BACKEND_TYPE=DirectVulkan" "MGITEST_HANDLE_ARM=aba" + ${MGL_ITEST_HANDLES_ARM_KNOBS} ${MGL_ITEST_ABA_ARM_KNOBS} + ${MGL_ITEST_CAPABILITY_ENV} ${MGL_ITEST_VULKAN_ENV}) gtest_discover_tests(MobileGLIntegrationTest TEST_PREFIX "DirectGLES.HandleRecycle.Handles." @@ -906,6 +918,15 @@ gtest_discover_tests(MobileGLIntegrationTest TIMEOUT ${MGL_ITEST_TIMEOUT} ENVIRONMENT "${MGL_ITEST_VULKAN_HANDLE_ABA_ENVIRONMENT}" ) +gtest_discover_tests(MobileGLIntegrationTest + TEST_PREFIX "DirectVulkan.HandleRecycle.AbaControlHandles." + TEST_FILTER "HandleRecycleScenario.*" + DISCOVERY_TIMEOUT 30 + PROPERTIES + LABELS integration-gpu + TIMEOUT ${MGL_ITEST_TIMEOUT} + ENVIRONMENT "${MGL_ITEST_VULKAN_HANDLE_ABA_HANDLES_ENVIRONMENT}" +) # --- G12: the CSO content-addressing negative control --------------------------------- # diff --git a/MobileGL/MG_IntegrationTest/Scenarios/HandleRecycleScenario.cpp b/MobileGL/MG_IntegrationTest/Scenarios/HandleRecycleScenario.cpp index cc873a28..3ec0c55e 100644 --- a/MobileGL/MG_IntegrationTest/Scenarios/HandleRecycleScenario.cpp +++ b/MobileGL/MG_IntegrationTest/Scenarios/HandleRecycleScenario.cpp @@ -26,32 +26,42 @@ // 2. it is unbound (so the frontend's last SharedPtr drops - a still-bound object keeps living, // TextureState.cpp) and deleted; // 3. a replacement is created IMMEDIATELY, with a byte-identical configuration, so that a -// content hash over the configuration matches the dead object's, and so that the allocator -// is as likely as it can be made to hand back the address it just freed; +// content hash over the configuration matches the dead object's; // 4. the replacement is given DIFFERENT CONTENTS - a different vertex buffer, different texels, // a different attachment; // 5. one draw, one readback. The pixels must come from the replacement. // -// The allocator is not under our control, so step 3 is a likelihood, not a guarantee, and a -// scenario that silently passed because the address was never reused would prove nothing. The -// public-GL proxy for "the allocator repeated itself" is the GL NAME: MobileGL's name allocators -// hand a deleted name straight back, so `TheReproducerRecyclesEveryName` asserts the recycle -// happened and every other case asserts on the name it got. When a name is NOT recycled the case -// SKIPS with that reason rather than passing - the shape MG_Test/State/ObjectLifetimeIdTest.cpp -// already uses for exactly this ("inconclusive, not proven"). +// The public-GL proxy for "the allocator repeated itself" is the GL NAME: MobileGL's name +// allocators hand a deleted name straight back, so `TheReproducerRecyclesEveryName` asserts the +// recycle happened and every other case asserts on the name it got. When a name is NOT recycled +// the case 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. +// WHAT THE NAME PROXY DOES NOT BUY, MEASURED RATHER THAN ASSUMED. The name comes back; the C++ +// HEAP BLOCK does not. A VertexArrayObject is 3920 bytes - past glibc's tcache - so its chunk goes +// to the unsorted bin and is split by the very next allocation the replacement path makes; four +// create/delete cycles in one run of this file produced four distinct addresses about a mebibyte +// apart, and the same is true of the BufferObject. An earlier revision of this file left the +// AbaControl arm's collision to that allocator, and the consequence was the failure mode this file +// exists to prevent, in its most literal form: with nothing colliding, the replacement inherited +// nothing, the arm asserted stale pixels, saw fresh ones, and went RED in an always-on +// integration-gpu lane while every guard it was supposed to be defeating was still standing. +// +// So the AbaControl arm no longer asks the allocator for the collision - MOBILEGL_PIPE_HANDLE_ABA_CONTROL +// manufactures it, by replacing the object identity in each key with a constant (see +// MagmaPipeArms.h's MagmaPipeAbaControlDefeatsIdentity). That is the strongest form of "the +// allocator handed the block back", it is deterministic, and - the reason it matters - it defeats +// the {slot, gen} GENERATION as well as the retired lifetime id, so the control covers the key P2 +// actually ships instead of only the one it replaced. +// +// AND THE ABA HAPPENS INSIDE ONE FRAME, which is not a detail. The only backend structure that can +// hand a draw a dead object's GPU slice is VulkanRenderer::ResolvedVertexBindings, and it refuses +// to be trusted across a frame boundary by design ("NO cross-frame trust"). Every other memo the +// recycle can poison holds LAYOUT, which is byte-identical between the two objects by construction +// and so cannot be seen in pixels. A reproducer that puts a frame boundary between the arming draw +// and the recycled draw therefore cannot produce wrong pixels no matter how completely the keys +// collide - it would be asserting a fact about the frame gate, not about identity. // // 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: @@ -61,13 +71,15 @@ // Legacy MOBILEGL_PIPE_PUSH=0. Today's lifetimeId + weak_ptr guards. Expects correct // pixels - they work, which is the point: the re-key is not fixing a live bug, it // is replacing a guard, and the replacement has to be at least as strong. -// AbaControl MOBILEGL_PIPE_PUSH=0 AND MOBILEGL_PIPE_HANDLE_ABA_CONTROL=1. The knob reverts -// exactly the two guards the re-key replaces (VertexInputStateFactory::ComputeHash -// hashes attr.Buffer.get() instead of GetLifetimeId(); LookupVaoDrawMemo skips the -// vaoLifetimeId compare), so this arm expects the CORRUPTION. It is what makes -// `HandleRecycleScenario green, and red before the re-key` an always-on CI fact -// instead of a one-off manual demonstration: if the reproducer ever stops -// reproducing the ABA, this arm fails. +// AbaControl MOBILEGL_PIPE_HANDLE_ABA_CONTROL=1, on TWO lanes: one with MOBILEGL_PIPE_PUSH=0 +// (the pre-handle arm, D18's lane verbatim) and one on the handle arm +// (MOBILEGL_PIPE_LEGACY_MEMOS=0). The knob defeats the object-identity half of +// every vertex-input memo key on whichever arm is running - the pre-handle +// (address, lifetime id) pair and the handle arm's {slot, gen} generation - so both +// lanes expect the CORRUPTION. Two lanes rather than one because the guard P2 SHIPS +// is the generation: a control that only defeated the retired guards would be green +// forever without saying anything about the re-key, which is exactly how this arm +// went vacuous once packages C and D landed. // // WHY AN ARM CAN SKIP, AND WHY THAT IS NOT A HOLE. Two of the three arms assert something that // only EXISTS once another P2 package has landed: `Handles` needs the backend's {slot, gen} arm @@ -95,6 +107,7 @@ #include #include #include +#include #include #include @@ -238,11 +251,25 @@ void main() { oColor = texture(uTex, vUv); } // (`fresh`) or from the object that died (`stale`)? The arm decides which is the pass. void ExpectPixelsFor(Arm arm, bool armExpectsCorruption, const Image& image, const char* fresh, const char* stale, const std::string& when) { - if (arm == Arm::AbaControl && armExpectsCorruption) { + // Say which of the two was actually observed, on EVERY arm and whether or not the case + // passes. The arm's expectation is only half the evidence, and a reader of the CI log + // should not have to infer the other half from the exit status - least of all for a + // control whose whole claim is "the corruption is still reproducible here". + const bool sawStale = static_cast(RegionIsMostly( + image, kInset, image.Width() - kInset, kInset, image.Height() - kInset, stale, 0.0, when)); + const bool sawFresh = static_cast(RegionIsMostly( + image, kInset, image.Width() - kInset, kInset, image.Height() - kInset, fresh, 0.0, when)); + const bool expectsStale = arm == Arm::AbaControl && armExpectsCorruption; + std::cout << "[ HandleRecycle ] arm=" << ArmName(arm) << " expected=" + << (expectsStale ? "STALE" : "FRESH") << " observed=" + << (sawStale ? "STALE" : (sawFresh ? "FRESH" : "NEITHER")) << " (stale=" << stale + << ", fresh=" << fresh << ") - " << when << std::endl; + if (expectsStale) { // The corruption IS the assertion. If this ever goes green-by-being-correct the // reproducer has stopped reproducing and the other two arms prove nothing. ExpectWholeViewportIs(image, stale, when + " [AbaControl expects the STALE object's pixels: " - "the two guards are deliberately defeated]"); + "the identity half of every key is deliberately " + "defeated]"); return; } ExpectWholeViewportIs(image, fresh, @@ -458,7 +485,21 @@ void main() { oColor = texture(uTex, vUv); } SkipUnlessTheArmIsAssertableHere(); if (IsSkipped()) return; + // BOTH buffers are created, and both are DRAWN WITH, before the recycle happens. + // Creating a buffer - or touching one for the first time - moves VkBufferManager's + // manager-wide slice-epoch counter, and a moved counter sends the resolved-bindings + // memo into a revalidation that re-reads every binding from the live VAO. That gate is + // not an identity gate and it is not what this case is about, so both buffers are + // realised up front and the ABA window contains no buffer traffic at all. const GLuint redBuffer = MakeQuadBuffer(1.0f, 0.0f, 0.0f); + const GLuint greenBuffer = MakeQuadBuffer(0.0f, 1.0f, 0.0f); + + GLuint primerVao = 0; + glGenVertexArrays(1, &primerVao); + ConfigureQuadVao(primerVao, greenBuffer); + const Image primed = DrawQuadAndRead(primerVao); + ExpectWholeViewportIs(primed, "green", "priming the replacement's buffer"); + GLuint redVao = 0; glGenVertexArrays(1, &redVao); ConfigureQuadVao(redVao, redBuffer); @@ -469,44 +510,55 @@ void main() { oColor = texture(uTex, vUv); } ExpectWholeViewportIs(warm, "red", "warm-up frame " + std::to_string(frame)); } + // ---- the ABA window: ONE frame, two draws ---- + // + // The arming draw and the recycled draw share a frame because + // ResolvedVertexBindings - the only memo that carries a GPU slice rather than a + // layout - declines across frames by design. See the header. + BindDefaultFramebuffer(); + ClearTo(0.0f, 0.0f, 0.0f, 1.0f); + glUseProgram(m_colorProgram); + glBindVertexArray(redVao); + glDrawArrays(GL_TRIANGLES, 0, kVertexCount); + // Unbind FIRST: a still-bound object keeps living, so the last SharedPtr would not - // drop and there would be no freed block for the replacement to land in. + // drop and the object would not die here at all. glBindVertexArray(0); glBindBuffer(GL_ARRAY_BUFFER, 0); glDeleteVertexArrays(1, &redVao); - GLuint doomedBuffer = redBuffer; - glDeleteBuffers(1, &doomedBuffer); - // The replacement, immediately and in the reverse order of the frees, which is the - // order a size-classed allocator is most likely to answer from its free lists. - const GLuint greenBuffer = MakeQuadBuffer(0.0f, 1.0f, 0.0f); + // The replacement, immediately, byte-identically configured, and reading the OTHER + // buffer - so its pixels differ from its predecessor's by exactly the thing a stale + // vertex binding would get wrong. GLuint greenVao = 0; glGenVertexArrays(1, &greenVao); 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 << " -> " - << greenBuffer << "), so no ABA was constructed"; + glBindVertexArray(greenVao); + glDrawArrays(GL_TRIANGLES, 0, kVertexCount); + const Image image = ReadPixels(Gl().Width(), Gl().Height()); + Gl().EndFrame(); + + // The name proxy. It no longer constructs the AbaControl arm's collision - the knob + // does that, deterministically, because the heap block is never handed back (header) - + // but it is still what makes this a RECYCLE rather than two unrelated objects, and it + // is what the Handles and Legacy arms are asserting is not enough to inherit anything. + if (greenVao != redVao) { + GTEST_SKIP() << "inconclusive, not proven: glGenVertexArrays returned " << greenVao + << " rather than the deleted " << redVao << ", so no ABA was constructed"; } RecordProperty("recycled_vao_name", static_cast(greenVao)); - const Image image = DrawQuadAndRead(greenVao); ExpectPixelsFor(m_arm, /*armExpectsCorruption=*/true, image, "green", "red", - "the draw after the VAO and its buffer were both recycled"); + "the draw after the VAO was recycled inside one frame"); glBindVertexArray(0); glBindBuffer(GL_ARRAY_BUFFER, 0); - glDeleteVertexArrays(1, &greenVao); - GLuint cleanup = greenBuffer; - glDeleteBuffers(1, &cleanup); + GLuint cleanupVaos[2] = {greenVao, primerVao}; + glDeleteVertexArrays(2, cleanupVaos); + GLuint cleanupBuffers[2] = {redBuffer, greenBuffer}; + glDeleteBuffers(2, cleanupBuffers); } // ------------------------------------------------------------------------------------