Files
MobileGL/MobileGL/MG_Test/State/TransformFeedbackLifetimeIdTest.cpp
T
swung0x48 bd2b4158e0 [Fix] (DirectVulkan, State): key the transform-feedback counter slots on the object's identity, not on its recycled GL name
- VulkanRenderer::CurrentXfbCounterSlot keyed m_xfbCounterSlotByObject on
  GetBoundTransformFeedbackName(). glGenTransformFeedbacks hands a deleted name
  straight back (IndexGenerator is LIFO) and nothing ever removed a map entry, so
  a transform feedback object created on a recycled name was served the DEAD
  object's counter group - and with it that group's m_xfbCountersValid and
  m_xfbLastSeenGeneration entries, which are the resume/fresh decision for
  vkCmdBeginTransformFeedbackEXT. This is D21 in plan B v2 4.7.3, the one entry
  in that table whose today-key "guards nothing", and 10.4-5 asks for it to land
  on dev on its own - hence this separate commit, kept in files no other commit
  on this branch touches so the cherry-pick applies unaided.
- Frontend: TransformFeedbackObjectState gains a never-reused `lifetimeId`
  through a default member initialiser, so every route into existence
  (operator[] materialisation, `= {}` in GenTransformFeedbackNames and
  CreateTransformFeedbackObject) mints a fresh one and a recycled name cannot
  carry the dead object's id back. The allocator is the same shape as
  BufferObject::AllocateLifetimeId (atomic, starts at 1 so a zeroed backend slot
  is never a live object).
- The bound object's id is mirrored in m_boundTransformFeedbackLifetimeId,
  refreshed by RestoreBoundTransformFeedbackState - which every bind, and the
  revert that deleting the bound object performs, goes through - and seeded for
  the default object by the GLContext constructor. GetBoundTransformFeedback
  LifetimeId is therefore a const load. Reading it through operator[] instead
  would have been an INSERT on the per-draw path, and UnorderedMap is
  ska::flat_hash_map, whose rehash invalidates every reference into the
  container, not just its iterators.
- Backend: the UnorderedMap is replaced by a fixed 16-entry owner table, which
  fixes the second half of the same defect - the map was keyed on a value that
  recycles yet was never pruned, so it grew for the life of the context. With
  lifetime ids as keys a map would have grown without bound instead, so the
  bounded table is required, not cosmetic.
- Slot exhaustion: past sixteen owners a group has to be taken over, and the
  victim is chosen among owners with NO OPEN SPAN, which
  GLContext::HasOpenTransformFeedbackSpan answers; an identity no live object
  carries any more answers false, and that is what lets a dead owner's group
  come back. Least-recently-used ALONE would have been exactly the wrong rule:
  GL only permits another object to capture while this one is PAUSED, so the
  paused span these groups exist to protect is by construction the least
  recently used entry, and an LRU takeover would reset the one resume offset
  that still matters. LRU is now only the tie-break among reclaimable groups.
  Sixteen genuinely open spans at once is reported (MGLOG_E_ONCE) rather than
  resolved silently, because whatever is taken then restarts at offset 0.
- Not done, and why: the natural place to hand a group back is
  glEndTransformFeedback, but registering DirectVulkan's EndTransformFeedback
  table entry would flip the test FixupGsStripCaptureOrder makes of that same
  pointer (GL_Drawing.cpp:1255) to decide whether the backend already captured
  in GL's vertex order, silently disabling the geometry-stage strip fixup for
  DirectVulkan. Giving that discriminator a name of its own is a separate
  change; until then the no-open-span rule is what keeps the table honest.
- CurrentXfbCounterSlot asserts the identity is never 0. Zero is the free-slot
  sentinel, so an identity of 0 would match every free slot as "mine" without
  ever claiming one - this bug reintroduced, with no symptom at the call site.
