Reland "Redo how extra emit code flushing operates" with type fix.
This reverts commit d0440195d5cc049062238d02b8a962ae49c7f4ff.
Reason for revert: Fixes size_t -> int that was triggering ASAN failures.
Original change's description:
> Revert "Redo how extra emit code flushing operates"
>
> This reverts commit 9b8181b05a84e7dd24234c46c87d0bb2c73a7c08.
>
> Reason for revert: <INSERT REASONING HERE>
>
> Original change's description:
> > 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>
>
> TBR=ethannicholas@google.com,michaelludwig@google.com
>
> Change-Id: Id0f908453b596873f43b86a1c14eed48b2474a76
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Bug: skia:
> Reviewed-on: https://skia-review.googlesource.com/152660
> Reviewed-by: Michael Ludwig <michaelludwig@google.com>
> Commit-Queue: Michael Ludwig <michaelludwig@google.com>
TBR=ethannicholas@google.com,michaelludwig@google.com
Change-Id: I3ccf2fee6ef96c6102dbe1c2c2ef6c14a701b8fd
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: skia:
Reviewed-on: https://skia-review.googlesource.com/152663
Commit-Queue: Michael Ludwig <michaelludwig@google.com>
Reviewed-by: Michael Ludwig <michaelludwig@google.com>
diff --git a/src/sksl/SkSLCPPCodeGenerator.cpp b/src/sksl/SkSLCPPCodeGenerator.cpp
index 5f4665f..88c7ed3 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;
+ int 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