[Fix] (ShaderTranspiler): route restored ES extension macros through glslang's custom preamble and detect a mid-line repeated #version

MobileGL/MG_Test/Program/ProgramUtilTest.cpp

@@ -4717,7 +4717,9 @@ void main() { fragColor = vec4(float(gl_NumSamples)); }
 // ---------------------------------------------------------------------------------------------
 // ES preamble extension macros. Rewriting "#version 310 es" to "#version 460 core" makes glslang
 // emit its DESKTOP preamble, which defines none of the OES/AEP extension macros - so a shader's
-// own "#if !GL_OES_sample_variables" guard takes the branch it was written to avoid.
+// own "#if !GL_OES_sample_variables" guard takes the branch it was written to avoid. The macros
+// travel through glslang's CUSTOM PREAMBLE rather than the shader text, because "#define GL_..."
+// in an application-supplied string is a hard error (reservedPpErrorCheck).
 // ---------------------------------------------------------------------------------------------
 
 TEST_F(ProgramUtilTest, EsSourceRegainsThePreambleMacrosForTheExtensionsItNames) {
@@ -4736,7 +4738,9 @@ void main() { fragColor = vec4(1.0); }
 )";
 
     PreprocessShaderSource(ShaderStage::Fragment, source);
-    EXPECT_NE(source.find("#define GL_OES_sample_variables 1"), String::npos) << source;
+    EXPECT_EQ(CollectEsPreambleMacroDefines(source), String("#define GL_OES_sample_variables 1\n")) << source;
+    // And the compiler really does feed it to glslang: without the preamble this source takes the
+    // "this is broken" arm and dies on a reserved word.
     ExpectShaderCompiles(GL_FRAGMENT_SHADER, source);
 }
 
@@ -4744,17 +4748,18 @@ TEST_F(ProgramUtilTest, EsPreambleMacroInjectionStaysNarrow) {
     using namespace MG_Util::ShaderTranspiler;
 
     {
-        SCOPED_TRACE("an extension the source never names is not defined");
+        SCOPED_TRACE("only the extensions the source names, and never GL_ES");
         String source = R"(#version 310 es
 #extension GL_OES_sample_variables : enable
 out vec4 fragColor;
 void main() { fragColor = vec4(1.0); }
 )";
         PreprocessShaderSource(ShaderStage::Fragment, source);
-        EXPECT_EQ(source.find("#define GL_OES_shader_image_atomic"), String::npos) << source;
+        const String defines = CollectEsPreambleMacroDefines(source);
+        EXPECT_EQ(defines.find("GL_OES_shader_image_atomic"), String::npos) << defines;
         // GL_ES stays undefined on purpose: the shader really is compiled as desktop now, and
         // flipping "#ifdef GL_ES" branches would break far more than it fixes.
-        EXPECT_EQ(source.find("#define GL_ES"), String::npos) << source;
+        EXPECT_EQ(defines.find("#define GL_ES "), String::npos) << defines;
     }
 
     {
@@ -4765,7 +4770,9 @@ out vec4 fragColor;
 void main() { fragColor = vec4(1.0); }
 )";
         PreprocessShaderSource(ShaderStage::Fragment, source);
-        EXPECT_EQ(source.find("#define GL_EXT_shader_non_constant_global_initializers"), String::npos) << source;
+        // Nothing to restore, so the source is not even marked.
+        EXPECT_EQ(source.find("mobilegl-es-preamble"), String::npos) << source;
+        EXPECT_TRUE(CollectEsPreambleMacroDefines(source).empty());
     }
 
     {
@@ -4775,8 +4782,32 @@ void main() { fragColor = vec4(1.0); }
 out vec4 fragColor;
 void main() { fragColor = vec4(1.0); }
 )";
+        const String before = source;
         PreprocessShaderSource(ShaderStage::Fragment, source);
