more skslc hardening
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2427693002
Review-Url: https://codereview.chromium.org/2427693002
diff --git a/src/sksl/SkSLIRGenerator.cpp b/src/sksl/SkSLIRGenerator.cpp
index b93cbfb..07fd9e2 100644
--- a/src/sksl/SkSLIRGenerator.cpp
+++ b/src/sksl/SkSLIRGenerator.cpp
@@ -204,10 +204,11 @@
}
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];
@@ -677,6 +678,29 @@
}
return left.kind() == Type::kVector_Kind && right.kind() == Type::kMatrix_Kind;
}
+
+static bool is_assignment(Token::Kind op) {
+ switch (op) {
+ case Token::EQ: // fall through
+ case Token::PLUSEQ: // fall through
+ case Token::MINUSEQ: // fall through
+ case Token::STAREQ: // fall through
+ case Token::SLASHEQ: // fall through
+ case Token::PERCENTEQ: // fall through
+ case Token::SHLEQ: // fall through
+ case Token::SHREQ: // fall through
+ 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:
+ return true;
+ default:
+ return false;
+ }
+}
+
/**
* Determines the operand and result types of a binary expression. Returns true if the expression is
* legal, false otherwise. If false, the values of the out parameters are undefined.
@@ -690,14 +714,24 @@
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,13 +782,26 @@
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) {
+ bool isVectorOrMatrix = left.kind() == Type::kVector_Kind || left.kind() == Type::kMatrix_Kind;
+ // FIXME: incorrect for shift
+ if (right.canCoerceTo(left) && (left.kind() == Type::kScalar_Kind ||
+ (isVectorOrMatrix && validMatrixOrVectorOp))) {
*outLeftType = &left;
*outRightType = &left;
if (isLogical) {
@@ -764,17 +811,6 @@
}
return true;
}
- // FIXME: incorrect for shift operations
- if (left.canCoerceTo(right)) {
- *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) &&
(right.kind() == Type::kScalar_Kind)) {
if (determine_binary_type(context, op, left.componentType(), right, outLeftType,
@@ -809,38 +845,25 @@
const Type* rightType;
const Type* resultType;
if (!determine_binary_type(fContext, expression.fOperator, left->fType, right->fType, &leftType,
- &rightType, &resultType, true)) {
+ &rightType, &resultType, !is_assignment(expression.fOperator))) {
fErrors.error(expression.fPosition, "type mismatch: '" +
Token::OperatorName(expression.fOperator) +
"' cannot operate on '" + left->fType.fName +
"', '" + right->fType.fName + "'");
return nullptr;
}
- switch (expression.fOperator) {
- case Token::EQ: // fall through
- case Token::PLUSEQ: // fall through
- case Token::MINUSEQ: // fall through
- case Token::STAREQ: // fall through
- case Token::SLASHEQ: // fall through
- case Token::PERCENTEQ: // fall through
- case Token::SHLEQ: // fall through
- case Token::SHREQ: // fall through
- 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;
+ if (is_assignment(expression.fOperator)) {
+ this->markWrittenTo(*left);
+ }
+ 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 +917,9 @@
}
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 +1017,7 @@
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 +1043,9 @@
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 +1329,9 @@
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");
+ }
}
}
diff --git a/src/sksl/ir/SkSLTypeReference.h b/src/sksl/ir/SkSLTypeReference.h
index 10f36aa..67e0466 100644
--- a/src/sksl/ir/SkSLTypeReference.h
+++ b/src/sksl/ir/SkSLTypeReference.h
@@ -23,8 +23,7 @@
, fValue(type) {}
std::string description() const override {
- ASSERT(false);
- return "<type>";
+ return fValue.name();
}
const Type& fValue;
diff --git a/tests/SkSLErrorTest.cpp b/tests/SkSLErrorTest.cpp
index d910948..47e31e6 100644
--- a/tests/SkSLErrorTest.cpp
+++ b/tests/SkSLErrorTest.cpp
@@ -139,10 +139,19 @@
"void main() { vec4 test = vec4(1); test.xyyz = vec4(1); }",
"error: 1: cannot write to the same swizzle field more than once\n1 error\n");
}
+
DEF_TEST(SkSLAssignmentTypeMismatch, r) {
test_failure(r,
"void main() { int x = 1.0; }",
"error: 1: expected 'int', but found 'float'\n1 error\n");
+ test_failure(r,
+ "void main() { int x; x = 1.0; }",
+ "error: 1: type mismatch: '=' cannot operate on 'int', 'float'\n1 error\n");
+ test_success(r,
+ "void main() { vec3 x = vec3(0); x *= 1.0; }");
+ test_failure(r,
+ "void main() { ivec3 x = ivec3(0); x *= 1.0; }",
+ "error: 1: type mismatch: '*=' cannot operate on 'ivec3', 'float'\n1 error\n");
}
DEF_TEST(SkSLReturnFromVoid, r) {