Enforce ES2 limits on indexing expressions (in runtime effects)
This enforces an even stricter version of the rules from GLSL ES 1.0
Appendix A, Section 5. Essentially, indices (to arrays, vectors,
matrices) must be made of literals, loop indices, and expressions made
of those two.
Bug: skia:10837
Bug: skia:11096
Change-Id: I437a5ed64da58e24d5991ddbde68859f5214e98b
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/354665
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
diff --git a/gn/sksl_tests.gni b/gn/sksl_tests.gni
index f9aa2d7..1efa2b6 100644
--- a/gn/sksl_tests.gni
+++ b/gn/sksl_tests.gni
@@ -498,6 +498,7 @@
]
sksl_rte_error_tests = [
+ "$_tests/sksl/runtime_errors/IllegalIndexing.rte",
"$_tests/sksl/runtime_errors/IllegalOperators.rte",
"$_tests/sksl/runtime_errors/IllegalStatements.rte",
"$_tests/sksl/runtime_errors/LoopConditionErrors.rte",
diff --git a/resources/particles/confetti.json b/resources/particles/confetti.json
index 93641ff..c2d8a53 100644
--- a/resources/particles/confetti.json
+++ b/resources/particles/confetti.json
@@ -17,13 +17,11 @@
],
"Code": [
"void spawn(inout Particle p) {",
- " float3 colors[4];",
- " colors[0] = float3(0.87, 0.24, 0.11);",
- " colors[1] = float3(1, 0.9, 0.2);",
- " colors[2] = float3(0.44, 0.73, 0.24);",
- " colors[3] = float3(0.38, 0.54, 0.95);",
" int idx = int(rand(p.seed) * 4);",
- " p.color.rgb = colors[idx];",
+ " p.color.rgb = (idx == 0) ? float3(0.87, 0.24, 0.11)",
+ " : (idx == 1) ? float3(1.00, 0.90, 0.20)",
+ " : (idx == 2) ? float3(0.44, 0.73, 0.24)",
+ " : float3(0.38, 0.54, 0.95);",
"",
" p.lifetime = (1 - effect.age) * effect.lifetime;",
" p.scale = mix(0.6, 1, rand(p.seed));",
diff --git a/site/user/modules/particles.md b/site/user/modules/particles.md
index 5c4d97f..faf63b2 100644
--- a/site/user/modules/particles.md
+++ b/site/user/modules/particles.md
@@ -44,7 +44,7 @@
<figure>
<canvas id=confetti width=400 height=400></canvas>
<figcaption>
- <a href="https://particles.skia.org/eb484bdbac5952c0184a7f1d25773746"
+ <a href="https://particles.skia.org/73bf9f720bb7ed03d94cdc18e366b1da"
target=_blank rel=noopener>Confetti</a>
</figcaption>
</figure>
@@ -145,13 +145,11 @@
],
"Code": [
"void spawn(inout Particle p) {",
- " float3 colors[4];",
- " colors[0] = float3(0.87, 0.24, 0.11);",
- " colors[1] = float3(1, 0.9, 0.2);",
- " colors[2] = float3(0.44, 0.73, 0.24);",
- " colors[3] = float3(0.38, 0.54, 0.95);",
" int idx = int(rand(p.seed) * 4);",
- " p.color.rgb = colors[idx];",
+ " p.color.rgb = (idx == 0) ? float3(0.87, 0.24, 0.11)",
+ " : (idx == 1) ? float3(1.00, 0.90, 0.20)",
+ " : (idx == 2) ? float3(0.44, 0.73, 0.24)",
+ " : float3(0.38, 0.54, 0.95);",
"",
" p.lifetime = (1 - effect.age) * effect.lifetime;",
" p.scale = mix(0.6, 1, rand(p.seed));",
diff --git a/src/sksl/SkSLAnalysis.cpp b/src/sksl/SkSLAnalysis.cpp
index f4a47e1..97f90b1 100644
--- a/src/sksl/SkSLAnalysis.cpp
+++ b/src/sksl/SkSLAnalysis.cpp
@@ -645,6 +645,108 @@
return true;
}
+class ES2IndexExpressionVisitor : public ProgramVisitor {
+public:
+ ES2IndexExpressionVisitor(const std::set<const Variable*>* loopIndices)
+ : fLoopIndices(loopIndices) {}
+
+ bool visitExpression(const Expression& e) override {
+ // A constant-(index)-expression is one of...
+ switch (e.kind()) {
+ // ... a literal value
+ case Expression::Kind::kBoolLiteral:
+ case Expression::Kind::kIntLiteral:
+ case Expression::Kind::kFloatLiteral:
+ return false;
+
+ // ... loop indices as defined in section 4. Today, SkSL allows no other variables,
+ // but GLSL allows 'const' variables, because it requires that const variables be
+ // initialized with constant-expressions. We could support that usage by checking
+ // for variables that are const, and whose initializing expression also pass our
+ // checks. For now, we'll be conservative. (skbug.com/10837)
+ case Expression::Kind::kVariableReference:
+ return fLoopIndices->find(e.as<VariableReference>().variable()) ==
+ fLoopIndices->end();
+
+ // ... expressions composed of both of the above
+ case Expression::Kind::kBinary:
+ case Expression::Kind::kConstructor:
+ case Expression::Kind::kFieldAccess:
+ case Expression::Kind::kIndex:
+ case Expression::Kind::kPrefix:
+ case Expression::Kind::kPostfix:
+ case Expression::Kind::kSwizzle:
+ case Expression::Kind::kTernary:
+ return INHERITED::visitExpression(e);
+
+ // These are completely disallowed in SkSL constant-index-expressions. GLSL allows
+ // calls to built-in functions where the arguments are all constant-expressions, but
+ // we don't guarantee that behavior. (skbug.com/10835)
+ case Expression::Kind::kExternalFunctionCall:
+ case Expression::Kind::kFunctionCall:
+ return true;
+
+ // These should never appear in final IR
+ case Expression::Kind::kDefined:
+ case Expression::Kind::kExternalFunctionReference:
+ case Expression::Kind::kFunctionReference:
+ case Expression::Kind::kSetting:
+ case Expression::Kind::kTypeReference:
+ default:
+ SkDEBUGFAIL("Unexpected expression type");
+ return true;
+ }
+ }
+
+private:
+ const std::set<const Variable*>* fLoopIndices;
+ using INHERITED = ProgramVisitor;
+};
+
+class ES2IndexingVisitor : public ProgramVisitor {
+public:
+ ES2IndexingVisitor(ErrorReporter& errors) : fErrors(errors) {}
+
+ bool visitStatement(const Statement& s) override {
+ if (s.is<ForStatement>()) {
+ const ForStatement& f = s.as<ForStatement>();
+ SkASSERT(f.initializer() && f.initializer()->is<VarDeclaration>());
+ const Variable* var = &f.initializer()->as<VarDeclaration>().var();
+ auto [iter, inserted] = fLoopIndices.insert(var);
+ SkASSERT(inserted);
+ bool result = this->visitStatement(*f.statement());
+ fLoopIndices.erase(iter);
+ return result;
+ }
+ return INHERITED::visitStatement(s);
+ }
+
+ bool visitExpression(const Expression& e) override {
+ if (e.is<IndexExpression>()) {
+ const IndexExpression& i = e.as<IndexExpression>();
+ ES2IndexExpressionVisitor indexerInvalid(&fLoopIndices);
+ if (indexerInvalid.visitExpression(*i.index())) {
+ fErrors.error(i.fOffset, "index expression must be constant");
+ return true;
+ }
+ }
+ return INHERITED::visitExpression(e);
+ }
+
+ using ProgramVisitor::visitProgramElement;
+
+private:
+ ErrorReporter& fErrors;
+ std::set<const Variable*> fLoopIndices;
+ using INHERITED = ProgramVisitor;
+};
+
+
+void Analysis::ValidateIndexingForES2(const ProgramElement& pe, ErrorReporter& errors) {
+ ES2IndexingVisitor visitor(errors);
+ visitor.visitProgramElement(pe);
+}
+
////////////////////////////////////////////////////////////////////////////////
// ProgramVisitor
diff --git a/src/sksl/SkSLAnalysis.h b/src/sksl/SkSLAnalysis.h
index 43883e1..bdce469 100644
--- a/src/sksl/SkSLAnalysis.h
+++ b/src/sksl/SkSLAnalysis.h
@@ -82,6 +82,8 @@
static bool ForLoopIsValidForES2(const ForStatement& loop,
UnrollableLoopInfo* outLoopInfo,
ErrorReporter* errors);
+
+ static void ValidateIndexingForES2(const ProgramElement& pe, ErrorReporter& errors);
};
/**
diff --git a/src/sksl/SkSLIRGenerator.cpp b/src/sksl/SkSLIRGenerator.cpp
index 2984a6e..8664506 100644
--- a/src/sksl/SkSLIRGenerator.cpp
+++ b/src/sksl/SkSLIRGenerator.cpp
@@ -2956,7 +2956,7 @@
this->findAndDeclareBuiltinVariables();
}
- // Do a final pass looking for dangling FunctionReference or TypeReference expressions
+ // Do a pass looking for dangling FunctionReference or TypeReference expressions
class FindIllegalExpressions : public ProgramVisitor {
public:
FindIllegalExpressions(IRGenerator* generator) : fGenerator(generator) {}
@@ -2974,6 +2974,15 @@
FindIllegalExpressions{this}.visitProgramElement(*pe);
}
+ // If we're in ES2 mode (runtime effects), do a pass to enforce Appendix A, Section 5 of the
+ // GLSL ES 1.00 spec -- Indexing. Don't bother if we've already found errors - this logic
+ // assumes that all loops meet the criteria of Section 4, and if they don't, could crash.
+ if (this->strictES2Mode() && this->errorReporter().errorCount() == 0) {
+ for (const auto& pe : *fProgramElements) {
+ Analysis::ValidateIndexingForES2(*pe, this->errorReporter());
+ }
+ }
+
fSettings = nullptr;
return IRBundle{std::move(elements), std::move(sharedElements), this->releaseModifiers(),
diff --git a/tests/SkSLInterpreterTest.cpp b/tests/SkSLInterpreterTest.cpp
index 543b621..233bc91 100644
--- a/tests/SkSLInterpreterTest.cpp
+++ b/tests/SkSLInterpreterTest.cpp
@@ -720,22 +720,6 @@
REPORTER_ASSERT(r, !program);
}
-static void expect_run_failure(skiatest::Reporter* r, const char* src, float* in) {
- GrShaderCaps caps(GrContextOptions{});
- SkSL::Compiler compiler(&caps);
- SkSL::Program::Settings settings;
- auto program = compiler.convertProgram(SkSL::Program::kGeneric_Kind,
- SkSL::String(src), settings);
- REPORTER_ASSERT(r, program);
-
- auto byteCode = compiler.toByteCode(*program);
- REPORTER_ASSERT(r, byteCode);
-
- auto fun = byteCode->getFunction("main");
- bool result = byteCode->run(fun, in, fun->getParameterCount(), nullptr, 0, nullptr, 0);
- REPORTER_ASSERT(r, !result);
-}
-
DEF_TEST(SkSLInterpreterRestrictLoops, r) {
// while and do-while loops are not allowed
expect_failure(r, "void main(inout float x) { while (x < 1) { x++; } }");
@@ -798,20 +782,6 @@
REPORTER_ASSERT(r, rets[1] == 2.0f);
}
-DEF_TEST(SkSLInterpreterArrayBounds, r) {
- // Out of bounds array access at compile time prevents a program from being generated at all
- // (tested in ArrayIndexOutOfRange.sksl).
-
- // Out of bounds array access at runtime is pinned, and we don't update any inout data.
- float in[3] = { -1.0f, 1.0f, 2.0f };
- expect_run_failure(r, "void main(inout float data[3]) { data[int(data[0])] = 0; }", in);
- REPORTER_ASSERT(r, in[0] == -1.0f && in[1] == 1.0f && in[2] == 2.0f);
-
- in[0] = 3.0f;
- expect_run_failure(r, "void main(inout float data[3]) { data[int(data[0])] = 0; }", in);
- REPORTER_ASSERT(r, in[0] == 3.0f && in[1] == 1.0f && in[2] == 2.0f);
-}
-
DEF_TEST(SkSLInterpreterFunctions, r) {
const char* src =
"float sqr(float x) { return x * x; }\n"
diff --git a/tests/sksl/runtime_errors/IllegalIndexing.rte b/tests/sksl/runtime_errors/IllegalIndexing.rte
new file mode 100644
index 0000000..0a1ee83
--- /dev/null
+++ b/tests/sksl/runtime_errors/IllegalIndexing.rte
@@ -0,0 +1,29 @@
+// Expect 4 errors
+
+// Legal cases that should not produce errors:
+void literal_index() { int a[2]; a[0] = 0; a[1] = a[0]; }
+void loop_index() { int a[2]; for (int i = 0; i < 2; ++i) { a[i] = i; } }
+void loop_expr_binary() { int a[2]; for (int i = 0; i < 2; ++i) { a[1 - i] = i; } }
+void loop_expr_unary() { int a[2]; for (int i = 0; i > -2; --i) { a[-i] = i; } }
+void loop_swizzle() { int a[2]; for (int i = 0; i < 2; ++i) { a[i.x] = i; } }
+void loop_ternary() { int a[2]; for (int i = 0; i < 2; ++i) { a[i > 0 ? i : 0] = i; } }
+
+void loop_type_coerce() { float a[2]; for (float i = 0; i < 2; ++i) { a[int(i)] = i; } }
+
+void loop_nested() {
+ int a[3];
+ for (int i = 0; i < 2; ++i)
+ for (int j = 0; j < 2; ++j) {
+ a[i + j] = j;
+ }
+}
+
+uniform int u;
+
+// Illegal cases that should produce errors:
+void uniform_index() { int a[2]; a[u] = 0; }
+void param_index(int p) { int a[2]; a[p] = 0; }
+
+// Legal in GLSL, not yet in SkSL:
+void func_index() { int a[2]; a[int(abs(-1))] = 0; } // skbug.com/10835
+void const_var_index() { int a[2]; const int b = 0; a[b] = 0; } // skbug.com/10837
diff --git a/tests/sksl/runtime_errors/golden/IllegalIndexing.skvm b/tests/sksl/runtime_errors/golden/IllegalIndexing.skvm
new file mode 100644
index 0000000..a04fd78
--- /dev/null
+++ b/tests/sksl/runtime_errors/golden/IllegalIndexing.skvm
@@ -0,0 +1,7 @@
+### Compilation failed:
+
+error: 24: index expression must be constant
+error: 25: index expression must be constant
+error: 28: index expression must be constant
+error: 29: index expression must be constant
+4 errors