Remove unsafe compiler methods related to external values
We don't want to be polluting the global namespace with external values,
especially when the typical/recommended way to use the Compiler is with
a single long-lived instance. Force client code to manage ownership (the
only non-unit-test case was already doing this), and pass external
values to convertProgram, so they can be added to the Program's symbol
table.
Change-Id: If4c1db5e48a62e2cf4333b8d80420f2dfede27ab
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/319125
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
diff --git a/modules/particles/include/SkParticleEffect.h b/modules/particles/include/SkParticleEffect.h
index 1780c37..4a474aa 100644
--- a/modules/particles/include/SkParticleEffect.h
+++ b/modules/particles/include/SkParticleEffect.h
@@ -17,12 +17,12 @@
#include "modules/particles/include/SkParticleData.h"
#include <memory>
+#include <vector>
class SkCanvas;
class SkFieldVisitor;
class SkParticleBinding;
class SkParticleDrawable;
-class SkParticleExternalValue;
namespace skresources {
class ResourceProvider;
@@ -30,6 +30,7 @@
namespace SkSL {
class ByteCode;
+ class ExternalValue;
} // namespace SkSL
class SkParticleEffectParams : public SkRefCnt {
@@ -129,7 +130,7 @@
// Cached
struct Program {
std::unique_ptr<SkSL::ByteCode> fByteCode;
- SkTArray<std::unique_ptr<SkParticleExternalValue>> fExternalValues;
+ std::vector<std::unique_ptr<SkSL::ExternalValue>> fExternalValues;
};
Program fEffectProgram;
diff --git a/modules/particles/src/SkParticleEffect.cpp b/modules/particles/src/SkParticleEffect.cpp
index 55c1bba..b754519 100644
--- a/modules/particles/src/SkParticleEffect.cpp
+++ b/modules/particles/src/SkParticleEffect.cpp
@@ -123,17 +123,17 @@
SkSL::Program::Settings settings;
settings.fRemoveDeadFunctions = false;
- SkTArray<std::unique_ptr<SkParticleExternalValue>> externalValues;
+ std::vector<std::unique_ptr<SkSL::ExternalValue>> externalValues;
+ externalValues.reserve(fBindings.size());
for (const auto& binding : fBindings) {
if (binding) {
- auto value = binding->toValue(compiler);
- compiler.registerExternalValue(value.get());
- externalValues.push_back(std::move(value));
+ externalValues.push_back(binding->toValue(compiler));
}
}
- auto program = compiler.convertProgram(SkSL::Program::kGeneric_Kind, code, settings);
+ auto program = compiler.convertProgram(SkSL::Program::kGeneric_Kind, code, settings,
+ &externalValues);
if (!program) {
SkDebugf("%s\n", compiler.errorText().c_str());
return;
@@ -225,7 +225,7 @@
if (const auto& byteCode = fParams->fEffectProgram.fByteCode) {
if (auto fun = byteCode->getFunction(entry)) {
for (const auto& value : fParams->fEffectProgram.fExternalValues) {
- value->setEffect(this);
+ static_cast<SkParticleExternalValue*>(value.get())->setEffect(this);
}
SkAssertResult(byteCode->run(fun, &fState.fAge, sizeof(EffectState) / sizeof(float),
nullptr, 0,
@@ -269,7 +269,7 @@
args[i] = fParticles.fData[i].get() + start;
}
for (const auto& value : fParams->fParticleProgram.fExternalValues) {
- value->setEffect(this);
+ static_cast<SkParticleExternalValue*>(value.get())->setEffect(this);
}
memcpy(&fParticleUniforms[1], &fState.fAge, sizeof(EffectState));
SkAssertResult(byteCode->runStriped(fun, count, args, SkParticles::kNumChannels,
diff --git a/src/sksl/SkSLCompiler.cpp b/src/sksl/SkSLCompiler.cpp
index f38da67..ffe6811 100644
--- a/src/sksl/SkSLCompiler.cpp
+++ b/src/sksl/SkSLCompiler.cpp
@@ -1556,16 +1556,13 @@
return madeChanges;
}
-void Compiler::registerExternalValue(ExternalValue* value) {
- fRootSymbolTable->addWithoutOwnership(value->fName, value);
-}
+std::unique_ptr<Program> Compiler::convertProgram(
+ Program::Kind kind,
+ String text,
+ const Program::Settings& settings,
+ const std::vector<std::unique_ptr<ExternalValue>>* externalValues) {
+ SkASSERT(!externalValues || (kind == Program::kGeneric_Kind));
-const Symbol* Compiler::takeOwnership(std::unique_ptr<const Symbol> symbol) {
- return fRootSymbolTable->takeOwnershipOfSymbol(std::move(symbol));
-}
-
-std::unique_ptr<Program> Compiler::convertProgram(Program::Kind kind, String text,
- const Program::Settings& settings) {
fErrorText = "";
fErrorCount = 0;
fInliner.reset(context(), settings);
@@ -1636,6 +1633,13 @@
fIRGenerator->start(&settings, fInterpreterSymbolTable, /*inherited=*/nullptr);
break;
}
+ if (externalValues) {
+ // Add any external values to the symbol table. IRGenerator::start() has pushed a table, so
+ // we're only making these visible to the current Program.
+ for (const auto& ev : *externalValues) {
+ fIRGenerator->fSymbolTable->addWithoutOwnership(ev->fName, ev.get());
+ }
+ }
std::unique_ptr<String> textPtr(new String(std::move(text)));
fSource = textPtr.get();
fIRGenerator->convertProgram(kind, textPtr->c_str(), textPtr->size(), &elements);
diff --git a/src/sksl/SkSLCompiler.h b/src/sksl/SkSLCompiler.h
index 6bd1423..d3915f4 100644
--- a/src/sksl/SkSLCompiler.h
+++ b/src/sksl/SkSLCompiler.h
@@ -117,12 +117,14 @@
Compiler& operator=(const Compiler&) = delete;
/**
- * Registers an ExternalValue as a top-level symbol which is visible in the global namespace.
+ * If externalValues is supplied, those values are registered in the symbol table of the
+ * Program, but ownership is *not* transferred. It is up to the caller to keep them alive.
*/
- void registerExternalValue(ExternalValue* value);
-
- std::unique_ptr<Program> convertProgram(Program::Kind kind, String text,
- const Program::Settings& settings);
+ std::unique_ptr<Program> convertProgram(
+ Program::Kind kind,
+ String text,
+ const Program::Settings& settings,
+ const std::vector<std::unique_ptr<ExternalValue>>* externalValues = nullptr);
bool toSPIRV(Program& program, OutputStream& out);
@@ -150,11 +152,6 @@
bool toPipelineStage(Program& program, PipelineStageArgs* outArgs);
#endif
- /**
- * Takes ownership of the given symbol. It will be destroyed when the compiler is destroyed.
- */
- const Symbol* takeOwnership(std::unique_ptr<const Symbol> symbol);
-
void error(int offset, String msg) override;
String errorText();
diff --git a/src/sksl/SkSLExternalValue.h b/src/sksl/SkSLExternalValue.h
index 1de6ab7..f5aeafa 100644
--- a/src/sksl/SkSLExternalValue.h
+++ b/src/sksl/SkSLExternalValue.h
@@ -93,9 +93,7 @@
/**
* Resolves 'name' within this context and returns an ExternalValue which represents it, or
- * null if no such child exists. If the implementation of this method creates new
- * ExternalValues and there isn't a more convenient place for ownership of the objects to
- * reside, the compiler's takeOwnership method may be useful.
+ * null if no such child exists.
*
* The 'name' string may not persist after this call; do not store this pointer.
*/
diff --git a/tests/SkSLInterpreterTest.cpp b/tests/SkSLInterpreterTest.cpp
index 13573de..a233ab1 100644
--- a/tests/SkSLInterpreterTest.cpp
+++ b/tests/SkSLInterpreterTest.cpp
@@ -956,8 +956,8 @@
SkSL::ExternalValue* getChild(const char* name) const override {
if (fValue.getType() == skjson::Value::Type::kObject) {
const skjson::Value& v = fValue.as<skjson::ObjectValue>()[name];
- return (SkSL::ExternalValue*) fCompiler.takeOwnership(std::unique_ptr<Symbol>(
- new JSONExternalValue(name, &v, &fCompiler)));
+ fOwned.push_back(std::make_unique<JSONExternalValue>(name, &v, &fCompiler));
+ return fOwned.back().get();
}
return nullptr;
}
@@ -965,6 +965,7 @@
private:
const skjson::Value& fValue;
SkSL::Compiler& fCompiler;
+ mutable std::vector<std::unique_ptr<SkSL::ExternalValue>> fOwned;
using INHERITED = SkSL::ExternalValue;
};
@@ -1009,17 +1010,13 @@
" outValue = 152;"
" return root.child.value2 ? root.value1 * root.child.value3 : -1;"
"}";
- compiler.registerExternalValue((SkSL::ExternalValue*) compiler.takeOwnership(
- std::unique_ptr<SkSL::Symbol>(new JSONExternalValue("root", &dom.root(), &compiler))));
int32_t outValue = -1;
- compiler.registerExternalValue((SkSL::ExternalValue*) compiler.takeOwnership(
- std::unique_ptr<SkSL::Symbol>(new PointerExternalValue("outValue",
- *compiler.context().fInt_Type,
- &outValue,
- sizeof(outValue)))));
+ std::vector<std::unique_ptr<SkSL::ExternalValue>> externalValues;
+ externalValues.push_back(std::make_unique<JSONExternalValue>("root", &dom.root(), &compiler));
+ externalValues.push_back(std::make_unique<PointerExternalValue>(
+ "outValue", *compiler.context().fInt_Type, &outValue, sizeof(outValue)));
std::unique_ptr<SkSL::Program> program = compiler.convertProgram(
- SkSL::Program::kGeneric_Kind,
- SkSL::String(src), settings);
+ SkSL::Program::kGeneric_Kind, SkSL::String(src), settings, &externalValues);
REPORTER_ASSERT(r, program);
if (program) {
std::unique_ptr<SkSL::ByteCode> byteCode = compiler.toByteCode(*program);
@@ -1045,14 +1042,11 @@
" value *= 2;"
"}";
int32_t value[4] = { 1, 2, 3, 4 };
- compiler.registerExternalValue((SkSL::ExternalValue*) compiler.takeOwnership(
- std::unique_ptr<SkSL::Symbol>(new PointerExternalValue("value",
- *compiler.context().fInt4_Type,
- value,
- sizeof(value)))));
- std::unique_ptr<SkSL::Program> program = compiler.convertProgram(SkSL::Program::kGeneric_Kind,
- SkSL::String(src),
- settings);
+ std::vector<std::unique_ptr<SkSL::ExternalValue>> externalValues;
+ externalValues.push_back(std::make_unique<PointerExternalValue>(
+ "value", *compiler.context().fInt4_Type, value, sizeof(value)));
+ std::unique_ptr<SkSL::Program> program = compiler.convertProgram(
+ SkSL::Program::kGeneric_Kind, SkSL::String(src), settings, &externalValues);
REPORTER_ASSERT(r, program);
if (program) {
std::unique_ptr<SkSL::ByteCode> byteCode = compiler.toByteCode(*program);
@@ -1109,15 +1103,11 @@
const char* src = "float main() {"
" return external(25);"
"}";
- compiler.registerExternalValue((SkSL::ExternalValue*) compiler.takeOwnership(
- std::unique_ptr<SkSL::Symbol>(new FunctionExternalValue("external",
- [] (float x) {
- return (float) sqrt(x);
- },
- compiler))));
- std::unique_ptr<SkSL::Program> program = compiler.convertProgram(SkSL::Program::kGeneric_Kind,
- SkSL::String(src),
- settings);
+ std::vector<std::unique_ptr<SkSL::ExternalValue>> externalValues;
+ externalValues.push_back(std::make_unique<FunctionExternalValue>(
+ "external", [](float x) { return (float)sqrt(x); }, compiler));
+ std::unique_ptr<SkSL::Program> program = compiler.convertProgram(
+ SkSL::Program::kGeneric_Kind, SkSL::String(src), settings, &externalValues);
REPORTER_ASSERT(r, program);
if (program) {
std::unique_ptr<SkSL::ByteCode> byteCode = compiler.toByteCode(*program);
@@ -1171,21 +1161,22 @@
DEF_TEST(SkSLInterpreterExternalValuesVectorCall, r) {
SkSL::Compiler compiler;
SkSL::Program::Settings settings;
- const char* src = "float4 main() {"
- " return external(float4(1, 4, 9, 16));"
- "}";
- compiler.registerExternalValue((SkSL::ExternalValue*) compiler.takeOwnership(
- std::unique_ptr<SkSL::Symbol>(new VectorFunctionExternalValue("external",
- [] (float in[4], float out[4]) {
- out[0] = sqrt(in[0]);
- out[1] = sqrt(in[1]);
- out[2] = sqrt(in[2]);
- out[3] = sqrt(in[3]);
- },
- compiler))));
- std::unique_ptr<SkSL::Program> program = compiler.convertProgram(SkSL::Program::kGeneric_Kind,
- SkSL::String(src),
- settings);
+ const char* src =
+ "float4 main() {"
+ " return external(float4(1, 4, 9, 16));"
+ "}";
+ std::vector<std::unique_ptr<SkSL::ExternalValue>> externalValues;
+ externalValues.push_back(std::make_unique<VectorFunctionExternalValue>(
+ "external",
+ [](float in[4], float out[4]) {
+ out[0] = sqrt(in[0]);
+ out[1] = sqrt(in[1]);
+ out[2] = sqrt(in[2]);
+ out[3] = sqrt(in[3]);
+ },
+ compiler));
+ std::unique_ptr<SkSL::Program> program = compiler.convertProgram(
+ SkSL::Program::kGeneric_Kind, SkSL::String(src), settings, &externalValues);
REPORTER_ASSERT(r, program);
if (program) {
std::unique_ptr<SkSL::ByteCode> byteCode = compiler.toByteCode(*program);
