Handle corner cases of shifting signed integers better

src/compiler/translator/ConstantUnion.cpp

@@ -381,9 +381,8 @@ TConstantUnion TConstantUnion::rshift(const TConstantUnion &lhs,
     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 @@ TConstantUnion TConstantUnion::rshift(const TConstantUnion &lhs,
     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();
             }
+            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 TConstantUnion::lshift(const TConstantUnion &lhs,
     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 @@ TConstantUnion TConstantUnion::lshift(const TConstantUnion &lhs,
         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();

src/tests/compiler_tests/ConstantFolding_test.cpp

@@ -825,6 +825,45 @@ TEST_F(ConstantFoldingTest, FoldBitShiftRightDifferentSignedness)
     ASSERT_TRUE(constantFoundInAST(0x3u));
 }
 
+// Test that folding signed bit shift right extends the sign bit.
+// ESSL 3.00.6 section 5.9 Expressions.
+TEST_F(ConstantFoldingTest, FoldBitShiftRightExtendSignBit)
+{
+    const std::string &shaderString =
+        "#version 300 es\n"
+        "precision mediump float;\n"
+        "out vec4 my_FragColor;\n"
+        "void main()\n"
+        "{\n"
+        "    const int i = 0x8fffe000 >> 6;\n"
+        "    uint u = uint(i);"
+        "    my_FragColor = vec4(u);\n"
+        "}\n";
+    compile(shaderString);
+    // The bits of the operand are 0x8fffe000 = 1000 1111 1111 1111 1110 0000 0000 0000
+    // After shifting, they become              1111 1110 0011 1111 1111 1111 1000 0000 = 0xfe3fff80
+    ASSERT_TRUE(constantFoundInAST(0xfe3fff80u));
+}
+
+// Signed bit shift left should interpret its operand as a bit pattern. As a consequence a number
+// may turn from positive to negative when shifted left.
+// ESSL 3.00.6 section 5.9 Expressions.
+TEST_F(ConstantFoldingTest, FoldBitShiftLeftInterpretedAsBitPattern)
+{
+    const std::string &shaderString =
+        "#version 300 es\n"
+        "precision mediump float;\n"
+        "out vec4 my_FragColor;\n"
+        "void main()\n"
+        "{\n"
+        "    const int i = 0x1fffffff << 3;\n"
+        "    uint u = uint(i);"
+        "    my_FragColor = vec4(u);\n"
+        "}\n";
+    compile(shaderString);
+    ASSERT_TRUE(constantFoundInAST(0xfffffff8u));
+}
+
 // Test that dividing the minimum signed integer by -1 works.
 // ESSL 3.00.6 section 4.1.3 Integers:
 // "However, for the case where the minimum representable value is divided by -1, it is allowed to