AuroraMiddleware/skia2
1
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

Added Poison values to SkSL to improve DSL error handling

Browse Source 
This also tightens up the rules around releasing DSL objects.

Bug: skia:12133
Change-Id: I11a6d8fbcec58374f7b5ed5ced1c5c112e2b7cc7
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/421323
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
This commit is contained in:
Ethan Nicholas 2021-06-25 12:31:44 -04:00 committed by Skia Commit-Bot
parent 1c39ecab1e
commit 549c6b8739
21 changed files with 135 additions and 74 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
gn/sksl.gni
View File

@ -151,6 +151,7 @@ skia_sksl_sources = [
"$_src/sksl/ir/SkSLInterfaceBlock.h",
"$_src/sksl/ir/SkSLModifiersDeclaration.h",
"$_src/sksl/ir/SkSLNop.h",
"$_src/sksl/ir/SkSLPoison.h",
"$_src/sksl/ir/SkSLPostfixExpression.cpp",
"$_src/sksl/ir/SkSLPostfixExpression.h",
"$_src/sksl/ir/SkSLPrefixExpression.cpp",

15
include/sksl/DSLExpression.h
View File

@ -114,16 +114,27 @@ public:
DSLPossibleExpression operator()(SkTArray<DSLWrapper<DSLExpression>> args);
/**
* Returns true if this object contains an expression. DSLExpressions which were created with
* the empty constructor or which have already been release()ed are not valid. DSLExpressions
* created with errors are still considered valid (but contain a poison value).
*/
bool valid() const {
return fExpression != nullptr;
}
/**
* Invalidates this object and returns the SkSL expression it represents.
* Invalidates this object and returns the SkSL expression it represents. It is an error to call
* this on an invalid DSLExpression.
*/
std::unique_ptr<SkSL::Expression> release();
private:
/**
* Calls release if this expression is valid, otherwise returns null.
*/
std::unique_ptr<SkSL::Expression> releaseIfValid();
void swap(DSLExpression& other);
/**
@ -246,7 +257,7 @@ public:
DSLPossibleExpression operator--(int);
std::unique_ptr<SkSL::Expression> release();
std::unique_ptr<SkSL::Expression> release(PositionInfo pos = PositionInfo());
private:
std::unique_ptr<SkSL::Expression> fExpression;

11
include/sksl/DSLStatement.h
View File

@ -48,7 +48,10 @@ public:
DSLStatement& operator=(DSLStatement&& other) = default;
bool valid() { return fStatement != nullptr; }
std::unique_ptr<SkSL::Statement> release() {
SkASSERT(this->valid());
return std::move(fStatement);
}
@ -57,6 +60,10 @@ private:
DSLStatement(std::unique_ptr<SkSL::Expression> expr);
std::unique_ptr<SkSL::Statement> releaseIfValid() {
return std::move(fStatement);
}
std::unique_ptr<SkSL::Statement> fStatement;
friend class DSLBlock;
@ -83,8 +90,10 @@ public:
~DSLPossibleStatement();
bool valid() { return fStatement != nullptr; }
std::unique_ptr<SkSL::Statement> release() {
return std::move(fStatement);
return DSLStatement(std::move(*this)).release();
}
private:

2
include/sksl/DSLSymbols.h
View File

@ -47,7 +47,7 @@ std::shared_ptr<SymbolTable> CurrentSymbolTable();
/**
* Returns an expression referring to the named symbol.
*/
DSLExpression Symbol(skstd::string_view name);
DSLPossibleExpression Symbol(skstd::string_view name);
/**
* Returns true if the name refers to a type.

4
src/sksl/SkSLAnalysis.cpp
View File

@ -1060,6 +1060,9 @@ public:
case Expression::Kind::kFunctionCall:
return true;
case Expression::Kind::kPoison:
return true;
// These should never appear in final IR
case Expression::Kind::kExternalFunctionReference:
case Expression::Kind::kFunctionReference:
@ -1153,6 +1156,7 @@ template <typename T> bool TProgramVisitor<T>::visitExpression(typename T::Expre
case Expression::Kind::kFloatLiteral:
case Expression::Kind::kFunctionReference:
case Expression::Kind::kIntLiteral:
case Expression::Kind::kPoison:
case Expression::Kind::kSetting:
case Expression::Kind::kTypeReference:
case Expression::Kind::kVariableReference:

2
src/sksl/SkSLBuiltinTypes.cpp
View File

@ -8,6 +8,7 @@
#include "src/sksl/SkSLBuiltinTypes.h"
#include "include/private/SkSLModifiers.h"
#include "src/sksl/SkSLCompiler.h"
#include "src/sksl/SkSLErrorReporter.h"
#include "src/sksl/ir/SkSLExpression.h"
#include "src/sksl/spirv.h"
@ -122,6 +123,7 @@ BuiltinTypes::BuiltinTypes()
, fBool3(MakeVectorType("bool3", "b3", *fBool, /*columns=*/3))
, fBool4(MakeVectorType("bool4", "b4", *fBool, /*columns=*/4))
, fInvalid(MakeSpecialType("<INVALID>", "O", Type::TypeKind::kOther))
, fPoison(MakeSpecialType(Compiler::POISON_TAG, "P", Type::TypeKind::kOther))
, fVoid(MakeSpecialType("void", "v", Type::TypeKind::kVoid))
, fFloatLiteral(MakeLiteralType("$floatLiteral", *fFloat, /*priority=*/8))
, fIntLiteral(MakeLiteralType("$intLiteral", *fInt, /*priority=*/5))

