Interpreter: Disallow return statements inside loops or conditionals

These didn't work correctly, and they're extremely tricky to get right
in the vectorized execution model (vs. structured control flow). As a
side effect, determine the maximum stack depth used for the execution
masking - the same idea will be used for the primary stack in a later
CL. Add a unit test to verify the new restriction, and fix two places
that were relying on this feature before.

In addition, boolean external values need to be masks. I may implement
this in the code-gen at some point, but this is already a fringe
feature, so just fix the one unit test for now.

Change-Id: I9607ffaf67c7795dbf42e4009180aea8f3e65c44
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/226849
Reviewed-by: Mike Klein <mtklein@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>

bench/SkSLInterpreterBench.cpp

@@ -89,10 +89,10 @@ const char* kHighContrastFilterSrc = R"(
     half hue2rgb_Stage2(half p, half q, half t) {
         if (t < 0)  t += 1;
         if (t > 1)  t -= 1;
-        if (t < 1 / 6.)  return p + (q - p) * 6 * t;
-        if (t < 1 / 2.)  return q;
-        if (t < 2 / 3.)  return p + (q - p) * (2 / 3. - t) * 6;
-        return p;
+        return (t < 1 / 6.) ? p + (q - p) * 6 * t
+             : (t < 1 / 2.) ? q
+             : (t < 2 / 3.) ? p + (q - p) * (2 / 3. - t) * 6
+             : p;
     }
     half max(half a, half b) { return a > b ? a : b; }
     half min(half a, half b) { return a < b ? a : b; }

src/sksl/SkSLByteCode.h

