sksl optimizer fixes

The main issue was a use-after-free due to removing (and thus destroying)
the binary expression prior to re-adding part of it. Also cleaned up the
way dead assignments are handled and added the test that originally
identified these problems.

Bug: skia:
Change-Id: Icda93d69a66c4e57850ecdc88fc4a4f634e1aac2
Reviewed-on: https://skia-review.googlesource.com/15383
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Reviewed-by: Ben Wagner <benjaminwagner@google.com>

src/sksl/SkSLCompiler.cpp

@@ -430,11 +430,26 @@ void delete_left(BasicBlock* b,
                  std::vector<BasicBlock::Node>::iterator* iter,
                  bool* outUpdated,
                  bool* outNeedsRescan) {
-    ASSERT((*(*iter)->expression())->fKind == Expression::kBinary_Kind);
     *outUpdated = true;
-    if (!try_replace_expression(b, iter, &((BinaryExpression&) **(*iter)->expression()).fRight)) {
-        *outNeedsRescan = true;
+    std::unique_ptr<Expression>* target = (*iter)->expression();
+    ASSERT((*target)->fKind == Expression::kBinary_Kind);
+    BinaryExpression& bin = (BinaryExpression&) **target;
+    bool result;
+    if (bin.fOperator == Token::EQ) {
+        result = !b->tryRemoveLValueBefore(iter, bin.fLeft.get());
+    } else {
+        result = !b->tryRemoveExpressionBefore(iter, bin.fLeft.get());
     }
+    if (!result) {
+        *target = std::move(bin.fRight);
+        *outNeedsRescan = true;
+        return;
+    }
+    --(*iter);
+    ASSERT((*iter)->expression() == &bin.fRight);
+    *iter = b->fNodes.erase(*iter);
+    ASSERT((*iter)->expression() == target);
+    *target = std::move(bin.fRight);
 }
 
 /**
@@ -445,11 +460,20 @@ void delete_right(BasicBlock* b,
                   std::vector<BasicBlock::Node>::iterator* iter,
                   bool* outUpdated,
                   bool* outNeedsRescan) {
-    ASSERT((*(*iter)->expression())->fKind == Expression::kBinary_Kind);
     *outUpdated = true;
-    if (!try_replace_expression(b, iter, &((BinaryExpression&) **(*iter)->expression()).fLeft)) {
+    std::unique_ptr<Expression>* target = (*iter)->expression();
+    ASSERT((*target)->fKind == Expression::kBinary_Kind);
+    BinaryExpression& bin = (BinaryExpression&) **target;
+    if (!b->tryRemoveExpressionBefore(iter, bin.fRight.get())) {
+        *target = std::move(bin.fLeft);
         *outNeedsRescan = true;
+        return;
     }
+    --(*iter);
+    ASSERT((*iter)->expression() == &bin.fLeft);
+    *iter = b->fNodes.erase(*iter);
+    ASSERT((*iter)->expression() == target);
+    *target = std::move(bin.fLeft);
 }
 
 /**
@@ -580,8 +604,12 @@ void Compiler::simplifyExpression(DefinitionMap& definitions,
             break;
         }
         case Expression::kBinary_Kind: {
-            // collapse useless expressions like x * 1 or x + 0
             BinaryExpression* bin = (BinaryExpression*) expr;
+            if (dead_assignment(*bin)) {
+                delete_left(&b, iter, outUpdated, outNeedsRescan);
+                break;
+            }
+            // collapse useless expressions like x * 1 or x + 0
             if (((bin->fLeft->fType.kind()  != Type::kScalar_Kind) &&
                  (bin->fLeft->fType.kind()  != Type::kVector_Kind)) ||
                 ((bin->fRight->fType.kind() != Type::kScalar_Kind) &&
@@ -795,28 +823,6 @@ void Compiler::simplifyStatement(DefinitionMap& definitions,
         case Statement::kExpression_Kind: {
             ExpressionStatement& e = (ExpressionStatement&) *stmt;
             ASSERT((*iter)->statement()->get() == &e);
-            if (e.fExpression->fKind == Expression::kBinary_Kind) {
-                BinaryExpression& bin = (BinaryExpression&) *e.fExpression;
-                if (dead_assignment(bin)) {
-                    if (!b.tryRemoveExpressionBefore(iter, &bin)) {
-                        *outNeedsRescan = true;
-                    }
-                    if (bin.fRight->hasSideEffects()) {
-                        // still have to evaluate the right due to side effects,
-                        // replace the binary expression with just the right side
-                        e.fExpression = std::move(bin.fRight);
-                        if (!b.tryInsertExpression(iter, &e.fExpression)) {
-                            *outNeedsRescan = true;
-                        }
-                    } else {
-                        // no side effects, kill the whole statement
-                        ASSERT((*iter)->statement()->get() == stmt);
-                        (*iter)->setStatement(std::unique_ptr<Statement>(new Nop()));
-                    }
-                    *outUpdated = true;
-                    break;
-                }
-            }
             if (!e.fExpression->hasSideEffects()) {
                 // Expression statement with no side effects, kill it
                 if (!b.tryRemoveExpressionBefore(iter, e.fExpression.get())) {

tests/SkSLGLSLTest.cpp

@@ -1213,4 +1213,21 @@ DEF_TEST(SkSLUnusedVars, r) {
          "}\n");
 }
 
+DEF_TEST(SkSLMultipleAssignments, r) {
+    test(r,
+         "void main() {"
+         "float x;"
+         "float y;"
+         "int z;"
+         "x = y = z = 1;"
+         "sk_FragColor = vec4(z);"
+         "}",
+         *SkSL::ShaderCapsFactory::Default(),
+         "#version 400\n"
+         "out vec4 sk_FragColor;\n"
+         "void main() {\n"
+         "    sk_FragColor = vec4(1.0);\n"
+         "}\n");
+}
+
 #endif