Check for recursion or too-deep function-call chains everywhere.

Previously, we allowed non-ES2 programs to contain recursion. However,
most GPU backends don't actually allow recursion at all. Now SkSL bans
it regardless of the strict-ES2-enforcement setting.

Change-Id: I572a30cafc39fe1791038a92d7e5c5d9bafa99aa
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/534777
Reviewed-by: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>

src/sksl/SkSLAnalysis.h

@@ -72,7 +72,7 @@ bool ReturnsOpaqueColor(const FunctionDefinition& function);
  * intended to prevent absurdly large programs from overwhemling SkVM. Only strict-ES2 mode is
  * supported; complex control flow is not SkVM-compatible (and this becomes the halting problem)
  */
-bool CheckProgramStructure(const Program& program);
+bool CheckProgramStructure(const Program& program, bool enforceSizeLimit);
 
 /**
  * Detect an orphaned variable declaration outside of a scope, e.g. if (true) int a;. Returns

src/sksl/SkSLCompiler.cpp

@@ -641,16 +641,16 @@ bool Compiler::finalize(Program& program) {
     // as errors.
     Analysis::DoFinalizationChecks(program);
 
-    // Verify that the program conforms to ES2 limitations.
     if (fContext->fConfig->strictES2Mode() && this->errorCount() == 0) {
         // Enforce Appendix A, Section 5 of the GLSL ES 1.00 spec -- Indexing. This logic assumes
         // that all loops meet the criteria of Section 4, and if they don't, could crash.
         for (const auto& pe : program.fOwnedElements) {
             Analysis::ValidateIndexingForES2(*pe, this->errorReporter());
         }
-        // Verify that the program size is reasonable after unrolling and inlining. This also
+    }
-        // issues errors for static recursion and overly-deep function-call chains.
+    if (this->errorCount() == 0) {
-        Analysis::CheckProgramStructure(program);
+        bool enforceSizeLimit = ProgramConfig::IsRuntimeEffect(program.fConfig->fKind);
+        Analysis::CheckProgramStructure(program, enforceSizeLimit);
     }
 
     return this->errorCount() == 0;

src/sksl/analysis/SkSLCheckProgramStructure.cpp

@@ -5,7 +5,7 @@
  * found in the LICENSE file.
  */
 
-#include "include/core/SkTypes.h"
+#include "include/core/SkTypes.h" // IWYU pragma: keep
 #include "include/private/SkSLProgramElement.h"
 #include "include/private/SkSLStatement.h"
 #include "include/private/SkTHash.h"
@@ -31,13 +31,11 @@
 
 namespace SkSL {
 
-bool Analysis::CheckProgramStructure(const Program& program) {
+bool Analysis::CheckProgramStructure(const Program& program, bool enforceSizeLimit) {
-    // We check the size of strict-ES2 programs since SkVM will completely unroll them.
+    // We check the size of strict-ES2 programs; since SkVM will completely unroll them, it's
-    // Note that we *cannot* safely check the program size of non-ES2 code at this time, as it is
+    // important to know how large the result will be. For non-ES2 code, we compute an approximate
-    // allowed to do things we can't measure (e.g. the program can contain a recursive cycle). We
+    // lower bound by assuming all non-unrollable loops will execute one time only.
-    // could, at best, compute a lower bound.
     const Context& context = *program.fContext;
-    SkASSERT(context.fConfig->strictES2Mode());
 
     // If we decide that expressions are cheaper than statements, or that certain statements are
     // more expensive than others, etc., we can always tweak these ratios as needed. A very rough
@@ -118,26 +116,34 @@ bool Analysis::CheckProgramStructure(const Program& program) {
             switch (stmt.kind()) {
                 case Statement::Kind::kFor: {
                     // We count a for-loop's unrolled size here. We expect that the init statement
-                    // will be emitted once, and the next-expr and statement will be repeated in the
+                    // will be emitted once, and the test-expr, next-expr and statement will be
-                    // output for every iteration of the loop. The test-expr is optimized away
+                    // repeated in the output for every iteration of the loop.
-                    // during the unroll and is not counted at all.
+                    bool earlyExit = false;
                     const ForStatement& forStmt = stmt.as<ForStatement>();
-                    bool result = this->visitStatement(*forStmt.initializer());
+                    if (forStmt.initializer() && this->visitStatement(*forStmt.initializer())) {
+                        earlyExit = true;
+                    }
 
                     size_t originalFunctionSize = fFunctionSize;
                     fFunctionSize = 0;
 
-                    result = this->visitExpression(*forStmt.next()) ||
+                    if (forStmt.next() && this->visitExpression(*forStmt.next())) {
-                             this->visitStatement(*forStmt.statement()) || result;
+                        earlyExit = true;
-
+                    }
-                    if (const LoopUnrollInfo* unrollInfo = forStmt.unrollInfo()) {
+                    if (forStmt.test() && this->visitExpression(*forStmt.test())) {
-                        fFunctionSize = SkSafeMath::Mul(fFunctionSize, unrollInfo->fCount);
+                        earlyExit = true;
-                    } else {
+                    }
-                        SkDEBUGFAIL("for-loops should always have unroll info in an ES2 program");
+                    if (this->visitStatement(*forStmt.statement())) {
+                        earlyExit = true;
                     }
 
+                    // ES2 programs always have a known unroll count. Non-ES2 programs don't enforce
+                    // a maximum program size, so it's fine to treat the loop as executing once.
+                    if (const LoopUnrollInfo* unrollInfo = forStmt.unrollInfo()) {
+                        fFunctionSize = SkSafeMath::Mul(fFunctionSize, unrollInfo->fCount);
+                    }
                     fFunctionSize = SkSafeMath::Add(fFunctionSize, originalFunctionSize);
-                    return result;
+                    return earlyExit;
                 }
 
                 case Statement::Kind::kExpression:
@@ -151,17 +157,15 @@ bool Analysis::CheckProgramStructure(const Program& program) {
                     // These statements don't directly consume any space in a compiled program.
                     break;
 
-                case Statement::Kind::kDo:
-                    SkDEBUGFAIL("encountered a statement that shouldn't exist in an ES2 program");
-                    break;
-
                 default:
+                    // Note that we don't make any attempt to estimate the number of iterations of
+                    // do-while loops here. Those aren't an ES2 construct so we aren't enforcing
+                    // program size on them.
                     fFunctionSize = SkSafeMath::Add(fFunctionSize, kStatementCost);
                     break;
             }
 
-            bool earlyExit = fFunctionSize > kProgramSizeLimit;
+            return INHERITED::visitStatement(stmt);
-            return earlyExit || INHERITED::visitStatement(stmt);
         }
 
         bool visitExpression(const Expression& expr) override {
@@ -206,7 +210,8 @@ bool Analysis::CheckProgramStructure(const Program& program) {
             // even in unreferenced functions.
             visitor.visitProgramElement(*element);
             // Report an error when main()'s flattened size is larger than our program limit.
-            if (visitor.functionSize() > kProgramSizeLimit &&
+            if (enforceSizeLimit &&
+                visitor.functionSize() > kProgramSizeLimit &&
                 element->as<FunctionDefinition>().declaration().isMain()) {
                 context.fErrors->error(Position(), "program is too large");
             }