[Merge] (ShaderTranspiler): land the macro-spelled storage-block binding repair

MobileGL/MG_Test/Program/ProgramUtilTest.cpp

@@ -4818,3 +4818,65 @@ subroutine(FuncType) void Func0(int coord) { fragColor = vec4(float(coord)); }
     EXPECT_EQ(conditional, conditionalBefore)
         << "an inactive #if arm must not have an unconditional forwarding body appended for it";
 }
+
+// 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<String> 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<String> 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";
+}

MobileGL/MG_Util/ShaderTranspiler/ShaderSourceProcessor.cpp

@@ -1621,6 +1621,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",
@@ -1972,12 +1988,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 == "}") {
@@ -1997,11 +2028,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<long long>(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<long long>(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;
                         }
@@ -2017,16 +2064,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);