From 7de7cfc6ebd83be2cf9dd9bfea339d8332260596 Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Fri, 21 Aug 2026 12:09:03 -0400 Subject: [PATCH] [Fix, Test] (ShaderTranspiler): read a storage block's macro-spelled binding as declared, not as absent --- MobileGL/MG_Test/Program/ProgramUtilTest.cpp | 62 ++++++++++++++++ .../ShaderSourceProcessor.cpp | 72 ++++++++++++++++--- 2 files changed, 126 insertions(+), 8 deletions(-) diff --git a/MobileGL/MG_Test/Program/ProgramUtilTest.cpp b/MobileGL/MG_Test/Program/ProgramUtilTest.cpp index 05c4bff8..f25b45fc 100644 --- a/MobileGL/MG_Test/Program/ProgramUtilTest.cpp +++ b/MobileGL/MG_Test/Program/ProgramUtilTest.cpp @@ -4678,3 +4678,65 @@ void main() { clearInstance.b = 0u; } EXPECT_EQ(unqualified.count("Clear"), 1u) << "the unambiguous block alongside it is still recognised"; } + +// THE TEXT THIS SCANS IS NOT MACRO-EXPANDED. MobileGL's preprocessing rewrites the source, it +// does not run the C preprocessor, so a `#define` and every use of it both survive into what +// RunSourceOnlyPipeline hands here. `binding = SOME_MACRO` is therefore the ordinary spelling in +// real shader packs - Flywheel's indirect engine writes every one of its storage blocks that way +// - and reading "no integer literal" as "no binding" defaulted all of them onto binding 0 at +// once, where they aliased and the whole engine drew nothing +// (minecraft-1.21.1-neoforge-create-indirect-in-world, both backends). +TEST_F(ProgramUtilTest, ExtractStorageBlocksWithoutExplicitBindingKeepsAMacroSpelledBinding) { + using namespace MG_Util::ShaderTranspiler; + + // Flywheel's own shape, verbatim in structure: the binding is a macro, the block carries + // memory qualifiers, and the macro's definition is still sitting in the text above it. + const String source = R"(#version 460 core +#define _FLW_MODEL_BUFFER_BINDING 3 +#define _FLW_DRAW_BUFFER_BINDING 4 +layout(local_size_x = 32) in; +layout(std430, binding = _FLW_MODEL_BUFFER_BINDING) restrict readonly buffer ModelBuffer { + uint models[]; +}; +layout(std430, binding = _FLW_DRAW_BUFFER_BINDING) restrict buffer DrawBuffer { + uint draws[]; +}; +layout(std430) buffer ReallyUnqualified { uint u; } reallyUnqualified; +void main() { draws[0] = models[0] + reallyUnqualified.u; } +)"; + + const std::set unqualified = ExtractStorageBlocksWithoutExplicitBinding(source); + EXPECT_EQ(unqualified.count("ModelBuffer"), 0u) + << "a binding spelled as a macro is still a declared binding, never an absent one"; + EXPECT_EQ(unqualified.count("DrawBuffer"), 0u) + << "every block of the engine would otherwise be defaulted onto 0 together"; + EXPECT_EQ(unqualified.count("ReallyUnqualified"), 1u) + << "a block that truly declares no binding is still recognised in the same shader"; +} + +// The other two ways an unexpanded macro can hide a binding: as a whole layout entry, and as the +// whole qualifier run. Both have to read as doubt for the same reason the macro-valued binding +// does - what the scanner cannot expand, it must not claim is absent. +TEST_F(ProgramUtilTest, ExtractStorageBlocksWithoutExplicitBindingDoubtsAMacroQualifier) { + using namespace MG_Util::ShaderTranspiler; + + const String source = R"(#version 430 core +#define FLW_BINDING binding = 2 +#define SSBO_QUALIFIER layout(std430, binding = 6) restrict +layout(local_size_x = 1) in; +layout(std430, FLW_BINDING) buffer EntryMacro { uint a; } entryMacro; +SSBO_QUALIFIER buffer RunMacro { uint b; } runMacro; +layout(std430, row_major) buffer PlainLayout { uint c; } plainLayout; +void main() { plainLayout.c = entryMacro.a + runMacro.b; } +)"; + + const std::set unqualified = ExtractStorageBlocksWithoutExplicitBinding(source); + EXPECT_EQ(unqualified.count("EntryMacro"), 0u) + << "an unreadable layout entry may itself be the binding"; + EXPECT_EQ(unqualified.count("RunMacro"), 0u) + << "a macro standing in for the whole qualifier run may carry the binding"; + // The counterweight: recognising doubt must not swallow the layout identifiers a buffer + // block legally carries, or nothing would ever be defaulted again. + EXPECT_EQ(unqualified.count("PlainLayout"), 1u) + << "std430/row_major are GLSL, not macros, and leave no doubt behind"; +} diff --git a/MobileGL/MG_Util/ShaderTranspiler/ShaderSourceProcessor.cpp b/MobileGL/MG_Util/ShaderTranspiler/ShaderSourceProcessor.cpp index fc67c811..06f0266a 100644 --- a/MobileGL/MG_Util/ShaderTranspiler/ShaderSourceProcessor.cpp +++ b/MobileGL/MG_Util/ShaderTranspiler/ShaderSourceProcessor.cpp @@ -1218,6 +1218,22 @@ namespace MobileGL { } namespace { + // The layout qualifiers a shader storage block may legally carry WITHOUT a + // value. GLSL 4.60 4.4.5: a buffer block's layout list holds the memory-layout + // and matrix-order identifiers below, plus binding/offset/align, which are + // spelled `name = value` and so never reach this test. Anything else bare in + // such a list is not GLSL - on the unexpanded text this scanner reads, it is a + // macro, and a macro may expand to the binding itself. + bool IsBufferBlockLayoutIdentifier(const String& text) { + static const char* kLayoutIdentifiers[] = { + "shared", "packed", "std140", "std430", "row_major", "column_major", + }; + for (const char* identifier : kLayoutIdentifiers) { + if (text == identifier) return true; + } + return false; + } + bool IsNonLayoutQualifierKeyword(const String& text) { static const char* kQualifiers[] = { "highp", "mediump", "lowp", "precise", "const", "flat", @@ -1569,12 +1585,27 @@ namespace MobileGL { // Several layout(...) lists may precede one declaration and the later one wins - // the same accumulate-then-consume shape FindShaderStorageBindingViolation uses. long long binding = -1; + // Set when the run carries something this scanner cannot read as a binding but + // which MAY BE ONE. THE TEXT SCANNED HERE IS NOT MACRO-EXPANDED - MobileGL's + // preprocessing rewrites the source, it does not run the C preprocessor, so + // `#define`s and their uses both survive into it. `binding = SOME_MACRO` is + // therefore the common spelling in real shader packs, not an exotic one + // (Flywheel's indirect engine writes every one of its blocks that way), and + // reading "no literal" as "no binding" DEFAULTS AWAY a binding the shader + // really declared: every such block would be seeded to 0 and alias there. Doubt + // is resolved by dropping the name, which restores the pre-seeding behaviour + // for exactly the declarations this scanner cannot read. + bool unreadableQualifier = false; long long literal = 0; + const auto endRun = [&binding, &unreadableQualifier]() { + binding = -1; + unreadableQualifier = false; + }; for (SizeT pos = 0; pos < count; ++pos) { const String& text = tokens[pos].text; if (text == "{") { ++braceDepth; - binding = -1; + endRun(); continue; } if (text == "}") { @@ -1594,11 +1625,27 @@ namespace MobileGL { ++parenDepth; } else if (layoutToken == ")") { --parenDepth; - } else if (parenDepth == 1 && layoutToken == "binding" && j + 2 < count && - tokens[j + 1].text == "=" && - ParseGlslIntegerLiteral(tokens[j + 2].text, literal)) { - binding = std::min(literal, static_cast(INT_MAX / 2)); + } else if (parenDepth != 1) { + // Nested parentheses belong to some entry's value expression, + // which is not a literal - the entry itself is judged below. + } else if (layoutToken == "binding" && j + 2 < count && tokens[j + 1].text == "=") { + if (ParseGlslIntegerLiteral(tokens[j + 2].text, literal)) { + binding = std::min(literal, static_cast(INT_MAX / 2)); + } else { + // A binding IS declared here; its value is just spelled as + // something this scanner does not evaluate (a macro, a + // const, an expression). The one thing it certainly is not + // is absent. + unreadableQualifier = true; + } j += 2; + } else if (IsIdentifierToken(tokens[j]) && !IsBufferBlockLayoutIdentifier(layoutToken) && + !(j + 1 < count && tokens[j + 1].text == "=")) { + // A bare identifier that is none of the layout qualifiers a + // buffer block may legally carry. On unexpanded text that is a + // macro, and a macro may well be the binding + // (`layout(std430, MY_BINDING) buffer B {...}`). + unreadableQualifier = true; } ++j; } @@ -1614,16 +1661,25 @@ namespace MobileGL { // fall through and keep today's behaviour. if (pos + 2 < count && IsIdentifierToken(tokens[pos + 1]) && tokens[pos + 2].text == "{") { - (binding < 0 ? names : qualified).insert(tokens[pos + 1].text); + // The token immediately before `buffer` is the last of the run. A + // non-keyword identifier there is a macro standing in for the whole + // qualifier list (`SSBO_QUALIFIER buffer B {...}`), so it may carry + // the binding just as an unreadable layout entry may. + const bool macroQualifier = pos > 0 && IsIdentifierToken(tokens[pos - 1]) && + !IsNonLayoutQualifierKeyword(tokens[pos - 1].text); + const bool unqualified = binding < 0 && !unreadableQualifier && !macroQualifier; + (unqualified ? names : qualified).insert(tokens[pos + 1].text); } - binding = -1; + endRun(); continue; } // Qualifiers may sit between the layout list and the `buffer` keyword; // anything else ends the run, so a binding never leaks onto an unrelated // declaration - and, just as importantly, the ABSENCE of one never does. - if (!IsNonLayoutQualifierKeyword(text)) binding = -1; + // Ending the run clears the doubt with it: the unreadable qualifier belonged + // to the declaration that just ended, not to whatever follows. + if (!IsNonLayoutQualifierKeyword(text)) endRun(); } for (const String& name : qualified) { names.erase(name);