Fix checking image memory access

src/compiler/translator/ParseContext.cpp

@@ -66,6 +66,24 @@ bool ContainsImage(const TType &type)
     return false;
 }
 
+// Get a token from an image argument to use as an error message token.
+const char *GetImageArgumentToken(TIntermTyped *imageNode)
+{
+    ASSERT(IsImage(imageNode->getBasicType()));
+    while (imageNode->getAsBinaryNode() &&
+           (imageNode->getAsBinaryNode()->getOp() == EOpIndexIndirect ||
+            imageNode->getAsBinaryNode()->getOp() == EOpIndexDirect))
+    {
+        imageNode = imageNode->getAsBinaryNode()->getLeft();
+    }
+    TIntermSymbol *imageSymbol = imageNode->getAsSymbolNode();
+    if (imageSymbol)
+    {
+        return imageSymbol->getSymbol().c_str();
+    }
+    return "image";
+}
+
 }  // namespace
 
 TParseContext::TParseContext(TSymbolTable &symt,
@@ -4184,10 +4202,9 @@ void TParseContext::checkImageMemoryAccessForBuiltinFunctions(TIntermAggregate *
     if (name.compare(0, 5, "image") == 0)
     {
         TIntermSequence *arguments = functionCall->getSequence();
-        TIntermNode *imageNode     = (*arguments)[0];
-        TIntermSymbol *imageSymbol = imageNode->getAsSymbolNode();
+        TIntermTyped *imageNode    = (*arguments)[0]->getAsTyped();
 
-        const TMemoryQualifier &memoryQualifier = imageSymbol->getMemoryQualifier();
+        const TMemoryQualifier &memoryQualifier = imageNode->getMemoryQualifier();
 
         if (name.compare(5, 5, "Store") == 0)
         {
@@ -4195,7 +4212,7 @@ void TParseContext::checkImageMemoryAccessForBuiltinFunctions(TIntermAggregate *
             {
                 error(imageNode->getLine(),
                       "'imageStore' cannot be used with images qualified as 'readonly'",
-                      imageSymbol->getSymbol().c_str());
+                      GetImageArgumentToken(imageNode));
             }
         }
         else if (name.compare(5, 4, "Load") == 0)
@@ -4204,7 +4221,7 @@ void TParseContext::checkImageMemoryAccessForBuiltinFunctions(TIntermAggregate *
             {
                 error(imageNode->getLine(),
                       "'imageLoad' cannot be used with images qualified as 'writeonly'",
-                      imageSymbol->getSymbol().c_str());
+                      GetImageArgumentToken(imageNode));
             }
         }
     }
@@ -4223,7 +4240,8 @@ void TParseContext::checkImageMemoryAccessForUserDefinedFunctions(
 
     for (size_t i = 0; i < arguments.size(); ++i)
     {
-        const TType &functionArgumentType  = arguments[i]->getAsTyped()->getType();
+        TIntermTyped *typedArgument        = arguments[i]->getAsTyped();
+        const TType &functionArgumentType  = typedArgument->getType();
         const TType &functionParameterType = *functionDefinition->getParam(i).type;
         ASSERT(functionArgumentType.getBasicType() == functionParameterType.getBasicType());
 
@@ -4238,7 +4256,7 @@ void TParseContext::checkImageMemoryAccessForUserDefinedFunctions(
             {
                 error(functionCall->getLine(),
                       "Function call discards the 'readonly' qualifier from image",
-                      arguments[i]->getAsSymbolNode()->getSymbol().c_str());
+                      GetImageArgumentToken(typedArgument));
             }
 
             if (functionArgumentMemoryQualifier.writeonly &&
@@ -4246,7 +4264,7 @@ void TParseContext::checkImageMemoryAccessForUserDefinedFunctions(
             {
                 error(functionCall->getLine(),
                       "Function call discards the 'writeonly' qualifier from image",
-                      arguments[i]->getAsSymbolNode()->getSymbol().c_str());
+                      GetImageArgumentToken(typedArgument));
             }
 
             if (functionArgumentMemoryQualifier.coherent &&
@@ -4254,7 +4272,7 @@ void TParseContext::checkImageMemoryAccessForUserDefinedFunctions(
             {
                 error(functionCall->getLine(),
                       "Function call discards the 'coherent' qualifier from image",
-                      arguments[i]->getAsSymbolNode()->getSymbol().c_str());
+                      GetImageArgumentToken(typedArgument));
             }
 
             if (functionArgumentMemoryQualifier.volatileQualifier &&
@@ -4262,7 +4280,7 @@ void TParseContext::checkImageMemoryAccessForUserDefinedFunctions(
             {
                 error(functionCall->getLine(),
                       "Function call discards the 'volatile' qualifier from image",
-                      arguments[i]->getAsSymbolNode()->getSymbol().c_str());
+                      GetImageArgumentToken(typedArgument));
             }
         }
     }

src/tests/compiler_tests/ShaderValidation_test.cpp

@@ -2576,6 +2576,52 @@ TEST_F(FragmentShaderValidationTest, LoadFromWriteOnlyImage)
     }
 }
 
+// It is a compile-time error to call imageStore when the image is qualified as readonly.
+// Test to make sure this is validated correctly for images in arrays.
+// GLSL ES 3.10 Revision 4, 4.9 Memory Access Qualifiers
+TEST_F(FragmentShaderValidationTest, StoreInReadOnlyImageArray)
+{
+    const std::string &shaderString =
+        "#version 310 es\n"
+        "precision mediump float;\n"
+        "precision mediump image2D;\n"
+        "in vec4 myInput;\n"
+        "layout(r32f) uniform readonly image2D myImage[2];\n"
+        "void main() {\n"
+        "   imageStore(myImage[0], ivec2(0), vec4(1.0));\n"
+        "}\n";
+
+    if (compile(shaderString))
+    {
+        FAIL() << "Shader compilation succeeded, expecting failure:\n" << mInfoLog;
+    }
+}
+
+// It is a compile-time error to call imageStore when the image is qualified as readonly.
+// Test to make sure that checking this doesn't crash when validating an image in a struct.
+// Image in a struct in itself isn't accepted by the parser, but error recovery still results in
+// an image in the struct.
+// GLSL ES 3.10 Revision 4, 4.9 Memory Access Qualifiers
+TEST_F(FragmentShaderValidationTest, StoreInReadOnlyImageInStruct)
+{
+    const std::string &shaderString =
+        "#version 310 es\n"
+        "precision mediump float;\n"
+        "precision mediump image2D;\n"
+        "in vec4 myInput;\n"
+        "uniform struct S {\n"
+        "    layout(r32f) readonly image2D myImage;\n"
+        "} s;\n"
+        "void main() {\n"
+        "   imageStore(s.myImage, ivec2(0), vec4(1.0));\n"
+        "}\n";
+
+    if (compile(shaderString))
+    {
+        FAIL() << "Shader compilation succeeded, expecting failure:\n" << mInfoLog;
+    }
+}
+
 // A valid declaration and usage of an image3D.
 TEST_F(FragmentShaderValidationTest, ValidImage3D)
 {
@@ -2691,6 +2737,52 @@ TEST_F(FragmentShaderValidationTest, ReadOnlyQualifierMissingInFunctionArgument)
     }
 }
 
+// Passing an image qualifier to a function should not be able to discard the readonly qualifier.
+// Test with an image from an array.
+// GLSL ES 3.10 Revision 4, 4.9 Memory Access Qualifiers
+TEST_F(FragmentShaderValidationTest, ReadOnlyQualifierMissingInFunctionArgumentArray)
+{
+    const std::string &shaderString =
+        "#version 310 es\n"
+        "precision mediump float;\n"
+        "precision mediump image2D;\n"
+        "layout(rgba32f) uniform readonly image2D myImage[2];\n"
+        "void myFunc(in image2D someImage) {}\n"
+        "void main() {\n"
+        "   myFunc(myImage[0]);\n"
+        "}\n";
+
+    if (compile(shaderString))
+    {
+        FAIL() << "Shader compilation succeeded, expecting failure:\n" << mInfoLog;
+    }
+}
+
+// Passing an image qualifier to a function should not be able to discard the readonly qualifier.
+// Test that validation doesn't crash on this for an image in a struct.
+// Image in a struct in itself isn't accepted by the parser, but error recovery still results in
+// an image in the struct.
+// GLSL ES 3.10 Revision 4, 4.9 Memory Access Qualifiers
+TEST_F(FragmentShaderValidationTest, ReadOnlyQualifierMissingInFunctionArgumentStruct)
+{
+    const std::string &shaderString =
+        "#version 310 es\n"
+        "precision mediump float;\n"
+        "precision mediump image2D;\n"
+        "uniform struct S {\n"
+        "    layout(r32f) readonly image2D myImage;\n"
+        "} s;\n"
+        "void myFunc(in image2D someImage) {}\n"
+        "void main() {\n"
+        "   myFunc(s.myImage);\n"
+        "}\n";
+
+    if (compile(shaderString))
+    {
+        FAIL() << "Shader compilation succeeded, expecting failure:\n" << mInfoLog;
+    }
+}
+
 // Passing an image qualifier to a function should not be able to discard the writeonly qualifier.
 // GLSL ES 3.10 Revision 4, 4.9 Memory Access Qualifiers
 TEST_F(FragmentShaderValidationTest, WriteOnlyQualifierMissingInFunctionArgument)

src/tests/deqp_support/deqp_gles31_test_expectations.txt

@@ -28,20 +28,6 @@
 1659 NVIDIA OPENGL : dEQP-GLES31.functional.draw_indirect.random.10 = SKIP
 1659 NVIDIA OPENGL : dEQP-GLES31.functional.draw_indirect.random.15 = SKIP
 1659 NVIDIA OPENGL : dEQP-GLES31.functional.draw_indirect.random.18 = SKIP
-1442 OPENGL D3D11 : dEQP-GLES31.functional.layout_binding.image.image2d.vertex_binding_array = SKIP
-1442 OPENGL D3D11 : dEQP-GLES31.functional.layout_binding.image.image2d.vertex_binding_max_array = SKIP
-1442 OPENGL D3D11 : dEQP-GLES31.functional.layout_binding.image.image2d.fragment_binding_array = SKIP
-1442 OPENGL D3D11 : dEQP-GLES31.functional.layout_binding.image.image2d.fragment_binding_max_array = SKIP
-1442 OPENGL D3D11 : dEQP-GLES31.functional.layout_binding.image.image3d.vertex_binding_array = SKIP
-1442 OPENGL D3D11 : dEQP-GLES31.functional.layout_binding.image.image3d.vertex_binding_max_array = SKIP
-1442 OPENGL D3D11 : dEQP-GLES31.functional.layout_binding.image.image3d.fragment_binding_array = SKIP
-1442 OPENGL D3D11 : dEQP-GLES31.functional.layout_binding.image.image3d.fragment_binding_max_array = SKIP
-1442 OPENGL D3D11 : dEQP-GLES31.functional.layout_binding.negative.image.image2d.vertex_binding_over_max_array = SKIP
-1442 OPENGL D3D11 : dEQP-GLES31.functional.layout_binding.negative.image.image2d.fragment_binding_over_max_array = SKIP
-1442 OPENGL D3D11 : dEQP-GLES31.functional.layout_binding.negative.image.image2d.binding_contradictory_array = SKIP
-1442 OPENGL D3D11 : dEQP-GLES31.functional.layout_binding.negative.image.image3d.vertex_binding_over_max_array = SKIP
-1442 OPENGL D3D11 : dEQP-GLES31.functional.layout_binding.negative.image.image3d.fragment_binding_over_max_array = SKIP
-1442 OPENGL D3D11 : dEQP-GLES31.functional.layout_binding.negative.image.image3d.binding_contradictory_array = SKIP
 1442 OPENGL D3D11 : dEQP-GLES31.functional.program_interface_query.transform_feedback_varying.resource_list.vertex_fragment.builtin_gl_position = SKIP
 1442 OPENGL D3D11 : dEQP-GLES31.functional.program_interface_query.transform_feedback_varying.resource_list.vertex_fragment.default_block_struct_member = SKIP
 1442 OPENGL D3D11 : dEQP-GLES31.functional.program_interface_query.transform_feedback_varying.array_size.vertex_fragment.default_block_struct_member = SKIP