[Fix, Test] (DirectVulkan): resolve a lowered atomic-counter block from the atomic-counter binding points

MobileGL/MG_Backend/DirectVulkan/Renderer/UniformManager.cpp

@@ -17,6 +17,7 @@
 #include "MG_Util/Converters/MGToStr/FramebufferEnumConverter.h"
 #include "MG_Util/Converters/MGToVk/TextureEnumConverter.h"
 #include "MG_Util/Metrics/TextureMetrics.h"
+#include "MG_Util/ShaderTranspiler/Types.h"
 #include <Config.h>
 #include <algorithm>
 #include <cstdio>
@@ -923,22 +924,46 @@ namespace MobileGL::MG_Backend::DirectVulkan {
         const Int blockIndex = programObj.storageBlockIndexByBinding[binding];
         MOBILEGL_ASSERT(blockIndex >= 0, "ResolveStorageBufferDescriptor: no SSBO block mapped to binding %u",
                         binding);
+        // An atomic counter is not an SSBO the application ever declared: glslang lowers every
+        // atomic_uint onto a synthesized gl_AtomicCounterBlock_<N> storage block, where N is the
+        // GL ATOMIC-COUNTER binding. That block arrives here auto-mapped to an arbitrary
+        // storage-block slot, so resolving it the SSBO way looked up GL_SHADER_STORAGE_BUFFER
+        // point N' - which is never where glBindBufferBase(GL_ATOMIC_COUNTER_BUFFER, N, ...) put
+        // the buffer. The counter therefore never reached the shader (KHR-GL43
+        // shader_atomic_counters.advanced-usage-*), and when the application also bound an SSBO at
+        // the colliding slot the descriptor silently aliased it, so the dispatch wrote over the
+        // application's own buffer. DirectGLES has always taken this branch explicitly
+        // (SyncAtomicCounterBuffers); this is the same rule in Magma's descriptor resolution.
+        //
+        // Only the SOURCE of the handle differs. The per-counter layout(offset=) is already folded
+        // into the block's SPIR-V member offsets on this path (FlattenAtomicCounterBlockPass is
+        // DirectGLES-only), so everything below - residency, the glBindBufferRange window, the
+        // descriptor fill - is target-agnostic and stays exactly as it was.
+        const String& blockName = programObj.storageBlockNameByBinding[binding];
+        const Int atomicCounterBinding = MG_Util::ShaderTranspiler::AtomicCounterBlockGlBinding(blockName);
+        const Bool isAtomicCounterBlock = atomicCounterBinding >= 0;
+        const BufferTarget bufferTarget =
+            isAtomicCounterBlock ? BufferTarget::AtomicCounter : BufferTarget::ShaderStorage;
         // A block instance array declares one block whose elements take consecutive GL binding
         // points from the declared one (GL 4.6 core 7.8), and the reflection collapses the whole
-        // array to that one block - so the element index IS the offset from its binding.
+        // array to that one block - so the element index IS the offset from its binding. glslang
+        // synthesizes one counter block per GL binding, so a counter block is never an instance
+        // array and `element` is always 0 there; the +element rule stays with the SSBO case.
         const GLuint frontendBinding =
-            GetShaderStorageBlockBinding(program, static_cast<GLuint>(blockIndex)) + element;
+            isAtomicCounterBlock
+                ? static_cast<GLuint>(atomicCounterBinding)
+                : GetShaderStorageBlockBinding(program, static_cast<GLuint>(blockIndex)) + element;
         const Uint32 bindingPointCount =
-            static_cast<Uint32>(MG_State::pGLContext->GetBufferBindingPointCount(BufferTarget::ShaderStorage));
+            static_cast<Uint32>(MG_State::pGLContext->GetBufferBindingPointCount(bufferTarget));
         MOBILEGL_ASSERT(frontendBinding < bindingPointCount,
-                        "ResolveStorageBufferDescriptor: frontend SSBO binding %u out of range for block '%s'",
+                        "ResolveStorageBufferDescriptor: frontend binding %u out of range for block '%s'",
-                        frontendBinding, programObj.storageBlockNameByBinding[binding].c_str());
+                        frontendBinding, blockName.c_str());
 
