[Fix] (DirectVulkan, GLImpl, MG_State, ShaderTranspiler): second audit round over the remaining memo sites

Six more verified defects from the residual memo/cache mechanisms:

- Program resource cache (DirectVulkan reflection): glShaderStorageBlockBinding
  deliberately does not bump the backend state version, and the SSO pipeline
  composite is unnamed so the by-name in-place patch can never reach its slot -
  the composite kept serving pre-rebind SSBO bindings. The cache now keys on the
  program's block-binding version; a binding-only change re-applies the overrides
  by name instead of re-running spirv-reflect. SetShaderStorageBlockBinding also
  gains the equality bail-out its uniform-block sibling has, so the composite
  mirror's replay stops churning the version every draw.
- LinkProgram's allowVSOnlyPrograms function-static latch never set its own
  initialized flag (dead memo, re-read every call) - and completing it would have
  frozen a per-backend capability across re-initialization. Replaced with a fresh
  per-link read from the null-checked active backend.
- Query object registry: drained at full library teardown (DestroyAllQueryObjects,
  mirroring DestroyAllSyncObjects) - undeleted queries and their backend wrappers
  leaked across Destroy/Initialize cycles, stale ids stayed IsQuery == GL_TRUE in
  the re-initialized library, and a later delete could hand the old backend's
  wrapper to a different backend's DeleteBackendQuery.
- Converted vertex streams and the host-side EBO max-index scan now SyncGpuWrites
  before reading the coherent mapping: XFB/SSBO/image writes are merely recorded
  at that point, so the conversion read pre-write bytes (the restart-index
  rewrite already synced; these two host reads did not).
- Zero-stride converted bindings: both converters rejected stride 0, making the
  factory's documented single-element conversion unreachable and silently
  dropping every draw using such a binding; the stride is substituted with the
  element size for the one-element case.
- DemoteFloat64Pass block relayout: measurement queued into the module eagerly,
  so a mid-struct failure left a half-relaid-out block (compacted offsets before
  the failing member, 64-bit offsets after) while claiming the block was left
  alone. Decoration writes are now collected and committed only when the whole
  block measures successfully.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MsqQQF7ugn7MqZXcmnmz1z
