From 3049c4b82b19131b41095a286092d3c18dbea620 Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Tue, 21 Jul 2026 05:27:01 -0400 Subject: [PATCH] [Fix] (ShaderTranspiler): stop blanking block comments in the source handed to glslang - BlankBlockComments replaced comment chars with spaces but preserved interior newlines, so a block comment spanning a newline inside a #define truncated the macro body (VALUE became empty) - glslang has a conformant preprocessor and collapses a block comment to one space across newlines, so the delivered source now keeps comments intact and lets glslang handle them - Fixes KHR-GL3x.shaders.preprocessor multiline_comment_define / redefine_object_multiline_comment / function_redefinition_3 (6 cases, both devices) - FilterUnsupportedGpuShaderInt64 relied on the blanking to skip commented-out #extension lines; it now masks comments locally (MaskCommentsAndQuotedText) like the sibling passes, collecting edits and applying them back-to-front - conditional_inclusion.basic_2 (defined() via macro expansion) stays failing by design: glslang rejects it as UB and working around it would mean re-running preprocessing MobileGL defers to glslang --- MobileGL/MG_Test/Program/ProgramUtilTest.cpp | 71 +++++++++++++++++++ .../ShaderSourceProcessor.cpp | 46 ++++++++---- 2 files changed, 103 insertions(+), 14 deletions(-) diff --git a/MobileGL/MG_Test/Program/ProgramUtilTest.cpp b/MobileGL/MG_Test/Program/ProgramUtilTest.cpp index 75f4405f..fcaf6174 100644 --- a/MobileGL/MG_Test/Program/ProgramUtilTest.cpp +++ b/MobileGL/MG_Test/Program/ProgramUtilTest.cpp @@ -238,6 +238,77 @@ void main() { } } +// KHR-GL33.shaders.preprocessor.* — a block comment is one preprocessing token that the C/GLSL +// preprocessor replaces with a single space, even when it spans newlines inside a directive. glslang +// handles this natively, so MobileGL must not mangle it. These reproduce the CTS cases that failed +// because comment blanking preserved the interior newline, truncating multi-line #define bodies. +static void ExpectCompiles(MobileGL::ShaderStage stage, GLenum glStage, MobileGL::String source) { + using namespace MG_Util::ShaderTranspiler; + PreprocessShaderSource(stage, source); + ShaderAttrib attrib{.shaderType = glStage, .sourceStr = source}; + auto res = ShaderCompiler::CompileShader(attrib); + if (!res) { + FAIL() << "errc: " << res.error().errc << "\nlog: " << res.error().log << "\nsource:\n" << source; + } +} + +TEST_F(ProgramUtilTest, PreprocessMultilineCommentInDefineBodyCompiles) { + ExpectCompiles(ShaderStage::Fragment, GL_FRAGMENT_SHADER, + R"(#version 330 +precision mediump float; +out float out0; +#define VALUE /* current + value */ 4.2 + +void main() +{ + out0 = VALUE; +})"); +} + +TEST_F(ProgramUtilTest, PreprocessRedefineObjectMultilineCommentCompiles) { + ExpectCompiles(ShaderStage::Fragment, GL_FRAGMENT_SHADER, + R"(#version 330 +precision mediump float; +out float out0; +# define VAL1 1.0 +#define VAL2 2.0 + +#define RES2 /* fdsjklfdsjkl + dsfjkhfdsjkh + fdsjklhfdsjkh */ (RES1 * VAL2) +#define RES1 (VAL2 / VAL1) +#define RES2 /* ewrlkjhsadf */ (RES1 * VAL2) +#define VALUE (RES2 + RES1) + +void main() +{ + out0 = VALUE; +})"); +} + +TEST_F(ProgramUtilTest, PreprocessFunctionMacroRedefinitionMultilineCommentCompiles) { + ExpectCompiles(ShaderStage::Fragment, GL_FRAGMENT_SHADER, + R"(#version 330 +precision mediump float; +out float out0; +# define FUNC(a,b) (a +b) +# define FUNC(a,b)(a /* comment + */ +b) + +void main() +{ + out0 = FUNC(1.0, 2.0); +})"); +} + +// Note: KHR-GL3x.shaders.preprocessor.conditional_inclusion.basic_2 (`#define AAA defined(BBB)` used +// in `#if !AAA`) is intentionally NOT handled here. Generating the `defined` operator via macro +// expansion is undefined per the C/GLSL preprocessor spec, and glslang deliberately rejects it +// ("'defined' : cannot use in preprocessor expression when expanded from macros"). Making it pass +// would require MobileGL to run its own macro expansion ahead of glslang, which is exactly the +// preprocessing we defer to glslang; the two cases stay failing by design. + TEST_F(ProgramUtilTest, PreprocessLegacyFragmentShaderModernizesGlmarkStyleSource) { using namespace MG_Util::ShaderTranspiler; diff --git a/MobileGL/MG_Util/ShaderTranspiler/ShaderSourceProcessor.cpp b/MobileGL/MG_Util/ShaderTranspiler/ShaderSourceProcessor.cpp index d874e6b8..56c07f8c 100644 --- a/MobileGL/MG_Util/ShaderTranspiler/ShaderSourceProcessor.cpp +++ b/MobileGL/MG_Util/ShaderTranspiler/ShaderSourceProcessor.cpp @@ -956,15 +956,28 @@ namespace { return; } + // Detect the directive on a comment/string-masked copy so a commented-out + // "#extension GL_ARB_gpu_shader_int64" is never turned into a synthesized #error. Comments are + // no longer blanked in the delivered source (glslang handles them), so this pass must mask + // locally like its siblings. Masking preserves offsets, so edits collected against the scan + // apply verbatim to `source`; they are applied back-to-front to keep earlier offsets valid. + const MobileGL::String scan = MaskCommentsAndQuotedText(source); + struct DirectiveEdit { + SizeT pos; + SizeT len; + MobileGL::String replacement; + }; + Vector edits; + SizeT lineStart = 0; - while (lineStart < source.size()) { - SizeT lineEnd = source.find('\n', lineStart); + while (lineStart < scan.size()) { + SizeT lineEnd = scan.find('\n', lineStart); const bool hasLineBreak = lineEnd != MobileGL::String::npos; if (!hasLineBreak) { - lineEnd = source.size(); + lineEnd = scan.size(); } - const MobileGL::String line = source.substr(lineStart, lineEnd - lineStart); + const MobileGL::String line = scan.substr(lineStart, lineEnd - lineStart); SizeT probe = 0; while (probe < line.size() && std::isspace(static_cast(line[probe]))) { probe++; @@ -1006,16 +1019,12 @@ namespace { const MobileGL::String behavior = TrimDirectiveToken(line.substr(probe)); const SizeT replaceLen = lineEnd - lineStart + (hasLineBreak ? 1 : 0); if (behavior == "require") { - const MobileGL::String replacement = - "#error GL_ARB_gpu_shader_int64 is not advertised by MobileGL\n"; - source.replace(lineStart, replaceLen, replacement); - lineStart += replacement.size(); + edits.push_back({lineStart, replaceLen, + "#error GL_ARB_gpu_shader_int64 is not advertised by MobileGL\n"}); } else if (behavior == "enable" || behavior == "warn") { - source.replace(lineStart, replaceLen, "\n"); - lineStart++; - } else { - lineStart = lineEnd + (hasLineBreak ? 1 : 0); + edits.push_back({lineStart, replaceLen, "\n"}); } + lineStart = lineEnd + (hasLineBreak ? 1 : 0); continue; } } @@ -1025,6 +1034,10 @@ namespace { lineStart = lineEnd + (hasLineBreak ? 1 : 0); } + for (auto it = edits.rbegin(); it != edits.rend(); ++it) { + source.replace(it->pos, it->len, it->replacement); + } + ReplaceIdentifier(source, "GL_ARB_gpu_shader_int64", "MG_DISABLED_GL_ARB_gpu_shader_int64"); } @@ -1247,8 +1260,13 @@ namespace MobileGL { const ShaderLanguageInfo originalLanguage = InspectShaderLanguage(source); NormalizeVersionDirective(source, originalLanguage); - BlankBlockComments(source); - + // 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 + // directive, so blanking it here (which preserved the interior newlines) truncated + // multi-line #define bodies and broke otherwise-valid shaders (KHR-GL3x.shaders. + // 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); // noperspective is intentionally NOT touched here. It is core in desktop GLSL (1.30+)