- TransformFeedbackLifetimeIdTest, in its own translation unit, pins the
  frontend halves: an object created on a recycled name must not report the dead
  object's id, the default object has an identity before anything binds it, and
  a PAUSED span still reads as open while another object is bound and capturing
  - which is the whole correctness argument for the eviction rule. The name
  reuse is not simulated: the test asks the real generator and skips (loudly) if
  it never recycled. Still untested: the >16-owners path itself, which needs a
  backend scenario with seventeen capturing objects and there is none.
- Negative controls, each applied then reverted: making a non-bound object's
  span read as closed reddens APausedSpanStaysOpenWhileAnotherObjectCaptures;
  making a vanished identity read as open reddens the same case on its delete
  assertion; dropping the constructor's seeding reddens
  AnObjectAtARecycledNameCarriesAFreshLifetimeId.
- Tested: cmake --build build-linux -j 24 (clean, 166 targets); ctest -L unit
  -j 12 -> 1386/1386 passed; ctest -L integration-gpu -> 866/866 passed
  serially, and 866/866 on one of two -j 8 runs. The other -j 8 run failed
  DirectGLES.PointSizeDemotionScenario.TheDemotionIsActuallyArmedWhenTheEnviron
  mentPinsItOn, a member of the pre-existing parallel-ctest flake family: it
  passes in isolation here, and the unmodified parent tree
  (~/w7/p0-noop-wins-base) reproduces the same family under -j 8.
2026-09-05 20:16:49 -04:00

144 lines
6.9 KiB
C++

