Improve handling of declarator lists with empty declarations
The code previously failed to check for correctness of layout qualifiers
in case a declarator followed an empty declaration, like so:
layout(packed) uniform float, a;
Fix this by running all necessary declaration checks also for declarators
which follow an empty declaration.
structQualifierErrorCheck is merged into singleDeclarationErrorCheck.
TEST=angle_unittests, WebGL conformance tests
BUG=angleproject:969
Change-Id: Idcb0673e3bcf64087744ff0d260f51a7546f024a
Reviewed-on: https://chromium-review.googlesource.com/264812
Tested-by: Olli Etuaho <oetuaho@nvidia.com>
Reviewed-by: Geoff Lang <geofflang@chromium.org>
diff --git a/src/compiler/translator/ParseContext.cpp b/src/compiler/translator/ParseContext.cpp
index b543ea2..e9b392e 100644
--- a/src/compiler/translator/ParseContext.cpp
+++ b/src/compiler/translator/ParseContext.cpp
@@ -626,30 +626,6 @@
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 @@
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 @@
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);
- if (identifier != "")
+ mDeferredSingleDeclarationErrorCheck = (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::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::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::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::parseArrayDeclarator(TPublicType &publicType, const TSourceLoc& identifierLocation, const TString &identifier, const TSourceLoc& arrayLocation, TIntermNode *declaratorList, TIntermTyped *indexExpression)
{
- if (structQualifierErrorCheck(identifierLocation, publicType))
- recover();
+ // 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::parseInitDeclarator(TPublicType &publicType, TIntermAggregate *declaratorList, const TSourceLoc& identifierLocation, const TString &identifier, const TSourceLoc& initLocation, TIntermTyped *initializer)
{
- if (structQualifierErrorCheck(identifierLocation, publicType))
- recover();
+ // 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();
diff --git a/src/compiler/translator/ParseContext.h b/src/compiler/translator/ParseContext.h
index 330685c..6c071b8 100644
--- a/src/compiler/translator/ParseContext.h
+++ b/src/compiler/translator/ParseContext.h
@@ -45,7 +45,10 @@
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 @@
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 @@
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 @@
// 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[],
diff --git a/src/tests/compiler_tests/MalformedShader_test.cpp b/src/tests/compiler_tests/MalformedShader_test.cpp
index 726000f..dd0d4f1 100644
--- a/src/tests/compiler_tests/MalformedShader_test.cpp
+++ b/src/tests/compiler_tests/MalformedShader_test.cpp
@@ -277,3 +277,39 @@
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;
+ }
+}