SkSL: Disallow fragmentProcessor variables in several places
We only want SkSL to contain fp variables at global scope, and to sample
directly from references to those global variables. Anything else will
confuse our sample-usage analysis, and often leads to asserts in the CPP
or pipeline-stage generators.
Bug: skia:10514
Change-Id: Ie1ef10821c1b2a946a92d050fea45d95569bc934
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/304599
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
diff --git a/src/sksl/SkSLIRGenerator.cpp b/src/sksl/SkSLIRGenerator.cpp
index 0c2fc60..2e91875 100644
--- a/src/sksl/SkSLIRGenerator.cpp
+++ b/src/sksl/SkSLIRGenerator.cpp
@@ -284,6 +284,11 @@
if (!baseType) {
return nullptr;
}
+ if (baseType->nonnullable() == *fContext.fFragmentProcessor_Type &&
+ storage != Variable::kGlobal_Storage) {
+ fErrors.error(decls.fOffset,
+ "variables of type '" + baseType->displayName() + "' must be global");
+ }
if (fKind != Program::kFragmentProcessor_Kind) {
if ((modifiers.fFlags & Modifiers::kIn_Flag) &&
baseType->kind() == Type::Kind::kMatrix_Kind) {
@@ -829,6 +834,11 @@
if (!returnType) {
return;
}
+ if (returnType->nonnullable() == *fContext.fFragmentProcessor_Type) {
+ fErrors.error(f.fOffset,
+ "functions may not return type '" + returnType->displayName() + "'");
+ return;
+ }
const ASTNode::FunctionData& fd = f.getFunctionData();
std::vector<const Variable*> parameters;
for (size_t i = 0; i < fd.fParameterCount; ++i) {
@@ -840,6 +850,12 @@
if (!type) {
return;
}
+ // Only the (builtin) declarations of 'sample' are allowed to have FP parameters
+ if (type->nonnullable() == *fContext.fFragmentProcessor_Type && !fIsBuiltinCode) {
+ fErrors.error(param.fOffset,
+ "parameters of type '" + type->displayName() + "' not allowed");
+ return;
+ }
for (int j = (int) pd.fSizeCount; j >= 1; j--) {
int size = (param.begin() + j)->getInt();
String name = type->name() + "[" + to_string(size) + "]";
@@ -1878,6 +1894,11 @@
ifFalse->fType.displayName() + "'");
return nullptr;
}
+ if (trueType->nonnullable() == *fContext.fFragmentProcessor_Type) {
+ fErrors.error(node.fOffset,
+ "ternary expression of type '" + trueType->displayName() + "' not allowed");
+ return nullptr;
+ }
ifTrue = this->coerce(std::move(ifTrue), *trueType);
if (!ifTrue) {
return nullptr;
@@ -2616,11 +2637,12 @@
const Type& type,
std::vector<std::unique_ptr<Expression>> args) {
// FIXME: add support for structs
- Type::Kind kind = type.kind();
- if (args.size() == 1 && args[0]->fType == type) {
+ if (args.size() == 1 && args[0]->fType == type &&
+ type.nonnullable() != *fContext.fFragmentProcessor_Type) {
// argument is already the right type, just return it
return std::move(args[0]);
}
+ Type::Kind kind = type.kind();
if (type.isNumber()) {
return this->convertNumberConstructor(offset, type, std::move(args));
} else if (kind == Type::kArray_Kind) {
diff --git a/tests/SkRuntimeEffectTest.cpp b/tests/SkRuntimeEffectTest.cpp
index 5271a20..84e0f9a 100644
--- a/tests/SkRuntimeEffectTest.cpp
+++ b/tests/SkRuntimeEffectTest.cpp
@@ -61,6 +61,23 @@
// Shouldn't be possible to create an SkRuntimeEffect without "main"
test("//", "", "main");
+
+ // Various places that shaders (fragmentProcessors) should not be allowed
+ test("",
+ "shader child;",
+ "must be global");
+ test("in shader child; half4 helper(shader fp) { return sample(fp); }",
+ "color = helper(child);",
+ "parameter");
+ test("in shader child; shader get_child() { return child; }",
+ "color = sample(get_child());",
+ "return");
+ test("in shader child;",
+ "color = sample(shader(child));",
+ "construct");
+ test("in shader child1; in shader child2;",
+ "color = sample(p.x > 10 ? child1 : child2);",
+ "expression");
}
class TestEffect {
diff --git a/tests/SkSLFPTest.cpp b/tests/SkSLFPTest.cpp
index c384c84..8fde219 100644
--- a/tests/SkSLFPTest.cpp
+++ b/tests/SkSLFPTest.cpp
@@ -818,6 +818,70 @@
"custom @setData function\n1 error\n");
}
+DEF_TEST(SkSLFPNoFPLocals, r) {
+ test_failure(r,
+ R"__SkSL__(
+ void main() {
+ fragmentProcessor child;
+ }
+ )__SkSL__",
+ "error: 1: variables of type 'fragmentProcessor' must be global\n"
+ "1 error\n");
+}
+
+DEF_TEST(SkSLFPNoFPParams, r) {
+ test_failure(r,
+ R"__SkSL__(
+ in fragmentProcessor child;
+ half4 helper(fragmentProcessor fp) { return sample(fp); }
+ void main() {
+ sk_OutColor = helper(child);
+ }
+ )__SkSL__",
+ "error: 3: parameters of type 'fragmentProcessor' not allowed\n"
+ "error: 5: unknown identifier 'helper'\n"
+ "2 errors\n");
+}
+
+DEF_TEST(SkSLFPNoFPReturns, r) {
+ test_failure(r,
+ R"__SkSL__(
+ in fragmentProcessor child;
+ fragmentProcessor get_child() { return child; }
+ void main() {
+ sk_OutColor = sample(get_child());
+ }
+ )__SkSL__",
+ "error: 3: functions may not return type 'fragmentProcessor'\n"
+ "error: 5: unknown identifier 'get_child'\n"
+ "2 errors\n");
+}
+
+DEF_TEST(SkSLFPNoFPConstructors, r) {
+ test_failure(r,
+ R"__SkSL__(
+ in fragmentProcessor child;
+ void main() {
+ sk_OutColor = sample(fragmentProcessor(child));
+ }
+ )__SkSL__",
+ "error: 4: cannot construct 'fragmentProcessor'\n"
+ "1 error\n");
+}
+
+DEF_TEST(SkSLFPNoFPExpressions, r) {
+ test_failure(r,
+ R"__SkSL__(
+ in fragmentProcessor child1;
+ in fragmentProcessor child2;
+ void main(float2 coord) {
+ sk_OutColor = sample(coord.x > 10 ? child1 : child2);
+ }
+ )__SkSL__",
+ "error: 5: ternary expression of type 'fragmentProcessor' not allowed\n"
+ "1 error\n");
+}
+
DEF_TEST(SkSLFPSampleCoords, r) {
test(r,
*SkSL::ShaderCapsFactory::Default(),
