Unify opaque type validation in GLSL parsing

src/compiler/translator/ParseContext.cpp

@@ -35,7 +35,7 @@ bool ContainsSampler(const TType &type)
     if (IsSampler(type.getBasicType()))
         return true;
 
-    if (type.getBasicType() == EbtStruct || type.isInterfaceBlock())
+    if (type.getBasicType() == EbtStruct)
     {
         const TFieldList &fields = type.getStruct()->fields();
         for (unsigned int i = 0; i < fields.size(); ++i)
@@ -48,24 +48,6 @@ bool ContainsSampler(const TType &type)
     return false;
 }
 
-bool ContainsImage(const TType &type)
-{
-    if (IsImage(type.getBasicType()))
-        return true;
-
-    if (type.getBasicType() == EbtStruct || type.isInterfaceBlock())
-    {
-        const TFieldList &fields = type.getStruct()->fields();
-        for (unsigned int i = 0; i < fields.size(); ++i)
-        {
-            if (ContainsImage(*fields[i]->type()))
-                return true;
-        }
-    }
-
-    return false;
-}
-
 // Get a token from an image argument to use as an error message token.
 const char *GetImageArgumentToken(TIntermTyped *imageNode)
 {
@@ -340,14 +322,9 @@ void TParseContext::checkPrecisionSpecified(const TSourceLoc &line,
                 error(line, "No precision specified (int)", "");
                 return;
             default:
-                if (IsSampler(type))
-                {
-                    error(line, "No precision specified (sampler)", "");
-                    return;
-                }
-                if (IsImage(type))
+                if (IsOpaqueType(type))
                 {
-                    error(line, "No precision specified (image)", "");
+                    error(line, "No precision specified", getBasicString(type));
                     return;
                 }
         }
@@ -389,7 +366,7 @@ bool TParseContext::checkCanBeLValue(const TSourceLoc &line, const char *op, TIn
         return false;
     }
 
-    const char *message = 0;
+    std::string message;
     switch (node->getQualifier())
     {
         case EvqConst:
@@ -454,17 +431,14 @@ bool TParseContext::checkCanBeLValue(const TSourceLoc &line, const char *op, TIn
             {
                 message = "can't modify void";
             }
-            if (IsSampler(node->getBasicType()))
-            {
-                message = "can't modify a sampler";
-            }
-            if (IsImage(node->getBasicType()))
+            if (IsOpaqueType(node->getBasicType()))
             {
-                message = "can't modify an image";
+                message = "can't modify a variable with type ";
+                message += getBasicString(node->getBasicType());
             }
     }
 
-    if (message == 0 && binaryNode == 0 && symNode == 0)
+    if (message.empty() && binaryNode == 0 && symNode == 0)
     {
         error(line, "l-value required", op);
 
@@ -474,7 +448,7 @@ bool TParseContext::checkCanBeLValue(const TSourceLoc &line, const char *op, TIn
     //
     // Everything else is okay, no error.
     //
-    if (message == 0)
+    if (message.empty())
         return true;
 
     //
@@ -686,14 +660,11 @@ bool TParseContext::checkConstructorArguments(const TSourceLoc &line,
     {
         TIntermTyped *argTyped = argNode->getAsTyped();
         ASSERT(argTyped != nullptr);
-        if (op != EOpConstructStruct && IsSampler(argTyped->getBasicType()))
+        if (op != EOpConstructStruct && IsOpaqueType(argTyped->getBasicType()))
         {
-            error(line, "cannot convert a sampler", "constructor");
-            return false;
-        }
-        if (op != EOpConstructStruct && IsImage(argTyped->getBasicType()))
-        {
-            error(line, "cannot convert an image", "constructor");
+            std::string reason("cannot convert a variable with type ");
+            reason += getBasicString(argTyped->getBasicType());
+            error(line, reason.c_str(), "constructor");
             return false;
         }
         if (argTyped->getBasicType() == EbtVoid)
@@ -780,9 +751,9 @@ void TParseContext::checkIsScalarBool(const TSourceLoc &line, const TPublicType
     }
 }
 
-bool TParseContext::checkIsNotSampler(const TSourceLoc &line,
-                                      const TTypeSpecifierNonArray &pType,
-                                      const char *reason)
+bool TParseContext::checkIsNotOpaqueType(const TSourceLoc &line,
+                                         const TTypeSpecifierNonArray &pType,
+                                         const char *reason)
 {
     if (pType.type == EbtStruct)
     {
@@ -794,10 +765,11 @@ bool TParseContext::checkIsNotSampler(const TSourceLoc &line,
             error(line, reasonStr.c_str(), getBasicString(pType.type));
             return false;
         }
-
+        // only samplers need to be checked from structs, since other opaque types can't be struct
+        // members.
         return true;
     }
-    else if (IsSampler(pType.type))
+    else if (IsOpaqueType(pType.type))
     {
         error(line, reason, getBasicString(pType.type));
         return false;
@@ -806,34 +778,6 @@ bool TParseContext::checkIsNotSampler(const TSourceLoc &line,
     return true;
 }
 
-bool TParseContext::checkIsNotImage(const TSourceLoc &line,
-                                    const TTypeSpecifierNonArray &pType,
-                                    const char *reason)
-{
-    if (pType.type == EbtStruct)
-    {
-        if (ContainsImage(*pType.userDef))
-        {
-            std::stringstream reasonStream;
-            reasonStream << reason << " (structure contains an image)";
-            std::string reasonStr = reasonStream.str();
-            error(line, reasonStr.c_str(), getBasicString(pType.type));
-
-            return false;
-        }
-
-        return true;
-    }
-    else if (IsImage(pType.type))
-    {
-        error(line, reason, getBasicString(pType.type));
-
-        return false;
-    }
-
-    return true;
-}
-
 void TParseContext::checkDeclaratorLocationIsNotSpecified(const TSourceLoc &line,
                                                           const TPublicType &pType)
 {
@@ -863,29 +807,10 @@ void TParseContext::checkOutParameterIsNotOpaqueType(const TSourceLoc &line,
                                                      TQualifier qualifier,
                                                      const TType &type)
 {
-    checkOutParameterIsNotSampler(line, qualifier, type);
-    checkOutParameterIsNotImage(line, qualifier, type);
-}
-
-void TParseContext::checkOutParameterIsNotSampler(const TSourceLoc &line,
-                                                  TQualifier qualifier,
-                                                  const TType &type)
-{
     ASSERT(qualifier == EvqOut || qualifier == EvqInOut);
-    if (IsSampler(type.getBasicType()))
+    if (IsOpaqueType(type.getBasicType()))
     {
-        error(line, "samplers cannot be output parameters", type.getBasicString());
-    }
-}
-
-void TParseContext::checkOutParameterIsNotImage(const TSourceLoc &line,
-                                                TQualifier qualifier,
-                                                const TType &type)
-{
-    ASSERT(qualifier == EvqOut || qualifier == EvqInOut);
-    if (IsImage(type.getBasicType()))
-    {
-        error(line, "images cannot be output parameters", type.getBasicString());
+        error(line, "opaque types cannot be output parameters", type.getBasicString());
     }
 }
 
@@ -1227,16 +1152,10 @@ void TParseContext::nonEmptyDeclarationErrorCheck(const TPublicType &publicType,
         default:
             break;
     }
-
-    if (publicType.qualifier != EvqUniform &&
-        !checkIsNotSampler(identifierLocation, publicType.typeSpecifierNonArray,
-                           "samplers must be uniform"))
-    {
-        return;
-    }
+    std::string reason(getBasicString(publicType.getBasicType()));
+    reason += "s must be uniform";
     if (publicType.qualifier != EvqUniform &&
-        !checkIsNotImage(identifierLocation, publicType.typeSpecifierNonArray,
-                         "images must be uniform"))
+        !checkIsNotOpaqueType(identifierLocation, publicType.typeSpecifierNonArray, reason.c_str()))
     {
         return;
     }
@@ -2757,10 +2676,10 @@ TFunction *TParseContext::parseFunctionHeader(const TPublicType &type,
     {
         error(location, "no qualifiers allowed for function return", "layout");
     }
-    // make sure a sampler or an image is not involved as well...
-    checkIsNotSampler(location, type.typeSpecifierNonArray,
-                      "samplers can't be function return values");
-    checkIsNotImage(location, type.typeSpecifierNonArray, "images can't be function return values");
+    // make sure an opaque type is not involved as well...
+    std::string reason(getBasicString(type.getBasicType()));
+    reason += "s can't be function return values";
+    checkIsNotOpaqueType(location, type.typeSpecifierNonArray, reason.c_str());
     if (mShaderVersion < 300)
     {
         // Array return values are forbidden, but there's also no valid syntax for declaring array
@@ -2911,18 +2830,12 @@ TIntermDeclaration *TParseContext::addInterfaceBlock(
     {
         TField *field    = (*fieldList)[memberIndex];
         TType *fieldType = field->type();
-        if (IsSampler(fieldType->getBasicType()))
+        if (IsOpaqueType(fieldType->getBasicType()))
         {
-            error(field->line(),
-                  "unsupported type - sampler types are not allowed in interface blocks",
-                  fieldType->getBasicString());
-        }
-
-        if (IsImage(fieldType->getBasicType()))
-        {
-            error(field->line(),
-                  "unsupported type - image types are not allowed in interface blocks",
-                  fieldType->getBasicString());
+            std::string reason("unsupported type - ");
+            reason += fieldType->getBasicString();
+            reason += " types are not allowed in interface blocks";
+            error(field->line(), reason.c_str(), fieldType->getBasicString());
         }
 
         const TQualifier qualifier = fieldType->getQualifier();
@@ -3856,6 +3769,9 @@ bool TParseContext::binaryOpCommonCheck(TOperator op,
                                         TIntermTyped *right,
                                         const TSourceLoc &loc)
 {
+    // TODO(jie.a.chen@intel.com): Validate opaque type variables can only be operands in array
+    // indexing, structure member selection, and parentheses expressions.
+
     if (left->getType().getStruct() || right->getType().getStruct())
     {
         switch (op)
@@ -3979,13 +3895,6 @@ bool TParseContext::binaryOpCommonCheck(TOperator op,
                 return false;
             }
 
-            if ((op == EOpAssign || op == EOpInitialize) &&
-                left->getType().isStructureContainingImages())
-            {
-                error(loc, "undefined operation for structs containing images",
-                      GetOperatorString(op));
-                return false;
-            }
             if ((left->getNominalSize() != right->getNominalSize()) ||
                 (left->getSecondarySize() != right->getSecondarySize()))
             {

src/compiler/translator/ParseContext.h

@@ -123,12 +123,9 @@ class TParseContext : angle::NonCopyable
     bool checkIsNonVoid(const TSourceLoc &line, const TString &identifier, const TBasicType &type);
     void checkIsScalarBool(const TSourceLoc &line, const TIntermTyped *type);
     void checkIsScalarBool(const TSourceLoc &line, const TPublicType &pType);
-    bool checkIsNotSampler(const TSourceLoc &line,
-                           const TTypeSpecifierNonArray &pType,
-                           const char *reason);
-    bool checkIsNotImage(const TSourceLoc &line,
-                         const TTypeSpecifierNonArray &pType,
-                         const char *reason);
+    bool checkIsNotOpaqueType(const TSourceLoc &line,
+                              const TTypeSpecifierNonArray &pType,
+                              const char *reason);
     void checkDeclaratorLocationIsNotSpecified(const TSourceLoc &line, const TPublicType &pType);
     void checkLocationIsNotSpecified(const TSourceLoc &location,
                                      const TLayoutQualifier &layoutQualifier);
@@ -384,15 +381,9 @@ class TParseContext : angle::NonCopyable
     // Assumes that multiplication op has already been set based on the types.
     bool isMultiplicationTypeCombinationValid(TOperator op, const TType &left, const TType &right);
 
-    void checkOutParameterIsNotImage(const TSourceLoc &line,
-                                     TQualifier qualifier,
-                                     const TType &type);
     void checkOutParameterIsNotOpaqueType(const TSourceLoc &line,
                                           TQualifier qualifier,
                                           const TType &type);
-    void checkOutParameterIsNotSampler(const TSourceLoc &line,
-                                       TQualifier qualifier,
-                                       const TType &type);
 
     void checkInternalFormatIsNotSpecified(const TSourceLoc &location,
                                            TLayoutImageInternalFormat internalFormat);

src/compiler/translator/Types.cpp

@@ -526,17 +526,6 @@ bool TStructure::containsSamplers() const
     return false;
 }
 
-bool TStructure::containsImages() const
-{
-    for (size_t i = 0; i < mFields->size(); ++i)
-    {
-        const TType *fieldType = (*mFields)[i]->type();
-        if (IsImage(fieldType->getBasicType()) || fieldType->isStructureContainingImages())
-            return true;
-    }
-    return false;
-}
-
 void TStructure::createSamplerSymbols(const TString &structName,
                                       const TString &structAPIName,
                                       const unsigned int arrayOfStructsSize,

src/compiler/translator/Types.h

@@ -98,7 +98,6 @@ class TStructure : public TFieldListCollection
     bool containsArrays() const;
     bool containsType(TBasicType t) const;
     bool containsSamplers() const;
-    bool containsImages() const;
 
     void createSamplerSymbols(const TString &structName,
                               const TString &structAPIName,
@@ -468,11 +467,6 @@ class TType
         return structure ? structure->containsSamplers() : false;
     }
 
-    bool isStructureContainingImages() const
-    {
-        return structure ? structure->containsImages() : false;
-    }
-
     void createSamplerSymbols(const TString &structName,
                               const TString &structAPIName,
                               const unsigned int arrayOfStructsSize,

src/tests/compiler_tests/ShaderValidation_test.cpp

@@ -3762,3 +3762,20 @@ TEST_F(FragmentShaderValidationTest, UniformLocationEmptyDeclaration)
         FAIL() << "Shader compilation succeeded, expecting failure:\n" << mInfoLog;
     }
 }
+
+// Test function parameters of opaque type can't be l-value too.
+TEST_F(FragmentShaderValidationTest, OpaqueParameterCanNotBeLValue)
+{
+    const std::string &shaderString =
+        "#version 310 es\n"
+        "uniform sampler2D s;\n"
+        "void foo(sampler2D as) {\n"
+        "    as = s;\n"
+        "}\n"
+        "void main() {}\n";
+
+    if (compile(shaderString))
+    {
+        FAIL() << "Shader compilation succeeded, expecting failure:\n" << mInfoLog;
+    }
+}