Fix undefined behaviour in integer shifts (#1385)

frontend-provider · web-flow · commit a6c6c9113d98 · 2018-11-15T11:16:02.000+01:00
Prior to this change, Integer shifts used to exhibit
undefined behavior if the number of bits is outside
of 0..BITS-1 range (i.e., 0..31 for shifts on Int and 0..63 for
shifts on Long). Here we address this change by masking
the bits part to always be in range.
diff --git a/nir/src/main/scala/scala/scalanative/nir/Ops.scala b/nir/src/main/scala/scala/scalanative/nir/Ops.scala
@@ -42,17 +42,44 @@ sealed abstract class Op {
 
   final def show: String = nir.Show(this)
 
+  /** Op is pure if it doesn't have any side-effects, including:
+   *
+   *  * doesn't throw exceptions
+   *  * doesn't perform any unsafe reads or writes from the memory
+   *  * doesn't call foreign code
+   *
+   *  Recomputing pure op will always yield to the same result.
+   */
   final def isPure: Boolean = this match {
     case _: Op.Elem | _: Op.Extract | _: Op.Insert | _: Op.Comp | _: Op.Conv |
-        _: Op.Select =>
+        _: Op.Select | _: Op.Is | _: Op.Copy | _: Op.Sizeof =>
       true
-    case Op.Bin(Bin.Sdiv | Bin.Udiv | Bin.Srem | Bin.Urem, _, _, _) =>
+    // Division and modulo on integers are not pure as
+    // they may throw if the divisor is zero.
+    case Op.Bin(Bin.Sdiv | Bin.Udiv | Bin.Srem | Bin.Urem, _: Type.I, _, _) =>
       false
     case _: Op.Bin =>
       true
     case _ =>
       false
   }
+
+  /** Op is idempotent if re-evaluation of the operation with the same
+   *  arguments is going to produce the same results, without any extra
+   *  side effects as long as previous evaluation did not throw.
+   */
+  final def isIdempotent: Boolean = this match {
+    case op if op.isPure =>
+      true
+    // Division and modulo are non-pure but idempotent.
+    case op: Op.Bin =>
+      true
+    case _: Op.Method | _: Op.Dynmethod | _: Op.Module | _: Op.Box |
+        _: Op.Unbox | _: Op.Arraylength =>
+      true
+    case _ =>
+      false
+  }
 }
 object Op {
   // low-level
diff --git a/tools/src/main/scala/scala/scalanative/codegen/Lower.scala b/tools/src/main/scala/scala/scalanative/codegen/Lower.scala
@@ -506,13 +506,32 @@ object Lower {
         label(resultL, Seq(Val.Local(n, ty)))
       }
 
+      // Shifts are undefined if the bits shifted by are >= bits in the type.
+      // We mask the right hand side with bits in type - 1 to make it defined.
+      def maskShift(op: Op.Bin) = {
+        val Op.Bin(_, ty: Type.I, _, r) = op
+        val mask = ty match {
+          case Type.Int  => Val.Int(31)
+          case Type.Long => Val.Int(63)
+          case _         => util.unreachable
+        }
+        val masked = bin(Bin.And, ty, r, mask, unwind)
+        let(n, op.copy(r = masked), unwind)
+      }
+
       op match {
         case op @ Op.Bin(bin @ (Bin.Srem | Bin.Urem | Bin.Sdiv | Bin.Udiv),
                          ty: Type.I,
                          l,
                          r) =>
           checkDivisionByZero(op)
 
+        case op @ Op.Bin(bin @ (Bin.Shl | Bin.Lshr | Bin.Ashr),
+                         ty: Type.I,
+                         l,
+                         r) =>
+          maskShift(op)
+
         case op =>
           let(n, op, unwind)
       }
diff --git a/tools/src/main/scala/scala/scalanative/optimizer/pass/GlobalValueNumbering.scala b/tools/src/main/scala/scala/scalanative/optimizer/pass/GlobalValueNumbering.scala
@@ -52,7 +52,7 @@ class GlobalValueNumbering extends Pass {
       val newBlockInsts = block.insts.map {
 
         case inst: Inst.Let => {
-          val idempotent = isIdempotent(inst.op)
+          val idempotent = inst.op.isIdempotent
 
           val instHash =
             if (idempotent)
@@ -97,24 +97,6 @@ class GlobalValueNumbering extends Pass {
 }
 
 object GlobalValueNumbering extends PassCompanion {
-  def isIdempotent(op: Op): Boolean = {
-    import Op._
-    op match {
-      // Always idempotent:
-      case (_: Method | _: Dynmethod | _: As | _: Is | _: Copy | _: Sizeof |
-          _: Module | _: Box | _: Unbox | _: Arraylength) =>
-        true
-      case op if op.isPure =>
-        true
-
-      // Never idempotent:
-      case (_: Load | _: Store | _: Stackalloc | _: Classalloc | _: Call |
-          _: Closure | _: Fieldload | _: Fieldstore | _: Var | _: Varload |
-          _: Varstore | _: Arrayalloc | _: Arrayload | _: Arraystore) =>
-        false
-    }
-  }
-
   class DeepEquals(localDefs: Local => Inst) {
 
     def eqInst(instA: Inst.Let, instB: Inst.Let): Boolean = {
@@ -123,7 +105,7 @@ object GlobalValueNumbering extends PassCompanion {
 
     def eqOp(opA: Op, opB: Op): Boolean = {
       import Op._
-      if (!(isIdempotent(opA) && isIdempotent(opB)))
+      if (!(opA.isIdempotent && opB.isIdempotent))
         false
       else {
         (opA, opB) match {
diff --git a/unit-tests/src/test/scala/scala/ShiftOverflowSuite.scala b/unit-tests/src/test/scala/scala/ShiftOverflowSuite.scala
@@ -0,0 +1,117 @@
+package scala
+
+object ShiftOverflowSuite extends tests.Suite {
+  @noinline def noinlineByte42: Byte   = 42.toByte
+  @noinline def noinlineShort42: Short = 42.toShort
+  @noinline def noinlineChar42: Char   = 42.toChar
+  @noinline def noinlineInt42: Int     = 42
+  @noinline def noinlineLong42: Long   = 42L
+  @noinline def noinlineInt33: Int     = 33
+  @noinline def noinlineLong65: Long   = 65L
+  @noinline def noinlineInt21: Int     = 21
+  @noinline def noinlineLong21: Long   = 21L
+  @noinline def noinlineInt84: Int     = 84
+  @noinline def noinlineLong84: Long   = 84L
+
+  @inline def inlineByte42: Byte   = 42.toByte
+  @inline def inlineShort42: Short = 42.toShort
+  @inline def inlineChar42: Char   = 42.toChar
+  @inline def inlineInt42: Int     = 42
+  @inline def inlineLong42: Long   = 42L
+  @inline def inlineInt33: Int     = 33
+  @inline def inlineLong65: Long   = 65L
+  @inline def inlineInt21: Int     = 21
+  @inline def inlineLong21: Long   = 21L
+  @inline def inlineInt84: Int     = 84
+  @inline def inlineLong84: Long   = 84L
+
+  test("x << 33 (noinline)") {
+    assert((noinlineByte42 << noinlineInt33) == noinlineInt84)
+    assert((noinlineShort42 << noinlineInt33) == noinlineInt84)
+    assert((noinlineChar42 << noinlineInt33) == noinlineInt84)
+    assert((noinlineInt42 << noinlineInt33) == noinlineInt84)
+  }
+
+  test("x << 33 (inline)") {
+    assert((inlineByte42 << inlineInt33) == inlineInt84)
+    assert((inlineShort42 << inlineInt33) == inlineInt84)
+    assert((inlineChar42 << inlineInt33) == inlineInt84)
+    assert((inlineInt42 << inlineInt33) == inlineInt84)
+  }
+
+  test("x << 65L (noinline)") {
+    assert((noinlineByte42 << noinlineLong65) == noinlineLong84)
+    assert((noinlineShort42 << noinlineLong65) == noinlineLong84)
+    assert((noinlineChar42 << noinlineLong65) == noinlineLong84)
+    assert((noinlineInt42 << noinlineLong65) == noinlineLong84)
+    assert((noinlineLong42 << noinlineLong65) == noinlineLong84)
+  }
+
+  test("x << 65L (inline)") {
+    assert((inlineByte42 << inlineLong65) == inlineLong84)
+    assert((inlineShort42 << inlineLong65) == inlineLong84)
+    assert((inlineChar42 << inlineLong65) == inlineLong84)
+    assert((inlineInt42 << inlineLong65) == inlineLong84)
+    assert((inlineLong42 << inlineLong65) == inlineLong84)
+  }
+
+  test("x >> 33 (noinline)") {
+    assert((noinlineByte42 >> noinlineInt33) == noinlineInt21)
+    assert((noinlineShort42 >> noinlineInt33) == noinlineInt21)
+    assert((noinlineChar42 >> noinlineInt33) == noinlineInt21)
+    assert((noinlineInt42 >> noinlineInt33) == noinlineInt21)
+  }
+
+  test("x >> 33 (inline)") {
+    assert((inlineByte42 >> inlineInt33) == inlineInt21)
+    assert((inlineShort42 >> inlineInt33) == inlineInt21)
+    assert((inlineChar42 >> inlineInt33) == inlineInt21)
+    assert((inlineInt42 >> inlineInt33) == inlineInt21)
+  }
+
+  test("x >> 65L (noinline)") {
+    assert((noinlineByte42 >> noinlineLong65) == noinlineLong21)
+    assert((noinlineShort42 >> noinlineLong65) == noinlineLong21)
+    assert((noinlineChar42 >> noinlineLong65) == noinlineLong21)
+    assert((noinlineInt42 >> noinlineLong65) == noinlineLong21)
+    assert((noinlineLong42 >> noinlineLong65) == noinlineLong21)
+  }
+
+  test("x >> 65L (inline)") {
+    assert((inlineByte42 >> inlineLong65) == inlineLong21)
+    assert((inlineShort42 >> inlineLong65) == inlineLong21)
+    assert((inlineChar42 >> inlineLong65) == inlineLong21)
+    assert((inlineInt42 >> inlineLong65) == inlineLong21)
+    assert((inlineLong42 >> inlineLong65) == inlineLong21)
+  }
+
+  test("x >>> 33 (noinline)") {
+    assert((noinlineByte42 >>> noinlineInt33) == noinlineInt21)
+    assert((noinlineShort42 >>> noinlineInt33) == noinlineInt21)
+    assert((noinlineChar42 >>> noinlineInt33) == noinlineInt21)
+    assert((noinlineInt42 >>> noinlineInt33) == noinlineInt21)
+  }
+
+  test("x >>> 33 (inline)") {
+    assert((inlineByte42 >>> inlineInt33) == inlineInt21)
+    assert((inlineShort42 >>> inlineInt33) == inlineInt21)
+    assert((inlineChar42 >>> inlineInt33) == inlineInt21)
+    assert((inlineInt42 >>> inlineInt33) == inlineInt21)
+  }
+
+  test("x >>> 65L (noinline)") {
+    assert((noinlineByte42 >>> noinlineLong65) == noinlineLong21)
+    assert((noinlineShort42 >>> noinlineLong65) == noinlineLong21)
+    assert((noinlineChar42 >>> noinlineLong65) == noinlineLong21)
+    assert((noinlineInt42 >>> noinlineLong65) == noinlineLong21)
+    assert((noinlineLong42 >>> noinlineLong65) == noinlineLong21)
+  }
+
+  test("x >>> 65L (inline)") {
+    assert((inlineByte42 >>> inlineLong65) == inlineLong21)
+    assert((inlineShort42 >>> inlineLong65) == inlineLong21)
+    assert((inlineChar42 >>> inlineLong65) == inlineLong21)
+    assert((inlineInt42 >>> inlineLong65) == inlineLong21)
+    assert((inlineLong42 >>> inlineLong65) == inlineLong21)
+  }
+}
