Revert "Interpreter: Bounds check array access, add bool return from run"
This reverts commit f42de9e1e535af7e1700e4758d22751ff6a4982e.
Reason for revert: All the SANs
Original change's description:
> Interpreter: Bounds check array access, add bool return from run
>
> Out of bounds access with constant indices is a compile error.
> At runtime, causes the interpreter to fail. Made several other
> conditions trigger the same failure logic, and updated all
> uses of the interpreter to validate success.
>
> Change-Id: I3720b3c83903220b010ec574121fc64dbe102378
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/228256
> Commit-Queue: Brian Osman <brianosman@google.com>
> Reviewed-by: Mike Reed <reed@google.com>
TBR=mtklein@google.com,brianosman@google.com,ethannicholas@google.com,reed@google.com
Change-Id: I434601960d54fbd7d00e2af2dc6269a83a768c5b
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/228352
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
diff --git a/src/sksl/SkSLByteCode.cpp b/src/sksl/SkSLByteCode.cpp
index 91d7abb..4e5a653 100644
--- a/src/sksl/SkSLByteCode.cpp
+++ b/src/sksl/SkSLByteCode.cpp
@@ -59,7 +59,6 @@
printf("callexternal %d, %d, %d", argumentCount, returnCount, externalValue);
break;
}
- case ByteCodeInstruction::kClampIndex: printf("clampindex %d", READ8()); break;
VECTOR_DISASSEMBLE(kCompareIEQ, "compareieq")
VECTOR_DISASSEMBLE(kCompareINEQ, "compareineq")
VECTOR_MATRIX_DISASSEMBLE(kCompareFEQ, "comparefeq")
@@ -352,7 +351,7 @@
return a - skvx::trunc(a / b) * b;
}
-static bool innerRun(const ByteCode* byteCode, const ByteCodeFunction* f, VValue* stack,
+static void innerRun(const ByteCode* byteCode, const ByteCodeFunction* f, VValue* stack,
float* outReturn[], VValue globals[], bool stripedOutput, int N,
int baseIndex) {
// Needs to be the first N non-negative integers, at least as large as VecWidth
@@ -383,7 +382,8 @@
if (f->fConditionCount + 1 > (int)SK_ARRAY_COUNT(condStack) ||
f->fLoopCount + 1 > (int)SK_ARRAY_COUNT(loopStack)) {
- return false;
+ SkDEBUGFAIL("Function with too much nested control flow to evaluate");
+ return;
}
auto mask = [&]() { return *maskPtr & *loopPtr; };
@@ -463,14 +463,6 @@
break;
}
- case ByteCodeInstruction::kClampIndex: {
- int length = READ8();
- if (skvx::any(mask() & ((sp[0].fSigned < 0) | (sp[0].fSigned >= length)))) {
- return false;
- }
- break;
- }
-
VECTOR_BINARY_OP(kCompareIEQ, fSigned, ==)
VECTOR_MATRIX_BINARY_OP(kCompareFEQ, fFloat, ==)
VECTOR_BINARY_OP(kCompareINEQ, fSigned, !=)
@@ -761,7 +753,7 @@
}
}
}
- return true;
+ return;
} else {
// When we were called, the caller reserved stack space for their copy of our
// return value, then 'stack' was positioned after that, where our parameters
@@ -996,11 +988,8 @@
default:
SkDEBUGFAILF("unsupported instruction %d\n", (int) inst);
- return false;
}
}
- // Unreachable
- return false;
}
} // namespace Interpreter
@@ -1018,7 +1007,7 @@
#endif
}
-bool ByteCode::run(const ByteCodeFunction* f, float* args, float* outReturn, int N,
+void ByteCode::run(const ByteCodeFunction* f, float* args, float* outReturn, int N,
const float* uniforms, int uniformCount) const {
#if defined(SK_ENABLE_SKSL_INTERPRETER)
#ifdef TRACE
@@ -1027,17 +1016,13 @@
Interpreter::VValue stack[128];
int stackNeeded = f->fParameterCount + f->fLocalCount + f->fStackCount;
if (stackNeeded > (int)SK_ARRAY_COUNT(stack)) {
- return false;
+ SkDEBUGFAIL("Function requires too much stack space to evaluate");
+ return;
}
- if (uniformCount != (int)fInputSlots.size()) {
- return false;
- }
-
+ SkASSERT(uniformCount == (int)fInputSlots.size());
Interpreter::VValue globals[32];
- if (fGlobalCount > (int)SK_ARRAY_COUNT(globals)) {
- return false;
- }
+ SkASSERT((int)SK_ARRAY_COUNT(globals) >= fGlobalCount);
for (uint8_t slot : fInputSlots) {
globals[slot].fFloat = *uniforms++;
}
@@ -1061,9 +1046,7 @@
bool stripedOutput = false;
float** outArray = outReturn ? &outReturn : nullptr;
- if (!innerRun(this, f, stack, outArray, globals, stripedOutput, w, baseIndex)) {
- return false;
- }
+ innerRun(this, f, stack, outArray, globals, stripedOutput, w, baseIndex);
// Transpose out parameters back
{
@@ -1091,14 +1074,12 @@
N -= w;
baseIndex += w;
}
- return true;
#else
SkDEBUGFAIL("ByteCode interpreter not enabled");
- return false;
#endif
}
-bool ByteCode::runStriped(const ByteCodeFunction* f, float* args[], int nargs, int N,
+void ByteCode::runStriped(const ByteCodeFunction* f, float* args[], int nargs, int N,
const float* uniforms, int uniformCount,
float* outArgs[], int outCount) const {
#if defined(SK_ENABLE_SKSL_INTERPRETER)
@@ -1108,21 +1089,8 @@
Interpreter::VValue stack[128];
int stackNeeded = f->fParameterCount + f->fLocalCount + f->fStackCount;
if (stackNeeded > (int)SK_ARRAY_COUNT(stack)) {
- return false;
- }
-
- if (nargs != f->fParameterCount ||
- outCount != f->fReturnCount ||
- uniformCount != (int)fInputSlots.size()) {
- return false;
- }
-
- Interpreter::VValue globals[32];
- if (fGlobalCount > (int)SK_ARRAY_COUNT(globals)) {
- return false;
- }
- for (uint8_t slot : fInputSlots) {
- globals[slot].fFloat = *uniforms++;
+ SkDEBUGFAIL("Function requires too much stack space to evaluate");
+ return;
}
// innerRun just takes outArgs, so clear it if the count is zero
@@ -1130,6 +1098,15 @@
outArgs = nullptr;
}
+ SkASSERT(nargs == f->fParameterCount);
+ SkASSERT(outCount == f->fReturnCount);
+ SkASSERT(uniformCount == (int)fInputSlots.size());
+ Interpreter::VValue globals[32];
+ SkASSERT((int)SK_ARRAY_COUNT(globals) >= fGlobalCount);
+ for (uint8_t slot : fInputSlots) {
+ globals[slot].fFloat = *uniforms++;
+ }
+
int baseIndex = 0;
while (N) {
@@ -1141,9 +1118,7 @@
}
bool stripedOutput = true;
- if (!innerRun(this, f, stack, outArgs, globals, stripedOutput, w, baseIndex)) {
- return false;
- }
+ innerRun(this, f, stack, outArgs, globals, stripedOutput, w, baseIndex);
// Copy out parameters back
int slot = 0;
@@ -1163,11 +1138,8 @@
N -= w;
baseIndex += w;
}
-
- return true;
#else
SkDEBUGFAIL("ByteCode interpreter not enabled");
- return false;
#endif
}