Make division by zero defined behaviour (#1384)

frontend-provider · web-flow · commit 1a0a87860263 · 2018-11-14T13:07:26.000+01:00
This change makes integer division behave the same as on JVM:

1. Division by 0 throws an ArithmeticException.
2. Division that overflows wraps around.

Previously these two cases were undefined. While integer segfaults
are relatively easy to debug, the problem gets worse once LLVM
starts optimizing assuming undefined behaviour (by removing said
segfaults and replacing ops with 0).
diff --git a/docs/user/lang.rst b/docs/user/lang.rst
@@ -34,7 +34,6 @@ A number of error conditions which are well-defined on JVM are undefined
 behavior:
 
 1. Dereferencing null.
-2. Division by zero.
 3. Stack overflows.
 
 Those typically crash application with a segfault on the supported architectures.
diff --git a/nativelib/src/main/scala/scala/scalanative/runtime/package.scala b/nativelib/src/main/scala/scala/scalanative/runtime/package.scala
@@ -89,5 +89,10 @@ package object runtime {
   /** Run the runtime's event loop. The method is called from the
    *  generated C-style after the application's main method terminates.
    */
-  def loop(): Unit = ExecutionContext.loop()
+  def loop(): Unit =
+    ExecutionContext.loop()
+
+  /** Called by the compiler in case of division by zero. */
+  def throwDivisionByZero(): Nothing =
+    throw new java.lang.ArithmeticException("/ by zero")
 }
diff --git a/nir/src/main/scala/scala/scalanative/nir/Ops.scala b/nir/src/main/scala/scala/scalanative/nir/Ops.scala
@@ -41,23 +41,33 @@ sealed abstract class Op {
   }
 
   final def show: String = nir.Show(this)
+
+  final def isPure: Boolean = this match {
+    case _: Op.Elem | _: Op.Extract | _: Op.Insert | _: Op.Comp | _: Op.Conv |
+        _: Op.Select =>
+      true
+    case Op.Bin(Bin.Sdiv | Bin.Udiv | Bin.Srem | Bin.Urem, _, _, _) =>
+      false
+    case _: Op.Bin =>
+      true
+    case _ =>
+      false
+  }
 }
 object Op {
-  sealed abstract class Pure extends Op
-
   // low-level
   final case class Call(ty: Type, ptr: Val, args: Seq[Val])      extends Op
   final case class Load(ty: Type, ptr: Val, isVolatile: Boolean) extends Op
   final case class Store(ty: Type, ptr: Val, value: Val, isVolatile: Boolean)
       extends Op
-  final case class Elem(ty: Type, ptr: Val, indexes: Seq[Val])      extends Pure
-  final case class Extract(aggr: Val, indexes: Seq[Int])            extends Pure
-  final case class Insert(aggr: Val, value: Val, indexes: Seq[Int]) extends Pure
+  final case class Elem(ty: Type, ptr: Val, indexes: Seq[Val])      extends Op
+  final case class Extract(aggr: Val, indexes: Seq[Int])            extends Op
+  final case class Insert(aggr: Val, value: Val, indexes: Seq[Int]) extends Op
   final case class Stackalloc(ty: Type, n: Val)                     extends Op
-  final case class Bin(bin: nir.Bin, ty: Type, l: Val, r: Val)      extends Pure
-  final case class Comp(comp: nir.Comp, ty: Type, l: Val, r: Val)   extends Pure
-  final case class Conv(conv: nir.Conv, ty: Type, value: Val)       extends Pure
-  final case class Select(cond: Val, thenv: Val, elsev: Val)        extends Pure
+  final case class Bin(bin: nir.Bin, ty: Type, l: Val, r: Val)      extends Op
+  final case class Comp(comp: nir.Comp, ty: Type, l: Val, r: Val)   extends Op
+  final case class Conv(conv: nir.Conv, ty: Type, value: Val)       extends Op
+  final case class Select(cond: Val, thenv: Val, elsev: Val)        extends Op
 
   def Load(ty: Type, ptr: Val): Load =
     Load(ty, ptr, isVolatile = false)
diff --git a/tools/src/main/scala/scala/scalanative/codegen/Lower.scala b/tools/src/main/scala/scala/scalanative/codegen/Lower.scala
@@ -427,36 +427,91 @@ object Lower {
     def genBinOp(buf: Buffer, n: Local, op: Op.Bin, unwind: Next): Unit = {
       import buf._
 
-      op match {
-        // Detects taking remainder for division by -1 and replaces
-        // it by division by 1 which can't overflow.
-        //
-        // We implement '%' (remainder) with LLVM's 'srem' and it
-        // can overflow for cases:
-        //
-        // - Int.MinValue % -1
-        // - Long.MinValue % -1
-        //
-        // E.g. On x86_64 'srem' might get translated to 'idiv'
-        // which computes both quotient and remainder at once
-        // and quotient can overflow.
-        case sremBin @ Op.Bin(Bin.Srem, intType: Type.I, _, divisor)
-            if intType.width == 32 || intType.width == 64 =>
-          val safeDivisor         = Val.Local(fresh(), intType)
-          val thenL, elseL, contL = fresh()
+      // LLVM's division by zero is undefined behaviour. We guard
+      // the case when the divisor is zero and fail gracefully
+      // by throwing an arithmetic exception.
+      def checkDivisionByZero(op: Op.Bin): Unit = {
+        val Op.Bin(bin, ty: Type.I, dividend, divisor) = op
+
+        val thenL, elseL = fresh()
+
+        val isZero =
+          comp(Comp.Ieq, ty, divisor, Val.Zero(ty), unwind)
+        branch(isZero, Next(thenL), Next(elseL))
+
+        label(thenL)
+        call(throwDivisionByZeroTy,
+             throwDivisionByZeroVal,
+             Seq(Val.Null),
+             unwind)
+        unreachable
+
+        label(elseL)
+        if (bin == Bin.Srem || bin == Bin.Sdiv) {
+          checkDivisionOverflow(op)
+        } else {
+          let(n, op, unwind)
+        }
+      }
 
-          val isPossibleOverflow =
-            let(Op.Comp(Comp.Ieq, intType, divisor, Val.Int(-1)), unwind)
-          branch(isPossibleOverflow, Next(thenL), Next(elseL))
+      // Detects taking remainder for division by -1 and replaces
+      // it by division by 1 which can't overflow.
+      //
+      // We implement '%' (remainder) with LLVM's 'srem' and it
+      // can overflow for cases:
+      //
+      // - Int.MinValue % -1
+      // - Long.MinValue % -1
+      //
+      // E.g. On x86_64 'srem' might get translated to 'idiv'
+      // which computes both quotient and remainder at once
+      // and quotient can overflow.
+      def checkDivisionOverflow(op: Op.Bin): Unit = {
+        val Op.Bin(bin, ty: Type.I, dividend, divisor) = op
+
+        val mayOverflowL, noOverflowL, didOverflowL, resultL = fresh()
+
+        val minus1 = ty match {
+          case Type.Int  => Val.Int(-1)
+          case Type.Long => Val.Long(-1L)
+          case _         => util.unreachable
+        }
+        val minValue = ty match {
+          case Type.Int  => Val.Int(java.lang.Integer.MIN_VALUE)
+          case Type.Long => Val.Long(java.lang.Long.MIN_VALUE)
+          case _         => util.unreachable
+        }
 
-          label(thenL)
-          jump(contL, Seq(Val.Int(1)))
+        val divisorIsMinus1 =
+          let(Op.Comp(Comp.Ieq, ty, divisor, minus1), unwind)
+        branch(divisorIsMinus1, Next(mayOverflowL), Next(noOverflowL))
 
-          label(elseL)
-          jump(contL, Seq(divisor))
+        label(mayOverflowL)
+        val dividendIsMinValue =
+          let(Op.Comp(Comp.Ieq, ty, dividend, minValue), unwind)
+        branch(dividendIsMinValue, Next(didOverflowL), Next(noOverflowL))
+
+        label(didOverflowL)
+        val overflowResult = bin match {
+          case Bin.Srem => Val.Zero(ty)
+          case Bin.Sdiv => minValue
+          case _        => util.unreachable
+        }
+        jump(resultL, Seq(overflowResult))
+
+        label(noOverflowL)
+        val noOverflowResult = let(op, unwind)
+        jump(resultL, Seq(noOverflowResult))
 
-          label(contL, Seq(safeDivisor))
-          let(n, sremBin.copy(r = safeDivisor), unwind)
+        label(resultL, Seq(Val.Local(n, ty)))
+      }
+
+      op match {
+        case op @ Op.Bin(bin @ (Bin.Srem | Bin.Urem | Bin.Sdiv | Bin.Udiv),
+                         ty: Type.I,
+                         l,
+                         r) =>
+          checkDivisionByZero(op)
 
         case op =>
           let(n, op, unwind)
@@ -792,6 +847,16 @@ object Lower {
     Type.Function(Seq(Type.Ref(Global.Top("scala.scalanative.runtime.Array"))),
                   Type.Int)
 
+  val throwDivisionByZeroTy =
+    Type.Function(
+      Seq(Type.Ref(Global.Top("scala.scalanative.runtime.package$"))),
+      Type.Nothing)
+  val throwDivisionByZero =
+    Global.Member(Global.Top("scala.scalanative.runtime.package$"),
+                  Sig.Method("throwDivisionByZero", Seq(Type.Nothing)))
+  val throwDivisionByZeroVal =
+    Val.Global(throwDivisionByZero, Type.Ptr)
+
   val injects: Seq[Defn] = {
     val buf = mutable.UnrolledBuffer.empty[Defn]
     buf += Defn.Declare(Attrs.None, allocSmallName, allocSig)
@@ -824,6 +889,7 @@ object Lower {
     buf ++= arrayApply.values
     buf ++= arrayUpdateGeneric.values
     buf ++= arrayUpdate.values
+    buf += throwDivisionByZero
     buf
   }
 }
diff --git a/tools/src/main/scala/scala/scalanative/optimizer/pass/GlobalValueNumbering.scala b/tools/src/main/scala/scala/scalanative/optimizer/pass/GlobalValueNumbering.scala
@@ -101,8 +101,10 @@ object GlobalValueNumbering extends PassCompanion {
     import Op._
     op match {
       // Always idempotent:
-      case (_: Pure | _: Method | _: Dynmethod | _: As | _: Is | _: Copy |
-          _: Sizeof | _: Module | _: Box | _: Unbox | _: Arraylength) =>
+      case (_: Method | _: Dynmethod | _: As | _: Is | _: Copy | _: Sizeof |
+          _: Module | _: Box | _: Unbox | _: Arraylength) =>
+        true
+      case op if op.isPure =>
         true
 
       // Never idempotent:
diff --git a/tools/src/main/scala/scala/scalanative/sema/UseDef.scala b/tools/src/main/scala/scala/scalanative/sema/UseDef.scala
@@ -72,7 +72,7 @@ object UseDef {
       pureWhitelist.contains(name)
     case Inst.Let(_, Op.Module(name), _) =>
       pureWhitelist.contains(name)
-    case Inst.Let(_, _: Op.Pure, _) =>
+    case Inst.Let(_, op, _) if op.isPure =>
       true
     case _ =>
       false
diff --git a/unit-tests/src/test/scala/scala/DivisionByZeroSuite.scala b/unit-tests/src/test/scala/scala/DivisionByZeroSuite.scala
@@ -0,0 +1,94 @@
+package scala
+
+object DivisionByZeroSuite extends tests.Suite {
+  @noinline def byte1  = 1.toByte
+  @noinline def char1  = 1.toChar
+  @noinline def short1 = 1.toShort
+  @noinline def int1   = 1
+  @noinline def long1  = 1L
+  @noinline def byte0  = 0.toByte
+  @noinline def char0  = 0.toChar
+  @noinline def short0 = 0.toShort
+  @noinline def int0   = 0
+  @noinline def long0  = 0L
+
+  test("byte / zero") {
+    assertThrows[ArithmeticException](byte1 / byte0)
+    assertThrows[ArithmeticException](byte1 / short0)
+    assertThrows[ArithmeticException](byte1 / char0)
+    assertThrows[ArithmeticException](byte1 / int0)
+    assertThrows[ArithmeticException](byte1 / long0)
+  }
+
+  test("byte % zero") {
+    assertThrows[ArithmeticException](byte1 / byte0)
+    assertThrows[ArithmeticException](byte1 / short0)
+    assertThrows[ArithmeticException](byte1 / char0)
+    assertThrows[ArithmeticException](byte1 / int0)
+    assertThrows[ArithmeticException](byte1 / long0)
+  }
+
+  test("short / zero") {
+    assertThrows[ArithmeticException](short1 / byte0)
+    assertThrows[ArithmeticException](short1 / short0)
+    assertThrows[ArithmeticException](short1 / char0)
+    assertThrows[ArithmeticException](short1 / int0)
+    assertThrows[ArithmeticException](short1 / long0)
+  }
+
+  test("short % zero") {
+    assertThrows[ArithmeticException](short1 / byte0)
+    assertThrows[ArithmeticException](short1 / short0)
+    assertThrows[ArithmeticException](short1 / char0)
+    assertThrows[ArithmeticException](short1 / int0)
+    assertThrows[ArithmeticException](short1 / long0)
+  }
+
+  test("char / zero") {
+    assertThrows[ArithmeticException](char1 / byte0)
+    assertThrows[ArithmeticException](char1 / short0)
+    assertThrows[ArithmeticException](char1 / char0)
+    assertThrows[ArithmeticException](char1 / int0)
+    assertThrows[ArithmeticException](char1 / long0)
+  }
+
+  test("char % zero") {
+    assertThrows[ArithmeticException](char1 / byte0)
+    assertThrows[ArithmeticException](char1 / short0)
+    assertThrows[ArithmeticException](char1 / char0)
+    assertThrows[ArithmeticException](char1 / int0)
+    assertThrows[ArithmeticException](char1 / long0)
+  }
+
+  test("int / zero") {
+    assertThrows[ArithmeticException](int1 / byte0)
+    assertThrows[ArithmeticException](int1 / short0)
+    assertThrows[ArithmeticException](int1 / char0)
+    assertThrows[ArithmeticException](int1 / int0)
+    assertThrows[ArithmeticException](int1 / long0)
+  }
+
+  test("int % zero") {
+    assertThrows[ArithmeticException](int1 / byte0)
+    assertThrows[ArithmeticException](int1 / short0)
+    assertThrows[ArithmeticException](int1 / char0)
+    assertThrows[ArithmeticException](int1 / int0)
+    assertThrows[ArithmeticException](int1 / long0)
+  }
+
+  test("long / zero") {
+    assertThrows[ArithmeticException](long1 / byte0)
+    assertThrows[ArithmeticException](long1 / short0)
+    assertThrows[ArithmeticException](long1 / char0)
+    assertThrows[ArithmeticException](long1 / int0)
+    assertThrows[ArithmeticException](long1 / long0)
+  }
+
+  test("long % zero") {
+    assertThrows[ArithmeticException](long1 / byte0)
+    assertThrows[ArithmeticException](long1 / short0)
+    assertThrows[ArithmeticException](long1 / char0)
+    assertThrows[ArithmeticException](long1 / int0)
+    assertThrows[ArithmeticException](long1 / long0)
+  }
+}
diff --git a/unit-tests/src/test/scala/scala/DivisionOverflowSuite.scala b/unit-tests/src/test/scala/scala/DivisionOverflowSuite.scala
@@ -0,0 +1,23 @@
+package scala
+
+object DivisionOverflowSuite extends tests.Suite {
+  @noinline def intMinus1  = -1
+  @noinline def longMinus1 = -1L
+
+  test("Integer.MIN_VALUE / -1") {
+    assert(
+      (java.lang.Integer.MIN_VALUE / intMinus1) == java.lang.Integer.MIN_VALUE)
+  }
+
+  test("Integer.MIN_VALUE % -1") {
+    assert((java.lang.Integer.MIN_VALUE % intMinus1) == 0)
+  }
+
+  test("Long.MIN_VALUE / -1") {
+    assert((java.lang.Long.MIN_VALUE / longMinus1) == java.lang.Long.MIN_VALUE)
+  }
+
+  test("Long.MIN_VALUE % -1") {
+    assert((java.lang.Long.MIN_VALUE % longMinus1) == 0)
+  }
+}
diff --git a/unit-tests/src/test/scala/scala/scalanative/IssuesSuite.scala b/unit-tests/src/test/scala/scala/scalanative/IssuesSuite.scala
@@ -49,15 +49,14 @@ object IssuesSuite extends tests.Suite {
   }
 
   test("#314") {
-    // Division by zero is undefined behavior in production mode.
-    // Optimizer can assume it never happens and remove unused result.
+    // Division by zero is defined behavior.
     assert {
       try {
         5 / 0
-        true
+        false
       } catch {
-        case _: Throwable =>
-          false
+        case _: ArithmeticException =>
+          true
       }
     }
   }

Original file line number	Diff line number	Diff line change
`@@ -49,15 +49,14 @@ object IssuesSuite extends tests.Suite {`
`49`	`49`	`}`
`50`	`50`
`51`	`51`	`test("#314") {`
`52`		`- // Division by zero is undefined behavior in production mode.`
`53`		`- // Optimizer can assume it never happens and remove unused result.`
	`52`	`+ // Division by zero is defined behavior.`
`54`	`53`	`assert {`
`55`	`54`	`try {`
`56`	`55`	`5 / 0`
`57`		`- true`
	`56`	`+ false`
`58`	`57`	`} catch {`
`59`		`- case _: Throwable =>`
`60`		`- false`
	`58`	`+ case _: ArithmeticException =>`
	`59`	`+ true`
`61`	`60`	`}`
`62`	`61`	`}`
`63`	`62`	`}`