From e2f873c95cf1bcb2c03795378ff8b21e0b459091 Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Thu, 16 Jul 2026 22:33:30 -0400 Subject: [PATCH] [Fix] (MG_Util/ShaderTranspiler): retry a legacy shader at 460 when it fails to parse as normalized 330 core, so sources using 420-era syntax without the matching #extension line keep compiling as they did on real drivers --- MobileGL/MG_Test/Program/ProgramUtilTest.cpp | 75 +++++++++++++++++++ .../ShaderTranspiler/ShaderCompiler.cpp | 58 ++++++++++---- .../ShaderSourceProcessor.cpp | 15 ++++ .../ShaderTranspiler/ShaderSourceProcessor.h | 8 ++ 4 files changed, 142 insertions(+), 14 deletions(-) diff --git a/MobileGL/MG_Test/Program/ProgramUtilTest.cpp b/MobileGL/MG_Test/Program/ProgramUtilTest.cpp index 1b272dd3..afbf2586 100644 --- a/MobileGL/MG_Test/Program/ProgramUtilTest.cpp +++ b/MobileGL/MG_Test/Program/ProgramUtilTest.cpp @@ -556,6 +556,81 @@ TEST_F(ProgramUtilTest, CompileSimpleVertexShader) { } } +// Legacy desktop sources are normalized to "#version 330 core", which is stricter than the 460 they +// used to be forced to. A shader declaring 330 while using 420-era syntax without the matching +// #extension line is accepted by real drivers, so CompileShader retries it at 460 instead of failing. +TEST_F(ProgramUtilTest, CompileShaderRetriesAt460WhenLegacyVersionRejects420Syntax) { + using namespace MG_Util::ShaderTranspiler; + String source = R"(#version 330 +layout(binding = 0) uniform sampler2D InSampler; +in vec2 texCoord; +out vec4 fragColor; +void main() { + fragColor = texture(InSampler, texCoord); +})"; + PreprocessShaderSource(ShaderStage::Fragment, source); + // The normal path still emits 330 - the retry must not become the default. + ASSERT_EQ(source.find("#version 330 core"), 0u); + + ShaderAttrib attrib{.shaderType = GL_FRAGMENT_SHADER, .sourceStr = source}; + auto res = ShaderCompiler::CompileShader(attrib); + if (!res) { + FAIL() << "errc: " << res.error().errc << "\nlog: " << res.error().log; + } + + // Same source compiled for the OpenGL environment must take the retry too. + ShaderAttrib glAttrib{ + .shaderType = GL_FRAGMENT_SHADER, .sourceStr = source, .flags = ShaderCompileBits::CompileForOpenGL}; + auto glRes = ShaderCompiler::CompileShader(glAttrib); + if (!glRes) { + FAIL() << "errc: " << glRes.error().errc << "\nlog: " << glRes.error().log; + } +} + +TEST_F(ProgramUtilTest, CompileShaderStillFailsWithOriginalDiagnosticsWhenRetryCannotHelp) { + using namespace MG_Util::ShaderTranspiler; + String source = R"(#version 330 +in vec2 texCoord; +out vec4 fragColor; +void main() { + fragColor = thisFunctionDoesNotExist(texCoord); +})"; + PreprocessShaderSource(ShaderStage::Fragment, source); + + ShaderAttrib attrib{.shaderType = GL_FRAGMENT_SHADER, .sourceStr = source}; + auto res = ShaderCompiler::CompileShader(attrib); + ASSERT_FALSE(res); + EXPECT_EQ(res.error().errc, -2); + EXPECT_NE(res.error().log.find("thisFunctionDoesNotExist"), String::npos) << res.error().log; +} + +TEST_F(ProgramUtilTest, RetargetLegacyVersionDirectiveOnlyTouchesNormalizedDesktopCore) { + using namespace MG_Util::ShaderTranspiler; + + String normalized = "#version 330 core\nvoid main() {}\n"; + EXPECT_TRUE(RetargetLegacyVersionDirectiveTo460(normalized)); + EXPECT_EQ(normalized.find("#version 460 core"), 0u); + + // Already modern: nothing to retarget. + String modern = "#version 460 core\nvoid main() {}\n"; + EXPECT_FALSE(RetargetLegacyVersionDirectiveTo460(modern)); + EXPECT_EQ(modern.find("#version 460 core"), 0u); + + // ES and compatibility sources keep what they declared. + String es = "#version 300 es\nvoid main() {}\n"; + EXPECT_FALSE(RetargetLegacyVersionDirectiveTo460(es)); + EXPECT_EQ(es.find("#version 300 es"), 0u); + + String compat = "#version 330 compatibility\nvoid main() {}\n"; + EXPECT_FALSE(RetargetLegacyVersionDirectiveTo460(compat)); + EXPECT_EQ(compat.find("#version 330 compatibility"), 0u); + + // A commented-out directive is not the real one. + String commented = "// #version 330 core\nvoid main() {}\n"; + EXPECT_FALSE(RetargetLegacyVersionDirectiveTo460(commented)); + EXPECT_EQ(commented.find("#version 460"), String::npos); +} + const char* fs = R"(#version 150 uniform sampler2D InSampler; diff --git a/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.cpp b/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.cpp index 4833d1d0..bd60797b 100644 --- a/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.cpp +++ b/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.cpp @@ -18,6 +18,7 @@ #include "spirv-tools/libspirv.h" #include "spirv-tools/optimizer.hpp" +#include "ShaderSourceProcessor.h" #include #include @@ -133,27 +134,23 @@ namespace MobileGL { return Resources; } - Result> ShaderCompiler::CompileShader(const ShaderAttrib& attrib) { - auto shaderType = attrib.shaderType; - auto& sourceStr = attrib.sourceStr; - - auto lang = MG_Util::ConvertGLEnumToEShLanguage(shaderType); - if (lang == EShLanguage::EShLangCount) { - ResultInfo r; - r.log += "Error: [Preprocess] Unsupported shader type: " + ConvertGLEnumToString(shaderType); - r.errc = -1; - return std::unexpected(r); - } - + // One parse attempt. A glslang::TShader cannot be re-parsed, so a retry has to build a + // fresh one with byte-identical setup - hence a single factored body rather than two + // copies that could drift apart. + static Result> ParseShaderSource(EShLanguage lang, GLenum shaderType, + const String& source, + Flags flags) { SharedPtr res; auto& tshader = res; tshader = MakeShared(lang); - const char* src[] = {sourceStr.data()}; + // setStrings gets no length array, so it relies on NUL termination: source must be an + // owning buffer that outlives parse(), never a StringView's substring. + const char* src[] = {source.c_str()}; tshader->setStrings(src, 1); tshader->setNanMinMaxClamp(true); tshader->setInvertY(true); tshader->setPreamble("#undef VULKAN\n"); - if (attrib.flags & ShaderCompileBits::CompileForOpenGL) { + if (flags & ShaderCompileBits::CompileForOpenGL) { tshader->setEnvInput(glslang::EShSourceGlsl, lang, glslang::EShClientVulkan, 450); tshader->setEnvClient(glslang::EShClientOpenGL, glslang::EShTargetOpenGL_450); tshader->setEnvTarget(glslang::EShTargetSpv, glslang::EShTargetSpv_1_3); @@ -182,6 +179,39 @@ namespace MobileGL { return res; } + Result> ShaderCompiler::CompileShader(const ShaderAttrib& attrib) { + auto shaderType = attrib.shaderType; + + auto lang = MG_Util::ConvertGLEnumToEShLanguage(shaderType); + if (lang == EShLanguage::EShLangCount) { + ResultInfo r; + r.log += "Error: [Preprocess] Unsupported shader type: " + ConvertGLEnumToString(shaderType); + r.errc = -1; + return std::unexpected(r); + } + + const String source(attrib.sourceStr); + auto result = ParseShaderSource(lang, shaderType, source, attrib.flags); + if (result) return result; + + // Legacy desktop sources are normalized to "#version 330 core", which parses under + // stricter rules than the 460 they used to be forced to: a shader declaring 330 while + // using e.g. layout(binding=...) without the matching #extension line compiles on real + // drivers but is rejected here. Retry once at 460 before reporting failure; a genuinely + // broken shader fails both attempts and keeps its original diagnostics. + String retrySource = source; + if (!MG_Util::ShaderTranspiler::RetargetLegacyVersionDirectiveTo460(retrySource)) { + return result; + } + + auto retryResult = ParseShaderSource(lang, shaderType, retrySource, attrib.flags); + if (!retryResult) return result; + + MGLOG_D("CompileShader: %s only parsed after retargeting its legacy #version to 460", + ConvertGLEnumToString(shaderType).c_str()); + return retryResult; + } + Result> ShaderCompiler::LinkProgram(const ProgramAttrib& attrib) { SharedPtr program = MakeShared(); for (auto& s : attrib.shaders) { diff --git a/MobileGL/MG_Util/ShaderTranspiler/ShaderSourceProcessor.cpp b/MobileGL/MG_Util/ShaderTranspiler/ShaderSourceProcessor.cpp index 5466f24e..87a82d69 100644 --- a/MobileGL/MG_Util/ShaderTranspiler/ShaderSourceProcessor.cpp +++ b/MobileGL/MG_Util/ShaderTranspiler/ShaderSourceProcessor.cpp @@ -633,6 +633,21 @@ namespace MobileGL { InjectDepthRangeBuiltinShim(stage, source); } + 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 + // must not be mistaken for the real one. + const ShaderLanguageInfo info = InspectShaderLanguage(source); + if (!info.HasVersionDirective()) 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; + + source.replace(info.versionDirectiveStart, info.versionDirectiveEnd - info.versionDirectiveStart, + "#version 460 core\n"); + return true; + } + } // namespace ShaderTranspiler } // namespace MG_Util } // namespace MobileGL diff --git a/MobileGL/MG_Util/ShaderTranspiler/ShaderSourceProcessor.h b/MobileGL/MG_Util/ShaderTranspiler/ShaderSourceProcessor.h index f9d1c510..b86cf153 100644 --- a/MobileGL/MG_Util/ShaderTranspiler/ShaderSourceProcessor.h +++ b/MobileGL/MG_Util/ShaderTranspiler/ShaderSourceProcessor.h @@ -20,6 +20,14 @@ namespace MobileGL { namespace MG_Util { namespace ShaderTranspiler { void PreprocessShaderSource(ShaderStage stage, String& source); + + // Rewrites a "#version 330 core" directive that PreprocessShaderSource normalized down + // from a legacy desktop version back up to "#version 460 core". Returns false (leaving + // the source untouched) for anything else: ES, compatibility, or an already-modern + // declaration. Exists so a shader that only parses under the laxer 460 rules - e.g. it + // uses 420-era syntax without the matching #extension line, which real drivers tend to + // accept - can be retried instead of failing to compile. + Bool RetargetLegacyVersionDirectiveTo460(String& source); } // namespace ShaderTranspiler } // namespace MG_Util } // namespace MobileGL \ No newline at end of file