Always consider type arrayness for atomic counters

src/compiler/translator/ParseContext.cpp

@@ -2248,30 +2248,35 @@ void TParseContext::checkMemoryQualifierIsNotSpecified(const TMemoryQualifier &m
 
 // Make sure there is no offset overlapping, and store the newly assigned offset to "type" in
 // intermediate tree.
-void TParseContext::checkAtomicCounterOffsetIsNotOverlapped(TPublicType &publicType,
-                                                            size_t size,
-                                                            bool forceAppend,
-                                                            const TSourceLoc &loc,
-                                                            TType &type)
+void TParseContext::checkAtomicCounterOffsetDoesNotOverlap(bool forceAppend,
+                                                           const TSourceLoc &loc,
+                                                           TType *type)
 {
-    auto &bindingState = mAtomicCounterBindingStates[publicType.layoutQualifier.binding];
+    if (!IsAtomicCounter(type->getBasicType()))
+    {
+        return;
+    }
+
+    const size_t size = type->isArray() ? kAtomicCounterArrayStride * type->getArraySizeProduct()
+                                        : kAtomicCounterSize;
+    TLayoutQualifier layoutQualifier = type->getLayoutQualifier();
+    auto &bindingState               = mAtomicCounterBindingStates[layoutQualifier.binding];
     int offset;
-    if (publicType.layoutQualifier.offset == -1 || forceAppend)
+    if (layoutQualifier.offset == -1 || forceAppend)
     {
         offset = bindingState.appendSpan(size);
     }
     else
     {
-        offset = bindingState.insertSpan(publicType.layoutQualifier.offset, size);
+        offset = bindingState.insertSpan(layoutQualifier.offset, size);
     }
     if (offset == -1)
     {
         error(loc, "Offset overlapping", "atomic counter");
         return;
     }
-    TLayoutQualifier qualifier = type.getLayoutQualifier();
-    qualifier.offset           = offset;
-    type.setLayoutQualifier(qualifier);
+    layoutQualifier.offset = offset;
+    type->setLayoutQualifier(layoutQualifier);
 }
 
 void TParseContext::checkGeometryShaderInputAndSetArraySize(const TSourceLoc &location,
@@ -2373,12 +2378,7 @@ TIntermDeclaration *TParseContext::parseSingleDeclaration(
 
         checkCanBeDeclaredWithoutInitializer(identifierOrTypeLocation, identifier, &type);
 
-        if (IsAtomicCounter(publicType.getBasicType()))
-        {
-
-            checkAtomicCounterOffsetIsNotOverlapped(publicType, kAtomicCounterSize, false,
-                                                    identifierOrTypeLocation, type);
-        }
+        checkAtomicCounterOffsetDoesNotOverlap(false, identifierOrTypeLocation, &type);
 
         TVariable *variable = nullptr;
         declareVariable(identifierOrTypeLocation, identifier, type, &variable);
@@ -2421,12 +2421,7 @@ TIntermDeclaration *TParseContext::parseSingleArrayDeclaration(TPublicType &elem
 
     checkCanBeDeclaredWithoutInitializer(identifierLocation, identifier, &arrayType);
 
-    if (IsAtomicCounter(elementType.getBasicType()))
-    {
-        checkAtomicCounterOffsetIsNotOverlapped(
-            elementType, kAtomicCounterArrayStride * arrayType.getArraySizeProduct(), false,
-            identifierLocation, arrayType);
-    }
+    checkAtomicCounterOffsetDoesNotOverlap(false, identifierLocation, &arrayType);
 
     TVariable *variable = nullptr;
     declareVariable(identifierLocation, identifier, arrayType, &variable);
@@ -2587,11 +2582,8 @@ void TParseContext::parseDeclarator(TPublicType &publicType,
 
     checkCanBeDeclaredWithoutInitializer(identifierLocation, identifier, &type);
 
-    if (IsAtomicCounter(publicType.getBasicType()))
-    {
-        checkAtomicCounterOffsetIsNotOverlapped(publicType, kAtomicCounterSize, true,
-                                                identifierLocation, type);
-    }
+    checkAtomicCounterOffsetDoesNotOverlap(true, identifierLocation, &type);
+
     declareVariable(identifierLocation, identifier, type, &variable);
 
     if (variable)
@@ -2628,12 +2620,7 @@ void TParseContext::parseArrayDeclarator(TPublicType &elementType,
 
         checkCanBeDeclaredWithoutInitializer(identifierLocation, identifier, &arrayType);
 
-        if (IsAtomicCounter(elementType.getBasicType()))
-        {
-            checkAtomicCounterOffsetIsNotOverlapped(
-                elementType, kAtomicCounterArrayStride * arrayType.getArraySizeProduct(), true,
-                identifierLocation, arrayType);
-        }
+        checkAtomicCounterOffsetDoesNotOverlap(true, identifierLocation, &arrayType);
 
         TVariable *variable = nullptr;
         declareVariable(identifierLocation, identifier, arrayType, &variable);

src/compiler/translator/ParseContext.h

@@ -441,6 +441,7 @@ class TParseContext : angle::NonCopyable
     // we treat it as always 4 in favour of the original interpretation in
     // "ARB_shader_atomic_counters".
     // TODO(jie.a.chen@intel.com): Double check this once the spec vagueness is resolved.
+    // Note that there may be tests in AtomicCounter_test that will need to be updated as well.
     constexpr static size_t kAtomicCounterArrayStride = 4;
 
     // Returns a clamped index. If it prints out an error message, the token is "[]".
@@ -480,11 +481,9 @@ class TParseContext : angle::NonCopyable
                                            TLayoutImageInternalFormat internalFormat);
     void checkMemoryQualifierIsNotSpecified(const TMemoryQualifier &memoryQualifier,
                                             const TSourceLoc &location);
-    void checkAtomicCounterOffsetIsNotOverlapped(TPublicType &publicType,
-                                                 size_t size,
-                                                 bool forceAppend,
-                                                 const TSourceLoc &loc,
-                                                 TType &type);
+    void checkAtomicCounterOffsetDoesNotOverlap(bool forceAppend,
+                                                const TSourceLoc &loc,
+                                                TType *type);
     void checkBindingIsValid(const TSourceLoc &identifierLocation, const TType &type);
     void checkBindingIsNotSpecified(const TSourceLoc &location, int binding);
     void checkOffsetIsNotSpecified(const TSourceLoc &location, int offset);

src/tests/compiler_tests/AtomicCounter_test.cpp

@@ -211,3 +211,20 @@ TEST_F(AtomicCounterTest, OffsetMustNotSpecifiedForGlobalLayoutQualifier)
         FAIL() << "Shader compilation succeeded, expecting failure:\n" << mInfoLog;
     }
 }
+
+// Test that offset overlapping leads to compile-time error (ESSL 3.10 section 4.4.6).
+// Note that there is some vagueness in the spec when it comes to this test.
+TEST_F(AtomicCounterTest, BindingOffsetOverlappingForArrays)
+{
+    const std::string &source =
+        "#version 310 es\n"
+        "layout(binding = 2, offset = 4) uniform atomic_uint[2] a;\n"
+        "layout(binding = 2, offset = 8) uniform atomic_uint b;\n"
+        "void main()\n"
+        "{\n"
+        "}\n";
+    if (compile(source))
+    {
+        FAIL() << "Shader compilation succeeded, expecting failure:\n" << mInfoLog;
+    }
+}