[Fix, Test] (GLState, ShaderTranspiler): report GL's default binding of zero for an unqualified uniform block

MobileGL/MG_State/GLState/ProgramState/ProgramLinkTask.cpp

@@ -627,7 +627,8 @@ namespace MobileGL::MG_State::GLState {
                              .explicitFragmentOutLocations = in.explicitFragDataLocation,
                              .explicitFragmentOutIndices = in.explicitFragDataIndex,
                              .explicitOpaqueUniformBindings = &artifacts.explicitOpaqueUniformBindings,
-                             .storageBlocksWithoutBinding = &artifacts.storageBlocksWithoutBinding};
+                             .storageBlocksWithoutBinding = &artifacts.storageBlocksWithoutBinding,
+                             .uniformBlocksWithoutBinding = &artifacts.uniformBlocksWithoutBinding};
 
         MGLOG_D("ProgramObject %u: Calling ShaderCompiler::LinkProgram", in.externalIndex);
         auto result = ShaderCompiler::LinkProgram(attrib);
@@ -1573,7 +1574,25 @@ namespace MobileGL::MG_State::GLState {
             // (DirectGLES.cpp / UniformManager.cpp), all 14 elements also read the same
             // buffer. This is the rule the storage-block path in ProgramInterface.cpp
             // already applies, and whose comment there claims uniform blocks follow.
-            const Int declaredBinding = ubo.getBinding();
+            //
+            // "Declared" cannot be read back off the reflection, though. MobileGL asks glslang
+            // to auto-map bindings, so mapIO writes an invented one into every block's
+            // qualifier before reflection ever runs and ubo.getBinding() is never negative;
+            // worse, glslang packs uniform blocks into the SAME slot space as samplers and
+            // images (setEnvClient(EShClientVulkan) leaves spvVersion.openGl at 0, so
+            // TDefaultGlslIoResolver::resolveBinding keys every resource kind on set 0), so a
+            // block declared after an unbound image gets 1. GL 4.6 core 7.6.2 says an
+            // unqualified block reports ZERO. The set below is the shader's own answer,
+            // captured during mapIO while the qualifier still meant it - the same mechanism
+            // SeedDefaultStorageBlockBindings uses for storage blocks, and the aliasing at 0
+            // that results is GL's, not a bug: unqualified blocks collide there until the
+            // application rebinds them.
+            //
+            // Only this GL-visible binding POINT changes. The backends' descriptor lookups run
+            // off glslang's assignment through uniformBlockIndexByBinding, which is untouched.
+            const String blockTypeName = StripArrayElementSuffix(ubo.name);
+            const Int declaredBinding =
+                artifacts.uniformBlocksWithoutBinding.contains(blockTypeName) ? 0 : ubo.getBinding();
             artifacts.uniformBlockBinding[i] =
                 declaredBinding < 0 ? declaredBinding : declaredBinding + BlockArrayElement(ubo.name);
             MGLOG_D("ProgramObject %u: Reflection - UBO[%d] name='%s' size=%u binding=%d", in.externalIndex, i,

MobileGL/MG_State/GLState/ProgramState/ProgramObject.cpp

@@ -369,6 +369,7 @@ namespace MobileGL::MG_State::GLState {
         // link, so a stale set would otherwise default a block the new sources do declare a
         // binding for.
         artifacts.storageBlocksWithoutBinding.clear();
+        artifacts.uniformBlocksWithoutBinding.clear();
         artifacts.attribs.clear();
         artifacts.attribTypes.clear();
         artifacts.activeUniformCount = 0;

MobileGL/MG_State/GLState/ProgramState/ProgramObject.h

@@ -1024,8 +1024,10 @@ namespace MobileGL::MG_State::GLState {
         Uint32 GetBlockBindingVersion() const { return m_blockBindingVersion; }
 
         // Set by glUniformBlockBinding. The vector is seeded at link with each block's DECLARED
-        // binding (layout(binding=N), else -1), so an untouched program already reports what its
-        // shaders asked for.
+        // binding (layout(binding=N)), and with GL's default of 0 for a block that declared none
+        // - which the reflection cannot tell apart on its own, so the seeder consults
+        // uniformBlocksWithoutBinding. Either way an untouched program already reports what GL
+        // says it should.
         void SetUniformBlockBinding(Uint index, Uint binding) {
             if (index >= Artifacts().uniformBlockBinding.size() || Artifacts().uniformBlockBinding[index] == static_cast<Int>(binding)) {
                 return;
@@ -1287,6 +1289,11 @@ namespace MobileGL::MG_State::GLState {
             // binding from an invented one - and, unlike the per-shader lexer this replaced,
             // sees the declaration with its macros expanded.
             std::set<String> storageBlocksWithoutBinding;
+            // The same list for UNIFORM blocks, and it is needed for the same reason: glslang's
+            // auto-mapper assigns every uniform block a binding whether or not the shader asked
+            // for one, so uniformBlockBinding below cannot tell "declared 1" from "invented 1".
+            // GL 4.6 core 7.6.2 requires an unqualified block to report ZERO.
+            std::set<String> uniformBlocksWithoutBinding;
 
             Uint activeUniformCount = 0;
             Uint maxUniformLocation = 0;

MobileGL/MG_Test/ShaderTranspiler/GlslangCaptureTest.cpp

@@ -123,6 +123,7 @@ namespace {
         String log;
         UnorderedMap<String, Uint> opaqueBindings;
         std::set<String> storageBlocksWithoutBinding;
+        std::set<String> uniformBlocksWithoutBinding;
         UnorderedMap<String, Int> uniformLocations;
     };
 
@@ -145,6 +146,7 @@ namespace {
         if (captureEnabled) {
             programAttrib.explicitOpaqueUniformBindings = &capture.opaqueBindings;
             programAttrib.storageBlocksWithoutBinding = &capture.storageBlocksWithoutBinding;
+            programAttrib.uniformBlocksWithoutBinding = &capture.uniformBlocksWithoutBinding;
         }
 
         auto programResult = ShaderCompiler::LinkProgram(programAttrib);
@@ -561,11 +563,49 @@ void main() {
         << "a declared binding must never be defaulted away";
     EXPECT_EQ(capture.storageBlocksWithoutBinding.count("BoundFirst"), 0u)
         << "the binding may appear anywhere in the layout list, not only last";
-    // A UNIFORM block is a different binding space with its own glUniformBlockBinding path, and
-    // its default is already handled where uniformBlockBinding is seeded. Naming it here would
-    // make the seeder default a resource it does not own.
+    // A UNIFORM block is a different binding space with its own glUniformBlockBinding path, so it
+    // must not reach the storage-block seeder - it has a capture set of its own (see
+    // UnqualifiedUniformBlocksAreCapturedSeparatelyFromStorageBlocks below).
     EXPECT_EQ(capture.storageBlocksWithoutBinding.count("InputBuffer"), 0u)
-        << "uniform blocks are out of scope";
+        << "uniform blocks belong to the other set";
+}
+
+// The uniform-block half of the same capture, and the reason it exists: glslang packs uniform
+// blocks into the same auto-mapped slot space as samplers and images, so an unqualified block
+// declared AFTER an unbound image comes back carrying binding 1 while GL 4.6 core 7.6.2 requires
+// it to report 0. Reflection cannot tell the invented number from a declared one, so the shader's
+// own answer has to be captured here, during mapIO, and applied at reflection time.
+// KHR-GL4{2,3}.shading_language_420pack.binding_uniform_default is exactly this shader shape.
+TEST_F(GlslangCaptureProbeTest, UnqualifiedUniformBlocksAreCapturedSeparatelyFromStorageBlocks) {
+    const String source = R"(#version 430 core
+layout(local_size_x = 1) in;
+writeonly uniform image2D uni_image;
+layout(std140) uniform GOKU { vec4 gohan; vec4 goten; } goku;
+layout(std140, binding = 3) uniform VEGETA { vec4 trunks; } vegeta;
+layout(std430) buffer OutputBuffer { vec4 data0[]; } g_out_buffer;
+void main() {
+  g_out_buffer.data0[0] = goku.gohan + goku.goten + vegeta.trunks;
+  imageStore(uni_image, ivec2(0), vec4(1.0));
+}
+)";
+
+    const LinkCapture capture = CaptureFromCompute(source);
+    ASSERT_TRUE(capture.linked) << capture.log;
+
+    EXPECT_EQ(capture.uniformBlocksWithoutBinding.count("GOKU"), 1u)
+        << "an unqualified uniform block declared after an unbound image is the regressing shape";
+    EXPECT_EQ(capture.uniformBlocksWithoutBinding.count("VEGETA"), 0u)
+        << "a declared binding must never be defaulted away";
+    EXPECT_EQ(capture.uniformBlocksWithoutBinding.count("OutputBuffer"), 0u)
+        << "storage blocks belong to the other set";
+    EXPECT_EQ(capture.storageBlocksWithoutBinding.count("GOKU"), 0u)
+        << "the two sets must not cross-contaminate";
+
+    // The negative control every capture case here carries: with the OUT pointer left null the
+    // resolver must write nothing at all.
+    const LinkCapture off = CaptureFromCompute(source, /*captureEnabled=*/false);
+    ASSERT_TRUE(off.linked) << off.log;
+    EXPECT_TRUE(off.uniformBlocksWithoutBinding.empty());
 }
 
 // The capture must not mistake a buffer-typed SAMPLER or a member qualifier for a block, and

MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.cpp

@@ -486,7 +486,8 @@ namespace MobileGL {
                                                        attrib.explicitFragmentOutLocations,
                                                        attrib.explicitFragmentOutIndices,
                                                        attrib.explicitOpaqueUniformBindings,
-                                                       attrib.storageBlocksWithoutBinding);
+                                                       attrib.storageBlocksWithoutBinding,
+                                                       attrib.uniformBlocksWithoutBinding);
                     break;
                 }
                 auto ioMapper = UniquePtr<glslang::TIoMapper>(glslang::GetGlslIoMapper());

MobileGL/MG_Util/ShaderTranspiler/Types.h

@@ -76,6 +76,7 @@ namespace MobileGL {
                 // assigned - see the comment on TMglGlslIoResolver::reserverResourceSlot.
                 UnorderedMap<String, Uint>* explicitOpaqueUniformBindings = nullptr;
                 std::set<String>* storageBlocksWithoutBinding = nullptr;
+                std::set<String>* uniformBlocksWithoutBinding = nullptr;
             };
 
             struct ProgramBinaryAttrib {

MobileGL/MG_Util/ShaderTranspiler/glslang/TMglGlslIoResolver.cpp

@@ -196,6 +196,17 @@ namespace MobileGL {
             m_storageBlocksWithoutBinding->insert(name.c_str());
         }
 
+        // A UNIFORM block that declared no binding. Same capture point and same union-across-
+        // stages reasoning as the storage-block set above, and the same reason it cannot be
+        // asked later: mapIO is about to write an auto-assigned binding into this very
+        // qualifier. MGL_GLOBAL_UBO is MobileGL's own synthesized block, not an application
+        // one - it never reaches the GL block space and must not be seeded here.
+        if (m_uniformBlocksWithoutBinding != nullptr && type.getBasicType() == glslang::EbtBlock &&
+            qualifier.storage == glslang::EvqUniform && !qualifier.hasBinding() &&
+            name.compare(MG_Util::ShaderTranspiler::GLOBAL_UBO_NAME) != 0) {
+            m_uniformBlocksWithoutBinding->insert(name.c_str());
+        }
+
         TDefaultGlslIoResolver::reserverResourceSlot(ent, infoSink);
     }
 

MobileGL/MG_Util/ShaderTranspiler/glslang/TMglGlslIoResolver.h

@@ -29,16 +29,19 @@ namespace MobileGL {
         TMglGlslIoResolver(const glslang::TIntermediate& intermediate, const ExplicitVarSlotMap& vertexIns,
                            const ExplicitVarSlotMap& fragOuts, const ExplicitVarSlotMap& fragOutIndices,
                            ExplicitVarSlotMap* opaqueUniformBindings,
-                           std::set<String>* storageBlocksWithoutBinding = nullptr)
+                           std::set<String>* storageBlocksWithoutBinding = nullptr,
+                           std::set<String>* uniformBlocksWithoutBinding = nullptr)
             : TDefaultGlslIoResolver(intermediate), m_explicitVertexIns(vertexIns), m_explicitFragOuts(fragOuts),
               m_explicitFragOutIndices(fragOutIndices), m_explicitOpaqueUniformBindings(opaqueUniformBindings),
-              m_storageBlocksWithoutBinding(storageBlocksWithoutBinding) {}
+              m_storageBlocksWithoutBinding(storageBlocksWithoutBinding),
+              m_uniformBlocksWithoutBinding(uniformBlocksWithoutBinding) {}
         TMglGlslIoResolver(const glslang::TProgram& program, const EShLanguage stage,
                            const ExplicitVarSlotMap& vertexIns, const ExplicitVarSlotMap& fragOuts,
                            const ExplicitVarSlotMap& fragOutIndices, ExplicitVarSlotMap* opaqueUniformBindings,
-                           std::set<String>* storageBlocksWithoutBinding = nullptr)
+                           std::set<String>* storageBlocksWithoutBinding = nullptr,
+                           std::set<String>* uniformBlocksWithoutBinding = nullptr)
             : TMglGlslIoResolver(*program.getIntermediate(stage), vertexIns, fragOuts, fragOutIndices,
-                                 opaqueUniformBindings, storageBlocksWithoutBinding) {}
+                                 opaqueUniformBindings, storageBlocksWithoutBinding, uniformBlocksWithoutBinding) {}
         void reserverStorageSlot(glslang::TVarEntryInfo& ent, TInfoSink& infoSink) override;
         void reserverResourceSlot(glslang::TVarEntryInfo& ent, TInfoSink& infoSink) override;
         int resolveInOutLocation(EShLanguage stage, glslang::TVarEntryInfo& ent) override;
@@ -62,6 +65,13 @@ namespace MobileGL {
         // layout(binding = N). GL 4.3 core 7.8 gives such a block binding ZERO; see
         // ProgramLinkTask::SeedDefaultStorageBlockBindings for what is done with them.
         std::set<String>* m_storageBlocksWithoutBinding = nullptr;
+        // The same capture for UNIFORM blocks. GL 4.6 core 7.6.2 gives an unqualified uniform
+        // block binding ZERO, and glslang's auto-mapper does not: it packs uniform blocks into
+        // the same slot space as samplers and images (spvVersion.openGl is 0 under
+        // setEnvClient(EShClientVulkan), so TDefaultGlslIoResolver::resolveBinding keys every
+        // resource kind on set 0), so an unbound block declared after an unbound image lands on
+        // 1. See ProgramLinkTask's UBO reflection loop for what is done with them.
+        std::set<String>* m_uniformBlocksWithoutBinding = nullptr;
         std::map<glslang::TString, int> m_plainUniformLocationSizeByName;
         std::map<glslang::TString, int> m_plainUniformLocationByName;
         bool m_plainUniformLocationsAssigned = false;