-        EXPECT_EQ(source.find("#define GL_OES_sample_variables"), String::npos) << source;
+        EXPECT_EQ(source, before);
+        EXPECT_TRUE(CollectEsPreambleMacroDefines(source).empty());
+    }
+
+    {
+        SCOPED_TRACE("a shader that merely contains the marker text cannot steer the preamble");
+        String source = R"(#version 460 core
+/*mobilegl-es-preamble:310*/
+#extension GL_OES_sample_variables : enable
+out vec4 fragColor;
+void main() { fragColor = vec4(1.0); }
+)";
+        // The extractor is honest about what it finds - a source carrying a well-formed marker is
+        // indistinguishable from one this pipeline wrote, which is exactly why the payload is
+        // re-derived from the whitelist here rather than read out of the marker.
+        EXPECT_EQ(CollectEsPreambleMacroDefines(source), String("#define GL_OES_sample_variables 1\n"));
+
+        String malformed = R"(#version 460 core
+/*mobilegl-es-preamble:not-a-version*/
+#extension GL_OES_sample_variables : enable
+out vec4 fragColor;
+void main() { fragColor = vec4(1.0); }
+)";
+        EXPECT_TRUE(CollectEsPreambleMacroDefines(malformed).empty());
     }
 }
 

MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.cpp

