Reland "GL: Limit declared vertex output varying components" This reverts commit 3cf1a6c74785246ed0c29f58816632e05d7d6616. Add a workaround which, when enabled, tracks the number of output components from the vertex shader and fails the compile if there's an attempt to exceed a specific limit (1024). Relative to the original commit, apply a much higher compile time limit which is unlikely to be reached by any test or real-world application, and apply it to all context types, not just WebGL's. Apply this workaround on Imagination's PowerVR OpenGL ES drivers to work around a bug in the driver's shader compiler. Co-authored with Gemini. Test: angle_end2end_tests \ --gtest_filter=GLSLValidationTest_ES3_LimitOutputVaryings.\ TooManyDeclaredVertexOutputComponents TAG=agy CONV=8ed05bff-eb25-4de9-92b8-d097e4c0abae Bug: chromium:529991907 Change-Id: I79d0bd117d6e0d6412011d361183484f088243e6 Reviewed-on: https://chromium-review.googlesource.com/c/angle/angle/+/8114814 Commit-Queue: Kenneth Russell <kbr@chromium.org> Reviewed-by: Shahbaz Youssefi <syoussefi@chromium.org>
diff --git a/include/GLSLANG/ShaderLang.h b/include/GLSLANG/ShaderLang.h index a725c38..dde82d2 100644 --- a/include/GLSLANG/ShaderLang.h +++ b/include/GLSLANG/ShaderLang.h
@@ -26,7 +26,7 @@ // Version number for shader translation API. // It is incremented every time the API changes. -#define ANGLE_SH_VERSION 417 +#define ANGLE_SH_VERSION 418 enum ShShaderSpec { @@ -179,8 +179,8 @@ // If requested, validates the AST after every transformation. Useful for debugging. uint64_t validateAST : 1; - // placeholder bit for removed validateLoopIndexing option. - uint64_t unused3 : 1; + // Limit the number of output varyings allowed in vertex shaders to work around driver bugs. + uint64_t limitOutputVaryingsTo256 : 1; // Emits #line directives in HLSL. uint64_t lineDirectives : 1;
diff --git a/include/platform/autogen/FeaturesGL_autogen.h b/include/platform/autogen/FeaturesGL_autogen.h index 9f60d6b..2b4a23e 100644 --- a/include/platform/autogen/FeaturesGL_autogen.h +++ b/include/platform/autogen/FeaturesGL_autogen.h
@@ -722,6 +722,12 @@ &members, }; + FeatureInfo limitOutputVaryingsTo256AtCompileTime = { + "limitOutputVaryingsTo256AtCompileTime", + FeatureCategory::OpenGLWorkarounds, + &members, + }; + }; inline FeaturesGL::FeaturesGL() = default;
diff --git a/include/platform/gl_features.json b/include/platform/gl_features.json index 127cccf..789e621 100644 --- a/include/platform/gl_features.json +++ b/include/platform/gl_features.json
@@ -942,6 +942,14 @@ "Split full-image level 0 PBO uploads via TexSubImage2D into two calls to work around driver bugs." ], "issue": "http://crbug.com/496807874" + }, + { + "name": "limit_output_varyings_to_256_at_compile_time", + "category": "Workarounds", + "description": [ + "Limit the number of declared varying components at compile time to work around a PowerVR driver bug." + ], + "issue": "http://crbug.com/529991907" } ] }
diff --git a/src/compiler/translator/ParseContext.cpp b/src/compiler/translator/ParseContext.cpp index ddcf9bc..7745cbe 100644 --- a/src/compiler/translator/ParseContext.cpp +++ b/src/compiler/translator/ParseContext.cpp
@@ -498,6 +498,31 @@ structDeclaration->appendDeclarator(structDeclarator); return structDeclaration; } + +unsigned int GetTypeComponentCount(const TType &type) +{ + unsigned int components = 0; + if (type.getBasicType() == EbtInterfaceBlock) + { + for (const TField *field : type.getInterfaceBlock()->fields()) + { + components += GetTypeComponentCount(*field->type()); + } + } + else if (type.getStruct()) + { + for (const TField *field : type.getStruct()->fields()) + { + components += GetTypeComponentCount(*field->type()); + } + } + else + { + components = static_cast<unsigned int>(type.getNominalSize()) * type.getSecondarySize(); + } + components *= type.getArraySizeProduct(); + return components; +} } // namespace // This tracks each binding point's current default offset for inheritance of subsequent @@ -577,6 +602,7 @@ mNumViews(-1), mMaxUniformBlocks(GetMaxUniformBlocksForShaderType(mShaderType, options, resources)), mNumUniformBlocks(0), + mNumOutputVaryingComponents(0), mDeclaringFunction(false), mDeclaringMain(false), mMainFunction(nullptr), @@ -2343,6 +2369,7 @@ error(line, "redefinition", identifier); return false; } + addAndCheckOutputVaryings(**variable, line); if (!checkIsNonVoid(line, identifier, type->getBasicType())) { @@ -7035,6 +7062,7 @@ error(field->line(), "redefinition of an interface block member name", field->name()); } + addAndCheckOutputVaryings(*fieldVariable, field->line()); // Don't declare variables for fields of nameless interface blocks in the IR, just // remember to implicitly index the instance variable when referenced. @@ -7064,6 +7092,7 @@ { error(instanceLine, "redefinition of an interface block instance name", instanceName); } + addAndCheckOutputVaryings(*instanceVariable, instanceLine); } TIntermSymbol *blockSymbol = new TIntermSymbol(instanceVariable); @@ -10768,6 +10797,44 @@ return numErrors() == 0; } +void TParseContext::addAndCheckOutputVaryings(const TVariable &variable, const TSourceLoc &line) +{ + if (mShaderType != GL_VERTEX_SHADER) + { + return; + } + + if (!mCompileOptions.limitOutputVaryingsTo256) + { + return; + } + + if (variable.symbolType() == SymbolType::BuiltIn) + { + return; + } + + if (!IsVaryingOut(variable.getType().getQualifier())) + { + return; + } + + angle::CheckedNumeric<unsigned int> checkedNum = mNumOutputVaryingComponents; + checkedNum += GetTypeComponentCount(variable.getType()); + mNumOutputVaryingComponents = + checkedNum.ValueOrDefault(std::numeric_limits<unsigned int>::max()); + + // The cap to 256 vec4s = 1024 components seems somewhat arbitrary, but this is intended as a + // workaround for a specific driver bug, and this limit being much + // higher than the device limits (mResources.MaxVertexOutputVectors * + // 4), it avoids regressing both tests and applications. + if (mNumOutputVaryingComponents > 1024) + { + error(line, "Too many declared shader output varying components for this device", + variable.name()); + } +} + // // Parse an array of strings using yyparse. //
diff --git a/src/compiler/translator/ParseContext.h b/src/compiler/translator/ParseContext.h index 3d62dfa..d086955 100644 --- a/src/compiler/translator/ParseContext.h +++ b/src/compiler/translator/ParseContext.h
@@ -613,6 +613,7 @@ const TType *type, GeomTessArray sized, TVariable **variable); + void addAndCheckOutputVaryings(const TVariable &variable, const TSourceLoc &line); void checkNestingLevel(const TSourceLoc &line); bool checkCase(const TSourceLoc &line, int64_t caseValue, const char *caseOrDefault); @@ -895,6 +896,9 @@ // Current count of declared uniform blocks. unsigned int mNumUniformBlocks; + // Current count of declared output varying components. + unsigned int mNumOutputVaryingComponents; + // Keeps track of whether any of the built-ins that can be redeclared (see // IsRedeclarableBuiltIn()) has been marked as invariant/precise before the possible // redeclaration.
diff --git a/src/libANGLE/renderer/gl/ShaderGL.cpp b/src/libANGLE/renderer/gl/ShaderGL.cpp index 02adcce..c23fccd 100644 --- a/src/libANGLE/renderer/gl/ShaderGL.cpp +++ b/src/libANGLE/renderer/gl/ShaderGL.cpp
@@ -271,6 +271,11 @@ options->expandFragmentOutputsToVec4 = true; } + if (features.limitOutputVaryingsTo256AtCompileTime.enabled) + { + options->limitOutputVaryingsTo256 = true; + } + return std::shared_ptr<ShaderTranslateTask>( new ShaderTranslateTaskGL(functions, mShaderID, contextGL->hasNativeParallelCompile())); }
diff --git a/src/libANGLE/renderer/gl/renderergl_utils.cpp b/src/libANGLE/renderer/gl/renderergl_utils.cpp index 5cbbd9f..8c78bae 100644 --- a/src/libANGLE/renderer/gl/renderergl_utils.cpp +++ b/src/libANGLE/renderer/gl/renderergl_utils.cpp
@@ -2834,6 +2834,10 @@ ANGLE_FEATURE_CONDITION(features, validateMaxPerStageUniformBlocksAtCompileTime, IsPowerVR(vendor)); + // Some drivers have compilation issues when shaders declare too many output varyings. + // crbug.com/529991907 + ANGLE_FEATURE_CONDITION(features, limitOutputVaryingsTo256AtCompileTime, IsPowerVR(vendor)); + // Mac Intel drivers are unable to allocate buffers larger than ~1gb ANGLE_FEATURE_CONDITION(features, limitMaxBufferSizeTo1gb, isApple && isIntel); }
diff --git a/src/tests/gl_tests/GLSLValidationTest.cpp b/src/tests/gl_tests/GLSLValidationTest.cpp index 014ab69..28a44aa 100644 --- a/src/tests/gl_tests/GLSLValidationTest.cpp +++ b/src/tests/gl_tests/GLSLValidationTest.cpp
@@ -9469,6 +9469,39 @@ validateError(GL_FRAGMENT_SHADER, kFS, "version"); } +class GLSLValidationTest_ES3_LimitOutputVaryings : public GLSLValidationTest_ES3 +{}; + +// Regression test for crbug.com/529991907. +// Verify that compiling a shader with up to 1024 output varying components +// succeeds, and exceeding 1024 components is rejected at compile time. +TEST_P(GLSLValidationTest_ES3_LimitOutputVaryings, TooManyDeclaredVertexOutputComponents) +{ + ANGLE_SKIP_TEST_IF( + !getEGLWindow()->isFeatureEnabled(Feature::LimitOutputVaryingsTo256AtCompileTime)); + + constexpr int kMaxVectors = 1024 / 4; + + std::stringstream vsValid; + vsValid << "#version 300 es\n"; + for (int i = 0; i < kMaxVectors; ++i) + { + vsValid << "out highp vec4 v" << i << ";\n"; + } + vsValid << "void main() { gl_Position = vec4(0.0); }\n"; + validateSuccess(GL_VERTEX_SHADER, vsValid.str().c_str()); + + std::stringstream vsInvalid; + vsInvalid << "#version 300 es\n"; + for (int i = 0; i < kMaxVectors + 1; ++i) + { + vsInvalid << "out highp vec4 v" << i << ";\n"; + } + vsInvalid << "void main() { gl_Position = vec4(0.0); }\n"; + validateError(GL_VERTEX_SHADER, vsInvalid.str().c_str(), + "Too many declared shader output varying components for this device"); +} + } // namespace ANGLE_INSTANTIATE_TEST_ES2_AND_ES3(GLSLValidationTest); @@ -9491,6 +9524,11 @@ GTEST_ALLOW_UNINSTANTIATED_PARAMETERIZED_TEST(WebGL2GLSLValidationTest); ANGLE_INSTANTIATE_TEST_ES3(WebGL2GLSLValidationTest); +GTEST_ALLOW_UNINSTANTIATED_PARAMETERIZED_TEST(GLSLValidationTest_ES3_LimitOutputVaryings); +ANGLE_INSTANTIATE_TEST(GLSLValidationTest_ES3_LimitOutputVaryings, + ES3_OPENGL().enable(Feature::LimitOutputVaryingsTo256AtCompileTime), + ES3_OPENGLES().enable(Feature::LimitOutputVaryingsTo256AtCompileTime)); + ANGLE_INSTANTIATE_TEST_ES2_AND(WebGLGLSLValidationExtensionDisableTest, ES2_OPENGL().enable(Feature::AllowExtensionDisableAfterNonPpTokens));
diff --git a/util/autogen/angle_features_autogen.cpp b/util/autogen/angle_features_autogen.cpp index adce1f1..2b52609 100644 --- a/util/autogen/angle_features_autogen.cpp +++ b/util/autogen/angle_features_autogen.cpp
@@ -246,6 +246,7 @@ {Feature::LimitMaxMSAASamplesTo4, "limitMaxMSAASamplesTo4"}, {Feature::LimitMaxStorageBufferSize, "limitMaxStorageBufferSize"}, {Feature::LimitMaxTextureBytesTo1MB, "limitMaxTextureBytesTo1MB"}, + {Feature::LimitOutputVaryingsTo256AtCompileTime, "limitOutputVaryingsTo256AtCompileTime"}, {Feature::LimitSampleCountTo2, "limitSampleCountTo2"}, {Feature::LimitWebglMaxTextureSizeTo4096, "limitWebglMaxTextureSizeTo4096"}, {Feature::LimitWebglMaxTextureSizeTo8192, "limitWebglMaxTextureSizeTo8192"},
diff --git a/util/autogen/angle_features_autogen.h b/util/autogen/angle_features_autogen.h index 09308ab..16733c1 100644 --- a/util/autogen/angle_features_autogen.h +++ b/util/autogen/angle_features_autogen.h
@@ -246,6 +246,7 @@ LimitMaxMSAASamplesTo4, LimitMaxStorageBufferSize, LimitMaxTextureBytesTo1MB, + LimitOutputVaryingsTo256AtCompileTime, LimitSampleCountTo2, LimitWebglMaxTextureSizeTo4096, LimitWebglMaxTextureSizeTo8192,