Report an error if an out param is never written to.
GLSL ES2 behavior is explicitly undefined if an out-param is never
written to: "If a function does not write to an out parameter, the value
of the actual parameter is undefined when the function returns."
We do see divergence here in practice: SkVM's behavior (the parameter is
left alone) differs from my GPU's behavior (the parameter is zeroed
out).
SkSL will now report an error if an out parameter is never assigned-to.
There is no control flow analysis performed, so we will not report
cases where the out parameter is assigned-to on some paths but not
others. (Technically the return-on-all-paths logic could be adapted
for this, but it would be a fair amount of work.)
Structs are currently exempt from the rule because custom mesh
specifications require an `out` parameter for a Varyings struct, even if
your mesh program doesn't need Varyings.
Bug: skia:12867
Change-Id: Ie828d3ce91c2c67e008ae304fdb163ffa88d744c
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/500440
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
diff --git a/gn/sksl_tests.gni b/gn/sksl_tests.gni
index 481a2cd..3c1bd88 100644
--- a/gn/sksl_tests.gni
+++ b/gn/sksl_tests.gni
@@ -215,6 +215,7 @@
"/sksl/errors/SwizzleOutOfBounds.sksl",
"/sksl/errors/SwizzleTooManyComponents.sksl",
"/sksl/errors/TernaryMismatch.sksl",
+ "/sksl/errors/UnassignedOutParameter.sksl",
"/sksl/errors/UndeclaredFunction.sksl",
"/sksl/errors/UndefinedFunction.sksl",
"/sksl/errors/UndefinedSymbol.sksl",
diff --git a/resources/sksl/errors/UnassignedOutParameter.sksl b/resources/sksl/errors/UnassignedOutParameter.sksl
new file mode 100644
index 0000000..990d00d
--- /dev/null
+++ b/resources/sksl/errors/UnassignedOutParameter.sksl
@@ -0,0 +1,11 @@
+// Expect 2 errors in "testOut"
+
+void testIn(in float2 a, in float2 b, in float2 c) { a = float2(1); }
+void testOut(out float2 a, out float2 b, out float2 c) { a = float2(1); }
+void testInout(inout float2 a, inout float2 b, inout float2 c) { a = float2(1); }
+
+void main(float2 p) {
+ testIn(p, p, p);
+ testOut(p, p, p);
+ testInout(p, p, p);
+}
diff --git a/resources/sksl/runtime/QualifierOrder.rts b/resources/sksl/runtime/QualifierOrder.rts
index 6d1ad4b..b08b282 100644
--- a/resources/sksl/runtime/QualifierOrder.rts
+++ b/resources/sksl/runtime/QualifierOrder.rts
@@ -7,7 +7,7 @@
// These qualifiers are reversed from the expected order, but SkSL should compile and run anyway.
noinline void const_after_in(in const vec2 x) {}
noinline void inout_after_high_precision(highp inout vec2 x) {}
-noinline void out_after_high_precision(highp out vec2 x) {}
+noinline void out_after_high_precision(highp out vec2 x) { x = vec2(0); }
vec4 main(vec2 coords) {
const_after_in(coords);
diff --git a/src/sksl/SkSLAnalysis.h b/src/sksl/SkSLAnalysis.h
index 11143dc..4f6f616 100644
--- a/src/sksl/SkSLAnalysis.h
+++ b/src/sksl/SkSLAnalysis.h
@@ -175,6 +175,7 @@
* Runs at finalization time to perform any last-minute correctness checks:
* - Reports @if/@switch statements that didn't optimize away
* - Reports dangling FunctionReference or TypeReference expressions
+ * - Reports function `out` params which are never written to (structs are currently exempt)
*/
void DoFinalizationChecks(const Program& program);
diff --git a/src/sksl/analysis/SkSLFinalizationChecks.cpp b/src/sksl/analysis/SkSLFinalizationChecks.cpp
index 6a93ddf..f47afe2 100644
--- a/src/sksl/analysis/SkSLFinalizationChecks.cpp
+++ b/src/sksl/analysis/SkSLFinalizationChecks.cpp
@@ -13,35 +13,70 @@
#include "src/sksl/analysis/SkSLProgramVisitor.h"
#include "src/sksl/ir/SkSLFunctionCall.h"
#include "src/sksl/ir/SkSLFunctionDeclaration.h"
+#include "src/sksl/ir/SkSLFunctionDefinition.h"
#include "src/sksl/ir/SkSLIfStatement.h"
#include "src/sksl/ir/SkSLProgram.h"
#include "src/sksl/ir/SkSLSwitchStatement.h"
#include "src/sksl/ir/SkSLVarDeclarations.h"
+#include "src/sksl/ir/SkSLVariable.h"
namespace SkSL {
namespace {
class FinalizationVisitor : public ProgramVisitor {
public:
- FinalizationVisitor(const Context& ctx) : fContext(ctx) {}
+ FinalizationVisitor(const Context& c, const ProgramUsage& u) : fContext(c), fUsage(u) {}
bool visitProgramElement(const ProgramElement& pe) override {
- if (pe.kind() == ProgramElement::Kind::kGlobalVar) {
- const VarDeclaration& decl =
- pe.as<GlobalVarDeclaration>().declaration()->as<VarDeclaration>();
-
- size_t prevSlotsUsed = fGlobalSlotsUsed;
- fGlobalSlotsUsed = SkSafeMath::Add(fGlobalSlotsUsed, decl.var().type().slotCount());
- // To avoid overzealous error reporting, only trigger the error at the first
- // place where the global limit is exceeded.
- if (prevSlotsUsed < kVariableSlotLimit && fGlobalSlotsUsed >= kVariableSlotLimit) {
- fContext.fErrors->error(pe.fLine, "global variable '" + decl.var().name() +
- "' exceeds the size limit");
+ switch (pe.kind()) {
+ case ProgramElement::Kind::kGlobalVar: {
+ this->checkGlobalVariableSizeLimit(pe.as<GlobalVarDeclaration>());
+ break;
}
+ case ProgramElement::Kind::kFunction: {
+ this->checkOutParamsAreAssigned(pe.as<FunctionDefinition>());
+ break;
+ }
+ default:
+ break;
}
return INHERITED::visitProgramElement(pe);
}
+ void checkGlobalVariableSizeLimit(const GlobalVarDeclaration& globalDecl) {
+ const VarDeclaration& decl = globalDecl.declaration()->as<VarDeclaration>();
+
+ size_t prevSlotsUsed = fGlobalSlotsUsed;
+ fGlobalSlotsUsed = SkSafeMath::Add(fGlobalSlotsUsed, decl.var().type().slotCount());
+ // To avoid overzealous error reporting, only trigger the error at the first place where the
+ // global limit is exceeded.
+ if (prevSlotsUsed < kVariableSlotLimit && fGlobalSlotsUsed >= kVariableSlotLimit) {
+ fContext.fErrors->error(decl.fLine, "global variable '" + decl.var().name() +
+ "' exceeds the size limit");
+ }
+ }
+
+ void checkOutParamsAreAssigned(const FunctionDefinition& funcDef) {
+ const FunctionDeclaration& funcDecl = funcDef.declaration();
+
+ // Searches for `out` parameters that are not written to. According to the GLSL spec,
+ // the value of an out-param that's never assigned to is unspecified, so report it.
+ // Structs are currently exempt from the rule because custom mesh specifications require an
+ // `out` parameter for a Varyings struct, even if your mesh program doesn't need Varyings.
+ for (const Variable* param : funcDecl.parameters()) {
+ const int paramInout = param->modifiers().fFlags & (Modifiers::Flag::kIn_Flag |
+ Modifiers::Flag::kOut_Flag);
+ if (!param->type().isStruct() && paramInout == Modifiers::Flag::kOut_Flag) {
+ ProgramUsage::VariableCounts counts = fUsage.get(*param);
+ if (counts.fWrite <= 0) {
+ fContext.fErrors->error(funcDecl.fLine,
+ "function '" + funcDecl.name() + "' never assigns a "
+ "value to out parameter '" + param->name() + "'");
+ }
+ }
+ }
+ }
+
bool visitStatement(const Statement& stmt) override {
if (!fContext.fConfig->fSettings.fPermitInvalidStaticTests) {
switch (stmt.kind()) {
@@ -53,8 +88,7 @@
case Statement::Kind::kSwitch:
if (stmt.as<SwitchStatement>().isStatic()) {
- fContext.fErrors->error(stmt.fLine,
- "static switch has non-static test");
+ fContext.fErrors->error(stmt.fLine, "static switch has non-static test");
}
break;
@@ -95,13 +129,14 @@
using INHERITED = ProgramVisitor;
size_t fGlobalSlotsUsed = 0;
const Context& fContext;
+ const ProgramUsage& fUsage;
};
} // namespace
void Analysis::DoFinalizationChecks(const Program& program) {
// Check all of the program's owned elements. (Built-in elements are assumed to be valid.)
- FinalizationVisitor visitor{*program.fContext};
+ FinalizationVisitor visitor{*program.fContext, *program.usage()};
for (const std::unique_ptr<ProgramElement>& element : program.fOwnedElements) {
visitor.visitProgramElement(*element);
}
diff --git a/tests/sksl/errors/UnassignedOutParameter.glsl b/tests/sksl/errors/UnassignedOutParameter.glsl
new file mode 100644
index 0000000..453cd77
--- /dev/null
+++ b/tests/sksl/errors/UnassignedOutParameter.glsl
@@ -0,0 +1,5 @@
+### Compilation failed:
+
+error: 4: function 'testOut' never assigns a value to out parameter 'b'
+error: 4: function 'testOut' never assigns a value to out parameter 'c'
+2 errors
diff --git a/tests/sksl/runtime/QualifierOrder.stage b/tests/sksl/runtime/QualifierOrder.stage
index 3990477..be9e879 100644
--- a/tests/sksl/runtime/QualifierOrder.stage
+++ b/tests/sksl/runtime/QualifierOrder.stage
@@ -10,6 +10,7 @@
}
noinline void out_after_high_precision_0(out float2 x)
{
+ x = float2(0.0);
}
float4 main(float2 coords)
{