1
src/sksl/SkSLBuiltinTypes.h
View File

@ -57,6 +57,7 @@ public:
const std::unique_ptr<Type> fBool4;
const std::unique_ptr<Type> fInvalid;
const std::unique_ptr<Type> fPoison;
const std::unique_ptr<Type> fVoid;
const std::unique_ptr<Type> fFloatLiteral;
const std::unique_ptr<Type> fIntLiteral;

4
src/sksl/SkSLCompiler.cpp
View File

@ -1033,6 +1033,10 @@ Position Compiler::position(int offset) {
}
void Compiler::error(int offset, String msg) {
if (strstr(msg.c_str(), POISON_TAG)) {
// don't report errors on poison values
return;
}
fErrorCount++;
Position pos = this->position(offset);
fErrorTextLength.push_back(fErrorText.length());

1
src/sksl/SkSLCompiler.h
View File

@ -73,6 +73,7 @@ public:
static constexpr const char FRAGCOLOR_NAME[] = "sk_FragColor";
static constexpr const char RTADJUST_NAME[] = "sk_RTAdjust";
static constexpr const char PERVERTEX_NAME[] = "sk_PerVertex";
static constexpr const char POISON_TAG[] = "<POISON>";
/**
* Gets a float4 that adjusts the position from Skia device coords to normalized device coords,

1
src/sksl/SkSLDehydrator.cpp
View File

@ -429,6 +429,7 @@ void Dehydrator::write(const Expression* e) {
break;
}
case Expression::Kind::kFunctionReference:
case Expression::Kind::kPoison:
case Expression::Kind::kTypeReference:
SkDEBUGFAIL("this expression shouldn't appear in finished code");
break;

11
src/sksl/dsl/DSLCore.cpp
View File

@ -161,15 +161,16 @@ public:
static DSLPossibleStatement For(DSLStatement initializer, DSLExpression test,
DSLExpression next, DSLStatement stmt, PositionInfo pos) {
return ForStatement::Convert(DSLWriter::Context(), /*offset=*/-1, initializer.release(),
test.release(), next.release(), stmt.release(),
return ForStatement::Convert(DSLWriter::Context(), /*offset=*/-1,
initializer.releaseIfValid(), test.releaseIfValid(),
next.releaseIfValid(), stmt.release(),
DSLWriter::SymbolTable());
}
static DSLPossibleStatement If(DSLExpression test, DSLStatement ifTrue, DSLStatement ifFalse,
bool isStatic) {
return IfStatement::Convert(DSLWriter::Context(), /*offset=*/-1, isStatic, test.release(),
ifTrue.release(), ifFalse.release());
ifTrue.release(), ifFalse.releaseIfValid());
}
static DSLPossibleStatement Return(DSLExpression value, PositionInfo pos) {
@ -177,7 +178,7 @@ public:
// this point we do not know the function's return type. We therefore do not check for
// errors, or coerce the value to the correct type, until the return statement is actually
// added to a function. (This is done in IRGenerator::finalizeFunction.)
return SkSL::ReturnStatement::Make(/*offset=*/-1, value.release());
return SkSL::ReturnStatement::Make(/*offset=*/-1, value.releaseIfValid());
}
static DSLExpression Swizzle(DSLExpression base, SkSL::SwizzleComponent::Type a,
@ -230,7 +231,7 @@ public:
SkTArray<StatementArray> statements;
statements.reserve_back(cases.count());
for (DSLCase& c : cases) {
values.push_back(c.fValue.release());
values.push_back(c.fValue.releaseIfValid());
statements.push_back(std::move(c.fStatements));
}
return DSLWriter::ConvertSwitch(value.release(), std::move(values), std::move(statements),

12
src/sksl/dsl/DSLExpression.cpp
View File

@ -16,6 +16,7 @@
#include "src/sksl/ir/SkSLBoolLiteral.h"
#include "src/sksl/ir/SkSLFloatLiteral.h"
#include "src/sksl/ir/SkSLIntLiteral.h"
#include "src/sksl/ir/SkSLPoison.h"
#include "math.h"
@ -82,6 +83,8 @@ DSLExpression::DSLExpression(DSLPossibleExpression expr, PositionInfo pos) {
DSLWriter::ReportErrors(pos);
if (expr.valid()) {
fExpression = std::move(expr.fExpression);
} else {
fExpression = SkSL::Poison::Make(DSLWriter::Context());
}
}
@ -102,6 +105,11 @@ void DSLExpression::swap(DSLExpression& other) {
}
std::unique_ptr<SkSL::Expression> DSLExpression::release() {
SkASSERT(this->valid());
return std::move(fExpression);
}
std::unique_ptr<SkSL::Expression> DSLExpression::releaseIfValid() {
return std::move(fExpression);
}
@ -343,8 +351,8 @@ DSLPossibleExpression DSLPossibleExpression::operator--(int) {
return DSLExpression(this->release())--;
}
std::unique_ptr<SkSL::Expression> DSLPossibleExpression::release() {
return std::move(fExpression);
std::unique_ptr<SkSL::Expression> DSLPossibleExpression::release(PositionInfo pos) {
return DSLExpression(std::move(*this), pos).release();
}
} // namespace dsl

3
src/sksl/dsl/DSLFunction.cpp
View File

@ -37,9 +37,10 @@ void DSLFunction::init(const DSLType& returnType, skstd::string_view name,
DSLWriter::ReportError("error: using an already-declared variable as a function "
"parameter\n");
}
if (param->fInitialValue.release()) {
if (param->fInitialValue.valid()) {
DSLWriter::ReportError("error: variables used as function parameters cannot have "
"initial values\n");
param->fInitialValue.release();
}
param->fDeclared = true;
param->fStorage = SkSL::VariableStorage::kParameter;

11
src/sksl/dsl/DSLStatement.cpp
View File

@ -13,6 +13,7 @@
#include "src/sksl/dsl/priv/DSLWriter.h"
#include "src/sksl/ir/SkSLBlock.h"
#include "src/sksl/ir/SkSLExpressionStatement.h"
#include "src/sksl/ir/SkSLNop.h"
#if !defined(SKSL_STANDALONE) && SK_SUPPORT_GPU
#include "src/gpu/glsl/GrGLSLFragmentShaderBuilder.h"
@ -35,7 +36,9 @@ DSLStatement::DSLStatement(DSLExpression expr) {
}
DSLStatement::DSLStatement(std::unique_ptr<SkSL::Expression> expr)
: fStatement(std::make_unique<SkSL::ExpressionStatement>(std::move(expr))) {}
: fStatement(std::make_unique<SkSL::ExpressionStatement>(std::move(expr))) {
SkASSERT(this->valid());
}
DSLStatement::DSLStatement(std::unique_ptr<SkSL::Statement> stmt)
: fStatement(std::move(stmt)) {
@ -47,7 +50,11 @@ DSLStatement::DSLStatement(DSLPossibleExpression expr, PositionInfo pos)
DSLStatement::DSLStatement(DSLPossibleStatement stmt, PositionInfo pos) {
DSLWriter::ReportErrors(pos);
fStatement = std::move(stmt.fStatement);
if (stmt.valid()) {
fStatement = std::move(stmt.fStatement);
} else {
fStatement = SkSL::Nop::Make();
}
}
DSLStatement::~DSLStatement() {

4
src/sksl/dsl/DSLSymbols.cpp
View File

@ -26,8 +26,8 @@ std::shared_ptr<SymbolTable> CurrentSymbolTable() {
return DSLWriter::IRGenerator().symbolTable();
}
DSLExpression Symbol(skstd::string_view name) {
return DSLExpression(DSLWriter::IRGenerator().convertIdentifier(/*offset=*/-1, name));
DSLPossibleExpression Symbol(skstd::string_view name) {
return DSLWriter::IRGenerator().convertIdentifier(/*offset=*/-1, name);
}
bool IsType(skstd::string_view name) {

2
src/sksl/dsl/priv/DSLFPs.cpp
View File

@ -30,7 +30,7 @@ DSLVar sk_SampleCoord() {
}
DSLExpression SampleChild(int index, DSLExpression sampleExpr) {
std::unique_ptr<SkSL::Expression> expr = sampleExpr.release();
std::unique_ptr<SkSL::Expression> expr = sampleExpr.releaseIfValid();
if (expr) {
SkASSERT(expr->type().isVector());
SkASSERT(expr->type().componentType().isFloat());

47
src/sksl/dsl/priv/DSLWriter.cpp
View File

@ -129,11 +129,6 @@ GrGLSLUniformHandler::UniformHandle DSLWriter::VarUniformHandle(const DSLVar& va
std::unique_ptr<SkSL::Expression> DSLWriter::Call(const FunctionDeclaration& function,
ExpressionArray arguments) {
for (const std::unique_ptr<SkSL::Expression>& expr : arguments) {
if (!expr) {
return nullptr;
}
}
// We can't call FunctionCall::Convert directly here, because intrinsic management is handled in
// IRGenerator::call.
return IRGenerator().call(/*offset=*/-1, function, std::move(arguments));
@ -141,23 +136,12 @@ std::unique_ptr<SkSL::Expression> DSLWriter::Call(const FunctionDeclaration& fun
std::unique_ptr<SkSL::Expression> DSLWriter::Call(std::unique_ptr<SkSL::Expression> expr,
ExpressionArray arguments) {
if (!expr) {
return nullptr;
}
for (const std::unique_ptr<SkSL::Expression>& arg : arguments) {
if (!arg) {
return nullptr;
}
}
// We can't call FunctionCall::Convert directly here, because intrinsic management is handled in
// IRGenerator::call.
return IRGenerator().call(/*offset=*/-1, std::move(expr), std::move(arguments));
}
DSLPossibleExpression DSLWriter::Coerce(std::unique_ptr<Expression> expr, const SkSL::Type& type) {
if (!expr) {
return DSLPossibleExpression(nullptr);
}
return IRGenerator().coerce(std::move(expr), type);
}
@ -167,9 +151,6 @@ DSLPossibleExpression DSLWriter::Construct(const SkSL::Type& type,
args.reserve_back(rawArgs.size());
for (DSLExpression& arg : rawArgs) {
if (!arg.valid()) {
return DSLPossibleExpression(nullptr);
}
args.push_back(arg.release());
}
return SkSL::Constructor::Convert(Context(), /*offset=*/-1, type, std::move(args));
@ -178,41 +159,26 @@ DSLPossibleExpression DSLWriter::Construct(const SkSL::Type& type,
std::unique_ptr<SkSL::Expression> DSLWriter::ConvertBinary(std::unique_ptr<Expression> left,
Operator op,
std::unique_ptr<Expression> right) {
if (!left || !right) {
return nullptr;
}
return BinaryExpression::Convert(Context(), std::move(left), op, std::move(right));
}
std::unique_ptr<SkSL::Expression> DSLWriter::ConvertField(std::unique_ptr<Expression> base,
skstd::string_view name) {
if (!base) {
return nullptr;
}
return FieldAccess::Convert(Context(), std::move(base), name);
}
std::unique_ptr<SkSL::Expression> DSLWriter::ConvertIndex(std::unique_ptr<Expression> base,
std::unique_ptr<Expression> index) {
if (!base || !index) {
return nullptr;
}
return IndexExpression::Convert(Context(), std::move(base), std::move(index));
}
std::unique_ptr<SkSL::Expression> DSLWriter::ConvertPostfix(std::unique_ptr<Expression> expr,
Operator op) {
if (!expr) {
return nullptr;
}
return PostfixExpression::Convert(Context(), std::move(expr), op);
}
std::unique_ptr<SkSL::Expression> DSLWriter::ConvertPrefix(Operator op,
std::unique_ptr<Expression> expr) {
if (!expr) {
return nullptr;
}
return PrefixExpression::Convert(Context(), op, std::move(expr));
}
@ -220,9 +186,6 @@ DSLPossibleStatement DSLWriter::ConvertSwitch(std::unique_ptr<Expression> value,
ExpressionArray caseValues,
SkTArray<SkSL::StatementArray> caseStatements,
bool isStatic) {
if (!value) {
return SkSL::Nop::Make();
}
StatementArray caseBlocks;
caseBlocks.resize(caseStatements.count());
for (int index = 0; index < caseStatements.count(); ++index) {
@ -270,8 +233,8 @@ const SkSL::Variable* DSLWriter::Var(DSLVar& var) {
// We can't call VarDeclaration::Convert directly here, because the IRGenerator has special
// treatment for sk_FragColor and sk_RTHeight that we want to preserve in DSL.
var.fDeclaration = DSLWriter::IRGenerator().convertVarDeclaration(
std::move(skslvar),
var.fInitialValue.release());
std::move(skslvar),
var.fInitialValue.releaseIfValid());
if (var.fDeclaration) {
var.fVar = varPtr;
}
@ -291,6 +254,12 @@ std::unique_ptr<SkSL::Variable> DSLWriter::ParameterVar(DSLVar& var) {
std::unique_ptr<SkSL::Statement> DSLWriter::Declaration(DSLVar& var) {
Var(var);
if (!var.fDeclaration) {
// We should have already reported an error before ending up here, just clean up the
// initial value so it doesn't assert and return a nop.
var.fInitialValue.releaseIfValid();
return SkSL::Nop::Make();
}
return std::move(var.fDeclaration);
}

3
src/sksl/ir/SkSLExpression.h
View File

@ -46,8 +46,9 @@ public:
kFunctionReference,
kFunctionCall,
kIndex,
kPrefix,
kPoison,
kPostfix,
kPrefix,
kSetting,
kSwizzle,
kTernary,

40
src/sksl/ir/SkSLPoison.h Normal file
View File

@ -0,0 +1,40 @@
/*
* Copyright 2021 Google LLC.
*
* Use of this source code is governed by a BSD-style license that can be
* found in the LICENSE file.
*/
#include "src/sksl/SkSLCompiler.h"
#include "src/sksl/SkSLContext.h"
namespace SkSL {
class Poison : public Expression {
public:
static constexpr Kind kExpressionKind = Kind::kPoison;
static std::unique_ptr<Expression> Make(const Context& context) {
return std::make_unique<Poison>(context.fTypes.fPoison.get());
}
Poison(const Type* type)
: INHERITED(/*offset=*/-1, kExpressionKind, type) {}
bool hasProperty(Property property) const override {
return false;
}
std::unique_ptr<Expression> clone() const override {
return std::make_unique<Poison>(&this->type());
}
String description() const override {
return Compiler::POISON_TAG;
}
private:
using INHERITED = Expression;
};
} // namespace SkSL

18
tests/SkSLDSLErrorLineNumbers.cpp
View File

@ -55,7 +55,7 @@ DEF_GPUTEST_FOR_MOCK_CONTEXT(DSLErrorLineNumbers, r, ctxInfo) {
ExpectErrorLineNumber error(r,
"error: type mismatch: '+' cannot operate on 'float', 'bool'\n",
__LINE__ + 1);
DSLExpression x = (Float(1) + true);
(Float(1) + true).release();
}
{
@ -64,7 +64,7 @@ DEF_GPUTEST_FOR_MOCK_CONTEXT(DSLErrorLineNumbers, r, ctxInfo) {
ExpectErrorLineNumber error(r,
"error: type mismatch: '=' cannot operate on 'bool', 'float'\n",
__LINE__ + 1);
DSLExpression x = (a = 5.0f);
(a = 5.0f).release();
}
{
@ -73,7 +73,7 @@ DEF_GPUTEST_FOR_MOCK_CONTEXT(DSLErrorLineNumbers, r, ctxInfo) {
ExpectErrorLineNumber error(r,
"error: expected 'int', but found 'bool'\n",
__LINE__ + 1);
DSLExpression x = (a[true]);
(a[true]).release();
}
{
@ -82,42 +82,42 @@ DEF_GPUTEST_FOR_MOCK_CONTEXT(DSLErrorLineNumbers, r, ctxInfo) {
ExpectErrorLineNumber error(r,
"error: '++' cannot operate on 'int[5]'\n",
__LINE__ + 1);
DSLExpression x = ++a;
(++a).release();
}
{
ExpectErrorLineNumber error(r,
"error: expected 'bool', but found 'int'\n",
__LINE__ + 1);
DSLStatement x = Do(Discard(), 5);
Do(Discard(), 5).release();
}
{
ExpectErrorLineNumber error(r,
"error: expected 'bool', but found 'int'\n",
__LINE__ + 1);
DSLStatement x = For(DSLStatement(), 5, DSLExpression(), DSLStatement());
For(DSLStatement(), 5, DSLExpression(), Block()).release();
}
{
ExpectErrorLineNumber error(r,
"error: expected 'bool', but found 'int'\n",
__LINE__ + 1);
DSLStatement x = If(5, Discard());
If(5, Discard()).release();
}
{
ExpectErrorLineNumber error(r,
"error: expected 'bool', but found 'int'\n",
__LINE__ + 1);
DSLStatement x = While(5, Discard());
While(5, Discard()).release();
}
{
ExpectErrorLineNumber error(r,
"error: no match for abs(bool)\n",
__LINE__ + 1);
DSLStatement x = Abs(true);
Abs(true).release();
}
End();
}

16
tests/SkSLDSLTest.cpp
View File

@ -689,7 +689,7 @@ DEF_GPUTEST_FOR_MOCK_CONTEXT(DSLPlus, r, ctxInfo) {
{
ExpectError error(r, "error: '+' cannot operate on 'bool'\n");
Var c(kBool_Type);
DSLExpression(+c);
DSLExpression(+c).release();
}
}
@ -728,7 +728,7 @@ DEF_GPUTEST_FOR_MOCK_CONTEXT(DSLMinus, r, ctxInfo) {
{
ExpectError error(r, "error: '-' cannot operate on 'bool'\n");
Var c(kBool_Type);
DSLExpression(-c);
DSLExpression(-c).release();
}
}
@ -1541,12 +1541,12 @@ DEF_GPUTEST_FOR_MOCK_CONTEXT(DSLSelect, r, ctxInfo) {
{
ExpectError error(r, "error: expected 'bool', but found 'int'\n");
DSLExpression x = Select(a, 1, -1);
Select(a, 1, -1).release();
}
{
ExpectError error(r, "error: ternary operator result mismatch: 'float2', 'float3'\n");
DSLExpression x = Select(a > 0, Float2(1), Float3(1));
Select(a > 0, Float2(1), Float3(1)).release();
}
}
@ -1669,7 +1669,7 @@ DEF_GPUTEST_FOR_MOCK_CONTEXT(DSLWhile, r, ctxInfo) {
{
ExpectError error(r, "error: expected 'bool', but found 'int'\n");
DSLStatement x = While(7, Block());
While(7, Block()).release();
}
}
@ -1682,17 +1682,17 @@ DEF_GPUTEST_FOR_MOCK_CONTEXT(DSLIndex, r, ctxInfo) {
{
ExpectError error(r, "error: expected 'int', but found 'bool'\n");
DSLExpression x = a[true];
a[true].release();
}
{
ExpectError error(r, "error: expected array, but found 'int'\n");
DSLExpression x = b[0];
b[0].release();
}
{
ExpectError error(r, "error: index -1 out of range for 'int[5]'\n");
DSLExpression x = a[-1];
a[-1].release();
}
}