Skip to content

A
angle
Commit fab397e5 by Ian Elliott Committed by Commit Bot
Options

Vulkan: Enforce an error when initializing a global with a non-const

The ESSL 1.0 spec is clear that "initializers must be a constant expression." Yet, because of "legacy" applications (apparently just WebGL applications), the code was only issuing a warning and not an error. The "KHR-GLES2.shaders.negative.initialize" test requires an error be generated. This change splits the semantics, allowing GLES applications to get an error, and WebGL applications to get a warning. Note: This change is related to https://chromium-review.googlesource.com/829138 (for angleproject:2285). Bug: angleproject:3381 Change-Id: Ie243b7dd72102aeb52df506d121d1d2a8f6974d3 Reviewed-on: https://chromium-review.googlesource.com/c/angle/angle/+/1716617Reviewed-by: 's avatarShahbaz Youssefi <syoussefi@chromium.org> Commit-Queue: Ian Elliott <ianelliott@google.com>
parent c327370e
Showing with 103 additions and 40 deletions
src/compiler/translator/ParseContext.cpp
......@@ -1987,7 +1987,8 @@ bool TParseContext::executeInitializer(const TSourceLoc &line,
bool globalInitWarning = false;
if (symbolTable.atGlobalLevel() &&
!ValidateGlobalInitializer(initializer, mShaderVersion, &globalInitWarning))
!ValidateGlobalInitializer(initializer, mShaderVersion, sh::IsWebGLBasedSpec(mShaderSpec),
&globalInitWarning))
{
// Error message does not completely match behavior with ESSL 1.00, but
// we want to steer developers towards only using constant expressions.
......
src/compiler/translator/ValidateGlobalInitializer.cpp
......@@ -19,7 +19,7 @@ const int kMaxAllowedTraversalDepth = 256;
class ValidateGlobalInitializerTraverser : public TIntermTraverser
{
public:
ValidateGlobalInitializerTraverser(int shaderVersion);
ValidateGlobalInitializerTraverser(int shaderVersion, bool isWebGL);
void visitSymbol(TIntermSymbol *node) override;
void visitConstantUnion(TIntermConstantUnion *node) override;
......@@ -32,6 +32,7 @@ class ValidateGlobalInitializerTraverser : public TIntermTraverser
private:
int mShaderVersion;
bool mIsWebGL;
bool mIsValid;
bool mIssueWarning;
};
......@@ -50,7 +51,7 @@ void ValidateGlobalInitializerTraverser::visitSymbol(TIntermSymbol *node)
// 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 (mShaderVersion >= 300)
if ((mShaderVersion >= 300) || !mIsWebGL)
{
mIsValid = false;
}
......@@ -73,7 +74,7 @@ void ValidateGlobalInitializerTraverser::visitConstantUnion(TIntermConstantUnion
case EvqConst:
break;
case EvqTemporary:
if (mShaderVersion >= 300)
if ((mShaderVersion >= 300) || !mIsWebGL)
{
mIsValid = false;
}
......@@ -118,9 +119,11 @@ bool ValidateGlobalInitializerTraverser::visitUnary(Visit visit, TIntermUnary *n
return true;
}
ValidateGlobalInitializerTraverser::ValidateGlobalInitializerTraverser(int shaderVersion)
ValidateGlobalInitializerTraverser::ValidateGlobalInitializerTraverser(int shaderVersion,
bool isWebGL)
: TIntermTraverser(true, false, false, nullptr),
mShaderVersion(shaderVersion),
mIsWebGL(isWebGL),
mIsValid(true),
mIssueWarning(false)
{
......@@ -129,9 +132,12 @@ ValidateGlobalInitializerTraverser::ValidateGlobalInitializerTraverser(int shade
} // namespace
bool ValidateGlobalInitializer(TIntermTyped *initializer, int shaderVersion, bool *warning)
bool ValidateGlobalInitializer(TIntermTyped *initializer,
int shaderVersion,
bool isWebGL,
bool *warning)
{
ValidateGlobalInitializerTraverser validate(shaderVersion);
ValidateGlobalInitializerTraverser validate(shaderVersion, isWebGL);
initializer->traverse(&validate);
ASSERT(warning != nullptr);
*warning = validate.issueWarning();
......
src/compiler/translator/ValidateGlobalInitializer.h
......@@ -13,7 +13,10 @@ namespace sh
class TIntermTyped;
// Returns true if the initializer is valid.
bool ValidateGlobalInitializer(TIntermTyped *initializer, int shaderVersion, bool *warning);
bool ValidateGlobalInitializer(TIntermTyped *initializer,
int shaderVersion,
bool isWebGL,
bool *warning);
} // namespace sh
......
src/tests/compiler_tests/ShaderValidation_test.cpp
......@@ -606,8 +606,8 @@ TEST_F(FragmentShaderValidationTest, AssignUniformToGlobalESSL3)
}
// Global variable initializers need to be constant expressions (ESSL 1.00 section 4.3)
// Initializing with an uniform should generate a warning
// (we don't generate an error on ESSL 1.00 because of legacy compatibility)
// Initializing with an uniform used to generate a warning on ESSL 1.00 because of legacy
// compatibility, but that causes dEQP to fail (which expects an error)
TEST_F(FragmentShaderValidationTest, AssignUniformToGlobalESSL1)
{
const std::string &shaderString =
......@@ -619,15 +619,7 @@ TEST_F(FragmentShaderValidationTest, AssignUniformToGlobalESSL1)
"}\n";
if (compile(shaderString))
{
if (!hasWarning())
{
FAIL() << "Shader compilation succeeded without warnings, expecting warning:\n"
<< mInfoLog;
}
}
else
{
FAIL() << "Shader compilation failed, expecting success with warning:\n" << mInfoLog;
FAIL() << "Shader compilation succeeded, expecting failure:\n" << mInfoLog;
}
}
......@@ -1349,6 +1341,46 @@ TEST_F(WebGL2FragmentShaderValidationTest, IndexFragDataWithNonConstant)
}
}
// Global variable initializers need to be constant expressions (ESSL 1.00 section 4.3)
// Initializing with an uniform should generate a warning
// (we don't generate an error on ESSL 1.00 because of WebGL compatibility)
TEST_F(WebGL2FragmentShaderValidationTest, AssignUniformToGlobalESSL1)
{
const std::string &shaderString =
"precision mediump float;\n"
"uniform float a;\n"
"float b = a * 2.0;\n"
"void main() {\n"
" gl_FragColor = vec4(b);\n"
"}\n";
if (compile(shaderString))
{
if (!hasWarning())
{
FAIL() << "Shader compilation succeeded without warnings, expecting warning:\n"
<< mInfoLog;
}
}
else
{
FAIL() << "Shader compilation failed, expecting success with warning:\n" << mInfoLog;
}
}
// Test that deferring global variable init works with an empty main().
TEST_F(WebGL2FragmentShaderValidationTest, DeferGlobalVariableInitWithEmptyMain)
{
const std::string &shaderString =
"precision mediump float;\n"
"uniform float u;\n"
"float foo = u;\n"
"void main() {}\n";
if (!compile(shaderString))
{
FAIL() << "Shader compilation failed, expecting success:\n" << mInfoLog;
}
}
// Test that a non-constant texture offset is not accepted for textureOffset.
// ESSL 3.00 section 8.8
TEST_F(FragmentShaderValidationTest, TextureOffsetNonConst)
......@@ -1469,6 +1501,46 @@ TEST_F(WebGL1FragmentShaderValidationTest, NonConstantLoopIndex)
}
}
// Global variable initializers need to be constant expressions (ESSL 1.00 section 4.3)
// Initializing with an uniform should generate a warning
// (we don't generate an error on ESSL 1.00 because of WebGL compatibility)
TEST_F(WebGL1FragmentShaderValidationTest, AssignUniformToGlobalESSL1)
{
const std::string &shaderString =
"precision mediump float;\n"
"uniform float a;\n"
"float b = a * 2.0;\n"
"void main() {\n"
" gl_FragColor = vec4(b);\n"
"}\n";
if (compile(shaderString))
{
if (!hasWarning())
{
FAIL() << "Shader compilation succeeded without warnings, expecting warning:\n"
<< mInfoLog;
}
}
else
{
FAIL() << "Shader compilation failed, expecting success with warning:\n" << mInfoLog;
}
}
// Test that deferring global variable init works with an empty main().
TEST_F(WebGL1FragmentShaderValidationTest, DeferGlobalVariableInitWithEmptyMain)
{
const std::string &shaderString =
"precision mediump float;\n"
"uniform float u;\n"
"float foo = u;\n"
"void main() {}\n";
if (!compile(shaderString))
{
FAIL() << "Shader compilation failed, expecting success:\n" << mInfoLog;
}
}
// Check that indices that are not integers are rejected.
// The check should be done even if ESSL 1.00 Appendix A limitations are not applied.
TEST_F(FragmentShaderValidationTest, NonIntegerIndex)
......@@ -2636,20 +2708,6 @@ TEST_F(FragmentShaderValidationTest, ShiftByNegative)
}
}
// Test that deferring global variable init works with an empty main().
TEST_F(FragmentShaderValidationTest, DeferGlobalVariableInitWithEmptyMain)
{
const std::string &shaderString =
"precision mediump float;\n"
"uniform float u;\n"
"float foo = u;\n"
"void main() {}\n";
if (!compile(shaderString))
{
FAIL() << "Shader compilation failed, expecting success:\n" << mInfoLog;
}
}
// Test that pruning empty declarations from loop init expression works.
TEST_F(FragmentShaderValidationTest, EmptyDeclarationAsLoopInit)
{
......@@ -6062,4 +6120,4 @@ void main() {
{
FAIL() << "Shader compilation succeeded, expecting failure:\n" << mInfoLog;
}
}
\ No newline at end of file
}
src/tests/deqp_support/deqp_khr_gles2_test_expectations.txt
......@@ -29,9 +29,6 @@
// fails on Nexus5X with GLES backend (hangs)
// 91531 NEXUS5X GLES : conformance_more_* = SKIP
// Relates to a difference in spec with ES and Core GL.
3381 : KHR-GLES2.shaders.negative.initialize = FAIL
// Failures with floating point extensions
3451 D3D9 D3D11 OPENGL GLES : KHR-GLES2.core.internalformat.texture2d.rgb_half_float_rgb16f = FAIL
3451 D3D9 D3D11 OPENGL GLES : KHR-GLES2.core.internalformat.texture2d.rgba_half_float_rgba16f = FAIL
......@@ -50,4 +47,4 @@
// Compressed texture tests on Android
3190 VULKAN PIXEL2 : KHR-GLES2.texture_3d.compressed_texture.rgba_astc_* = FAIL
3190 VULKAN PIXEL2 : KHR-GLES2.texture_3d.compressed_texture.sgb8_alpha8_astc_* = FAIL
3190 VULKAN PIXEL2 : KHR-GLES2.texture_3d.compressed_texture.srgb8_alpha8_astc_* = FAIL
\ No newline at end of file
3190 VULKAN PIXEL2 : KHR-GLES2.texture_3d.compressed_texture.srgb8_alpha8_astc_* = FAIL
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