// MobileGL - MobileGL/MG_Test/State/TransformFeedbackLifetimeIdTest.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
//
// D21 (plan B v2 §4.7.3): DirectVulkan hands every transform feedback object one of sixteen
// counter-buffer groups, and the group carries that span's resume offset. The map was keyed on
// the GL NAME, which glGenTransformFeedbacks recycles the moment the object is deleted, so an
// object created on a recycled name was served the DEAD object's group together with its
// m_xfbCountersValid / m_xfbLastSeenGeneration entries.
//
// The transform feedback object is a plain struct inside a map rather than a heap object, so the
// reuse to defend against is the NAME's, not an address's - which is why these cases live here
// and not in ObjectLifetimeIdTest.cpp with the heap-allocated object types. Keeping them in
// their own translation unit also keeps the D21 commit textually independent of the rest of the
// branch, which plan B §10.4-5 asks for so it can be cherry-picked to dev on its own.
//
// The second case pins the OTHER half of the backend contract: a bounded slot table has to be
// able to tell an object whose span is still open (and may yet resume) from one whose span is
// closed or whose object is gone.
#include <gtest/gtest.h>
#include "Includes.h"
#include "Init.h"
#include <MG_State/GLState/Core.h>
using namespace MobileGL;
namespace {
MG_State::GLState::GLContext& FreshContext() {
MobileGL::Initialize();
MG_State::pGLContext = MakeUnique<MG_State::GLState::GLContext>();
return *MG_State::pGLContext;
}
} // namespace
TEST(TransformFeedbackLifetimeIdTest, AnObjectAtARecycledNameCarriesAFreshLifetimeId) {
auto& context = FreshContext();
// Before anything is bound. The default object exists from the start of the context, and the
// identity has to exist with it: a backend reading 0 here would match every FREE slot in its
// table without ever claiming one, which is the same bug this id was added to remove.
EXPECT_NE(context.GetBoundTransformFeedbackLifetimeId(), 0u)
<< "the default transform feedback object has no identity until something binds it";
Vector<Uint> names;
context.GenTransformFeedbackNames(1, names);
ASSERT_EQ(names.size(), 1u);
const Uint name = names[0];
ASSERT_NE(name, 0u);
context.BindTransformFeedbackObject(name);
const Uint64 firstId = context.GetBoundTransformFeedbackLifetimeId();
EXPECT_NE(firstId, 0u) << "a live transform feedback object answered to id 0, which is the value a "
"zero-initialised backend slot already carries";
// The default object is a different object and must not share the id.
context.BindTransformFeedbackObject(0);
EXPECT_NE(context.GetBoundTransformFeedbackLifetimeId(), firstId)
<< "the default transform feedback object shares an identity with a generated one";
// Deleting while bound reverts to the default object (GL 4.6 core 13.2.1), which is the
// shape the backend sees; delete from there anyway so the test does not depend on it.
context.BindTransformFeedbackObject(name);
context.MarkTransformFeedbackObjectForDeletion(name);
Vector<Uint> reborn;
context.GenTransformFeedbackNames(1, reborn);
ASSERT_EQ(reborn.size(), 1u);
if (reborn[0] != name) {
GTEST_SKIP() << "inconclusive, not proven: the name generator did not hand the deleted name back, so "
"the recycled-name case was never exercised";
}
context.BindTransformFeedbackObject(reborn[0]);
EXPECT_NE(context.GetBoundTransformFeedbackLifetimeId(), firstId)
<< "a transform feedback object created on a recycled name reports the DEAD object's lifetime id - "
"DirectVulkan would hand it the dead span's counter slot, and with it that span's resume state";
}
// The predicate DirectVulkan's slot table asks before it takes a group over. The case that
// matters is the middle one: object A is PAUSED and another object is bound and capturing, so A
// looks completely idle to a least-recently-used rule while being exactly the object whose
// counters must survive.
TEST(TransformFeedbackLifetimeIdTest, APausedSpanStaysOpenWhileAnotherObjectCaptures) {
auto& context = FreshContext();
Vector<Uint> names;
context.GenTransformFeedbackNames(2, names);
ASSERT_EQ(names.size(), 2u);
const Uint nameA = names[0];
const Uint nameB = names[1];
context.BindTransformFeedbackObject(nameA);
const Uint64 idA = context.GetBoundTransformFeedbackLifetimeId();
EXPECT_FALSE(context.HasOpenTransformFeedbackSpan(idA)) << "an object that never began a span reads as open";
context.BeginTransformFeedback(GL_POINTS, nullptr);
EXPECT_TRUE(context.HasOpenTransformFeedbackSpan(idA));
// Pausing is what makes interleaving legal (ARB_transform_feedback2); the span is still open.
context.SetTransformFeedbackPaused(true);
EXPECT_TRUE(context.HasOpenTransformFeedbackSpan(idA));
// Now the shape the slot table sees: B is bound and capturing, A is paused and untouched.
context.BindTransformFeedbackObject(nameB);
const Uint64 idB = context.GetBoundTransformFeedbackLifetimeId();
EXPECT_NE(idB, idA);
context.BeginTransformFeedback(GL_POINTS, nullptr);
EXPECT_TRUE(context.HasOpenTransformFeedbackSpan(idA))
<< "a paused span stopped reading as open the moment another object was bound - a backend "
"reclaiming slots by 'is this owner still going' would take A's counters away";
EXPECT_TRUE(context.HasOpenTransformFeedbackSpan(idB));
context.EndTransformFeedback();
EXPECT_FALSE(context.HasOpenTransformFeedbackSpan(idB)) << "a closed span still reads as open";
EXPECT_TRUE(context.HasOpenTransformFeedbackSpan(idA));
// A closes its own span; its slot becomes reclaimable.
context.BindTransformFeedbackObject(nameA);
context.EndTransformFeedback();
EXPECT_FALSE(context.HasOpenTransformFeedbackSpan(idA));
// A deleted object can never resume, so its identity must not hold a slot either.
context.BindTransformFeedbackObject(nameB);
context.BeginTransformFeedback(GL_POINTS, nullptr);
EXPECT_TRUE(context.HasOpenTransformFeedbackSpan(idB));
context.MarkTransformFeedbackObjectForDeletion(nameB);
EXPECT_FALSE(context.HasOpenTransformFeedbackSpan(idB))
<< "the identity of a deleted transform feedback object still claims an open span, so its counter "
"group would be pinned for the life of the context";
// Identities the context never issued, and the free-slot sentinel, are not open spans.
EXPECT_FALSE(context.HasOpenTransformFeedbackSpan(0));
EXPECT_FALSE(context.HasOpenTransformFeedbackSpan(~0ull));
}