From 05260d1262c0b965021963949237d80aee98c5b5 Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Mon, 20 Jul 2026 09:30:45 -0400 Subject: [PATCH] [Fix] (MG_Util/ShaderTranspiler): blank block comments lexically instead of erasing them - a '//*** banner ***' line opened a comment the old scanner never closed, so it deleted the rest of the shader, and a commented-out builtin definition renamed every genuine call to a name nothing defines --- MobileGL/MG_Test/Program/ProgramUtilTest.cpp | 71 +++++++++++++++ .../ShaderSourceProcessor.cpp | 89 ++++++++++++++++--- 2 files changed, 147 insertions(+), 13 deletions(-) diff --git a/MobileGL/MG_Test/Program/ProgramUtilTest.cpp b/MobileGL/MG_Test/Program/ProgramUtilTest.cpp index 6114fc24..e49184e9 100644 --- a/MobileGL/MG_Test/Program/ProgramUtilTest.cpp +++ b/MobileGL/MG_Test/Program/ProgramUtilTest.cpp @@ -455,6 +455,77 @@ void main() { } } +// A banner line like "//*** NOTE ***" contains "/*" at offset 1 and no "*/" anywhere after it. The +// old hand-rolled comment stripper searched for "/*" with no lexical state, found that, failed to +// find a terminator, and erased everything from there to the end of the file - deleting the entire +// shader. Banner comments in that exact shape are common in Iris and OptiFine packs. +TEST_F(ProgramUtilTest, PreprocessKeepsShaderBodyAfterAStarredLineComment) { + using namespace MG_Util::ShaderTranspiler; + + String source = R"(#version 330 core +//*** lighting pass *** +out vec4 fragColor; +void main() { + fragColor = vec4(1.0); +} +)"; + PreprocessShaderSource(ShaderStage::Fragment, source); + + EXPECT_NE(source.find("void main()"), String::npos) << "shader body was truncated:\n" << source; + EXPECT_NE(source.find("fragColor = vec4(1.0);"), String::npos); + + ShaderAttrib attrib{.shaderType = GL_FRAGMENT_SHADER, .sourceStr = source}; + auto res = ShaderCompiler::CompileShader(attrib); + if (!res) { + FAIL() << "errc: " << res.error().errc << "\nlog: " << res.error().log << "\nsource:\n" << source; + } +} + +// The builtin-shadowing rename only fires when the shader really defines its own round/tanh/etc. +// Deciding that from a commented-out definition renames every genuine call to the builtin to a +// mg_ name that nothing defines, which fails to link. +TEST_F(ProgramUtilTest, PreprocessIgnoresCommentedOutBuiltinShadowingDefinition) { + using namespace MG_Util::ShaderTranspiler; + + String source = R"(#version 330 core +// float round(float x) { return floor(x + 0.5); } +out vec4 fragColor; +void main() { + fragColor = vec4(round(1.25)); +} +)"; + PreprocessShaderSource(ShaderStage::Fragment, source); + + EXPECT_NE(source.find("round(1.25)"), String::npos) << "call was renamed from a comment:\n" << source; + EXPECT_EQ(source.find("mg_round"), String::npos); +} + +// A block-commented extension directive must not be treated as a real one - the int64 filter turns +// unsupported directives into #error, so reading one out of a comment manufactures a compile +// failure for a shader that never asked for the extension. +TEST_F(ProgramUtilTest, PreprocessIgnoresBlockCommentedExtensionDirectives) { + using namespace MG_Util::ShaderTranspiler; + + String source = R"(#version 330 core +/* +#extension GL_ARB_gpu_shader_int64 : require +*/ +out vec4 fragColor; +void main() { + fragColor = vec4(1.0); +} +)"; + PreprocessShaderSource(ShaderStage::Fragment, source); + + EXPECT_EQ(source.find("#error"), String::npos) << "#error synthesized from a comment:\n" << source; + + ShaderAttrib attrib{.shaderType = GL_FRAGMENT_SHADER, .sourceStr = source}; + auto res = ShaderCompiler::CompileShader(attrib); + if (!res) { + FAIL() << "errc: " << res.error().errc << "\nlog: " << res.error().log << "\nsource:\n" << source; + } +} + TEST_F(ProgramUtilTest, PreprocessModernSampleQualifierStaysAtVersion460) { using namespace MG_Util::ShaderTranspiler; diff --git a/MobileGL/MG_Util/ShaderTranspiler/ShaderSourceProcessor.cpp b/MobileGL/MG_Util/ShaderTranspiler/ShaderSourceProcessor.cpp index ba521a8e..f121ecf3 100644 --- a/MobileGL/MG_Util/ShaderTranspiler/ShaderSourceProcessor.cpp +++ b/MobileGL/MG_Util/ShaderTranspiler/ShaderSourceProcessor.cpp @@ -96,6 +96,77 @@ namespace { return masked; } + // Blank out block comments in place, leaving line comments and every other byte where it is. + // + // The passes that follow scan the source as raw text, so block comments have to stop being + // visible to them - but they must not be *deleted*: replacing the bytes with spaces keeps every + // later offset valid and keeps newlines, so glslang's diagnostics still point at the line the + // application wrote. It also has to be lexically aware. A banner line such as + // + // //*** lighting pass *** + // + // contains "/*" one byte in, and a naive search for that opener treats the rest of the file as + // an unterminated comment. + void BlankBlockComments(MobileGL::String& source) { + enum class Region { Code, SingleLineComment, MultiLineComment, QuotedText }; + + Region region = Region::Code; + char quote = '\0'; + bool escaped = false; + + for (SizeT pos = 0; pos < source.size(); pos++) { + const char ch = source[pos]; + const char next = pos + 1 < source.size() ? source[pos + 1] : '\0'; + + if (region == Region::Code) { + if (ch == '/' && next == '/') { + pos++; + region = Region::SingleLineComment; + } else if (ch == '/' && next == '*') { + source[pos] = ' '; + source[pos + 1] = ' '; + pos++; + region = Region::MultiLineComment; + } else if (ch == '"' || ch == '\'') { + quote = ch; + escaped = false; + region = Region::QuotedText; + } + continue; + } + + if (region == Region::SingleLineComment) { + if (ch == '\n' || ch == '\r') region = Region::Code; + continue; + } + + if (region == Region::MultiLineComment) { + if (ch == '*' && next == '/') { + source[pos] = ' '; + source[pos + 1] = ' '; + pos++; + region = Region::Code; + } else if (ch != '\n' && ch != '\r') { + source[pos] = ' '; + } + continue; + } + + // GLSL has no multi-line string literals, so a quote that reaches end of line was never + // a literal to begin with - most likely an apostrophe in a #error or #pragma message. + // Ending the region here keeps one stray apostrophe from swallowing the rest of the file. + if (ch == '\n' || ch == '\r') { + region = Region::Code; + } else if (escaped) { + escaped = false; + } else if (ch == '\\') { + escaped = true; + } else if (ch == quote) { + region = Region::Code; + } + } + } + struct CodeToken { String text; SizeT begin = 0; @@ -686,7 +757,10 @@ namespace { void RenameBuiltinShadowingFunction(MobileGL::String& source, const char* from, const char* to) { const MobileGL::String fromName = from; - if (!HasSingleLineFunctionDefinition(source, fromName)) { + // Decide from a comment-free view. A commented-out definition is not a definition, and + // acting on one renames every genuine call to the builtin to a name nothing defines - which + // then fails to resolve. Line comments survive BlankBlockComments, so this matters. + if (!HasSingleLineFunctionDefinition(MaskCommentsAndQuotedText(source), fromName)) { return; } @@ -1078,18 +1152,7 @@ namespace MobileGL { const ShaderLanguageInfo originalLanguage = InspectShaderLanguage(source); NormalizeVersionDirective(source, originalLanguage); - // remove multi-line comment - size_t commentStartPos = source.find("/*"); - while (commentStartPos != String::npos) { - size_t commentEndPos = source.find("*/", commentStartPos); - if (commentEndPos == String::npos) { - source.erase(commentStartPos); - break; - } - // + length of "*/" - source = source.replace(commentStartPos, commentEndPos - commentStartPos + 2, ""); - commentStartPos = source.find("/*", commentStartPos); - } + BlankBlockComments(source); // remove #line directives SizeT linedirPos = source.find("#line");