diff --git a/MobileGL/MG_Test/Program/ProgramUtilTest.cpp b/MobileGL/MG_Test/Program/ProgramUtilTest.cpp index a7b8f1ae..41fc355b 100644 --- a/MobileGL/MG_Test/Program/ProgramUtilTest.cpp +++ b/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()); } } diff --git a/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.cpp b/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.cpp index a5d71789..e6a6825b 100644 --- a/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.cpp +++ b/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); diff --git a/MobileGL/MG_Util/ShaderTranspiler/ShaderSourceProcessor.cpp b/MobileGL/MG_Util/ShaderTranspiler/ShaderSourceProcessor.cpp index 480f9f62..2d975531 100644 --- a/MobileGL/MG_Util/ShaderTranspiler/ShaderSourceProcessor.cpp +++ b/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 " [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(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); - unsigned version = 0; - bool hasVersionDigits = false; - while (probe < lineEnd && code[probe] >= '0' && code[probe] <= '9') { - hasVersionDigits = true; - version = version * 10 + static_cast(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; - + ParsedVersionDirective parsed; + if (ParseVersionDirectiveBody(code, probe, lineEnd, parsed)) { if (!info.HasVersionDirective()) { - info.version = version; - info.profile = profile; + info.version = parsed.version; + info.profile = parsed.profile; info.versionDirectiveStart = directiveStart; info.versionDirectiveEnd = lineEnd + (hasLineBreak ? 1 : 0); - info.hasValidVersionDirective = directiveIsValid; - } else if (directiveIsValid && info.hasValidVersionDirective && version == info.version && - profile == info.profile) { - info.redundantVersionDirectives.push_back({directiveStart, lineEnd}); + info.hasValidVersionDirective = parsed.isValid; + } else { + recordIfRedundant(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, - AfterVersionAnchor& afterVersion) { + void MarkEsPreambleExtensionMacros(const ShaderLanguageInfo& info, MobileGL::String& source, + 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; - // std::set iteration order, so the injected block is deterministic for the translation - // cache and for the byte-exact preprocessor tests. + MobileGL::String macros; for (const MobileGL::String& extension : info.namedExtensions) { if (!IsEsOnlyPreambleExtensionMacro(extension, info.version)) continue; - shim += "#define " + extension + " 1\n"; + macros += extension; } - if (shim.empty()) return; - source.insert(afterVersion.Get(source), shim); + // Nothing the desktop preamble is missing: leave the source byte-identical. + 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 - // lost glslang's ES preamble, and the macros it carried are what the shader's own - // #if guards read. Keyed off originalLanguage because the directive has already - // been rewritten by now and no longer says "es". - InjectEsPreambleExtensionMacros(originalLanguage, source, afterVersion); + // An ES source rewritten to desktop has lost glslang's ES preamble, and the macros + // it carried are what the shader's own #if guards read. Keyed off originalLanguage + // because the directive has already been rewritten by now and no longer says "es"; + // the macros themselves are restored through the compiler's preamble, which is why + // 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::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(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 diff --git a/MobileGL/MG_Util/ShaderTranspiler/ShaderSourceProcessor.h b/MobileGL/MG_Util/ShaderTranspiler/ShaderSourceProcessor.h index 28369aef..fb673ebe 100644 --- a/MobileGL/MG_Util/ShaderTranspiler/ShaderSourceProcessor.h +++ b/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 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.