From 794c10e56c1435cb6f1d18ba19164809160f14d4 Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Tue, 11 Aug 2026 23:04:11 -0400 Subject: [PATCH 1/3] [Fix] (3rdparty/glslang): bump the fork to the invariant-placement and atomic-counter-lowering patches --- 3rdparty/glslang | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/3rdparty/glslang b/3rdparty/glslang index beaa9532..6f125987 160000 --- a/3rdparty/glslang +++ b/3rdparty/glslang @@ -1 +1 @@ -Subproject commit beaa9532a20832e5a310a7f85ed330e6a7db6898 +Subproject commit 6f12598784a553f61568eb7340cfcffd2a502317 From bcb0e894ef42aa7285f0652127532fd2fd58ec8b Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Tue, 11 Aug 2026 23:04:55 -0400 Subject: [PATCH 2/3] [Fix, Test] (MG_State, MG_Backend/DirectGLES, MG_Util, MG_IntegrationTest, MG_Test): array layout(binding=N) elements bind consecutively, a read+write image reaches ESSL legally, compute local_size comes from the linked intermediate, and glShaderStorageBlockBinding is baked into the generated source --- MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp | 12 +- MobileGL/MG_Backend/DirectGLES/Managers.cpp | 107 ++++- MobileGL/MG_Backend/DirectGLES/Managers.h | 31 +- MobileGL/MG_Backend/DirectGLES/Utils.cpp | 347 +++++++++++++++ MobileGL/MG_Backend/DirectGLES/Utils.h | 34 ++ MobileGL/MG_IntegrationTest/CMakeLists.txt | 1 + .../Scenarios/Glsl420DeclarationScenario.cpp | 400 ++++++++++++++++++ .../Scenarios/ProgramPipelineScenario.cpp | 28 +- .../GLState/ProgramState/ProgramLinkTask.cpp | 104 +++-- .../MG_Test/Backend/DirectGLES/CMakeLists.txt | 20 + .../Backend/DirectGLES/EsslShaderPassTest.cpp | 255 +++++++++++ MobileGL/MG_Test/CMakeLists.txt | 3 + .../MG_Util/ShaderTranspiler/SpvcSession.cpp | 37 ++ .../MG_Util/ShaderTranspiler/SpvcSession.h | 11 + 14 files changed, 1313 insertions(+), 77 deletions(-) create mode 100644 MobileGL/MG_IntegrationTest/Scenarios/Glsl420DeclarationScenario.cpp create mode 100644 MobileGL/MG_Test/Backend/DirectGLES/CMakeLists.txt create mode 100644 MobileGL/MG_Test/Backend/DirectGLES/EsslShaderPassTest.cpp diff --git a/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp b/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp index ddb042b6..64f0303c 100644 --- a/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp +++ b/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp @@ -2077,11 +2077,21 @@ namespace MobileGL::MG_Backend::DirectGLES { // A link-version mismatch means the program was relinked: the backend // shaders and every cache built by CacheResourceLocations (block // indices, sampler locations, UBO upload gate) are stale. + // + // The storage-block signature is the same shape of condition: ES cannot move a + // storage block's binding after link, so glShaderStorageBlockBinding is honoured by + // baking the effective binding into the generated ESSL - which makes a program built + // against a different override set stale. It is compared HERE rather than acted on in + // the entry point because that one must never trigger a build (see + // ShaderStorageBlockBinding below). The signature is over the values, so an + // application that re-sets the same bindings every frame rebuilds nothing. if (!twin->GetBackendProgramId() || twin->GetSyncedLinkVersion() != currentProgram->GetLinkVersion() || twin->GetSnormFallbackClampOutputMask() != g_snormFallbackClampOutputMask || twin->GetUnormFallbackClampOutputMask() != g_unormFallbackClampOutputMask || - twin->GetFragColorBroadcastCount() != g_fragColorBroadcastCount) { + twin->GetFragColorBroadcastCount() != g_fragColorBroadcastCount || + twin->GetShaderStorageBlockBindingSignature() != + ComputeShaderStorageBlockBindingSignature(*currentProgram)) { twin->SyncToBackend(currentProgram); } g_currentDrawFrontendProgram = currentProgram.get(); diff --git a/MobileGL/MG_Backend/DirectGLES/Managers.cpp b/MobileGL/MG_Backend/DirectGLES/Managers.cpp index 83f83495..f6f3f0ce 100644 --- a/MobileGL/MG_Backend/DirectGLES/Managers.cpp +++ b/MobileGL/MG_Backend/DirectGLES/Managers.cpp @@ -4182,6 +4182,31 @@ namespace MobileGL::MG_Backend::DirectGLES { } } + Uint64 ComputeShaderStorageBlockBindingSignature( + const MG_State::GLState::ProgramObject& stateProgramObject) { + const auto& overrides = stateProgramObject.GetShaderStorageBlockBindingOverrides(); + if (overrides.empty()) return 0; // the overwhelming majority of programs + // Order-independent on purpose: the source is an UnorderedMap, so any signature that + // depended on iteration order would differ between two identical override sets and + // rebuild the program for nothing. + // + // Built from the VALUES, not from a change counter, so re-setting a block to the + // binding it already carries produces the same signature and forces no rebuild - an + // application that calls glShaderStorageBlockBinding every frame with unchanged + // arguments must not retranspile every frame. + Uint64 signature = 0; + for (const auto& [blockName, binding] : overrides) { + if (binding < 0) continue; // never rebound; the declared qualifier still stands + Uint64 entry = std::hash{}(blockName); + // Mixed rather than merely summed with the name hash: name and binding must not + // be able to trade places between two entries and cancel out. + entry ^= (static_cast(static_cast(binding)) + 0x9e3779b97f4a7c15ull + + (entry << 6) + (entry >> 2)); + signature += entry; // commutative combine + } + return signature; + } + void BackendProgramObjectImpl::SyncToBackend( const SharedPtr& stateProgramObject) { #ifdef TRACY_ENABLE @@ -4191,6 +4216,9 @@ namespace MobileGL::MG_Backend::DirectGLES { MGLOG_E("State program object is null, skipping backend sync."); return; } + // Recorded before either early return below, so Use() can always name the GL + // program a no-op draw belongs to - including the "linked but not drawable" exit. + m_frontendProgramId = stateProgramObject->GetExternalIndex(); // GetSpirvStatus() as well as GetLinkStatus(): a program whose phase-B job was // cancelled (teardown) or whose optimizer run failed is fully linked and fully @@ -4214,6 +4242,11 @@ namespace MobileGL::MG_Backend::DirectGLES { m_snormFallbackClampOutputMask = g_snormFallbackClampOutputMask; m_unormFallbackClampOutputMask = g_unormFallbackClampOutputMask; m_fragColorBroadcastCount = g_fragColorBroadcastCount; + // The generated ESSL bakes these in (see the SetShaderStorageBlockBinding call in the + // transpile loop below), so the set they were generated against is part of what makes + // this build current - the draw path compares the signature and rebuilds on a change. + const auto& storageBlockBindingOverrides = stateProgramObject->GetShaderStorageBlockBindingOverrides(); + m_shaderStorageBlockBindingSignature = ComputeShaderStorageBlockBindingSignature(*stateProgramObject); // Detach all existing shaders GLint attachedCount = 0; @@ -4326,6 +4359,15 @@ namespace MobileGL::MG_Backend::DirectGLES { spvcSession.SetOptions(options); + // ES fixes a storage block's binding at link from its layout(binding=) qualifier + // and has no glShaderStorageBlockBinding to move it afterwards, so a rebinding + // can only be honoured by printing it INTO the qualifier. Rewriting the Binding + // decoration before SPIRV-Cross emits is what does that; RemoveLayoutBinding + // then deliberately preserves the qualifier for `buffer` declarations. + if (!storageBlockBindingOverrides.empty()) { // empty for almost every program + spvcSession.SetShaderStorageBlockBinding(storageBlockBindingOverrides); + } + const char* result = nullptr; spvcSession.Compile(&result); @@ -4342,6 +4384,15 @@ namespace MobileGL::MG_Backend::DirectGLES { source = result; source = RebindImageUniformsToFrontendUnits(std::move(source), stateProgramObject); + // Wedged between those two on purpose: + // * AFTER RebindImageUniformsToFrontendUnits, so the binding it copies onto + // both halves of a split image is already the frontend texture unit (and so + // that pass never has to reason about the alias it introduces); + // * BEFORE RemoveLayoutBinding, whose keepBindingRegex recognises an image + // declaration and preserves its binding - an image unit cannot be set from + // the API in ES, so the qualifier is the only binding mechanism there is, + // and both halves of the pair have to still be carrying theirs when it runs. + source = SplitReadWriteImageUniforms(source); source = RemoveLayoutBinding(source); source = ProcessOutColorLocations(source); source = ForceFlatIntegerVaryings(source, glShaderType); @@ -4478,11 +4529,12 @@ namespace MobileGL::MG_Backend::DirectGLES { } CacheResourceLocations(stateProgramObject); - // AFTER the link, because glShaderStorageBlockBinding needs the driver's linked - // interface. This is the only place Espryt applies a rebinding: the frontend - // record is authoritative and the glShaderStorageBlockBinding entry point itself - // deliberately never forces a program build (see DirectGLES.cpp), so a rebinding - // requested while no backend program existed yet arrives here instead. + // NOT the mechanism that makes a rebinding work - the transpiled qualifier above is. + // glShaderStorageBlockBinding is a GL 4.3 entry point that no real ES driver exposes, + // so this replay is a no-op almost everywhere; it stays because it is still correct + // (and cheaper than a rebuild) on a driver that does expose it, e.g. a desktop GL + // driver used as the ES backend. AFTER the link either way, because it needs the + // driver's linked interface. ReseedShaderStorageBlockBindings(m_backendProgramId, *stateProgramObject); m_syncedLinkVersion = stateProgramObject->GetLinkVersion(); @@ -4490,6 +4542,24 @@ namespace MobileGL::MG_Backend::DirectGLES { MGLOG_D("Program sync completed. backend ID %u", m_backendProgramId); } + namespace { + // The GL name of the array element that lives at `location`, given the reflection + // name reported for it. Reflection reports one name per UNIFORM ("goku[0]") but + // one location per ELEMENT, so a caller walking locations sees the same name + // repeatedly; this turns it back into "goku[k]". Anything that is not an array + // (or whose base location cannot be resolved) comes back unchanged, so the only + // behaviour that moves is the array case. + String SubscriptUniformNameForElement(const MG_State::GLState::ProgramObject& program, const String& name, + Uint location) { + if (name.size() < 3 || name.compare(name.size() - 3, 3, "[0]") != 0) return name; + const Int base = program.GetUniformLocation(name); + if (base < 0 || static_cast(base) > location) return name; + const Uint element = location - static_cast(base); + if (element == 0) return name; + return name.substr(0, name.size() - 3) + "[" + std::to_string(element) + "]"; + } + } // namespace + // Resolves every name-based resource lookup once per link so the per-draw path // (BindCurrentProgramWithResources) never issues glGetUniformBlockIndex / // glGetUniformLocation string queries; block-to-binding-point assignments are @@ -4552,7 +4622,17 @@ namespace MobileGL::MG_Backend::DirectGLES { // is an INVALID_OPERATION. continue; } - const Int backendLoc = g_GLESFuncs.glGetUniformLocation(m_backendProgramId, name.c_str()); + // Reflection names an array uniform after its FIRST element ("goku[0]") at + // every location the array spans, so asking the driver for that one name + // once per location hands back the same backend location N times. The + // per-draw pass then issues N glUniform1i calls against it and only the + // last element's unit survives - "layout(binding = 1) uniform sampler2D + // goku[7]" ended up with goku[0] on unit 7 and goku[1..6] still on 0. + // Address each element by its own name instead; the frontend already + // reserves one location per element, so the element index is the distance + // from the array's base location. + const String elementName = SubscriptUniformNameForElement(*stateProgramObject, name, loc); + const Int backendLoc = g_GLESFuncs.glGetUniformLocation(m_backendProgramId, elementName.c_str()); if (backendLoc < 0) continue; SamplerUniformBinding binding; binding.frontendLocation = loc; @@ -4561,8 +4641,8 @@ namespace MobileGL::MG_Backend::DirectGLES { binding.lastAssignedUnit = -1; // Present only for the samplers EmulateTextureLodBias actually rewrote; the // pass names it after the sampler, which SPIRV-Cross preserves verbatim. - binding.lodBiasLocation = - g_GLESFuncs.glGetUniformLocation(m_backendProgramId, (String(LOD_BIAS_UNIFORM_PREFIX) + name).c_str()); + binding.lodBiasLocation = g_GLESFuncs.glGetUniformLocation( + m_backendProgramId, (String(LOD_BIAS_UNIFORM_PREFIX) + elementName).c_str()); binding.lastAssignedLodBias = 0.0f; m_samplerUniformBindings.push_back(binding); } @@ -4581,6 +4661,17 @@ namespace MobileGL::MG_Backend::DirectGLES { if (g_lastUsedBackendProgramId == programToBind) { return; } + if (!m_backendProgramUsable) { + // MGLOG_I, not MGLOG_W: at MOBILEGL_LOG_LEVEL_INFO - the level the shipped + // fordebug builds compile at - only I and F survive, and this is precisely the + // line those builds need. Every draw made with this program renders nothing and + // raises no GL error, so without it the only symptom is a framebuffer that kept + // its clear colour. The early return above keeps it to at most one line per + // program state change, not one per draw. + MGLOG_I("Backend program for GL program %u is unusable (a shader failed to transpile, " + "compile or link); binding program 0 - draws with it will render nothing", + m_frontendProgramId); + } MGLOG_D("Using program %u", programToBind); g_GLESFuncs.glUseProgram(programToBind); g_lastUsedBackendProgramId = programToBind; diff --git a/MobileGL/MG_Backend/DirectGLES/Managers.h b/MobileGL/MG_Backend/DirectGLES/Managers.h index ddaa6aed..6cc9b369 100644 --- a/MobileGL/MG_Backend/DirectGLES/Managers.h +++ b/MobileGL/MG_Backend/DirectGLES/Managers.h @@ -1028,6 +1028,11 @@ namespace MobileGL::MG_Backend::DirectGLES { Uint32 GetSnormFallbackClampOutputMask() const { return m_snormFallbackClampOutputMask; } Uint32 GetUnormFallbackClampOutputMask() const { return m_unormFallbackClampOutputMask; } Uint GetFragColorBroadcastCount() const { return m_fragColorBroadcastCount; } + // Signature of the glShaderStorageBlockBinding override set the generated ESSL was + // transpiled against (ES can only express a storage-block binding as the declared + // qualifier, so the overrides are baked into the source). A mismatch means the + // program is stale exactly like the clamp masks above. + Uint64 GetShaderStorageBlockBindingSignature() const { return m_shaderStorageBlockBindingSignature; } Bool HasGlobalUboBlock() const { return m_globalUboBackendBlockIndex >= 0; } const Vector& GetUniformBlockBackendIndices() const { return m_uniformBlockBackendIndices; } @@ -1048,6 +1053,10 @@ namespace MobileGL::MG_Backend::DirectGLES { void CacheResourceLocations(const SharedPtr& stateProgramObject); Uint m_backendProgramId = 0; + // GL name of the frontend program this was last synced from; diagnostics only, so + // an unusable backend program can be traced back to the glCreateProgram id the app + // knows it by. + Uint m_frontendProgramId = 0; Uint m_backendGlobalUBOId = 0; Int m_baseInstanceUniformLocation = -1; Int m_drawIdUniformLocation = -1; @@ -1058,6 +1067,8 @@ namespace MobileGL::MG_Backend::DirectGLES { // Draw buffers a legacy gl_FragColor write has to reach (see // PrgramImpl::BroadcastLegacyFragColor); 1 keeps the plain single-output shader. Uint m_fragColorBroadcastCount = 1; + // 0 is the signature of an empty override set, i.e. what almost every program has. + Uint64 m_shaderStorageBlockBindingSignature = 0; Bool m_isInitialized = false; Bool m_backendProgramUsable = false; @@ -1091,14 +1102,26 @@ namespace MobileGL::MG_Backend::DirectGLES { // on the backend program (eliminated as unused, or the driver lacks the entry // points), which is not an error - GL_BUFFER_BINDING is served from the frontend // record either way. + // + // NOT how a rebinding reaches the shader. glShaderStorageBlockBinding has no ES + // equivalent and is absent from every real ES driver, so this is a no-op there; + // SyncToBackend bakes the effective binding into the ESSL it generates instead + // (SpvcSession::SetShaderStorageBlockBinding). This is kept as the cheaper path on + // a driver that does happen to expose the entry point. Bool ApplyShaderStorageBlockBinding(Uint backendProgramId, const String& blockName, Uint binding); // Replays every glShaderStorageBlockBinding recorded on the program onto a backend - // program that was just built. The frontend record is authoritative (only the - // shader's DECLARED binding survives in the SPIR-V), so without this replay any - // rebuild would silently revert rebound blocks. Mirrors DirectVulkan's - // reseed-on-rebuild in BuildProgramResourceCache. + // program that was just built - best effort, on the same "only where the driver has + // the entry point" terms as ApplyShaderStorageBlockBinding above. Mirrors + // DirectVulkan's reseed-on-rebuild in BuildProgramResourceCache. void ReseedShaderStorageBlockBindings(Uint backendProgramId, const MG_State::GLState::ProgramObject& stateProgramObject); + // Order-independent digest of the program's glShaderStorageBlockBinding overrides. + // The generated ESSL carries them (ES has no way to move a storage block's binding + // after link), so a program built against a different set is stale and the draw path + // has to rebuild it. Computed from the values, so re-setting a block to the binding it + // already has costs nothing. 0 when nothing was ever rebound. + Uint64 ComputeShaderStorageBlockBindingSignature( + const MG_State::GLState::ProgramObject& stateProgramObject); } // namespace PrgramImpl namespace SamplerImpl { diff --git a/MobileGL/MG_Backend/DirectGLES/Utils.cpp b/MobileGL/MG_Backend/DirectGLES/Utils.cpp index 42d10487..431086cc 100644 --- a/MobileGL/MG_Backend/DirectGLES/Utils.cpp +++ b/MobileGL/MG_Backend/DirectGLES/Utils.cpp @@ -21,6 +21,7 @@ #include #include +#include #include #include #include @@ -470,6 +471,352 @@ namespace MobileGL::MG_Backend::DirectGLES { return result; } + namespace { + Bool IsImagePassIdentifierChar(char c) { + return std::isalnum(static_cast(c)) || c == '_'; + } + + // Occurrences of `identifier` in `code` that are whole identifiers, i.e. not the + // tail or head of a longer one. "goku" must not find "goku_hd" or "my_goku". + SizeT CountIdentifierOccurrences(const String& code, const String& identifier) { + if (identifier.empty()) return 0; + SizeT count = 0; + for (SizeT pos = code.find(identifier); pos != String::npos; + pos = code.find(identifier, pos + 1)) { + if (pos > 0 && IsImagePassIdentifierChar(code[pos - 1])) continue; + const SizeT after = pos + identifier.size(); + if (after < code.size() && IsImagePassIdentifierChar(code[after])) continue; + ++count; + } + return count; + } + + Bool ContainsIdentifier(const String& code, const String& identifier) { + return CountIdentifierOccurrences(code, identifier) > 0; + } + + // The image format layout qualifiers ESSL accepts (GLSL ES 3.20 4.4.7 table 4.6 - + // the ES-legal subset of what SPIRV-Cross's format_to_glsl can print). The + // readonly/writeonly rule only applies to a declaration that carries one of them. + Bool IsImageFormatQualifier(const String& token) { + static constexpr StringView FORMATS[] = { + "rgba32f", "rgba16f", "rg32f", "rg16f", "r11f_g11f_b10f", + "r32f", "r16f", "rgba16", "rgb10_a2", "rgba8", + "rg16", "rg8", "r16", "r8", "rgba16_snorm", + "rgba8_snorm", "rg16_snorm", "rg8_snorm", "r16_snorm", "r8_snorm", + "rgba32i", "rgba16i", "rgba8i", "rg32i", "rg16i", + "rg8i", "r32i", "r16i", "r8i", "rgba32ui", + "rgba16ui", "rgb10_a2ui", "rgba8ui", "rg32ui", "rg16ui", + "rg8ui", "r32ui", "r16ui", "r8ui", + }; + for (const StringView format : FORMATS) { + if (token == format) return true; + } + return false; + } + + // "Except for image variables qualified with the format qualifiers r32f, r32i, and + // r32ui, image variables must specify either memory qualifier readonly or the + // memory qualifier writeonly." (GLSL ES 3.20 4.10) + Bool IsMemoryQualifierExemptImageFormat(const String& token) { + return token == "r32f" || token == "r32i" || token == "r32ui"; + } + + // Comma-separated contents of a layout(...) list, each entry trimmed. + Vector SplitLayoutQualifierList(const String& layout) { + Vector tokens; + SizeT start = 0; + while (start <= layout.size()) { + SizeT comma = layout.find(',', start); + const Bool last = comma == String::npos; + String token = layout.substr(start, last ? String::npos : comma - start); + const SizeT first = token.find_first_not_of(" \t\r\n"); + if (first == String::npos) { + token.clear(); + } else { + token = token.substr(first, token.find_last_not_of(" \t\r\n") - first + 1); + } + if (!token.empty()) tokens.push_back(Move(token)); + if (last) break; + start = comma + 1; + } + return tokens; + } + + // Trims both ends and collapses every internal whitespace run to one space, so a + // qualifier list or array suffix can be spliced back into a rebuilt declaration + // whatever the original spacing was. + String NormalizeDeclarationSpacing(const String& text) { + String out; + out.reserve(text.size()); + Bool pendingSpace = false; + for (const char c : text) { + if (std::isspace(static_cast(c))) { + pendingSpace = !out.empty(); + continue; + } + if (pendingSpace) out += ' '; + pendingSpace = false; + out += c; + } + return out; + } + + // How an image builtin touches the image it is handed. + enum class ImageBuiltinAccess { None, Load, Store, Unknown }; + + ImageBuiltinAccess ClassifyImageBuiltin(const String& name) { + if (name == "imageStore") return ImageBuiltinAccess::Store; + if (name == "imageLoad") return ImageBuiltinAccess::Load; + // imageAtomic* both reads and writes, but ES only defines the atomics on + // r32i/r32ui/r32f images - exactly the formats the rule above exempts - so this + // pass has already skipped any declaration they can legally appear on. Load is + // enough to keep the classification total without ever being acted upon. + if (name.compare(0, 11, "imageAtomic") == 0) return ImageBuiltinAccess::Load; + if (name == "imageSize" || name == "imageSamples") return ImageBuiltinAccess::None; + // Some other identifier that starts with "image" and is being called: not a + // shape this pass can reason about, so it poisons the declaration instead of + // being guessed at. + return ImageBuiltinAccess::Unknown; + } + + struct ImageUniformDecl { + String name; + String writeName; // the writeonly half's name, when split + String layout; // raw contents of layout(...) + String qualifiers; // memory/precision qualifiers, normalized, no trailing space + String type; // image2D, uimage2DArray, ... + String arraySuffix; // "" or "[7]" + SizeT declStart = 0; + SizeT declLength = 0; + SizeT referenceCount = 0; // uses this pass recognized and accounted for + Bool loaded = false; + Bool stored = false; + Bool unknownUse = false; + Bool split = false; + }; + + // A rebuilt declaration. Keeps SPIRV-Cross's own word order (`uniform readonly + // highp image2D`) so the image-rebinding regex in Managers.cpp still matches what + // comes out of here, whichever order the two passes end up running in. + String BuildImageDeclaration(const ImageUniformDecl& decl, const char* memoryQualifier, + const String& variableName) { + String out = "layout(" + decl.layout + ") uniform "; + out += memoryQualifier; + out += ' '; + if (!decl.qualifiers.empty()) { + out += decl.qualifiers; + out += ' '; + } + out += decl.type; + out += ' '; + out += variableName; + out += decl.arraySuffix; + out += ';'; + return out; + } + + // A name for the writeonly half that no identifier in the shader (and no other + // half already minted) can collide with. + String MakeImageWriteAliasName(const String& name, const String& source, + const Vector& taken) { + String candidate = String(IMAGE_WRITE_ALIAS_PREFIX) + name; + // "__" anywhere in an identifier is reserved (GLSL ES 3.20 3.7), which a name + // that already starts with '_' would otherwise produce. + for (SizeT doubled = candidate.find("__"); doubled != String::npos; + doubled = candidate.find("__", doubled)) { + candidate.erase(doubled, 1); + } + auto isTaken = [&](const String& identifier) { + if (ContainsIdentifier(source, identifier)) return true; + for (const auto& other : taken) { + if (other == identifier) return true; + } + return false; + }; + while (isTaken(candidate)) candidate += 'X'; + return candidate; + } + + struct ImageSourceEdit { + SizeT start; + SizeT length; + String text; + }; + } // namespace + + String SplitReadWriteImageUniforms(const String& glslCode) { +#ifdef TRACY_ENABLE + ZoneScopedC(TRACY_ZONECOLOR_BACKEND); +#endif + if (glslCode.find("image") == String::npos) { + return glslCode; + } + + // layout(...) uniform [array]; + // The qualifier alternation is order-free even though SPIRV-Cross emits a fixed + // order (to_qualifiers_glsl: storage, then coherent/restrict/readonly/writeonly, + // then precision), and the array group is repeated so a hypothetical multi- + // dimensional image array survives the round trip intact. + static const std::regex imageDeclRegex( + R"(layout\s*\(([^)]*)\)\s*uniform\s+)" + R"(((?:(?:readonly|writeonly|coherent|volatile|restrict|highp|mediump|lowp)\s+)*))" + R"(([iu]?image[A-Za-z0-9_]*)\s+([A-Za-z_][A-Za-z0-9_]*)\s*((?:\[[^\]]*\]\s*)*);)"); + + Vector decls; + for (std::sregex_iterator it(glslCode.begin(), glslCode.end(), imageDeclRegex), last; it != last; ++it) { + const std::smatch& match = *it; + const String qualifiers = match[2].str(); + // Already legal: SPIRV-Cross decided one way, leave it alone. + if (ContainsIdentifier(qualifiers, "readonly") || ContainsIdentifier(qualifiers, "writeonly")) { + continue; + } + + Bool hasFormat = false; + Bool exemptFormat = false; + for (const String& token : SplitLayoutQualifierList(match[1].str())) { + if (!IsImageFormatQualifier(token)) continue; + hasFormat = true; + exemptFormat = IsMemoryQualifierExemptImageFormat(token); + } + // No format qualifier at all is a different (and, in ES, unconditionally + // illegal) shape that GL_EXT_shader_image_load_formatted would be needed for; + // SPIRV-Cross refuses to emit it for an ES target, so nothing to do here. + if (!hasFormat || exemptFormat) continue; + + ImageUniformDecl decl; + decl.layout = match[1].str(); + decl.qualifiers = NormalizeDeclarationSpacing(qualifiers); + decl.type = match[3].str(); + decl.name = match[4].str(); + decl.arraySuffix = NormalizeDeclarationSpacing(match[5].str()); + decl.declStart = static_cast(match.position(0)); + decl.declLength = match[0].str().size(); + decls.push_back(Move(decl)); + } + if (decls.empty()) { + return glslCode; + } + + auto findDecl = [&decls](const String& name) -> SizeT { + for (SizeT i = 0; i < decls.size(); ++i) { + if (decls[i].name == name) return i; + } + return decls.size(); + }; + + // Walk every `image*(` call and attribute its first argument to a declaration. + struct StoreSite { + SizeT declIndex; + SizeT start; + SizeT length; + }; + Vector storeSites; + for (SizeT pos = glslCode.find("image"); pos != String::npos; pos = glslCode.find("image", pos + 1)) { + if (pos > 0 && IsImagePassIdentifierChar(glslCode[pos - 1])) continue; // uimage2D, myimageFoo + SizeT tokenEnd = pos; + while (tokenEnd < glslCode.size() && IsImagePassIdentifierChar(glslCode[tokenEnd])) ++tokenEnd; + const String builtin = glslCode.substr(pos, tokenEnd - pos); + + const SizeT openParen = glslCode.find_first_not_of(" \t\r\n", tokenEnd); + if (openParen == String::npos || glslCode[openParen] != '(') continue; // a type, not a call + + const SizeT argStart = glslCode.find_first_not_of(" \t\r\n", openParen + 1); + if (argStart == String::npos) continue; + if (!std::isalpha(static_cast(glslCode[argStart])) && glslCode[argStart] != '_') { + continue; // an expression, not a bare variable - it names no image of ours + } + SizeT argEnd = argStart; + while (argEnd < glslCode.size() && IsImagePassIdentifierChar(glslCode[argEnd])) ++argEnd; + + const SizeT declIndex = findDecl(glslCode.substr(argStart, argEnd - argStart)); + if (declIndex == decls.size()) continue; + ImageUniformDecl& decl = decls[declIndex]; + ++decl.referenceCount; + + // The operand has to be the bare variable, optionally subscripted. Anything + // else (a member access, a call result) is a shape this pass cannot rewrite. + SizeT after = glslCode.find_first_not_of(" \t\r\n", argEnd); + if (after != String::npos && glslCode[after] == '[') { + Int depth = 0; + SizeT scan = after; + for (; scan < glslCode.size(); ++scan) { + if (glslCode[scan] == '[') ++depth; + else if (glslCode[scan] == ']' && --depth == 0) break; + } + after = scan >= glslCode.size() ? String::npos + : glslCode.find_first_not_of(" \t\r\n", scan + 1); + } + const char nextChar = after == String::npos ? '\0' : glslCode[after]; + if (nextChar != ',' && nextChar != ')') { + decl.unknownUse = true; + continue; + } + + switch (ClassifyImageBuiltin(builtin)) { + case ImageBuiltinAccess::Load: + decl.loaded = true; + break; + case ImageBuiltinAccess::Store: + decl.stored = true; + storeSites.push_back({declIndex, argStart, argEnd - argStart}); + break; + case ImageBuiltinAccess::None: + break; + default: + decl.unknownUse = true; + break; + } + } + + // Every mention of the name has to be one this pass saw, or the split would leave + // a store pointing at the readonly half. One occurrence is the declaration itself. + for (auto& decl : decls) { + if (CountIdentifierOccurrences(glslCode, decl.name) != decl.referenceCount + 1) { + decl.unknownUse = true; + } + } + + Vector edits; + Vector takenAliases; + for (auto& decl : decls) { + if (decl.unknownUse) continue; // leave it exactly as it was; no guessing + if (decl.loaded && decl.stored) { + decl.writeName = MakeImageWriteAliasName(decl.name, glslCode, takenAliases); + takenAliases.push_back(decl.writeName); + decl.split = true; + edits.push_back({decl.declStart, decl.declLength, + BuildImageDeclaration(decl, "readonly", decl.name) + "\n" + + BuildImageDeclaration(decl, "writeonly", decl.writeName)}); + } else if (decl.stored) { + edits.push_back({decl.declStart, decl.declLength, + BuildImageDeclaration(decl, "writeonly", decl.name)}); + } else { + // Loaded only, or only ever handed to imageSize (or unused): readonly is + // the qualifier that keeps every one of those legal. + edits.push_back({decl.declStart, decl.declLength, + BuildImageDeclaration(decl, "readonly", decl.name)}); + } + } + for (const StoreSite& site : storeSites) { + const ImageUniformDecl& decl = decls[site.declIndex]; + if (!decl.split) continue; + edits.push_back({site.start, site.length, decl.writeName}); + } + if (edits.empty()) { + return glslCode; + } + + // Back to front, so an earlier edit's offsets stay valid. + std::sort(edits.begin(), edits.end(), + [](const ImageSourceEdit& a, const ImageSourceEdit& b) { return a.start > b.start; }); + String result = glslCode; + for (const ImageSourceEdit& edit : edits) { + result.replace(edit.start, edit.length, edit.text); + } + return result; + } + namespace { // How a lookup carries its level of detail, and how many arguments it takes // before the optional bias. diff --git a/MobileGL/MG_Backend/DirectGLES/Utils.h b/MobileGL/MG_Backend/DirectGLES/Utils.h index 841636ba..2a7473be 100644 --- a/MobileGL/MG_Backend/DirectGLES/Utils.h +++ b/MobileGL/MG_Backend/DirectGLES/Utils.h @@ -131,6 +131,40 @@ namespace MobileGL::MG_Backend::DirectGLES { // enables several draw buffers, so the ordinary single-target shader is untouched. String BroadcastLegacyFragColor(String glslCode, GLenum shaderType, Uint drawBufferCount); String RemoveLayoutBinding(const String& glslCode); + // Prefix of the writeonly half a read+write image uniform is split into (see + // SplitReadWriteImageUniforms); the suffix is the image's own name. + constexpr const char* IMAGE_WRITE_ALIAS_PREFIX = "mg_imageWrite_"; + // ESSL refuses an image variable that carries a format qualifier other than r32f / + // r32i / r32ui unless it also carries `readonly` or `writeonly` (GLSL ES 3.10 4.9 / + // 3.20 4.10; glslang enforces it verbatim in ParseHelper.cpp's layoutObjectCheck). + // SPIRV-Cross emits NEITHER for an image the shader both reads and writes: it + // speculatively decorates every storage image NonWritable+NonReadable + // (fixup_image_load_store_access), then OpImageRead clears NonReadable and + // OpImageWrite clears NonWritable, and to_qualifiers_glsl only prints `readonly` + // from NonWritable and `writeonly` from NonReadable. Desktop GLSL is happy with the + // bare declaration, so the frontend raises no error and the illegal ESSL only shows + // up as a device compile failure - and then as a silently no-op draw. + // + // Restores a legal declaration: + // * loaded only -> add `readonly` + // * stored only -> add `writeonly` + // * both -> emit TWO declarations on the same binding and of the + // same type, `readonly ` and `writeonly + // `, and point every + // imageStore at the second one. Several image variables + // may share an image unit as long as they have the same + // type and format, which is exactly what the pair is. + // + // Budget note: the split DOUBLES the image-uniform count of the stage it fires in, so + // a driver advertising a tight GL_MAX_{FRAGMENT,VERTEX,...}_IMAGE_UNIFORMS can turn a + // shader that used to compile into a link failure. ES only guarantees 4 fragment image + // uniforms, so a shader with more than half the limit in read+write images is the case + // to watch. + // + // Runs on the transpiled ESSL, so it must see the bindings the frontend units were + // already rewritten to and must run before those bindings are stripped - see the call + // site in Managers.cpp. + String SplitReadWriteImageUniforms(const String& glslCode); // Prefix of the per-sampler float uniform that carries GL_TEXTURE_LOD_BIAS into // the shader (see EmulateTextureLodBias); the suffix is the sampler's own name. constexpr const char* LOD_BIAS_UNIFORM_PREFIX = "mg_lodBias_"; diff --git a/MobileGL/MG_IntegrationTest/CMakeLists.txt b/MobileGL/MG_IntegrationTest/CMakeLists.txt index 2359f348..48963737 100644 --- a/MobileGL/MG_IntegrationTest/CMakeLists.txt +++ b/MobileGL/MG_IntegrationTest/CMakeLists.txt @@ -63,6 +63,7 @@ add_executable(MobileGLIntegrationTest Scenarios/UniformInitializerScenario.cpp Scenarios/SwizzleAccessRoutineScenario.cpp Scenarios/ProgramPipelineScenario.cpp + Scenarios/Glsl420DeclarationScenario.cpp ) target_include_directories(MobileGLIntegrationTest PRIVATE diff --git a/MobileGL/MG_IntegrationTest/Scenarios/Glsl420DeclarationScenario.cpp b/MobileGL/MG_IntegrationTest/Scenarios/Glsl420DeclarationScenario.cpp new file mode 100644 index 00000000..3412396e --- /dev/null +++ b/MobileGL/MG_IntegrationTest/Scenarios/Glsl420DeclarationScenario.cpp @@ -0,0 +1,400 @@ +// MobileGL - MobileGL/MG_IntegrationTest/Scenarios/Glsl420DeclarationScenario.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 +// +// Scenario - GLSL 4.20 DECLARATIONS THE FRONTEND USED TO REJECT OR COLLAPSE. +// +// GLSL 4.20 gives an array of opaque uniforms or of block instances CONSECUTIVE binding +// points: "layout(binding = 1) uniform sampler2D goku[7]" puts goku[0] on texture unit 1 +// and goku[6] on unit 7, and the same rule holds for "layout(binding = 2) uniform GOKU +// {...} goku[14]" over uniform buffer binding points 2..15 (GLSL 4.20 4.4.5, GL 4.6 7.6.2). +// One qualifier, N bindings - which is exactly the part that is easy to get wrong, because +// every element shares one declaration and one reflection record. +// +// Three separate mechanisms all collapsed that array down to its first element, and the +// three cases below pin one each: +// +// * the SAMPLER array (Espryt): reflection names an array after its first element at +// every location it spans, so the backend resolved "goku[0]" once per element, got one +// backend location N times, and the per-draw pass's last glUniform1i was the only one +// that survived. goku[0] ended up holding the LAST element's unit and goku[1..N-1] kept +// unit 0 - so every element sampled whatever was bound to unit 0. +// * the uniform BLOCK array (both backends): glslang reports the declared binding for +// every expanded instance, so nothing added the element offset. glGetActiveUniformBlockiv +// answered the base binding for all of them, and since both backends feed a block from +// that same number at draw time, all instances also read one buffer. +// * 'invariant' on a non-vertex stage's INPUT: legal desktop GLSL at every version, and +// ignored where it is written, but glslang rejected it from 4.20 up - so a shader that +// compiled as "#version 400" stopped compiling as "#version 420". +// +// The fourth case is the same species as the third - a legal 4.20 shader the frontend +// refused - and lives here for that reason: atomicCounterIncrement() was rejected because +// glslang applied its atomicAdd() extension gate to the atomicAdd() its own Vulkan-relaxed +// lowering had just synthesized. +// +// Conformance cases behind these: KHR-GL42.shading_language_420pack.binding_sampler_array, +// .binding_uniform_block_array, .qualifier_order[_block]_test_id_*, and +// KHR-GL42.shader_image_load_store.advanced-sso-atomicCounters. + +#include +#include +#include + +#include "../Harness/HeadlessGL.h" +#include "../Harness/ScenarioFixture.h" + +#ifdef GLAPI +#undef GLAPI +#endif +#define GL_GLEXT_PROTOTYPES +#include +#include +#undef GL_GLEXT_PROTOTYPES + +namespace MGITest { + namespace { + + constexpr int kElements = 4; + + // No vertex attributes: the quad comes from gl_VertexID, so nothing here depends on + // the harness's attribute pinning and the fragment stage is the only thing under test. + constexpr const char* kQuadVS = R"(#version 420 core +void main() +{ + switch (gl_VertexID) + { + case 0: gl_Position = vec4(-1.0, -1.0, 0.0, 1.0); break; + case 1: gl_Position = vec4( 1.0, -1.0, 0.0, 1.0); break; + case 2: gl_Position = vec4(-1.0, 1.0, 0.0, 1.0); break; + default: gl_Position = vec4( 1.0, 1.0, 0.0, 1.0); break; + } +} +)"; + + // The red channel comes back as a BITMASK of which elements read the wrong thing, so + // a failure names the element instead of just saying "not green". float(bad)/255.0 + // round-trips exactly through an RGBA8 target for every mask this can produce. + constexpr const char* kSamplerArrayFS = R"(#version 420 core +layout(binding = 1) uniform sampler2D goku[4]; +out vec4 o_color; +void main() +{ + const vec2 uv = vec2(0.5, 0.5); + int bad = 0; + if (texture(goku[0], uv) != vec4(1.0, 0.0, 0.0, 1.0)) bad |= 1; + if (texture(goku[1], uv) != vec4(0.0, 0.0, 1.0, 1.0)) bad |= 2; + if (texture(goku[2], uv) != vec4(1.0, 1.0, 0.0, 1.0)) bad |= 4; + if (texture(goku[3], uv) != vec4(0.0, 1.0, 1.0, 1.0)) bad |= 8; + o_color = vec4(float(bad) / 255.0, bad == 0 ? 1.0 : 0.0, 0.0, 1.0); +} +)"; + + constexpr const char* kBlockArrayFS = R"(#version 420 core +layout(std140, binding = 2) uniform GOKU +{ + vec4 gohan; +} goku[4]; +out vec4 o_color; +void main() +{ + int bad = 0; + if (goku[0].gohan != vec4(1.0, 0.0, 0.0, 1.0)) bad |= 1; + if (goku[1].gohan != vec4(0.0, 0.0, 1.0, 1.0)) bad |= 2; + if (goku[2].gohan != vec4(1.0, 1.0, 0.0, 1.0)) bad |= 4; + if (goku[3].gohan != vec4(0.0, 1.0, 1.0, 1.0)) bad |= 8; + o_color = vec4(float(bad) / 255.0, bad == 0 ? 1.0 : 0.0, 0.0, 1.0); +} +)"; + + // The producing stage declares the varying invariant (always legal) and the consuming + // stage redeclares it (the part that regressed at 4.20). The qualifier ORDER is the + // shuffled one 420pack exists to allow, so this also covers the parse path the + // qualifier_order cases exercise. + constexpr const char* kInvariantInVS = R"(#version 420 core +smooth invariant out highp vec4 v_data; +void main() +{ + v_data = vec4(0.0, 1.0, 0.0, 1.0); + switch (gl_VertexID) + { + case 0: gl_Position = vec4(-1.0, -1.0, 0.0, 1.0); break; + case 1: gl_Position = vec4( 1.0, -1.0, 0.0, 1.0); break; + case 2: gl_Position = vec4(-1.0, 1.0, 0.0, 1.0); break; + default: gl_Position = vec4( 1.0, 1.0, 0.0, 1.0); break; + } +} +)"; + + constexpr const char* kInvariantInFS = R"(#version 420 core +highp in smooth invariant vec4 v_data; +out vec4 o_color; +void main() { o_color = v_data; } +)"; + + // atomicCounterIncrement() is core GLSL from 4.20 and needs no extension. MobileGL + // parses under Vulkan-relaxed rules, which rewrite it into an atomicAdd() on a buffer + // block - and glslang then applied to its OWN rewrite the desktop-below-430 gate that + // demands GL_ARB_shader_storage_buffer_object for atomicAdd, rejecting a shader it had + // just accepted. The shape is lifted from + // KHR-GL42.shader_image_load_store.advanced-sso-atomicCounters. + constexpr const char* kAtomicCounterVS = R"(#version 420 core +layout(binding = 0, offset = 0) uniform atomic_uint g_counter; +out flat uint v_index; +void main() +{ + v_index = atomicCounterIncrement(g_counter); + switch (gl_VertexID) + { + case 0: gl_Position = vec4(-1.0, -1.0, 0.0, 1.0); break; + case 1: gl_Position = vec4( 1.0, -1.0, 0.0, 1.0); break; + case 2: gl_Position = vec4(-1.0, 1.0, 0.0, 1.0); break; + default: gl_Position = vec4( 1.0, 1.0, 0.0, 1.0); break; + } +} +)"; + + constexpr const char* kAtomicCounterFS = R"(#version 420 core +in flat uint v_index; +out vec4 o_color; +void main() { o_color = vec4(0.0, 1.0, 0.0, 1.0); } +)"; + + class Glsl420DeclarationScenario : public ScenarioTest { + protected: + void TearDown() override { + if (!Ready()) return; + glUseProgram(0); + if (!m_textures.empty()) glDeleteTextures(static_cast(m_textures.size()), m_textures.data()); + if (!m_buffers.empty()) glDeleteBuffers(static_cast(m_buffers.size()), m_buffers.data()); + for (GLuint p : m_programs) glDeleteProgram(p); + if (m_vao != 0) glDeleteVertexArrays(1, &m_vao); + m_textures.clear(); + m_buffers.clear(); + m_programs.clear(); + m_vao = 0; + } + + GLuint Build(const char* vs, const char* fs) { + std::string error; + const GLuint program = CompileProgram(vs, fs, &error); + if (program == 0) { + ADD_FAILURE() << "program did not build: " << error; + return 0; + } + m_programs.push_back(program); + return program; + } + + // One 1x1 RGBA8 texture per element, each a colour whose channels are exactly 0 or + // 255 so the shader's == comparisons are exact. + void MakeElementTextures(const std::uint8_t colors[kElements][4]) { + m_textures.assign(kElements, 0); + glGenTextures(kElements, m_textures.data()); + for (int i = 0; i < kElements; ++i) { + glActiveTexture(GL_TEXTURE0 + 1 + i); + glBindTexture(GL_TEXTURE_2D, m_textures[i]); + glTexImage2D(GL_TEXTURE_2D, 0, GL_RGBA8, 1, 1, 0, GL_RGBA, GL_UNSIGNED_BYTE, colors[i]); + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_NEAREST); + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_NEAREST); + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAX_LEVEL, 0); + } + glActiveTexture(GL_TEXTURE0); + } + + void MakeElementBuffers(const float values[kElements][4], GLuint firstBinding) { + m_buffers.assign(kElements, 0); + glGenBuffers(kElements, m_buffers.data()); + for (int i = 0; i < kElements; ++i) { + glBindBuffer(GL_UNIFORM_BUFFER, m_buffers[i]); + glBufferData(GL_UNIFORM_BUFFER, 4 * sizeof(float), values[i], GL_STATIC_DRAW); + glBindBufferBase(GL_UNIFORM_BUFFER, firstBinding + i, m_buffers[i]); + } + glBindBuffer(GL_UNIFORM_BUFFER, 0); + } + + // Draws the full-screen quad and hands back the centre pixel. + Rgba8 DrawAndRead(GLuint program) { + HeadlessGL& gl = Gl(); + if (m_vao == 0) glGenVertexArrays(1, &m_vao); + glBindVertexArray(m_vao); + BindDefaultFramebuffer(); + glViewport(0, 0, gl.Width(), gl.Height()); + glDisable(GL_SCISSOR_TEST); + glDisable(GL_DEPTH_TEST); + ClearTo(0.0f, 0.0f, 0.0f, 1.0f); + glUseProgram(program); + glDrawArrays(GL_TRIANGLE_STRIP, 0, 4); + const Image image = ReadPixels(gl.Width(), gl.Height()); + glUseProgram(0); + return image.At(gl.Width() / 2, gl.Height() / 2); + } + + // Magma turns a sampler array into ONE descriptor with descriptorCount = N, and + // ProgramFactory::ReflectLayout refuses any descriptor array that is not a + // dynamic UBO (MG_Backend/DirectVulkan/Renderer/ProgramFactory.cpp - "descriptor + // arrays are unsupported for this descriptor kind"), so program creation fails + // and the draw samples descriptors that were never written. On a hardware driver + // that reads back as wrong pixels; under lavapipe it is a segfault in the + // rasterizer thread. Supporting it means carrying an element dimension through + // UniformManager's per-binding location tables, which is a feature, not a fix - + // so this case is SCOPED rather than disabled, because the frontend half it also + // covers (the seeded units) is real on both backends and is asserted below + // before the draw. + bool SamplerArrayDescriptorsAreSupported() const { return Gl().BackendName() != "DirectVulkan"; } + + // Same shape, different gap: with the compile fixed, this shader now links on + // both backends but paints nothing on Magma - the atomic counter becomes a + // buffer descriptor there and that half is not wired up yet (the conformance + // case KHR-GL42.shader_image_load_store.advanced-sso-atomicCounters is where it + // is measured). The regression this case exists for is the COMPILE, which is + // asserted on both backends above; only the paint is scoped. + bool AtomicCounterDrawsAreSupported() const { return Gl().BackendName() != "DirectVulkan"; } + + static std::string BadElements(std::uint8_t mask) { + if (mask == 0) return "none"; + std::string out; + for (int i = 0; i < kElements; ++i) { + if ((mask & (1u << i)) == 0) continue; + if (!out.empty()) out += ", "; + out += "[" + std::to_string(i) + "]"; + } + return out; + } + + std::vector m_textures; + std::vector m_buffers; + std::vector m_programs; + GLuint m_vao = 0; + }; + + } // namespace + + // Element k of a sampler array samples texture unit N+k - both as the API reports it and, + // the part that was actually broken, as the draw behaves. + TEST_F(Glsl420DeclarationScenario, SamplerArrayElementsSampleConsecutiveTextureUnits) { + if (!Ready()) return; + + static const std::uint8_t colors[kElements][4] = { + {255, 0, 0, 255}, {0, 0, 255, 255}, {255, 255, 0, 255}, {0, 255, 255, 255}}; + MakeElementTextures(colors); + + const GLuint program = Build(kQuadVS, kSamplerArrayFS); + if (program == 0) return; + + // The reported unit is the shadow the frontend seeds from the qualifier. It was + // already right when the draw was wrong, so checking only this would have passed + // straight through the bug - it is here to separate a reflection regression from a + // backend one if this case ever fails again. + glUseProgram(program); + for (int i = 0; i < kElements; ++i) { + const std::string name = "goku[" + std::to_string(i) + "]"; + const GLint location = glGetUniformLocation(program, name.c_str()); + ASSERT_GE(location, 0) << name << " has no location"; + GLint unit = -1; + glGetUniformiv(program, location, &unit); + EXPECT_EQ(unit, 1 + i) << name << " should default to texture unit " << (1 + i); + } + glUseProgram(0); + + if (!SamplerArrayDescriptorsAreSupported()) { + GTEST_SKIP() << "sampler descriptor arrays are unimplemented on " << Gl().BackendName() + << "; the seeded units above are the half of this case it can answer"; + } + + const Rgba8 centre = DrawAndRead(program); + EXPECT_EQ(FirstGLError(), 0u); + EXPECT_EQ(centre.r, 0) << "sampler array elements that read the wrong texture: " << BadElements(centre.r); + EXPECT_EQ(centre.g, 255) << "the draw did not reach the fragment stage at all"; + } + + // Instance k of a uniform block array sits on buffer binding point N+k - again both as + // reported and as fed to the shader. + TEST_F(Glsl420DeclarationScenario, UniformBlockArrayInstancesTakeConsecutiveBindings) { + if (!Ready()) return; + + static const float values[kElements][4] = { + {1.0f, 0.0f, 0.0f, 1.0f}, {0.0f, 0.0f, 1.0f, 1.0f}, {1.0f, 1.0f, 0.0f, 1.0f}, {0.0f, 1.0f, 1.0f, 1.0f}}; + constexpr GLuint kFirstBinding = 2; + MakeElementBuffers(values, kFirstBinding); + + const GLuint program = Build(kQuadVS, kBlockArrayFS); + if (program == 0) return; + + for (int i = 0; i < kElements; ++i) { + const std::string name = "GOKU[" + std::to_string(i) + "]"; + const GLuint index = glGetUniformBlockIndex(program, name.c_str()); + ASSERT_NE(index, static_cast(GL_INVALID_INDEX)) << name << " is not an active block"; + GLint binding = -1; + glGetActiveUniformBlockiv(program, index, GL_UNIFORM_BLOCK_BINDING, &binding); + EXPECT_EQ(binding, static_cast(kFirstBinding) + i) + << name << " should start on binding point " << (kFirstBinding + i); + } + EXPECT_EQ(FirstGLError(), 0u) << "the block queries left a GL error behind"; + + const Rgba8 centre = DrawAndRead(program); + EXPECT_EQ(FirstGLError(), 0u); + EXPECT_EQ(centre.r, 0) << "block array instances that read the wrong buffer: " << BadElements(centre.r); + EXPECT_EQ(centre.g, 255) << "the draw did not reach the fragment stage at all"; + } + + // 'invariant' written on a fragment input at #version 420. The same source compiles at + // #version 400 on any implementation, so a version-dependent rejection is the defect. + TEST_F(Glsl420DeclarationScenario, InvariantIsAcceptedOnANonVertexStageInput) { + if (!Ready()) return; + + const GLuint program = Build(kInvariantInVS, kInvariantInFS); + if (program == 0) return; + + const Rgba8 centre = DrawAndRead(program); + EXPECT_EQ(FirstGLError(), 0u); + EXPECT_EQ(centre.g, 255) << "the invariant-qualified varying did not arrive"; + EXPECT_EQ(centre.r, 0); + } + + // A #version 420 shader may call atomicCounterIncrement() with no extension at all. The + // assertion is deliberately the COMPILE, because the defect was a compile-time gate on + // glslang's own atomic-counter lowering; the draw that follows only checks the shader + // survives the rest of the pipeline without leaving an error behind. + TEST_F(Glsl420DeclarationScenario, AnAtomicCounterCompilesWithoutTheSsboExtension) { + if (!Ready()) return; + + const GLuint shader = glCreateShader(GL_VERTEX_SHADER); + glShaderSource(shader, 1, &kAtomicCounterVS, nullptr); + glCompileShader(shader); + GLint compiled = GL_FALSE; + glGetShaderiv(shader, GL_COMPILE_STATUS, &compiled); + if (compiled == GL_FALSE) { + char log[2048] = {}; + glGetShaderInfoLog(shader, sizeof(log) - 1, nullptr, log); + glDeleteShader(shader); + FAIL() << "atomicCounterIncrement() at #version 420 core did not compile: " << log; + } + glDeleteShader(shader); + + const GLuint program = Build(kAtomicCounterVS, kAtomicCounterFS); + if (program == 0) return; + + GLuint counter = 0; + glGenBuffers(1, &counter); + m_buffers.push_back(counter); + const GLuint zero = 0; + glBindBuffer(GL_ATOMIC_COUNTER_BUFFER, counter); + glBufferData(GL_ATOMIC_COUNTER_BUFFER, sizeof(GLuint), &zero, GL_DYNAMIC_DRAW); + glBindBufferBase(GL_ATOMIC_COUNTER_BUFFER, 0, counter); + glBindBuffer(GL_ATOMIC_COUNTER_BUFFER, 0); + + if (!AtomicCounterDrawsAreSupported()) { + GTEST_SKIP() << "atomic-counter draws do not paint on " << Gl().BackendName() + << " yet; the compile above is what this case pins"; + } + + const Rgba8 centre = DrawAndRead(program); + EXPECT_EQ(FirstGLError(), 0u); + EXPECT_EQ(centre.g, 255) << "the atomic-counter shader linked but painted nothing"; + } + +} // namespace MGITest diff --git a/MobileGL/MG_IntegrationTest/Scenarios/ProgramPipelineScenario.cpp b/MobileGL/MG_IntegrationTest/Scenarios/ProgramPipelineScenario.cpp index 942d8626..a00e8af5 100644 --- a/MobileGL/MG_IntegrationTest/Scenarios/ProgramPipelineScenario.cpp +++ b/MobileGL/MG_IntegrationTest/Scenarios/ProgramPipelineScenario.cpp @@ -141,20 +141,6 @@ void main() return pipeline; } - // glShaderStorageBlockBinding is a GL 4.3 entry point with NO equivalent in ES: a - // storage block's binding there is fixed by its layout(binding=) qualifier at link - // and cannot be changed afterwards. So Espryt, which reaches the GPU through an ES - // driver, can only honour a rebinding by baking it into the ESSL it generates - - // and it does not yet (RemoveLayoutBinding in MG_Backend/DirectGLES/Utils.cpp - // deliberately PRESERVES the declared qualifier for `buffer` declarations, which - // is what the driver then goes by). Its best-effort API replay - // (ReseedShaderStorageBlockBindings) is a no-op wherever the driver lacks the - // entry point, which is every real ES driver. - // - // Scoped rather than disabled, because the defect is per-backend and the - // pipeline-side mechanism these cases exist for is fully exercised on Magma. - bool StorageBlockRebindingIsHonoured() const { return Gl().BackendName() == "DirectVulkan"; } - std::vector m_programs; std::vector m_pipelines; }; @@ -392,10 +378,6 @@ void main() { o_color = u_color; } if (vertexStorageBlocks < 2) { GTEST_SKIP() << "fewer than two vertex shader storage blocks available"; } - if (!StorageBlockRebindingIsHonoured()) { - GTEST_SKIP() << "backend cannot honour glShaderStorageBlockBinding at all; see the companion " - "AStorageBlockRebindingHoldsWithoutAPipeline case"; - } const GLuint vs = MakeSeparable(GL_VERTEX_SHADER, kStorageBlockVS); if (vs == 0) return; @@ -469,6 +451,12 @@ void main() { o_color = u_color; } // Two stages on purpose. Handing glUseProgram a vertex-ONLY program would confound the // experiment - a program with no fragment stage is a thing some backends cannot build at // all, so its failure would say nothing about block bindings. + // + // Runs on both backends. glShaderStorageBlockBinding is a GL 4.3 entry point with no ES + // equivalent - ES fixes a storage block's binding at link from its layout(binding=) + // qualifier - so Espryt honours a rebinding by writing the effective binding into the ESSL + // it generates (the Binding decoration is rewritten before SPIRV-Cross emits, and the draw + // path rebuilds a program whose override set has moved). TEST_F(ProgramPipelineScenario, AStorageBlockRebindingHoldsWithoutAPipeline) { if (!Ready()) return; HeadlessGL& gl = Gl(); @@ -478,10 +466,6 @@ void main() { o_color = u_color; } if (vertexStorageBlocks < 2) { GTEST_SKIP() << "fewer than two vertex shader storage blocks available"; } - if (!StorageBlockRebindingIsHonoured()) { - GTEST_SKIP() << "backend honours a storage-block rebinding only through the declared " - "layout(binding=) qualifier, which it does not yet rewrite"; - } static const char* kMonolithicVS = R"(#version 430 core layout(std430) buffer Output0 { uint value0; }; diff --git a/MobileGL/MG_State/GLState/ProgramState/ProgramLinkTask.cpp b/MobileGL/MG_State/GLState/ProgramState/ProgramLinkTask.cpp index d0cd328e..f90f79b6 100644 --- a/MobileGL/MG_State/GLState/ProgramState/ProgramLinkTask.cpp +++ b/MobileGL/MG_State/GLState/ProgramState/ProgramLinkTask.cpp @@ -41,6 +41,28 @@ namespace { return bracket == MobileGL::String::npos ? name : name.substr(0, bracket); } + // Element index of an arrayed interface-block instance: "GOKU[3]" -> 3, "GOKU" -> 0. + // Reflection spells arrayed instances exactly this way (glslang expands the instance + // array into one TObjectReflection per element), and the subscript it writes is a plain + // decimal, so a strict-decimal parse is both sufficient and the same rule GL 4.6 + // 7.3.1.1 puts on the name a program-resource query may use. + static MobileGL::Int BlockArrayElement(const MobileGL::String& name) { + if (name.empty() || name.back() != ']') return 0; + const MobileGL::SizeT bracket = name.rfind('['); + if (bracket == MobileGL::String::npos) return 0; + const MobileGL::SizeT first = bracket + 1; + const MobileGL::SizeT last = name.length() - 1; + if (first >= last) return 0; + if (name[first] == '0' && last - first > 1) return 0; // no leading zeros + MobileGL::Int element = 0; + for (MobileGL::SizeT i = first; i < last; ++i) { + if (name[i] < '0' || name[i] > '9') return 0; + element = element * 10 + static_cast(name[i] - '0'); + if (element > 0x0FFFFFFF) return 0; + } + return element; + } + static bool IsBuiltInPipelineOutput(const glslang::TObjectReflection& output) { const auto* type = output.getType(); return type && type->getQualifier().builtIn != glslang::EbvNone; @@ -97,39 +119,6 @@ namespace { return std::max(1, uniform.size); } - static bool ComputeShaderDeclaresLocalSize(const MobileGL::String& source) { - bool inLineComment = false; - bool inBlockComment = false; - for (MobileGL::SizeT i = 0; i < source.length(); ++i) { - if (inLineComment) { - inLineComment = source[i] != '\n'; - continue; - } - if (inBlockComment) { - if (source[i] == '*' && i + 1 < source.length() && source[i + 1] == '/') { - inBlockComment = false; - ++i; - } - continue; - } - if (source[i] == '/' && i + 1 < source.length()) { - if (source[i + 1] == '/') { - inLineComment = true; - ++i; - continue; - } - if (source[i + 1] == '*') { - inBlockComment = true; - ++i; - continue; - } - } - if (source.compare(i, 11, "local_size_") == 0) { - return true; - } - } - return false; - } } // namespace namespace MobileGL::MG_State::GLState { @@ -370,6 +359,31 @@ namespace MobileGL::MG_State::GLState { return; } + // A compute program must have a fixed local group size, and GL states that as a + // property of the PROGRAM: "at least one" of its compute shaders declares it (GL 4.6 + // core 7.13 / GLSL 4.30 4.4.1.4). MobileGL used to answer that question per SHADER, + // by scanning each source for the text "local_size_" - which rejected the perfectly + // legal shape KHR-GL42.compute_shader.build-monolithic submits, three compilation + // units of which only two carry the layout and the third holds nothing but a buffer + // block and a function. It also could not see a local size that arrived through a + // macro, and it happily accepted the substring inside an unrelated identifier. + // + // glslang already merged the units' modes at link (linkValidate.cpp mergeModes, which + // also diagnoses two units declaring CONTRADICTORY sizes), so the linked + // intermediate is the thing that knows - and asking it is both correct and free. + if (const glslang::TIntermediate* cs = artifacts.program->getIntermediate(EShLangCompute); + cs != nullptr && !cs->isLocalSizeSet()) { + artifacts.linkStatus = false; + // The gate this replaced ran before LinkProgram, so a program that failed it + // published no TProgram at all. Keep that invariant: everything downstream reads + // artifacts.program as "the linked program", and a rejected link should not leave + // one behind for a query surface to find. + artifacts.program.reset(); + artifacts.infoLog = "Compute shader is missing a local_size layout declaration."; + DeferLog(std::format("ProgramObject {}: Link failed - {}", in.externalIndex, artifacts.infoLog)); + return; + } + // GL_GEOMETRY_INPUT_TYPE. A draw's primitive type has to be compatible with it // (GL 4.6 core 11.3.1), so it is resolved for every link, not only a capturing one. artifacts.gsInputPrimitive = GL_NONE; @@ -510,13 +524,6 @@ namespace MobileGL::MG_State::GLState { in.externalIndex, i, artifacts.infoLog)); return false; } - if (input.stage == ShaderStage::Compute && - !ComputeShaderDeclaresLocalSize(input.source ? *input.source : String())) { - artifacts.infoLog = "Compute shader is missing a local_size layout declaration."; - DeferLog(std::format("ProgramObject {}: Link failed - {}", in.externalIndex, artifacts.infoLog)); - return false; - } - String reparseLog; outShaders[i] = input.compiled->ClaimParsedShader(reparseLog); if (!outShaders[i]) { @@ -921,8 +928,21 @@ namespace MobileGL::MG_State::GLState { std::max(artifacts.uniformBlockNameMaxLength, (Int)ubo.name.length()); artifacts.uniformBlockIndexByName[ubo.name] = i; // if there's binding defined in shader as layout(binding = ...), - // retrieve it here - artifacts.uniformBlockBinding[i] = ubo.getBinding(); + // retrieve it here. + // + // An instance array takes CONSECUTIVE binding points: "layout(binding = 2) + // uniform GOKU {...} goku[14];" puts goku[0] on 2 and goku[13] on 15 (GL 4.6 + // 7.6.2 / GLSL 4.20 4.4.5). glslang expands the array into one reflection + // record per element but hands every one of them the DECLARED binding, because + // they all share the block's TType - so the element offset has to be added + // here. Without it every element reported the base binding, and since both + // backends feed a block from GetUniformBlockBinding() at draw time + // (DirectGLES.cpp / UniformManager.cpp), all 14 elements also read the same + // buffer. This is the rule the storage-block path in ProgramInterface.cpp + // already applies, and whose comment there claims uniform blocks follow. + const Int declaredBinding = ubo.getBinding(); + artifacts.uniformBlockBinding[i] = + declaredBinding < 0 ? declaredBinding : declaredBinding + BlockArrayElement(ubo.name); MGLOG_D("ProgramObject %u: Reflection - UBO[%d] name='%s' size=%u binding=%d", in.externalIndex, i, ubo.name.c_str(), ubo.size, ubo.getBinding()); } diff --git a/MobileGL/MG_Test/Backend/DirectGLES/CMakeLists.txt b/MobileGL/MG_Test/Backend/DirectGLES/CMakeLists.txt new file mode 100644 index 00000000..c88c7534 --- /dev/null +++ b/MobileGL/MG_Test/Backend/DirectGLES/CMakeLists.txt @@ -0,0 +1,20 @@ +cmake_minimum_required(VERSION 3.14) + +add_executable( + EsslShaderPassTest + EsslShaderPassTest.cpp +) + +target_include_directories(EsslShaderPassTest PRIVATE + ${MGL_ROOT}/include + ${MGL_ROOT}/MobileGL +) + +target_link_libraries( + EsslShaderPassTest PRIVATE + GTest::gtest_main + ${LINK_LIBRARIES} +) + +include(GoogleTest) +gtest_discover_tests(EsslShaderPassTest DISCOVERY_TIMEOUT 30 PROPERTIES LABELS unit) diff --git a/MobileGL/MG_Test/Backend/DirectGLES/EsslShaderPassTest.cpp b/MobileGL/MG_Test/Backend/DirectGLES/EsslShaderPassTest.cpp new file mode 100644 index 00000000..f3e4d2f3 --- /dev/null +++ b/MobileGL/MG_Test/Backend/DirectGLES/EsslShaderPassTest.cpp @@ -0,0 +1,255 @@ +// MobileGL - MobileGL/MG_Test/Backend/DirectGLES/EsslShaderPassTest.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 +// +// The post-transpile textual passes the DirectGLES ("Espryt") backend runs over the ESSL +// SPIRV-Cross hands it (MG_Backend/DirectGLES/Utils.cpp). No GL context and no driver: the +// passes are pure String -> String, so the shapes they have to survive can be pinned here +// instead of only on a device. + +#include + +#include + +using namespace MobileGL; +using MobileGL::MG_Backend::DirectGLES::PrgramImpl::IMAGE_WRITE_ALIAS_PREFIX; +using MobileGL::MG_Backend::DirectGLES::PrgramImpl::RemoveLayoutBinding; +using MobileGL::MG_Backend::DirectGLES::PrgramImpl::SplitReadWriteImageUniforms; + +namespace { + Bool Contains(const String& haystack, const String& needle) { + return haystack.find(needle) != String::npos; + } + + SizeT CountOf(const String& haystack, const String& needle) { + SizeT count = 0; + for (SizeT pos = haystack.find(needle); pos != String::npos; pos = haystack.find(needle, pos + 1)) { + ++count; + } + return count; + } + + String WriteAlias(const String& name) { return String(IMAGE_WRITE_ALIAS_PREFIX) + name; } +} // namespace + +// The bug the pass exists for. SPIRV-Cross speculatively marks every storage image +// NonWritable+NonReadable, then clears NonReadable at the OpImageRead and NonWritable at the +// OpImageWrite, so an image the shader both reads and writes comes out carrying NEITHER +// `readonly` nor `writeonly` - which ESSL rejects for any format other than r32f/r32i/r32ui +// (GLSL ES 3.20 4.10). The device compile then fails and the draw silently binds program 0. +TEST(SplitReadWriteImageUniformsTest, ReadWriteImageIsSplitIntoAnAliasingPair) { + const String source = R"(#version 320 es +layout(binding = 2, rgba8) uniform highp image2D goku; +layout(location = 0) out highp vec4 mg_FragColor; +void main() +{ + highp vec4 loaded = imageLoad(goku, ivec2(gl_FragCoord.xy)); + imageStore(goku, ivec2(gl_FragCoord.xy), loaded + vec4(0.25)); + mg_FragColor = loaded; +} +)"; + const String out = SplitReadWriteImageUniforms(source); + + // Both halves: same binding, same format, same type - which is what makes two image + // variables on one image unit legal. + EXPECT_TRUE(Contains(out, "layout(binding = 2, rgba8) uniform readonly highp image2D goku;")); + EXPECT_TRUE(Contains(out, "layout(binding = 2, rgba8) uniform writeonly highp image2D " + WriteAlias("goku") + ";")); + + // The load keeps the original name, the store moves to the writeonly half. + EXPECT_TRUE(Contains(out, "imageLoad(goku,")); + EXPECT_TRUE(Contains(out, "imageStore(" + WriteAlias("goku") + ",")); + EXPECT_FALSE(Contains(out, "imageStore(goku,")); +} + +// The split has to survive RemoveLayoutBinding, which runs straight after it: an ES image +// unit cannot be assigned through the API, so the layout qualifier is the only binding +// mechanism and both halves must still carry theirs afterwards. +TEST(SplitReadWriteImageUniformsTest, BothHalvesKeepTheirBindingThroughRemoveLayoutBinding) { + const String source = R"(#version 320 es +layout(binding = 5, rgba8) uniform highp image2D goku; +void main() +{ + imageStore(goku, ivec2(0), imageLoad(goku, ivec2(0))); +} +)"; + const String out = RemoveLayoutBinding(SplitReadWriteImageUniforms(source)); + EXPECT_EQ(CountOf(out, "binding = 5"), 2u); +} + +// Cheap hardening: the pass does not depend on SPIRV-Cross getting the read-only case right, +// and a shader that only reads must not pay for a second uniform. +TEST(SplitReadWriteImageUniformsTest, ReadOnlyImageGetsReadonlyAndIsNotSplit) { + const String source = R"(#version 320 es +layout(binding = 1, rgba16f) uniform highp image2DArray trunks; +layout(location = 0) out highp vec4 mg_FragColor; +void main() +{ + mg_FragColor = imageLoad(trunks, ivec3(0)); +} +)"; + const String out = SplitReadWriteImageUniforms(source); + EXPECT_TRUE(Contains(out, "layout(binding = 1, rgba16f) uniform readonly highp image2DArray trunks;")); + EXPECT_FALSE(Contains(out, "writeonly")); + EXPECT_FALSE(Contains(out, IMAGE_WRITE_ALIAS_PREFIX)); + EXPECT_EQ(CountOf(out, "image2DArray"), 1u); +} + +TEST(SplitReadWriteImageUniformsTest, WriteOnlyImageGetsWriteonlyAndIsNotSplit) { + const String source = R"(#version 320 es +layout(binding = 3, rgba8) uniform highp image2D gohan; +void main() +{ + imageStore(gohan, ivec2(0), vec4(1.0)); +} +)"; + const String out = SplitReadWriteImageUniforms(source); + EXPECT_TRUE(Contains(out, "layout(binding = 3, rgba8) uniform writeonly highp image2D gohan;")); + EXPECT_FALSE(Contains(out, "readonly")); + EXPECT_FALSE(Contains(out, IMAGE_WRITE_ALIAS_PREFIX)); +} + +// r32f / r32i / r32ui are exactly the formats GLSL ES 3.20 4.10 exempts from the rule, so a +// read+write image in one of them is already legal and must not be doubled. +TEST(SplitReadWriteImageUniformsTest, ExemptFormatsAreLeftCompletelyAlone) { + for (const char* format : {"r32f", "r32i", "r32ui"}) { + const String type = String(format) == "r32f" ? "image2D" : (String(format) == "r32i" ? "iimage2D" : "uimage2D"); + const String source = "#version 320 es\nlayout(binding = 4, " + String(format) + ") uniform highp " + type + + " vegeta;\nvoid main()\n{\n imageStore(vegeta, ivec2(0), imageLoad(vegeta, " + "ivec2(0)));\n}\n"; + EXPECT_EQ(SplitReadWriteImageUniforms(source), source) << "format " << format; + } +} + +// A declaration SPIRV-Cross already qualified is none of this pass's business. +TEST(SplitReadWriteImageUniformsTest, AlreadyQualifiedDeclarationsAreUntouched) { + const String source = R"(#version 320 es +layout(binding = 0, rgba8) uniform readonly highp image2D reader; +layout(binding = 1, rgba8) uniform writeonly highp image2D writer; +void main() +{ + imageStore(writer, ivec2(0), imageLoad(reader, ivec2(0))); +} +)"; + EXPECT_EQ(SplitReadWriteImageUniforms(source), source); +} + +// The binding of an image array is the array's base; splitting must keep the array on both +// halves (dropping the subscript would silently turn 3 units into 1). +TEST(SplitReadWriteImageUniformsTest, ImageArraySplitsAndKeepsItsArraySize) { + const String source = R"(#version 320 es +layout(binding = 6, rgba8) uniform highp image2D gohan[3]; +void main() +{ + imageStore(gohan[1], ivec2(0), imageLoad(gohan[2], ivec2(0))); +} +)"; + const String out = SplitReadWriteImageUniforms(source); + EXPECT_TRUE(Contains(out, "layout(binding = 6, rgba8) uniform readonly highp image2D gohan[3];")); + EXPECT_TRUE(Contains(out, + "layout(binding = 6, rgba8) uniform writeonly highp image2D " + WriteAlias("gohan") + "[3];")); + EXPECT_TRUE(Contains(out, "imageStore(" + WriteAlias("gohan") + "[1],")); + EXPECT_TRUE(Contains(out, "imageLoad(gohan[2],")); +} + +// The rewrite is by identifier, not by substring: "goku" must not reach into "goku_hd", and +// the two images have to be classified independently. +TEST(SplitReadWriteImageUniformsTest, ANameThatIsAPrefixOfAnotherIsNotClobbered) { + const String source = R"(#version 320 es +layout(binding = 1, rgba8) uniform highp image2D goku; +layout(binding = 2, rgba8) uniform highp image2D goku_hd; +void main() +{ + highp vec4 loaded = imageLoad(goku, ivec2(0)); + imageStore(goku, ivec2(0), loaded); + imageStore(goku_hd, ivec2(0), loaded); +} +)"; + const String out = SplitReadWriteImageUniforms(source); + + // goku is read+write -> split; goku_hd is write-only -> qualified in place, not split. + EXPECT_TRUE(Contains(out, "layout(binding = 1, rgba8) uniform readonly highp image2D goku;")); + EXPECT_TRUE(Contains(out, "layout(binding = 1, rgba8) uniform writeonly highp image2D " + WriteAlias("goku") + ";")); + EXPECT_TRUE(Contains(out, "layout(binding = 2, rgba8) uniform writeonly highp image2D goku_hd;")); + EXPECT_TRUE(Contains(out, "imageStore(goku_hd,")); + EXPECT_FALSE(Contains(out, WriteAlias("goku") + "_hd")); + EXPECT_FALSE(Contains(out, WriteAlias("goku_hd"))); +} + +// Other qualifiers belong to both halves, and the memory qualifier goes where SPIRV-Cross +// puts it (right after `uniform`) so the image-rebinding regex in Managers.cpp still matches. +TEST(SplitReadWriteImageUniformsTest, ExistingQualifiersAreCarriedOntoBothHalves) { + const String source = R"(#version 320 es +layout(binding = 2, rgba8) uniform coherent restrict highp image2D goku; +void main() +{ + imageStore(goku, ivec2(0), imageLoad(goku, ivec2(0))); +} +)"; + const String out = SplitReadWriteImageUniforms(source); + EXPECT_TRUE(Contains(out, "uniform readonly coherent restrict highp image2D goku;")); + EXPECT_TRUE( + Contains(out, "uniform writeonly coherent restrict highp image2D " + WriteAlias("goku") + ";")); +} + +// imageSize reads no texels and writes none, so it decides nothing; readonly is what keeps +// such a declaration legal. +TEST(SplitReadWriteImageUniformsTest, ImageSizeAloneDoesNotCountAsALoadOrAStore) { + const String source = R"(#version 320 es +layout(binding = 8, rgba8ui) uniform highp uimage2D sizeOnly; +layout(location = 0) out highp vec4 mg_FragColor; +void main() +{ + mg_FragColor = vec4(float(imageSize(sizeOnly).x)); +} +)"; + const String out = SplitReadWriteImageUniforms(source); + EXPECT_TRUE(Contains(out, "layout(binding = 8, rgba8ui) uniform readonly highp uimage2D sizeOnly;")); + EXPECT_FALSE(Contains(out, IMAGE_WRITE_ALIAS_PREFIX)); +} + +// The alias must not land on an identifier the shader already uses. +TEST(SplitReadWriteImageUniformsTest, AliasNameAvoidsAnExistingIdentifier) { + const String source = R"(#version 320 es +layout(binding = 6, rgba8) uniform highp image2D taken; +highp vec4 mg_imageWrite_taken; +void main() +{ + imageStore(taken, ivec2(0), imageLoad(taken, ivec2(0)) + mg_imageWrite_taken); +} +)"; + const String out = SplitReadWriteImageUniforms(source); + EXPECT_FALSE(Contains(out, "image2D " + WriteAlias("taken") + ";")); + EXPECT_TRUE(Contains(out, "image2D " + WriteAlias("taken") + "X;")); + EXPECT_TRUE(Contains(out, "imageStore(" + WriteAlias("taken") + "X,")); + EXPECT_TRUE(Contains(out, "+ mg_imageWrite_taken)")); +} + +// A use the pass cannot account for (here: the image handed to a user function) means it +// cannot know every store site, so it declines rather than emitting a half-rewritten shader. +TEST(SplitReadWriteImageUniformsTest, AnUnrecognizedUseLeavesTheDeclarationAlone) { + const String source = R"(#version 320 es +layout(binding = 2, rgba8) uniform highp image2D passed; +highp vec4 helper(highp image2D img) { return imageLoad(img, ivec2(0)); } +void main() +{ + imageStore(passed, ivec2(0), helper(passed)); +} +)"; + EXPECT_EQ(SplitReadWriteImageUniforms(source), source); +} + +TEST(SplitReadWriteImageUniformsTest, ShaderWithoutImagesIsReturnedUnchanged) { + const String source = R"(#version 320 es +layout(binding = 0) uniform highp sampler2D goku; +layout(location = 0) out highp vec4 mg_FragColor; +void main() +{ + mg_FragColor = texture(goku, vec2(0.5)); +} +)"; + EXPECT_EQ(SplitReadWriteImageUniforms(source), source); +} diff --git a/MobileGL/MG_Test/CMakeLists.txt b/MobileGL/MG_Test/CMakeLists.txt index c0edcc40..7f513507 100644 --- a/MobileGL/MG_Test/CMakeLists.txt +++ b/MobileGL/MG_Test/CMakeLists.txt @@ -78,6 +78,9 @@ add_subdirectory(Query) add_subdirectory(Pipeline) add_subdirectory(ShaderTranspiler) add_subdirectory(Util) +# The DirectGLES post-transpile ESSL passes are pure String -> String, so unlike the +# DirectVulkan suite below this one needs no device and always builds. +add_subdirectory(Backend/DirectGLES) if (ENABLE_INTEGRATION_TESTS) add_subdirectory(Backend/DirectVulkan) endif() diff --git a/MobileGL/MG_Util/ShaderTranspiler/SpvcSession.cpp b/MobileGL/MG_Util/ShaderTranspiler/SpvcSession.cpp index 0abe8f20..41e43a0d 100644 --- a/MobileGL/MG_Util/ShaderTranspiler/SpvcSession.cpp +++ b/MobileGL/MG_Util/ShaderTranspiler/SpvcSession.cpp @@ -289,6 +289,43 @@ namespace MobileGL { SPVC_CHK_RETURN } + spvc_result SpvcSession::SetShaderStorageBlockBinding(const UnorderedMap& bindings) { + if (!(usage & SessionUsageBit::Transpile)) return SPVC_ERROR_INVALID_ARGUMENT; + + SPVC_CHK_INIT + const spvc_reflected_resource* list = nullptr; + size_t count = 0; + SPVC_CHK_RESULT(spvc_resources_get_resource_list_for_type( + resources, SPVC_RESOURCE_TYPE_STORAGE_BUFFER, &list, &count)); + for (size_t i = 0; i < count; ++i) { + auto& resource = list[i]; + // Two spellings, because neither one alone identifies the block the GL + // interface query named. `resource.name` is the block's instance name when + // the declaration has one; the block TYPE name (which is what the GL query + // reports for a block) lives on base_type_id. An arrayed block collapses to + // a single SPIR-V resource while GL enumerates it per element, so the bare + // name is also tried with element zero's subscript - the same convention + // ProgramObject::GetShaderStorageBlockBindingOverride documents. + const char* blockTypeName = spvc_compiler_get_name(compiler, resource.base_type_id); + const String candidates[] = { + blockTypeName != nullptr ? String(blockTypeName) : String(), + resource.name != nullptr ? String(resource.name) : String(), + }; + for (const auto& candidate : candidates) { + if (candidate.empty()) continue; + auto it = bindings.find(candidate); + if (it == bindings.end()) it = bindings.find(candidate + "[0]"); + if (it == bindings.end()) continue; + // Negative is "never rebound" - the declared qualifier still stands. + if (it->second < 0) break; + spvc_compiler_set_decoration(compiler, resource.id, SpvDecorationBinding, + static_cast(it->second)); + break; + } + } + SPVC_CHK_RETURN + } + spvc_result SpvcSession::Compile(const char** result) { if (!(usage & SessionUsageBit::Transpile)) return SPVC_ERROR_INVALID_ARGUMENT; SPVC_CHK_INIT diff --git a/MobileGL/MG_Util/ShaderTranspiler/SpvcSession.h b/MobileGL/MG_Util/ShaderTranspiler/SpvcSession.h index f5296b2e..d01434d2 100644 --- a/MobileGL/MG_Util/ShaderTranspiler/SpvcSession.h +++ b/MobileGL/MG_Util/ShaderTranspiler/SpvcSession.h @@ -94,6 +94,17 @@ namespace MobileGL { spvc_result SetOptions(spvc_compiler_options options); Vector GetShaderInterface(spvc_resource_type resource_type) const; spvc_result SetVertexAttribLocation(const UnorderedMap& location); + // Rewrites the Binding decoration of shader storage blocks before emission, so + // the generated source carries the EFFECTIVE binding rather than the declared + // one. This exists for the ESSL backend: glShaderStorageBlockBinding is a GL 4.3 + // entry point with no ES equivalent (ES fixes a storage block's binding at link + // from its layout(binding=) qualifier), so the only place a rebinding can be + // expressed there is the qualifier the transpiler prints. + // + // Keyed by the GL interface-query name of the BLOCK (the block/type name; an + // arrayed block's elements are separate GL resources spelled "B[0]", "B[1]"). + // Entries with a negative value mean "never rebound" and are skipped. + spvc_result SetShaderStorageBlockBinding(const UnorderedMap& bindings); spvc_result Compile(const char** result); const SpvcMetadata& GetMetadata() const; const char* GetLastErrorString() const; From 19fc7346c5e6393eee7ee1144994fe548762443b Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Tue, 11 Aug 2026 23:05:13 -0400 Subject: [PATCH 3/3] [Fix] (MG_Backend/DirectVulkan): a re-entrant cache lookup dangled the reference glShaderStorageBlockBinding then wrote through --- MobileGL/MG_Backend/DirectVulkan/DirectVulkan.cpp | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/MobileGL/MG_Backend/DirectVulkan/DirectVulkan.cpp b/MobileGL/MG_Backend/DirectVulkan/DirectVulkan.cpp index eb108838..046c8a9e 100644 --- a/MobileGL/MG_Backend/DirectVulkan/DirectVulkan.cpp +++ b/MobileGL/MG_Backend/DirectVulkan/DirectVulkan.cpp @@ -801,9 +801,18 @@ namespace MobileGL::MG_Backend::DirectVulkan { // already recorded the new binding on the program - which is what reseeds this cache // whenever it is rebuilt. Writing the entry here as well keeps an ALREADY-BUILT cache // (the common case: the very next draw reads it) from having to be thrown away. - auto& cache = GetProgramResourceCache(*programObject); + // + // Resolve the index BEFORE taking the reference, and bounds-check the way the + // sibling getter does. GetShaderStorageBlockIndex re-enters GetProgramResourceCache, + // which indexes g_programResourceCaches and can therefore insert - and that map is + // FastSTL's open-addressed unordered_map, whose rehash MOVES its buckets, so a + // reference taken before the call is left dangling. Binding a program's storage block + // while another program's entry was still absent from the cache was a reproducible + // segfault (ProgramPipelineScenario's two storage-block cases, in one process). const GLuint blockIndex = GetShaderStorageBlockIndex(*programObject, storageBlockName); if (blockIndex == GL_INVALID_INDEX) return; + auto& cache = GetProgramResourceCache(*programObject); + if (blockIndex >= cache.storageBlocks.size()) return; cache.storageBlocks[blockIndex].binding = storageBlockBinding; } void ReadPixels(GLint x, GLint y, GLsizei width, GLsizei height, GLenum format, GLenum type, void* pixels) {