This commit is contained in:
Claude
2026-08-19 16:48:15 +00:00
parent a4ccf3a837
commit ee908aab46
8 changed files with 158 additions and 18 deletions
+6
View File
@@ -15,6 +15,7 @@
#include <MG_Impl/GLImpl/Texture/ProxyTexture.h>
#include <MG_Impl/GLImpl/Framebuffer/GL_Framebuffer.h>
#include <MG_Impl/GLImpl/Sync/GL_Sync.h>
#include <MG_Impl/GLImpl/Query/GL_Query.h>
#include <MG_Util/Async/ShaderCompilePool.h>
#include <MG_Util/ShaderTranspiler/ShaderCompiler.h>
@@ -51,6 +52,11 @@ namespace MobileGL {
// before a re-initialized library could pair them with the wrong
// backend's DeleteSync).
MG_Impl::GLImpl::DestroyAllSyncObjects();
// Queries die with their contexts for the same reason, and their registry
// is the same shape of process-global map: drain it here too, while the
// function table can still pair each backend handle with the backend that
// minted it.
MG_Impl::GLImpl::DestroyAllQueryObjects();
MG_Backend::pActiveBackendObject.reset();
MG_State::pGLContext.reset();
MG_State::pEGLContext.reset();
@@ -69,6 +69,12 @@ namespace MobileGL::MG_Backend::DirectVulkan {
// slot's ownership unambiguous.
Uint64 programLifetimeId = 0;
Uint32 backendStateVersion = 0;
// glShaderStorageBlockBinding deliberately does NOT bump the backend state
// version, and the pipeline composite is unnamed so the in-place patch in
// DirectVulkan::ShaderStorageBlockBinding can never reach its slot - the
// mirror replay bumps only the program's block-binding version. Without this
// key the composite's slot kept serving the pre-rebind block.binding.
Uint32 blockBindingVersion = 0;
Vector<StorageBlockResource> storageBlocks;
Vector<BufferVariableResource> bufferVariables;
GLint computeWorkGroupSize[3] = {1, 1, 1};
@@ -156,18 +162,33 @@ namespace MobileGL::MG_Backend::DirectVulkan {
auto& cache = g_programResourceCaches[program.GetExternalIndex()];
const Uint64 programLifetimeId = program.GetLifetimeId();
const Uint32 backendStateVersion = program.GetBackendStateVersion();
const Uint32 blockBindingVersion = program.GetBlockBindingVersion();
// The lifetime id must match too: a new program that reuses a deleted
// program's name and happens to land on the same backendStateVersion (both
// count from zero) would otherwise be served the dead program's reflection.
if (cache.programLifetimeId == programLifetimeId &&
cache.backendStateVersion == backendStateVersion &&
(!cache.storageBlocks.empty() || !cache.bufferVariables.empty())) {
if (cache.blockBindingVersion != blockBindingVersion) {
// Only the block bindings moved (glShaderStorageBlockBinding, or the
// pipeline composite's mirror replay - neither touches the backend
// state version): the reflection itself is unchanged, so re-apply the
// overrides by name instead of re-running spirv-reflect. Overrides
// only ever accumulate, so a block without one still holds its
// declared binding.
for (auto& block : cache.storageBlocks) {
const Int rebound = program.GetShaderStorageBlockBindingOverride(block.name);
if (rebound >= 0) block.binding = static_cast<Uint32>(rebound);
}
cache.blockBindingVersion = blockBindingVersion;
}
return cache;
}
cache = {};
cache.programLifetimeId = programLifetimeId;
cache.backendStateVersion = backendStateVersion;
cache.blockBindingVersion = blockBindingVersion;
Vector<SpvReflectShaderModule> modules;
Vector<Bool> validModules;
@@ -3323,6 +3323,11 @@ void main() {
indexView.indexByteSize > bufferSize - indexView.indexByteOffset) {
return false;
}
// Recorded-but-unexecuted GPU writes (XFB capture, SSBO, storage texel
// buffer) land in the coherent mapping this scan is about to read;
// submit-and-wait first, exactly like the restart-index rewrite does.
// A no-op unless the gpu-write flag is set.
indexBufferShared->SyncGpuWrites();
indexBufferShared->SyncPersistentMappedRange();
indexBytes = indexBufferShared->MappedData() + indexView.indexByteOffset;
} else {
@@ -3560,6 +3565,16 @@ void main() {
const Uint8* sourceData, SizeT sourceStride,
SizeT elementSize, SizeT elementCount,
BufferSlice& outSlice) -> Bool {
// A resolved stride of 0 is the binding model's "never advance" (see the
// factory's layout notes): exactly one element is converted and every vertex
// reads it. That single element is read at offset 0, so the stride is never
// actually used - but both converters reject 0 as a degenerate input, which
// made the documented single-element conversion unreachable and silently
// dropped every draw using such a binding. Substitute the element's own
// size; the caller's cache key still carries the distinct stride 0.
if (sourceStride == 0 && elementCount == 1) {
sourceStride = elementSize;
}
const void* uploadData = nullptr;
VkDeviceSize uploadSize = 0;
switch (conversion) {
@@ -3693,6 +3708,12 @@ void main() {
return false;
}
// A GPU-written source (XFB capture, SSBO, storage texel buffer) has its
// bytes produced by commands that are merely RECORDED at this point, and
// MappedData() aliases the coherent GPU memory they will write into -
// converting now would read pre-write garbage. Submit-and-wait first,
// mirroring the restart-index rewrite; a flag-test no-op otherwise.
sourceBufferShared->SyncGpuWrites();
sourceBufferShared->SyncPersistentMappedRange();
const SizeT availableElementCount =
sourceStride == 0 ? 1 : 1 + (sourceSize - baseOffset - elementSize) / sourceStride;
+12 -13
View File
@@ -1057,21 +1057,20 @@ namespace MobileGL::MG_Impl::GLImpl {
return;
}
static Bool allowVSOnlyPrograms;
static Bool initialized = false;
if (!initialized) {
const auto& activeBackendObject = MG_Backend::pActiveBackendObject;
if (!activeBackendObject) {
MGLOG_E_ONCE("activeBackendObject is not initialized!");
return;
}
const auto& rendererInfo = activeBackendObject->GetRendererInfo();
allowVSOnlyPrograms = (Int)rendererInfo.StaticBackendCapability.AllowVSOnlyPrograms;
}
// Read fresh every link, never latched in a static: the capability is
// per-backend, and a latch would freeze it across a backend teardown +
// re-initialization (the previous function-static memo here never even set
// its own initialized flag, so it re-read every call anyway - this makes
// the always-fresh behavior the stated one). A struct-field read per
// glLinkProgram costs nothing.
const auto& activeBackendObject = MG_Backend::pActiveBackendObject;
if (activeBackendObject) {
programObject->SetMaxFragmentOutputColorNumber(activeBackendObject->GetDynamicParameters().MaxDrawBuffers);
if (!activeBackendObject) {
MGLOG_E_ONCE("activeBackendObject is not initialized!");
return;
}
const Bool allowVSOnlyPrograms =
activeBackendObject->GetRendererInfo().StaticBackendCapability.AllowVSOnlyPrograms;
programObject->SetMaxFragmentOutputColorNumber(activeBackendObject->GetDynamicParameters().MaxDrawBuffers);
programObject->Link(!allowVSOnlyPrograms);
}
@@ -648,4 +648,39 @@ namespace MobileGL::MG_Impl::GLImpl {
if (!ValidateQueryStreamIndex(__FUNCTION__, target, index)) return;
GetQueryiv(target, pname, params);
}
void DestroyAllQueryObjects() {
// Detach the registry under the lock, release outside it - same discipline
// (and the same accepted teardown race) as DestroyAllSyncObjects. Without
// this drain, every query the app left undeleted survived full library
// teardown in the process-global registry: the objects and their backend
// wrappers leaked across Destroy/Initialize cycles, stale ids kept
// answering IsQuery == GL_TRUE in the re-initialized library, and a later
// glDeleteQueries could hand the OLD backend's handle to a DIFFERENT
// backend's DeleteBackendQuery, which casts it to the wrong wrapper type.
UnorderedMap<GLuint, QueryObject*> orphans;
{
const std::lock_guard<std::mutex> lock(g_queryObjectsMutex);
orphans.swap(g_liveQueryObjects);
g_activeTimeElapsedQueryId = 0;
g_activePrimitivesWrittenQueryId = 0;
g_activePrimitivesGeneratedQueryId = 0;
g_activeSamplesPassedQueryId = 0;
}
if (orphans.empty()) {
return;
}
// Backend handles must be released by the backend that created them, so
// this runs while the function table is still populated. Both backends'
// DeleteBackendQuery are generation-guarded, so a handle whose renderer
// or ES context is already gone frees only the wrapper.
const auto deleteBackendQuery = MG_Backend::gBackendFunctionsTable.GL.DeleteBackendQuery;
for (const auto& [_, queryObject] : orphans) {
if (deleteBackendQuery && queryObject->backendHandle) {
deleteBackendQuery(queryObject->backendHandle);
}
delete queryObject;
}
MGLOG_D("DestroyAllQueryObjects: reclaimed %zu query object(s) the app left undeleted", orphans.size());
}
} // namespace MobileGL::MG_Impl::GLImpl
+9
View File
@@ -29,4 +29,13 @@ namespace MobileGL::MG_Impl::GLImpl {
void GetQueryBufferObjecti64v(GLuint id, GLuint buffer, GLenum pname, GLintptr offset);
void GetQueryBufferObjectui64v(GLuint id, GLuint buffer, GLenum pname, GLintptr offset);
void QueryCounter(GLuint id, GLenum target);
// Destroys every still-registered query object exactly as DeleteQueries would.
// GL requires queries to die with their context; called only from full library
// teardown (DestroyImpl), where no context survives on any thread, so the
// process-global registry can be drained wholesale. Must run while the backend
// function table is still populated: each backend handle has to be released by
// the backend that created it, never by a later re-initialized one (whose
// DeleteBackendQuery would cast the wrapper to the wrong backend's type).
// Same contract as DestroyAllSyncObjects.
void DestroyAllQueryObjects();
} // namespace MobileGL::MG_Impl::GLImpl
@@ -806,6 +806,13 @@ namespace MobileGL::MG_State::GLState {
// order - and the name is the only coordinate all three agree on. Absent from the map
// means "never rebound", and the shader's declared binding still stands.
void SetShaderStorageBlockBinding(const String& blockName, Uint binding) {
// Equality bail-out like SetUniformBlockBinding's: the pipeline composite
// mirror replays every override each draw, and without this every replay
// would churn m_blockBindingVersion and rebuild whatever keys on it.
const auto it = Artifacts().shaderStorageBlockBinding.find(blockName);
if (it != Artifacts().shaderStorageBlockBinding.end() && it->second == static_cast<Int>(binding)) {
return;
}
Artifacts().shaderStorageBlockBinding[blockName] = static_cast<Int>(binding);
// Deliberately NOT m_backendStateVersion: Espryt's entry point never forces a
// program build off this, and bumping that version would start doing so. The
@@ -91,9 +91,15 @@ namespace MobileGL {
BlockRelayout(IRContext* irContext, Bool std140)
: m_irContext(irContext), m_std140(std140) {}
// Size and alignment of `typeId`, applying every stride decoration it implies
// on the way down. Zero size means "not a type this layout knows how to
// describe"; the caller then leaves the block alone rather than guessing.
// Size and alignment of `typeId`, QUEUING every offset/stride decoration it
// implies on the way down. Zero size means "not a type this layout knows how
// to describe"; the caller then leaves the block alone rather than guessing.
// The queue is what makes that fallback honest: measurement must be
// side-effect-free until it is known to succeed, or a mid-struct failure
// would leave the block half-relaid-out - members before the failing one at
// compacted 32-bit offsets, members after it at the original 64-bit ones, a
// layout matching neither convention. Commit() flushes the queue and is
// called only on a successful Measure of the whole block.
struct Extent {
Uint32 size = 0;
Uint32 alignment = 0;
@@ -108,7 +114,29 @@ namespace MobileGL {
return extent;
}
// Flushes the decoration writes a successful Measure queued. Call exactly
// once, only when Measure returned a non-zero size; a failed measurement's
// queue dies with this per-block instance, leaving the module untouched.
void Commit() {
for (const PendingDecoration& pending : m_pendingWrites) {
if (pending.member) {
ApplyMemberDecoration(pending.targetId, pending.memberIndex, pending.decoration,
pending.value);
} else {
ApplyTypeDecoration(pending.targetId, pending.decoration, pending.value);
}
}
m_pendingWrites.clear();
}
private:
struct PendingDecoration {
Bool member = false;
Uint32 targetId = 0;
Uint32 memberIndex = 0;
spv::Decoration decoration = spv::Decoration::Offset;
Uint32 value = 0;
};
Extent MeasureUncached(Uint32 typeId) {
const Instruction* type = m_irContext->get_def_use_mgr()->GetDef(typeId);
if (type == nullptr) return {};
@@ -204,7 +232,17 @@ namespace MobileGL {
return length->GetSingleWordInOperand(0);
}
// Queue-only during measurement; the module is mutated in Commit().
void SetTypeDecoration(Uint32 targetId, spv::Decoration decoration, Uint32 value) {
m_pendingWrites.push_back({false, targetId, 0, decoration, value});
}
void SetMemberDecoration(Uint32 structId, Uint32 member, spv::Decoration decoration,
Uint32 value) {
m_pendingWrites.push_back({true, structId, member, decoration, value});
}
void ApplyTypeDecoration(Uint32 targetId, spv::Decoration decoration, Uint32 value) {
for (Instruction& annotation : m_irContext->annotations()) {
if (annotation.opcode() != spv::Op::OpDecorate) continue;
if (annotation.GetSingleWordInOperand(0) != targetId) continue;
@@ -216,8 +254,8 @@ namespace MobileGL {
}
}
void SetMemberDecoration(Uint32 structId, Uint32 member, spv::Decoration decoration,
Uint32 value) {
void ApplyMemberDecoration(Uint32 structId, Uint32 member, spv::Decoration decoration,
Uint32 value) {
for (Instruction& annotation : m_irContext->annotations()) {
if (annotation.opcode() != spv::Op::OpMemberDecorate) continue;
if (annotation.GetSingleWordInOperand(0) != structId) continue;
@@ -233,6 +271,7 @@ namespace MobileGL {
IRContext* m_irContext = nullptr;
Bool m_std140 = true;
std::unordered_map<Uint32, Extent> m_extents;
std::vector<PendingDecoration> m_pendingWrites;
};
} // namespace
@@ -529,9 +568,12 @@ namespace MobileGL {
// A member shape the layout rules here do not describe. Leaving the block
// at its 64-bit offsets keeps the module valid for Vulkan; SPIRV-Cross will
// decline it for ESSL, which is the same outcome as before the demotion.
// Nothing was written: Measure only queues, and the queue dies here.
MGLOG_D("DemoteFloat64Pass: block %%%u contains a member this pass cannot lay "
"out; its 64-bit offsets are left in place",
blockType->result_id());
} else {
relayout.Commit();
}
}