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>
This commit is contained in:
Brian Osman 2020-07-21 09:39:27 -04:00 committed by Skia Commit-Bot
parent 8e77631a80
commit 82329004d0
3 changed files with 105 additions and 2 deletions

View File

@ -284,6 +284,11 @@ std::unique_ptr<VarDeclarations> IRGenerator::convertVarDeclarations(const ASTNo
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 @@ void IRGenerator::convertFunction(const ASTNode& f) {
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 @@ void IRGenerator::convertFunction(const ASTNode& f) {
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 @@ std::unique_ptr<Expression> IRGenerator::convertTernaryExpression(const ASTNode&
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 @@ std::unique_ptr<Expression> IRGenerator::convertConstructor(
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) {

View File

@ -61,6 +61,23 @@ DEF_TEST(SkRuntimeEffectInvalid, r) {
// 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 {

View File

@ -818,6 +818,70 @@ DEF_TEST(SkSLFPBadIn, r) {
"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(),