@@ -158,6 +158,8 @@ struct ByteCodeFunction {
     // TODO: Compute this value analytically. For now, just pick an arbitrary value that we probably
     // won't overflow.
     int fStackCount = 128;
+    int fConditionCount = 0;
+    int fLoopCount = 0;
     int fReturnCount = 0;
     std::vector<uint8_t> fCode;
 

src/sksl/SkSLByteCodeGenerator.cpp

@@ -88,11 +88,20 @@ std::unique_ptr<ByteCodeFunction> ByteCodeGenerator::writeFunction(const Functio
     fFunction = &f;
     std::unique_ptr<ByteCodeFunction> result(new ByteCodeFunction(&f.fDeclaration));
     fParameterCount = result->fParameterCount;
+    fLoopCount = fMaxLoopCount = 0;
+    fConditionCount = fMaxConditionCount = 0;
     fCode = &result->fCode;
+
     this->writeStatement(*f.fBody);
+    SkASSERT(fLoopCount == 0);
+    SkASSERT(fConditionCount == 0);
     this->write(ByteCodeInstruction::kReturn);
     this->write8(0);
-    result->fLocalCount = fLocals.size();
+
+    result->fLocalCount     = fLocals.size();
+    result->fConditionCount = fMaxConditionCount;
+    result->fLoopCount      = fMaxLoopCount;
+
     const Type& returnType = f.fDeclaration.fReturnType;
     if (returnType != *fContext.fVoid_Type) {
         result->fReturnCount = SlotCount(returnType);
@@ -319,6 +328,15 @@ void ByteCodeGenerator::write32(uint32_t i) {
 }
 
 void ByteCodeGenerator::write(ByteCodeInstruction i) {
+    switch (i) {
+        case ByteCodeInstruction::kLoopBegin: this->enterLoop();      break;
+        case ByteCodeInstruction::kLoopEnd:   this->exitLoop();       break;
+
+        case ByteCodeInstruction::kMaskPush:  this->enterCondition(); break;
+        case ByteCodeInstruction::kMaskPop:
+        case ByteCodeInstruction::kMaskBlend: this->exitCondition();  break;
+        default: /* Do nothing */ break;
+    }
     this->write16((uint16_t)i);
 }
 
@@ -702,6 +720,10 @@ void ByteCodeGenerator::writeFunctionCall(const FunctionCall& f) {
     this->write(ByteCodeInstruction::kCall);
     this->write8(idx);
 
+    const ByteCodeFunction* callee = fOutput->fFunctions[idx].get();
+    fMaxLoopCount      = std::max(fMaxLoopCount,      fLoopCount      + callee->fLoopCount);
+    fMaxConditionCount = std::max(fMaxConditionCount, fConditionCount + callee->fConditionCount);
+
     // After the called function returns, the stack will still contain our arguments. We have to
     // pop them (storing any out parameters back to their lvalues as we go). We glob together slot
     // counts for all parameters that aren't out-params, so we can pop them in one big chunk.
@@ -1147,6 +1169,10 @@ void ByteCodeGenerator::writeIfStatement(const IfStatement& i) {
 }
 
 void ByteCodeGenerator::writeReturnStatement(const ReturnStatement& r) {
+    if (fLoopCount || fConditionCount) {
+        fErrors.error(r.fOffset, "return not allowed inside conditional or loop");
+        return;
+    }
     this->writeExpression(*r.fExpression);
     this->write(ByteCodeInstruction::kReturn);
     this->write8(SlotCount(r.fExpression->fType));

src/sksl/SkSLByteCodeGenerator.h

@@ -8,8 +8,8 @@
 #ifndef SKSL_BYTECODEGENERATOR
 #define SKSL_BYTECODEGENERATOR
 
+#include <algorithm>
 #include <stack>
-#include <tuple>
 #include <unordered_map>
 
 #include "src/sksl/SkSLByteCode.h"
@@ -247,6 +247,26 @@ private:
     // updates the current set of continues to branch to the current location
     void setContinueTargets();
 
+    void enterLoop() {
+        fLoopCount++;
+        fMaxLoopCount = std::max(fMaxLoopCount, fLoopCount);
+    }
+
+    void exitLoop() {
+        SkASSERT(fLoopCount > 0);
+        fLoopCount--;
+    }
+
+    void enterCondition() {
+        fConditionCount++;
+        fMaxConditionCount = std::max(fMaxConditionCount, fConditionCount);
+    }
+
+    void exitCondition() {
+        SkASSERT(fConditionCount > 0);
+        fConditionCount--;
+    }
+
     const Context& fContext;
 
     ByteCode* fOutput;
@@ -265,6 +285,11 @@ private:
 
     int fParameterCount;
 
+    int fLoopCount;
+    int fMaxLoopCount;
+    int fConditionCount;
+    int fMaxConditionCount;
+
     const std::unordered_map<String, Intrinsic> fIntrinsics;
 
     friend class DeferredLocation;

tests/SkSLInterpreterTest.cpp

@@ -601,7 +601,6 @@ DEF_TEST(SkSLInterpreterCompound, r) {
 }
 
 DEF_TEST(SkSLInterpreterRestrictFunctionCalls, r) {
-    // Helper to validate that a chunk of code can not be converted to byte code
     auto check = [r](const char* src) {
         SkSL::Compiler compiler;
         auto program = compiler.convertProgram(SkSL::Program::kGeneric_Kind, SkSL::String(src),
@@ -618,6 +617,10 @@ DEF_TEST(SkSLInterpreterRestrictFunctionCalls, r) {
 
     // Ensure that calls to undefined functions are not allowed (to prevent mutual recursion)
     check("float foo(); float bar() { return foo(); } float foo() { return bar(); }");
+
+    // returns are not allowed inside conditionals (or loops, which are effectively the same thing)
+    check("float main(float x, float y) { if (x < y) { return x; } return y; }");
+    check("float main(float x) { while (x > 1) { return x; } return 0; }");
 }
 
 DEF_TEST(SkSLInterpreterFunctions, r) {
@@ -781,7 +784,8 @@ public:
         } else if (type() == *fCompiler.context().fFloat_Type) {
             *(float*) target = *fValue.as<skjson::NumberValue>();
         } else if (type() == *fCompiler.context().fBool_Type) {
-            *(bool*) target = *fValue.as<skjson::BoolValue>();
+            // ByteCode "booleans" are actually bit-masks
+            *(int*) target = *fValue.as<skjson::BoolValue>() ? ~0 : 0;
         } else {
             SkASSERT(false);
         }
@@ -841,9 +845,7 @@ DEF_TEST(SkSLInterpreterExternalValues, r) {
     SkSL::Program::Settings settings;
     const char* src = "float main() {"
                       "    outValue = 152;"
-                      "    if (root.child.value2)"
-                      "        return root.value1 * root.child.value3;"
-                      "    return -1;"
+                      "    return root.child.value2 ? root.value1 * root.child.value3 : -1;"
                       "}";
     compiler.registerExternalValue((SkSL::ExternalValue*) compiler.takeOwnership(
              std::unique_ptr<SkSL::Symbol>(new JSONExternalValue("root", &dom.root(), &compiler))));