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>

gn/sksl_tests.gni

@@ -496,6 +496,10 @@ sksl_rte_error_tests = [
   "$_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

src/sksl/SkSLAnalysis.cpp

@@ -460,6 +460,191 @@ bool Analysis::IsTrivialExpression(const Expression& expr) {
             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
 

src/sksl/SkSLAnalysis.h

@@ -17,6 +17,7 @@ namespace SkSL {
 
 class ErrorReporter;
 class Expression;
+class ForStatement;
 class FunctionDeclaration;
 class FunctionDefinition;
 struct LoadedModule;
@@ -66,6 +67,21 @@ struct Analysis {
     // - 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);
 };
 
 /**

src/sksl/SkSLIRGenerator.cpp

@@ -568,8 +568,17 @@ std::unique_ptr<Statement> IRGenerator::convertFor(const ASTNode& f) {
     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,

tests/SkSLInterpreterTest.cpp

@@ -508,8 +508,8 @@ DEF_TEST(SkSLInterpreterFor, r) {
     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 @@ DEF_TEST(SkSLInterpreterFor, r) {
     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 @@ DEF_TEST(SkSLInterpreterCompound, r) {
         // 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"

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) {} }

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) {} }

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) {} }

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); } }

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

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

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

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