Clarify why we emit ATHROW after expressions of type Nothing

Tests for emitting expressions of type Nothing.

10 files changed, 282 insertions, 7 deletions

diff --git a/src/compiler/scala/tools/nsc/backend/jvm/BCodeBodyBuilder.scala b/src/compiler/scala/tools/nsc/backend/jvm/BCodeBodyBuilder.scala
index d71d71362e..68b6f1af37 100644
--- a/src/compiler/scala/tools/nsc/backend/jvm/BCodeBodyBuilder.scala
+++ b/src/compiler/scala/tools/nsc/backend/jvm/BCodeBodyBuilder.scala
@@ -814,7 +814,51 @@ abstract class BCodeBodyBuilder extends BCodeSkelBuilder {
           case _    => bc.emitT2T(from, to)
         }
       } else if (from.isNothingType) {
-        emit(asm.Opcodes.ATHROW) // ICode enters here into enterIgnoreMode, we'll rely instead on DCE at ClassNode level.
+        /* There are two possibilities for from.isNothingType: emitting a "throw e" expressions and
+         * loading a (phantom) value of type Nothing.
+         *
+         * The Nothing type in Scala's type system does not exist in the JVM. In bytecode, Nothing
+         * is mapped to scala.runtime.Nothing$. To the JVM, a call to Predef.??? looks like it would
+         * return an object of type Nothing$. We need to do something with that phantom object on
+         * the stack. "Phantom" because it never exists: such methods always throw, but the JVM does
+         * not know that.
+         *
+         * Note: The two verifiers (old: type inference, new: type checking) have differnet
+         * requirements. Very briefly:
+         *
+         * Old (http://docs.oracle.com/javase/specs/jvms/se8/html/jvms-4.html#jvms-4.10.2.1): at
+         * each program point, no matter what branches were taken to get there
+         *   - Stack is same size and has same typed values
+         *   - Local and stack values need to have consistent types
+         *   - In practice, the old verifier seems to ignore unreachable code and accept any
+         *     instrucitons after an ATHROW. For example, there can be another ATHROW (without
+         *     loading another throwable first).
+         *
+         * New (http://docs.oracle.com/javase/specs/jvms/se8/html/jvms-4.html#jvms-4.10.1)
+         *   - Requires consistent stack map frames. GenBCode generates stack frames if -target:jvm-1.6
+         *     or higher.
+         *   - In practice: the ASM library computes stack map frames for us (ClassWriter). Emitting
+         *     correct frames after an ATHROW is probably complex, so ASM uses the following strategy:
+         *       - Every time when generating an ATHROW, a new basic block is started.
+         *       - During classfile writing, such basic blocks are found to be dead: no branches go there
+         *       - Eliminating dead code would probably require complex shifts in the output byte buffer
+         *       - But there's an easy solution: replace all code in the dead block with with
+         *         `nop; nop; ... nop; athrow`, making sure the bytecode size stays the same
+         *       - The corresponding stack frame can be easily generated: on entering a dead the block,
+         *         the frame requires a single Throwable on the stack.
+         *       - Since there are no branches to the dead block, the frame requirements are never violated.
+         *
+         * To summarize the above: it does matter what we emit after an ATHROW.
+         *
+         * NOW: if we end up here because we emitted a load of a (phantom) value of type Nothing$,
+         * there was no ATHROW emitted. So, we have to make the verifier happy and do something
+         * with that value. Since Nothing$ extends Throwable, the easiest is to just emit an ATHROW.
+         *
+         * If we ended up here because we generated a "throw e" expression, we know the last
+         * emitted instruction was an ATHROW. As explained above, it is OK to emit a second ATHROW,
+         * the verifiers will be happy.
+         */
+        emit(asm.Opcodes.ATHROW)
       } else if (from.isNullType) {
         bc drop from
         emit(asm.Opcodes.ACONST_NULL)
diff --git a/test/files/run/nothingTypeDce.flags b/test/files/run/nothingTypeDce.flags
new file mode 100644
index 0000000000..d85321ca0e
--- /dev/null
+++ b/test/files/run/nothingTypeDce.flags
@@ -0,0 +1 @@
+-target:jvm-1.6 -Ybackend:GenBCode -Yopt:unreachable-code
diff --git a/test/files/run/nothingTypeDce.scala b/test/files/run/nothingTypeDce.scala
new file mode 100644
index 0000000000..4b9d934eac
--- /dev/null
+++ b/test/files/run/nothingTypeDce.scala
@@ -0,0 +1,61 @@
+// See comment in BCodeBodyBuilder
+
+// -target:jvm-1.6 -Ybackend:GenBCode -Yopt:unreachable-code
+// target enables stack map frames generation
+
+class C {
+  // can't just emit a call to ???, that returns value of type Nothing$ (not Int).
+  def f1: Int = ???
+
+  def f2: Int = throw new Error("")
+
+  def f3(x: Boolean) = {
+    var y = 0
+    // cannot assign an object of type Nothing$ to Int
+    if (x) y = ???
+    else   y = 1
+    y
+  }
+
+  def f4(x: Boolean) = {
+    var y = 0
+    // tests that whatever is emitted after the throw is valid (what? depends on opts, presence of stack map frames)
+    if (x) y = throw new Error("")
+    else   y = 1
+    y
+  }
+
+  def f5(x: Boolean) = {
+    // stack heights need to be the smae. ??? looks to the jvm like returning a value of
+    // type Nothing$, need to drop or throw it.
+    println(
+      if (x) { ???; 10 }
+      else 20
+    )
+  }
+
+  def f6(x: Boolean) = {
+    println(
+      if (x) { throw new Error(""); 10 }
+      else 20
+    )
+  }
+
+  def f7(x: Boolean) = {
+    println(
+      if (x) throw new Error("")
+      else 20
+    )
+  }
+
+  def f8(x: Boolean) = {
+    println(
+      if (x) throw new Error("")
+      else 20
+    )
+  }
+}
+
+object Test extends App {
+  new C()
+}
diff --git a/test/files/run/nothingTypeNoFramesNoDce.check b/test/files/run/nothingTypeNoFramesNoDce.check
new file mode 100644
index 0000000000..b1d08b45ff
--- /dev/null
+++ b/test/files/run/nothingTypeNoFramesNoDce.check
@@ -0,0 +1 @@
+warning: -target:jvm-1.5 is deprecated: use target for Java 1.6 or above.
diff --git a/test/files/run/nothingTypeNoFramesNoDce.flags b/test/files/run/nothingTypeNoFramesNoDce.flags
new file mode 100644
index 0000000000..a035c86179
--- /dev/null
+++ b/test/files/run/nothingTypeNoFramesNoDce.flags
@@ -0,0 +1 @@
+-target:jvm-1.5 -Ybackend:GenBCode -Yopt:l:none -deprecation
diff --git a/test/files/run/nothingTypeNoFramesNoDce.scala b/test/files/run/nothingTypeNoFramesNoDce.scala
new file mode 100644
index 0000000000..3d1298303a
--- /dev/null
+++ b/test/files/run/nothingTypeNoFramesNoDce.scala
@@ -0,0 +1,61 @@
+// See comment in BCodeBodyBuilder
+
+// -target:jvm-1.5 -Ybackend:GenBCode -Yopt:l:none
+// target disables stack map frame generation. in this mode, the ClssWriter just emits dead code as is.
+
+class C {
+  // can't just emit a call to ???, that returns value of type Nothing$ (not Int).
+  def f1: Int = ???
+
+  def f2: Int = throw new Error("")
+
+  def f3(x: Boolean) = {
+    var y = 0
+    // cannot assign an object of type Nothing$ to Int
+    if (x) y = ???
+    else   y = 1
+    y
+  }
+
+  def f4(x: Boolean) = {
+    var y = 0
+    // tests that whatever is emitted after the throw is valid (what? depends on opts, presence of stack map frames)
+    if (x) y = throw new Error("")
+    else   y = 1
+    y
+  }
+
+  def f5(x: Boolean) = {
+    // stack heights need to be the smae. ??? looks to the jvm like returning a value of
+    // type Nothing$, need to drop or throw it.
+    println(
+      if (x) { ???; 10 }
+      else 20
+    )
+  }
+
+  def f6(x: Boolean) = {
+    println(
+      if (x) { throw new Error(""); 10 }
+      else 20
+    )
+  }
+
+  def f7(x: Boolean) = {
+    println(
+      if (x) throw new Error("")
+      else 20
+    )
+  }
+
+  def f8(x: Boolean) = {
+    println(
+      if (x) throw new Error("")
+      else 20
+    )
+  }
+}
+
+object Test extends App {
+  new C()
+}
diff --git a/test/files/run/nothingTypeNoOpt.flags b/test/files/run/nothingTypeNoOpt.flags
new file mode 100644
index 0000000000..b3b518051b
--- /dev/null
+++ b/test/files/run/nothingTypeNoOpt.flags
@@ -0,0 +1 @@
+-target:jvm-1.6 -Ybackend:GenBCode -Yopt:l:none
diff --git a/test/files/run/nothingTypeNoOpt.scala b/test/files/run/nothingTypeNoOpt.scala
new file mode 100644
index 0000000000..5c5a20fa3b
--- /dev/null
+++ b/test/files/run/nothingTypeNoOpt.scala
@@ -0,0 +1,61 @@
+// See comment in BCodeBodyBuilder
+
+// -target:jvm-1.6 -Ybackend:GenBCode -Yopt:l:none
+// target enables stack map frame generation
+
+class C {
+  // can't just emit a call to ???, that returns value of type Nothing$ (not Int).
+  def f1: Int = ???
+
+  def f2: Int = throw new Error("")
+
+  def f3(x: Boolean) = {
+    var y = 0
+    // cannot assign an object of type Nothing$ to Int
+    if (x) y = ???
+    else   y = 1
+    y
+  }
+
+  def f4(x: Boolean) = {
+    var y = 0
+    // tests that whatever is emitted after the throw is valid (what? depends on opts, presence of stack map frames)
+    if (x) y = throw new Error("")
+    else   y = 1
+    y
+  }
+
+  def f5(x: Boolean) = {
+    // stack heights need to be the smae. ??? looks to the jvm like returning a value of
+    // type Nothing$, need to drop or throw it.
+    println(
+      if (x) { ???; 10 }
+      else 20
+    )
+  }
+
+  def f6(x: Boolean) = {
+    println(
+      if (x) { throw new Error(""); 10 }
+      else 20
+    )
+  }
+
+  def f7(x: Boolean) = {
+    println(
+      if (x) throw new Error("")
+      else 20
+    )
+  }
+
+  def f8(x: Boolean) = {
+    println(
+      if (x) throw new Error("")
+      else 20
+    )
+  }
+}
+
+object Test extends App {
+  new C()
+}
diff --git a/test/junit/scala/tools/nsc/backend/jvm/CodeGenTools.scala b/test/junit/scala/tools/nsc/backend/jvm/CodeGenTools.scala
index 8518d5c832..fb677fc84b 100644
--- a/test/junit/scala/tools/nsc/backend/jvm/CodeGenTools.scala
+++ b/test/junit/scala/tools/nsc/backend/jvm/CodeGenTools.scala
@@ -42,7 +42,7 @@ object CodeGenTools {
     compiler
   }
 
-  def compile(compiler: Global = newCompiler())(code: String): List[(String, Array[Byte])] = {
+  def compile(compiler: Global)(code: String): List[(String, Array[Byte])] = {
     compiler.reporter.reset()
     resetOutput(compiler)
     val run = new compiler.Run()
@@ -51,11 +51,16 @@ object CodeGenTools {
     (for (f <- outDir.iterator if !f.isDirectory) yield (f.name, f.toByteArray)).toList
   }
 
-  def compileClasses(compiler: Global = newCompiler())(code: String): List[ClassNode] = {
+  def compileClasses(compiler: Global)(code: String): List[ClassNode] = {
     compile(compiler)(code).map(p => AsmUtils.readClass(p._2)).sortBy(_.name)
   }
 
-  def compileMethods(compiler: Global = newCompiler())(code: String): List[MethodNode] = {
+  def compileMethods(compiler: Global)(code: String): List[MethodNode] = {
     compileClasses(compiler)(s"class C { $code }").head.methods.asScala.toList.filterNot(_.name == "<init>")
   }
+
+  def singleMethodInstructions(compiler: Global)(code: String): List[Instruction] = {
+    val List(m) = compileMethods(compiler)(code)
+    instructionsFromMethod(m)
+  }
 }
diff --git a/test/junit/scala/tools/nsc/backend/jvm/opt/UnreachableCodeTest.scala b/test/junit/scala/tools/nsc/backend/jvm/opt/UnreachableCodeTest.scala
index 4e62cc64e4..ae08d10b79 100644
--- a/test/junit/scala/tools/nsc/backend/jvm/opt/UnreachableCodeTest.scala
+++ b/test/junit/scala/tools/nsc/backend/jvm/opt/UnreachableCodeTest.scala
@@ -18,6 +18,14 @@ import ASMConverters._
 class UnreachableCodeTest {
   import UnreachableCodeTest._
 
+  // jvm-1.6 enables emitting stack map frames, which impacts the code generation wrt dead basic blocks,
+  // see comment in BCodeBodyBuilder
+  val dceCompiler = newCompiler(extraArgs = "-target:jvm-1.6 -Ybackend:GenBCode -Yopt:unreachable-code")
+  val noOptCompiler = newCompiler(extraArgs = "-target:jvm-1.6 -Ybackend:GenBCode -Yopt:l:none")
+
+  // jvm-1.5 disables computing stack map frames, and it emits dead code as-is.
+  val noOptNoFramesCompiler = newCompiler(extraArgs = "-target:jvm-1.5 -Ybackend:GenBCode -Yopt:l:none")
+
   @Test
   def basicElimination(): Unit = {
     assertEliminateDead(
@@ -97,6 +105,36 @@ class UnreachableCodeTest {
   }
 
   @Test
+  def basicEliminationCompiler(): Unit = {
+    val code = "def f: Int = { return 1; 2 }"
+    val withDce = singleMethodInstructions(dceCompiler)(code)
+    assertSameCode(withDce.dropNonOp, List(Op(ICONST_1), Op(IRETURN)))
+
+    val noDce = singleMethodInstructions(noOptCompiler)(code)
+
+    // The emitted code is ICONST_1, IRETURN, ICONST_2, IRETURN. The latter two are dead.
+    //
+    // GenBCode puts the last IRETURN into a new basic block: it emits a label before the second
+    // IRETURN. This is an implementation detail, it may change; it affects the outcome of this test.
+    //
+    // During classfile writing with COMPUTE_FAMES (-target:jvm-1.6 or larger), the ClassfileWriter
+    // puts the ICONST_2 into a new basic block, because the preceding operation (IRETURN) ends
+    // the current block. We get something like
+    //
+    //   L1: ICONST_1; IRETURN
+    //   L2: ICONST_2            << dead
+    //   L3: IRETURN             << dead
+    //
+    // Finally, instructions in the dead basic blocks are replaced by ATHROW, as explained in
+    // a comment in BCodeBodyBuilder.
+    assertSameCode(noDce.dropNonOp, List(Op(ICONST_1), Op(IRETURN), Op(ATHROW), Op(ATHROW)))
+
+    // when NOT computing stack map frames, ASM's ClassWriter does not replace dead code by NOP/ATHROW
+    val noDceNoFrames = singleMethodInstructions(noOptNoFramesCompiler)(code)
+    assertSameCode(noDceNoFrames.dropNonOp, List(Op(ICONST_1), Op(IRETURN), Op(ICONST_2), Op(IRETURN)))
+  }
+
+  @Test
   def metaTest(): Unit = {
     assertEliminateDead() // no instructions
 
@@ -153,10 +191,11 @@ object UnreachableCodeTest {
     val cls = wrapInClass(genMethod()(code.map(_._1): _*))
     LocalOpt.removeUnreachableCode(cls)
     val nonEliminated = instructionsFromMethod(cls.methods.get(0))
-
     val expectedLive = code.filter(_._2).map(_._1).toList
-
-    assertTrue(s"\nExpected: $expectedLive\nActual  : $nonEliminated", nonEliminated === expectedLive)
+    assertSameCode(nonEliminated, expectedLive)
   }
 
+  def assertSameCode(actual: List[Instruction], expected: List[Instruction]): Unit = {
+    assertTrue(s"\nExpected: $expected\nActual  : $actual", actual === expected)
+  }
 }