Merge pull request #3765 from retronym/ticket/8601

Reining on over eager optimizations (take 2)

16 files changed, 111 insertions, 6 deletions

diff --git a/src/compiler/scala/tools/nsc/backend/opt/ClosureElimination.scala b/src/compiler/scala/tools/nsc/backend/opt/ClosureElimination.scala
index c49f23852f..a866173a88 100644
--- a/src/compiler/scala/tools/nsc/backend/opt/ClosureElimination.scala
+++ b/src/compiler/scala/tools/nsc/backend/opt/ClosureElimination.scala
@@ -56,11 +56,8 @@ abstract class ClosureElimination extends SubComponent {
       case (BOX(t1), UNBOX(t2)) if (t1 == t2) =>
         Some(Nil)
 
-      case (LOAD_FIELD(sym, isStatic), DROP(_)) if !sym.hasAnnotation(definitions.VolatileAttr) =>
-        if (isStatic)
-          Some(Nil)
-        else
-          Some(DROP(REFERENCE(definitions.ObjectClass)) :: Nil)
+      case (LOAD_FIELD(sym, /* isStatic */false), DROP(_)) if !sym.hasAnnotation(definitions.VolatileAttr) && inliner.isClosureClass(sym.owner) =>
+        Some(DROP(REFERENCE(definitions.ObjectClass)) :: Nil)
 
       case _ => None
     }
