[Fix] (Shader): keep ESSL 3.10 block-array indexing and image-uniform rules for sources that declared an es version

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<GenericErrorInfo>("MG_Impl/GLImpl", __func__,
+                                                 "An image uniform of an ES shader cannot be set with Uniform*."));
+                return;
+            }
             if constexpr (!std::is_same_v<std::remove_cv_t<T>, GLint> || ItemCount != 1) {
                 MG_State::pGLContext->RecordError(
                     ErrorCode::InvalidOperation,

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);

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

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

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<LinkArtifacts>(), 58u);
+    // storageBlocks, which follows the record arm's guard (ProgramArtifacts.h). esOrigin then
+    // made it 60 and 59.
+    EXPECT_EQ(ProgramArtifactsVisitedFieldCount<LinkArtifacts>(), 59u);
     EXPECT_EQ(ProgramArtifactsVisitedFieldCount<StorageBlockReflection>(), 3u);
     EXPECT_EQ(ProgramArtifactsVisitedFieldCount<SpirvArtifacts>(), 8u);
     EXPECT_EQ(ProgramArtifactsVisitedFieldCount<ResourceReflection>(), 14u);

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<StorageBlockReflection>(), 3u);
-        EXPECT_EQ(CountFields<LinkArtifacts>(), 58u);
+        EXPECT_EQ(CountFields<LinkArtifacts>(), 59u);
         EXPECT_EQ(CountFields<SpirvArtifacts>(), 8u);
     }
 

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<GLenum>(GL_INVALID_OPERATION));
+        const GLint unit = 1;
+        Uniform1iv(esLocation, 1, &unit);
+        EXPECT_EQ(TakeError(), static_cast<GLenum>(GL_INVALID_OPERATION));
+        UseProgram(0);
+        ProgramUniform1i(es, esLocation, 1);
+        EXPECT_EQ(TakeError(), static_cast<GLenum>(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<GLenum>(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

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) {

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<EsOrigin> 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<SharedPtr<glslang::TShader>> 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

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<EsOrigin> FindEsOrigin(const String& preprocessedSource) {
+                const SizeT markerStart = preprocessedSource.find(kEsOriginMarkerPrefix);
+                if (markerStart == String::npos) return std::nullopt;
+                SizeT probe = markerStart + std::char_traits<char>::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<Uint>(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 {};

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<EsOrigin> 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.