Fix fuzzer-discovered error with inlining.
In http://review.skia.org/375776, an optimization was added to the
Inliner, causing it to skip generation of unnecessary temporary
variables. The fuzzer immediately discovered a flaw in this logic: the
"unnecessary" variable was actually used in the rare case that a
function failed to actually return a value. The inliner didn't detect
this case. Of course, this isn't a valid program either, so now we
report the error and cleanly fail.
Change-Id: I1f201cfd33f45cace3be93765a4e214e43a46e69
Bug: oss-fuzz:31469, oss-fuzz:31525
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/377101
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
diff --git a/gn/sksl_tests.gni b/gn/sksl_tests.gni
index fefbb09..5f9c3ab 100644
--- a/gn/sksl_tests.gni
+++ b/gn/sksl_tests.gni
@@ -144,6 +144,7 @@
"/sksl/errors/Ossfuzz29845.sksl",
"/sksl/errors/Ossfuzz29849.sksl",
"/sksl/errors/Ossfuzz31410.sksl",
+ "/sksl/errors/Ossfuzz31469.sksl",
"/sksl/errors/OverflowFloatLiteral.sksl",
"/sksl/errors/OverflowIntLiteral.sksl",
"/sksl/errors/OverflowParamArraySize.sksl",
diff --git a/resources/sksl/errors/Ossfuzz31469.sksl b/resources/sksl/errors/Ossfuzz31469.sksl
new file mode 100644
index 0000000..bd23a0c
--- /dev/null
+++ b/resources/sksl/errors/Ossfuzz31469.sksl
@@ -0,0 +1,3 @@
+half n() {}
+void S() { -n(); }
+void l() { S(); }
diff --git a/src/sksl/SkSLInliner.cpp b/src/sksl/SkSLInliner.cpp
index e6e55bb..86db614 100644
--- a/src/sksl/SkSLInliner.cpp
+++ b/src/sksl/SkSLInliner.cpp
@@ -760,15 +760,22 @@
&arguments[i]->type())));
}
- if (resultExpr != nullptr) {
- // Return our result variable as our replacement expression.
+ if (resultExpr) {
+ // Return our result expression as-is.
inlinedCall.fReplacementExpr = std::move(resultExpr);
- } else {
+ } else if (function.declaration().returnType() == *fContext->fTypes.fVoid) {
// It's a void function, so it doesn't actually result in anything, but we have to return
// something non-null as a standin.
inlinedCall.fReplacementExpr = std::make_unique<BoolLiteral>(*fContext,
offset,
/*value=*/false);
+ } else {
+ // It's a non-void function, but it never created a result expression--that is, it never
+ // returned anything! Discard our output and generate an error.
+ fContext->fErrors.error(function.fOffset, String("function '") +
+ function.declaration().name() +
+ "' exits without returning a value");
+ inlinedCall = {};
}
return inlinedCall;
@@ -1198,25 +1205,29 @@
// Convert the function call to its inlined equivalent.
InlinedCall inlinedCall = this->inlineCall(&funcCall, candidate.fSymbols,
&candidate.fEnclosingFunction->declaration());
- if (inlinedCall.fInlinedBody) {
- // Ensure that the inlined body has a scope if it needs one.
- this->ensureScopedBlocks(inlinedCall.fInlinedBody.get(), candidate.fParentStmt->get());
- // Add references within the inlined body
- usage->add(inlinedCall.fInlinedBody.get());
-
- // Move the enclosing statement to the end of the unscoped Block containing the inlined
- // function, then replace the enclosing statement with that Block.
- // Before:
- // fInlinedBody = Block{ stmt1, stmt2, stmt3 }
- // fEnclosingStmt = stmt4
- // After:
- // fInlinedBody = null
- // fEnclosingStmt = Block{ stmt1, stmt2, stmt3, stmt4 }
- inlinedCall.fInlinedBody->children().push_back(std::move(*candidate.fEnclosingStmt));
- *candidate.fEnclosingStmt = std::move(inlinedCall.fInlinedBody);
+ // Stop if an error was detected during the inlining process.
+ if (!inlinedCall.fInlinedBody && !inlinedCall.fReplacementExpr) {
+ break;
}
+ // Ensure that the inlined body has a scope if it needs one.
+ this->ensureScopedBlocks(inlinedCall.fInlinedBody.get(), candidate.fParentStmt->get());
+
+ // Add references within the inlined body
+ usage->add(inlinedCall.fInlinedBody.get());
+
+ // Move the enclosing statement to the end of the unscoped Block containing the inlined
+ // function, then replace the enclosing statement with that Block.
+ // Before:
+ // fInlinedBody = Block{ stmt1, stmt2, stmt3 }
+ // fEnclosingStmt = stmt4
+ // After:
+ // fInlinedBody = null
+ // fEnclosingStmt = Block{ stmt1, stmt2, stmt3, stmt4 }
+ inlinedCall.fInlinedBody->children().push_back(std::move(*candidate.fEnclosingStmt));
+ *candidate.fEnclosingStmt = std::move(inlinedCall.fInlinedBody);
+
// Replace the candidate function call with our replacement expression.
usage->replace(candidate.fCandidateExpr->get(), inlinedCall.fReplacementExpr.get());
*candidate.fCandidateExpr = std::move(inlinedCall.fReplacementExpr);
diff --git a/tests/sksl/errors/Ossfuzz31469.glsl b/tests/sksl/errors/Ossfuzz31469.glsl
new file mode 100644
index 0000000..62d9a44
--- /dev/null
+++ b/tests/sksl/errors/Ossfuzz31469.glsl
@@ -0,0 +1,5 @@
+### Compilation failed:
+
+error: 1: function 'n' can exit without returning a value
+error: 1: function 'n' exits without returning a value
+2 errors
