diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/MagmaPipeArms.h b/MobileGL/MG_Backend/DirectVulkan/Renderer/MagmaPipeArms.h index e1a54486..68b98a70 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/MagmaPipeArms.h +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/MagmaPipeArms.h @@ -157,11 +157,33 @@ namespace MobileGL::MG_Backend::DirectVulkan { // 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. + // than D18's spelling, and in particular it reaches the arm P2 SHIPS: on the handle arm + // the constant defeats the OBJECT IDENTITY THAT SELECTS THE SLOT - the key the handle arm + // ships - so the replacement VAO is handed the dead one's memo entry and its content hash. + // Defeating only the retired lifetime-id/address guards would leave that key untested, + // which is exactly the vacuity this control exists to catch. + // + // WHAT IT DOES NOT COVER, AND WHY NO REPRODUCER OF THIS SHAPE CAN [fix-aba review v1, + // MAJOR 1]. It does NOT exercise the GENERATION half of {slot, gen}: + // + // * this mint has no death notification - nothing in MG_Backend/DirectVulkan consumes + // NotifyStateObjectDestroyed - so a slot returns to the free list only through + // OnFrameBoundary's age sweep (kSweepInterval 256, kRetireAgeBoundaries 1024, below); + // * HandleRecycleScenario issues five frame boundaries, so the free list is empty when + // the replacement VAO acquires and it gets a BRAND-NEW slot at Gen 1 (measured: + // redVao slot=2 gen=1, greenVao slot=3 gen=1). The knob-off FRESH verdict there is + // decided by the SLOT alone, and deleting the ++Gen below leaves all four arms green; + // * a genuine slot REUSE needs >= 1024 idle boundaries after the dead object's last + // draw, which necessarily puts the two draws in different frames - and the only memo + // that carries a GPU slice rather than a layout, ResolvedVertexBindings, declines + // across frames by design. The two requirements are mutually exclusive, so the + // generation is out of reach of any same-frame pixel reproducer for this memo. + // + // The generation is covered where it IS expressible, over this mint and the claim rule + // MagmaPipeClaimSlotMemos below: MG_Test/Pipe/MagmaPipeIdentityTest.cpp drives a real + // retire -> reuse and asserts that a memo stamped at {slot, gen=N} is not served at + // {slot, gen=N+1} with the knob off and IS served with it on. Deleting the ++Gen reds that + // suite; it is the only place in the tree where that deletion is caught. // // 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 @@ -442,5 +464,35 @@ namespace MobileGL::MG_Backend::DirectVulkan { }; Vector> m_chunks; }; + + // The claim rule every per-slot memo table uses, in one place so that the rule and the + // negative control that defeats it cannot drift apart between consumers - and so that the + // unit suite which drives a REAL slot reuse (MG_Test/Pipe/MagmaPipeIdentityTest.cpp) tests + // this code rather than a copy of it. + // + // The SLOT picks the entry; the WHOLE handle - Gen included - decides whether the entry is + // this object's. A slot the mint recycled for a different object comes back with a moved + // Gen, so the compare fails and the entry is cleared rather than inherited. That is the + // half HandleRecycleScenario cannot reach (see MagmaPipeAbaControlDefeatsIdentity). + // + // With negative control C on, every object collapses onto one entry and the entry is handed + // back UNCLEARED and UNCLAIMED - at once "the replacement reproduced its predecessor's + // slot" and "the slot was reused and Gen did not move". + // + // `Memos` needs a MG_Pipe::MGPipeHandle member named Owner and a default constructor that + // means "empty"; VertexInputStateFactory::VaoBackendMemos is the one production instance. + template + inline Memos& MagmaPipeClaimSlotMemos(MagmaPipeSlotTable& table, + const MG_Pipe::MGPipeHandle& handle) { + if (MagmaPipeAbaControlDefeatsIdentity()) { + return table[kMagmaPipeAbaControlSlotIndex]; + } + Memos& memos = table[MagmaPipeSlotIndex(handle)]; + if (!(memos.Owner == handle)) { + memos = Memos{}; + memos.Owner = handle; + } + return memos; + } #endif // MOBILEGL_PIPE_PUSH } // namespace MobileGL::MG_Backend::DirectVulkan diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/VertexInputStateFactory.cpp b/MobileGL/MG_Backend/DirectVulkan/Renderer/VertexInputStateFactory.cpp index 02013f47..8c498758 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/VertexInputStateFactory.cpp +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/VertexInputStateFactory.cpp @@ -98,22 +98,16 @@ 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 - // object. Claim it, contents cleared - never inherited, which is the whole point - // of keying on the generation. - memos = VaoBackendMemos{}; - memos.Owner = handle; - } - return memos; + // + // The claim rule - the slot picks the entry, the whole handle (Gen included) decides + // whose it is - and negative control C's defeat of it are MagmaPipeArms.h's + // MagmaPipeClaimSlotMemos, so that the unit suite which drives a REAL slot reuse + // (MG_Test/Pipe/MagmaPipeIdentityTest.cpp) exercises this code and not a copy of it. + // What the control defeats HERE is the identity that SELECTS the entry: every VAO + // collapses onto one, handed back uncleared, so the replacement inherits the dead + // VAO's content hash and its resolved-entry pointer. The GENERATION half is the unit + // suite's business, for the reason MagmaPipeAbaControlDefeatsIdentity spells out. + return MagmaPipeClaimSlotMemos(m_vaoMemos, handle); } #endif diff --git a/MobileGL/MG_IntegrationTest/CMakeLists.txt b/MobileGL/MG_IntegrationTest/CMakeLists.txt index 23232940..d2650e4b 100644 --- a/MobileGL/MG_IntegrationTest/CMakeLists.txt +++ b/MobileGL/MG_IntegrationTest/CMakeLists.txt @@ -829,13 +829,21 @@ gtest_discover_tests(MobileGLIntegrationTest # 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. +# It gets TWO lanes, because there are two arms and the control has to reach 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 default push mask) and defeats the object identity that SELECTS THE SLOT - the key the handle +# arm ships. 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. +# +# NEITHER lane exercises the GENERATION half of {slot, gen}, and no lane of this shape can. Magma's +# mint has no death notification and returns a slot only through its age sweep (256/1024 boundaries, +# MagmaPipeArms.h), so the five frame boundaries this scenario issues always hand the replacement a +# brand-new slot at Gen 1; a real reuse needs >= 1024 idle boundaries, which puts the two draws in +# different frames - where the only pixel-visible memo declines by design. The generation is covered +# by the unit suite MG_Test/Pipe/MagmaPipeIdentityTest.cpp instead, which drives a real +# retire -> reuse; MagmaPipeAbaControlDefeatsIdentity carries the measurement. # # 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 diff --git a/MobileGL/MG_Test/Pipe/CMakeLists.txt b/MobileGL/MG_Test/Pipe/CMakeLists.txt index 04c4840b..5e3fce1a 100644 --- a/MobileGL/MG_Test/Pipe/CMakeLists.txt +++ b/MobileGL/MG_Test/Pipe/CMakeLists.txt @@ -107,8 +107,37 @@ if (MSVC) target_compile_options(RenderStateSpansTest PRIVATE /Zc:preprocessor) endif() +# Magma's own {slot, gen} mint and the claim rule its per-slot memo tables use +# (MG_Backend/DirectVulkan/Renderer/MagmaPipeArms.h, which is header-only). It sits here rather +# than under Backend/DirectVulkan because it needs no device, no GLFW and no Vulkan loader - and +# because that directory is registered only when ENABLE_INTEGRATION_TESTS is on, while this +# suite is the only place in the tree where a deleted `++Gen` is caught. See the file header. +add_executable( + MagmaPipeIdentityTest + MagmaPipeIdentityTest.cpp +) + +target_include_directories(MagmaPipeIdentityTest PRIVATE + ${MGL_ROOT}/include + ${MGL_ROOT}/MobileGL + ${MGL_ROOT}/MobileGL/MG_Pipe + ${MGL_ROOT}/3rdparty/xxHash + ${MGL_ROOT}/3rdparty/Vulkan-Headers/include + ${MGL_ROOT}/3rdparty/SPIRV-Reflect +) + +target_link_libraries(MagmaPipeIdentityTest PRIVATE + GTest::gtest_main + ${LINK_LIBRARIES} +) + +if (MSVC) + target_compile_options(MagmaPipeIdentityTest PRIVATE /Zc:preprocessor) +endif() + include(GoogleTest) gtest_discover_tests(PipeCatalogueTest DISCOVERY_TIMEOUT 30 PROPERTIES LABELS unit) +gtest_discover_tests(MagmaPipeIdentityTest DISCOVERY_TIMEOUT 30 PROPERTIES LABELS unit) gtest_discover_tests(PipeInputsTest DISCOVERY_TIMEOUT 30 PROPERTIES LABELS unit) gtest_discover_tests(RenderStateSpansTest DISCOVERY_TIMEOUT 30 PROPERTIES LABELS unit) foreach(pipeTest TrackerTest SlotAllocatorTest CsoCacheTest) diff --git a/MobileGL/MG_Test/Pipe/MagmaPipeIdentityTest.cpp b/MobileGL/MG_Test/Pipe/MagmaPipeIdentityTest.cpp new file mode 100644 index 00000000..e7ce2e90 --- /dev/null +++ b/MobileGL/MG_Test/Pipe/MagmaPipeIdentityTest.cpp @@ -0,0 +1,217 @@ +// MobileGL - MobileGL/MG_Test/Pipe/MagmaPipeIdentityTest.cpp +// Copyright (c) 2025-2026 MobileGL-Dev +// Licensed under the GNU Lesser General Public License v3.0: +// https://www.gnu.org/licenses/gpl-3.0.txt +// https://www.gnu.org/licenses/lgpl-3.0.txt +// SPDX-License-Identifier: LGPL-3.0-only +// End of Source File Header + +// Magma's {slot, gen} mint (MG_Backend/DirectVulkan/Renderer/MagmaPipeArms.h) and the claim +// rule its per-slot memo tables use, at the one point HandleRecycleScenario cannot reach: a +// REAL SLOT REUSE. +// +// WHY THIS SUITE EXISTS (fix-aba review v1, MAJOR 1). The AbaControl integration lanes defeat +// the object identity that SELECTS the slot, and that is the whole of what a same-frame pixel +// reproducer can defeat: +// +// * the mint has no death notification, so a slot returns to the free list only through +// OnFrameBoundary's age sweep (kSweepInterval 256, kRetireAgeBoundaries 1024); +// * HandleRecycleScenario issues five frame boundaries, so its replacement VAO gets a +// BRAND-NEW slot at Gen 1 and the generation never participates in a compare; +// * a real reuse needs >= 1024 idle boundaries, which puts the two draws in different frames +// - and ResolvedVertexBindings, the only memo carrying a GPU slice rather than a layout, +// declines across frames by design. +// +// So deleting the `++m_entries[index].Gen` in MagmaPipeIdentityTable::ClaimSlot leaves every +// arm of HandleRecycleScenario green. It reds AnIdleSlotIsRetiredAndReusedWithANewGeneration +// and AReusedSlotDoesNotServeItsPredecessorsMemo below, which is the whole point of the file. +// +// The suite lives beside SlotAllocatorTest because it asserts the same identity contract that +// file asserts for the client allocator - "Gen moves on REUSE and never on respecify" - for the +// second mint in the tree, the one Magma keeps because nothing in P2 can call the client +// allocator's Free (MagmaPipeArms.h says why). It needs no GL context, no driver and no Vulkan +// loader: MagmaPipeArms.h is header-only. +// +// Push-only, like everything it tests, so every case is a visible SKIP in a pull build rather +// than a vanishing test (G2 name parity). +#include + +#include "Includes.h" +#include +#include + +#if MOBILEGL_PIPE_PUSH +#include +#include +#endif + +using namespace MobileGL; + +namespace { +#if !MOBILEGL_PIPE_PUSH + // The push build's case list, declared once so a case added on one side and forgotten on + // the other shows up as a ctest-name diff rather than as a test that silently is not there + // (the shape CsoCacheTest.cpp established). +#define MGL_MAGMA_PIPE_IDENTITY_TEST_LIST(X) \ + X(MagmaPipeIdentityTest, AnIdleSlotIsRetiredAndReusedWithANewGeneration) \ + X(MagmaPipeIdentityTest, AReusedSlotDoesNotServeItsPredecessorsMemo) \ + X(MagmaPipeIdentityTest, TheAbaControlKnobServesTheStaleMemoAcrossAReusedSlot) \ + X(MagmaPipeIdentityTest, ALiveObjectKeepsItsSlotItsGenerationAndItsMemo) + +#define MGL_DECLARE_PULL_SKIP(Suite, Name) \ + TEST(Suite, Name) { GTEST_SKIP() << "compiled only under MOBILEGL_PIPE_PUSH"; } + MGL_MAGMA_PIPE_IDENTITY_TEST_LIST(MGL_DECLARE_PULL_SKIP) +#undef MGL_DECLARE_PULL_SKIP +#else + using namespace MobileGL::MG_Backend::DirectVulkan; + using MG_Pipe::MGPipeHandle; + + // What VertexInputStateFactory::VaoBackendMemos is, reduced to the two fields the claim + // rule needs: the Owner it compares, and one payload word standing in for the memo's + // contents (there, the content hash and the resolved-entry pointer). The RULE is production + // code - MagmaPipeClaimSlotMemos - not a copy of it. + struct TestMemos { + MGPipeHandle Owner = MG_Pipe::kMGPipeNullHandle; + Uint64 Payload = 0; + }; + + // MagmaPipeIdentityTable's sweep cadence and retirement age are private, so the number of + // boundaries needed to retire an object last used at boundary 0 is spelled out here: the + // sweep runs when (boundary % 256) == 0 and retires entries idle for more than 1024 + // boundaries, so the first sweep that can retire it is boundary 1280. Every case that + // depends on this ASSERTs the retire actually happened, so a change to either constant + // fails loudly instead of silently turning these cases into "two unrelated objects". + constexpr int kBoundariesToRetireAnObjectIdleSinceTheStart = 1280; + + class MagmaPipeIdentityTest : public ::testing::Test { + protected: + void SetUp() override { + m_savedKnob = MG_Config::Features.PipeHandleAbaControl; + MG_Config::Features.PipeHandleAbaControl = false; + } + void TearDown() override { MG_Config::Features.PipeHandleAbaControl = m_savedKnob; } + + // Ages the mint far enough for the sweep to retire everything that has been idle since + // the start, while touching `keepAliveLifetimeId` on every boundary so that IT is never + // retired. The keep-alive is what makes the reuse non-degenerate: it holds the first + // allocatable slot, so the slot under test is not the one negative control C aliases + // everything onto (kMagmaPipeAbaControlSlotIndex). + static void AgeUntilTheSweepRetiresTheIdleSlots(MagmaPipeIdentityTable& mint, + Uint64 keepAliveLifetimeId) { + for (int i = 0; i < kBoundariesToRetireAnObjectIdleSinceTheStart; ++i) { + mint.Acquire(keepAliveLifetimeId); + mint.OnFrameBoundary(); + } + } + + Bool m_savedKnob = false; + }; + + // The precondition every case below rests on, asserted on its own so that a failure here + // reads as "the mint stopped reusing slots" rather than as a memo bug. + TEST_F(MagmaPipeIdentityTest, AnIdleSlotIsRetiredAndReusedWithANewGeneration) { + MagmaPipeIdentityTable mint("VertexElementsCso"); + const MGPipeHandle keepAlive = mint.Acquire(1); + const MGPipeHandle first = mint.Acquire(2); + ASSERT_EQ(keepAlive.Slot, MG_Pipe::kMGPipeFirstAllocatableSlot); + ASSERT_NE(first.Slot, keepAlive.Slot); + ASSERT_EQ(first.Gen, 1u) << "a slot's first handout is generation 1"; + ASSERT_EQ(mint.LiveCount(), 2u); + + AgeUntilTheSweepRetiresTheIdleSlots(mint, 1); + ASSERT_EQ(mint.LiveCount(), 1u) + << "the idle slot was not retired, so nothing in this file is a slot REUSE"; + + // The step HandleRecycleScenario cannot take. With an empty free list this would be a + // brand-new slot at Gen 1 and the generation would never participate in any compare - + // which is exactly what the scenario measures (redVao slot=2 gen=1, greenVao slot=3 + // gen=1) and why it cannot catch a deleted ++Gen. + const MGPipeHandle second = mint.Acquire(3); + EXPECT_EQ(second.Slot, first.Slot) << "a retired slot must come back before the high-water mark"; + EXPECT_EQ(second.Gen, first.Gen + 1u) << "a slot that changes owner must change generation"; + EXPECT_FALSE(second == first); + EXPECT_EQ(mint.Count(), 2u) << "the reuse must not mint a third slot"; + } + + // THE CASE THE ++Gen IS LOAD-BEARING FOR. Knob off: the replacement gets the predecessor's + // SLOT, so the slot cannot be what separates them - only the generation can. + TEST_F(MagmaPipeIdentityTest, AReusedSlotDoesNotServeItsPredecessorsMemo) { + MagmaPipeIdentityTable mint("VertexElementsCso"); + MagmaPipeSlotTable memos; + + mint.Acquire(1); // the keep-alive, so the slot under test is not slot index 0 + const MGPipeHandle first = mint.Acquire(2); + MagmaPipeClaimSlotMemos(memos, first).Payload = 0xDEADull; + ASSERT_TRUE(MagmaPipeClaimSlotMemos(memos, first).Owner == first); + ASSERT_EQ(MagmaPipeClaimSlotMemos(memos, first).Payload, 0xDEADull); + + AgeUntilTheSweepRetiresTheIdleSlots(mint, 1); + const MGPipeHandle second = mint.Acquire(3); + ASSERT_EQ(second.Slot, first.Slot) << "not a slot reuse, so this case would prove nothing"; + // EXPECT, not ASSERT: with the generation frozen the memo assertion below is exactly what + // goes red, and a reader of the failure should see both halves rather than stop here. + EXPECT_NE(second.Gen, first.Gen); + + const TestMemos& served = MagmaPipeClaimSlotMemos(memos, second); + EXPECT_TRUE(served.Owner == second) << "the entry was not claimed for its new owner"; + EXPECT_EQ(served.Payload, 0ull) + << "the replacement inherited the dead object's memo out of the SAME slot: the " + "generation is the only thing that separates {slot, gen=N} from {slot, gen=N+1}, " + "and it did not"; + } + + // The same shape with negative control C on, which is what makes the case above a control + // rather than a tautology: with the knob on the memo IS served across the generation. + // + // The knob is set before the first claim, as a process-wide knob is in a real run: what it + // defeats is the identity that selects the entry, so a run that stamps with it off and reads + // with it on would be reading a different entry, not an aliased one. + TEST_F(MagmaPipeIdentityTest, TheAbaControlKnobServesTheStaleMemoAcrossAReusedSlot) { + MG_Config::Features.PipeHandleAbaControl = true; + + MagmaPipeIdentityTable mint("VertexElementsCso"); + MagmaPipeSlotTable memos; + + mint.Acquire(1); + const MGPipeHandle first = mint.Acquire(2); + TestMemos& stamped = MagmaPipeClaimSlotMemos(memos, first); + stamped.Payload = 0xDEADull; + + AgeUntilTheSweepRetiresTheIdleSlots(mint, 1); + const MGPipeHandle second = mint.Acquire(3); + ASSERT_EQ(second.Slot, first.Slot); + EXPECT_NE(second.Gen, first.Gen); + + const TestMemos& served = MagmaPipeClaimSlotMemos(memos, second); + EXPECT_EQ(&served, &stamped) << "the control must collapse every object onto one entry"; + EXPECT_EQ(served.Payload, 0xDEADull) + << "negative control C is not defeating the claim rule any more: the replacement was " + "NOT handed its predecessor's memo, so the AbaControl lanes assert nothing"; + EXPECT_TRUE(MG_Pipe::MGPipeHandleIsNull(served.Owner)) + << "the control hands the entry back UNCLEARED and UNCLAIMED - it never learns whose " + "it is, which is what 'replace the identity with a constant' means"; + } + + // The other half of the {slot, gen} contract, and the reason a deleted ++Gen cannot be + // 'fixed' by bumping Gen on every acquisition: a live object keeps its handle across + // sweeps, so its memo survives a reconfiguration instead of being recomputed per draw. + TEST_F(MagmaPipeIdentityTest, ALiveObjectKeepsItsSlotItsGenerationAndItsMemo) { + MagmaPipeIdentityTable mint("VertexElementsCso"); + MagmaPipeSlotTable memos; + + const MGPipeHandle handle = mint.Acquire(2); + MagmaPipeClaimSlotMemos(memos, handle).Payload = 0xBEEFull; + + // Past two sweeps (256 and 512), drawn on every boundary. + for (int i = 0; i < 700; ++i) { + mint.Acquire(2); + mint.OnFrameBoundary(); + } + const MGPipeHandle again = mint.Acquire(2); + EXPECT_TRUE(again == handle) << "a live object's handle moved under the age sweep"; + EXPECT_EQ(MagmaPipeClaimSlotMemos(memos, again).Payload, 0xBEEFull) + << "a live object's memo was cleared without its slot changing owner"; + EXPECT_EQ(mint.Count(), 1u); + } +#endif // MOBILEGL_PIPE_PUSH +} // namespace