From 07669aacd4952464762986a909b4cc671a47583d Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Thu, 27 Aug 2026 03:01:27 -0400 Subject: [PATCH] [Fix] (ShaderTranspiler): fan #extension implications out to both new gates and bound the mid-line #version probe to its line --- MobileGL/MG_Test/Program/ProgramUtilTest.cpp | 141 +++++++++++++++++- .../ShaderSourceProcessor.cpp | 123 +++++++++++++-- 2 files changed, 248 insertions(+), 16 deletions(-) diff --git a/MobileGL/MG_Test/Program/ProgramUtilTest.cpp b/MobileGL/MG_Test/Program/ProgramUtilTest.cpp index 41fc355b..1498af42 100644 --- a/MobileGL/MG_Test/Program/ProgramUtilTest.cpp +++ b/MobileGL/MG_Test/Program/ProgramUtilTest.cpp @@ -4689,19 +4689,43 @@ TEST_F(ProgramUtilTest, NumSamplesShimHonoursTheVersionAndExtensionGate) { Bool expectShim; }; // Mirrors glslang's own gate (Initialize.cpp): desktop from 4.00, or from 1.30 with - // ARB_sample_shading; ESSL from 3.20, or from 3.10 with OES_sample_variables. + // ARB_sample_shading; ESSL from 3.20, or from 3.10 with OES_sample_variables - which + // GL_ANDROID_extension_pack_es31a and `#extension all : warn` also turn on + // (TParseVersions::updateExtensionBehavior). const Case cases[] = { {"desktop 460 core", "#version 460 core\n", true}, {"desktop 400 core", "#version 400 core\n", true}, {"desktop 330 core, no extension", "#version 330 core\n", false}, {"desktop 330 core + ARB_sample_shading", "#version 330 core\n#extension GL_ARB_sample_shading : require\n", true}, + {"desktop 330 core + all : warn", + "#version 330 core\n#extension all : warn\n", true}, {"desktop 120, no extension", "#version 120\n", false}, + {"desktop 120 + all : warn (below the 1.30 floor)", + "#version 120\n#extension all : warn\n", false}, {"ESSL 320", "#version 320 es\n", true}, {"ESSL 310, no extension", "#version 310 es\n", false}, {"ESSL 310 + OES_sample_variables", "#version 310 es\n#extension GL_OES_sample_variables : require\n", true}, + // The AEP spellings. glslang applies the directive's behavior to all twelve AEP members, + // GL_OES_sample_variables among them, so these are legal ES 3.1 shaders. + {"ESSL 310 + AEP : require", + "#version 310 es\n#extension GL_ANDROID_extension_pack_es31a : require\n", true}, + {"ESSL 310 + AEP : enable", + "#version 310 es\n#extension GL_ANDROID_extension_pack_es31a : enable\n", true}, + {"ESSL 310 + AEP : warn", + "#version 310 es\n#extension GL_ANDROID_extension_pack_es31a : warn\n", true}, + // ...but `disable` is not an opt-in, and the implication carries the behavior with it. + {"ESSL 310 + AEP : disable", + "#version 310 es\n#extension GL_ANDROID_extension_pack_es31a : disable\n", false}, + {"ESSL 310 + all : warn", + "#version 310 es\n#extension all : warn\n", true}, + // An AEP member that does NOT imply sample variables must not open the gate. + {"ESSL 310 + EXT_geometry_shader only", + "#version 310 es\n#extension GL_EXT_geometry_shader : require\n", false}, {"ESSL 300", "#version 300 es\n", false}, + {"ESSL 300 + AEP (below the 3.10 floor)", + "#version 300 es\n#extension GL_ANDROID_extension_pack_es31a : require\n", false}, }; for (const Case& testCase : cases) { @@ -4873,3 +4897,118 @@ TEST_F(ProgramUtilTest, OnlyAnExactVersionRepeatIsElided) { EXPECT_NE(source.find("#version 330 foobar"), String::npos) << source; } } + +// glslang applies an #extension directive's behavior to every extension the named one IMPLIES +// (TParseVersions::updateExtensionBehavior, Versions.cpp:1039-1064). Both consumers of the +// extension sets have to see that expansion or the AEP spelling of a shader behaves differently +// from the byte-equivalent one that names its members directly. +TEST_F(ProgramUtilTest, AepFansOutToItsMemberExtensionMacros) { + using namespace MG_Util::ShaderTranspiler; + + // The CTS-shaped guard, opted in the AEP way. Before the fan-out this took the broken arm. + String source = R"(#version 310 es +#extension GL_ANDROID_extension_pack_es31a : require +precision highp float; +out vec4 fragColor; +#if !GL_OES_sample_variables +this is broken +#endif +#if !GL_OES_shader_multisample_interpolation +this is also broken +#endif +void main() { fragColor = vec4(float(gl_NumSamples)); } +)"; + + PreprocessShaderSource(ShaderStage::Fragment, source); + // Both halves of the AEP path: the built-in shim AND the restored member macros. + EXPECT_TRUE(HasNumSamplesShim(source)) << source; + const String defines = CollectEsPreambleMacroDefines(source); + for (const char* member : {"GL_ANDROID_extension_pack_es31a", "GL_OES_sample_variables", + "GL_OES_shader_image_atomic", "GL_OES_shader_multisample_interpolation", + "GL_OES_texture_storage_multisample_2d_array", "GL_EXT_geometry_shader", + "GL_EXT_gpu_shader5", "GL_EXT_primitive_bounding_box", + "GL_EXT_shader_io_blocks", "GL_EXT_tessellation_shader", + "GL_EXT_texture_buffer", "GL_EXT_texture_cube_map_array"}) { + EXPECT_NE(defines.find(String("#define ") + member + " 1\n"), String::npos) + << member << " missing from:\n" << defines; + } + // GL_KHR_blend_equation_advanced is an AEP member glslang propagates to, but its macro is in + // the DESKTOP preamble too - so the rewrite never took it away and it must not be restored. + EXPECT_EQ(defines.find("GL_KHR_blend_equation_advanced"), String::npos) << defines; + + ExpectShaderCompiles(GL_FRAGMENT_SHADER, source); +} + +TEST_F(ProgramUtilTest, ExtensionImplicationIsTransitiveAndStaysNamed) { + using namespace MG_Util::ShaderTranspiler; + + { + SCOPED_TRACE("geometry/tessellation imply the matching io_blocks"); + // glslang re-enters updateExtensionBehavior for each implication, so the graph is walked + // to a fixed point rather than one level deep. + String source = R"(#version 310 es +#extension GL_OES_geometry_shader : require +out vec4 fragColor; +void main() { fragColor = vec4(1.0); } +)"; + PreprocessShaderSource(ShaderStage::Fragment, source); + const String defines = CollectEsPreambleMacroDefines(source); + EXPECT_NE(defines.find("#define GL_OES_geometry_shader 1\n"), String::npos) << defines; + EXPECT_NE(defines.find("#define GL_OES_shader_io_blocks 1\n"), String::npos) << defines; + // The EXT spelling is a different extension and must not come along. + EXPECT_EQ(defines.find("GL_EXT_shader_io_blocks"), String::npos) << defines; + } + + { + SCOPED_TRACE("a source that names nothing implied still gets nothing"); + String source = R"(#version 310 es +#extension GL_OES_sample_variables : enable +out vec4 fragColor; +void main() { fragColor = vec4(1.0); } +)"; + PreprocessShaderSource(ShaderStage::Fragment, source); + const String defines = CollectEsPreambleMacroDefines(source); + EXPECT_EQ(defines, String("#define GL_OES_sample_variables 1\n")) << defines; + } + + { + SCOPED_TRACE("`all` opens the built-in gate but does not define every ES macro"); + // The two questions differ: `all : warn` really does turn every extension on in glslang, + // but the preamble macros are defined before any #extension line runs, so `all` says + // nothing about which ones the ES -> desktop rewrite took away. + String source = R"(#version 310 es +#extension all : warn +out vec4 fragColor; +void main() { fragColor = vec4(float(gl_NumSamples)); } +)"; + PreprocessShaderSource(ShaderStage::Fragment, source); + EXPECT_TRUE(HasNumSamplesShim(source)) << source; + EXPECT_EQ(source.find("mobilegl-es-preamble"), String::npos) << source; + EXPECT_TRUE(CollectEsPreambleMacroDefines(source).empty()); + } +} + +// The mid-line #version probe must search THE LINE, not the rest of the file: an unbounded +// std::string::find makes InspectShaderLanguage quadratic on the ordinary resolved-shader-pack +// shape (one leading #version, no further '#' anywhere). This pins both halves - the detection +// still fires, and it fires on a source whose only other content is a long directive-free body. +TEST_F(ProgramUtilTest, MidLineVersionDetectionSurvivesALongDirectiveFreeBody) { + using namespace MG_Util::ShaderTranspiler; + + String body; + body.reserve(64 * 1024); + for (int line = 0; line < 2000; ++line) { + body += " float v" + std::to_string(line) + " = 0.0;\n"; + } + + // The CTS concatenation shape, followed by a body with no '#' in it at all. + String source = "#version 310 es\nprecision highp float;#version 310 es\nout vec4 fragColor;\nvoid main() {\n" + + body + " fragColor = vec4(1.0);\n}\n"; + const SizeT lineCountBefore = static_cast(std::count(source.begin(), source.end(), '\n')); + + PreprocessShaderSource(ShaderStage::Fragment, source); + + EXPECT_EQ(source.find("#version", source.find("#version") + 1), String::npos) << source.substr(0, 200); + EXPECT_EQ(static_cast(std::count(source.begin(), source.end(), '\n')), lineCountBefore); + ExpectShaderCompiles(GL_FRAGMENT_SHADER, source); +} diff --git a/MobileGL/MG_Util/ShaderTranspiler/ShaderSourceProcessor.cpp b/MobileGL/MG_Util/ShaderTranspiler/ShaderSourceProcessor.cpp index 4d60829a..9434690e 100644 --- a/MobileGL/MG_Util/ShaderTranspiler/ShaderSourceProcessor.cpp +++ b/MobileGL/MG_Util/ShaderTranspiler/ShaderSourceProcessor.cpp @@ -13,6 +13,7 @@ #include #include #include +#include #include #include #include @@ -255,6 +256,50 @@ namespace { bool isValid = false; }; + // glslang's #extension implication graph, transcribed from + // TParseVersions::updateExtensionBehavior (Versions.cpp:1039-1064). Naming one of these + // extensions applies the SAME behavior to every name it implies, so a source that says + // `#extension GL_ANDROID_extension_pack_es31a : require` has really required all twelve AEP + // members - and glslang's ES gl_NumSamples gate reads GL_OES_sample_variables, one of them. + // + // Transcribed rather than approximated: the AEP membership list is glslang's, and a guess that + // drifts from it would make MobileGL accept or reject a shader glslang does not. + // GL_KHR_blend_equation_advanced is in the list for completeness even though it has no ES + // preamble macro - IsEsOnlyPreambleExtensionMacro filters it out on its own. + const Vector>>& GetExtensionImplications() { + static const Vector>> kImplications = { + {"GL_ANDROID_extension_pack_es31a", + {"GL_KHR_blend_equation_advanced", "GL_OES_sample_variables", "GL_OES_shader_image_atomic", + "GL_OES_shader_multisample_interpolation", "GL_OES_texture_storage_multisample_2d_array", + "GL_EXT_geometry_shader", "GL_EXT_gpu_shader5", "GL_EXT_primitive_bounding_box", + "GL_EXT_shader_io_blocks", "GL_EXT_tessellation_shader", "GL_EXT_texture_buffer", + "GL_EXT_texture_cube_map_array"}}, + // geometry / tessellation to io_blocks + {"GL_EXT_geometry_shader", {"GL_EXT_shader_io_blocks"}}, + {"GL_OES_geometry_shader", {"GL_OES_shader_io_blocks"}}, + {"GL_EXT_tessellation_shader", {"GL_EXT_shader_io_blocks"}}, + {"GL_OES_tessellation_shader", {"GL_OES_shader_io_blocks"}}, + }; + return kImplications; + } + + // Closes `extensions` under the graph above. glslang propagates by RE-ENTERING + // updateExtensionBehavior, so the propagation is transitive (AEP -> GL_EXT_geometry_shader -> + // GL_EXT_shader_io_blocks); the fixed-point loop below is that re-entry. + void AddImpliedExtensions(std::set& extensions) { + if (extensions.empty()) return; + bool grew = true; + while (grew) { + grew = false; + for (const auto& [source, implied] : GetExtensionImplications()) { + if (extensions.count(source) == 0) continue; + for (const char* name : implied) { + grew |= extensions.insert(name).second; + } + } + } + } + // Reads " [profile]" out of a "#version" directive whose keyword ends at `probe`, and // decides whether it is one MobileGL is willing to rewrite. `code` must be the masked source, // so a trailing comment has already become blanks. @@ -337,12 +382,21 @@ namespace { // BlankRedundantVersionDirectives for why that one is tolerated and nothing else. // // Gated on a directive having been accepted already, so an ordinary shader - which - // has none of these - pays one memchr per line past its #version line and nothing - // before it. - if (info.hasValidVersionDirective) { - const SizeT hashPos = code.find('#', probe); - if (hashPos != MobileGL::String::npos && hashPos < lineEnd) { - recordIfRedundant(hashPos, lineEnd); + // has none of these - pays nothing at all before its #version line. + // + // BOUNDED TO THE LINE, and that is not a detail. std::string::find(char, pos) has + // no end bound, so a `code.find('#', probe)` filtered afterwards by + // `hashPos < lineEnd` scans from this line to the END OF THE SOURCE whenever no + // '#' follows - which is the ordinary shape of a resolved shader-pack source (one + // leading #version, nothing after it), and it makes this whole sweep quadratic in + // shader size. A 131 KB glsl-transformer output in .trace-work has exactly one '#' + // in the file. Searching the line span is behaviour-identical: every hashPos the + // unbounded form could accept already had to satisfy hashPos < lineEnd. + if (info.hasValidVersionDirective && probe < lineEnd) { + const void* hash = std::memchr(code.data() + probe, '#', lineEnd - probe); + if (hash != nullptr) { + recordIfRedundant(static_cast(static_cast(hash) - code.data()), + lineEnd); } } } else { @@ -390,6 +444,19 @@ namespace { lineStart = lineEnd + (hasLineBreak ? 1 : 0); } + // Both extension sets are closed under glslang's implication graph BEFORE anyone reads + // them, so every consumer sees the same expansion and none of them can forget it. Applied + // here rather than at the directive because an implication may be named before its source + // (`#extension GL_EXT_shader_io_blocks : disable` then `... AEP : require`), and the + // fixed point of the whole set is what glslang's re-entrant propagation ends up at. + // + // enablesGpuShader5 is deliberately NOT recomputed from the expanded set: it gates the + // 460 version escalation on the DESKTOP ARB/NV spellings, and AEP implies the ESSL + // GL_EXT_gpu_shader5, a different extension. An ES source is rewritten to 460 core + // anyway, so there is nothing for the escalation to do there. + AddImpliedExtensions(info.namedExtensions); + AddImpliedExtensions(info.enabledExtensions); + return info; } @@ -1653,21 +1720,47 @@ namespace { if (!info.hasValidVersionDirective) return; if (info.namedExtensions.empty()) return; - MobileGL::String macros; - for (const MobileGL::String& extension : info.namedExtensions) { - if (!IsEsOnlyPreambleExtensionMacro(extension, info.version)) continue; - macros += extension; - } + // namedExtensions is already closed under glslang's implication graph, so a source that + // names only GL_ANDROID_extension_pack_es31a marks its twelve members too - glslang's ES + // preamble defines all of them, and the CTS-shaped "#if !GL_OES_sample_variables" guard + // reads one of them. + // + // `#extension all : warn` is deliberately NOT honoured here, unlike in the built-in gate. + // The two answer different questions: the gate asks "would glslang have this extension + // turned on", where `all` genuinely says yes, while this asks "which preamble macros did + // the ES -> desktop rewrite take away". glslang's preamble runs BEFORE any #extension line + // and defines the ES macros regardless of behavior, so `all` adds no information - and + // emitting all thirty-five for a source that named nothing is exactly the broad rewrite + // the named-extensions-only policy exists to avoid. + const bool hasMacroToRestore = + std::any_of(info.namedExtensions.begin(), info.namedExtensions.end(), + [&info](const MobileGL::String& extension) { + return IsEsOnlyPreambleExtensionMacro(extension, info.version); + }); // Nothing the desktop preamble is missing: leave the source byte-identical. - if (macros.empty()) return; + if (!hasMacroToRestore) return; source.insert(afterVersion.Get(source), MobileGL::String(kEsPreambleMarkerPrefix) + std::to_string(info.version) + "*/\n"); } + // "Would glslang have this extension turned on?", mirroring TParseVersions::extensionTurnedOn. + // + // Two spellings besides the name itself reach it. The implication graph is already folded into + // enabledExtensions (AddImpliedExtensions), so only `#extension all : ` is left: + // glslang applies that behavior to EVERY registered extension at once, and rejects `all` with + // require/enable outright (Versions.cpp:1136-1141) - so the only spellings that survive are + // `all : warn`, which turns everything ON (behavior != EBhDisable), and `all : disable`. + // InspectShaderLanguage only records a name in enabledExtensions for enable/require/warn, so + // the literal "all" appearing here means `all : warn` and nothing else. + bool ExtensionTurnedOn(const ShaderLanguageInfo& info, const char* extension) { + return info.enabledExtensions.count(extension) != 0 || info.enabledExtensions.count("all") != 0; + } + // gl_NumSamples is legal in this source only where glslang would have declared it with a // non-SPIR-V target (Initialize.cpp): desktop from 4.00 core, or from 1.30 with - // ARB_sample_shading; ESSL from 3.20, or from 3.10 with OES_sample_variables. + // ARB_sample_shading; ESSL from 3.20, or from 3.10 with OES_sample_variables - the last of + // which GL_ANDROID_extension_pack_es31a also turns on, via the implication graph. // // The gate matters because the shim ends in "#define gl_NumSamples mg_NumSamples", and a // #define is not scoped by anything: defining it for a source where the built-in does not @@ -1676,10 +1769,10 @@ namespace { if (!info.HasVersionDirective() || !info.hasValidVersionDirective) return false; if (info.profile == MobileGL::ShaderProfile::ES) { if (info.version >= 320) return true; - return info.version >= 310 && info.enabledExtensions.count("GL_OES_sample_variables") != 0; + return info.version >= 310 && ExtensionTurnedOn(info, "GL_OES_sample_variables"); } if (info.version >= 400) return true; - return info.version >= 130 && info.enabledExtensions.count("GL_ARB_sample_shading") != 0; + return info.version >= 130 && ExtensionTurnedOn(info, "GL_ARB_sample_shading"); } // gl_NumSamples has no SPIR-V built-in to lower to, so glslang declares it only when it is NOT