Redo how extra emit code flushing operates
The previous implementation of flushEmittedCode(), that flushed on
demand when a process() was encountered, was brittle and susceptible to
mangling the expected sksl when fOut was modified outside of its
control. Given that writeFunction() and generateCode() in the parent
class all do this, it's possible to generate a simple SkSL snippet that
would generate a CPP file that builds invalid final SkSL:
```
in fragmentProcessor child;
bool someGlobalVar = ...;
void main() {
if (someGlobalVar) {
sk_OutColor = process(child, sk_InColor);
} else {
sk_OutColor = half4(1);
}
}
```
The CPP generated code *should* insert 'bool someGlobalVar' at the start
but because of the early flush from the child process and because of
how fOut was overwritten, someGlobalVar's declaration is put into a
stream that is not visible to the flush and ends up being inserted into
the output sksl in an incorrect location (namely after the if condition
that depends on it).
This CL updates the extra emitted code logic to support multiple blocks
of extra CPP code. When a flush point occurs in SkSL writing, a special
token is inserted into the SkSL and a new CPP code buffer is associated
with that token. Then once all of the SkSL is accumulated into the root
output stream, it is processed into sections for each extra CPP block.
Special logic is done so that the SkSL that is emitted before the next
CPP block terminates at the end of the last valid statement before the
special token.
A unit test demonstrating this failure condition is added to SkSLFPTest
and the CL properly passes. Since this bug did not trigger on existing
.fp files, the updated generator does not modify the generated FPs.
Bug: skia:
Change-Id: Ib74911942080f1b964159807a06805bc52898789
Reviewed-on: https://skia-review.googlesource.com/152321
Commit-Queue: Michael Ludwig <michaelludwig@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
diff --git a/src/sksl/SkSLCPPCodeGenerator.cpp b/src/sksl/SkSLCPPCodeGenerator.cpp
index 5f4665f..5c24610 100644
--- a/src/sksl/SkSLCPPCodeGenerator.cpp
+++ b/src/sksl/SkSLCPPCodeGenerator.cpp
@@ -97,9 +97,9 @@
String name = "sk_TransformedCoords2D_" + to_string(index);
fFormatArgs.push_back(name + ".c_str()");
if (fWrittenTransformedCoords.find(index) == fWrittenTransformedCoords.end()) {
- fExtraEmitCodeCode += " SkString " + name +
- " = fragBuilder->ensureCoords2D(args.fTransformedCoords[" +
- to_string(index) + "]);\n";
+ addExtraEmitCodeLine("SkString " + name +
+ " = fragBuilder->ensureCoords2D(args.fTransformedCoords[" +
+ to_string(index) + "]);");
fWrittenTransformedCoords.insert(index);
}
return;
@@ -361,23 +361,20 @@
}
SkASSERT(found);
- // Flush all previous statements so that this emitted child can
- // depend upon any declared variables within that section
- this->flushEmittedCode();
+ // Start a new extra emit code section so that the emitted child processor can depend on
+ // sksl variables defined in earlier sksl code.
+ this->newExtraEmitCodeBlock();
- // Set to the empty string when no input color parameter should be emitted,
- // which means this must be properly formatted with a prefixed comma
- // when the parameter should be inserted into the emitChild() parameter list.
+ // Set to the empty string when no input color parameter should be emitted, which means this
+ // must be properly formatted with a prefixed comma when the parameter should be inserted
+ // into the emitChild() parameter list.
String inputArg;
if (c.fArguments.size() > 1) {
SkASSERT(c.fArguments.size() == 2);
- // Use the emitChild() variant that accepts an input color, so convert
- // the 2nd argument's expression into C++ code that produces sksl
- // stored in an SkString.
-
+ // Use the emitChild() variant that accepts an input color, so convert the 2nd
+ // argument's expression into C++ code that produces sksl stored in an SkString.
String inputName = "_input" + to_string(index);
- fExtraEmitCodeCode += " " + convertSKSLExpressionToCPP(
- *c.fArguments[1], inputName);
+ addExtraEmitCodeLine(convertSKSLExpressionToCPP(*c.fArguments[1], inputName));
// emitChild() needs a char*
inputArg = ", " + inputName + ".c_str()";
@@ -385,9 +382,9 @@
// Write the output handling after the possible input handling
String childName = "_child" + to_string(index);
- fExtraEmitCodeCode += " SkString " + childName + "(\"" + childName + "\");\n";
- fExtraEmitCodeCode += " this->emitChild(" + to_string(index) + inputArg +
- ", &" + childName + ", args);\n";
+ addExtraEmitCodeLine("SkString " + childName + "(\"" + childName + "\");");
+ addExtraEmitCodeLine("this->emitChild(" + to_string(index) + inputArg +
+ ", &" + childName + ", args);");
this->write("%s");
fFormatArgs.push_back(childName + ".c_str()");
@@ -575,7 +572,26 @@
Type::kOther_Kind != var.fType.kind();
}
-void CPPCodeGenerator::flushEmittedCode(bool flushAll) {
+void CPPCodeGenerator::newExtraEmitCodeBlock() {
+ // This should only be called when emitting SKSL for emitCode(), which can be detected if the
+ // cpp buffer is not null, and the cpp buffer is not the current output.
+ SkASSERT(fCPPBuffer && fCPPBuffer != fOut);
+
+ // Start a new block as an empty string
+ fExtraEmitCodeBlocks.push_back("");
+ // Mark its location in the output buffer, uses ${\d} for the token since ${} will not occur in
+ // valid sksl and makes detection trivial.
+ this->writef("${%zu}", fExtraEmitCodeBlocks.size() - 1);
+}
+
+void CPPCodeGenerator::addExtraEmitCodeLine(const String& toAppend) {
+ SkASSERT(fExtraEmitCodeBlocks.size() > 0);
+ String& currentBlock = fExtraEmitCodeBlocks[fExtraEmitCodeBlocks.size() - 1];
+ // Automatically add indentation and newline
+ currentBlock += " " + toAppend + "\n";
+}
+
+void CPPCodeGenerator::flushEmittedCode() {
if (fCPPBuffer == nullptr) {
// Not actually within writeEmitCode() so nothing to flush
return;
@@ -584,41 +600,69 @@
StringStream* skslBuffer = static_cast<StringStream*>(fOut);
String sksl = skslBuffer->str();
- // Empty the accumulation buffer; if there are any partial statements in
- // the extracted sksl string they will be re-added later
+ // Empty the accumulation buffer since its current contents are consumed.
skslBuffer->reset();
- if (!flushAll) {
- // Find the last ';', '{', or '}' and leave everything after that in the buffer
- int lastStatementEnd = sksl.findLastOf(';');
- int lastBlockOpen = sksl.findLastOf('{');
- int lastBlockEnd = sksl.findLastOf('}');
-
- int flushPoint = std::max(lastStatementEnd, std::max(lastBlockEnd, lastBlockOpen));
-
- // NOTE: this does the right thing when flushPoint = -1
- if (flushPoint < (int) sksl.size() - 1) {
- // There is partial sksl content that can't be flushed yet so put
- // that back into the sksl buffer and remove it from the string
- skslBuffer->writeText(sksl.c_str() + flushPoint + 1);
- sksl = String(sksl.c_str(), flushPoint + 1);
- }
- }
-
- // Switch back to the CPP buffer for the actual code appending statements
+ // Switch to the cpp buffer
fOut = fCPPBuffer;
- if (fExtraEmitCodeCode.size() > 0) {
- this->writef("%s", fExtraEmitCodeCode.c_str());
- fExtraEmitCodeCode.reset();
+
+ // Iterate through the sksl, keeping track of where the last statement ended (e.g. the latest
+ // encountered ';', '{', or '}'). If an extra emit code block token is encountered then the
+ // code from 0 to last statement end is sent to writeCodeAppend, the extra code block is
+ // appended to the cpp buffer, and then the sksl string is trimmed to start where the last
+ // statement left off (minus the encountered token).
+ size_t i = 0;
+ size_t flushPoint = -1;
+ int tokenStart = -1;
+ while (i < sksl.size()) {
+ if (tokenStart >= 0) {
+ // Looking for the end of the token
+ if (sksl[i] == '}') {
+ // Must append the sksl from 0 to flushPoint (inclusive) then the extra code
+ // accumulated in the block with index parsed from chars [tokenStart+2, i-1]
+ String toFlush = String(sksl.c_str(), flushPoint + 1);
+ // writeCodeAppend automatically removes the format args that it consumed, so
+ // fFormatArgs will be in a valid state for any future sksl
+ this->writeCodeAppend(toFlush);
+
+ int codeBlock = stoi(String(sksl.c_str() + tokenStart + 2, i - tokenStart - 2));
+ SkASSERT(codeBlock < (int) fExtraEmitCodeBlocks.size());
+ if (fExtraEmitCodeBlocks[codeBlock].size() > 0) {
+ this->write(fExtraEmitCodeBlocks[codeBlock].c_str());
+ }
+
+ // Now reset the sksl buffer to start after the flush point, but remove the token.
+ String compacted = String(sksl.c_str() + flushPoint + 1,
+ tokenStart - flushPoint - 1);
+ if (i < sksl.size() - 1) {
+ compacted += String(sksl.c_str() + i + 1, sksl.size() - i - 1);
+ }
+ sksl = compacted;
+
+ // And reset iteration
+ i = -1;
+ flushPoint = -1;
+ tokenStart = -1;
+ }
+ } else {
+ // Looking for the start of extra emit block tokens, and tracking when statements end
+ if (sksl[i] == ';' || sksl[i] == '{' || sksl[i] == '}') {
+ flushPoint = i;
+ } else if (i < sksl.size() - 1 && sksl[i] == '$' && sksl[i + 1] == '{') {
+ // found an extra emit code block token
+ tokenStart = i++;
+ }
+ }
+ i++;
}
- // writeCodeAppend automatically removes the format args that it consumed,
- // so fFormatArgs will be in a valid state for any partial statements left
- // in the sksl buffer.
+
+ // Once we've gone through the sksl string to this point, there are no remaining extra emit
+ // code blocks to interleave, so append the remainder as usual.
this->writeCodeAppend(sksl);
- // After appending, switch back to an sksl buffer that contains any
- // remaining partial statements that couldn't be appended
+ // After appending, switch back to the emptied sksl buffer and reset the extra code blocks
fOut = skslBuffer;
+ fExtraEmitCodeBlocks.clear();
}
void CPPCodeGenerator::writeCodeAppend(const String& code) {
@@ -679,18 +723,39 @@
// Convert the argument expression into a format string and args
this->writeExpression(e, Precedence::kTopLevel_Precedence);
std::vector<String> newArgs(fFormatArgs);
- String exprFormat = exprBuffer.str();
+ String expr = exprBuffer.str();
// After generating, restore the original output stream and format args
fFormatArgs = oldArgs;
fOut = oldSKSL;
+ // The sksl written to exprBuffer is not processed by flushEmittedCode(), so any extra emit code
+ // block tokens won't get handled. So we need to strip them from the expression and stick them
+ // to the end of the original sksl stream.
+ String exprFormat = "";
+ int tokenStart = -1;
+ for (size_t i = 0; i < expr.size(); i++) {
+ if (tokenStart >= 0) {
+ if (expr[i] == '}') {
+ // End of the token, so append the token to fOut
+ fOut->write(expr.c_str() + tokenStart, i - tokenStart + 1);
+ tokenStart = -1;
+ }
+ } else {
+ if (i < expr.size() - 1 && expr[i] == '$' && expr[i + 1] == '{') {
+ tokenStart = i++;
+ } else {
+ exprFormat += expr[i];
+ }
+ }
+ }
+
// Now build the final C++ code snippet from the format string and args
String cppExpr;
if (newArgs.size() == 0) {
// This was a static expression, so we can simplify the input
// color declaration in the emitted code to just a static string
- cppExpr = "SkString " + cppVar + "(\"" + exprFormat + "\");\n";
+ cppExpr = "SkString " + cppVar + "(\"" + exprFormat + "\");";
} else {
// String formatting must occur dynamically, so have the C++ declaration
// use SkStringPrintf with the format args that were accumulated
@@ -699,7 +764,7 @@
for (size_t i = 0; i < newArgs.size(); i++) {
cppExpr += ", " + newArgs[i];
}
- cppExpr += ");\n";
+ cppExpr += ");";
}
return cppExpr;
}
@@ -737,9 +802,9 @@
StringStream skslBuffer;
fOut = &skslBuffer;
+ this->newExtraEmitCodeBlock();
bool result = INHERITED::generateCode();
- // Final flush in case there is anything extra, forcing it to emit everything
- this->flushEmittedCode(true);
+ this->flushEmittedCode();
// Then restore the original CPP buffer and close the function
fOut = fCPPBuffer;
diff --git a/src/sksl/SkSLCPPCodeGenerator.h b/src/sksl/SkSLCPPCodeGenerator.h
index 6c67bad..4b62fc6 100644
--- a/src/sksl/SkSLCPPCodeGenerator.h
+++ b/src/sksl/SkSLCPPCodeGenerator.h
@@ -23,21 +23,6 @@
bool generateCode() override;
private:
- // When inside writeEmitCode(), certain SkSL elements need to control
- // when fragBuilder->codeAppendf is added to the function block. This
- // takes all completed statements in the SkSL buffer, and their corresponding
- // format args, and writes them into the emitCode()'s statement block
- // using writeCodeAppend().
- //
- // This control is necessary for handling special functions in SkSL, like
- // process(), which need to intermix the current FP's SkSL with that of
- // an emitted child.
- //
- // :forceAll - If false, only the completed statements (terminated by ;),
- // will be flushed and the sksl buffer will be set to any partial
- // statements that remain. If true, everything is flushed, regardless.
- void flushEmittedCode(bool forceAll = false);
-
void writef(const char* s, va_list va) SKSL_PRINTF_LIKE(2, 0);
void writef(const char* s, ...) SKSL_PRINTF_LIKE(2, 3);
@@ -103,19 +88,36 @@
void writeTest();
- // If the returned C++ is included in the generated code, then the variable
- // name stored in cppVar will refer to a valid SkString that matches the
- // Expression. Successful returns leave the output buffer (and related state)
- // unmodified.
+ // If the returned C++ is included in the generated code, then the variable name stored in
+ // cppVar will refer to a valid SkString that matches the Expression. Successful returns leave
+ // the output buffer (and related state) unmodified.
//
- // In the simplest cases, this will return "SkString {cppVar}(\"{e}\");",
- // while more advanced cases will properly insert format arguments.
+ // In the simplest cases, this will return "SkString {cppVar}(\"{e}\");", while more advanced
+ // cases will properly insert format arguments.
String convertSKSLExpressionToCPP(const Expression& e, const String& cppVar);
+ // Process accumulated sksl to split it into appended code sections, properly interleaved with
+ // the extra emit code blocks, based on statement/block locations and the inserted tokens
+ // from newExtraEmitCodeBlock(). It is necessary to split the sksl after the program has been
+ // fully walked since many elements redirect fOut to simultaneously build header sections and
+ // bodies that are then concatenated; due to this it is not possible to split the sksl emission
+ // on the fly.
+ void flushEmittedCode();
+
+ // Start a new extra emit code block for accumulating C++ code. This will insert a token into
+ // the sksl stream to mark the fence between previous complete sksl statements and where the
+ // C++ code added to the new block will be added to emitCode(). These tokens are removed by
+ // flushEmittedCode() as it consumes them before passing pure sksl to writeCodeAppend().
+ void newExtraEmitCodeBlock();
+
+ // Append CPP code to the current extra emit code block.
+ void addExtraEmitCodeLine(const String& toAppend);
+
String fName;
String fFullName;
SectionAndParameterHelper fSectionAndParameterHelper;
- String fExtraEmitCodeCode;
+ std::vector<String> fExtraEmitCodeBlocks;
+
std::vector<String> fFormatArgs;
std::set<int> fWrittenTransformedCoords;
// if true, we are writing a C++ expression instead of a GLSL expression
diff --git a/tests/SkSLFPTest.cpp b/tests/SkSLFPTest.cpp
index 510b51f..4602bf8 100644
--- a/tests/SkSLFPTest.cpp
+++ b/tests/SkSLFPTest.cpp
@@ -545,3 +545,31 @@
"this->registerChildProcessor(src.childProcessor(1).clone());"
});
}
+
+DEF_TEST(SkSLFPChildFPAndGlobal, r) {
+ test(r,
+ "in fragmentProcessor child;"
+ "bool hasCap = sk_Caps.externalTextureSupport;"
+ "void main() {"
+ " if (hasCap) {"
+ " sk_OutColor = process(child, sk_InColor);"
+ " } else {"
+ " sk_OutColor = half4(1);"
+ " }"
+ "}",
+ *SkSL::ShaderCapsFactory::Default(),
+ {
+ "this->registerChildProcessor(std::move(child));"
+ },
+ {
+ "hasCap = sk_Caps.externalTextureSupport;",
+ "fragBuilder->codeAppendf(\"bool hasCap = %s;\\nif (hasCap) {\", "
+ "(hasCap ? \"true\" : \"false\"));",
+ "SkString _input0 = SkStringPrintf(\"%s\", args.fInputColor);",
+ "SkString _child0(\"_child0\");",
+ "this->emitChild(0, _input0.c_str(), &_child0, args);",
+ "fragBuilder->codeAppendf(\"\\n %s = %s;\\n} else {\\n %s = half4(1.0);\\n}"
+ "\\n\", args.fOutputColor, _child0.c_str(), args.fOutputColor);",
+ "this->registerChildProcessor(src.childProcessor(0).clone());"
+ });
+}