From ff6c5bf5ed1bf253505e4b3c7774925408ec21fa Mon Sep 17 00:00:00 2001 From: John Stiles Date: Tue, 7 Sep 2021 15:22:02 -0400 Subject: [PATCH] Generate more diagnostics from IRGenerator, when given bad code. Two minor changes: - Converting a Block with bad statements will now generate a partial block instead of nullptr. The change mirrors how DSL behaves; functions containing invalid statements will now be created and added to the program. Previously, we would discard a function definition with any invalid statements inside; this prevented duplicate-function-definition errors from appearing. - Converting a return with a bad expression will now generate a poisoned return instead of nullptr. This change improves diagnostics for functions with invalid return statements. If we eliminate the return statements (by returning null), we report bad return statements as "function can exit without returning a value" (which is confusing). Change-Id: I6d998d5c50585f8d96bb7e3cb7f59b63125d6a62 Reviewed-on: https://skia-review.googlesource.com/c/skia/+/446325 Commit-Queue: John Stiles Auto-Submit: John Stiles Reviewed-by: Ethan Nicholas Reviewed-by: Brian Osman --- src/sksl/SkSLIRGenerator.cpp | 18 +++++++++--------- tests/sksl/errors/MismatchedNumbers.glsl | 4 +++- tests/sksl/errors/Ossfuzz27650.glsl | 3 ++- tests/sksl/errors/Ossfuzz28050.glsl | 3 +-- tests/sksl/errors/Ossfuzz29444.glsl | 3 +-- .../sksl/runtime_errors/IllegalStatements.skvm | 3 ++- 6 files changed, 18 insertions(+), 16 deletions(-) diff --git a/src/sksl/SkSLIRGenerator.cpp b/src/sksl/SkSLIRGenerator.cpp index 4f234ad686..3e841fd717 100644 --- a/src/sksl/SkSLIRGenerator.cpp +++ b/src/sksl/SkSLIRGenerator.cpp @@ -47,6 +47,7 @@ #include "src/sksl/ir/SkSLInterfaceBlock.h" #include "src/sksl/ir/SkSLMethodReference.h" #include "src/sksl/ir/SkSLNop.h" +#include "src/sksl/ir/SkSLPoison.h" #include "src/sksl/ir/SkSLPostfixExpression.h" #include "src/sksl/ir/SkSLPrefixExpression.h" #include "src/sksl/ir/SkSLReturnStatement.h" @@ -141,10 +142,9 @@ std::unique_ptr IRGenerator::convertBlock(const ASTNode& block) { StatementArray statements; for (const auto& child : block) { std::unique_ptr statement = this->convertStatement(child); - if (!statement) { - return nullptr; + if (statement) { + statements.push_back(std::move(statement)); } - statements.push_back(std::move(statement)); } return Block::Make(block.fOffset, std::move(statements), fSymbolTable); } @@ -518,14 +518,14 @@ std::unique_ptr IRGenerator::convertReturn(int offset, std::unique_ptr IRGenerator::convertReturn(const ASTNode& r) { SkASSERT(r.fKind == ASTNode::Kind::kReturn); if (r.begin() != r.end()) { - std::unique_ptr value = this->convertExpression(*r.begin()); - if (!value) { - return nullptr; + if (std::unique_ptr value = this->convertExpression(*r.begin())) { + return this->convertReturn(r.fOffset, std::move(value)); + } else { + return this->convertReturn(r.fOffset, Poison::Make(r.fOffset, fContext)); } - return this->convertReturn(r.fOffset, std::move(value)); - } else { - return this->convertReturn(r.fOffset, /*result=*/nullptr); } + + return this->convertReturn(r.fOffset, /*result=*/nullptr); } std::unique_ptr IRGenerator::convertBreak(const ASTNode& b) { diff --git a/tests/sksl/errors/MismatchedNumbers.glsl b/tests/sksl/errors/MismatchedNumbers.glsl index ae2ce87607..9bcfd24a43 100644 --- a/tests/sksl/errors/MismatchedNumbers.glsl +++ b/tests/sksl/errors/MismatchedNumbers.glsl @@ -5,7 +5,9 @@ error: 29: type mismatch: '=' cannot operate on 'uint', 'float' error: 30: type mismatch: '=' cannot operate on 'int', 'uint' error: 31: type mismatch: '=' cannot operate on 'int', 'float' error: 32: type mismatch: '=' cannot operate on 'float', 'int' +error: 32: function 'f_eq_i_disallowed' can exit without returning a value error: 33: type mismatch: '=' cannot operate on 'float', 'uint' +error: 33: function 'f_eq_u_disallowed' can exit without returning a value error: 34: type mismatch: '=' cannot operate on 'uint', 'int' error: 35: type mismatch: '=' cannot operate on 'uint', 'float' error: 36: type mismatch: '+' cannot operate on 'int', 'float' @@ -48,4 +50,4 @@ error: 72: type mismatch: '+' cannot operate on 'uint', 'int' error: 73: type mismatch: '-' cannot operate on 'uint', 'int' error: 74: type mismatch: '*' cannot operate on 'uint', 'int' error: 75: type mismatch: '/' cannot operate on 'uint', 'int' -48 errors +50 errors diff --git a/tests/sksl/errors/Ossfuzz27650.glsl b/tests/sksl/errors/Ossfuzz27650.glsl index 37c35c1778..4257eda9c5 100644 --- a/tests/sksl/errors/Ossfuzz27650.glsl +++ b/tests/sksl/errors/Ossfuzz27650.glsl @@ -1,4 +1,5 @@ ### Compilation failed: error: 1: expected 'int', but found 'float' -1 error +error: 1: unknown identifier 'i' +2 errors diff --git a/tests/sksl/errors/Ossfuzz28050.glsl b/tests/sksl/errors/Ossfuzz28050.glsl index 6fc2ee0ef1..a73760e7a9 100644 --- a/tests/sksl/errors/Ossfuzz28050.glsl +++ b/tests/sksl/errors/Ossfuzz28050.glsl @@ -1,5 +1,4 @@ ### Compilation failed: error: 2: invalid arguments to 'float[1]' constructor (expected 1 elements, but found 0) -error: 3: function 'void wna()' is not defined -2 errors +1 error diff --git a/tests/sksl/errors/Ossfuzz29444.glsl b/tests/sksl/errors/Ossfuzz29444.glsl index 6fc2ee0ef1..a73760e7a9 100644 --- a/tests/sksl/errors/Ossfuzz29444.glsl +++ b/tests/sksl/errors/Ossfuzz29444.glsl @@ -1,5 +1,4 @@ ### Compilation failed: error: 2: invalid arguments to 'float[1]' constructor (expected 1 elements, but found 0) -error: 3: function 'void wna()' is not defined -2 errors +1 error diff --git a/tests/sksl/runtime_errors/IllegalStatements.skvm b/tests/sksl/runtime_errors/IllegalStatements.skvm index 2e155845fc..abea4174fd 100644 --- a/tests/sksl/runtime_errors/IllegalStatements.skvm +++ b/tests/sksl/runtime_errors/IllegalStatements.skvm @@ -4,4 +4,5 @@ error: 3: discard statement is only permitted in fragment shaders error: 5: do-while loops are not supported error: 7: while loops are not supported error: 9: switch statements are not supported -4 errors +error: 9: function 'switch_stmt' can exit without returning a value +5 errors