Fix assertion when calling a built-in function at global scope.
The fuzzer invented a much more elaborate example, but I was able to
winnow it down to a simple otherwise-normal test case. This also fixes
a latent DSL bug; DSL functions were not updating the list of referenced
intrinsics, so the compiler might emit finished programs that called
built-in functions that didn't exist in the code.
Change-Id: I095bb566b9db9f87cbe9460732c300b7973eb112
Bug: oss-fuzz:37659
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/442325
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
diff --git a/gn/sksl_tests.gni b/gn/sksl_tests.gni
index ab04a02..1309c0f 100644
--- a/gn/sksl_tests.gni
+++ b/gn/sksl_tests.gni
@@ -64,6 +64,7 @@
"/sksl/errors/InterfaceBlockScope.sksl",
"/sksl/errors/InterfaceBlockStorageModifiers.sksl",
"/sksl/errors/InterfaceBlockWithNoMembers.sksl",
+ "/sksl/errors/IntrinsicInGlobalVariable.sksl",
"/sksl/errors/InvalidAssignment.sksl",
"/sksl/errors/InvalidOutParams.sksl",
"/sksl/errors/InvalidToken.sksl",
diff --git a/resources/sksl/errors/IntrinsicInGlobalVariable.sksl b/resources/sksl/errors/IntrinsicInGlobalVariable.sksl
new file mode 100644
index 0000000..d1eb39f
--- /dev/null
+++ b/resources/sksl/errors/IntrinsicInGlobalVariable.sksl
@@ -0,0 +1,2 @@
+float c = blend_src_over(half4(1), half4(0));
+void f() {}
diff --git a/src/sksl/SkSLIRGenerator.cpp b/src/sksl/SkSLIRGenerator.cpp
index cd5e239..5206457 100644
--- a/src/sksl/SkSLIRGenerator.cpp
+++ b/src/sksl/SkSLIRGenerator.cpp
@@ -640,8 +640,11 @@
std::vector<const Variable*>(),
fContext.fTypes.fVoid.get(),
fIsBuiltinCode));
- auto invokeDef = std::make_unique<FunctionDefinition>(/*offset=*/-1, invokeDecl, fIsBuiltinCode,
- std::move(main));
+ IntrinsicSet referencedIntrinsics;
+ main = this->finalizeFunction(*invokeDecl, std::move(main), &referencedIntrinsics);
+ auto invokeDef = std::make_unique<FunctionDefinition>(/*offset=*/-1, invokeDecl,
+ fIsBuiltinCode, std::move(main),
+ std::move(referencedIntrinsics));
invokeDecl->setDefinition(invokeDef.get());
fProgramElements->push_back(std::move(invokeDef));
@@ -759,12 +762,15 @@
}
std::unique_ptr<Block> IRGenerator::finalizeFunction(const FunctionDeclaration& funcDecl,
- std::unique_ptr<Block> body) {
+ std::unique_ptr<Block> body,
+ IntrinsicSet* referencedIntrinsics) {
class Finalizer : public ProgramWriter {
public:
- Finalizer(IRGenerator* irGenerator, const FunctionDeclaration* function)
+ Finalizer(IRGenerator* irGenerator, const FunctionDeclaration* function,
+ IntrinsicSet* referencedIntrinsics)
: fIRGenerator(irGenerator)
- , fFunction(function) {}
+ , fFunction(function)
+ , fReferencedIntrinsics(referencedIntrinsics) {}
~Finalizer() override {
SkASSERT(!fBreakableLevel);
@@ -776,8 +782,13 @@
}
bool visitExpression(Expression& expr) override {
- // Do not recurse into expressions.
- return false;
+ if (expr.is<FunctionCall>()) {
+ const FunctionDeclaration& func = expr.as<FunctionCall>().function();
+ if (func.isBuiltin() && func.definition()) {
+ fReferencedIntrinsics->insert(&func);
+ }
+ }
+ return INHERITED::visitExpression(expr);
}
bool visitStatement(Statement& stmt) override {
@@ -850,6 +861,8 @@
private:
IRGenerator* fIRGenerator;
const FunctionDeclaration* fFunction;
+ // which intrinsics have we encountered in this function
+ IntrinsicSet* fReferencedIntrinsics;
// how deeply nested we are in breakable constructs (for, do, switch).
int fBreakableLevel = 0;
// how deeply nested we are in continuable constructs (for, do).
@@ -868,7 +881,7 @@
body->children().push_back(this->getNormalizeSkPositionCode());
}
- Finalizer finalizer{this, &funcDecl};
+ Finalizer finalizer{this, &funcDecl, referencedIntrinsics};
finalizer.visitStatement(*body);
if (Analysis::CanExitWithoutReturningValue(funcDecl, *body)) {
@@ -879,9 +892,6 @@
}
void IRGenerator::convertFunction(const ASTNode& f) {
- SkASSERT(fReferencedIntrinsics.empty());
- SK_AT_SCOPE_EXIT(fReferencedIntrinsics.clear());
-
auto iter = f.begin();
const Type* returnType = this->convertType(*(iter++), /*allowVoid=*/true);
if (returnType == nullptr) {
@@ -954,9 +964,10 @@
if (!body) {
return;
}
- body = this->finalizeFunction(*decl, std::move(body));
+ IntrinsicSet referencedIntrinsics;
+ body = this->finalizeFunction(*decl, std::move(body), &referencedIntrinsics);
auto result = std::make_unique<FunctionDefinition>(
- f.fOffset, decl, fIsBuiltinCode, std::move(body), std::move(fReferencedIntrinsics));
+ f.fOffset, decl, fIsBuiltinCode, std::move(body), std::move(referencedIntrinsics));
decl->setDefinition(result.get());
result->setSource(&f);
fProgramElements->push_back(std::move(result));
@@ -1287,9 +1298,6 @@
if (function.intrinsicKind() == k_dFdy_IntrinsicKind) {
fInputs.fUseFlipRTUniform = true;
}
- if (function.definition()) {
- fReferencedIntrinsics.insert(&function);
- }
if (!fIsBuiltinCode && fIntrinsics) {
this->copyIntrinsicIfNeeded(function);
}
diff --git a/src/sksl/SkSLIRGenerator.h b/src/sksl/SkSLIRGenerator.h
index 691feec..e23078f 100644
--- a/src/sksl/SkSLIRGenerator.h
+++ b/src/sksl/SkSLIRGenerator.h
@@ -230,10 +230,12 @@
void copyIntrinsicIfNeeded(const FunctionDeclaration& function);
void findAndDeclareBuiltinVariables();
bool detectVarDeclarationWithoutScope(const Statement& stmt);
- // Coerces returns to correct type, detects invalid break / continue placement, and otherwise
+ // Coerces returns to correct type, detects invalid break / continue placement, identifies any
+ // built-in functions that will need to be added to the shared elements list, and otherwise
// massages the function into its final form
std::unique_ptr<Block> finalizeFunction(const FunctionDeclaration& funcDecl,
- std::unique_ptr<Block> body);
+ std::unique_ptr<Block> body,
+ IntrinsicSet* referencedIntrinsics);
// Runtime effects (and the interpreter, which uses the same CPU runtime) require adherence to
// the strict rules from The OpenGL ES Shading Language Version 1.00. (Including Appendix A).
@@ -260,7 +262,6 @@
std::shared_ptr<SymbolTable> fSymbolTable = nullptr;
// Symbols which have definitions in the include files.
IRIntrinsicMap* fIntrinsics = nullptr;
- std::unordered_set<const FunctionDeclaration*> fReferencedIntrinsics;
int fInvocations;
std::unordered_set<const Type*> fDefinedStructs;
std::vector<std::unique_ptr<ProgramElement>>* fProgramElements = nullptr;
diff --git a/src/sksl/SkSLRehydrator.cpp b/src/sksl/SkSLRehydrator.cpp
index 331e199..6ed4ad5 100644
--- a/src/sksl/SkSLRehydrator.cpp
+++ b/src/sksl/SkSLRehydrator.cpp
@@ -279,7 +279,7 @@
const FunctionDeclaration* decl = this->symbolRef<FunctionDeclaration>(
Symbol::Kind::kFunctionDeclaration);
std::unique_ptr<Statement> body = this->statement();
- std::unordered_set<const FunctionDeclaration*> refs;
+ IntrinsicSet refs;
uint8_t refCount = this->readU8();
for (int i = 0; i < refCount; ++i) {
refs.insert(this->symbolRef<FunctionDeclaration>(
diff --git a/src/sksl/codegen/SkSLSPIRVCodeGenerator.cpp b/src/sksl/codegen/SkSLSPIRVCodeGenerator.cpp
index b30d250..a4baece 100644
--- a/src/sksl/codegen/SkSLSPIRVCodeGenerator.cpp
+++ b/src/sksl/codegen/SkSLSPIRVCodeGenerator.cpp
@@ -3542,8 +3542,8 @@
// Define it.
adapter.entrypointDef =
std::make_unique<FunctionDefinition>(/*offset=*/-1, adapter.entrypointDecl.get(),
- /*builtin=*/false,
- /*body=*/std::move(entrypointBlock));
+ /*builtin=*/false, std::move(entrypointBlock),
+ IntrinsicSet{});
adapter.entrypointDecl->setDefinition(adapter.entrypointDef.get());
return adapter;
diff --git a/src/sksl/dsl/DSLFunction.cpp b/src/sksl/dsl/DSLFunction.cpp
index 21305c1..18a8892 100644
--- a/src/sksl/dsl/DSLFunction.cpp
+++ b/src/sksl/dsl/DSLFunction.cpp
@@ -89,9 +89,12 @@
}
SkASSERTF(!fDecl->definition(), "function '%s' already defined", fDecl->description().c_str());
std::unique_ptr<Block> body = block.release();
- body = DSLWriter::IRGenerator().finalizeFunction(*fDecl, std::move(body));
+ IntrinsicSet referencedIntrinsics;
+ body = DSLWriter::IRGenerator().finalizeFunction(*fDecl, std::move(body),
+ &referencedIntrinsics);
auto function = std::make_unique<SkSL::FunctionDefinition>(/*offset=*/-1, fDecl,
- /*builtin=*/false, std::move(body));
+ /*builtin=*/false, std::move(body),
+ std::move(referencedIntrinsics));
DSLWriter::ReportErrors();
fDecl->fDefinition = function.get();
DSLWriter::ProgramElements().push_back(std::move(function));
diff --git a/src/sksl/ir/SkSLFunctionDefinition.h b/src/sksl/ir/SkSLFunctionDefinition.h
index 9244c16..c9c554d 100644
--- a/src/sksl/ir/SkSLFunctionDefinition.h
+++ b/src/sksl/ir/SkSLFunctionDefinition.h
@@ -16,6 +16,8 @@
struct ASTNode;
+using IntrinsicSet = std::unordered_set<const FunctionDeclaration*>;
+
/**
* A function definition (a declaration plus an associated block of code).
*/
@@ -23,10 +25,8 @@
public:
static constexpr Kind kProgramElementKind = Kind::kFunction;
- FunctionDefinition(int offset,
- const FunctionDeclaration* declaration, bool builtin,
- std::unique_ptr<Statement> body,
- std::unordered_set<const FunctionDeclaration*> referencedIntrinsics = {})
+ FunctionDefinition(int offset, const FunctionDeclaration* declaration, bool builtin,
+ std::unique_ptr<Statement> body, IntrinsicSet referencedIntrinsics)
: INHERITED(offset, kProgramElementKind)
, fDeclaration(declaration)
, fBuiltin(builtin)
diff --git a/tests/sksl/errors/IntrinsicInGlobalVariable.glsl b/tests/sksl/errors/IntrinsicInGlobalVariable.glsl
new file mode 100644
index 0000000..f85146c
--- /dev/null
+++ b/tests/sksl/errors/IntrinsicInGlobalVariable.glsl
@@ -0,0 +1,4 @@
+### Compilation failed:
+
+error: 1: global variable initializer must be a constant expression
+1 error