Add return-value check to the function finalizer.
Rather than have the inliner own this responsibility, the function
finalizer now detects if a function is supposed to return a value but
never actually does. This will allow us to detect this error case even
if the inliner is disabled. The inliner should no longer encounter
functions that claim to return a value but don't, so it will now assert
if one is encountered. (The inliner still has the logic to handle this
case gracefully, just in case.)
The check is currently very simple and doesn't analyze the structure of
the function, so it won't report cases where some paths return a value
and others don't, e.g. this will pass the test:
int func() { if (something()) return 123; }
(This is good enough to resolve the inliner issue, though, as it only
occurred in functions with no value-returns at all.)
Change-Id: I21f13daffe66c8f2e72932b320ee268ba9207bfa
Bug: oss-fuzz:31469, oss-fuzz:31525, skia:11377
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/377196
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
diff --git a/src/sksl/SkSLIRGenerator.cpp b/src/sksl/SkSLIRGenerator.cpp
index c507d08..9709da8 100644
--- a/src/sksl/SkSLIRGenerator.cpp
+++ b/src/sksl/SkSLIRGenerator.cpp
@@ -928,6 +928,22 @@
~Finalizer() override {
SkASSERT(!fBreakableLevel);
SkASSERT(!fContinuableLevel);
+
+ if (!fEncounteredReturnValue && this->functionReturnsValue()) {
+ // It's a non-void function, but it never created a result expression--that is, it
+ // never returned anything.
+ fIRGenerator->errorReporter().error(
+ fFunction->fOffset,
+ "function '" + fFunction->name() + "' exits without returning a value");
+ }
+ }
+
+ bool functionReturnsValue() const {
+ return fFunction->returnType() != *fIRGenerator->fContext.fTypes.fVoid;
+ }
+
+ bool encounteredReturnValue() const {
+ return fEncounteredReturnValue;
}
bool visitStatement(Statement& stmt) override {
@@ -940,22 +956,28 @@
SkASSERT(fIRGenerator->programKind() != ProgramKind::kVertex ||
!fIRGenerator->fRTAdjust ||
fFunction->name() != "main");
- ReturnStatement& r = stmt.as<ReturnStatement>();
+
+ // Verify that the return statement matches the function's return type.
+ ReturnStatement& returnStmt = stmt.as<ReturnStatement>();
const Type& returnType = fFunction->returnType();
- std::unique_ptr<Expression> result;
- if (r.expression()) {
- if (returnType == *fIRGenerator->fContext.fTypes.fVoid) {
- fIRGenerator->errorReporter().error(r.fOffset,
- "may not return a value from a void function");
+ if (returnStmt.expression()) {
+ if (this->functionReturnsValue()) {
+ // Coerce return expression to the function's return type.
+ returnStmt.setExpression(fIRGenerator->coerce(
+ std::move(returnStmt.expression()), returnType));
+ fEncounteredReturnValue = true;
} else {
- result = fIRGenerator->coerce(std::move(r.expression()), returnType);
+ // Returning something from a function with a void return type.
+ fIRGenerator->errorReporter().error(returnStmt.fOffset,
+ "may not return a value from a void function");
}
- } else if (returnType != *fIRGenerator->fContext.fTypes.fVoid) {
- fIRGenerator->errorReporter().error(r.fOffset,
- "expected function to return '" +
- returnType.displayName() + "'");
+ } else {
+ if (this->functionReturnsValue()) {
+ // Returning nothing from a function with a non-void return type.
+ fIRGenerator->errorReporter().error(returnStmt.fOffset,
+ "expected function to return '" + returnType.displayName() + "'");
+ }
}
- r.setExpression(std::move(result));
break;
}
case Statement::Kind::kDo:
@@ -998,6 +1020,8 @@
int fBreakableLevel = 0;
// how deeply nested we are in continuable constructs (for, do).
int fContinuableLevel = 0;
+ // have we found a return statement with a return value?
+ bool fEncounteredReturnValue = false;
using INHERITED = ProgramWriter;
};