In runtime effects, verify that loops conform to ES2 rules
Bug: skia:11094
Change-Id: I68a08e79d29579901b74daca3c22f5112fbb3c8c
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/353356
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Mike Klein <mtklein@google.com>
diff --git a/gn/sksl_tests.gni b/gn/sksl_tests.gni
index 33565279..f45d06c 100644
--- a/gn/sksl_tests.gni
+++ b/gn/sksl_tests.gni
@@ -496,6 +496,10 @@
"$_tests/sksl/errors/IllegalOperators.rte",
"$_tests/sksl/errors/UnsupportedTypeSampler.rte",
"$_tests/sksl/errors/UnsupportedTypeTexture.rte",
+ "$_tests/sksl/runtime_errors/LoopConditionErrors.rte",
+ "$_tests/sksl/runtime_errors/LoopExpressionErrors.rte",
+ "$_tests/sksl/runtime_errors/LoopInitializerErrors.rte",
+ "$_tests/sksl/runtime_errors/LoopStructureErrors.rte",
]
# Tests in sksl_fp_tests_sources will be compiled with --settings on, and are expected to generate
diff --git a/src/sksl/SkSLAnalysis.cpp b/src/sksl/SkSLAnalysis.cpp
index 3d5ef22..2462ec3 100644
--- a/src/sksl/SkSLAnalysis.cpp
+++ b/src/sksl/SkSLAnalysis.cpp
@@ -460,6 +460,191 @@
IsTrivialExpression(*expr.as<IndexExpression>().base()));
}
+static const char* invalid_for_ES2(const ForStatement& loop,
+ Analysis::UnrollableLoopInfo& loopInfo) {
+ auto getConstant = [&](const std::unique_ptr<Expression>& expr, double* val) {
+ if (!expr->isCompileTimeConstant()) {
+ return false;
+ }
+ if (!expr->type().isInteger() && !expr->type().isFloat()) {
+ SkDEBUGFAIL("unexpected constant type");
+ return false;
+ }
+
+ *val = expr->type().isInteger() ? static_cast<double>(expr->getConstantInt())
+ : static_cast<double>(expr->getConstantFloat());
+ return true;
+ };
+
+ //
+ // init_declaration has the form: type_specifier identifier = constant_expression
+ //
+ if (!loop.initializer()) {
+ return "missing init declaration";
+ }
+ if (!loop.initializer()->is<VarDeclaration>()) {
+ return "invalid init declaration";
+ }
+ const VarDeclaration& initDecl = loop.initializer()->as<VarDeclaration>();
+ if (!initDecl.baseType().isInteger() && !initDecl.baseType().isFloat()) {
+ return "invalid type for loop index";
+ }
+ if (initDecl.arraySize() != 0) {
+ return "invalid type for loop index";
+ }
+ if (!initDecl.value()) {
+ return "missing loop index initializer";
+ }
+ if (!getConstant(initDecl.value(), &loopInfo.fStart)) {
+ return "loop index initializer must be a constant expression";
+ }
+
+ loopInfo.fIndex = &initDecl.var();
+
+ auto is_loop_index = [&](const std::unique_ptr<Expression>& expr) {
+ return expr->is<VariableReference>() &&
+ expr->as<VariableReference>().variable() == loopInfo.fIndex;
+ };
+
+ //
+ // condition has the form: loop_index relational_operator constant_expression
+ //
+ if (!loop.test()) {
+ return "missing condition";
+ }
+ if (!loop.test()->is<BinaryExpression>()) {
+ return "invalid condition";
+ }
+ const BinaryExpression& cond = loop.test()->as<BinaryExpression>();
+ if (!is_loop_index(cond.left())) {
+ return "expected loop index on left hand side of condition";
+ }
+ // relational_operator is one of: > >= < <= == or !=
+ switch (cond.getOperator()) {
+ case Token::Kind::TK_GT:
+ case Token::Kind::TK_GTEQ:
+ case Token::Kind::TK_LT:
+ case Token::Kind::TK_LTEQ:
+ case Token::Kind::TK_EQEQ:
+ case Token::Kind::TK_NEQ:
+ break;
+ default:
+ return "invalid relational operator";
+ }
+ double loopEnd = 0;
+ if (!getConstant(cond.right(), &loopEnd)) {
+ return "loop index must be compared with a constant expression";
+ }
+
+ //
+ // expression has one of the following forms:
+ // loop_index++
+ // loop_index--
+ // loop_index += constant_expression
+ // loop_index -= constant_expression
+ // The spec doesn't mention prefix increment and decrement, but there is some consensus that
+ // it's an oversight, so we allow those as well.
+ //
+ if (!loop.next()) {
+ return "missing loop expression";
+ }
+ switch (loop.next()->kind()) {
+ case Expression::Kind::kBinary: {
+ const BinaryExpression& next = loop.next()->as<BinaryExpression>();
+ if (!is_loop_index(next.left())) {
+ return "expected loop index in loop expression";
+ }
+ if (!getConstant(next.right(), &loopInfo.fDelta)) {
+ return "loop index must be modified by a constant expression";
+ }
+ switch (next.getOperator()) {
+ case Token::Kind::TK_PLUSEQ: break;
+ case Token::Kind::TK_MINUSEQ: loopInfo.fDelta = -loopInfo.fDelta; break;
+ default:
+ return "invalid operator in loop expression";
+ }
+ } break;
+ case Expression::Kind::kPrefix: {
+ const PrefixExpression& next = loop.next()->as<PrefixExpression>();
+ if (!is_loop_index(next.operand())) {
+ return "expected loop index in loop expression";
+ }
+ switch (next.getOperator()) {
+ case Token::Kind::TK_PLUSPLUS: loopInfo.fDelta = 1; break;
+ case Token::Kind::TK_MINUSMINUS: loopInfo.fDelta = -1; break;
+ default:
+ return "invalid operator in loop expression";
+ }
+ } break;
+ case Expression::Kind::kPostfix: {
+ const PostfixExpression& next = loop.next()->as<PostfixExpression>();
+ if (!is_loop_index(next.operand())) {
+ return "expected loop index in loop expression";
+ }
+ switch (next.getOperator()) {
+ case Token::Kind::TK_PLUSPLUS: loopInfo.fDelta = 1; break;
+ case Token::Kind::TK_MINUSMINUS: loopInfo.fDelta = -1; break;
+ default:
+ return "invalid operator in loop expression";
+ }
+ } break;
+ default:
+ return "invalid loop expression";
+ }
+
+ //
+ // Within the body of the loop, the loop index is not statically assigned to, nor is it used as
+ // argument to a function 'out' or 'inout' parameter.
+ //
+ if (Analysis::StatementWritesToVariable(*loop.statement(), initDecl.var())) {
+ return "loop index must not be modified within body of the loop";
+ }
+
+ // Finally, compute the iteration count, based on the bounds, and the termination operator.
+ constexpr int kMaxUnrollableLoopLength = 128;
+ loopInfo.fCount = 0;
+
+ double val = loopInfo.fStart;
+ auto evalCond = [&]() {
+ switch (cond.getOperator()) {
+ case Token::Kind::TK_GT: return val > loopEnd;
+ case Token::Kind::TK_GTEQ: return val >= loopEnd;
+ case Token::Kind::TK_LT: return val < loopEnd;
+ case Token::Kind::TK_LTEQ: return val <= loopEnd;
+ case Token::Kind::TK_EQEQ: return val == loopEnd;
+ case Token::Kind::TK_NEQ: return val != loopEnd;
+ default: SkUNREACHABLE;
+ }
+ };
+
+ for (loopInfo.fCount = 0; loopInfo.fCount <= kMaxUnrollableLoopLength; ++loopInfo.fCount) {
+ if (!evalCond()) {
+ break;
+ }
+ val += loopInfo.fDelta;
+ }
+
+ if (loopInfo.fCount > kMaxUnrollableLoopLength) {
+ return "loop must guarantee termination in fewer iterations";
+ }
+
+ return nullptr; // All checks pass
+}
+
+bool Analysis::ForLoopIsValidForES2(const ForStatement& loop,
+ Analysis::UnrollableLoopInfo* outLoopInfo,
+ ErrorReporter* errors) {
+ UnrollableLoopInfo ignored,
+ *loopInfo = outLoopInfo ? outLoopInfo : &ignored;
+ if (const char* msg = invalid_for_ES2(loop, *loopInfo)) {
+ if (errors) {
+ errors->error(loop.fOffset, msg);
+ }
+ return false;
+ }
+ return true;
+}
+
////////////////////////////////////////////////////////////////////////////////
// ProgramVisitor
diff --git a/src/sksl/SkSLAnalysis.h b/src/sksl/SkSLAnalysis.h
index c853964..43883e1 100644
--- a/src/sksl/SkSLAnalysis.h
+++ b/src/sksl/SkSLAnalysis.h
@@ -17,6 +17,7 @@
class ErrorReporter;
class Expression;
+class ForStatement;
class FunctionDeclaration;
class FunctionDefinition;
struct LoadedModule;
@@ -66,6 +67,21 @@
// - half4(myColor.a)
// - myStruct.myArrayField[7].xyz
static bool IsTrivialExpression(const Expression& expr);
+
+ struct UnrollableLoopInfo {
+ const Variable* fIndex;
+ double fStart;
+ double fDelta;
+ int fCount;
+ };
+
+ // Ensures that 'loop' meets the strict requirements of The OpenGL ES Shading Language 1.00,
+ // Appendix A, Section 4.
+ // Information about the loop's structure are placed in outLoopInfo (if not nullptr).
+ // If the function returns false, specific reasons are reported via errors (if not nullptr).
+ static bool ForLoopIsValidForES2(const ForStatement& loop,
+ UnrollableLoopInfo* outLoopInfo,
+ ErrorReporter* errors);
};
/**
diff --git a/src/sksl/SkSLIRGenerator.cpp b/src/sksl/SkSLIRGenerator.cpp
index 1e3e2bc..75ca4e8 100644
--- a/src/sksl/SkSLIRGenerator.cpp
+++ b/src/sksl/SkSLIRGenerator.cpp
@@ -568,8 +568,17 @@
if (!statement) {
return nullptr;
}
- return std::make_unique<ForStatement>(f.fOffset, std::move(initializer), std::move(test),
- std::move(next), std::move(statement), fSymbolTable);
+
+ auto forStmt =
+ std::make_unique<ForStatement>(f.fOffset, std::move(initializer), std::move(test),
+ std::move(next), std::move(statement), fSymbolTable);
+ if (this->strictES2Mode()) {
+ if (!Analysis::ForLoopIsValidForES2(*forStmt, /*outLoopInfo=*/nullptr,
+ &this->errorReporter())) {
+ return nullptr;
+ }
+ }
+ return std::move(forStmt);
}
std::unique_ptr<Statement> IRGenerator::convertWhile(int offset, std::unique_ptr<Expression> test,
diff --git a/tests/SkSLInterpreterTest.cpp b/tests/SkSLInterpreterTest.cpp
index 92622d22d..3292ee3 100644
--- a/tests/SkSLInterpreterTest.cpp
+++ b/tests/SkSLInterpreterTest.cpp
@@ -508,8 +508,8 @@
test(r,
"void main(inout half4 color) {"
" for (int i = 1; i <= 10; ++i)"
- " for (int j = i; j <= 10; ++j)"
- " color.r += j;"
+ " for (int j = 1; j <= 10; ++j)"
+ " if (j >= i) { color.r += j; }"
"}",
0, 0, 0, 0,
385, 0, 0, 0,
@@ -517,7 +517,7 @@
test(r,
"void main(inout half4 color) {"
" for (int i = 1; i <= 10; ++i)"
- " for (int j = 1; ; ++j) {"
+ " for (int j = 1; j < 20 ; ++j) {"
" if (i == j) continue;"
" if (j > 10) break;"
" color.r += j;"
@@ -602,7 +602,8 @@
// Kitchen sink (swizzles, inout, SoAoS)
"struct ManyRects { int numRects; RectAndColor rects[4]; };\n"
"void fill_rects(inout ManyRects mr) {\n"
- " for (int i = 0; i < mr.numRects; ++i) {\n"
+ " for (int i = 0; i < 4; ++i) {\n"
+ " if (i >= mr.numRects) { break; }\n"
" mr.rects[i].r = gRects[i];\n"
" float b = mr.rects[i].r.p1.y;\n"
" mr.rects[i].color = float4(b, b, b, b);\n"
diff --git a/tests/sksl/runtime_errors/LoopConditionErrors.rte b/tests/sksl/runtime_errors/LoopConditionErrors.rte
new file mode 100644
index 0000000..d9109ff
--- /dev/null
+++ b/tests/sksl/runtime_errors/LoopConditionErrors.rte
@@ -0,0 +1,16 @@
+// Expect 9 errors
+
+void no_condition() { for (int i = 0;;i++) {} }
+
+void unary_cond_op() { for (int i = 0; !(i > 1); ++i) {} }
+void implict_cond_op() { for (int i = 1; bool(i); --i) {} }
+void complex_cond_op() { for (int i = 0; i < 1 && i < 2; ++i) {} }
+
+void cond_wrong_var() { int j = 0; for (int i = 0; j < 1; ++i) {} }
+void cond_wrong_side() { for (int i = 0; 1 > i; ++i) {} }
+void cond_index_cast() { for (int i = 0; float(i) < 1.5; ++i) {} }
+
+uniform int u;
+
+void cond_uniform_val() { for (int i = 0; i < u; ++i) {} }
+void cond_param_val(int p) { for (int i = 0; i < p; ++i) {} }
diff --git a/tests/sksl/runtime_errors/LoopExpressionErrors.rte b/tests/sksl/runtime_errors/LoopExpressionErrors.rte
new file mode 100644
index 0000000..a161a16
--- /dev/null
+++ b/tests/sksl/runtime_errors/LoopExpressionErrors.rte
@@ -0,0 +1,13 @@
+// Expect 7 errors
+
+void no_expression() { for (int i = 0; i < 1;) {} }
+
+void expression_equals() { for (int i = 0; i < 1; i = 1) {} }
+void expression_equal_plus() { for (int i = 0; i < 1; i = i + 1) {} }
+void expression_times_eq() { for (int i = 1; i < 2; i *= 2) {} }
+void expression_bad_unary() { for (int i = 0; i < 1; -i) {} }
+
+uniform int u;
+
+void expression_uniform_val() { for (int i = 0; i < 1; i += u) {} }
+void expression_param_val(int p) { for (int i = 0; i < 1; i += p) {} }
diff --git a/tests/sksl/runtime_errors/LoopInitializerErrors.rte b/tests/sksl/runtime_errors/LoopInitializerErrors.rte
new file mode 100644
index 0000000..974a48f
--- /dev/null
+++ b/tests/sksl/runtime_errors/LoopInitializerErrors.rte
@@ -0,0 +1,14 @@
+// Expect 8 errors
+
+void no_initializer() { int i = 0; for ( ; i < 1; i++) {} }
+void init_not_decl() { int i; for (i = 0; i < 1; i++) {} }
+void index_no_init() { for (int i; i < 1; i++) {} }
+
+void bool_index() { for (bool i = false; i != true; i = !i) {} }
+void vec_index() { for (float2 i = float2(0); i.x < 1; i.x++) {} }
+void array_index() { for (int i[2] = int[2](0, 0); i[0] < 1; ++i[0]) {} }
+
+uniform int u;
+
+void uniform_init() { for (int i = u; i < 1; ++i) {} }
+void param_init(int p) { for (int i = p; i < 1; ++i) {} }
diff --git a/tests/sksl/runtime_errors/LoopStructureErrors.rte b/tests/sksl/runtime_errors/LoopStructureErrors.rte
new file mode 100644
index 0000000..48a5c60
--- /dev/null
+++ b/tests/sksl/runtime_errors/LoopStructureErrors.rte
@@ -0,0 +1,12 @@
+// Expect 5 errors
+
+void loop_length_ok() { for (int i = 0; i < 128; i++) {} } // LEGAL: See kMaxUnrollableLoopLength
+void loop_too_long() { for (int i = 0; i < 129; i++) {} }
+void infinite_loop() { for (int i = 0; i < 1; i += 0) {} }
+
+void set(out int x) { x = 1; }
+void inc(inout int x) { x++; }
+
+void index_modified() { for (int i = 0; i < 2; i++) { i++; } }
+void index_out_param() { for (int i = 0; i < 1; i++) { set(i); } }
+void index_inout_param() { for (int i = 0; i < 1; i++) { inc(i); } }
diff --git a/tests/sksl/runtime_errors/golden/LoopConditionErrors.skvm b/tests/sksl/runtime_errors/golden/LoopConditionErrors.skvm
new file mode 100644
index 0000000..bf5e9b4
--- /dev/null
+++ b/tests/sksl/runtime_errors/golden/LoopConditionErrors.skvm
@@ -0,0 +1,12 @@
+### Compilation failed:
+
+error: 3: missing condition
+error: 5: invalid condition
+error: 6: invalid condition
+error: 7: expected loop index on left hand side of condition
+error: 9: expected loop index on left hand side of condition
+error: 10: expected loop index on left hand side of condition
+error: 11: expected loop index on left hand side of condition
+error: 15: loop index must be compared with a constant expression
+error: 16: loop index must be compared with a constant expression
+9 errors
diff --git a/tests/sksl/runtime_errors/golden/LoopExpressionErrors.skvm b/tests/sksl/runtime_errors/golden/LoopExpressionErrors.skvm
new file mode 100644
index 0000000..d0e92e1
--- /dev/null
+++ b/tests/sksl/runtime_errors/golden/LoopExpressionErrors.skvm
@@ -0,0 +1,10 @@
+### Compilation failed:
+
+error: 3: missing loop expression
+error: 5: invalid operator in loop expression
+error: 6: loop index must be modified by a constant expression
+error: 7: invalid operator in loop expression
+error: 8: invalid operator in loop expression
+error: 12: loop index must be modified by a constant expression
+error: 13: loop index must be modified by a constant expression
+7 errors
diff --git a/tests/sksl/runtime_errors/golden/LoopInitializerErrors.skvm b/tests/sksl/runtime_errors/golden/LoopInitializerErrors.skvm
new file mode 100644
index 0000000..4326716
--- /dev/null
+++ b/tests/sksl/runtime_errors/golden/LoopInitializerErrors.skvm
@@ -0,0 +1,11 @@
+### Compilation failed:
+
+error: 3: missing init declaration
+error: 4: invalid init declaration
+error: 5: missing loop index initializer
+error: 7: invalid type for loop index
+error: 8: invalid type for loop index
+error: 9: invalid type for loop index
+error: 13: loop index initializer must be a constant expression
+error: 14: loop index initializer must be a constant expression
+8 errors
diff --git a/tests/sksl/runtime_errors/golden/LoopStructureErrors.skvm b/tests/sksl/runtime_errors/golden/LoopStructureErrors.skvm
new file mode 100644
index 0000000..851758a
--- /dev/null
+++ b/tests/sksl/runtime_errors/golden/LoopStructureErrors.skvm
@@ -0,0 +1,8 @@
+### Compilation failed:
+
+error: 4: loop must guarantee termination in fewer iterations
+error: 5: loop must guarantee termination in fewer iterations
+error: 10: loop index must not be modified within body of the loop
+error: 11: loop index must not be modified within body of the loop
+error: 12: loop index must not be modified within body of the loop
+5 errors
