commit 842ee5bd8d6f3aece0cfba4bc623263b9da756c1
author John Stiles <johnstiles@google.com> Fri Aug 27 12:37:27 2021 -0400
committer SkCQ <skcq-be@skia-corp.google.com.iam.gserviceaccount.com> Fri Aug 27 17:05:21 2021 +0000
tree 42cb7b04cf323b047a54366231e131ae928bfb62
parent c82eab82f15ae946784f199e9381551088e4b24d

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>

src/sksl/SkSLIRGenerator.cpp

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);
         }