more skslc hardening

src/sksl/SkSLIRGenerator.cpp

Index: src/sksl/SkSLIRGenerator.cpp
diff --git a/src/sksl/SkSLIRGenerator.cpp b/src/sksl/SkSLIRGenerator.cpp
index b93cbfbe9931495765f20b2e8f86e614c59c0980..e70af8fd0c710ad29ea901df4452465105cda795 100644
--- a/src/sksl/SkSLIRGenerator.cpp
+++ b/src/sksl/SkSLIRGenerator.cpp
@@ -204,10 +204,11 @@ std::unique_ptr<VarDeclarations> IRGenerator::convertVarDeclarations(const ASTVa
             }
             value = this->coerce(std::move(value), *type);
         }
-        if ("sk_FragColor" == varDecl.fName && (*fSymbolTable)[varDecl.fName]) {
+        if (storage == Variable::kGlobal_Storage && "sk_FragColor" == varDecl.fName && 
+            (*fSymbolTable)[varDecl.fName]) {
             // already defined, ignore
-        } else if ((*fSymbolTable)[varDecl.fName] && 
-                   (*fSymbolTable)[varDecl.fName]->fKind == Symbol::kVariable_Kind && 
+        } else if (storage == Variable::kGlobal_Storage && (*fSymbolTable)[varDecl.fName] &&
+                   (*fSymbolTable)[varDecl.fName]->fKind == Symbol::kVariable_Kind &&
                    ((Variable*) (*fSymbolTable)[varDecl.fName])->fModifiers.fLayout.fBuiltin >= 0) {
             // already defined, just update the modifiers
             Variable* old = (Variable*) (*fSymbolTable)[varDecl.fName];
@@ -690,14 +691,24 @@ static bool determine_binary_type(const Context& context,
                                   const Type** outResultType,
                                   bool tryFlipped) {
     bool isLogical;
+    bool validMatrixOrVectorOp;
     switch (op) {
+        case Token::EQ:
+            *outLeftType = &left;
+            *outRightType = &left;
+            *outResultType = &left;
+            return right.canCoerceTo(left);
         case Token::EQEQ: // fall through
-        case Token::NEQ:  // fall through
+        case Token::NEQ:
+            isLogical = true;
+            validMatrixOrVectorOp = true;
+            break;
         case Token::LT:   // fall through
         case Token::GT:   // fall through
         case Token::LTEQ: // fall through
         case Token::GTEQ:
             isLogical = true;
+            validMatrixOrVectorOp = false;
             break;
         case Token::LOGICALOR: // fall through
         case Token::LOGICALAND: // fall through
@@ -748,24 +759,25 @@ static bool determine_binary_type(const Context& context,
                     return false;
                 }
             }
-            // fall through
+            isLogical = false;
+            validMatrixOrVectorOp = true;
+            break;
+        case Token::PLUS:    // fall through
+        case Token::PLUSEQ:  // fall through
+        case Token::MINUS:   // fall through
+        case Token::MINUSEQ: // fall through
+        case Token::SLASH:   // fall through
+        case Token::SLASHEQ:
+            isLogical = false;
+            validMatrixOrVectorOp = true;
+            break;
         default:
             isLogical = false;
+            validMatrixOrVectorOp = false;
     }
-    // FIXME: need to disallow illegal operations like vec3 > vec3. Also do not currently have
-    // full support for numbers other than float.
-    if (left == right) {
-        *outLeftType = &left;
-        *outRightType = &left;
-        if (isLogical) {
-            *outResultType = context.fBool_Type.get();
-        } else {
-            *outResultType = &left;
-        }
-        return true;
-    }
-    // FIXME: incorrect for shift operations

[dogben, 2016/10/17 15:30:36]

I think this still applies.

-    if (left.canCoerceTo(right)) {
+    bool isVectorOrMatrix = left.kind() == Type::kVector_Kind || left.kind() == Type::kMatrix_Kind;
+    if (left.canCoerceTo(right) && (left.kind() == Type::kScalar_Kind || 

[dogben, 2016/10/17 15:30:36]

I don't think this is correct for the FOOEQ cases. E.g.:
float x = 1.0;
int y = 1;
y *= x;

Left is int, right is float, and this condition is true, but I think in this
case the function should return false.

+                                   (isVectorOrMatrix && validMatrixOrVectorOp))) {
         *outLeftType = &right;
         *outRightType = &right;
         if (isLogical) {
@@ -835,12 +847,15 @@ std::unique_ptr<Expression> IRGenerator::convertBinaryExpression(
         default:
             break;
     }
+    left = this->coerce(std::move(left), *leftType);
+    right = this->coerce(std::move(right), *rightType);
+    if (!left || !right) {
+        return nullptr;
+    }
     return std::unique_ptr<Expression>(new BinaryExpression(expression.fPosition, 
-                                                            this->coerce(std::move(left), 
-                                                                         *leftType), 
+                                                            std::move(left), 
                                                             expression.fOperator, 
-                                                            this->coerce(std::move(right), 
-                                                                         *rightType), 
+                                                            std::move(right), 
                                                             *resultType));
 }
 
@@ -894,6 +909,9 @@ std::unique_ptr<Expression> IRGenerator::call(Position position,
     }
     for (size_t i = 0; i < arguments.size(); i++) {
         arguments[i] = this->coerce(std::move(arguments[i]), function.fParameters[i]->fType);
+        if (!arguments[i]) {
+            return nullptr;
+        }
         if (arguments[i] && (function.fParameters[i]->fModifiers.fFlags & Modifiers::kOut_Flag)) {
             this->markWrittenTo(*arguments[i]);
         }
@@ -991,6 +1009,7 @@ std::unique_ptr<Expression> IRGenerator::convertConstructor(
             fErrors.error(position, "invalid arguments to '" + type.description() + 
                                     "' constructor, (expected exactly 1 argument, but found " +
                                     to_string((uint64_t) args.size()) + ")");
+            return nullptr;
         }
         if (args[0]->fType == *fContext.fBool_Type) {
             std::unique_ptr<IntLiteral> zero(new IntLiteral(fContext, position, 0));
@@ -1016,6 +1035,9 @@ std::unique_ptr<Expression> IRGenerator::convertConstructor(
         const Type& base = type.componentType();
         for (size_t i = 0; i < args.size(); i++) {
             args[i] = this->coerce(std::move(args[i]), base);
+            if (!args[i]) {
+                return nullptr;
+            }
         }
     } else {
         ASSERT(kind == Type::kVector_Kind || kind == Type::kMatrix_Kind);
@@ -1299,8 +1321,9 @@ void IRGenerator::checkValid(const Expression& expr) {
             fErrors.error(expr.fPosition, "expected '(' to begin constructor invocation");
             break;
         default:
-            ASSERT(expr.fType != *fContext.fInvalid_Type);
-            break;
+            if (expr.fType == *fContext.fInvalid_Type) {
+                fErrors.error(expr.fPosition, "invalid expression");
+            }
     }
 }