From 2406e2d219dd0e5607a745c0add73325ff6c61a3 Mon Sep 17 00:00:00 2001 From: BZLZHH Date: Sat, 8 Aug 2026 03:43:56 -0400 Subject: [PATCH] [Perf, Fix] (MG_Util): preprocessing cleanups - dead scanners, quote-mask bug, one version inspection per compile Three scoped changes to ShaderSourceProcessor, none altering any transform's output (pinned by a byte-stability test across the legacy-shader anchor path): - Delete BlankBlockComments and RemoveDefineForIdentifier - dead since their callers left; the former's newline-terminated quote handling moves into MaskCommentsAndQuotedText (below) together with its rationale comment. - Fix MaskCommentsAndQuotedText treating a quote as running past end-of-line. GLSL has no multi-line literals, but a stray apostrophe in a directive or comment tail ("#pragma message can't") blanked the REST OF THE FILE for every masked consumer - the tokenizer, the version inspection, and the P0a explicit-location/binding extractors silently lost everything after it. - Inspect the shader language once per PreprocessShaderSource run instead of up to five times: NormalizeVersionDirective now takes the already-computed ShaderLanguageInfo, and the two after-version injections share one AfterVersionAnchor instead of re-running a full masked sweep each (FindAfterVersionDirective -> InspectShaderLanguage) to find the same spot. Compile-phase timings hold (BSL 1.848s, complementary-reimagined ~5.7s); retraces and the 435-test unit suite unchanged. --- MobileGL/MG_Test/Program/ProgramUtilTest.cpp | 154 +++++++++++ .../ShaderSourceProcessor.cpp | 244 ++++++++---------- 2 files changed, 263 insertions(+), 135 deletions(-) diff --git a/MobileGL/MG_Test/Program/ProgramUtilTest.cpp b/MobileGL/MG_Test/Program/ProgramUtilTest.cpp index 280b95cd..700ee23a 100644 --- a/MobileGL/MG_Test/Program/ProgramUtilTest.cpp +++ b/MobileGL/MG_Test/Program/ProgramUtilTest.cpp @@ -2221,3 +2221,157 @@ void main() { EXPECT_NE(source.find("mg_min3(0.1, 0.2, 0.3)"), String::npos) << source; EXPECT_EQ(source.find("float min3("), String::npos) << source; } + +// GLSL has no multi-line string or character literal, so a lone apostrophe never opens one - it is +// an English contraction, in a comment or in a diagnostic directive. MaskCommentsAndQuotedText used +// to disagree: it entered its quoted-text region on the apostrophe and, having no end-of-line rule, +// stayed there to the end of the file, blanking everything after it for every consumer of the mask +// (the tokenizer, the #version inspection, the explicit-location and opaque-binding extractors). +// +// Apostrophes inside comments were never affected - the comment region claims them first - but that +// is exactly the property the fix must not break, so pin it. +TEST_F(ProgramUtilTest, PreprocessKeepsApostrophesInsideCommentsHarmless) { + using namespace MG_Util::ShaderTranspiler; + + String source = R"(#version 460 core +// don't do this: the sampler isn't bound before the first frame +/* and here's a block comment whose apostrophes shouldn't matter either */ +uniform sampler2D tex; +in vec2 uv; +out vec4 fragColor; + +void main() { + fragColor = texture(tex, uv); +} +)"; + PreprocessShaderSource(ShaderStage::Fragment, source); + + EXPECT_NE(source.find("void main()"), String::npos) << "shader body was blanked:\n" << source; + EXPECT_NE(source.find("fragColor = texture(tex, uv);"), String::npos) << 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; + } +} + +// The case the old masker actually broke: an apostrophe in real (non-comment) text. Everything after +// it looked like string interior, so ExtractExplicitUniformLocations tokenized a blank source and +// handed the GL location assigner an empty map - the uniform silently lost its explicit location. +TEST_F(ProgramUtilTest, PreprocessApostropheInDirectiveKeepsLaterCodeVisibleToExtractors) { + using namespace MG_Util::ShaderTranspiler; + + String source = R"(#version 460 core +#pragma MG_NOTE(this pack can't run without explicit locations) +layout(location = 7) uniform vec4 tint; +in vec2 uv; +out vec4 fragColor; + +void main() { + fragColor = tint * uv.x; +} +)"; + PreprocessShaderSource(ShaderStage::Fragment, source); + + const UnorderedMap locations = ExtractExplicitUniformLocations(source); + ASSERT_EQ(locations.count("tint"), 1u) << "extractor went blind past the apostrophe:\n" << source; + EXPECT_EQ(locations.at("tint"), 7); + + 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; + } +} + +// PreprocessShaderSource used to rediscover "where does the #version directive end?" once per +// injection - up to five whole-source masks and line scans per compile for one offset. It now takes +// the anchor once, from the pass that creates it, and tracks it. +// +// These three sources drive every consumer of that anchor: NormalizeLineDirectives (both the +// keep branch and the drop-ahead-of-#version branch), ModernizeLegacyGLSL's gl_FragColor +// injection, and InjectDepthRangeBuiltinShim's. The expected texts are the byte-exact output of +// the pre-memo implementation, captured from it - the change is pure memoization and is allowed to +// move no byte at all. +// +// Case B and case C are the two ways the anchor moves out from under the memo, and are why it is +// tracked rather than simply cached: B deletes a #line that precedes the version directive, and C +// has ModernizeLegacyGLSL's raw ReplaceIdentifier rewrite "varying"/"texture2D" inside a comment +// banner ahead of it, pulling the anchor six bytes left. An offset cached blindly would put the +// injected declaration six bytes inside the version line. +TEST_F(ProgramUtilTest, PreprocessLegacyFragmentShaderOutputIsByteStableAcrossTheVersionAnchor) { + using namespace MG_Util::ShaderTranspiler; + + const char* kLegacyBody = R"(#line 30 +varying vec2 uv; +uniform sampler2D tex; + +void main() { + float d = gl_DepthRange.diff; + gl_FragColor = texture2D(tex, uv) * d; +} +)"; + const char* kExpectedBody = + "#version 330 core /*mobilegl-normalized-legacy*/\n" + "struct mg_DepthRangeParameters { float near; float far; float diff; };\n" + "const mg_DepthRangeParameters mg_DepthRange = mg_DepthRangeParameters(0.0, 1.0, 1.0);\n" + "#define gl_DepthRange mg_DepthRange\n" + "out vec4 mg_FragColor;\n" + "#line 30\n" + "in vec2 uv;\n" + "uniform sampler2D tex;\n" + "\n" + "void main() {\n" + " float d = gl_DepthRange.diff;\n" + " mg_FragColor = texture(tex, uv) * d;\n" + "}\n"; + + { + SCOPED_TRACE("A: version directive at offset 0"); + String source = String("#version 120\n") + kLegacyBody; + PreprocessShaderSource(ShaderStage::Fragment, source); + EXPECT_EQ(source, String(kExpectedBody)); + + 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; + } + } + + { + SCOPED_TRACE("B: a #line ahead of the version directive is dropped, shortening the prefix"); + String source = String("// pack preamble\n#line 1 \"world.fsh\"\n#version 120\n") + kLegacyBody; + PreprocessShaderSource(ShaderStage::Fragment, source); + // The dropped directive leaves its newline behind, so line numbering is untouched. + EXPECT_EQ(source, String("// pack preamble\n\n") + kExpectedBody); + } + + { + SCOPED_TRACE("C: a comment banner ahead of the version directive is itself rewritten"); + String source = R"(/* legacy varying / texture2D helpers */ +#version 120 +varying vec2 uv; +uniform sampler2D tex; +void main() { + gl_FragColor = texture2D(tex, uv); +} +)"; + PreprocessShaderSource(ShaderStage::Fragment, source); + EXPECT_EQ(source, String("/* legacy in / texture helpers */\n" + "#version 330 core /*mobilegl-normalized-legacy*/\n" + "out vec4 mg_FragColor;\n" + "in vec2 uv;\n" + "uniform sampler2D tex;\n" + "void main() {\n" + " mg_FragColor = texture(tex, uv);\n" + "}\n")); + + 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; + } + } +} diff --git a/MobileGL/MG_Util/ShaderTranspiler/ShaderSourceProcessor.cpp b/MobileGL/MG_Util/ShaderTranspiler/ShaderSourceProcessor.cpp index d47066cc..8d96bd98 100644 --- a/MobileGL/MG_Util/ShaderTranspiler/ShaderSourceProcessor.cpp +++ b/MobileGL/MG_Util/ShaderTranspiler/ShaderSourceProcessor.cpp @@ -32,6 +32,20 @@ namespace { return (ch >= 'A' && ch <= 'Z') || (ch >= 'a' && ch <= 'z') || ch == '_'; } + // Return a copy of `source` with every comment and string-literal interior blanked to spaces. + // + // The passes that follow answer lexical questions ("is this identifier real code?", "where does + // the #version line end?"), so comment and literal text has to stop being visible to them - but + // it must not be *deleted*: replacing the bytes with spaces keeps every offset 1:1 with the + // original, so an edit collected against the mask applies verbatim to the source, and keeping + // newlines means glslang's diagnostics still point at the line the application wrote. + // + // It also has to be lexically stateful. 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. MobileGL::String MaskCommentsAndQuotedText(const MobileGL::String& source) { enum class Region { Code, SingleLineComment, MultiLineComment, QuotedText }; @@ -85,9 +99,17 @@ namespace { continue; } - if (ch != '\n' && ch != '\r') { - masked[pos] = ' '; + // 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 + // for every consumer of this mask: the tokenizer, the #version inspection, and the + // explicit-location / opaque-binding extractors all go blind past that point otherwise. + if (ch == '\n' || ch == '\r') { + region = Region::Code; + continue; } + + masked[pos] = ' '; if (escaped) { escaped = false; } else if (ch == '\\') { @@ -100,77 +122,6 @@ 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; @@ -729,7 +680,17 @@ namespace { : "#version 460 core\n"; } - void NormalizeVersionDirective(MobileGL::String& source, const ShaderLanguageInfo& info) { + // Rewrites the #version directive and returns the offset just past it in the rewritten source - + // the anchor every later injection inserts at. + // + // The offset is returned rather than rediscovered because this function is the only place that + // knows it for free; recovering it costs a whole-source mask plus a line scan + // (FindAfterVersionDirective -> InspectShaderLanguage). Each branch below leaves the bytes + // ahead of the directive untouched apart from the BOM erase, and each replacement text is + // exactly one newline-terminated line, so the arithmetic is exact in all three cases. + SizeT NormalizeVersionDirective(MobileGL::String& source, const ShaderLanguageInfo& info) { + const SizeT bomBytes = info.hasUtf8Bom ? 3 : 0; + // 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 @@ -738,7 +699,8 @@ namespace { if (info.hasUtf8Bom) { source.erase(0, 3); } - return; + // The directive keeps its text and only slides left by the erased BOM. + return info.versionDirectiveEnd - bomBytes; } const MobileGL::String replacement = GetNormalizedVersionDirective(info); @@ -748,13 +710,16 @@ namespace { if (info.hasUtf8Bom) { source.erase(0, 3); } - return; + // Only whitespace can precede the directive on its own line, so the replacement occupies + // the whole rest of that line and ends it. + return info.versionDirectiveStart - bomBytes + replacement.size(); } if (info.hasUtf8Bom) { source.erase(0, 3); } source.insert(0, replacement); + return replacement.size(); } // Start of the physical line containing `offset`, never scanning before `lowerBound`. @@ -969,65 +934,64 @@ namespace { } } - void RemoveDefineForIdentifier(MobileGL::String& source, const MobileGL::String& identifier) { - SizeT lineStart = 0; - while (lineStart < source.size()) { - SizeT lineEnd = source.find('\n', lineStart); - const bool hasLineBreak = lineEnd != MobileGL::String::npos; - if (!hasLineBreak) { - lineEnd = source.size(); - } - - SizeT probe = lineStart; - while (probe < lineEnd && std::isspace(static_cast(source[probe]))) { - probe++; - } - if (probe < lineEnd && source[probe] == '#') { - probe++; - while (probe < lineEnd && std::isspace(static_cast(source[probe]))) { - probe++; - } - - constexpr const char* defineToken = "define"; - constexpr SizeT defineLen = 6; - const bool hasDefine = probe + defineLen <= lineEnd && - source.compare(probe, defineLen, defineToken) == 0 && - (probe + defineLen == lineEnd || - !IsIdentifierChar(source[probe + defineLen])); - if (hasDefine) { - probe += defineLen; - while (probe < lineEnd && std::isspace(static_cast(source[probe]))) { - probe++; - } - - const bool hasIdentifier = probe + identifier.size() <= lineEnd && - source.compare(probe, identifier.size(), identifier) == 0 && - (probe + identifier.size() == lineEnd || - !IsIdentifierChar(source[probe + identifier.size()])); - if (hasIdentifier) { - source.erase(lineStart, lineEnd - lineStart + (hasLineBreak ? 1 : 0)); - continue; - } - } - } - - lineStart = lineEnd + (hasLineBreak ? 1 : 0); - } - } - SizeT FindAfterVersionDirective(const MobileGL::String& source) { const ShaderLanguageInfo info = InspectShaderLanguage(source); return info.HasVersionDirective() ? info.versionDirectiveEnd : 0; } + // Holds the offset just past the #version directive - the anchor every injected declaration is + // inserted at - across the passes of one PreprocessShaderSource call. + // + // Four consumers want that one number, and each used to buy it with its own + // FindAfterVersionDirective, i.e. its own whole-source mask plus line scan. Taking it once and + // handing it down turns up to five InspectShaderLanguage sweeps per compile into one. + // + // It stays EXACT rather than merely cached. The memo is handed out only while the bytes ahead + // of the anchor are byte-for-byte what they were when it was taken, and that is precisely the + // condition under which a fresh FindAfterVersionDirective returns the same answer: the whole + // version line, and every line the scan looks at before reaching it, lies inside that prefix, + // so an unchanged prefix means the same directive is still found ending at the same offset. + // The guard is load-bearing, not decoration - passes really do rewrite ahead of the anchor. + // NormalizeLineDirectives deletes #line directives that precede the version line, and + // ModernizeLegacyGLSL's ReplaceIdentifier is raw text and so rewrites inside a leading comment + // banner. When the guard trips the offset is simply recomputed, which is the pre-memo behavior. + // + // The one-argument constructor is that pre-memo behavior in full, for any caller that has a + // source but no anchor to hand. + class AfterVersionAnchor { + public: + explicit AfterVersionAnchor(const MobileGL::String& source) { Recompute(source); } + AfterVersionAnchor(const MobileGL::String& source, SizeT offset) { Adopt(source, offset); } + + SizeT Get(const MobileGL::String& source) { + if (source.size() < m_offset || source.compare(0, m_offset, m_prefix) != 0) { + Recompute(source); + } + return m_offset; + } + + private: + void Recompute(const MobileGL::String& source) { Adopt(source, FindAfterVersionDirective(source)); } + + void Adopt(const MobileGL::String& source, SizeT offset) { + m_offset = offset; + m_prefix.assign(source, 0, offset); + } + + SizeT m_offset = 0; + MobileGL::String m_prefix; + }; + // GLSL's #line takes integer expressions only, but plenty of shader-pack preprocessors emit the // C form with a quoted filename. Deleting every #line outright made those harmless - at the cost // of __LINE__ reporting the position in MobileGL's rewritten text rather than the one the pack // author wrote, and of every later diagnostic pointing at the wrong line. Dropping just the // quoted operand keeps the directive doing its job and still hands glslang something it accepts. - void NormalizeLineDirectives(MobileGL::String& source) { + // + // `versionEnd` is the after-version anchor for the current `source` (AfterVersionAnchor::Get); + // this pass only reads the source ahead of its own rewrites, so the plain offset is enough. + void NormalizeLineDirectives(MobileGL::String& source, SizeT versionEnd) { const MobileGL::String masked = MaskCommentsAndQuotedText(source); - const SizeT versionEnd = FindAfterVersionDirective(source); MobileGL::String result; result.reserve(source.size()); @@ -1244,7 +1208,12 @@ namespace { } } - void ModernizeLegacyGLSL(MobileGL::ShaderStage stage, MobileGL::String& source) { + // `afterVersion` tracks the anchor the two injections below insert at. It is passed as the + // tracker rather than a bare offset because this pass rewrites identifiers first, and those + // rewrites are raw text: a leading comment banner mentioning `varying` or `texture2D` moves the + // anchor, and the tracker notices. + void ModernizeLegacyGLSL(MobileGL::ShaderStage stage, MobileGL::String& source, + AfterVersionAnchor& afterVersion) { // Precision qualifiers (highp/mediump/lowp and default-precision statements) are legal and // ignored in the normalized desktop core profiles, so glslang handles them natively. @@ -1265,16 +1234,17 @@ namespace { const bool usesFragData = source.find("gl_FragData") != MobileGL::String::npos; if (usesFragColor) { ReplaceIdentifier(source, "gl_FragColor", "mg_FragColor"); - source.insert(FindAfterVersionDirective(source), "out vec4 mg_FragColor;\n"); + source.insert(afterVersion.Get(source), "out vec4 mg_FragColor;\n"); } if (usesFragData) { ReplaceIdentifier(source, "gl_FragData", "mg_FragData"); - source.insert(FindAfterVersionDirective(source), "layout(location = 0) out vec4 mg_FragData[8];\n"); + source.insert(afterVersion.Get(source), "layout(location = 0) out vec4 mg_FragData[8];\n"); } } } - void InjectDepthRangeBuiltinShim(MobileGL::ShaderStage stage, MobileGL::String& source) { + void InjectDepthRangeBuiltinShim(MobileGL::ShaderStage stage, MobileGL::String& source, + AfterVersionAnchor& afterVersion) { if (stage != MobileGL::ShaderStage::Fragment) return; if (source.find("gl_DepthRange") == MobileGL::String::npos) return; if (source.find("mg_DepthRangeParameters") != MobileGL::String::npos) return; @@ -1283,7 +1253,7 @@ namespace { "struct mg_DepthRangeParameters { float near; float far; float diff; };\n" "const mg_DepthRangeParameters mg_DepthRange = mg_DepthRangeParameters(0.0, 1.0, 1.0);\n" "#define gl_DepthRange mg_DepthRange\n"; - source.insert(FindAfterVersionDirective(source), shim); + source.insert(afterVersion.Get(source), shim); } } // namespace @@ -1399,10 +1369,14 @@ namespace MobileGL { } // namespace void PreprocessShaderSource(ShaderStage stage, String& source) { - // Normalize while the inspector's source span still refers to the untouched input. Later passes - // remove comments and directives, so any subsequent insertion re-inspects the current source. + // Normalize while the inspector's source span still refers to the untouched input. const ShaderLanguageInfo originalLanguage = InspectShaderLanguage(source); - NormalizeVersionDirective(source, originalLanguage); + + // Four passes below inject just past the #version directive, and each of them used + // to locate that anchor for itself - a whole-source mask plus line scan apiece, up + // to five per compile for one offset. NormalizeVersionDirective hands back the + // anchor it just created and the tracker keeps it honest from there. + AfterVersionAnchor afterVersion(source, NormalizeVersionDirective(source, originalLanguage)); // Comments are left intact for glslang's own preprocessor: a block comment is a single // preprocessing token that collapses to one space even across newlines and inside a @@ -1411,7 +1385,7 @@ namespace MobileGL { // preprocessor multiline_comment_define / redefine_object / function_redefinition). // Every MobileGL pass that must ignore comment/string text already masks them locally // via MaskCommentsAndQuotedText/TokenizeCode, so the source we hand glslang keeps them. - NormalizeLineDirectives(source); + NormalizeLineDirectives(source, afterVersion.Get(source)); // 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 @@ -1426,8 +1400,8 @@ namespace MobileGL { RenameBuiltinShadowingFunctions(source); - ModernizeLegacyGLSL(stage, source); - InjectDepthRangeBuiltinShim(stage, source); + ModernizeLegacyGLSL(stage, source, afterVersion); + InjectDepthRangeBuiltinShim(stage, source, afterVersion); ApplyShaderSourceQuirks(stage, source); }