-        auto& bindingPoint = MG_State::pGLContext->GetBufferBindingPoint(BufferTarget::ShaderStorage, frontendBinding);
+        auto& bindingPoint = MG_State::pGLContext->GetBufferBindingPoint(bufferTarget, frontendBinding);
         const auto& bufferObject = bindingPoint.GetBoundObject();
         if (bufferObject == nullptr) {
-            MGLOG_E_ONCE("ResolveStorageBufferDescriptor: no SSBO bound at frontend binding %u for block '%s'",
+            MGLOG_E_ONCE("ResolveStorageBufferDescriptor: no buffer bound at frontend binding %u for block '%s'",
-                    frontendBinding, programObj.storageBlockNameByBinding[binding].c_str());
+                    frontendBinding, blockName.c_str());
             return false;
         }
 

MobileGL/MG_IntegrationTest/Scenarios/AtomicCounterScenario.cpp

@@ -87,11 +87,6 @@ void main() {
                                  << " and GL_MAX_COMPUTE_ATOMIC_COUNTER_BUFFERS is " << buffers
                                  << "; this needs 3 and 2";
                 }
-                if (!AtomicCountersAreWired()) {
-                    GTEST_SKIP() << "atomic counter buffers are not wired up on " << Gl().BackendName()
-                                 << " yet: glslang lowers them onto a storage block and that block's descriptor "
-                                 << "is still resolved from the shader-storage binding points";
-                }
                 m_program = CompileComputeProgram(kCounterComputeSource);
                 ASSERT_NE(m_program, 0u) << m_buildLog;
             }
@@ -105,12 +100,6 @@ void main() {
                 m_program = 0;
             }
 
-            // Magma binds the lowered block as an ordinary storage-buffer descriptor resolved
-            // from GL_SHADER_STORAGE_BUFFER point N, so the counter buffer never reaches it. The
-            // frontend half (limits, reflection queries, the link-time offset rules) is
-            // backend-agnostic and is covered by the unit suites; only the VALUE is scoped here.
-            bool AtomicCountersAreWired() const { return Gl().BackendName() != "DirectVulkan"; }
-
             unsigned int CompileComputeProgram(const char* source) {
                 const GLuint shader = glCreateShader(GL_COMPUTE_SHADER);
                 glShaderSource(shader, 1, &source, nullptr);

MobileGL/MG_Util/ShaderTranspiler/Types.h

@@ -20,6 +20,28 @@ namespace MobileGL {
             // buffer, and the trailing number is the only place the GL binding survives.
             inline constexpr const char* ATOMIC_COUNTER_BLOCK_PREFIX = "gl_AtomicCounterBlock";
 
+            // "gl_AtomicCounterBlock_5" -> 5; -1 for any name that is not one of these blocks.
+            // Recovering N from the NAME is not a shortcut, it is the only way: the block reaches
+            // a backend auto-mapped to whatever storage-block slot the IO mapper had free, and
+            // that number has no relation to the GL atomic-counter binding the application asked
+            // for (see TMglGlslIoResolver). A backend that resolves the block from the
+            // shader-storage binding points therefore binds the wrong buffer - or, worse, the
+            // application's own SSBO at the same slot.
+            inline Int AtomicCounterBlockGlBinding(StringView name) {
+                const SizeT prefixLength = StringView(ATOMIC_COUNTER_BLOCK_PREFIX).size();
+                // Needs the prefix, the '_' and at least one digit.
+                if (name.size() <= prefixLength + 1) return -1;
+                if (name.compare(0, prefixLength, ATOMIC_COUNTER_BLOCK_PREFIX) != 0) return -1;
+                if (name[prefixLength] != '_') return -1;
+                Int binding = 0;
+                for (SizeT i = prefixLength + 1; i < name.size(); ++i) {
+                    if (name[i] < '0' || name[i] > '9') return -1;
+                    binding = binding * 10 + (name[i] - '0');
+                    if (binding > 0x0FFFFFFF) return -1; // absurd suffix; treat as not-a-counter
+                }
+                return binding;
+            }
+
             // Atomic-counter limits, in ONE place because GL 4.6 requires glGetIntegerv and the
             // shading language's gl_MaxAtomicCounter* constants to report the same numbers
             // (KHR-GL43.shader_atomic_counters.basic-glsl-built-in compares them directly).