diff --git a/MobileGL/MG_Test/Program/ProgramUtilTest.cpp b/MobileGL/MG_Test/Program/ProgramUtilTest.cpp index 1ccf7d42..565712b5 100644 --- a/MobileGL/MG_Test/Program/ProgramUtilTest.cpp +++ b/MobileGL/MG_Test/Program/ProgramUtilTest.cpp @@ -426,6 +426,62 @@ void main() { verifyVersion("#version 460 core"); } +// KHR-GL33.shaders.preprocessor.directive.version_* (also re-run verbatim under GL40-GL44): the +// compiler must REJECT a malformed #version line. MobileGL used to rewrite the whole line to +// "#version 330 core" whenever it could scrape a leading integer - or treat an unknown profile token +// as core - which silently legalized every form below. CTS compiles the shader's own #version +// verbatim, so the rejection has to survive preprocessing (and the 460 retry). +TEST_F(ProgramUtilTest, PreprocessRejectsMalformedVersionDirectives) { + using namespace MG_Util::ShaderTranspiler; + + const char* body = "\nout vec4 fragColor;\nvoid main() { fragColor = vec4(1.0); }\n"; + const auto rejects = [](const String& fullSource) { + String src = fullSource; + PreprocessShaderSource(ShaderStage::Fragment, src); + ShaderAttrib attrib{.shaderType = GL_FRAGMENT_SHADER, .sourceStr = src}; + auto res = ShaderCompiler::CompileShader(attrib); + return res ? false : true; // "rejects" == compile failed + }; + + // Silently legalized today - the five this fix must flip to rejection: + EXPECT_TRUE(rejects(String("#version 329") + body)) << "329 is not a real version"; + EXPECT_TRUE(rejects(String("#version 331") + body)) << "331 is not a real version"; + EXPECT_TRUE(rejects(String("#version 330 foo") + body)) << "unknown profile keyword"; + EXPECT_TRUE(rejects(String("#version 330.0") + body)) << "float literal, not an int token"; + EXPECT_TRUE(rejects(String("#version 330 foobar") + body)) << "trailing tokens after a valid decl"; + + // Already rejected (no leading integer, or #version is not the first token) - pinned so a future + // change to the normalizer cannot start legalizing them either: + EXPECT_TRUE(rejects(String("#version") + body)) << "missing version number"; + EXPECT_TRUE(rejects(String("#version foobar") + body)) << "identifier where the int belongs"; + EXPECT_TRUE(rejects(String("#version AAA") + body)) << "identifier where the int belongs"; + EXPECT_TRUE(rejects(String("precision mediump float;\n#version 330") + body)) + << "#version must be the first statement"; + EXPECT_TRUE(rejects(String("#define FOO BAR\n#version 330") + body)) + << "#version must precede a #define"; +} + +// The PASS half of the same CTS group: a valid decl, and #version preceded only by whitespace or a +// comment, must still compile. Guards the fix above from over-rejecting. +TEST_F(ProgramUtilTest, PreprocessKeepsValidVersionDirectivesCompiling) { + using namespace MG_Util::ShaderTranspiler; + + const char* body = "\nout vec4 fragColor;\nvoid main() { fragColor = vec4(1.0); }\n"; + const auto compiles = [](const String& fullSource) { + String src = fullSource; + PreprocessShaderSource(ShaderStage::Fragment, src); + ShaderAttrib attrib{.shaderType = GL_FRAGMENT_SHADER, .sourceStr = src}; + auto res = ShaderCompiler::CompileShader(attrib); + return res ? true : false; + }; + + EXPECT_TRUE(compiles(String("#version 330 core") + body)); + EXPECT_TRUE(compiles(String("\n#version 330 core") + body)) + << "leading whitespace is legal before #version"; + EXPECT_TRUE(compiles(String("// test\n#version 330 core") + body)) + << "a leading comment is legal before #version"; +} + TEST_F(ProgramUtilTest, PreprocessUsesRealSpacedVersionDirectiveForInjectedOutput) { using namespace MG_Util::ShaderTranspiler; @@ -847,6 +903,16 @@ TEST_F(ProgramUtilTest, RetargetLegacyVersionDirectiveOnlyTouchesNormalizedDeskt String commented = "// #version 330 core\nvoid main() {}\n"; EXPECT_FALSE(RetargetLegacyVersionDirectiveTo460(commented)); EXPECT_EQ(commented.find("#version 460"), String::npos); + + // A malformed directive must NOT be rescued to 460 - that is what silently legalized the CTS + // directive.version_* rejection cases. The bad version stays put so glslang keeps rejecting it. + String badNumber = "#version 331\nvoid main() {}\n"; + EXPECT_FALSE(RetargetLegacyVersionDirectiveTo460(badNumber)); + EXPECT_EQ(badNumber.find("#version 460"), String::npos); + + String badProfile = "#version 330 foo\nvoid main() {}\n"; + EXPECT_FALSE(RetargetLegacyVersionDirectiveTo460(badProfile)); + EXPECT_EQ(badProfile.find("#version 460"), String::npos); } const char* fs = R"(#version 150 diff --git a/MobileGL/MG_Util/ShaderTranspiler/ShaderSourceProcessor.cpp b/MobileGL/MG_Util/ShaderTranspiler/ShaderSourceProcessor.cpp index 4129a304..47c9f234 100644 --- a/MobileGL/MG_Util/ShaderTranspiler/ShaderSourceProcessor.cpp +++ b/MobileGL/MG_Util/ShaderTranspiler/ShaderSourceProcessor.cpp @@ -573,6 +573,23 @@ namespace { static_cast(source[1]) == 0xbb && static_cast(source[2]) == 0xbf; } + // The GLSL versions MobileGL is willing to normalize. Anything else in a #version line - a number + // that is not a real language version (329, 331), a bad profile keyword, a float/identifier where + // the integer belongs, or trailing tokens - is left untouched so glslang rejects it, matching + // KHR-GL33.shaders.preprocessor.directive.version_*. The set is deliberately generous (every real + // desktop and ES version) so the normalizer never starts rejecting a form it used to accept. + bool IsRecognizedGlslVersion(unsigned version) { + switch (version) { + case 100: case 110: case 120: case 130: case 140: case 150: + case 300: case 310: case 320: + case 330: case 400: case 410: case 420: case 430: + case 440: case 450: case 460: + return true; + default: + return false; + } + } + struct ShaderLanguageInfo { unsigned version = 110; MobileGL::ShaderProfile profile = MobileGL::ShaderProfile::Core; @@ -580,6 +597,9 @@ namespace { SizeT versionDirectiveEnd = MobileGL::String::npos; bool hasUtf8Bom = false; bool enablesGpuShader5 = false; + // Whether the parsed #version directive is a well-formed one MobileGL should rewrite. A + // malformed directive (see IsRecognizedGlslVersion) is left alone for glslang to reject. + bool hasValidVersionDirective = false; bool HasVersionDirective() const { return versionDirectiveStart != MobileGL::String::npos; } }; @@ -623,13 +643,25 @@ namespace { info.versionDirectiveEnd = lineEnd + (hasLineBreak ? 1 : 0); SkipDirectiveWhitespace(code, probe, lineEnd); const MobileGL::String profile = ReadDirectiveIdentifier(code, probe, lineEnd); - if (profile == "es" || profile == "ES") { + bool profileTokenValid = true; + if (profile.empty() || profile == "core") { + info.profile = MobileGL::ShaderProfile::Core; + } else if (profile == "es" || profile == "ES") { info.profile = MobileGL::ShaderProfile::ES; } else if (profile == "compatibility") { info.profile = MobileGL::ShaderProfile::Compatibility; } else { + // "#version 330 foo": an unrecognized profile keyword. Keep Core for any + // downstream routing, but mark the directive malformed. info.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; + info.hasValidVersionDirective = + IsRecognizedGlslVersion(info.version) && profileTokenValid && !hasTrailingTokens; } } else if (directive == "extension") { SkipDirectiveWhitespace(code, probe, lineEnd); @@ -676,6 +708,17 @@ namespace { } void NormalizeVersionDirective(MobileGL::String& source, const ShaderLanguageInfo& info) { + // A malformed #version (329, 331, bad profile, float/trailing tokens) is left exactly as the + // application wrote it so glslang rejects it - rewriting it to "#version 330 core" would + // silently legalize the CTS directive.version_* rejection cases. Still drop a leading BOM so + // the reported error is the bad version rather than a stray byte-order mark. + if (info.HasVersionDirective() && !info.hasValidVersionDirective) { + if (info.hasUtf8Bom) { + source.erase(0, 3); + } + return; + } + const MobileGL::String replacement = GetNormalizedVersionDirective(info); if (info.HasVersionDirective()) { source.replace(info.versionDirectiveStart, info.versionDirectiveEnd - info.versionDirectiveStart, @@ -1240,6 +1283,10 @@ namespace MobileGL { // must not be mistaken for the real one. const ShaderLanguageInfo info = InspectShaderLanguage(source); if (!info.HasVersionDirective()) return false; + // Never rescue a malformed directive to 460: that is precisely what re-legalized the + // CTS directive.version_* rejection cases after the first compile failed. The shader- + // pack retry this exists for only ever sees a valid low version (a real "#version 330"). + if (!info.hasValidVersionDirective) return false; // Only the set NormalizeVersionDirective downgraded: desktop core below 400. ES and // compatibility shaders keep whatever they declared. if (info.profile != ShaderProfile::Core || info.version >= 400) return false;