Handle values above int32 safely during IR generation.
Previously, SKSL_INT was limited to an int32_t, so we couldn't
differentiate between -1 and 4294967295. We could paper over the
difference in some cases by relying on the expression's type, but this
was imperfect and left us unable to differentiate between an overflow
and valid results. SKSL_INT is now an int64_t; the code has been
updated to fix bugs that shook out as a result of the change.
This isn't a complete solution for overflow handling. There are still
lots of obvious places for improvement--e.g. constant folding can
easily overflow, and statements like `byte x = 1000;` are still
happily accepted.
Change-Id: I30d1f56b6f264543f3aa83046f43c2eb56d5fce4
Bug: skia:10932
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/345173
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
diff --git a/src/sksl/SkSLCPPCodeGenerator.cpp b/src/sksl/SkSLCPPCodeGenerator.cpp
index 54fc1ce..4c1e42f 100644
--- a/src/sksl/SkSLCPPCodeGenerator.cpp
+++ b/src/sksl/SkSLCPPCodeGenerator.cpp
@@ -257,7 +257,7 @@
}
void CPPCodeGenerator::writeIntLiteral(const IntLiteral& i) {
- this->write(to_string((int32_t) i.value()));
+ this->write(to_string(i.value()));
}
void CPPCodeGenerator::writeSwizzle(const Swizzle& swizzle) {
@@ -819,11 +819,11 @@
// fFormatArgs will be in a valid state for any future sksl
this->writeCodeAppend(toFlush);
- int codeBlock;
+ SKSL_INT codeBlock;
SkAssertResult(
stoi(StringFragment(sksl.c_str() + tokenStart + 2, i - tokenStart - 2),
&codeBlock));
- SkASSERT(codeBlock < (int)fExtraEmitCodeBlocks.size());
+ SkASSERT((size_t)codeBlock < fExtraEmitCodeBlocks.size());
if (fExtraEmitCodeBlocks[codeBlock].size() > 0) {
this->write(fExtraEmitCodeBlocks[codeBlock].c_str());
}
diff --git a/src/sksl/SkSLDefines.h b/src/sksl/SkSLDefines.h
index c1376c6..77bdcea 100644
--- a/src/sksl/SkSLDefines.h
+++ b/src/sksl/SkSLDefines.h
@@ -35,7 +35,7 @@
#define SKSL_USE_THREAD_LOCAL 1
#endif
-using SKSL_INT = int32_t;
+using SKSL_INT = int64_t;
using SKSL_FLOAT = float;
#endif
diff --git a/src/sksl/SkSLGLSLCodeGenerator.cpp b/src/sksl/SkSLGLSLCodeGenerator.cpp
index bcd1f7e..12d0811 100644
--- a/src/sksl/SkSLGLSLCodeGenerator.cpp
+++ b/src/sksl/SkSLGLSLCodeGenerator.cpp
@@ -1024,7 +1024,7 @@
} else if (type == *fContext.fUByte_Type) {
this->write(to_string(i.value() & 0xff) + "u");
} else {
- this->write(to_string((int32_t) i.value()));
+ this->write(to_string(i.value()));
}
}
diff --git a/src/sksl/SkSLIRGenerator.cpp b/src/sksl/SkSLIRGenerator.cpp
index 92e4e6e..4e00e0a 100644
--- a/src/sksl/SkSLIRGenerator.cpp
+++ b/src/sksl/SkSLIRGenerator.cpp
@@ -384,13 +384,11 @@
if (!size) {
return {};
}
- String name(type->name());
- int64_t count;
if (!size->is<IntLiteral>()) {
fErrors.error(size->fOffset, "array size must be an integer");
return {};
}
- count = size->as<IntLiteral>().value();
+ SKSL_INT count = size->as<IntLiteral>().value();
if (count <= 0) {
fErrors.error(size->fOffset, "array size must be positive");
return {};
@@ -613,7 +611,7 @@
if (!caseValue) {
return nullptr;
}
- int64_t v = 0;
+ SKSL_INT v = 0;
if (!this->getConstantInt(*caseValue, &v)) {
fErrors.error(caseValue->fOffset, "case value must be a constant integer");
return nullptr;
@@ -1211,7 +1209,7 @@
symbols);
}
-bool IRGenerator::getConstantInt(const Expression& value, int64_t* out) {
+bool IRGenerator::getConstantInt(const Expression& value, SKSL_INT* out) {
switch (value.kind()) {
case Expression::Kind::kIntLiteral:
*out = value.as<IntLiteral>().value();
@@ -1241,7 +1239,7 @@
}
SkASSERT(e.fKind == ASTNode::Kind::kEnum);
- int64_t currentValue = 0;
+ SKSL_INT currentValue = 0;
Layout layout;
ASTNode enumType(e.fNodes, e.fOffset, ASTNode::Kind::kType,
ASTNode::TypeData(e.getString(), /*isStructDeclaration=*/false));
@@ -1865,17 +1863,17 @@
#define RESULT(t, op) std::make_unique<t ## Literal>(fContext, left.fOffset, \
leftVal op rightVal)
#define URESULT(t, op) std::make_unique<t ## Literal>(fContext, left.fOffset, \
- (uint32_t) leftVal op \
- (uint32_t) rightVal)
+ (uint64_t) leftVal op \
+ (uint64_t) rightVal)
if (left.is<IntLiteral>() && right.is<IntLiteral>()) {
- int64_t leftVal = left.as<IntLiteral>().value();
- int64_t rightVal = right.as<IntLiteral>().value();
+ SKSL_INT leftVal = left.as<IntLiteral>().value();
+ SKSL_INT rightVal = right.as<IntLiteral>().value();
switch (op) {
case Token::Kind::TK_PLUS: return URESULT(Int, +);
case Token::Kind::TK_MINUS: return URESULT(Int, -);
case Token::Kind::TK_STAR: return URESULT(Int, *);
case Token::Kind::TK_SLASH:
- if (leftVal == std::numeric_limits<int64_t>::min() && rightVal == -1) {
+ if (leftVal == std::numeric_limits<SKSL_INT>::min() && rightVal == -1) {
fErrors.error(right.fOffset, "arithmetic overflow");
return nullptr;
}
@@ -1885,7 +1883,7 @@
}
return RESULT(Int, /);
case Token::Kind::TK_PERCENT:
- if (leftVal == std::numeric_limits<int64_t>::min() && rightVal == -1) {
+ if (leftVal == std::numeric_limits<SKSL_INT>::min() && rightVal == -1) {
fErrors.error(right.fOffset, "arithmetic overflow");
return nullptr;
}
@@ -1905,13 +1903,13 @@
case Token::Kind::TK_LTEQ: return RESULT(Bool, <=);
case Token::Kind::TK_SHL:
if (rightVal >= 0 && rightVal <= 31) {
- return URESULT(Int, <<);
+ return RESULT(Int, <<);
}
fErrors.error(right.fOffset, "shift value out of range");
return nullptr;
case Token::Kind::TK_SHR:
if (rightVal >= 0 && rightVal <= 31) {
- return URESULT(Int, >>);
+ return RESULT(Int, >>);
}
fErrors.error(right.fOffset, "shift value out of range");
return nullptr;
@@ -2291,8 +2289,8 @@
return std::make_unique<FloatLiteral>(offset, value, &type);
}
if (type.isFloat() && args.size() == 1 && args[0]->is<IntLiteral>()) {
- int64_t value = args[0]->as<IntLiteral>().value();
- return std::make_unique<FloatLiteral>(offset, (float)value, &type);
+ SKSL_INT value = args[0]->as<IntLiteral>().value();
+ return std::make_unique<FloatLiteral>(offset, (SKSL_FLOAT)value, &type);
}
if (args[0]->is<IntLiteral>() && (type == *fContext.fInt_Type ||
type == *fContext.fUInt_Type)) {
@@ -2794,7 +2792,7 @@
}
// Perform compile-time bounds checking on constant indices.
if (converted->is<IntLiteral>()) {
- int64_t index = converted->as<IntLiteral>().value();
+ SKSL_INT index = converted->as<IntLiteral>().value();
const int upperBound = (baseType.isArray() && baseType.columns() == Type::kUnsizedArray)
? INT_MAX
diff --git a/src/sksl/SkSLIRGenerator.h b/src/sksl/SkSLIRGenerator.h
index 46834b2..350ff1d 100644
--- a/src/sksl/SkSLIRGenerator.h
+++ b/src/sksl/SkSLIRGenerator.h
@@ -230,7 +230,7 @@
void checkValid(const Expression& expr);
bool typeContainsPrivateFields(const Type& type);
bool setRefKind(Expression& expr, VariableReference::RefKind kind);
- bool getConstantInt(const Expression& value, int64_t* out);
+ bool getConstantInt(const Expression& value, SKSL_INT* out);
void copyIntrinsicIfNeeded(const FunctionDeclaration& function);
void findAndDeclareBuiltinVariables();
diff --git a/src/sksl/SkSLMetalCodeGenerator.cpp b/src/sksl/SkSLMetalCodeGenerator.cpp
index 8d12554..563ac71 100644
--- a/src/sksl/SkSLMetalCodeGenerator.cpp
+++ b/src/sksl/SkSLMetalCodeGenerator.cpp
@@ -1279,10 +1279,15 @@
}
void MetalCodeGenerator::writeIntLiteral(const IntLiteral& i) {
- if (i.type() == *fContext.fUInt_Type) {
+ const Type& type = i.type();
+ if (type == *fContext.fUInt_Type) {
this->write(to_string(i.value() & 0xffffffff) + "u");
+ } else if (type == *fContext.fUShort_Type) {
+ this->write(to_string(i.value() & 0xffff) + "u");
+ } else if (type == *fContext.fUByte_Type) {
+ this->write(to_string(i.value() & 0xff) + "u");
} else {
- this->write(to_string((int32_t) i.value()));
+ this->write(to_string(i.value()));
}
}
diff --git a/src/sksl/SkSLString.cpp b/src/sksl/SkSLString.cpp
index df7039f..ec3e569 100644
--- a/src/sksl/SkSLString.cpp
+++ b/src/sksl/SkSLString.cpp
@@ -249,8 +249,6 @@
char* p;
errno = 0;
unsigned long long result = strtoull(s.begin(), &p, /*base=*/0);
- // TODO(skia:10932): SKSL_INT is still a signed 32-bit int. Values above 0x7FFFFFFF can be
- // interpreted incorrectly depending on the types involved.
*value = static_cast<SKSL_INT>(result);
return p == s.end() && errno == 0 && result <= 0xFFFFFFFF;
}
diff --git a/src/sksl/SkSLVMGenerator.cpp b/src/sksl/SkSLVMGenerator.cpp
index bc8ace8..c8a6cb9 100644
--- a/src/sksl/SkSLVMGenerator.cpp
+++ b/src/sksl/SkSLVMGenerator.cpp
@@ -489,7 +489,7 @@
const Expression& index = *i.index();
SkASSERT(index.isCompileTimeConstant());
- int64_t indexValue = index.getConstantInt();
+ SKSL_INT indexValue = index.getConstantInt();
SkASSERT(indexValue >= 0 && indexValue < i.base()->type().columns());
size_t stride = slot_count(i.type());
diff --git a/src/sksl/ir/SkSLConstructor.cpp b/src/sksl/ir/SkSLConstructor.cpp
index 9a5e219..6faa299 100644
--- a/src/sksl/ir/SkSLConstructor.cpp
+++ b/src/sksl/ir/SkSLConstructor.cpp
@@ -14,7 +14,7 @@
if (this->arguments().size() == 1 && this->arguments()[0]->is<IntLiteral>()) {
const Context& context = irGenerator.fContext;
const Type& type = this->type();
- int64_t intValue = this->arguments()[0]->as<IntLiteral>().value();
+ SKSL_INT intValue = this->arguments()[0]->as<IntLiteral>().value();
if (type.isFloat()) {
// promote float(1) to 1.0
@@ -195,7 +195,7 @@
ABORT("can't happen, matrix component out of bounds");
}
-int64_t Constructor::getConstantInt() const {
+SKSL_INT Constructor::getConstantInt() const {
// We're looking for scalar integer constructors only, i.e. `int(1)`.
SkASSERT(this->arguments().size() == 1);
SkASSERT(this->type().columns() == 1);
@@ -204,7 +204,7 @@
// The inner argument might actually be a float! `int(1.0)` is a valid cast.
const Expression& expr = *this->arguments().front();
SkASSERT(expr.type().isScalar());
- return expr.type().isInteger() ? expr.getConstantInt() : (int64_t)expr.getConstantFloat();
+ return expr.type().isInteger() ? expr.getConstantInt() : (SKSL_INT)expr.getConstantFloat();
}
SKSL_FLOAT Constructor::getConstantFloat() const {
diff --git a/src/sksl/ir/SkSLConstructor.h b/src/sksl/ir/SkSLConstructor.h
index 27a4687..ca2a5fb 100644
--- a/src/sksl/ir/SkSLConstructor.h
+++ b/src/sksl/ir/SkSLConstructor.h
@@ -123,7 +123,7 @@
SKSL_FLOAT getMatComponent(int col, int row) const override;
- int64_t getConstantInt() const override;
+ SKSL_INT getConstantInt() const override;
SKSL_FLOAT getConstantFloat() const override;
diff --git a/src/sksl/ir/SkSLExpression.h b/src/sksl/ir/SkSLExpression.h
index 72d81de..d56b5dc 100644
--- a/src/sksl/ir/SkSLExpression.h
+++ b/src/sksl/ir/SkSLExpression.h
@@ -122,7 +122,7 @@
* For an expression which evaluates to a constant int, returns the value. Otherwise calls
* ABORT.
*/
- virtual int64_t getConstantInt() const {
+ virtual SKSL_INT getConstantInt() const {
ABORT("not a constant int");
}
diff --git a/src/sksl/ir/SkSLIntLiteral.h b/src/sksl/ir/SkSLIntLiteral.h
index b7863ff..0be0f51 100644
--- a/src/sksl/ir/SkSLIntLiteral.h
+++ b/src/sksl/ir/SkSLIntLiteral.h
@@ -25,9 +25,9 @@
public:
static constexpr Kind kExpressionKind = Kind::kIntLiteral;
- // FIXME: we will need to revisit this if/when we add full support for both signed and unsigned
- // 64-bit integers, but for right now an int64_t will hold every value we care about
- Literal(const Context& context, int offset, int64_t value)
+ // We will need to revisit this if we want full support for unsigned 64-bit integers,
+ // but for now an SKSL_INT (int64_t) will hold every value we care about.
+ Literal(const Context& context, int offset, SKSL_INT value)
: INHERITED(offset, kExpressionKind, context.fInt_Type.get())
, fValue(value) {}
@@ -35,7 +35,7 @@
: INHERITED(offset, kExpressionKind, type)
, fValue(value) {}
- int64_t value() const {
+ SKSL_INT value() const {
return fValue;
}
@@ -68,7 +68,7 @@
return INHERITED::coercionCost(target);
}
- int64_t getConstantInt() const override {
+ SKSL_INT getConstantInt() const override {
return this->value();
}
@@ -77,7 +77,7 @@
}
private:
- int64_t fValue;
+ SKSL_INT fValue;
using INHERITED = Expression;
};
diff --git a/src/sksl/ir/SkSLPrefixExpression.h b/src/sksl/ir/SkSLPrefixExpression.h
index 362de08..6ce9b49 100644
--- a/src/sksl/ir/SkSLPrefixExpression.h
+++ b/src/sksl/ir/SkSLPrefixExpression.h
@@ -85,7 +85,7 @@
return Compiler::OperatorName(this->getOperator()) + this->operand()->description();
}
- int64_t getConstantInt() const override {
+ SKSL_INT getConstantInt() const override {
SkASSERT(this->isNegationOfCompileTimeConstant());
return -this->operand()->getConstantInt();
}
diff --git a/src/sksl/ir/SkSLSwizzle.h b/src/sksl/ir/SkSLSwizzle.h
index 7d812b2..af61404 100644
--- a/src/sksl/ir/SkSLSwizzle.h
+++ b/src/sksl/ir/SkSLSwizzle.h
@@ -52,7 +52,7 @@
const Type& type = this->type();
if (type.isInteger()) {
SkASSERT(this->components().size() == 1);
- int64_t value = constructor.getIVecComponent(this->components()[0]);
+ SKSL_INT value = constructor.getIVecComponent(this->components()[0]);
return std::make_unique<IntLiteral>(irGenerator.fContext, constructor.fOffset,
value);
} else if (type.isFloat()) {
diff --git a/tests/sksl/errors/golden/ArrayIndexOutOfRange.glsl b/tests/sksl/errors/golden/ArrayIndexOutOfRange.glsl
index a133fae..825b170 100644
--- a/tests/sksl/errors/golden/ArrayIndexOutOfRange.glsl
+++ b/tests/sksl/errors/golden/ArrayIndexOutOfRange.glsl
@@ -3,9 +3,9 @@
error: 3: index -1 out of range for 'int[123]'
error: 6: index 123 out of range for 'half4x4[123]'
error: 7: index 1000000000 out of range for 'int4[123]'
-error: 8: index -1294967296 out of range for 'half3[123]'
+error: 8: index 3000000000 out of range for 'half3[123]'
error: 10: index -1 out of range for 'Block[]'
-error: 13: index -1294967296 out of range for 'Block[]'
+error: 13: index 3000000000 out of range for 'Block[]'
error: 15: index -1 out of range for 'half4'
error: 20: index 4 out of range for 'half4'
error: 21: index 1000000000 out of range for 'half4'
diff --git a/tests/sksl/errors/golden/OverflowIntLiteral.glsl b/tests/sksl/errors/golden/OverflowIntLiteral.glsl
index a484a2c..c3a6ab7 100644
--- a/tests/sksl/errors/golden/OverflowIntLiteral.glsl
+++ b/tests/sksl/errors/golden/OverflowIntLiteral.glsl
@@ -1,9 +1,9 @@
out vec4 sk_FragColor;
int intMin = -2147483648;
-int intMinMinusOne = 2147483647;
+int intMinMinusOne = -2147483649;
int intMax = 2147483647;
-int intMaxPlusOne = -2147483648;
+int intMaxPlusOne = 2147483648;
void main() {
sk_FragColor.x = float(intMin);
sk_FragColor.x = float(intMax);
diff --git a/tests/sksl/shared/golden/Hex.glsl b/tests/sksl/shared/golden/Hex.glsl
index 2aa8413..e2fabda 100644
--- a/tests/sksl/shared/golden/Hex.glsl
+++ b/tests/sksl/shared/golden/Hex.glsl
@@ -6,7 +6,7 @@
i2++;
int i3 = 2147483647;
i3++;
- int i4 = -1;
+ int i4 = 4294967295;
i4++;
int i5 = -48879;
i5++;
diff --git a/tests/sksl/shared/golden/Hex.metal b/tests/sksl/shared/golden/Hex.metal
index 205c57d..794f80c 100644
--- a/tests/sksl/shared/golden/Hex.metal
+++ b/tests/sksl/shared/golden/Hex.metal
@@ -15,7 +15,7 @@
i2++;
int i3 = 2147483647;
i3++;
- int i4 = -1;
+ int i4 = 4294967295;
i4++;
int i5 = -48879;
i5++;
@@ -27,7 +27,7 @@
u3++;
uint u4 = 4294967295u;
u4++;
- ushort u5 = 65535;
+ ushort u5 = 65535u;
u5++;
return *_out;
}