diff --git a/MobileGL/MG_Impl/GLImpl/Program/GL_Program.cpp b/MobileGL/MG_Impl/GLImpl/Program/GL_Program.cpp index 6d8b9476..96dd005d 100644 --- a/MobileGL/MG_Impl/GLImpl/Program/GL_Program.cpp +++ b/MobileGL/MG_Impl/GLImpl/Program/GL_Program.cpp @@ -1581,6 +1581,16 @@ namespace MobileGL::MG_Impl::GLImpl { } else { const auto& ttype = programObject.GetUniformTypeFacts(location); if (!ttype.isTexture && !ttype.isImage) return; + // ESSL 3.10 has no way to re-point an image uniform: its unit is the binding layout + // qualifier, and ES 3.1 core 7.6.1 makes every Uniform* on one INVALID_OPERATION. + // Desktop GL sets the unit with Uniform1i, so only a program built from `es` sources. + if (ttype.isImage && programObject.IsEsOrigin()) { + MG_State::pGLContext->RecordError( + ErrorCode::InvalidOperation, + MakeUnique("MG_Impl/GLImpl", __func__, + "An image uniform of an ES shader cannot be set with Uniform*.")); + return; + } if constexpr (!std::is_same_v, GLint> || ItemCount != 1) { MG_State::pGLContext->RecordError( ErrorCode::InvalidOperation, diff --git a/MobileGL/MG_State/GLState/ProgramState/ProgramArtifacts.h b/MobileGL/MG_State/GLState/ProgramState/ProgramArtifacts.h index 90fce830..f354f073 100644 --- a/MobileGL/MG_State/GLState/ProgramState/ProgramArtifacts.h +++ b/MobileGL/MG_State/GLState/ProgramState/ProgramArtifacts.h @@ -337,6 +337,10 @@ namespace MobileGL::MG_State::GLState { // gate has to be answerable without joining the SPIR-V job, or every draw of every // program would pay a join to discover it has nothing to write. Bool usesReservedNumSamples = false; + // A shader of this program declared an `es` #version (ShaderTranspiler::FindEsOrigin). + // ESSL fixes an image uniform's unit at its binding qualifier, so glUniform* on one is + // INVALID_OPERATION for such a program and legal for a desktop one. + Bool esOrigin = false; Uint maxUniformLocation = 0; Int uniformNameMaxLength = 0; Int attribInNameMaxLength = 0; @@ -575,6 +579,7 @@ namespace MobileGL::MG_State::GLState { v("uniformBlocksWithoutBinding", a.uniformBlocksWithoutBinding); v("activeUniformCount", a.activeUniformCount); v("usesReservedNumSamples", a.usesReservedNumSamples); + v("esOrigin", a.esOrigin); v("maxUniformLocation", a.maxUniformLocation); v("uniformNameMaxLength", a.uniformNameMaxLength); v("attribInNameMaxLength", a.attribInNameMaxLength); diff --git a/MobileGL/MG_State/GLState/ProgramState/ProgramLinkTask.cpp b/MobileGL/MG_State/GLState/ProgramState/ProgramLinkTask.cpp index b58e256c..58d82057 100644 --- a/MobileGL/MG_State/GLState/ProgramState/ProgramLinkTask.cpp +++ b/MobileGL/MG_State/GLState/ProgramState/ProgramLinkTask.cpp @@ -795,6 +795,13 @@ namespace MobileGL::MG_State::GLState { MergeShaderSideChannels(); if (!artifacts.infoLog.empty()) return; // a conflicting explicit uniform location + // Before the L1 probe, like the side channels: a memo hit publishes the artifacts of an + // identical earlier link, which computed the same answer from the same sources. + artifacts.esOrigin = std::any_of(in.shaders.begin(), in.shaders.end(), [](const LinkShaderInput& input) { + return MG_Util::ShaderTranspiler::FindEsOrigin(CompiledArtifacts(input.compiled).preprocessedSource) + .has_value(); + }); + // ---- L1 of the shader translation memo ---- // Everything below this point - the parse, the link, mapIO, GlslangToSpv, spirv-opt, // buildReflection and the global-UBO routing - is what a hit skips. See diff --git a/MobileGL/MG_State/GLState/ProgramState/ProgramObject.h b/MobileGL/MG_State/GLState/ProgramState/ProgramObject.h index e5fce84b..392373b8 100644 --- a/MobileGL/MG_State/GLState/ProgramState/ProgramObject.h +++ b/MobileGL/MG_State/GLState/ProgramState/ProgramObject.h @@ -783,6 +783,7 @@ namespace MobileGL::MG_State::GLState { // PHASE A: answerable without joining the SPIR-V job, which is what lets the draw path ask // every program this question and pay nothing for the overwhelming majority that say no. Bool UsesReservedNumSamples() const { return Artifacts().usesReservedNumSamples; } + Bool IsEsOrigin() const { return Artifacts().esOrigin; } // Publishes `samples` into the global-UBO shadow. Returns false when there is nowhere to // put it - no shim in this program, no SPIR-V (a cancelled phase B), or the optimizer diff --git a/MobileGL/MG_Test/Program/ProgramArtifactsCodecTest.cpp b/MobileGL/MG_Test/Program/ProgramArtifactsCodecTest.cpp index aeebd9b0..2c043fa4 100644 --- a/MobileGL/MG_Test/Program/ProgramArtifactsCodecTest.cpp +++ b/MobileGL/MG_Test/Program/ProgramArtifactsCodecTest.cpp @@ -530,8 +530,9 @@ TEST(ProgramArtifactsCodec, AVersionMismatchIsRefused) { // no archive owns. A codec arm for it would be a use-after-free waiting for a cache hit. TEST(ProgramArtifactsCodec, TheTablesVisitEveryMemberExceptTheLiveProgram) { // P7 OQ-8: 59 members, 58 visited. The 59th is still the live TProgram; the 58th is - // storageBlocks, which follows the record arm's guard (ProgramArtifacts.h). - EXPECT_EQ(ProgramArtifactsVisitedFieldCount(), 58u); + // storageBlocks, which follows the record arm's guard (ProgramArtifacts.h). esOrigin then + // made it 60 and 59. + EXPECT_EQ(ProgramArtifactsVisitedFieldCount(), 59u); EXPECT_EQ(ProgramArtifactsVisitedFieldCount(), 3u); EXPECT_EQ(ProgramArtifactsVisitedFieldCount(), 8u); EXPECT_EQ(ProgramArtifactsVisitedFieldCount(), 14u); diff --git a/MobileGL/MG_Test/Program/ProgramArtifactsTest.cpp b/MobileGL/MG_Test/Program/ProgramArtifactsTest.cpp index 98a6212a..1e87cbe3 100644 --- a/MobileGL/MG_Test/Program/ProgramArtifactsTest.cpp +++ b/MobileGL/MG_Test/Program/ProgramArtifactsTest.cpp @@ -84,7 +84,7 @@ namespace { // P7 OQ-8 added LinkArtifacts::storageBlocks and its own three-field table; since P13 W5 // the member follows the record arm (ProgramArtifacts.h), which every library builds. EXPECT_EQ(CountFields(), 3u); - EXPECT_EQ(CountFields(), 58u); + EXPECT_EQ(CountFields(), 59u); EXPECT_EQ(CountFields(), 8u); } diff --git a/MobileGL/MG_Test/Program/ProgramInterfaceTest.cpp b/MobileGL/MG_Test/Program/ProgramInterfaceTest.cpp index dbb2d0e0..1e9f75b7 100644 --- a/MobileGL/MG_Test/Program/ProgramInterfaceTest.cpp +++ b/MobileGL/MG_Test/Program/ProgramInterfaceTest.cpp @@ -974,6 +974,99 @@ layout(points, max_vertices = 1) out; ClearErrors(); } + // ESSL 3.10 4.3.9 makes a variable index into a shader storage (or uniform) block array a + // compile error; desktop GLSL and ESSL 3.20 do not. An `es` source is compiled through a + // rewrite to desktop 460, so the rule is kept only for the sources that declared `es` - + // KHR-GL4x.es_31_compatibility.shader_storage_buffer_object.negative-glsl-compileTime. + TEST_F(ProgramInterfaceTest, EsslBlockArraysNeedAConstantIndexOnlyInEsslBelow320) { + std::string log; + const auto compiles = [&log](const std::string& version) { + const std::string source = version + R"( +layout(local_size_x = 1) in; +layout(std430, binding = 0) buffer Input { float data0; } g_input[4]; +layout(std430, binding = 4) buffer Output { float data0; } g_output[4]; +void main() { + for (int i = 0; i < 4; ++i) g_output[i].data0 = g_input[i].data0; +} +)"; + const char* sourcePtr = source.c_str(); + const GLuint sh = CreateShader(GL_COMPUTE_SHADER); + ShaderSource(sh, 1, &sourcePtr, nullptr); + CompileShader(sh); + GLint status = -1; + GetShaderiv(sh, GL_COMPILE_STATUS, &status); + char buffer[4096] = ""; + GetShaderInfoLog(sh, sizeof(buffer), nullptr, buffer); + log = buffer; + DeleteShader(sh); + return status == GL_TRUE; + }; + EXPECT_FALSE(compiles("#version 310 es")); + EXPECT_NE(log.find("constant integral expression"), std::string::npos) << "info log was: " << log; + EXPECT_TRUE(compiles("#version 320 es")); + EXPECT_TRUE(compiles("#version 430")); + + const char* constantIndex = R"(#version 310 es +layout(local_size_x = 1) in; +layout(std430, binding = 0) buffer Input { float data0; } g_input[4]; +void main() { g_input[3].data0 = g_input[0].data0; } +)"; + const GLuint sh = CreateShader(GL_COMPUTE_SHADER); + ShaderSource(sh, 1, &constantIndex, nullptr); + CompileShader(sh); + GLint status = -1; + GetShaderiv(sh, GL_COMPILE_STATUS, &status); + EXPECT_EQ(status, GL_TRUE) << "a constant index stays legal"; + DeleteShader(sh); + ClearErrors(); + } + + // ES 3.1 fixes an image uniform's unit at its binding qualifier, so every Uniform* on one is + // INVALID_OPERATION in a program built from `es` sources - while a desktop program sets the + // unit with glUniform1i. KHR-GL4x.es_31_compatibility.shader_image_load_store.negative-uniform. + TEST_F(ProgramInterfaceTest, ImageUniformsOfEsProgramsCannotBeSet) { + const auto imageProgram = [](const char* version) { + const std::string vs = std::string(version) + R"( +layout(location = 0) in vec4 i_position; +void main() { gl_Position = i_position; } +)"; + const std::string fs = std::string(version) + R"( +layout(rgba32f, binding = 0) writeonly uniform highp image2D g_image; +void main() { + imageStore(g_image, ivec2(gl_FragCoord.xy), vec4(0.0)); + discard; +} +)"; + const GLuint p = MakeProgram(vs.c_str(), fs.c_str()); + LinkProgram(p); + return p; + }; + + const GLuint es = imageProgram("#version 310 es"); + ExpectLinked(es); + ClearErrors(); + const GLint esLocation = GetUniformLocation(es, "g_image"); + ASSERT_GE(esLocation, 0); + UseProgram(es); + Uniform1i(esLocation, 1); + EXPECT_EQ(TakeError(), static_cast(GL_INVALID_OPERATION)); + const GLint unit = 1; + Uniform1iv(esLocation, 1, &unit); + EXPECT_EQ(TakeError(), static_cast(GL_INVALID_OPERATION)); + UseProgram(0); + ProgramUniform1i(es, esLocation, 1); + EXPECT_EQ(TakeError(), static_cast(GL_INVALID_OPERATION)); + + const GLuint desktop = imageProgram("#version 430"); + ExpectLinked(desktop); + ClearErrors(); + const GLint desktopLocation = GetUniformLocation(desktop, "g_image"); + ASSERT_GE(desktopLocation, 0); + ProgramUniform1i(desktop, desktopLocation, 1); + EXPECT_EQ(TakeError(), static_cast(GL_NO_ERROR)); + ClearErrors(); + } + // GL 4.6 core 7.6 fails the link when a stage's active image uniforms exceed // GL_MAX_*_IMAGE_UNIFORMS, or when their sum exceeds GL_MAX_COMBINED_IMAGE_UNIFORMS. Nothing // counted them - glslang keeps those numbers only so gl_Max*ImageUniforms can expand from diff --git a/MobileGL/MG_Test/Program/ProgramUtilTest.cpp b/MobileGL/MG_Test/Program/ProgramUtilTest.cpp index 5a3e3704..dd5233c4 100644 --- a/MobileGL/MG_Test/Program/ProgramUtilTest.cpp +++ b/MobileGL/MG_Test/Program/ProgramUtilTest.cpp @@ -558,8 +558,9 @@ void main() { }; // Preserve the pre-existing desktop-core route: the current resource table cannot parse ESSL built-ins. - verifyVersion("#version 300 es", "#version 460 core\n"); - verifyVersion("#version 310 es", "#version 460 core\n"); + // The directive carries the declared ESSL version, which FindEsOrigin reads back. + verifyVersion("#version 300 es", "#version 460 core /*mobilegl-es-origin:300*/\n"); + verifyVersion("#version 310 es", "#version 460 core /*mobilegl-es-origin:310*/\n"); } TEST_F(ProgramUtilTest, PreprocessModernDesktopVersionsRecognizesUtf8Bom) { diff --git a/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.cpp b/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.cpp index b4b6a952..b3c0f85f 100644 --- a/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.cpp +++ b/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.cpp @@ -348,6 +348,48 @@ namespace MobileGL { return res; } + namespace { + // The first variable index into a uniform or shader storage block ARRAY, if any. + class BlockArrayVariableIndexFinder final : public glslang::TIntermTraverser { + public: + const glslang::TIntermBinary* found = nullptr; + + bool visitBinary(glslang::TVisit, glslang::TIntermBinary* node) override { + if (found) return false; + if (node->getOp() != glslang::EOpIndexIndirect) return true; + const glslang::TType& base = node->getLeft()->getType(); + const glslang::TStorageQualifier storage = base.getQualifier().storage; + if (base.getBasicType() == glslang::EbtBlock && base.isArray() && + (storage == glslang::EvqBuffer || storage == glslang::EvqUniform)) { + found = node; + return false; + } + return true; + } + }; + + // ESSL 3.10 4.3.9: "All indices used to index a uniform or shader storage block array + // must be constant integral expressions." ESSL 3.20 and GL_EXT/OES_gpu_shader5 relax + // it to dynamically uniform; desktop GLSL has no such rule at all. The rewrite to + // "#version 460 core" lost it, so it is put back here for the sources that declared + // an `es` version - and only those. Returns the info-log line, or empty. + String FindEsBlockArrayIndexViolation(const glslang::TShader& shader, const String& source) { + const std::optional origin = FindEsOrigin(source); + if (!origin || origin->version >= 320 || origin->gpuShader5) return {}; + glslang::TIntermediate* intermediate = shader.getIntermediate(); + if (intermediate == nullptr || intermediate->getTreeRoot() == nullptr) return {}; + BlockArrayVariableIndexFinder finder; + intermediate->getTreeRoot()->traverse(&finder); + if (!finder.found) return {}; + const glslang::TType& base = finder.found->getLeft()->getType(); + return std::format("ERROR: 0:{}: '[' : a {} block array must be indexed with a constant " + "integral expression in ESSL {} without GL_EXT_gpu_shader5\n", + finder.found->getLoc().line, + base.getQualifier().storage == glslang::EvqBuffer ? "shader storage" : "uniform", + origin->version); + } + } // namespace + Result> ShaderCompiler::CompileShader(const ShaderAttrib& attrib) { auto shaderType = attrib.shaderType; @@ -361,7 +403,19 @@ namespace MobileGL { const String source(attrib.sourceStr); auto result = ParseShaderSource(lang, shaderType, source, attrib.flags, attrib.env); - if (result) return result; + if (result) { + // Part of the parse verdict, so an L1c hit and ClaimParsedShader's re-parse reach + // the same answer as this parse did. + if (String violation = FindEsBlockArrayIndexViolation(*result.value(), source); + !violation.empty()) { + ResultInfo r; + r.log = "Error: [glslang] Cannot compile " + ConvertGLEnumToString(shaderType) + ":\n" + + violation; + r.errc = -2; + return std::unexpected(r); + } + return result; + } // Legacy desktop sources are normalized to "#version 330 core" (with a marker on the // directive), which parses under stricter rules than the 460 they used to be forced diff --git a/MobileGL/MG_Util/ShaderTranspiler/ShaderSourceProcessor.cpp b/MobileGL/MG_Util/ShaderTranspiler/ShaderSourceProcessor.cpp index d7ed95b4..f848c980 100644 --- a/MobileGL/MG_Util/ShaderTranspiler/ShaderSourceProcessor.cpp +++ b/MobileGL/MG_Util/ShaderTranspiler/ShaderSourceProcessor.cpp @@ -465,13 +465,18 @@ namespace { // Stamped onto the normalized directive when a legacy (or absent) desktop // version was rewritten to 330; consumed by RetargetLegacyVersionDirectiveTo460. constexpr const char* kNormalizedLegacyMarker = "/*mobilegl-normalized-legacy*/"; + // Stamped onto the directive an ES source is rewritten to, with the ESSL version it declared: + // the rules ESSL has and desktop GLSL does not are enforced off it (FindEsOrigin), and it keeps + // an ES source from sharing a parse verdict with a byte-identical desktop one. + constexpr const char* kEsOriginMarkerPrefix = "/*mobilegl-es-origin:"; MobileGL::String GetNormalizedVersionDirective(const ShaderLanguageInfo& info) { if (info.profile == MobileGL::ShaderProfile::ES) { // Preserve the pre-existing behavior for standard lowercase "es" directives. MobileGL's Vulkan // glslang resource table cannot parse its ESSL built-ins today, even at ESSL 310, whereas the same // source is accepted through the normalized desktop core path. - return "#version 460 core\n"; + return MobileGL::String("#version 460 core ") + kEsOriginMarkerPrefix + std::to_string(info.version) + + "*/\n"; } // Keep compatibility-profile handling on its pre-existing 460 path. Vulkan glslang does not accept that @@ -1978,6 +1983,32 @@ namespace MobileGL { return names; } + std::optional FindEsOrigin(const String& preprocessedSource) { + const SizeT markerStart = preprocessedSource.find(kEsOriginMarkerPrefix); + if (markerStart == String::npos) return std::nullopt; + SizeT probe = markerStart + std::char_traits::length(kEsOriginMarkerPrefix); + Uint esVersion = 0; + bool hasDigits = false; + while (probe < preprocessedSource.size() && preprocessedSource[probe] >= '0' && + preprocessedSource[probe] <= '9') { + hasDigits = true; + esVersion = esVersion * 10 + static_cast(preprocessedSource[probe] - '0'); + if (esVersion > 1000) return std::nullopt; // absurd; not a marker this pipeline wrote + probe++; + } + if (!hasDigits || preprocessedSource.compare(probe, 2, "*/") != 0) return std::nullopt; + + // The #extension lines survive preprocessing, and enabledExtensions is already + // closed under glslang's implication graph (GL_ANDROID_extension_pack_es31a turns + // GL_EXT_gpu_shader5 on). + const ShaderLanguageInfo info = InspectShaderLanguage(preprocessedSource); + EsOrigin origin; + origin.version = esVersion; + origin.gpuShader5 = ExtensionTurnedOn(info, "GL_EXT_gpu_shader5") || + ExtensionTurnedOn(info, "GL_OES_gpu_shader5"); + return origin; + } + String CollectEsPreambleMacroDefines(const String& preprocessedSource) { const SizeT markerStart = preprocessedSource.find(kEsPreambleMarkerPrefix); if (markerStart == String::npos) return {}; diff --git a/MobileGL/MG_Util/ShaderTranspiler/ShaderSourceProcessor.h b/MobileGL/MG_Util/ShaderTranspiler/ShaderSourceProcessor.h index d19ab93a..4fbcdf74 100644 --- a/MobileGL/MG_Util/ShaderTranspiler/ShaderSourceProcessor.h +++ b/MobileGL/MG_Util/ShaderTranspiler/ShaderSourceProcessor.h @@ -61,6 +61,16 @@ namespace MobileGL { // GetNormalizedVersionDirective records why that has not happened. String CollectEsPreambleMacroDefines(const String& preprocessedSource); + // What an ES-profile source declared before PreprocessShaderSource rewrote its + // #version to desktop - recovered from the marker the rewrite leaves on the directive - + // or nullopt for a source that did not declare an `es` version. The few rules ESSL has + // and desktop GLSL does not are enforced off this, and only for such sources. + struct EsOrigin { + Uint version = 0; // 300, 310, 320 + Bool gpuShader5 = false; // GL_EXT_gpu_shader5 / GL_OES_gpu_shader5 turned on + }; + std::optional FindEsOrigin(const String& preprocessedSource); + // The uniforms a shader declared as samplerExternalOES (GL_OES_EGL_image_external), // which PreprocessShaderSource turned into sampler2D and named in a marker: the // reflection reports them as GL_SAMPLER_EXTERNAL_OES. Empty for every other shader.