diff --git a/src/compiler/scala/tools/nsc/backend/opt/DeadCodeElimination.scala b/src/compiler/scala/tools/nsc/backend/opt/DeadCodeElimination.scala
index 90c37ba0b3..4b419b210c 100644
--- a/src/compiler/scala/tools/nsc/backend/opt/DeadCodeElimination.scala
+++ b/src/compiler/scala/tools/nsc/backend/opt/DeadCodeElimination.scala
@@ -169,9 +169,14 @@ abstract class DeadCodeElimination extends SubComponent {
 
             case RETURN(_) | JUMP(_) | CJUMP(_, _, _, _) | CZJUMP(_, _, _, _) | STORE_FIELD(_, _) |
                  THROW(_)   | LOAD_ARRAY_ITEM(_) | STORE_ARRAY_ITEM(_) | SCOPE_ENTER(_) | SCOPE_EXIT(_) | STORE_THIS(_) |
-                 LOAD_EXCEPTION(_) | SWITCH(_, _) | MONITOR_ENTER() | MONITOR_EXIT() | CHECK_CAST(_) =>
+                 LOAD_EXCEPTION(_) | SWITCH(_, _) | MONITOR_ENTER() | MONITOR_EXIT() | CHECK_CAST(_) | CREATE_ARRAY(_, _) =>
               moveToWorkList()
 
+            case LOAD_FIELD(sym, isStatic) if isStatic || !inliner.isClosureClass(sym.owner) =>
+              // static load may trigger static initization.
+              // non-static load can throw NPE (but we know closure fields can't be accessed via a
+              // null reference.
+              moveToWorkList()
             case CALL_METHOD(m1, _) if isSideEffecting(m1) =>
               moveToWorkList()
 
@@ -193,6 +198,8 @@ abstract class DeadCodeElimination extends SubComponent {
               moveToWorkListIf(necessary)
             case LOAD_MODULE(sym) if isLoadNeeded(sym) =>
               moveToWorkList() // SI-4859 Module initialization might side-effect.
+            case CALL_PRIMITIVE(Arithmetic(DIV | REM, INT | LONG) | ArrayLength(_)) =>
+              moveToWorkList() // SI-8601 Might divide by zero
             case _ => ()
               moveToWorkListIf(cond = false)
           }
diff --git a/test/files/run/t8601-closure-elim.flags b/test/files/run/t8601-closure-elim.flags
new file mode 100644
index 0000000000..49d036a887
--- /dev/null
+++ b/test/files/run/t8601-closure-elim.flags
@@ -0,0 +1 @@
+-optimize
diff --git a/test/files/run/t8601-closure-elim.scala b/test/files/run/t8601-closure-elim.scala
new file mode 100644
index 0000000000..2c5b03af77
--- /dev/null
+++ b/test/files/run/t8601-closure-elim.scala
@@ -0,0 +1,26 @@
+import scala.tools.partest.BytecodeTest
+import scala.tools.asm
+import scala.tools.asm.util._
+import scala.collection.JavaConverters._
+
+object Test extends BytecodeTest {
+  val nullChecks = Set(asm.Opcodes.NEW)
+
+  def show: Unit = {
+    def test(methodName: String) {
+      val classNode = loadClassNode("Foo")
+      val methodNode = getMethod(classNode, "b")
+      val ops = methodNode.instructions.iterator.asScala.map(_.getOpcode).toList
+      assert(!ops.contains(asm.Opcodes.NEW), ops)// should be allocation free if the closure is eliminiated
+    }
+    test("b")
+  }
+}
+
+class Foo {
+  @inline final def a(x: Int => Int) = x(1)
+  final def b {
+    val delta = 0
+    a(x => delta + 1)
+  }
+}
diff --git a/test/files/run/t8601.flags b/test/files/run/t8601.flags
new file mode 100644
index 0000000000..1182725e86
--- /dev/null
+++ b/test/files/run/t8601.flags
@@ -0,0 +1 @@
+-optimize
\ No newline at end of file
diff --git a/test/files/run/t8601.scala b/test/files/run/t8601.scala
new file mode 100644
index 0000000000..e1afc23cc4
--- /dev/null
+++ b/test/files/run/t8601.scala
@@ -0,0 +1,15 @@
+object Test {
+  def idiv(x: Int): Unit = x / 0
+  def ldiv(x: Long): Unit = x / 0
+  def irem(x: Int): Unit = x % 0
+  def lrem(x: Long): Unit = x % 0
+
+  def check(x: => Any) = try { x; sys.error("failed to throw divide by zero!") } catch { case _: ArithmeticException => }
+
+  def main(args: Array[String]) {
+    check(idiv(1))
+    check(ldiv(1L))
+    check(irem(1))
+    check(lrem(1L))
+  }
+}
diff --git a/test/files/run/t8601b.flags b/test/files/run/t8601b.flags
new file mode 100644
index 0000000000..1182725e86
--- /dev/null
+++ b/test/files/run/t8601b.flags
@@ -0,0 +1 @@
+-optimize
\ No newline at end of file
diff --git a/test/files/run/t8601b.scala b/test/files/run/t8601b.scala
new file mode 100644
index 0000000000..9c37ce33d6
--- /dev/null
+++ b/test/files/run/t8601b.scala
@@ -0,0 +1,14 @@
+object Test {
+  def len(x: Array[String]): Unit = x.length
+  def load(x: Array[String]): Unit = x(0)
+  def newarray(i: Int): Unit = new Array[Int](i)
+
+  def check(x: => Any) = try { x; sys.error("failed to throw NPE!") } catch { case _: NullPointerException => }
+  def checkNegSize(x: => Any) = try { x; sys.error("failed to throw NegativeArraySizeException!") } catch { case _: NegativeArraySizeException => }
+
+  def main(args: Array[String]) {
+    check(len(null)) // bug: did not NPE
+    check(load(null))
+    checkNegSize(newarray(-1))
+  }
+}
diff --git a/test/files/run/t8601c.flags b/test/files/run/t8601c.flags
new file mode 100644
index 0000000000..1182725e86
--- /dev/null
+++ b/test/files/run/t8601c.flags
@@ -0,0 +1 @@
+-optimize
\ No newline at end of file
diff --git a/test/files/run/t8601c.scala b/test/files/run/t8601c.scala
new file mode 100644
index 0000000000..c487d6825e
--- /dev/null
+++ b/test/files/run/t8601c.scala
@@ -0,0 +1,12 @@
+object Test {
+  def loadField(x: scala.runtime.IntRef): Unit = x.elem
+  def storeField(x: scala.runtime.IntRef): Unit = x.elem = 42
+
+  def check(x: => Any) = try { x; sys.error("failed to throw NPE!") } catch { case _: NullPointerException => }
+
+  def main(args: Array[String]) {
+    check(loadField(null)) // bug: did not NPE under -Ydead-code
+    check(storeField(null))
+
+  }
+}
diff --git a/test/files/run/t8601d.flags b/test/files/run/t8601d.flags
new file mode 100644
index 0000000000..1182725e86
--- /dev/null
+++ b/test/files/run/t8601d.flags
@@ -0,0 +1 @@
+-optimize
\ No newline at end of file
diff --git a/test/files/run/t8601d.scala b/test/files/run/t8601d.scala
new file mode 100644
index 0000000000..ac89963d67
--- /dev/null
+++ b/test/files/run/t8601d.scala
@@ -0,0 +1,8 @@
+object Test {
+  def monitor(x: AnyRef): Unit = {x.synchronized(()); ()}
+  def check(x: => Any) = try { x; sys.error("failed to throw NPE") } catch { case _: NullPointerException => }
+
+  def main(args: Array[String]) {
+    check(monitor(null))
+  }
+}
diff --git a/test/files/run/t8601e.flags b/test/files/run/t8601e.flags
new file mode 100644
index 0000000000..49d036a887
--- /dev/null
+++ b/test/files/run/t8601e.flags
@@ -0,0 +1 @@
+-optimize
diff --git a/test/files/run/t8601e/StaticInit.class b/test/files/run/t8601e/StaticInit.class
new file mode 100644
index 0000000000..99a0e2a643
--- /dev/null
+++ b/test/files/run/t8601e/StaticInit.class
diff --git a/test/files/run/t8601e/StaticInit.java b/test/files/run/t8601e/StaticInit.java
new file mode 100644
index 0000000000..7543ed98b8
--- /dev/null
+++ b/test/files/run/t8601e/StaticInit.java
@@ -0,0 +1,8 @@
+public class StaticInit {
+	static {
+		if ("".isEmpty()) {
+			throw new RuntimeException();
+		}
+	}
+	public static int fld = 42;
+}
diff --git a/test/files/run/t8601e/Test.scala b/test/files/run/t8601e/Test.scala
new file mode 100644
index 0000000000..838114f6a7
--- /dev/null
+++ b/test/files/run/t8601e/Test.scala
@@ -0,0 +1,12 @@
+class C {
+  def foo: Unit = {StaticInit.fld}
+}
+
+object Test extends App {
+  try {
+    new C().foo
+    sys.error("StaticInit.<clinit> was not run!")
+  } catch {
+    case t: ExceptionInInitializerError =>
+  }
+}