Remove 'private' symbol table from the compiler.
Previously, the root symbol table held universally-available symbols
and the private symbol table was a child of root which held symbols that
aren't accessible to public (runtime-shader) programs.
Now, the root symbol table holds both private and public symbols. The
public symbol table overrides the private symbols to the SkSL built-in
"Invalid" type. We were already doing something very similar to this
anyway, so that those names couldn't be used in public code.
This simplifies our symbol table inheritance structure and should make
it easier to move shared symbols into a common ancestor module.
Change-Id: I41cfe3c9bf2f4ebdf1a26eb96c710c2e6bdc13ec
Bug: skia:13540
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/562301
Reviewed-by: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
diff --git a/src/sksl/SkSLCompiler.cpp b/src/sksl/SkSLCompiler.cpp
index 2d2c1a8..02c64b6 100644
--- a/src/sksl/SkSLCompiler.cpp
+++ b/src/sksl/SkSLCompiler.cpp
@@ -155,7 +155,6 @@
, fInliner(fContext.get()) {
SkASSERT(caps);
fRootModule.fSymbols = this->makeRootSymbolTable();
- fPrivateModule.fSymbols = this->makePrivateSymbolTable(fRootModule.fSymbols);
}
Compiler::~Compiler() {}
@@ -216,38 +215,32 @@
#undef TYPE
-std::shared_ptr<SymbolTable> Compiler::makeRootSymbolTable() const {
+std::shared_ptr<SymbolTable> Compiler::makeRootSymbolTable() {
auto rootSymbolTable = std::make_shared<SymbolTable>(*fContext, /*builtin=*/true);
for (BuiltinTypePtr rootType : kRootTypes) {
rootSymbolTable->addWithoutOwnership((fContext->fTypes.*rootType).get());
}
- return rootSymbolTable;
-}
-
-std::shared_ptr<SymbolTable> Compiler::makePrivateSymbolTable(std::shared_ptr<SymbolTable> parent) {
- auto privateSymbolTable = std::make_shared<SymbolTable>(parent, /*builtin=*/true);
-
for (BuiltinTypePtr privateType : kPrivateTypes) {
- privateSymbolTable->addWithoutOwnership((fContext->fTypes.*privateType).get());
+ rootSymbolTable->addWithoutOwnership((fContext->fTypes.*privateType).get());
}
// sk_Caps is "builtin", but all references to it are resolved to Settings, so we don't need to
// treat it as builtin (ie, no need to clone it into the Program).
- privateSymbolTable->add(std::make_unique<Variable>(/*pos=*/Position(),
- /*modifiersPosition=*/Position(),
- fCoreModifiers.add(Modifiers{}),
- "sk_Caps",
- fContext->fTypes.fSkCaps.get(),
- /*builtin=*/false,
- Variable::Storage::kGlobal));
- return privateSymbolTable;
+ rootSymbolTable->add(std::make_unique<Variable>(/*pos=*/Position(),
+ /*modifiersPosition=*/Position(),
+ fCoreModifiers.add(Modifiers{}),
+ "sk_Caps",
+ fContext->fTypes.fSkCaps.get(),
+ /*builtin=*/false,
+ Variable::Storage::kGlobal));
+ return rootSymbolTable;
}
const ParsedModule& Compiler::loadGPUModule() {
if (!fGPUModule.fSymbols) {
- fGPUModule = this->parseModule(ProgramKind::kFragment, MODULE_DATA(gpu), fPrivateModule);
+ fGPUModule = this->parseModule(ProgramKind::kFragment, MODULE_DATA(gpu), fRootModule);
}
return fGPUModule;
}
@@ -302,7 +295,7 @@
#endif
}
-static void add_glsl_type_aliases(SkSL::SymbolTable* symbols, const SkSL::BuiltinTypes& types) {
+static void add_public_type_aliases(SkSL::SymbolTable* symbols, const SkSL::BuiltinTypes& types) {
// Add some aliases to the runtime effect modules so that it's friendlier, and more like GLSL.
symbols->addWithoutOwnership(types.fVec2.get());
symbols->addWithoutOwnership(types.fVec3.get());
@@ -330,23 +323,24 @@
symbols->addWithoutOwnership(types.fMat4x3.get());
symbols->addWithoutOwnership(types.fMat4x4.get());
- // Alias every private type to "invalid". This will prevent code from using built-in names like
- // `sampler2D` as variable names.
+ // Hide all the private symbols by aliasing them all to "invalid". This will prevent code from
+ // using built-in names like `sampler2D` as variable names.
for (BuiltinTypePtr privateType : kPrivateTypes) {
symbols->add(Type::MakeAliasType((types.*privateType)->name(), *types.fInvalid));
}
+ symbols->add(Type::MakeAliasType("sk_Caps", *types.fInvalid));
}
-std::shared_ptr<SymbolTable> Compiler::makeGLSLRootSymbolTable() const {
+std::shared_ptr<SymbolTable> Compiler::makeRootSymbolTableWithPublicTypes() {
auto result = this->makeRootSymbolTable();
- add_glsl_type_aliases(result.get(), fContext->fTypes);
+ add_public_type_aliases(result.get(), fContext->fTypes);
return result;
}
const ParsedModule& Compiler::loadPublicModule() {
if (!fPublicModule.fSymbols) {
fPublicModule = this->parseModule(ProgramKind::kGeneric, MODULE_DATA(public), fRootModule);
- add_glsl_type_aliases(fPublicModule.fSymbols.get(), fContext->fTypes);
+ add_public_type_aliases(fPublicModule.fSymbols.get(), fContext->fTypes);
}
return fPublicModule;
}
@@ -382,13 +376,11 @@
std::shared_ptr<SymbolTable> base,
bool dehydrate) {
if (dehydrate) {
- // NOTE: This is a workaround. When dehydrating includes, skslc doesn't know which module
- // it's preparing, nor what the correct base module is. We can't use 'Root', because many
- // GPU intrinsics reference private types, like samplers or textures. Today, 'Private' does
- // contain the union of all known types, so this is safe. If we ever have types that only
- // exist in 'Public' (for example), this logic needs to be smarter (by choosing the correct
- // base for the module we're compiling).
- base = fPrivateModule.fSymbols;
+ // sksl-precompile passes `true` when dehydrating the lowest-level module to indicate that
+ // we should use the root module. Child modules that depend on the earlier module will pass
+ // `false` and pass the lower-level module's symbol table in `base`.
+ SkASSERT(base == nullptr);
+ base = fRootModule.fSymbols;
}
SkASSERT(base);
diff --git a/src/sksl/SkSLCompiler.h b/src/sksl/SkSLCompiler.h
index 9080281..0a83f9a 100644
--- a/src/sksl/SkSLCompiler.h
+++ b/src/sksl/SkSLCompiler.h
@@ -245,9 +245,8 @@
const ParsedModule& loadPublicModule();
const ParsedModule& loadPrivateRTShaderModule();
- std::shared_ptr<SymbolTable> makeRootSymbolTable() const;
- std::shared_ptr<SymbolTable> makeGLSLRootSymbolTable() const;
- std::shared_ptr<SymbolTable> makePrivateSymbolTable(std::shared_ptr<SymbolTable> parent);
+ std::shared_ptr<SymbolTable> makeRootSymbolTable();
+ std::shared_ptr<SymbolTable> makeRootSymbolTableWithPublicTypes();
/** Optimize every function in the program. */
bool optimize(Program& program);
diff --git a/src/sksl/SkSLRehydrator.cpp b/src/sksl/SkSLRehydrator.cpp
index 16926f6..2c85003 100644
--- a/src/sksl/SkSLRehydrator.cpp
+++ b/src/sksl/SkSLRehydrator.cpp
@@ -98,10 +98,13 @@
std::shared_ptr<SymbolTable> fOldSymbols;
};
+Rehydrator::Rehydrator(Compiler& compiler, const uint8_t* src, size_t length)
+ : Rehydrator(compiler, src, length, compiler.makeRootSymbolTableWithPublicTypes()) {}
+
Rehydrator::Rehydrator(Compiler& compiler, const uint8_t* src, size_t length,
- std::shared_ptr<SymbolTable> symbols)
+ std::shared_ptr<SymbolTable> symbols)
: fCompiler(compiler)
- , fSymbolTable(symbols ? std::move(symbols) : compiler.makeGLSLRootSymbolTable())
+ , fSymbolTable(std::move(symbols))
SkDEBUGCODE(, fEnd(src + length)) {
SkASSERT(fSymbolTable);
SkASSERT(fSymbolTable->isBuiltin());
diff --git a/src/sksl/SkSLRehydrator.h b/src/sksl/SkSLRehydrator.h
index 9b952cf..189ddf7 100644
--- a/src/sksl/SkSLRehydrator.h
+++ b/src/sksl/SkSLRehydrator.h
@@ -107,8 +107,10 @@
};
// src must remain in memory as long as the objects created from it do
+ Rehydrator(Compiler& compiler, const uint8_t* src, size_t length);
+
Rehydrator(Compiler& compiler, const uint8_t* src, size_t length,
- std::shared_ptr<SymbolTable> base = nullptr);
+ std::shared_ptr<SymbolTable> base);
#ifdef SK_DEBUG
~Rehydrator();
diff --git a/tests/SkRuntimeEffectTest.cpp b/tests/SkRuntimeEffectTest.cpp
index 6cb9618..655111c 100644
--- a/tests/SkRuntimeEffectTest.cpp
+++ b/tests/SkRuntimeEffectTest.cpp
@@ -66,7 +66,7 @@
test_invalid_effect(
r,
"half4 main(float2 p) { return sk_Caps.floatIs32Bits ? half4(1) : half4(0); }",
- "unknown identifier 'sk_Caps'");
+ "type '<INVALID>' does not have a field named 'floatIs32Bits'");
}
DEF_TEST(SkRuntimeEffect_DeadCodeEliminationStackOverflow, r) {
diff --git a/tests/SkSLTest.cpp b/tests/SkSLTest.cpp
index 3405107..0ff3bf8 100644
--- a/tests/SkSLTest.cpp
+++ b/tests/SkSLTest.cpp
@@ -353,8 +353,9 @@
SkSL::StringStream stream;
dehydrator.finish(stream);
- SkSL::Rehydrator rehydrator(compiler, (const uint8_t*) stream.str().data(),
- stream.str().length());
+ SkSL::Rehydrator rehydrator(compiler,
+ reinterpret_cast<const uint8_t*>(stream.str().data()),
+ stream.str().length());
std::unique_ptr<SkSL::Program> rehydrated = rehydrator.program();
REPORTER_ASSERT(r, rehydrated->description() == program->description(),
"Mismatch between original and dehydrated/rehydrated:\n-- Original:\n%s\n"