@@ -270,7 +270,15 @@ namespace MobileGL {
                 tshader->setStrings(src, 1);
                 tshader->setNanMinMaxClamp(true);
                 tshader->setInvertY(true);
-                tshader->setPreamble("#undef VULKAN\n");
+                // The custom preamble is glslang string -1, which CPPdefine exempts from the
+                // "names beginning with GL_ can't be (un)defined" rule - so it is the only place
+                // an ES source's extension macros can be put back after PreprocessShaderSource
+                // rewrote its #version to desktop and cost it glslang's ES preamble. Empty for
+                // every source that was not rewritten from ES, which is almost all of them.
+                //
+                // setPreamble stores the POINTER, so the buffer has to outlive parse() below.
+                const String preamble = String("#undef VULKAN\n") + CollectEsPreambleMacroDefines(source);
+                tshader->setPreamble(preamble.c_str());
                 if (flags & ShaderCompileBits::CompileForOpenGL) {
                     tshader->setEnvInput(glslang::EShSourceGlsl, lang, glslang::EShClientVulkan, 450);
                     tshader->setEnvClient(glslang::EShClientOpenGL, glslang::EShTargetOpenGL_450);

MobileGL/MG_Util/ShaderTranspiler/ShaderSourceProcessor.cpp

@@ -249,11 +249,73 @@ namespace {
         bool HasVersionDirective() const { return versionDirectiveStart != MobileGL::String::npos; }
     };
 
+    struct ParsedVersionDirective {
+        unsigned version = 0;
+        MobileGL::ShaderProfile profile = MobileGL::ShaderProfile::Core;
+        bool isValid = false;
+    };
+
+    // Reads "<digits> [profile]" out of a "#version" directive whose keyword ends at `probe`, and
+    // decides whether it is one MobileGL is willing to rewrite. `code` must be the masked source,
+    // so a trailing comment has already become blanks.
+    bool ParseVersionDirectiveBody(const MobileGL::String& code, SizeT probe, SizeT lineEnd,
+                                   ParsedVersionDirective& out) {
+        SkipDirectiveWhitespace(code, probe, lineEnd);
+        unsigned version = 0;
+        bool hasVersionDigits = false;
+        while (probe < lineEnd && code[probe] >= '0' && code[probe] <= '9') {
+            hasVersionDigits = true;
+            version = version * 10 + static_cast<unsigned>(code[probe] - '0');
+            probe++;
+        }
+        if (!hasVersionDigits) return false;
+
+        SkipDirectiveWhitespace(code, probe, lineEnd);
+        const MobileGL::String profileToken = ReadDirectiveIdentifier(code, probe, lineEnd);
+        bool profileTokenValid = true;
+        MobileGL::ShaderProfile profile = MobileGL::ShaderProfile::Core;
+        if (profileToken.empty() || profileToken == "core") {
+            profile = MobileGL::ShaderProfile::Core;
+        } else if (profileToken == "es" || profileToken == "ES") {
+            profile = MobileGL::ShaderProfile::ES;
+        } else if (profileToken == "compatibility") {
+            profile = MobileGL::ShaderProfile::Compatibility;
+        } else {
+            // "#version 330 foo": an unrecognized profile keyword. Keep Core for any downstream
+            // routing, but mark the directive malformed.
+            profile = MobileGL::ShaderProfile::Core;
+            profileTokenValid = false;
+        }
+        // Comments are already masked to spaces, so anything non-blank left on the line is real
+        // trailing garbage: "#version 330 foobar" / "#version 330.0".
+        SkipDirectiveWhitespace(code, probe, lineEnd);
+        const bool hasTrailingTokens = probe < lineEnd;
+
+        out.version = version;
+        out.profile = profile;
+        out.isValid = IsRecognizedGlslVersion(version) && profileTokenValid && !hasTrailingTokens;
+        return true;
+    }
+
     ShaderLanguageInfo InspectShaderLanguage(const MobileGL::String& source) {
         const MobileGL::String code = MaskCommentsAndQuotedText(source);
         ShaderLanguageInfo info;
         info.hasUtf8Bom = HasUtf8Bom(source);
 
+        // An exact repeat of the accepted directive, wherever on the line it sits. Recorded for
+        // BlankRedundantVersionDirectives; never called before a valid first directive was found,
+        // which is what keeps a LONE misplaced #version rejected.
+        const auto recordIfRedundant = [&info, &code](SizeT hashPos, SizeT lineEnd) {
+            if (!info.hasValidVersionDirective) return;
+            SizeT probe = hashPos + 1;
+            SkipDirectiveWhitespace(code, probe, lineEnd);
+            if (ReadDirectiveIdentifier(code, probe, lineEnd) != "version") return;
+            ParsedVersionDirective parsed;
+            if (!ParseVersionDirectiveBody(code, probe, lineEnd, parsed)) return;
+            if (!parsed.isValid || parsed.version != info.version || parsed.profile != info.profile) return;
+            info.redundantVersionDirectives.push_back({hashPos, lineEnd});
+        };
+
         SizeT lineStart = 0;
         while (lineStart < code.size()) {
             SizeT lineEnd = code.find('\n', lineStart);
@@ -267,54 +329,34 @@ namespace {
                 probe = 3;
             }
             SkipDirectiveWhitespace(code, probe, lineEnd);
-            if (probe < lineEnd && code[probe] == '#') {
+            if (probe >= lineEnd || code[probe] != '#') {
+                // A directive that is not first on its line is not a directive at all - except for
+                // the one case glShaderSource creates on its own: two strings each headed by a
+                // #version splice the second into the tail of the first. Only an EXACT repeat of
+                // the directive already accepted is recognized here; see
+                // BlankRedundantVersionDirectives for why that one is tolerated and nothing else.
+                for (SizeT scan = probe; scan < lineEnd; ++scan) {
+                    if (code[scan] != '#') continue;
+                    recordIfRedundant(scan, lineEnd);
+                    break;
+                }
+            } else {
                 const SizeT directiveStart = probe;
                 probe++;
                 SkipDirectiveWhitespace(code, probe, lineEnd);
                 const MobileGL::String directive = ReadDirectiveIdentifier(code, probe, lineEnd);
 
                 if (directive == "version") {
-                    SkipDirectiveWhitespace(code, probe, lineEnd);
+                    ParsedVersionDirective parsed;
-                    unsigned version = 0;
+                    if (ParseVersionDirectiveBody(code, probe, lineEnd, parsed)) {
-                    bool hasVersionDigits = false;
-                    while (probe < lineEnd && code[probe] >= '0' && code[probe] <= '9') {
-                        hasVersionDigits = true;
-                        version = version * 10 + static_cast<unsigned>(code[probe] - '0');
-                        probe++;
-                    }
-                    if (hasVersionDigits) {
-                        SkipDirectiveWhitespace(code, probe, lineEnd);
-                        const MobileGL::String profileToken = ReadDirectiveIdentifier(code, probe, lineEnd);
-                        MobileGL::ShaderProfile profile = MobileGL::ShaderProfile::Core;
-                        bool profileTokenValid = true;
-                        if (profileToken.empty() || profileToken == "core") {
-                            profile = MobileGL::ShaderProfile::Core;
-                        } else if (profileToken == "es" || profileToken == "ES") {
-                            profile = MobileGL::ShaderProfile::ES;
-                        } else if (profileToken == "compatibility") {
-                            profile = MobileGL::ShaderProfile::Compatibility;
-                        } else {
-                            // "#version 330 foo": an unrecognized profile keyword. Keep Core for any
-                            // downstream routing, but mark the directive malformed.
-                            profile = MobileGL::ShaderProfile::Core;
-                            profileTokenValid = false;
-                        }
-                        // Comments are already masked to spaces, so anything non-blank left on the
-                        // line is real trailing garbage: "#version 330 foobar" / "#version 330.0".
-                        SkipDirectiveWhitespace(code, probe, lineEnd);
-                        const bool hasTrailingTokens = probe < lineEnd;
-                        const bool directiveIsValid =
-                            IsRecognizedGlslVersion(version) && profileTokenValid && !hasTrailingTokens;
-
                         if (!info.HasVersionDirective()) {
-                            info.version = version;
+                            info.version = parsed.version;
-                            info.profile = profile;
+                            info.profile = parsed.profile;
                             info.versionDirectiveStart = directiveStart;
                             info.versionDirectiveEnd = lineEnd + (hasLineBreak ? 1 : 0);
-                            info.hasValidVersionDirective = directiveIsValid;
+                            info.hasValidVersionDirective = parsed.isValid;
-                        } else if (directiveIsValid && info.hasValidVersionDirective && version == info.version &&
+                        } else {
-                                   profile == info.profile) {
+                            recordIfRedundant(directiveStart, lineEnd);
-                            info.redundantVersionDirectives.push_back({directiveStart, lineEnd});
                         }
                     }
                 } else if (directive == "extension") {
@@ -1556,35 +1598,66 @@ namespace {
         return kEsOnlyPreambleMacros.count(name) != 0;
     }
 
+    // Marker recording that PreprocessShaderSource rewrote an ES-profile source to desktop AND
+    // that the source names at least one extension whose macro glslang's ES preamble would have
+    // defined. The declared ESSL version rides along because two of those macros are themselves
+    // version-gated in glslang.
+    //
+    // A marker rather than a "#define" block, because the macros CANNOT live in the shader text:
+    // glslang rejects "#define GL_..." outright (TParseContext::reservedPpErrorCheck, "names
+    // beginning with GL_ can't be (un)defined") for every string the application supplied - but
+    // deliberately NOT for the preamble strings, which is where its own ES preamble defines them
+    // (CPPdefine's `if (ppToken->loc.string >= 0)` gate; the two preambles sit at string index -2
+    // and -1). So the macros have to reach glslang through TShader::setPreamble, and this marker is
+    // how the decision - which needs the ORIGINAL profile and version, both gone by then - travels
+    // to the compiler. It rides inside the preprocessed source, so the preprocess cache and the
+    // translation cache both key on it for free.
+    constexpr const char* kEsPreambleMarkerPrefix = "/*mobilegl-es-preamble:";
+
+    // The set of macros named by an ES source that the desktop preamble will not define. Shared by
+    // the injector below and by CollectEsPreambleMacroDefines, which re-derives it at compile time
+    // from the marker - one whitelist, one version rule, no chance of the two disagreeing.
+    MobileGL::String BuildEsPreambleMacroList(const MobileGL::String& source, unsigned esVersion) {
+        MobileGL::String macros;
+        // std::set iteration order, so the result is deterministic for the caches and for the
+        // byte-exact preprocessor tests.
+        for (const MobileGL::String& extension : InspectShaderLanguage(source).namedExtensions) {
+            if (!IsEsOnlyPreambleExtensionMacro(extension, esVersion)) continue;
+            macros += "#define " + extension + " 1\n";
+        }
+        return macros;
+    }
+
     // GetNormalizedVersionDirective rewrites every ES-profile shader to "#version 460 core", so
     // glslang deduces a desktop profile and emits its DESKTOP preamble - and every ES-only
     // extension macro the shader is entitled to disappears with it. A CTS shader guarded by
     // `#if !GL_OES_sample_variables / this is broken / #endif` then takes the broken branch.
     //
     // The extension BEHAVIOUR survives the rewrite (glslang honours "#extension X : require" under
-    // either profile), so this is a preamble-fidelity gap and nothing more; restoring the macro is
+    // either profile), so this is a preamble-fidelity gap and nothing more; restoring the macros is
     // the whole fix.
     //
     // Strictly limited to extensions the source itself NAMES in an #extension directive. Any macro
     // injected into a desktop parse can flip a preprocessor branch, and the ES preamble carries
     // three dozen of them - defining the lot would rewrite shaders that never asked.
-    void InjectEsPreambleExtensionMacros(const ShaderLanguageInfo& info, MobileGL::String& source,
+    void MarkEsPreambleExtensionMacros(const ShaderLanguageInfo& info, MobileGL::String& source,
-                                         AfterVersionAnchor& afterVersion) {
+                                       AfterVersionAnchor& afterVersion) {
         // Only where the rewrite actually happened: a malformed directive is left for glslang to
         // reject, and a desktop source already gets the preamble it is entitled to.
         if (info.profile != MobileGL::ShaderProfile::ES) return;
         if (!info.hasValidVersionDirective) return;
         if (info.namedExtensions.empty()) return;
 
-        MobileGL::String shim;
+        MobileGL::String macros;
-        // std::set iteration order, so the injected block is deterministic for the translation
-        // cache and for the byte-exact preprocessor tests.
         for (const MobileGL::String& extension : info.namedExtensions) {
             if (!IsEsOnlyPreambleExtensionMacro(extension, info.version)) continue;
-            shim += "#define " + extension + " 1\n";
+            macros += extension;
         }
-        if (shim.empty()) return;
+        // Nothing the desktop preamble is missing: leave the source byte-identical.
-        source.insert(afterVersion.Get(source), shim);
+        if (macros.empty()) return;
+
+        source.insert(afterVersion.Get(source),
+                      MobileGL::String(kEsPreambleMarkerPrefix) + std::to_string(info.version) + "*/\n");
     }
 
     // gl_NumSamples is legal in this source only where glslang would have declared it with a
@@ -1670,11 +1743,12 @@ namespace MobileGL {
                 // via MaskCommentsAndQuotedText/TokenizeCode, so the source we hand glslang keeps them.
                 NormalizeLineDirectives(source, afterVersion.Get(source));
 
-                // Before anything that could branch on one: an ES source rewritten to desktop has
+                // An ES source rewritten to desktop has lost glslang's ES preamble, and the macros
-                // lost glslang's ES preamble, and the macros it carried are what the shader's own
+                // it carried are what the shader's own #if guards read. Keyed off originalLanguage
-                // #if guards read. Keyed off originalLanguage because the directive has already
+                // because the directive has already been rewritten by now and no longer says "es";
-                // been rewritten by now and no longer says "es".
+                // the macros themselves are restored through the compiler's preamble, which is why
-                InjectEsPreambleExtensionMacros(originalLanguage, source, afterVersion);
+                // this only leaves a marker behind (see kEsPreambleMarkerPrefix).
+                MarkEsPreambleExtensionMacros(originalLanguage, source, afterVersion);
 
                 // noperspective is intentionally NOT touched here. It is core in desktop GLSL (1.30+)
                 // and maps to the core SPIR-V NoPerspective decoration, which DirectVulkan renders
@@ -1704,6 +1778,27 @@ namespace MobileGL {
 
             }
 
+            String CollectEsPreambleMacroDefines(const String& preprocessedSource) {
+                const SizeT markerStart = preprocessedSource.find(kEsPreambleMarkerPrefix);
+                if (markerStart == String::npos) return {};
+
+                SizeT probe = markerStart + std::char_traits<char>::length(kEsPreambleMarkerPrefix);
+                unsigned esVersion = 0;
+                bool hasDigits = false;
+                while (probe < preprocessedSource.size() && preprocessedSource[probe] >= '0' &&
+                       preprocessedSource[probe] <= '9') {
+                    hasDigits = true;
+                    esVersion = esVersion * 10 + static_cast<unsigned>(preprocessedSource[probe] - '0');
+                    if (esVersion > 1000) return {}; // absurd; not a marker this pipeline wrote
+                    probe++;
+                }
+                // Only MobileGL's own marker, spelled exactly: a shader that happens to contain the
+                // prefix inside a comment of its own must not be able to steer the preamble.
+                if (!hasDigits || preprocessedSource.compare(probe, 2, "*/") != 0) return {};
+
+                return BuildEsPreambleMacroList(preprocessedSource, esVersion);
+            }
+
             Bool RetargetLegacyVersionDirectiveTo460(String& source) {
                 // Re-inspect rather than searching for the literal directive: it is not necessarily at
                 // offset 0 (a BOM or comments may precede it) and a commented-out "#version" elsewhere

MobileGL/MG_Util/ShaderTranspiler/ShaderSourceProcessor.h

@@ -40,6 +40,27 @@ namespace MobileGL {
             // accept - can be retried instead of failing to compile.
             Bool RetargetLegacyVersionDirectiveTo460(String& source);
 
+            // The "#define <EXT> 1" lines an ES-profile source needs restored after
+            // PreprocessShaderSource rewrote its #version to desktop, or "" for every other source.
+            //
+            // glslang defines the OES/AEP extension macros only in its ES preamble
+            // (TParseVersions::getPreamble), selected by the profile it deduces from the directive -
+            // so the rewrite silently takes them away and the shader's own
+            // "#if !GL_OES_sample_variables" guard flips. They cannot simply be written into the
+            // shader text: "#define GL_..." is a hard error for every application-supplied string
+            // (TParseContext::reservedPpErrorCheck). They therefore go into glslang's CUSTOM
+            // PREAMBLE, which sits at string index -1 and is exempt from that check by the same
+            // gate that exempts glslang's own preamble - hence a separate function called by the
+            // compiler rather than another injection pass.
+            //
+            // Deliberately narrow: only extensions the source itself NAMES in an #extension
+            // directive, and never GL_ES or GL_FRAGMENT_PRECISION_HIGH. The shader really is being
+            // compiled as desktop now, and flipping "#ifdef GL_ES" branches would break far more
+            // than it fixes - which is why this is a partial fix by construction. The clean
+            // long-term fix is to stop rewriting ES sources to desktop at all; the comment in
+            // GetNormalizedVersionDirective records why that has not happened.
+            String CollectEsPreambleMacroDefines(const String& preprocessedSource);
+
             // GLSL reserves a few names glslang happily accepts as identifiers ("packed",
             // "row_major" outside a layout(...) list, the image*Shadow family). Returns the
             // compile-error text for the first violation, or nullopt for a clean source.