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/resources/sksl/errors/CanExitWithoutReturningValue.sksl b/resources/sksl/errors/CanExitWithoutReturningValue.sksl
index 6d4be94..e678746 100644
--- a/resources/sksl/errors/CanExitWithoutReturningValue.sksl
+++ b/resources/sksl/errors/CanExitWithoutReturningValue.sksl
@@ -1 +1,4 @@
-int foo() { if (2 > 5) return 3; }
+// Expect 1 error
+
+int not_detected() { if (sqrt(1) == 1) return 3; } // function finalizer doesn't analyze all paths
+int is_detected() { if (2 > 5) return 3; } // optimizer reduces this to an empty function
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;
};
diff --git a/src/sksl/SkSLInliner.cpp b/src/sksl/SkSLInliner.cpp
index bad4829..d0a4dd7 100644
--- a/src/sksl/SkSLInliner.cpp
+++ b/src/sksl/SkSLInliner.cpp
@@ -767,10 +767,12 @@
/*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 '") +
+ // returned anything on any path! This should have been detected in the function finalizer.
+ // Still, discard our output and generate an error.
+ SkDEBUGFAIL("inliner found non-void function that fails to return a value on any path");
+ fContext->fErrors.error(function.fOffset, "inliner found non-void function '" +
function.declaration().name() +
- "' exits without returning a value");
+ "' that fails to return a value on any path");
inlinedCall = {};
}
diff --git a/tests/SkSLDSLTest.cpp b/tests/SkSLDSLTest.cpp
index ac73b93..f51523c 100644
--- a/tests/SkSLDSLTest.cpp
+++ b/tests/SkSLDSLTest.cpp
@@ -1066,7 +1066,8 @@
}
{
- ExpectError error(r, "error: expected function to return 'float'\n");
+ ExpectError error(r, "error: expected function to return 'float'\n"
+ "error: function 'broken' exits without returning a value\n");
DSLWriter::ProgramElements().clear();
DSLFunction(kFloat, "broken").define(
Return()
diff --git a/tests/sksl/errors/CanExitWithoutReturningValue.glsl b/tests/sksl/errors/CanExitWithoutReturningValue.glsl
index e48b40b..bfbcc10 100644
--- a/tests/sksl/errors/CanExitWithoutReturningValue.glsl
+++ b/tests/sksl/errors/CanExitWithoutReturningValue.glsl
@@ -1,4 +1,4 @@
### Compilation failed:
-error: 1: function 'foo' can exit without returning a value
+error: 4: function 'is_detected' exits without returning a value
1 error
diff --git a/tests/sksl/errors/Ossfuzz31469.glsl b/tests/sksl/errors/Ossfuzz31469.glsl
index 62d9a44..dd03a87 100644
--- a/tests/sksl/errors/Ossfuzz31469.glsl
+++ b/tests/sksl/errors/Ossfuzz31469.glsl
@@ -1,5 +1,4 @@
### Compilation failed:
-error: 1: function 'n' can exit without returning a value
error: 1: function 'n' exits without returning a value
-2 errors
+1 error
diff --git a/tests/sksl/errors/ReturnMissingValue.glsl b/tests/sksl/errors/ReturnMissingValue.glsl
index 525de89..4a4338f 100644
--- a/tests/sksl/errors/ReturnMissingValue.glsl
+++ b/tests/sksl/errors/ReturnMissingValue.glsl
@@ -1,4 +1,5 @@
### Compilation failed:
error: 1: expected function to return 'int'
-1 error
+error: 1: function 'foo' exits without returning a value
+2 errors
