Fix assertion when arrays are double-declared.
Multi-dimensional arrays aren't legal in GLSL/SkSL, so this should be
caught and flagged as an error. The parser now verifies that a
variable's type isn't an array-type before accepting a `[` token to
open an array on the variable name.
This CL also refactors the IR generator's `convertArraySize` method to
make sure that various checks are made for all callers. Originally this
restructuring was used to verify array multi-dimensionality, but that
didn't detect errors inside struct declarations (which get no error
checking inside the IR generator) so the IR generator updates no longer
need to check the array dimensions.
Bug: skia:11322
Change-Id: Id33f4bdfb544019ddf995a8196c3c09cfe5a4525
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/369916
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
diff --git a/gn/sksl_tests.gni b/gn/sksl_tests.gni
index 14dd410..f738bfb 100644
--- a/gn/sksl_tests.gni
+++ b/gn/sksl_tests.gni
@@ -70,8 +70,21 @@
"/sksl/errors/ArgumentModifiers.sksl",
"/sksl/errors/ArrayConstructorElementCount.sksl",
"/sksl/errors/ArrayIndexOutOfRange.sksl",
+ "/sksl/errors/ArrayOfVoid.sksl",
+ "/sksl/errors/ArrayOfVoidInStruct.sksl",
"/sksl/errors/ArrayReturnTypes.sksl",
+ "/sksl/errors/ArraySplitDimensions.sksl",
+ "/sksl/errors/ArraySplitDimensionsInFuncBody.sksl",
+ "/sksl/errors/ArraySplitDimensionsInFuncDecl.sksl",
+ "/sksl/errors/ArraySplitDimensionsInStruct.sksl",
"/sksl/errors/ArrayTooManyDimensions.sksl",
+ "/sksl/errors/ArrayTooManyDimensionsInFuncBody.sksl",
+ "/sksl/errors/ArrayTooManyDimensionsInFuncDecl.sksl",
+ "/sksl/errors/ArrayTooManyDimensionsInStruct.sksl",
+ "/sksl/errors/ArrayTypeTooManyDimensions.sksl",
+ "/sksl/errors/ArrayTypeTooManyDimensionsInFuncBody.sksl",
+ "/sksl/errors/ArrayTypeTooManyDimensionsInFuncDecl.sksl",
+ "/sksl/errors/ArrayTypeTooManyDimensionsInStruct.sksl",
"/sksl/errors/ArrayUnspecifiedDimensions.sksl",
"/sksl/errors/AssignmentTypeMismatch.sksl",
"/sksl/errors/BadCaps.sksl",
diff --git a/resources/sksl/errors/ArrayOfVoid.sksl b/resources/sksl/errors/ArrayOfVoid.sksl
new file mode 100644
index 0000000..c56cdb1
--- /dev/null
+++ b/resources/sksl/errors/ArrayOfVoid.sksl
@@ -0,0 +1,10 @@
+// Expect 7 errors
+
+void a[2];
+void[2] b;
+
+void funcD(void d[2]) {}
+void funcE(void[2] e) {}
+void[2] funcF() {}
+void funcG() { void g[2]; }
+void funcH() { void[2] h; }
diff --git a/resources/sksl/errors/ArrayOfVoidInStruct.sksl b/resources/sksl/errors/ArrayOfVoidInStruct.sksl
new file mode 100644
index 0000000..24fe326
--- /dev/null
+++ b/resources/sksl/errors/ArrayOfVoidInStruct.sksl
@@ -0,0 +1,7 @@
+// Expect 3 errors
+
+struct S {
+ void a[2];
+ void[2] b;
+ void[2] c[2];
+};
diff --git a/resources/sksl/errors/ArraySplitDimensions.sksl b/resources/sksl/errors/ArraySplitDimensions.sksl
new file mode 100644
index 0000000..b98afc2
--- /dev/null
+++ b/resources/sksl/errors/ArraySplitDimensions.sksl
@@ -0,0 +1 @@
+float[2] x[2];
diff --git a/resources/sksl/errors/ArraySplitDimensionsInFuncBody.sksl b/resources/sksl/errors/ArraySplitDimensionsInFuncBody.sksl
new file mode 100644
index 0000000..76c4f08
--- /dev/null
+++ b/resources/sksl/errors/ArraySplitDimensionsInFuncBody.sksl
@@ -0,0 +1 @@
+void func() { float[2] x[2]; }
diff --git a/resources/sksl/errors/ArraySplitDimensionsInFuncDecl.sksl b/resources/sksl/errors/ArraySplitDimensionsInFuncDecl.sksl
new file mode 100644
index 0000000..f1a0dfe
--- /dev/null
+++ b/resources/sksl/errors/ArraySplitDimensionsInFuncDecl.sksl
@@ -0,0 +1 @@
+void func(float[2] x[2]) {}
diff --git a/resources/sksl/errors/ArraySplitDimensionsInStruct.sksl b/resources/sksl/errors/ArraySplitDimensionsInStruct.sksl
new file mode 100644
index 0000000..2053da7
--- /dev/null
+++ b/resources/sksl/errors/ArraySplitDimensionsInStruct.sksl
@@ -0,0 +1 @@
+struct S { float[2] x[2]; }; // TODO(skia:11322): flag this as an error
diff --git a/resources/sksl/errors/ArrayTooManyDimensions.sksl b/resources/sksl/errors/ArrayTooManyDimensions.sksl
index 385bfb5..aca7490 100644
--- a/resources/sksl/errors/ArrayTooManyDimensions.sksl
+++ b/resources/sksl/errors/ArrayTooManyDimensions.sksl
@@ -1 +1 @@
-in int ar[2][4];
+float x[2][2];
diff --git a/resources/sksl/errors/ArrayTooManyDimensionsInFuncBody.sksl b/resources/sksl/errors/ArrayTooManyDimensionsInFuncBody.sksl
new file mode 100644
index 0000000..c067db2
--- /dev/null
+++ b/resources/sksl/errors/ArrayTooManyDimensionsInFuncBody.sksl
@@ -0,0 +1 @@
+void main() { float x[2][2]; }
diff --git a/resources/sksl/errors/ArrayTooManyDimensionsInFuncDecl.sksl b/resources/sksl/errors/ArrayTooManyDimensionsInFuncDecl.sksl
new file mode 100644
index 0000000..bd1bf03
--- /dev/null
+++ b/resources/sksl/errors/ArrayTooManyDimensionsInFuncDecl.sksl
@@ -0,0 +1 @@
+void func(float x[2][2]) {}
diff --git a/resources/sksl/errors/ArrayTooManyDimensionsInStruct.sksl b/resources/sksl/errors/ArrayTooManyDimensionsInStruct.sksl
new file mode 100644
index 0000000..4c15ed8
--- /dev/null
+++ b/resources/sksl/errors/ArrayTooManyDimensionsInStruct.sksl
@@ -0,0 +1 @@
+struct S { float x[2][2]; };
diff --git a/resources/sksl/errors/ArrayTypeTooManyDimensions.sksl b/resources/sksl/errors/ArrayTypeTooManyDimensions.sksl
new file mode 100644
index 0000000..9807106
--- /dev/null
+++ b/resources/sksl/errors/ArrayTypeTooManyDimensions.sksl
@@ -0,0 +1 @@
+float[2][2] x;
diff --git a/resources/sksl/errors/ArrayTypeTooManyDimensionsInFuncBody.sksl b/resources/sksl/errors/ArrayTypeTooManyDimensionsInFuncBody.sksl
new file mode 100644
index 0000000..099f5c9
--- /dev/null
+++ b/resources/sksl/errors/ArrayTypeTooManyDimensionsInFuncBody.sksl
@@ -0,0 +1 @@
+void main() { float[2][2] x; }
diff --git a/resources/sksl/errors/ArrayTypeTooManyDimensionsInFuncDecl.sksl b/resources/sksl/errors/ArrayTypeTooManyDimensionsInFuncDecl.sksl
new file mode 100644
index 0000000..2cf1ba8
--- /dev/null
+++ b/resources/sksl/errors/ArrayTypeTooManyDimensionsInFuncDecl.sksl
@@ -0,0 +1 @@
+void func(float[2][2] x) {}
diff --git a/resources/sksl/errors/ArrayTypeTooManyDimensionsInStruct.sksl b/resources/sksl/errors/ArrayTypeTooManyDimensionsInStruct.sksl
new file mode 100644
index 0000000..054dca0
--- /dev/null
+++ b/resources/sksl/errors/ArrayTypeTooManyDimensionsInStruct.sksl
@@ -0,0 +1 @@
+struct S { float[2][2] x; };
diff --git a/src/sksl/SkSLIRGenerator.cpp b/src/sksl/SkSLIRGenerator.cpp
index 4064c08..9800dfd 100644
--- a/src/sksl/SkSLIRGenerator.cpp
+++ b/src/sksl/SkSLIRGenerator.cpp
@@ -267,7 +267,7 @@
}
}
-int IRGenerator::convertArraySize(int offset, const ASTNode& s) {
+int IRGenerator::convertArraySize(const Type& type, int offset, const ASTNode& s) {
if (!s) {
this->errorReporter().error(offset, "array must have a size");
return 0;
@@ -276,14 +276,23 @@
if (!size) {
return 0;
}
- return this->convertArraySize(std::move(size));
+ return this->convertArraySize(type, std::move(size));
}
-int IRGenerator::convertArraySize(std::unique_ptr<Expression> size) {
+int IRGenerator::convertArraySize(const Type& type, std::unique_ptr<Expression> size) {
size = this->coerce(std::move(size), *fContext.fTypes.fInt);
if (!size) {
return 0;
}
+ if (type == *fContext.fTypes.fVoid) {
+ this->errorReporter().error(size->fOffset, "type 'void' may not be used in an array");
+ return 0;
+ }
+ if (type.isOpaque()) {
+ this->errorReporter().error(
+ size->fOffset, "opaque type '" + type.name() + "' may not be used in an array");
+ return 0;
+ }
if (!size->is<IntLiteral>()) {
this->errorReporter().error(size->fOffset, "array size must be an integer");
return 0;
@@ -421,13 +430,11 @@
const Type* type = baseType;
int arraySizeValue = 0;
if (isArray) {
- if (type->isOpaque()) {
- this->errorReporter().error(
- offset,
- "opaque type '" + type->name() + "' may not be used in an array");
- }
SkASSERT(arraySize);
- arraySizeValue = this->convertArraySize(std::move(arraySize));
+ arraySizeValue = this->convertArraySize(*type, std::move(arraySize));
+ if (!arraySizeValue) {
+ return {};
+ }
type = fSymbolTable->addArrayDimension(type, arraySizeValue);
}
auto var = std::make_unique<Variable>(offset, fModifiers->addToPool(modifiers),
@@ -1145,7 +1152,7 @@
return;
}
if (pd.fIsArray) {
- int arraySize = this->convertArraySize(param.fOffset, *paramIter++);
+ int arraySize = this->convertArraySize(*type, param.fOffset, *paramIter++);
if (!arraySize) {
return;
}
@@ -1405,7 +1412,7 @@
// convertArraySize rejects unsized arrays. This is the one place we allow those, but
// we've already checked for that, so this is verifying the other aspects (constant,
// positive, not too large).
- arraySize = this->convertArraySize(size.fOffset, size);
+ arraySize = this->convertArraySize(*type, size.fOffset, size);
if (!arraySize) {
return nullptr;
}
@@ -1531,30 +1538,18 @@
}
const Type* result = &symbol->as<Type>();
const bool isArray = (type.begin() != type.end());
- if (*result == *fContext.fTypes.fVoid) {
- if (!allowVoid) {
- this->errorReporter().error(type.fOffset,
- "type '" + name + "' not allowed in this context");
- return nullptr;
- }
- if (isArray) {
- this->errorReporter().error(type.fOffset,
- "type '" + name + "' may not be used in an array");
- return nullptr;
- }
+ if (*result == *fContext.fTypes.fVoid && !allowVoid) {
+ this->errorReporter().error(type.fOffset,
+ "type '" + name + "' not allowed in this context");
+ return nullptr;
}
if (!fIsBuiltinCode && this->typeContainsPrivateFields(*result)) {
this->errorReporter().error(type.fOffset, "type '" + name + "' is private");
return nullptr;
}
- if (isArray && result->isOpaque()) {
- this->errorReporter().error(type.fOffset,
- "opaque type '" + name + "' may not be used in an array");
- return nullptr;
- }
if (isArray) {
auto iter = type.begin();
- int arraySize = this->convertArraySize(type.fOffset, *iter);
+ int arraySize = this->convertArraySize(*result, type.fOffset, *iter);
if (!arraySize) {
return nullptr;
}
@@ -2802,11 +2797,11 @@
this->errorReporter().error(index.fOffset, "array must have a size");
return nullptr;
}
- int arraySize = this->convertArraySize(index.fOffset, *iter);
+ const Type* type = &base->as<TypeReference>().value();
+ int arraySize = this->convertArraySize(*type, index.fOffset, *iter);
if (!arraySize) {
return nullptr;
}
- const Type* type = &base->as<TypeReference>().value();
type = fSymbolTable->addArrayDimension(type, arraySize);
return std::make_unique<TypeReference>(fContext, base->fOffset, type);
}
diff --git a/src/sksl/SkSLIRGenerator.h b/src/sksl/SkSLIRGenerator.h
index 80633fd..39561c2 100644
--- a/src/sksl/SkSLIRGenerator.h
+++ b/src/sksl/SkSLIRGenerator.h
@@ -182,8 +182,8 @@
const ExpressionArray& arguments);
std::unique_ptr<Expression> coerce(std::unique_ptr<Expression> expr, const Type& type);
CoercionCost coercionCost(const Expression& expr, const Type& type);
- int convertArraySize(int offset, const ASTNode& s);
- int convertArraySize(std::unique_ptr<Expression> s);
+ int convertArraySize(const Type& type, int offset, const ASTNode& s);
+ int convertArraySize(const Type& type, std::unique_ptr<Expression> s);
bool containsConstantZero(Expression& expr);
bool dividesByZero(Token::Kind op, Expression& right);
std::unique_ptr<Expression> convertBinaryExpression(std::unique_ptr<Expression> left,
diff --git a/src/sksl/SkSLParser.cpp b/src/sksl/SkSLParser.cpp
index d6977bb..3211e15 100644
--- a/src/sksl/SkSLParser.cpp
+++ b/src/sksl/SkSLParser.cpp
@@ -313,6 +313,12 @@
return s && s->is<Type>();
}
+bool Parser::isArrayType(ASTNode::ID type) {
+ const ASTNode& node = this->getNode(type);
+ SkASSERT(node.fKind == ASTNode::Kind::kType);
+ return node.begin() != node.end();
+}
+
/* DIRECTIVE(#version) INT_LITERAL ("es" | "compatibility")? |
DIRECTIVE(#extension) IDENTIFIER COLON IDENTIFIER */
ASTNode::ID Parser::directive() {
@@ -667,9 +673,9 @@
this->addChild(result, this->createNode(offset, ASTNode::Kind::kModifiers, mods));
getNode(result).addChild(type);
- auto parseArrayDimensions = [this](ASTNode::ID currentVar, ASTNode::VarData* vd) -> bool {
+ auto parseArrayDimensions = [&](ASTNode::ID currentVar, ASTNode::VarData* vd) -> bool {
while (this->checkNext(Token::Kind::TK_LBRACKET)) {
- if (vd->fIsArray) {
+ if (vd->fIsArray || this->isArrayType(type)) {
this->error(this->peek(), "multi-dimensional arrays are not supported");
return false;
}
@@ -753,7 +759,7 @@
ASTNode::ParameterData pd(modifiers, this->text(name), 0);
getNode(result).addChild(type);
while (this->checkNext(Token::Kind::TK_LBRACKET)) {
- if (pd.fIsArray) {
+ if (pd.fIsArray || this->isArrayType(type)) {
this->error(this->peek(), "multi-dimensional arrays are not supported");
return ASTNode::ID::Invalid();
}
diff --git a/src/sksl/SkSLParser.h b/src/sksl/SkSLParser.h
index fea436e..bb663ed 100644
--- a/src/sksl/SkSLParser.h
+++ b/src/sksl/SkSLParser.h
@@ -152,6 +152,11 @@
*/
bool isType(StringFragment name);
+ /**
+ * Returns true if the passed-in ASTNode is an array type, or false if it is a non-arrayed type.
+ */
+ bool isArrayType(ASTNode::ID type);
+
// The pointer to the node may be invalidated by modifying the fNodes vector
ASTNode& getNode(ASTNode::ID id) {
SkASSERT(id.fValue >= 0 && id.fValue < (int) fFile->fNodes.size());
diff --git a/tests/sksl/errors/ArrayOfVoid.glsl b/tests/sksl/errors/ArrayOfVoid.glsl
new file mode 100644
index 0000000..efe8735
--- /dev/null
+++ b/tests/sksl/errors/ArrayOfVoid.glsl
@@ -0,0 +1,10 @@
+### Compilation failed:
+
+error: 3: type 'void' not allowed in this context
+error: 4: type 'void' not allowed in this context
+error: 6: type 'void' not allowed in this context
+error: 7: type 'void' not allowed in this context
+error: 8: type 'void' may not be used in an array
+error: 9: type 'void' not allowed in this context
+error: 10: type 'void' not allowed in this context
+7 errors
diff --git a/tests/sksl/errors/ArrayOfVoidInStruct.glsl b/tests/sksl/errors/ArrayOfVoidInStruct.glsl
new file mode 100644
index 0000000..acde5ff
--- /dev/null
+++ b/tests/sksl/errors/ArrayOfVoidInStruct.glsl
@@ -0,0 +1,6 @@
+### Compilation failed:
+
+error: 4: opaque type 'void' is not permitted in a struct
+error: 5: opaque type 'void' is not permitted in a struct
+error: 6: multi-dimensional arrays are not supported
+3 errors
diff --git a/tests/sksl/errors/ArraySplitDimensions.glsl b/tests/sksl/errors/ArraySplitDimensions.glsl
new file mode 100644
index 0000000..7ec388d
--- /dev/null
+++ b/tests/sksl/errors/ArraySplitDimensions.glsl
@@ -0,0 +1,4 @@
+### Compilation failed:
+
+error: 1: multi-dimensional arrays are not supported
+1 error
diff --git a/tests/sksl/errors/ArraySplitDimensionsInFuncBody.glsl b/tests/sksl/errors/ArraySplitDimensionsInFuncBody.glsl
new file mode 100644
index 0000000..7ec388d
--- /dev/null
+++ b/tests/sksl/errors/ArraySplitDimensionsInFuncBody.glsl
@@ -0,0 +1,4 @@
+### Compilation failed:
+
+error: 1: multi-dimensional arrays are not supported
+1 error
diff --git a/tests/sksl/errors/ArraySplitDimensionsInFuncDecl.glsl b/tests/sksl/errors/ArraySplitDimensionsInFuncDecl.glsl
new file mode 100644
index 0000000..7ec388d
--- /dev/null
+++ b/tests/sksl/errors/ArraySplitDimensionsInFuncDecl.glsl
@@ -0,0 +1,4 @@
+### Compilation failed:
+
+error: 1: multi-dimensional arrays are not supported
+1 error
diff --git a/tests/sksl/errors/ArraySplitDimensionsInStruct.glsl b/tests/sksl/errors/ArraySplitDimensionsInStruct.glsl
new file mode 100644
index 0000000..7ec388d
--- /dev/null
+++ b/tests/sksl/errors/ArraySplitDimensionsInStruct.glsl
@@ -0,0 +1,4 @@
+### Compilation failed:
+
+error: 1: multi-dimensional arrays are not supported
+1 error
diff --git a/tests/sksl/errors/ArrayTooManyDimensionsInFuncBody.glsl b/tests/sksl/errors/ArrayTooManyDimensionsInFuncBody.glsl
new file mode 100644
index 0000000..7ec388d
--- /dev/null
+++ b/tests/sksl/errors/ArrayTooManyDimensionsInFuncBody.glsl
@@ -0,0 +1,4 @@
+### Compilation failed:
+
+error: 1: multi-dimensional arrays are not supported
+1 error
diff --git a/tests/sksl/errors/ArrayTooManyDimensionsInFuncDecl.glsl b/tests/sksl/errors/ArrayTooManyDimensionsInFuncDecl.glsl
new file mode 100644
index 0000000..7ec388d
--- /dev/null
+++ b/tests/sksl/errors/ArrayTooManyDimensionsInFuncDecl.glsl
@@ -0,0 +1,4 @@
+### Compilation failed:
+
+error: 1: multi-dimensional arrays are not supported
+1 error
diff --git a/tests/sksl/errors/ArrayTooManyDimensionsInStruct.glsl b/tests/sksl/errors/ArrayTooManyDimensionsInStruct.glsl
new file mode 100644
index 0000000..7ec388d
--- /dev/null
+++ b/tests/sksl/errors/ArrayTooManyDimensionsInStruct.glsl
@@ -0,0 +1,4 @@
+### Compilation failed:
+
+error: 1: multi-dimensional arrays are not supported
+1 error
diff --git a/tests/sksl/errors/ArrayTypeTooManyDimensions.glsl b/tests/sksl/errors/ArrayTypeTooManyDimensions.glsl
new file mode 100644
index 0000000..7ec388d
--- /dev/null
+++ b/tests/sksl/errors/ArrayTypeTooManyDimensions.glsl
@@ -0,0 +1,4 @@
+### Compilation failed:
+
+error: 1: multi-dimensional arrays are not supported
+1 error
diff --git a/tests/sksl/errors/ArrayTypeTooManyDimensionsInFuncBody.glsl b/tests/sksl/errors/ArrayTypeTooManyDimensionsInFuncBody.glsl
new file mode 100644
index 0000000..69e8f45
--- /dev/null
+++ b/tests/sksl/errors/ArrayTypeTooManyDimensionsInFuncBody.glsl
@@ -0,0 +1,4 @@
+### Compilation failed:
+
+error: 1: expected ';', but found 'x'
+1 error
diff --git a/tests/sksl/errors/ArrayTypeTooManyDimensionsInFuncDecl.glsl b/tests/sksl/errors/ArrayTypeTooManyDimensionsInFuncDecl.glsl
new file mode 100644
index 0000000..7ec388d
--- /dev/null
+++ b/tests/sksl/errors/ArrayTypeTooManyDimensionsInFuncDecl.glsl
@@ -0,0 +1,4 @@
+### Compilation failed:
+
+error: 1: multi-dimensional arrays are not supported
+1 error
diff --git a/tests/sksl/errors/ArrayTypeTooManyDimensionsInStruct.glsl b/tests/sksl/errors/ArrayTypeTooManyDimensionsInStruct.glsl
new file mode 100644
index 0000000..7ec388d
--- /dev/null
+++ b/tests/sksl/errors/ArrayTypeTooManyDimensionsInStruct.glsl
@@ -0,0 +1,4 @@
+### Compilation failed:
+
+error: 1: multi-dimensional arrays are not supported
+1 error
