Commit a2d98141 by Olli Etuaho Committed by Commit Bot

Fix allowing non-constant ternary global initializer

Check the qualifier of a node resulting from the folding of a ternary node correctly. The folded node might even be a TIntermConstantUnion with a non-constant qualifier. BUG=angleproject:2285 TEST=angle_unittests Change-Id: I74516e44ce9d78bc54093a5b58d14cf33a57e6e5 Reviewed-on: https://chromium-review.googlesource.com/829138Reviewed-by: 's avatarCorentin Wallez <cwallez@chromium.org> Reviewed-by: 's avatarJamie Madill <jmadill@chromium.org> Commit-Queue: Olli Etuaho <oetuaho@nvidia.com>
parent 647dca76
...@@ -1933,7 +1933,7 @@ bool TParseContext::executeInitializer(const TSourceLoc &line, ...@@ -1933,7 +1933,7 @@ bool TParseContext::executeInitializer(const TSourceLoc &line,
bool globalInitWarning = false; bool globalInitWarning = false;
if (symbolTable.atGlobalLevel() && if (symbolTable.atGlobalLevel() &&
!ValidateGlobalInitializer(initializer, this, &globalInitWarning)) !ValidateGlobalInitializer(initializer, mShaderVersion, &globalInitWarning))
{ {
// Error message does not completely match behavior with ESSL 1.00, but // Error message does not completely match behavior with ESSL 1.00, but
// we want to steer developers towards only using constant expressions. // we want to steer developers towards only using constant expressions.
......
...@@ -7,7 +7,6 @@ ...@@ -7,7 +7,6 @@
#include "compiler/translator/ValidateGlobalInitializer.h" #include "compiler/translator/ValidateGlobalInitializer.h"
#include "compiler/translator/IntermTraverse.h" #include "compiler/translator/IntermTraverse.h"
#include "compiler/translator/ParseContext.h"
namespace sh namespace sh
{ {
...@@ -18,9 +17,10 @@ namespace ...@@ -18,9 +17,10 @@ namespace
class ValidateGlobalInitializerTraverser : public TIntermTraverser class ValidateGlobalInitializerTraverser : public TIntermTraverser
{ {
public: public:
ValidateGlobalInitializerTraverser(const TParseContext *context); ValidateGlobalInitializerTraverser(int shaderVersion);
void visitSymbol(TIntermSymbol *node) override; void visitSymbol(TIntermSymbol *node) override;
void visitConstantUnion(TIntermConstantUnion *node) override;
bool visitAggregate(Visit visit, TIntermAggregate *node) override; bool visitAggregate(Visit visit, TIntermAggregate *node) override;
bool visitBinary(Visit visit, TIntermBinary *node) override; bool visitBinary(Visit visit, TIntermBinary *node) override;
bool visitUnary(Visit visit, TIntermUnary *node) override; bool visitUnary(Visit visit, TIntermUnary *node) override;
...@@ -29,42 +29,59 @@ class ValidateGlobalInitializerTraverser : public TIntermTraverser ...@@ -29,42 +29,59 @@ class ValidateGlobalInitializerTraverser : public TIntermTraverser
bool issueWarning() const { return mIssueWarning; } bool issueWarning() const { return mIssueWarning; }
private: private:
const TParseContext *mContext; int mShaderVersion;
bool mIsValid; bool mIsValid;
bool mIssueWarning; bool mIssueWarning;
}; };
void ValidateGlobalInitializerTraverser::visitSymbol(TIntermSymbol *node) void ValidateGlobalInitializerTraverser::visitSymbol(TIntermSymbol *node)
{ {
const TSymbol *sym = // ESSL 1.00 section 4.3 (or ESSL 3.00 section 4.3):
mContext->symbolTable.find(node->getSymbol(), mContext->getShaderVersion()); // Global initializers must be constant expressions.
if (sym->isVariable()) switch (node->getType().getQualifier())
{ {
// ESSL 1.00 section 4.3 (or ESSL 3.00 section 4.3): case EvqConst:
// Global initializers must be constant expressions. break;
const TVariable *var = static_cast<const TVariable *>(sym); case EvqGlobal:
switch (var->getType().getQualifier()) case EvqTemporary:
{ case EvqUniform:
case EvqConst: // We allow these cases to be compatible with legacy ESSL 1.00 content.
break; // Implement stricter rules for ESSL 3.00 since there's no legacy content to deal
case EvqGlobal: // with.
case EvqTemporary: if (mShaderVersion >= 300)
case EvqUniform: {
// We allow these cases to be compatible with legacy ESSL 1.00 content.
// Implement stricter rules for ESSL 3.00 since there's no legacy content to deal
// with.
if (mContext->getShaderVersion() >= 300)
{
mIsValid = false;
}
else
{
mIssueWarning = true;
}
break;
default:
mIsValid = false; mIsValid = false;
} }
else
{
mIssueWarning = true;
}
break;
default:
mIsValid = false;
}
}
void ValidateGlobalInitializerTraverser::visitConstantUnion(TIntermConstantUnion *node)
{
// Constant unions that are not constant expressions may result from folding a ternary
// expression.
switch (node->getType().getQualifier())
{
case EvqConst:
break;
case EvqTemporary:
if (mShaderVersion >= 300)
{
mIsValid = false;
}
else
{
mIssueWarning = true;
}
break;
default:
UNREACHABLE();
} }
} }
...@@ -99,18 +116,19 @@ bool ValidateGlobalInitializerTraverser::visitUnary(Visit visit, TIntermUnary *n ...@@ -99,18 +116,19 @@ bool ValidateGlobalInitializerTraverser::visitUnary(Visit visit, TIntermUnary *n
return true; return true;
} }
ValidateGlobalInitializerTraverser::ValidateGlobalInitializerTraverser(const TParseContext *context) ValidateGlobalInitializerTraverser::ValidateGlobalInitializerTraverser(int shaderVersion)
: TIntermTraverser(true, false, false), mContext(context), mIsValid(true), mIssueWarning(false) : TIntermTraverser(true, false, false),
mShaderVersion(shaderVersion),
mIsValid(true),
mIssueWarning(false)
{ {
} }
} // namespace } // namespace
bool ValidateGlobalInitializer(TIntermTyped *initializer, bool ValidateGlobalInitializer(TIntermTyped *initializer, int shaderVersion, bool *warning)
const TParseContext *context,
bool *warning)
{ {
ValidateGlobalInitializerTraverser validate(context); ValidateGlobalInitializerTraverser validate(shaderVersion);
initializer->traverse(&validate); initializer->traverse(&validate);
ASSERT(warning != nullptr); ASSERT(warning != nullptr);
*warning = validate.issueWarning(); *warning = validate.issueWarning();
......
...@@ -11,12 +11,9 @@ namespace sh ...@@ -11,12 +11,9 @@ namespace sh
{ {
class TIntermTyped; class TIntermTyped;
class TParseContext;
// Returns true if the initializer is valid. // Returns true if the initializer is valid.
bool ValidateGlobalInitializer(TIntermTyped *initializer, bool ValidateGlobalInitializer(TIntermTyped *initializer, int shaderVersion, bool *warning);
const TParseContext *context,
bool *warning);
} // namespace sh } // namespace sh
......
...@@ -5572,3 +5572,51 @@ TEST_F(FragmentShaderValidationTest, MatrixNegativeIndex) ...@@ -5572,3 +5572,51 @@ TEST_F(FragmentShaderValidationTest, MatrixNegativeIndex)
FAIL() << "Shader compilation succeeded, expecting failure:\n" << mInfoLog; FAIL() << "Shader compilation succeeded, expecting failure:\n" << mInfoLog;
} }
} }
// Global variable initializers need to be constant expressions. Test with assigning a ternary
// expression that ANGLE can fold.
TEST_F(FragmentShaderValidationTest, AssignConstantFoldedFromNonConstantTernaryToGlobal)
{
const std::string &shaderString =
R"(#version 300 es
precision mediump float;
uniform float u;
float f = true ? 1.0 : u;
out vec4 my_FragColor;
void main()
{
my_FragColor = vec4(f);
})";
if (compile(shaderString))
{
FAIL() << "Shader compilation succeeded, expecting failure:\n" << mInfoLog;
}
}
// Global variable initializers need to be constant expressions. Test with assigning a ternary
// expression that ANGLE can fold.
TEST_F(FragmentShaderValidationTest,
AssignConstantArrayVariableFoldedFromNonConstantTernaryToGlobal)
{
const std::string &shaderString =
R"(#version 300 es
precision mediump float;
uniform float u[2];
const float c[2] = float[2](1.0, 2.0);
float f[2] = true ? c : u;
out vec4 my_FragColor;
void main()
{
my_FragColor = vec4(f[0], f[1], 0.0, 1.0);
})";
if (compile(shaderString))
{
FAIL() << "Shader compilation succeeded, expecting failure:\n" << mInfoLog;
}
}
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment