From 107669b3dbb5b4913142a66187e8658e012879d2 Mon Sep 17 00:00:00 2001 From: BZLZHH Date: Sun, 9 Aug 2026 11:28:24 -0400 Subject: [PATCH] [Fix, Test] (MG_Impl, MG_Backend/DirectGLES): DSA by-name texture calls corrupted borrowed-slot memo pairings - process-wide glyph death under Iris Field report: on Espryt with a BSL Iris pipeline built, every glyph in the game died - HUD, menu labels, even the vanilla title screen after leaving the world - while sprites kept rendering. Captured on-device (FCL apitrace rig), reproduced headlessly on llvmpipe, and pinned with a three-way replay: the same trace renders full text on raw Mesa desktop GL and on Magma, so the stream was intact and the execution was Espryt's. MECHANISM. WithTemporarilyBoundNamedTexture implements the by-name (DSA) texture entry points by binding the named texture onto the active unit's real slot, running the bound-texture code, and restoring - without moving the texture bind generation on either edge. DirectGLES's per-draw texture sync memo keys on that generation and BORROWS the slot pointer, so a memo built for texture A kept passing every key while a by-name call had texture B sitting in the slot: A's backend twin was driven with B's frontend object, and SyncMipmapsToBackend re-specified A's storage with B's shape. In the trace, a by-name upload to a BSL 2048x2048 map while the 16x16 lightmap was bound re-specified the lightmap's GL texture 2048x2048-NULL and back 16x16-NULL. The lightmap exists only as render output - no glTexSubImage2D ever touches it - so it stayed zero forever, and rendertype_text (vertexColor = Color * texelFetch(lightmap, ...)), alpha-discards every glyph. Background quads never sample the lightmap, which is why only text died. FIX, class-level, two layers: - Frontend (shared, closes the same hole for DirectVulkan's generation- keyed memos): the temporary bind and the restore each bump the texture bind generation (only when the slot actually changed), and the restore is an RAII scope guard so a throwing body can no longer leak the temporary binding - a second latent bug of the same class. Deliberately a generation bump and not a touched-unit note: the high-water mark must not chase by-name calls, and a completed bind/restore pair leaves the content epoch unchanged, so the cost is an owner-compare re-walk, not a memo rebuild. - DirectGLES defense in depth: both borrowed-pair memos (g_unitTextureSyncList, g_fboTextureSyncList) record which frontend texture each backend twin was paired with and re-check it before any replay (last in the key conjunction, behind the context-id compare). A stale pairing now costs a list rebuild instead of silent cross-texture storage corruption. Tests, both red with their own layer reverted: TextureTest.NamedTextureCallKeepsUnitBindingAccountingCoherent (the accounting contract) and DirectGLESTextureSync.UnitMemoRefusesToDriveA- TwinFromAnotherTexture (the corrupting sequence shape against a mock GLES table, asserting the resident texture's storage is never re-specified). 595/595 unit at default and with the async kill switch. Replay evidence: the captured BSL ESC-menu trace renders all text through Espryt post-fix, byte-comparable to the Mesa-direct and Magma replays; the no-shaderpack control is unchanged. A trace fixture wiring this scene into CI follows in a separate commit. --- MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp | 33 ++++- MobileGL/MG_Backend/DirectGLES/Managers.h | 4 + .../MG_Impl/GLImpl/Texture/GL_Texture.cpp | 54 +++++++- MobileGL/MG_Test/SanityTest.cpp | 129 ++++++++++++++++++ MobileGL/MG_Test/Texture/TextureTest.cpp | 50 +++++++ 5 files changed, 261 insertions(+), 9 deletions(-) diff --git a/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp b/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp index 3b3e16d6..bb52bf76 100644 --- a/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp +++ b/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp @@ -1127,10 +1127,29 @@ namespace MobileGL::MG_Backend::DirectGLES { // slot the key covers still holds a reference to it. Holding either side by shared_ptr // instead would keep dead frontend textures alive and defeat the registry's // weak-reference GC. + // + // `texture` records WHICH frontend object `backend` was paired with when the entry was + // built, and PairingsIntact re-checks it before any replay. The keys above are the + // primary guard, but they are all derived state: a slot swap that never reaches the + // bind generation (the DSA by-name emulation used to swap a slot silently) would leave + // every key matching while the borrowed slot pointed at a different texture, and the + // replay would then drive texture A's backend twin from texture B's frontend state - + // re-specifying A's backend storage with B's shape and destroying A's contents. A raw + // pointer compare per entry is far cheaper than the walk it guards, and a stale pairing + // costs only a list rebuild, so this stays as the structural net under the keys. struct UnitTextureSyncEntry { const SharedPtr* slot = nullptr; + MG_State::GLState::ITextureObject* texture = nullptr; BackendTextureObject* backend = nullptr; }; + // True while every entry's borrowed slot still holds the texture the entry was paired + // with. Callers put it LAST in the key conjunction so it only runs on a key hit. + static Bool PairingsIntact(const Vector& list) { + for (const auto& entry : list) { + if (entry.slot->get() != entry.texture) return false; + } + return true; + } static Vector g_unitTextureSyncList; static Bool g_unitTextureSyncListValid = false; static Uint64 g_unitTextureSyncListContextId = 0; @@ -1197,7 +1216,8 @@ namespace MobileGL::MG_Backend::DirectGLES { g_unitTextureSyncListMaxUnit == maxTouchedUnit && g_unitTextureSyncListContextGeneration == g_textureContextGeneration && g_unitTextureSyncListEpoch == unitBindingsEpoch && - g_unitTextureSyncListSamplingGeneration == samplingGeneration) { + g_unitTextureSyncListSamplingGeneration == samplingGeneration && + PairingsIntact(g_unitTextureSyncList)) { for (const auto& entry : g_unitTextureSyncList) { // Aggregate gate == the conjunction of the three callees' own // early-outs (see IsDrawSyncClean); skipping on true is @@ -1219,8 +1239,8 @@ namespace MobileGL::MG_Backend::DirectGLES { // An image-less default texture (name 0) is the slot's initial / "unbound" // state; it has nothing to sync, so skip it as cheaply as the old null slot. if (textureObject && !MG_State::GLState::IsUndefinedDefaultTexture(textureObject.get())) { - g_unitTextureSyncList.push_back( - {&textureObject, SyncTextureObjectToBackend(textureObject).get()}); + g_unitTextureSyncList.push_back({&textureObject, textureObject.get(), + SyncTextureObjectToBackend(textureObject).get()}); } } } @@ -1254,7 +1274,8 @@ namespace MobileGL::MG_Backend::DirectGLES { g_fboTextureSyncListSlotVersion == fboSlotVersion && g_fboTextureSyncListObjectVersion == fboObjectVersion && g_fboTextureSyncListContextId == keys.contextId && - g_fboTextureSyncListContextGeneration == g_textureContextGeneration; + g_fboTextureSyncListContextGeneration == g_textureContextGeneration && + PairingsIntact(g_fboTextureSyncList); if (fboListValid) { for (const auto& entry : g_fboTextureSyncList) { // Same aggregate gate as the unit list above. @@ -1273,8 +1294,8 @@ namespace MobileGL::MG_Backend::DirectGLES { if (!attachment.IsTexture()) continue; auto& textureObject = attachment.GetTexture(); if (textureObject) { - g_fboTextureSyncList.push_back( - {&textureObject, SyncTextureObjectToBackend(textureObject).get()}); + g_fboTextureSyncList.push_back({&textureObject, textureObject.get(), + SyncTextureObjectToBackend(textureObject).get()}); } } g_fboTextureSyncListFbo = currentFBO.get(); diff --git a/MobileGL/MG_Backend/DirectGLES/Managers.h b/MobileGL/MG_Backend/DirectGLES/Managers.h index a940f08f..d3458e0d 100644 --- a/MobileGL/MG_Backend/DirectGLES/Managers.h +++ b/MobileGL/MG_Backend/DirectGLES/Managers.h @@ -630,6 +630,10 @@ namespace MobileGL::MG_Backend::DirectGLES { SharedPtr& SyncTextureObjectToBackend( const SharedPtr& textureObject, Bool imageBindableStorageRequired = false); + // Brings every texture the next draw reads - the touched units' bindings and the draw + // FBO's texture attachments - onto the backend, through the two borrowed-pair memos + // documented at their definitions. Declared here so tests can drive those memos directly. + void SyncNeccessaryTextures(); extern Array, MG_State::GLState::TextureState::MAX_TEXTURE_IMAGE_UNITS> g_boundTexturesCache; diff --git a/MobileGL/MG_Impl/GLImpl/Texture/GL_Texture.cpp b/MobileGL/MG_Impl/GLImpl/Texture/GL_Texture.cpp index aea8e676..b1eae2ef 100644 --- a/MobileGL/MG_Impl/GLImpl/Texture/GL_Texture.cpp +++ b/MobileGL/MG_Impl/GLImpl/Texture/GL_Texture.cpp @@ -86,17 +86,65 @@ namespace MobileGL::MG_Impl::GLImpl { return false; } + // DSA emulation: the by-name entry points are implemented by putting the named texture + // on the active unit's slot for their target, running the classic bound-texture code, + // then putting the previous binding back. + // + // Both of those binds are REAL changes to "which texture is bound at this unit" for as + // long as `fn` runs, so both have to move the texture bind generation. Backends memoise + // per-unit work keyed on that generation and BORROW the binding slot (they hold a + // pointer to the slot's shared_ptr, not a copy); a slot swap the generation never saw + // let such a memo replay texture A's backend twin against texture B now sitting in the + // slot - which re-specified A's backend storage with B's shape and silently destroyed + // A's GPU-rendered contents (Minecraft's lightmap, blanked by a by-name upload to an + // Iris shadow map, which then discarded every glyph). + // + // The generation is bumped directly rather than through NoteTextureUnitTouched because + // the touched-unit HIGH-WATER MARK must NOT move: glActiveTexture does not advance it, + // so a DSA-only app would otherwise have every later draw walk up to the highest unit it + // ever aimed a by-name call at. Not advancing it is also sufficient - a unit above the + // mark is outside every memo's coverage and outside the epoch walk, so nothing can + // observe the transient swap there; at or below it, the bump is exactly what makes the + // epoch re-derive. Bumping only on a real change keeps the very common redundant case (a + // by-name call on the texture already bound to the active unit) free. + // + // The restore is a scope guard because `fn` can throw (the unsupported-state paths use + // THROW_EXCEPTION): leaking the temporary binding would leave the wrong texture bound to + // a live unit for the rest of the context's life. template void WithTemporarilyBoundNamedTexture(const SharedPtr& textureObject, Fn&& fn) { if (!textureObject) return; - auto& activeUnit = MG_State::pGLContext->GetTextureUnitObject(MG_State::pGLContext->GetActiveTextureUnit()); + const Int activeUnitIndex = MG_State::pGLContext->GetActiveTextureUnit(); + auto& activeUnit = MG_State::pGLContext->GetTextureUnitObject(activeUnitIndex); auto& bindingSlot = activeUnit.GetBindingSlot(textureObject->GetTarget()); const auto previousBinding = bindingSlot.GetBoundObject(); - bindingSlot.Bind(textureObject); + + using SlotType = std::remove_reference_t; + class ScopedSlotRestore { + public: + ScopedSlotRestore(SlotType& slot, SharedPtr previous) + : m_slot(slot), m_previous(Move(previous)) {} + ~ScopedSlotRestore() { + if (m_slot.Bind(m_previous)) { + MG_State::pGLContext->BumpTextureBindGeneration(); + } + } + ScopedSlotRestore(const ScopedSlotRestore&) = delete; + ScopedSlotRestore& operator=(const ScopedSlotRestore&) = delete; + + private: + SlotType& m_slot; + SharedPtr m_previous; + }; + + if (bindingSlot.Bind(textureObject)) { + MG_State::pGLContext->BumpTextureBindGeneration(); + } + ScopedSlotRestore restore(bindingSlot, previousBinding); + fn(MG_Util::ConvertTextureTargetToGLEnum(textureObject->GetTarget())); - bindingSlot.Bind(previousBinding); } SizeT ComputeTextureStorageByteSize(TextureInternalFormat textureInternalFormat, GLsizei width, GLsizei height, diff --git a/MobileGL/MG_Test/SanityTest.cpp b/MobileGL/MG_Test/SanityTest.cpp index ddff2bc9..be60fd2e 100644 --- a/MobileGL/MG_Test/SanityTest.cpp +++ b/MobileGL/MG_Test/SanityTest.cpp @@ -1898,3 +1898,132 @@ TEST(FastSTLSanity, ErasingTheOnlyElementReturnsEnd) { EXPECT_EQ(next, map.end()); EXPECT_TRUE(map.empty()); } + +namespace { + // Records what the per-unit texture sync actually pushed at the driver: which backend + // texture id was current when each glTexImage2D landed, and the shape it was given. + struct TexSpecCall { + GLuint texture; + GLsizei width; + GLsizei height; + }; + MobileGL::Vector* g_texSpecCalls = nullptr; + GLuint g_texSpecBoundTexture = 0; + + void TS_BindTexture(GLenum, GLuint texture) { g_texSpecBoundTexture = texture; } + void TS_ActiveTexture(GLenum) {} + void TS_TexParameteri(GLenum, GLenum, GLint) {} + void TS_TexParameterf(GLenum, GLenum, GLfloat) {} + void TS_TexParameterfv(GLenum, GLenum, const GLfloat*) {} + void TS_PixelStorei(GLenum, GLint) {} + void TS_BindBuffer(GLenum, GLuint) {} + void TS_TexImage2D(GLenum, GLint level, GLint, GLsizei width, GLsizei height, GLint, GLenum, GLenum, + const void*) { + if (g_texSpecCalls && level == 0) { + g_texSpecCalls->push_back({g_texSpecBoundTexture, width, height}); + } + } + + // Clears the recording hook even when a gtest assertion unwinds the test body. + struct ScopedTexSpecRecording { + explicit ScopedTexSpecRecording(MobileGL::Vector& sink) { + g_texSpecCalls = &sink; + g_texSpecBoundTexture = 0; + } + ~ScopedTexSpecRecording() { g_texSpecCalls = nullptr; } + ScopedTexSpecRecording(const ScopedTexSpecRecording&) = delete; + ScopedTexSpecRecording& operator=(const ScopedTexSpecRecording&) = delete; + }; + + // Gives `name` a complete single-level 2D image of the requested size without going through + // the frontend upload path (the mock table below wires only the state-pushing entry points). + MobileGL::SharedPtr MakeComplete2DTexture(GLuint name, + MobileGL::Int size) { + using namespace MobileGL; + MG_Impl::GLImpl::BindTexture(GL_TEXTURE_2D, name); + auto object = MG_State::pGLContext->GetTextureUnitObject(0) + .GetBindingSlot(TextureTarget::Texture2D) + .GetBoundObject(); + object->SetInternalFormat(TextureInternalFormat::RGBA8); + MG_State::GLState::AsMipmapTexture(object.get()) + ->AllocateStorage(TextureUploadTarget::Texture2D, 0, {{size, size, 1}, 4}); + return object; + } +} // namespace + +// The per-unit texture sync memo BORROWS the binding slot: an entry holds a pointer to the +// slot's shared_ptr plus the backend twin of whatever was in it when the entry was built. Its +// keys (context id, bind-generation epoch, high-water mark, sampling generation) are the primary +// guard, but they are all derived state - so the memo also has to survive a slot swap that never +// reached them. +// +// It did not. The DSA by-name emulation swapped a slot silently, every key still matched, and +// the replay drove texture A's backend twin from texture B's frontend object: A's backend +// storage was re-specified with B's shape, destroying anything A only ever had on the GPU. On +// Espryt + Iris/BSL that blanked Minecraft's 16x16 lightmap the moment a 2048x2048 shadow map +// was uploaded through a by-name call, and since the text shader multiplies by the lightmap, +// `if (color.a < 0.1) discard` then threw away every glyph in the process - HUD, menus and the +// vanilla title screen alike. +TEST(DirectGLESTextureSync, UnitMemoRefusesToDriveATwinFromAnotherTexture) { + using namespace MobileGL; + ScopedDirectGLESTextureBindings scoped; // fresh GLContext + registry + binding caches + Vector specs; + ScopedTexSpecRecording recording(specs); + + auto functions = MG_Backend::DirectGLES::g_GLESFuncs; + functions.glBindTexture = TS_BindTexture; + functions.glActiveTexture = TS_ActiveTexture; + functions.glTexImage2D = TS_TexImage2D; + functions.glTexParameteri = TS_TexParameteri; + functions.glTexParameterf = TS_TexParameterf; + functions.glTexParameterfv = TS_TexParameterfv; + functions.glPixelStorei = TS_PixelStorei; + functions.glBindBuffer = TS_BindBuffer; + MG_Backend::DirectGLES::SetGLESFuncsTable(functions); + + GLuint names[2] = {}; + MG_Impl::GLImpl::GenTextures(2, names); + // `foreign` stands in for the shadow map, `resident` for the lightmap. Both are fully + // specified BEFORE the first sync so that nothing between the two syncs can move the + // sampling-resolution generation and invalidate the memo for an unrelated reason. + const auto foreign = MakeComplete2DTexture(names[1], 32); + const auto resident = MakeComplete2DTexture(names[0], 16); + ASSERT_NE(foreign, nullptr); + ASSERT_NE(resident, nullptr); + + // First sync: builds the memo with unit 0 -> `resident`, and gives `resident`'s twin its + // 16x16 backend storage. + MG_Backend::DirectGLES::TextureImpl::SyncNeccessaryTextures(); + auto* residentSlot = MG_Backend::DirectGLES::TextureImpl::g_backendTextureObjects.Find(resident.get()); + ASSERT_NE(residentSlot, nullptr); + ASSERT_NE(*residentSlot, nullptr); + const GLuint residentBackendId = (*residentSlot)->GetBackendTextureId(); + ASSERT_NE(residentBackendId, 0u); + ASSERT_FALSE(specs.empty()); + EXPECT_EQ(specs.back().texture, residentBackendId); + EXPECT_EQ(specs.back().width, 16); + + // The hazard, reproduced at the state level: put `foreign` on the slot the memo borrows + // WITHOUT telling the binding accounting, exactly as the by-name emulation used to. + MG_State::pGLContext->GetTextureUnitObject(0).GetBindingSlot(TextureTarget::Texture2D).Bind(foreign); + + const SizeT specsBeforeReplay = specs.size(); + MG_Backend::DirectGLES::TextureImpl::SyncNeccessaryTextures(); + + // `foreign` must have been synced through its OWN twin... + auto* foreignSlot = MG_Backend::DirectGLES::TextureImpl::g_backendTextureObjects.Find(foreign.get()); + ASSERT_NE(foreignSlot, nullptr); + ASSERT_NE(*foreignSlot, nullptr); + const GLuint foreignBackendId = (*foreignSlot)->GetBackendTextureId(); + EXPECT_NE(foreignBackendId, residentBackendId); + + // ...and above all, nothing may have re-specified the RESIDENT texture's backend storage. + // That single call is what destroyed the lightmap. + for (SizeT i = specsBeforeReplay; i < specs.size(); ++i) { + EXPECT_NE(specs[i].texture, residentBackendId) + << "the stale memo entry re-specified the resident texture's backend storage with " + << specs[i].width << "x" << specs[i].height; + } + + MG_Impl::GLImpl::BindTexture(GL_TEXTURE_2D, 0); +} diff --git a/MobileGL/MG_Test/Texture/TextureTest.cpp b/MobileGL/MG_Test/Texture/TextureTest.cpp index 1dedbe70..a4d5004b 100644 --- a/MobileGL/MG_Test/Texture/TextureTest.cpp +++ b/MobileGL/MG_Test/Texture/TextureTest.cpp @@ -2835,3 +2835,53 @@ TEST_F(TextureTest, BindSamplerRejectsUnitsBeyondMaxCombinedTextureImageUnits) { MG_Impl::GLImpl::DeleteSamplers(1, &sampler); DrainPendingGlErrors(); } + +// The DSA by-name entry points are emulated by temporarily binding the named texture onto the +// active unit's slot for its target, running the classic bound-texture code, then putting the +// previous binding back. For as long as the emulated call runs, that swap is a REAL change to +// which texture is bound at that unit, so both transitions have to move the texture bind +// generation. +// +// They used to move nothing. Backends memoise per-unit work keyed on the bind generation and +// BORROW the binding slot (they hold a pointer to the slot's shared_ptr, not a copy), so a memo +// built while texture A sat in the slot stayed "valid" while B was temporarily in it - and the +// backend then drove A's backend twin from B's frontend state, re-specifying A's backend storage +// with B's shape. Any content A only ever had on the GPU was gone. That is what blanked +// Minecraft's lightmap when Iris uploaded to a BSL shadow map: the text shader multiplies by the +// lightmap, so `if (color.a < 0.1) discard` then threw away every glyph in the process. +TEST_F(TextureTest, NamedTextureCallKeepsUnitBindingAccountingCoherent) { + GLuint names[2] = {}; + MG_Impl::GLImpl::GenTextures(2, names); + const GLuint boundName = names[0]; + const GLuint namedName = names[1]; + + MG_Impl::GLImpl::ActiveTexture(GL_TEXTURE0); + // Instantiate both as 2D objects, then leave `boundName` on the unit. + MG_Impl::GLImpl::BindTexture(GL_TEXTURE_2D, namedName); + MG_Impl::GLImpl::BindTexture(GL_TEXTURE_2D, boundName); + ASSERT_EQ(MG_Impl::GLImpl::GetError(), GL_NO_ERROR); + + auto& slot = MG_State::pGLContext->GetTextureUnitObject(0).GetBindingSlot(TextureTarget::Texture2D); + const auto boundObject = slot.GetBoundObject(); + ASSERT_NE(boundObject, nullptr); + ASSERT_EQ(boundObject->GetExternalIndex(), boundName); + + // TextureParameteriv is one of the by-name calls that is emulated by binding: it reaches + // WithTemporarilyBoundNamedTexture, unlike the scalar TextureParameteri, which edits the + // object directly and never touches a unit. + const Uint64 base = MG_State::pGLContext->GetTextureBindGeneration(); + const GLint maxLevel = 0; + MG_Impl::GLImpl::TextureParameteriv(namedName, GL_TEXTURE_MAX_LEVEL, &maxLevel); + EXPECT_EQ(MG_Impl::GLImpl::GetError(), GL_NO_ERROR); + + // The emulation put `namedName` on the unit and took it off again. A generation-keyed memo + // must be able to see that the slot it borrows was not stable across the call. + EXPECT_GT(MG_State::pGLContext->GetTextureBindGeneration(), base) + << "a by-name texture call swapped a live unit binding without moving the bind generation"; + // ...and the application-visible binding is exactly what it was before the call. + EXPECT_EQ(slot.GetBoundObject(), boundObject); + + MG_Impl::GLImpl::BindTexture(GL_TEXTURE_2D, 0); + MG_Impl::GLImpl::DeleteTextures(2, names); + DrainPendingGlErrors(); +}