Improve handling of declarator lists with empty declarations

src/compiler/translator/ParseContext.cpp

@@ -626,30 +626,6 @@ bool TParseContext::samplerErrorCheck(const TSourceLoc& line, const TPublicType&
     return false;
 }
 
-bool TParseContext::structQualifierErrorCheck(const TSourceLoc& line, const TPublicType& pType)
-{
-    switch (pType.qualifier)
-    {
-      case EvqVaryingIn:
-      case EvqVaryingOut:
-      case EvqAttribute:
-      case EvqVertexIn:
-      case EvqFragmentOut:
-        if (pType.type == EbtStruct)
-        {
-            error(line, "cannot be used with a structure", getQualifierString(pType.qualifier));
-            return true;
-        }
-
-      default: break;
-    }
-
-    if (pType.qualifier != EvqUniform && samplerErrorCheck(line, pType, "samplers must be uniform"))
-        return true;
-
-    return false;
-}
-
 bool TParseContext::locationDeclaratorListCheck(const TSourceLoc& line, const TPublicType &pType)
 {
     if (pType.layoutQualifier.location != -1)
@@ -899,27 +875,53 @@ bool TParseContext::extensionErrorCheck(const TSourceLoc& line, const TString& e
     return false;
 }
 
-bool TParseContext::singleDeclarationErrorCheck(TPublicType &publicType, const TSourceLoc& identifierLocation, const TString &identifier)
+// These checks are common for all declarations starting a declarator list, and declarators that follow an empty
+// declaration.
+//
+bool TParseContext::singleDeclarationErrorCheck(TPublicType &publicType, const TSourceLoc &identifierLocation)
 {
-    if (structQualifierErrorCheck(identifierLocation, publicType))
+    switch (publicType.qualifier)
+    {
+      case EvqVaryingIn:
+      case EvqVaryingOut:
+      case EvqAttribute:
+      case EvqVertexIn:
+      case EvqFragmentOut:
+        if (publicType.type == EbtStruct)
+        {
+            error(identifierLocation, "cannot be used with a structure",
+                  getQualifierString(publicType.qualifier));
             return true;
+        }
+
+      default: break;
+    }
+
+    if (publicType.qualifier != EvqUniform && samplerErrorCheck(identifierLocation, publicType,
+                                                                "samplers must be uniform"))
+    {
+        return true;
+    }
 
     // check for layout qualifier issues
     const TLayoutQualifier layoutQualifier = publicType.layoutQualifier;
 
     if (layoutQualifier.matrixPacking != EmpUnspecified)
     {
-        error(identifierLocation, "layout qualifier", getMatrixPackingString(layoutQualifier.matrixPacking), "only valid for interface blocks");
+        error(identifierLocation, "layout qualifier", getMatrixPackingString(layoutQualifier.matrixPacking),
+              "only valid for interface blocks");
         return true;
     }
 
     if (layoutQualifier.blockStorage != EbsUnspecified)
     {
-        error(identifierLocation, "layout qualifier", getBlockStorageString(layoutQualifier.blockStorage), "only valid for interface blocks");
+        error(identifierLocation, "layout qualifier", getBlockStorageString(layoutQualifier.blockStorage),
+              "only valid for interface blocks");
         return true;
     }
 
-    if (publicType.qualifier != EvqVertexIn && publicType.qualifier != EvqFragmentOut && layoutLocationErrorCheck(identifierLocation, publicType.layoutQualifier))
+    if (publicType.qualifier != EvqVertexIn && publicType.qualifier != EvqFragmentOut &&
+        layoutLocationErrorCheck(identifierLocation, publicType.layoutQualifier))
     {
         return true;
     }
@@ -1229,21 +1231,25 @@ TPublicType TParseContext::addFullySpecifiedType(TQualifier qualifier, TLayoutQu
     return returnType;
 }
 
-TIntermAggregate* TParseContext::parseSingleDeclaration(TPublicType &publicType, const TSourceLoc& identifierLocation, const TString &identifier)
+TIntermAggregate *TParseContext::parseSingleDeclaration(TPublicType &publicType,
+                                                        const TSourceLoc &identifierOrTypeLocation,
+                                                        const TString &identifier)
 {
-    TIntermSymbol* symbol = intermediate.addSymbol(0, identifier, TType(publicType), identifierLocation);
-    TIntermAggregate* aggregate = intermediate.makeAggregate(symbol, identifierLocation);
+    TIntermSymbol *symbol = intermediate.addSymbol(0, identifier, TType(publicType), identifierOrTypeLocation);
+    TIntermAggregate *aggregate = intermediate.makeAggregate(symbol, identifierOrTypeLocation);
+
+    mDeferredSingleDeclarationErrorCheck = (identifier == "");
 
-    if (identifier != "")
+    if (!mDeferredSingleDeclarationErrorCheck)
     {
-        if (singleDeclarationErrorCheck(publicType, identifierLocation, identifier))
+        if (singleDeclarationErrorCheck(publicType, identifierOrTypeLocation))
             recover();
 
-        if (nonInitConstErrorCheck(identifierLocation, identifier, &publicType))
+        if (nonInitConstErrorCheck(identifierOrTypeLocation, identifier, &publicType))
             recover();
 
         TVariable *variable = nullptr;
-        if (!declareVariable(identifierLocation, identifier, TType(publicType), &variable))
+        if (!declareVariable(identifierOrTypeLocation, identifier, TType(publicType), &variable))
             recover();
 
         if (variable && symbol)
@@ -1257,7 +1263,9 @@ TIntermAggregate* TParseContext::parseSingleDeclaration(TPublicType &publicType,
 
 TIntermAggregate* TParseContext::parseSingleArrayDeclaration(TPublicType &publicType, const TSourceLoc& identifierLocation, const TString &identifier, const TSourceLoc& indexLocation, TIntermTyped *indexExpression)
 {
-    if (singleDeclarationErrorCheck(publicType, identifierLocation, identifier))
+    mDeferredSingleDeclarationErrorCheck = false;
+
+    if (singleDeclarationErrorCheck(publicType, identifierLocation))
         recover();
 
     if (nonInitConstErrorCheck(identifierLocation, identifier, &publicType))
@@ -1297,7 +1305,9 @@ TIntermAggregate* TParseContext::parseSingleArrayDeclaration(TPublicType &public
 
 TIntermAggregate* TParseContext::parseSingleInitDeclaration(TPublicType &publicType, const TSourceLoc& identifierLocation, const TString &identifier, const TSourceLoc& initLocation, TIntermTyped *initializer)
 {
-    if (singleDeclarationErrorCheck(publicType, identifierLocation, identifier))
+    mDeferredSingleDeclarationErrorCheck = false;
+
+    if (singleDeclarationErrorCheck(publicType, identifierLocation))
         recover();
 
     TIntermNode* intermNode;
@@ -1356,12 +1366,17 @@ TIntermAggregate* TParseContext::parseInvariantDeclaration(const TSourceLoc &inv
 
 TIntermAggregate* TParseContext::parseDeclarator(TPublicType &publicType, TIntermAggregate *aggregateDeclaration, TSymbol *identifierSymbol, const TSourceLoc& identifierLocation, const TString &identifier)
 {
+    // If the declaration starting this declarator list was empty (example: int,), some checks were not performed.
+    if (mDeferredSingleDeclarationErrorCheck)
+    {
+        if (singleDeclarationErrorCheck(publicType, identifierLocation))
+            recover();
+        mDeferredSingleDeclarationErrorCheck = false;
+    }
+
     TIntermSymbol* symbol = intermediate.addSymbol(0, identifier, TType(publicType), identifierLocation);
     TIntermAggregate* intermAggregate = intermediate.growAggregate(aggregateDeclaration, symbol, identifierLocation);
 
-    if (structQualifierErrorCheck(identifierLocation, publicType))
-        recover();
-
     if (locationDeclaratorListCheck(identifierLocation, publicType))
         recover();
 
@@ -1379,8 +1394,13 @@ TIntermAggregate* TParseContext::parseDeclarator(TPublicType &publicType, TInter
 
 TIntermAggregate* TParseContext::parseArrayDeclarator(TPublicType &publicType, const TSourceLoc& identifierLocation, const TString &identifier, const TSourceLoc& arrayLocation, TIntermNode *declaratorList, TIntermTyped *indexExpression)
 {
-    if (structQualifierErrorCheck(identifierLocation, publicType))
+    // If the declaration starting this declarator list was empty (example: int,), some checks were not performed.
+    if (mDeferredSingleDeclarationErrorCheck)
+    {
+        if (singleDeclarationErrorCheck(publicType, identifierLocation))
             recover();
+        mDeferredSingleDeclarationErrorCheck = false;
+    }
 
     if (locationDeclaratorListCheck(identifierLocation, publicType))
         recover();
@@ -1416,8 +1436,13 @@ TIntermAggregate* TParseContext::parseArrayDeclarator(TPublicType &publicType, c
 
 TIntermAggregate* TParseContext::parseInitDeclarator(TPublicType &publicType, TIntermAggregate *declaratorList, const TSourceLoc& identifierLocation, const TString &identifier, const TSourceLoc& initLocation, TIntermTyped *initializer)
 {
-    if (structQualifierErrorCheck(identifierLocation, publicType))
+    // If the declaration starting this declarator list was empty (example: int,), some checks were not performed.
+    if (mDeferredSingleDeclarationErrorCheck)
+    {
+        if (singleDeclarationErrorCheck(publicType, identifierLocation))
             recover();
+        mDeferredSingleDeclarationErrorCheck = false;
+    }
 
     if (locationDeclaratorListCheck(identifierLocation, publicType))
         recover();

src/compiler/translator/ParseContext.h

@@ -45,7 +45,10 @@ struct TParseContext {
             shaderVersion(100),
             directiveHandler(ext, diagnostics, shaderVersion, debugShaderPrecisionSupported),
             preprocessor(&diagnostics, &directiveHandler),
-            scanner(NULL) {  }
+            scanner(NULL),
+            mDeferredSingleDeclarationErrorCheck(false)
+    {
+    }
     TIntermediate& intermediate; // to hold and build a parse tree
     TSymbolTable& symbolTable;   // symbol table that goes with the language currently being parsed
     sh::GLenum shaderType;              // vertex or fragment language (future: pack or unpack)
@@ -101,13 +104,12 @@ struct TParseContext {
     bool boolErrorCheck(const TSourceLoc&, const TIntermTyped*);
     bool boolErrorCheck(const TSourceLoc&, const TPublicType&);
     bool samplerErrorCheck(const TSourceLoc& line, const TPublicType& pType, const char* reason);
-    bool structQualifierErrorCheck(const TSourceLoc& line, const TPublicType& pType);
     bool locationDeclaratorListCheck(const TSourceLoc& line, const TPublicType &pType);
     bool parameterSamplerErrorCheck(const TSourceLoc& line, TQualifier qualifier, const TType& type);
     bool nonInitConstErrorCheck(const TSourceLoc &line, const TString &identifier, TPublicType *type);
     bool paramErrorCheck(const TSourceLoc& line, TQualifier qualifier, TQualifier paramQualifier, TType* type);
     bool extensionErrorCheck(const TSourceLoc& line, const TString&);
-    bool singleDeclarationErrorCheck(TPublicType &publicType, const TSourceLoc& identifierLocation, const TString &identifier);
+    bool singleDeclarationErrorCheck(TPublicType &publicType, const TSourceLoc &identifierLocation);
     bool layoutLocationErrorCheck(const TSourceLoc& location, const TLayoutQualifier &layoutQualifier);
     bool functionCallLValueErrorCheck(const TFunction *fnCandidate, TIntermAggregate *);
 
@@ -126,7 +128,8 @@ struct TParseContext {
 
     TPublicType addFullySpecifiedType(TQualifier qualifier, const TPublicType& typeSpecifier);
     TPublicType addFullySpecifiedType(TQualifier qualifier, TLayoutQualifier layoutQualifier, const TPublicType& typeSpecifier);
-    TIntermAggregate* parseSingleDeclaration(TPublicType &publicType, const TSourceLoc& identifierLocation, const TString &identifier);
+    TIntermAggregate *parseSingleDeclaration(TPublicType &publicType,
+                                             const TSourceLoc &identifierOrTypeLocation, const TString &identifier);
     TIntermAggregate* parseSingleArrayDeclaration(TPublicType &publicType, const TSourceLoc& identifierLocation, const TString &identifier, const TSourceLoc& indexLocation, TIntermTyped *indexExpression);
     TIntermAggregate* parseSingleInitDeclaration(TPublicType &publicType, const TSourceLoc& identifierLocation, const TString &identifier, const TSourceLoc& initLocation, TIntermTyped *initializer);
     TIntermAggregate* parseInvariantDeclaration(const TSourceLoc &invariantLoc, const TSourceLoc &identifierLoc, const TString *identifier, const TSymbol *symbol);
@@ -199,6 +202,9 @@ struct TParseContext {
     // Return true if the checks pass
     bool binaryOpCommonCheck(TOperator op, TIntermTyped *left, TIntermTyped *right,
         const TSourceLoc &loc);
+
+    // Set to true when the last/current declarator list was started with an empty declaration.
+    bool mDeferredSingleDeclarationErrorCheck;
 };
 
 int PaParseStrings(size_t count, const char* const string[], const int length[],

src/tests/compiler_tests/MalformedShader_test.cpp

@@ -277,3 +277,39 @@ TEST_F(MalformedShaderTest, ConstArrayNotInitialized)
         FAIL() << "Shader compilation succeeded, expecting failure " << mInfoLog;
     }
 }
+
+// Block layout qualifiers can't be used on non-block uniforms (ESSL 3.00 section 4.3.8.3)
+TEST_F(MalformedShaderTest, BlockLayoutQualifierOnRegularUniform)
+{
+    const std::string &shaderString =
+        "#version 300 es\n"
+        "precision mediump float;\n"
+        "layout(packed) uniform mat2 x;\n"
+        "out vec4 my_FragColor;\n"
+        "void main() {\n"
+        "   my_FragColor = vec4(1.0);\n"
+        "}\n";
+    if (compile(shaderString))
+    {
+        FAIL() << "Shader compilation succeeded, expecting failure " << mInfoLog;
+    }
+}
+
+// Block layout qualifiers can't be used on non-block uniforms (ESSL 3.00 section 4.3.8.3)
+TEST_F(MalformedShaderTest, BlockLayoutQualifierOnUniformWithEmptyDecl)
+{
+    // Yes, the comma in the declaration below is not a typo.
+    // Empty declarations are allowed in GLSL.
+    const std::string &shaderString =
+        "#version 300 es\n"
+        "precision mediump float;\n"
+        "layout(packed) uniform mat2, x;\n"
+        "out vec4 my_FragColor;\n"
+        "void main() {\n"
+        "   my_FragColor = vec4(1.0);\n"
+        "}\n";
+    if (compile(shaderString))
+    {
+        FAIL() << "Shader compilation succeeded, expecting failure " << mInfoLog;
+    }
+}