more skslc hardening

src/sksl/SkSLIRGenerator.cpp

 /*
  * Copyright 2016 Google Inc.
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
  
 #include "SkSLIRGenerator.h"
 
 #include "limits.h"
@@ skipping 186 matching lines @@
         auto var = std::unique_ptr<Variable>(new Variable(decl.fPosition, modifiers, varDecl.fName, 
                                                           *type, storage));
         std::unique_ptr<Expression> value;
         if (varDecl.fValue) {
             value = this->convertExpression(*varDecl.fValue);
             if (!value) {
                 return nullptr;
             }
             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];
             old->fModifiers = var->fModifiers;
         } else {
             variables.emplace_back(var.get(), std::move(sizes), std::move(value));
             fSymbolTable->add(varDecl.fName, std::move(var));
         }
     }
     return std::unique_ptr<VarDeclarations>(new VarDeclarations(decl.fPosition, 
@@ skipping 462 matching lines @@
  */
 static bool determine_binary_type(const Context& context, 
                                   Token::Kind op, 
                                   const Type& left, 
                                   const Type& right, 
                                   const Type** outLeftType,
                                   const Type** outRightType,
                                   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
         case Token::LOGICALXOR: // fall through
         case Token::LOGICALOREQ: // fall through
         case Token::LOGICALANDEQ: // fall through
         case Token::LOGICALXOREQ:
             *outLeftType = context.fBool_Type.get();
             *outRightType = context.fBool_Type.get();
             *outResultType = context.fBool_Type.get();
@@ skipping 30 matching lines @@
                     } else {
                         // result was a column vector, transpose it back to a row
                         *outResultType = &(*outResultType)->toCompound(context, leftRows,
                                                                        rightColumns);
                     }
                     return leftColumns == rightRows;
                 } else {
                     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) {
             *outResultType = context.fBool_Type.get();
         } else {
             *outResultType = &right;
         }
         return true;
     }
     if ((left.kind() == Type::kVector_Kind || left.kind() == Type::kMatrix_Kind) && 
@@ skipping 49 matching lines @@
         case Token::BITWISEOREQ:  // fall through
         case Token::BITWISEXOREQ: // fall through
         case Token::BITWISEANDEQ: // fall through
         case Token::LOGICALOREQ:  // fall through
         case Token::LOGICALXOREQ: // fall through
         case Token::LOGICALANDEQ: 
             this->markWrittenTo(*left);
         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));
 }
 
 std::unique_ptr<Expression> IRGenerator::convertTernaryExpression(  
                                                            const ASTTernaryExpression& expression) {
     std::unique_ptr<Expression> test = this->coerce(this->convertExpression(*expression.fTest), 
                                                     *fContext.fBool_Type);
     if (!test) {
         return nullptr;
     }
@@ skipping 33 matching lines @@
                                  " argument";
         if (function.fParameters.size() != 1) {
             msg += "s";
         }
         msg += ", but found " + to_string((uint64_t) arguments.size());
         fErrors.error(position, msg);
         return nullptr;
     }
     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]);
         }
     }
     return std::unique_ptr<FunctionCall>(new FunctionCall(position, function,
                                                           std::move(arguments)));
 }
 
 /**
  * Determines the cost of coercing the arguments of a function to the required types. Returns true 
@@ skipping 77 matching lines @@
     }
     if (args.size() == 1 && args[0]->fType == type) {
         // argument is already the right type, just return it
         return std::move(args[0]);
     }
     if (type.isNumber()) {
         if (args.size() != 1) {
             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));
             std::unique_ptr<IntLiteral> one(new IntLiteral(fContext, position, 1));
             return std::unique_ptr<Expression>(
                                          new TernaryExpression(position, std::move(args[0]),
                                                                this->coerce(std::move(one), type),
                                                                this->coerce(std::move(zero), 
                                                                             type)));
         } else if (!args[0]->fType.isNumber()) {
             fErrors.error(position, "invalid argument to '" + type.description() + 
                                     "' constructor (expected a number or bool, but found '" +
                                     args[0]->fType.description() + "')");
         }
         if (args[0]->fKind == Expression::kIntLiteral_Kind && (type == *fContext.fInt_Type || 
             type == *fContext.fUInt_Type)) {
             return std::unique_ptr<Expression>(new IntLiteral(fContext, 
                                                               position, 
                                                               ((IntLiteral&) *args[0]).fValue,
                                                               &type));
         }
     } else if (kind == Type::kArray_Kind) {
         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);
         int actual = 0;
         for (size_t i = 0; i < args.size(); i++) {
             if (args[i]->fType.kind() == Type::kVector_Kind || 
                 args[i]->fType.kind() == Type::kMatrix_Kind) {
                 int columns = args[i]->fType.columns();
                 int rows = args[i]->fType.rows();
                 args[i] = this->coerce(std::move(args[i]), 
@@ skipping 263 matching lines @@
 
 void IRGenerator::checkValid(const Expression& expr) {
     switch (expr.fKind) {
         case Expression::kFunctionReference_Kind:
             fErrors.error(expr.fPosition, "expected '(' to begin function call");
             break;
         case Expression::kTypeReference_Kind:
             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");
+            }
     }
 }
 
 void IRGenerator::markReadFrom(const Variable& var) {
     var.fIsReadFrom = true;
 }
 
 static bool has_duplicates(const Swizzle& swizzle) {
     int bits = 0;
     for (int idx : swizzle.fComponents) {
@@ skipping 31 matching lines @@
         case Expression::kIndex_Kind:
             this->markWrittenTo(*((IndexExpression&) expr).fBase);
             break;
         default:
             fErrors.error(expr.fPosition, "cannot assign to '" + expr.description() + "'");
             break;
     }
 }
 
 }