Handle corner cases of shifting signed integers better
Right-shifting a negative number should sign-extend according to the
ESSL 3.00.6 spec. Implement sign-extending right shift so that it
doesn't hit any undefined behavior in the C++ spec. Negative lhs
operands are now allowed for bit-shift right.
Also implement bit-shift left via conversion to unsigned integer, so
that it does not hit signed integer overflow. Negative lhs operands
are now allowed also for bit-shift left as well.
BUG=chromium:654103
TEST=angle_unittests
Change-Id: Iee241de9fd0d74c2f8a88219bddec690bb8e4db2
Reviewed-on: https://chromium-review.googlesource.com/395688
Commit-Queue: Olli Etuaho <oetuaho@nvidia.com>
Reviewed-by: Geoff Lang <geofflang@chromium.org>
diff --git a/src/compiler/translator/ConstantUnion.cpp b/src/compiler/translator/ConstantUnion.cpp
index 10a70f3..b4074bf 100644
--- a/src/compiler/translator/ConstantUnion.cpp
+++ b/src/compiler/translator/ConstantUnion.cpp
@@ -381,9 +381,8 @@
TConstantUnion returnValue;
ASSERT(lhs.type == EbtInt || lhs.type == EbtUInt);
ASSERT(rhs.type == EbtInt || rhs.type == EbtUInt);
- if ((lhs.type == EbtInt && lhs.iConst < 0) ||
- (rhs.type == EbtInt && (rhs.iConst < 0 || rhs.iConst > 31)) ||
- (rhs.type == EbtUInt && rhs.uConst > 31))
+ if ((rhs.type == EbtInt && (rhs.iConst < 0 || rhs.iConst > 31)) ||
+ (rhs.type == EbtUInt && rhs.uConst > 31u))
{
diag->error(line, "Undefined shift (operand out of range)", ">>", "");
switch (lhs.type)
@@ -403,19 +402,48 @@
switch (lhs.type)
{
case EbtInt:
+ {
+ unsigned int shiftOffset = 0;
switch (rhs.type)
{
case EbtInt:
- returnValue.setIConst(lhs.iConst >> rhs.iConst);
+ shiftOffset = static_cast<unsigned int>(rhs.iConst);
break;
case EbtUInt:
- returnValue.setIConst(lhs.iConst >> rhs.uConst);
+ shiftOffset = rhs.uConst;
break;
default:
UNREACHABLE();
}
- break;
+ if (shiftOffset > 0)
+ {
+ // ESSL 3.00.6 section 5.9: "If E1 is a signed integer, the right-shift will extend
+ // the sign bit." In C++ shifting negative integers is undefined, so we implement
+ // extending the sign bit manually.
+ bool extendSignBit = false;
+ int lhsSafe = lhs.iConst;
+ if (lhsSafe < 0)
+ {
+ extendSignBit = true;
+ // Clear the sign bit so that bitshift right is defined in C++.
+ lhsSafe &= 0x7fffffff;
+ ASSERT(lhsSafe > 0);
+ }
+ returnValue.setIConst(lhsSafe >> shiftOffset);
+ // Manually fill in the extended sign bit if necessary.
+ if (extendSignBit)
+ {
+ int extendedSignBit = static_cast<int>(0xffffffffu << (31 - shiftOffset));
+ returnValue.setIConst(returnValue.getIConst() | extendedSignBit);
+ }
+ }
+ else
+ {
+ returnValue.setIConst(rhs.iConst);
+ }
+ break;
+ }
case EbtUInt:
switch (rhs.type)
{
@@ -445,9 +473,8 @@
TConstantUnion returnValue;
ASSERT(lhs.type == EbtInt || lhs.type == EbtUInt);
ASSERT(rhs.type == EbtInt || rhs.type == EbtUInt);
- if ((lhs.type == EbtInt && lhs.iConst < 0) ||
- (rhs.type == EbtInt && (rhs.iConst < 0 || rhs.iConst > 31)) ||
- (rhs.type == EbtUInt && rhs.uConst > 31))
+ if ((rhs.type == EbtInt && (rhs.iConst < 0 || rhs.iConst > 31)) ||
+ (rhs.type == EbtUInt && rhs.uConst > 31u))
{
diag->error(line, "Undefined shift (operand out of range)", "<<", "");
switch (lhs.type)
@@ -469,11 +496,16 @@
case EbtInt:
switch (rhs.type)
{
+ // Cast to unsigned integer before shifting, since ESSL 3.00.6 section 5.9 says that
+ // lhs is "interpreted as a bit pattern". This also avoids the possibility of signed
+ // integer overflow or undefined shift of a negative integer.
case EbtInt:
- returnValue.setIConst(lhs.iConst << rhs.iConst);
+ returnValue.setIConst(
+ static_cast<int>(static_cast<uint32_t>(lhs.iConst) << rhs.iConst));
break;
case EbtUInt:
- returnValue.setIConst(lhs.iConst << rhs.uConst);
+ returnValue.setIConst(
+ static_cast<int>(static_cast<uint32_t>(lhs.iConst) << rhs.uConst));
break;
default:
UNREACHABLE();