From c9ec916f20c4f06a0aebe0a9929443c1c8b60c5c Mon Sep 17 00:00:00 2001 From: Som Snytt Date: Wed, 20 Aug 2014 10:25:28 -0700 Subject: SI-8806 Add lower bound check to Any lint We already exclude the lint check for infer-any if Any is somewhere explicit. This commit adds lower bounds of type params to the somewheres. Motivated by: ``` scala> f"${42}" :8: warning: a type was inferred to be `Any`; this may indicate a programming error. f"${42}" ^ res0: String = 42 ``` --- test/files/neg/warn-inferred-any.check | 5 ++++- test/files/neg/warn-inferred-any.scala | 8 ++++++++ 2 files changed, 12 insertions(+), 1 deletion(-) (limited to 'test') diff --git a/test/files/neg/warn-inferred-any.check b/test/files/neg/warn-inferred-any.check index 4628033e55..8ad81d1529 100644 --- a/test/files/neg/warn-inferred-any.check +++ b/test/files/neg/warn-inferred-any.check @@ -7,6 +7,9 @@ warn-inferred-any.scala:16: warning: a type was inferred to be `AnyVal`; this ma warn-inferred-any.scala:17: warning: a type was inferred to be `AnyVal`; this may indicate a programming error. { 1l to 5l contains 5d } ^ +warn-inferred-any.scala:25: warning: a type was inferred to be `Any`; this may indicate a programming error. + def za = f(1, "one") + ^ error: No warnings can be incurred under -Xfatal-warnings. -three warnings found +four warnings found one error found diff --git a/test/files/neg/warn-inferred-any.scala b/test/files/neg/warn-inferred-any.scala index b853e6e5a8..693c33e7be 100644 --- a/test/files/neg/warn-inferred-any.scala +++ b/test/files/neg/warn-inferred-any.scala @@ -17,3 +17,11 @@ trait Ys[+A] { { 1l to 5l contains 5d } { 1l to 5l contains 5l } } + +trait Zs { + def f[A](a: A*) = 42 + def g[A >: Any](a: A*) = 42 // don't warn + + def za = f(1, "one") + def zu = g(1, "one") +} -- cgit v1.2.3 From 44b5c261a8e585c5747380895aa06c84f9d63f6c Mon Sep 17 00:00:00 2001 From: Lukas Rytz Date: Mon, 25 Aug 2014 23:13:05 +0200 Subject: JUnit tests for dead code elimination. JUnit tests may use tools from partest-extras (ASMConverters) --- build.xml | 1 + .../scala/tools/partest/ASMConverters.scala | 209 ++++++++++++++++----- .../scala/tools/partest/BytecodeTest.scala | 30 ++- test/files/jvm/t6941/test.scala | 4 +- test/files/jvm/t7253/test.scala | 6 +- .../scala/tools/nsc/backend/jvm/BTypesTest.scala | 1 - .../scala/tools/nsc/backend/jvm/CodeGenTools.scala | 27 +++ .../nsc/backend/jvm/opt/UnreachableCodeTest.scala | 162 ++++++++++++++++ .../scala/tools/testing/AssertThrowsTest.scala | 11 +- 9 files changed, 379 insertions(+), 72 deletions(-) create mode 100644 test/junit/scala/tools/nsc/backend/jvm/CodeGenTools.scala create mode 100644 test/junit/scala/tools/nsc/backend/jvm/opt/UnreachableCodeTest.scala (limited to 'test') diff --git a/build.xml b/build.xml index 6c750e530d..b43d8a826b 100755 --- a/build.xml +++ b/build.xml @@ -970,6 +970,7 @@ TODO: + diff --git a/src/partest-extras/scala/tools/partest/ASMConverters.scala b/src/partest-extras/scala/tools/partest/ASMConverters.scala index d618e086f4..50057d058c 100644 --- a/src/partest-extras/scala/tools/partest/ASMConverters.scala +++ b/src/partest-extras/scala/tools/partest/ASMConverters.scala @@ -2,70 +2,181 @@ package scala.tools.partest import scala.collection.JavaConverters._ import scala.tools.asm -import asm.tree.{ClassNode, MethodNode, InsnList} +import asm.{tree => t} /** Makes using ASM from ByteCodeTests more convenient. * * Wraps ASM instructions in case classes so that equals and toString work * for the purpose of bytecode diffing and pretty printing. */ -trait ASMConverters { - // wrap ASM's instructions so we get case class-style `equals` and `toString` - object instructions { - def fromMethod(meth: MethodNode): List[Instruction] = { - val insns = meth.instructions - val asmToScala = new AsmToScala{ def labelIndex(l: asm.tree.AbstractInsnNode) = insns.indexOf(l) } - - asmToScala.mapOver(insns.iterator.asScala.toList).asInstanceOf[List[Instruction]] +object ASMConverters { + + /** + * Transform the instructions of an ASM Method into a list of [[Instruction]]s. + */ + def instructionsFromMethod(meth: t.MethodNode): List[Instruction] = { + val insns = meth.instructions + val asmToScala = new AsmToScala { + def labelIndex(l: t.LabelNode) = insns.indexOf(l) } + asmToScala.convert(insns.iterator.asScala.toList) + } + + implicit class CompareInstructionLists(val self: List[Instruction]) { + def === (other: List[Instruction]) = equivalentBytecode(self, other) + } - sealed abstract class Instruction { def opcode: String } - case class Field (opcode: String, desc: String, name: String, owner: String) extends Instruction - case class Incr (opcode: String, incr: Int, `var`: Int) extends Instruction - case class Op (opcode: String) extends Instruction - case class IntOp (opcode: String, operand: Int) extends Instruction - case class Jump (opcode: String, label: Label) extends Instruction - case class Ldc (opcode: String, cst: Any) extends Instruction - case class LookupSwitch (opcode: String, dflt: Label, keys: List[Integer], labels: List[Label]) extends Instruction - case class TableSwitch (opcode: String, dflt: Label, max: Int, min: Int, labels: List[Label]) extends Instruction - case class Method (opcode: String, desc: String, name: String, owner: String) extends Instruction - case class NewArray (opcode: String, desc: String, dims: Int) extends Instruction - case class TypeOp (opcode: String, desc: String) extends Instruction - case class VarOp (opcode: String, `var`: Int) extends Instruction - case class Label (offset: Int) extends Instruction { def opcode: String = "" } - case class FrameEntry (local: List[Any], stack: List[Any]) extends Instruction { def opcode: String = "" } - case class LineNumber (line: Int, start: Label) extends Instruction { def opcode: String = "" } + sealed abstract class Instruction extends Product { + def opcode: Int + + // toString such that the first field, "opcode: Int", is printed textually. + final override def toString() = { + import scala.tools.asm.util.Printer.OPCODES + def opString(op: Int) = if (OPCODES.isDefinedAt(op)) OPCODES(op) else "?" + val printOpcode = opcode != -1 + + productPrefix + ( + if (printOpcode) Iterator(opString(opcode)) ++ productIterator.drop(1) + else productIterator + ).mkString("(", ", ", ")") + } } + case class Field (opcode: Int, owner: String, name: String, desc: String) extends Instruction + case class Incr (opcode: Int, `var`: Int, incr: Int) extends Instruction + case class Op (opcode: Int) extends Instruction + case class IntOp (opcode: Int, operand: Int) extends Instruction + case class Jump (opcode: Int, label: Label) extends Instruction + case class Ldc (opcode: Int, cst: Any) extends Instruction + case class LookupSwitch(opcode: Int, dflt: Label, keys: List[Int], labels: List[Label]) extends Instruction + case class TableSwitch (opcode: Int, min: Int, max: Int, dflt: Label, labels: List[Label]) extends Instruction + case class Method (opcode: Int, owner: String, name: String, desc: String, itf: Boolean) extends Instruction + case class NewArray (opcode: Int, desc: String, dims: Int) extends Instruction + case class TypeOp (opcode: Int, desc: String) extends Instruction + case class VarOp (opcode: Int, `var`: Int) extends Instruction + case class Label (offset: Int) extends Instruction { def opcode: Int = -1 } + case class FrameEntry (`type`: Int, local: List[Any], stack: List[Any]) extends Instruction { def opcode: Int = -1 } + case class LineNumber (line: Int, start: Label) extends Instruction { def opcode: Int = -1 } + abstract class AsmToScala { - import instructions._ - def labelIndex(l: asm.tree.AbstractInsnNode): Int + def labelIndex(l: t.LabelNode): Int - def mapOver(is: List[Any]): List[Any] = is map { - case i: asm.tree.AbstractInsnNode => apply(i) + def op(i: t.AbstractInsnNode): Int = i.getOpcode + + def convert(instructions: List[t.AbstractInsnNode]): List[Instruction] = instructions map apply + + private def lst[T](xs: java.util.List[T]): List[T] = if (xs == null) Nil else xs.asScala.toList + + // Heterogenous List[Any] is used in FrameNode: type information about locals / stack values + // are stored in a List[Any] (Integer, String or LabelNode), see Javadoc of MethodNode#visitFrame. + // Opcodes (eg Opcodes.INTEGER) and Reference types (eg "java/lang/Object") are returned unchanged, + // LabelNodes are mapped to their LabelEntry. + private def mapOverFrameTypes(is: List[Any]): List[Any] = is map { + case i: t.LabelNode => applyLabel(i) case x => x } - def op(i: asm.tree.AbstractInsnNode) = if (asm.util.Printer.OPCODES.isDefinedAt(i.getOpcode)) asm.util.Printer.OPCODES(i.getOpcode) else "?" - def lst[T](xs: java.util.List[T]): List[T] = if (xs == null) Nil else xs.asScala.toList - def apply(l: asm.tree.LabelNode): Label = this(l: asm.tree.AbstractInsnNode).asInstanceOf[Label] - def apply(x: asm.tree.AbstractInsnNode): Instruction = x match { - case i: asm.tree.FieldInsnNode => Field (op(i), i.desc: String, i.name: String, i.owner: String) - case i: asm.tree.IincInsnNode => Incr (op(i), i.incr: Int, i.`var`: Int) - case i: asm.tree.InsnNode => Op (op(i)) - case i: asm.tree.IntInsnNode => IntOp (op(i), i.operand: Int) - case i: asm.tree.JumpInsnNode => Jump (op(i), this(i.label)) - case i: asm.tree.LdcInsnNode => Ldc (op(i), i.cst: Any) - case i: asm.tree.LookupSwitchInsnNode => LookupSwitch (op(i), this(i.dflt), lst(i.keys), mapOver(lst(i.labels)).asInstanceOf[List[Label]]) - case i: asm.tree.TableSwitchInsnNode => TableSwitch (op(i), this(i.dflt), i.max: Int, i.min: Int, mapOver(lst(i.labels)).asInstanceOf[List[Label]]) - case i: asm.tree.MethodInsnNode => Method (op(i), i.desc: String, i.name: String, i.owner: String) - case i: asm.tree.MultiANewArrayInsnNode => NewArray (op(i), i.desc: String, i.dims: Int) - case i: asm.tree.TypeInsnNode => TypeOp (op(i), i.desc: String) - case i: asm.tree.VarInsnNode => VarOp (op(i), i.`var`: Int) - case i: asm.tree.LabelNode => Label (labelIndex(x)) - case i: asm.tree.FrameNode => FrameEntry (mapOver(lst(i.local)), mapOver(lst(i.stack))) - case i: asm.tree.LineNumberNode => LineNumber (i.line: Int, this(i.start): Label) + // avoids some casts + private def applyLabel(l: t.LabelNode) = this(l: t.AbstractInsnNode).asInstanceOf[Label] + + private def apply(x: t.AbstractInsnNode): Instruction = x match { + case i: t.FieldInsnNode => Field (op(i), i.owner, i.name, i.desc) + case i: t.IincInsnNode => Incr (op(i), i.`var`, i.incr) + case i: t.InsnNode => Op (op(i)) + case i: t.IntInsnNode => IntOp (op(i), i.operand) + case i: t.JumpInsnNode => Jump (op(i), applyLabel(i.label)) + case i: t.LdcInsnNode => Ldc (op(i), i.cst: Any) + case i: t.LookupSwitchInsnNode => LookupSwitch (op(i), applyLabel(i.dflt), lst(i.keys) map (x => x: Int), lst(i.labels) map applyLabel) + case i: t.TableSwitchInsnNode => TableSwitch (op(i), i.min, i.max, applyLabel(i.dflt), lst(i.labels) map applyLabel) + case i: t.MethodInsnNode => Method (op(i), i.desc, i.name, i.owner, i.itf) + case i: t.MultiANewArrayInsnNode => NewArray (op(i), i.desc, i.dims) + case i: t.TypeInsnNode => TypeOp (op(i), i.desc) + case i: t.VarInsnNode => VarOp (op(i), i.`var`) + case i: t.LabelNode => Label (labelIndex(i)) + case i: t.FrameNode => FrameEntry (i.`type`, mapOverFrameTypes(lst(i.local)), mapOverFrameTypes(lst(i.stack))) + case i: t.LineNumberNode => LineNumber (i.line, applyLabel(i.start)) + } + } + + import collection.mutable.{Map => MMap} + + /** + * Bytecode is equal modula local variable numbering and label numbering. + */ + def equivalentBytecode(as: List[Instruction], bs: List[Instruction], varMap: MMap[Int, Int] = MMap(), labelMap: MMap[Int, Int] = MMap()): Boolean = { + def same(v1: Int, v2: Int, m: MMap[Int, Int]) = { + if (m contains v1) m(v1) == v2 + else if (m.valuesIterator contains v2) false // v2 is already associated with some different value v1 + else { m(v1) = v2; true } } + def sameVar(v1: Int, v2: Int) = same(v1, v2, varMap) + def sameLabel(l1: Label, l2: Label) = same(l1.offset, l2.offset, labelMap) + def sameLabels(ls1: List[Label], ls2: List[Label]) = ls1.length == ls2.length && (ls1, ls2).zipped.forall(sameLabel) + + def sameFrameTypes(ts1: List[Any], ts2: List[Any]) = ts1.length == ts2.length && (ts1, ts2).zipped.forall { + case (t1: Label, t2: Label) => sameLabel(t1, t2) + case (x, y) => x == y + } + + if (as.isEmpty) bs.isEmpty + else if (bs.isEmpty) false + else ((as.head, bs.head) match { + case (VarOp(op1, v1), VarOp(op2, v2)) => op1 == op2 && sameVar(v1, v2) + case (Incr(op1, v1, inc1), Incr(op2, v2, inc2)) => op1 == op2 && sameVar(v1, v2) && inc1 == inc2 + + case (l1 @ Label(_), l2 @ Label(_)) => sameLabel(l1, l2) + case (Jump(op1, l1), Jump(op2, l2)) => op1 == op2 && sameLabel(l1, l2) + case (LookupSwitch(op1, l1, keys1, ls1), LookupSwitch(op2, l2, keys2, ls2)) => op1 == op2 && sameLabel(l1, l2) && keys1 == keys2 && sameLabels(ls1, ls2) + case (TableSwitch(op1, min1, max1, l1, ls1), TableSwitch(op2, min2, max2, l2, ls2)) => op1 == op2 && min1 == min2 && max1 == max2 && sameLabel(l1, l2) && sameLabels(ls1, ls2) + case (LineNumber(line1, l1), LineNumber(line2, l2)) => line1 == line2 && sameLabel(l1, l2) + case (FrameEntry(tp1, loc1, stk1), FrameEntry(tp2, loc2, stk2)) => tp1 == tp2 && sameFrameTypes(loc1, loc2) && sameFrameTypes(stk1, stk2) + + // this needs to go after the above. For example, Label(1) may not equal Label(1), if before + // the left 1 was associated with another right index. + case (a, b) if a == b => true + + case _ => false + }) && equivalentBytecode(as.tail, bs.tail, varMap, labelMap) + } + + /** + * Convert back a list of [[Instruction]]s to ASM land. The code is emitted into the parameter + * `method`. + */ + def applyToMethod(method: t.MethodNode, instructions: List[Instruction]): Unit = { + val asmLabel = createLabelNodes(instructions) + instructions.foreach(visitMethod(method, _, asmLabel)) + } + + private def createLabelNodes(instructions: List[Instruction]): Map[Label, asm.Label] = { + val labels = instructions collect { + case l: Label => l + } + assert(labels.distinct == labels, s"Duplicate labels in: $labels") + labels.map(l => (l, new asm.Label())).toMap + } + + private def frameTypesToAsm(l: List[Any], asmLabel: Map[Label, asm.Label]): List[Object] = l map { + case l: Label => asmLabel(l) + case x => x.asInstanceOf[Object] + } + + private def visitMethod(method: t.MethodNode, instruction: Instruction, asmLabel: Map[Label, asm.Label]): Unit = instruction match { + case Field(op, owner, name, desc) => method.visitFieldInsn(op, owner, name, desc) + case Incr(op, vr, incr) => method.visitIincInsn(vr, incr) + case Op(op) => method.visitInsn(op) + case IntOp(op, operand) => method.visitIntInsn(op, operand) + case Jump(op, label) => method.visitJumpInsn(op, asmLabel(label)) + case Ldc(op, cst) => method.visitLdcInsn(cst) + case LookupSwitch(op, dflt, keys, labels) => method.visitLookupSwitchInsn(asmLabel(dflt), keys.toArray, (labels map asmLabel).toArray) + case TableSwitch(op, min, max, dflt, labels) => method.visitTableSwitchInsn(min, max, asmLabel(dflt), (labels map asmLabel).toArray: _*) + case Method(op, owner, name, desc, itf) => method.visitMethodInsn(op, owner, name, desc, itf) + case NewArray(op, desc, dims) => method.visitMultiANewArrayInsn(desc, dims) + case TypeOp(op, desc) => method.visitTypeInsn(op, desc) + case VarOp(op, vr) => method.visitVarInsn(op, vr) + case l: Label => method.visitLabel(asmLabel(l)) + case FrameEntry(tp, local, stack) => method.visitFrame(tp, local.length, frameTypesToAsm(local, asmLabel).toArray, stack.length, frameTypesToAsm(stack, asmLabel).toArray) + case LineNumber(line, start) => method.visitLineNumber(line, asmLabel(start)) } -} \ No newline at end of file +} diff --git a/src/partest-extras/scala/tools/partest/BytecodeTest.scala b/src/partest-extras/scala/tools/partest/BytecodeTest.scala index 1e4362fcde..c6fa279c50 100644 --- a/src/partest-extras/scala/tools/partest/BytecodeTest.scala +++ b/src/partest-extras/scala/tools/partest/BytecodeTest.scala @@ -3,7 +3,7 @@ package scala.tools.partest import scala.tools.nsc.util.JavaClassPath import scala.collection.JavaConverters._ import scala.tools.asm.{ClassWriter, ClassReader} -import scala.tools.asm.tree.{ClassNode, MethodNode, InsnList} +import scala.tools.asm.tree._ import java.io.{FileOutputStream, FileInputStream, File => JFile, InputStream} import AsmNode._ @@ -28,8 +28,8 @@ import AsmNode._ * See test/files/jvm/bytecode-test-example for an example of bytecode test. * */ -abstract class BytecodeTest extends ASMConverters { - import instructions._ +abstract class BytecodeTest { + import ASMConverters._ /** produce the output to be compared against a checkfile */ protected def show(): Unit @@ -38,8 +38,8 @@ abstract class BytecodeTest extends ASMConverters { // asserts def sameBytecode(methA: MethodNode, methB: MethodNode) = { - val isa = instructions.fromMethod(methA) - val isb = instructions.fromMethod(methB) + val isa = instructionsFromMethod(methA) + val isb = instructionsFromMethod(methB) if (isa == isb) println("bytecode identical") else diffInstructions(isa, isb) } @@ -81,18 +81,16 @@ abstract class BytecodeTest extends ASMConverters { } } - // bytecode is equal modulo local variable numbering - def equalsModuloVar(a: Instruction, b: Instruction) = (a, b) match { - case _ if a == b => true - case (VarOp(op1, _), VarOp(op2, _)) if op1 == op2 => true - case _ => false - } - - def similarBytecode(methA: MethodNode, methB: MethodNode, similar: (Instruction, Instruction) => Boolean) = { - val isa = fromMethod(methA) - val isb = fromMethod(methB) + /** + * Compare the bytecodes of two methods. + * + * The for the `similar` function, you probably want to pass [[ASMConverters.equivalentBytecode]]. + */ + def similarBytecode(methA: MethodNode, methB: MethodNode, similar: (List[Instruction], List[Instruction]) => Boolean) = { + val isa = instructionsFromMethod(methA) + val isb = instructionsFromMethod(methB) if (isa == isb) println("bytecode identical") - else if ((isa, isb).zipped.forall { case (a, b) => similar(a, b) }) println("bytecode similar") + else if (similar(isa, isb)) println("bytecode similar") else diffInstructions(isa, isb) } diff --git a/test/files/jvm/t6941/test.scala b/test/files/jvm/t6941/test.scala index 248617f71f..fceb54487f 100644 --- a/test/files/jvm/t6941/test.scala +++ b/test/files/jvm/t6941/test.scala @@ -1,4 +1,4 @@ -import scala.tools.partest.BytecodeTest +import scala.tools.partest.{BytecodeTest, ASMConverters} import scala.tools.nsc.util.JavaClassPath import java.io.InputStream @@ -10,6 +10,6 @@ import scala.collection.JavaConverters._ object Test extends BytecodeTest { def show: Unit = { val classNode = loadClassNode("SameBytecode") - similarBytecode(getMethod(classNode, "a"), getMethod(classNode, "b"), equalsModuloVar) + similarBytecode(getMethod(classNode, "a"), getMethod(classNode, "b"), ASMConverters.equivalentBytecode(_, _)) } } diff --git a/test/files/jvm/t7253/test.scala b/test/files/jvm/t7253/test.scala index 7fe08e8813..a3f1e86e65 100644 --- a/test/files/jvm/t7253/test.scala +++ b/test/files/jvm/t7253/test.scala @@ -1,4 +1,4 @@ -import scala.tools.partest.BytecodeTest +import scala.tools.partest.{BytecodeTest, ASMConverters} import scala.tools.nsc.util.JavaClassPath import java.io.InputStream @@ -8,10 +8,10 @@ import asm.tree.{ClassNode, InsnList} import scala.collection.JavaConverters._ object Test extends BytecodeTest { - import instructions._ + import ASMConverters._ def show: Unit = { - val instrBaseSeqs = Seq("ScalaClient_1", "JavaClient_1") map (name => instructions.fromMethod(getMethod(loadClassNode(name), "foo"))) + val instrBaseSeqs = Seq("ScalaClient_1", "JavaClient_1") map (name => instructionsFromMethod(getMethod(loadClassNode(name), "foo"))) val instrSeqs = instrBaseSeqs map (_ filter isInvoke) cmpInstructions(instrSeqs(0), instrSeqs(1)) } diff --git a/test/junit/scala/tools/nsc/backend/jvm/BTypesTest.scala b/test/junit/scala/tools/nsc/backend/jvm/BTypesTest.scala index cb7e7050b0..221aad6536 100644 --- a/test/junit/scala/tools/nsc/backend/jvm/BTypesTest.scala +++ b/test/junit/scala/tools/nsc/backend/jvm/BTypesTest.scala @@ -1,7 +1,6 @@ package scala.tools.nsc package backend.jvm -import scala.tools.testing.AssertUtil._ import org.junit.runner.RunWith import org.junit.runners.JUnit4 import org.junit.Test diff --git a/test/junit/scala/tools/nsc/backend/jvm/CodeGenTools.scala b/test/junit/scala/tools/nsc/backend/jvm/CodeGenTools.scala new file mode 100644 index 0000000000..256dff85c3 --- /dev/null +++ b/test/junit/scala/tools/nsc/backend/jvm/CodeGenTools.scala @@ -0,0 +1,27 @@ +package scala.tools.nsc.backend.jvm + +import scala.tools.asm.Opcodes +import scala.tools.asm.tree.{AbstractInsnNode, LabelNode, ClassNode, MethodNode} +import scala.tools.partest.ASMConverters +import scala.collection.JavaConverters._ + +object CodeGenTools { + import ASMConverters._ + + def genMethod( flags: Int = Opcodes.ACC_PUBLIC, + name: String = "m", + descriptor: String = "()V", + genericSignature: String = null, + throwsExceptions: Array[String] = null)(body: Instruction*): MethodNode = { + val node = new MethodNode(flags, name, descriptor, genericSignature, throwsExceptions) + applyToMethod(node, body.toList) + node + } + + def wrapInClass(method: MethodNode): ClassNode = { + val cls = new ClassNode() + cls.visit(Opcodes.V1_6, Opcodes.ACC_PUBLIC, "C", null, "java/lang/Object", null) + cls.methods.add(method) + cls + } +} diff --git a/test/junit/scala/tools/nsc/backend/jvm/opt/UnreachableCodeTest.scala b/test/junit/scala/tools/nsc/backend/jvm/opt/UnreachableCodeTest.scala new file mode 100644 index 0000000000..4e62cc64e4 --- /dev/null +++ b/test/junit/scala/tools/nsc/backend/jvm/opt/UnreachableCodeTest.scala @@ -0,0 +1,162 @@ +package scala.tools.nsc +package backend.jvm +package opt + +import org.junit.runner.RunWith +import org.junit.runners.JUnit4 +import org.junit.Test +import scala.tools.asm.Opcodes._ +import org.junit.Assert._ + +import scala.tools.testing.AssertUtil._ + +import CodeGenTools._ +import scala.tools.partest.ASMConverters +import ASMConverters._ + +@RunWith(classOf[JUnit4]) +class UnreachableCodeTest { + import UnreachableCodeTest._ + + @Test + def basicElimination(): Unit = { + assertEliminateDead( + Op(ACONST_NULL), + Op(ATHROW), + Op(RETURN).dead + ) + + assertEliminateDead( + Op(RETURN) + ) + + assertEliminateDead( + Op(RETURN), + Op(ACONST_NULL).dead, + Op(ATHROW).dead + ) + } + + @Test + def eliminateNop(): Unit = { + assertEliminateDead( + // not dead, since visited by data flow analysis. need a different opt to eliminate it. + Op(NOP), + Op(RETURN), + Op(NOP).dead + ) + } + + @Test + def eliminateBranchOver(): Unit = { + assertEliminateDead( + Jump(GOTO, Label(1)), + Op(ACONST_NULL).dead, + Op(ATHROW).dead, + Label(1), + Op(RETURN) + ) + + assertEliminateDead( + Jump(GOTO, Label(1)), + Label(1), + Op(RETURN) + ) + } + + @Test + def deadLabelsRemain(): Unit = { + assertEliminateDead( + Op(RETURN), + Jump(GOTO, Label(1)).dead, + // not dead - labels may be referenced from other places in a classfile (eg exceptions table). + // will need a different opt to get rid of them + Label(1) + ) + } + + @Test + def pushPopNotEliminated(): Unit = { + assertEliminateDead( + // not dead, visited by data flow analysis. + Op(ACONST_NULL), + Op(POP), + Op(RETURN) + ) + } + + @Test + def nullnessNotConsidered(): Unit = { + assertEliminateDead( + Op(ACONST_NULL), + Jump(IFNULL, Label(1)), + Op(RETURN), // not dead + Label(1), + Op(RETURN) + ) + } + + @Test + def metaTest(): Unit = { + assertEliminateDead() // no instructions + + assertThrows[AssertionError]( + assertEliminateDead(Op(RETURN).dead), + _.contains("Expected: List()\nActual : List(Op(RETURN))") + ) + + assertThrows[AssertionError]( + assertEliminateDead(Op(RETURN), Op(RETURN)), + _.contains("Expected: List(Op(RETURN), Op(RETURN))\nActual : List(Op(RETURN))") + ) + } + + @Test + def bytecodeEquivalence: Unit = { + assertTrue(List(VarOp(ILOAD, 1)) === + List(VarOp(ILOAD, 2))) + assertTrue(List(VarOp(ILOAD, 1), VarOp(ISTORE, 1)) === + List(VarOp(ILOAD, 2), VarOp(ISTORE, 2))) + + // the first Op will associate 1->2, then the 2->2 will fail + assertFalse(List(VarOp(ILOAD, 1), VarOp(ISTORE, 2)) === + List(VarOp(ILOAD, 2), VarOp(ISTORE, 2))) + + // will associate 1->2 and 2->1, which is OK + assertTrue(List(VarOp(ILOAD, 1), VarOp(ISTORE, 2)) === + List(VarOp(ILOAD, 2), VarOp(ISTORE, 1))) + + assertTrue(List(Label(1), Label(2), Label(1)) === + List(Label(2), Label(4), Label(2))) + assertTrue(List(LineNumber(1, Label(1)), Label(1)) === + List(LineNumber(1, Label(3)), Label(3))) + assertFalse(List(LineNumber(1, Label(1)), Label(1)) === + List(LineNumber(1, Label(3)), Label(1))) + + assertTrue(List(TableSwitch(TABLESWITCH, 1, 3, Label(4), List(Label(5), Label(6))), Label(4), Label(5), Label(6)) === + List(TableSwitch(TABLESWITCH, 1, 3, Label(9), List(Label(3), Label(4))), Label(9), Label(3), Label(4))) + + assertTrue(List(FrameEntry(F_FULL, List(INTEGER, DOUBLE, Label(3)), List("java/lang/Object", Label(4))), Label(3), Label(4)) === + List(FrameEntry(F_FULL, List(INTEGER, DOUBLE, Label(1)), List("java/lang/Object", Label(3))), Label(1), Label(3))) + } +} + +object UnreachableCodeTest { + import scala.language.implicitConversions + implicit def aliveInstruction(ins: Instruction): (Instruction, Boolean) = (ins, true) + + implicit class MortalInstruction(val ins: Instruction) extends AnyVal { + def dead: (Instruction, Boolean) = (ins, false) + } + + def assertEliminateDead(code: (Instruction, Boolean)*): Unit = { + 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) + } + +} diff --git a/test/junit/scala/tools/testing/AssertThrowsTest.scala b/test/junit/scala/tools/testing/AssertThrowsTest.scala index a70519e63c..d91e450bac 100644 --- a/test/junit/scala/tools/testing/AssertThrowsTest.scala +++ b/test/junit/scala/tools/testing/AssertThrowsTest.scala @@ -31,4 +31,13 @@ class AssertThrowsTest { } }) -} \ No newline at end of file + @Test + def errorIfNoThrow: Unit = { + try { + assertThrows[Foo] { () } + } catch { + case e: AssertionError => return + } + assert(false, "assertThrows should error if the tested expression does not throw anything") + } +} -- cgit v1.2.3 From 35c53af7e3bbe19d50845e698c02a49d0a022409 Mon Sep 17 00:00:00 2001 From: Lukas Rytz Date: Wed, 27 Aug 2014 14:00:36 +0200 Subject: Tools to run the compiler in JUnit tests --- .../scala/tools/partest/ASMConverters.scala | 23 +++++- .../scala/tools/nsc/backend/jvm/CodeGenTools.scala | 34 +++++++++ .../tools/nsc/backend/jvm/DirectCompileTest.scala | 81 ++++++++++++++++++++++ 3 files changed, 136 insertions(+), 2 deletions(-) create mode 100644 test/junit/scala/tools/nsc/backend/jvm/DirectCompileTest.scala (limited to 'test') diff --git a/src/partest-extras/scala/tools/partest/ASMConverters.scala b/src/partest-extras/scala/tools/partest/ASMConverters.scala index 50057d058c..f4a90d9acf 100644 --- a/src/partest-extras/scala/tools/partest/ASMConverters.scala +++ b/src/partest-extras/scala/tools/partest/ASMConverters.scala @@ -3,6 +3,7 @@ package scala.tools.partest import scala.collection.JavaConverters._ import scala.tools.asm import asm.{tree => t} +import scala.tools.asm.tree.LabelNode /** Makes using ASM from ByteCodeTests more convenient. * @@ -22,8 +23,26 @@ object ASMConverters { asmToScala.convert(insns.iterator.asScala.toList) } - implicit class CompareInstructionLists(val self: List[Instruction]) { + implicit class RichInstructionLists(val self: List[Instruction]) extends AnyVal { def === (other: List[Instruction]) = equivalentBytecode(self, other) + + def dropLinesFrames = self.filterNot(i => i.isInstanceOf[LineNumber] || i.isInstanceOf[FrameEntry]) + + private def referencedLabels(instruction: Instruction): Set[Instruction] = instruction match { + case Jump(op, label) => Set(label) + case LookupSwitch(op, dflt, keys, labels) => (dflt :: labels).toSet + case TableSwitch(op, min, max, dflt, labels) => (dflt :: labels).toSet + case LineNumber(line, start) => Set(start) + case _ => Set.empty + } + + def dropStaleLabels = { + val definedLabels: Set[Instruction] = self.filter(_.isInstanceOf[Label]).toSet + val usedLabels: Set[Instruction] = self.flatMap(referencedLabels)(collection.breakOut) + self.filterNot(definedLabels diff usedLabels) + } + + def dropNonOp = dropLinesFrames.dropStaleLabels } sealed abstract class Instruction extends Product { @@ -89,7 +108,7 @@ object ASMConverters { case i: t.LdcInsnNode => Ldc (op(i), i.cst: Any) case i: t.LookupSwitchInsnNode => LookupSwitch (op(i), applyLabel(i.dflt), lst(i.keys) map (x => x: Int), lst(i.labels) map applyLabel) case i: t.TableSwitchInsnNode => TableSwitch (op(i), i.min, i.max, applyLabel(i.dflt), lst(i.labels) map applyLabel) - case i: t.MethodInsnNode => Method (op(i), i.desc, i.name, i.owner, i.itf) + case i: t.MethodInsnNode => Method (op(i), i.owner, i.name, i.desc, i.itf) case i: t.MultiANewArrayInsnNode => NewArray (op(i), i.desc, i.dims) case i: t.TypeInsnNode => TypeOp (op(i), i.desc) case i: t.VarInsnNode => VarOp (op(i), i.`var`) diff --git a/test/junit/scala/tools/nsc/backend/jvm/CodeGenTools.scala b/test/junit/scala/tools/nsc/backend/jvm/CodeGenTools.scala index 256dff85c3..8518d5c832 100644 --- a/test/junit/scala/tools/nsc/backend/jvm/CodeGenTools.scala +++ b/test/junit/scala/tools/nsc/backend/jvm/CodeGenTools.scala @@ -1,7 +1,11 @@ package scala.tools.nsc.backend.jvm +import scala.reflect.internal.util.BatchSourceFile +import scala.reflect.io.VirtualDirectory import scala.tools.asm.Opcodes import scala.tools.asm.tree.{AbstractInsnNode, LabelNode, ClassNode, MethodNode} +import scala.tools.cmd.CommandLineParser +import scala.tools.nsc.{Settings, Global} import scala.tools.partest.ASMConverters import scala.collection.JavaConverters._ @@ -24,4 +28,34 @@ object CodeGenTools { cls.methods.add(method) cls } + + private def resetOutput(compiler: Global): Unit = { + compiler.settings.outputDirs.setSingleOutput(new VirtualDirectory("(memory)", None)) + } + + def newCompiler(defaultArgs: String = "-usejavacp", extraArgs: String = ""): Global = { + val settings = new Settings() + val args = (CommandLineParser tokenize defaultArgs) ++ (CommandLineParser tokenize extraArgs) + settings.processArguments(args, processAll = true) + val compiler = new Global(settings) + resetOutput(compiler) + compiler + } + + def compile(compiler: Global = newCompiler())(code: String): List[(String, Array[Byte])] = { + compiler.reporter.reset() + resetOutput(compiler) + val run = new compiler.Run() + run.compileSources(List(new BatchSourceFile("unitTestSource.scala", code))) + val outDir = compiler.settings.outputDirs.getSingleOutput.get + (for (f <- outDir.iterator if !f.isDirectory) yield (f.name, f.toByteArray)).toList + } + + def compileClasses(compiler: Global = newCompiler())(code: String): List[ClassNode] = { + compile(compiler)(code).map(p => AsmUtils.readClass(p._2)).sortBy(_.name) + } + + def compileMethods(compiler: Global = newCompiler())(code: String): List[MethodNode] = { + compileClasses(compiler)(s"class C { $code }").head.methods.asScala.toList.filterNot(_.name == "") + } } diff --git a/test/junit/scala/tools/nsc/backend/jvm/DirectCompileTest.scala b/test/junit/scala/tools/nsc/backend/jvm/DirectCompileTest.scala new file mode 100644 index 0000000000..640f22fc47 --- /dev/null +++ b/test/junit/scala/tools/nsc/backend/jvm/DirectCompileTest.scala @@ -0,0 +1,81 @@ +package scala.tools.nsc.backend.jvm + +import org.junit.Test +import org.junit.runner.RunWith +import org.junit.runners.JUnit4 +import org.junit.Assert._ +import CodeGenTools._ +import scala.tools.asm.Opcodes._ +import scala.tools.partest.ASMConverters._ + +@RunWith(classOf[JUnit4]) +class DirectCompileTest { + val compiler = newCompiler(extraArgs = "-Ybackend:GenBCode") + + @Test + def testCompile(): Unit = { + val List(("C.class", bytes)) = compile(compiler)( + """ + |class C { + | def f = 1 + |} + """.stripMargin) + def s(i: Int, n: Int) = (bytes(i) & 0xff) << n + assertTrue((s(0, 24) | s(1, 16) | s(2, 8) | s(3, 0)) == 0xcafebabe) // mocha java latte machiatto surpreme dark roasted espresso + } + + @Test + def testCompileClasses(): Unit = { + val List(cClass, cModuleClass) = compileClasses(compiler)( + """ + |class C + |object C + """.stripMargin) + + assertTrue(cClass.name == "C") + assertTrue(cModuleClass.name == "C$") + + val List(dMirror, dModuleClass) = compileClasses(compiler)( + """ + |object D + """.stripMargin) + + assertTrue(dMirror.name == "D") + assertTrue(dModuleClass.name == "D$") + } + + @Test + def testCompileMethods(): Unit = { + val List(f, g) = compileMethods(compiler)( + """ + |def f = 10 + |def g = f + """.stripMargin) + assertTrue(f.name == "f") + assertTrue(g.name == "g") + + assertTrue(instructionsFromMethod(f).dropNonOp === + List(IntOp(BIPUSH, 10), Op(IRETURN))) + + assertTrue(instructionsFromMethod(g).dropNonOp === + List(VarOp(ALOAD, 0), Method(INVOKEVIRTUAL, "C", "f", "()I", false), Op(IRETURN))) + } + + @Test + def testDropNonOpAliveLabels(): Unit = { + val List(f) = compileMethods(compiler)("""def f(x: Int) = if (x == 0) "a" else "b"""") + assertTrue(instructionsFromMethod(f).dropNonOp === List( + VarOp(ILOAD, 1), + Op(ICONST_0), + Jump(IF_ICMPEQ, Label(6)), + Jump(GOTO, Label(10)), + Label(6), + Ldc(LDC, "a"), + Jump(GOTO, Label(13)), + Label(10), + Ldc(LDC, "b"), + Label(13), + Op(ARETURN) + )) + } +} -- cgit v1.2.3 From 630bd29f9e92ad26b8ce05835e2edc115633072c Mon Sep 17 00:00:00 2001 From: Lukas Rytz Date: Thu, 28 Aug 2014 15:47:07 +0200 Subject: Clarify why we emit ATHROW after expressions of type Nothing Tests for emitting expressions of type Nothing. --- .../tools/nsc/backend/jvm/BCodeBodyBuilder.scala | 46 +++++++++++++++- test/files/run/nothingTypeDce.flags | 1 + test/files/run/nothingTypeDce.scala | 61 ++++++++++++++++++++++ test/files/run/nothingTypeNoFramesNoDce.check | 1 + test/files/run/nothingTypeNoFramesNoDce.flags | 1 + test/files/run/nothingTypeNoFramesNoDce.scala | 61 ++++++++++++++++++++++ test/files/run/nothingTypeNoOpt.flags | 1 + test/files/run/nothingTypeNoOpt.scala | 61 ++++++++++++++++++++++ .../scala/tools/nsc/backend/jvm/CodeGenTools.scala | 11 ++-- .../nsc/backend/jvm/opt/UnreachableCodeTest.scala | 45 ++++++++++++++-- 10 files changed, 282 insertions(+), 7 deletions(-) create mode 100644 test/files/run/nothingTypeDce.flags create mode 100644 test/files/run/nothingTypeDce.scala create mode 100644 test/files/run/nothingTypeNoFramesNoDce.check create mode 100644 test/files/run/nothingTypeNoFramesNoDce.flags create mode 100644 test/files/run/nothingTypeNoFramesNoDce.scala create mode 100644 test/files/run/nothingTypeNoOpt.flags create mode 100644 test/files/run/nothingTypeNoOpt.scala (limited to 'test') 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 == "") } + + 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( @@ -96,6 +104,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) + } } -- cgit v1.2.3 From 01f5435318551ce12787b120440572f169c71463 Mon Sep 17 00:00:00 2001 From: Lukas Rytz Date: Fri, 29 Aug 2014 13:44:36 +0200 Subject: SI-8568 unreachable test now passes in GenBCode --- test/files/jvm/unreachable.flags | 1 - 1 file changed, 1 deletion(-) delete mode 100644 test/files/jvm/unreachable.flags (limited to 'test') diff --git a/test/files/jvm/unreachable.flags b/test/files/jvm/unreachable.flags deleted file mode 100644 index 49f2d2c4c8..0000000000 --- a/test/files/jvm/unreachable.flags +++ /dev/null @@ -1 +0,0 @@ --Ybackend:GenASM -- cgit v1.2.3 From 59070cc385560b48267cbc77e872027dd8304c05 Mon Sep 17 00:00:00 2001 From: Lukas Rytz Date: Thu, 4 Sep 2014 08:07:57 +0200 Subject: Remove stale local variables and exception handlers after DCE This is required for correctness of the generated bytecode. Exception handlers and local variable descriptors specify code offset ranges. These offsets have to exist, not be eliminated. --- .../tools/nsc/backend/jvm/BCodeBodyBuilder.scala | 3 +- .../tools/nsc/backend/jvm/BCodeSkelBuilder.scala | 7 ++ .../scala/tools/nsc/backend/jvm/BackendStats.scala | 7 ++ .../scala/tools/nsc/backend/jvm/GenBCode.scala | 13 ++- .../scala/tools/nsc/backend/jvm/opt/LocalOpt.scala | 116 +++++++++++++++++++-- .../scala/tools/partest/ASMConverters.scala | 54 ++++++---- .../scala/tools/nsc/backend/jvm/CodeGenTools.scala | 17 ++- .../tools/nsc/backend/jvm/DirectCompileTest.scala | 2 +- .../jvm/opt/EmptyExceptionHandlersTest.scala | 92 ++++++++++++++++ .../nsc/backend/jvm/opt/UnreachableCodeTest.scala | 24 ++++- .../backend/jvm/opt/UnusedLocalVariablesTest.scala | 87 ++++++++++++++++ 11 files changed, 386 insertions(+), 36 deletions(-) create mode 100644 test/junit/scala/tools/nsc/backend/jvm/opt/EmptyExceptionHandlersTest.scala create mode 100644 test/junit/scala/tools/nsc/backend/jvm/opt/UnusedLocalVariablesTest.scala (limited to 'test') diff --git a/src/compiler/scala/tools/nsc/backend/jvm/BCodeBodyBuilder.scala b/src/compiler/scala/tools/nsc/backend/jvm/BCodeBodyBuilder.scala index 68b6f1af37..ba97cf3748 100644 --- a/src/compiler/scala/tools/nsc/backend/jvm/BCodeBodyBuilder.scala +++ b/src/compiler/scala/tools/nsc/backend/jvm/BCodeBodyBuilder.scala @@ -282,9 +282,10 @@ abstract class BCodeBodyBuilder extends BCodeSkelBuilder { val Local(tk, _, idx, isSynth) = locals.getOrMakeLocal(sym) if (rhs == EmptyTree) { emitZeroOf(tk) } else { genLoad(rhs, tk) } + val localVarStart = currProgramPoint() bc.store(idx, tk) if (!isSynth) { // there are case ValDef's emitted by patmat - varsInScope ::= (sym -> currProgramPoint()) + varsInScope ::= (sym -> localVarStart) } generatedType = UNIT diff --git a/src/compiler/scala/tools/nsc/backend/jvm/BCodeSkelBuilder.scala b/src/compiler/scala/tools/nsc/backend/jvm/BCodeSkelBuilder.scala index 4592031a31..03bc32061b 100644 --- a/src/compiler/scala/tools/nsc/backend/jvm/BCodeSkelBuilder.scala +++ b/src/compiler/scala/tools/nsc/backend/jvm/BCodeSkelBuilder.scala @@ -346,6 +346,13 @@ abstract class BCodeSkelBuilder extends BCodeHelpers { /* * Bookkeeping for method-local vars and method-params. + * + * TODO: use fewer slots. local variable slots are never re-used in separate blocks. + * In the following example, x and y could use the same slot. + * def foo() = { + * { val x = 1 } + * { val y = "a" } + * } */ object locals { diff --git a/src/compiler/scala/tools/nsc/backend/jvm/BackendStats.scala b/src/compiler/scala/tools/nsc/backend/jvm/BackendStats.scala index 921aef7aca..d96ee933b7 100644 --- a/src/compiler/scala/tools/nsc/backend/jvm/BackendStats.scala +++ b/src/compiler/scala/tools/nsc/backend/jvm/BackendStats.scala @@ -16,4 +16,11 @@ object BackendStats { val bcodeGenStat = newSubTimer("code generation", bcodeTimer) val bcodeDceTimer = newSubTimer("dead code elimination", bcodeTimer) val bcodeWriteTimer = newSubTimer("classfile writing", bcodeTimer) + + def timed[T](timer: Statistics.Timer)(body: => T) = { + val start = Statistics.startTimer(timer) + val res = body + Statistics.stopTimer(timer, start) + res + } } diff --git a/src/compiler/scala/tools/nsc/backend/jvm/GenBCode.scala b/src/compiler/scala/tools/nsc/backend/jvm/GenBCode.scala index a788cea532..ba94a9c44c 100644 --- a/src/compiler/scala/tools/nsc/backend/jvm/GenBCode.scala +++ b/src/compiler/scala/tools/nsc/backend/jvm/GenBCode.scala @@ -14,6 +14,7 @@ import scala.annotation.switch import scala.reflect.internal.util.Statistics import scala.tools.asm +import scala.tools.asm.tree.ClassNode /* * Prepare in-memory representations of classfiles using the ASM Tree API, and serialize them to disk. @@ -214,6 +215,14 @@ abstract class GenBCode extends BCodeSyncAndTry { * - converting the plain ClassNode to byte array and placing it on queue-3 */ class Worker2 { + def localOptimizations(classNode: ClassNode): Unit = { + def dce(): Boolean = BackendStats.timed(BackendStats.bcodeDceTimer) { + if (settings.YoptUnreachableCode) opt.LocalOpt.removeUnreachableCode(classNode) + else false + } + + dce() + } def run() { while (true) { @@ -224,9 +233,7 @@ abstract class GenBCode extends BCodeSyncAndTry { } else { try { - val dceStart = Statistics.startTimer(BackendStats.bcodeDceTimer) - if (settings.YoptUnreachableCode) opt.LocalOpt.removeUnreachableCode(item.plain) - Statistics.stopTimer(BackendStats.bcodeDceTimer, dceStart) + localOptimizations(item.plain) addToQ3(item) } catch { case ex: Throwable => diff --git a/src/compiler/scala/tools/nsc/backend/jvm/opt/LocalOpt.scala b/src/compiler/scala/tools/nsc/backend/jvm/opt/LocalOpt.scala index b3a515c2ae..3acd2d6154 100644 --- a/src/compiler/scala/tools/nsc/backend/jvm/opt/LocalOpt.scala +++ b/src/compiler/scala/tools/nsc/backend/jvm/opt/LocalOpt.scala @@ -7,10 +7,11 @@ package scala.tools.nsc package backend.jvm package opt -import scala.tools.asm.{MethodWriter, ClassWriter} +import scala.tools.asm.{Opcodes, MethodWriter, ClassWriter} import scala.tools.asm.tree.analysis.{Analyzer, BasicValue, BasicInterpreter} -import scala.tools.asm.tree.{LabelNode, ClassNode, MethodNode} +import scala.tools.asm.tree._ import scala.collection.convert.decorateAsScala._ +import scala.collection.{ mutable => m } /** * Intra-Method optimizations. @@ -24,7 +25,7 @@ object LocalOpt { */ def removeUnreachableCode(clazz: ClassNode): Boolean = { clazz.methods.asScala.foldLeft(false) { - case (changed, method) => removeUnreachableCode(method, clazz) || changed + case (changed, method) => removeUnreachableCode(method, clazz.name) || changed } } @@ -34,15 +35,44 @@ object LocalOpt { * Guide (http://asm.ow2.org/index.html), Section 8.2.1. It runs a data flow analysis, which only * computes Frame information for reachable instructions. Instructions for which no Frame data is * available after the analyis are unreachable. + * + * TODO doc: it also removes empty handlers, unused local vars + * + * Returns `true` if dead code in `method` has been eliminated. */ - private def removeUnreachableCode(method: MethodNode, ownerClass: ClassNode): Boolean = { + private def removeUnreachableCode(method: MethodNode, ownerClassName: String): Boolean = { + if (method.instructions.size == 0) return false // fast path for abstract methods + + val codeRemoved = removeUnreachableCodeImpl(method, ownerClassName) + + // unreachable-code also removes unused local variable nodes and empty exception handlers. + // This is required for correctness: such nodes are not allowed to refer to instruction offsets + // that don't exist (because they have been eliminated). + val localsRemoved = removeUnusedLocalVariableNodes(method) + val handlersRemoved = removeEmptyExceptionHandlers(method) + + // When eliminating a handler, the catch block becomes unreachable. The recursive invocation + // removes these blocks. + // Note that invoking removeUnreachableCode*Impl* a second time is not enough: removing the dead + // catch block can render other handlers empty, which also have to be removed in turn. + if (handlersRemoved) removeUnreachableCode(method, ownerClassName) + + // assert that we can leave local variable annotations as-is + def nullOrEmpty[T](l: java.util.List[T]) = l == null || l.isEmpty + assert(nullOrEmpty(method.visibleLocalVariableAnnotations), method.visibleLocalVariableAnnotations) + assert(nullOrEmpty(method.invisibleLocalVariableAnnotations), method.invisibleLocalVariableAnnotations) + + codeRemoved || localsRemoved || handlersRemoved + } + + private def removeUnreachableCodeImpl(method: MethodNode, ownerClassName: String): Boolean = { val initialSize = method.instructions.size if (initialSize == 0) return false // The data flow analysis requires the maxLocals / maxStack fields of the method to be computed. computeMaxLocalsMaxStack(method) val a = new Analyzer[BasicValue](new BasicInterpreter) - a.analyze(ownerClass.name, method) + a.analyze(ownerClassName, method) val frames = a.getFrames var i = 0 @@ -58,9 +88,83 @@ object LocalOpt { i += 1 } - method.instructions.size == initialSize + method.instructions.size != initialSize + } + + /** + * Remove exception handlers that cover empty code blocks from all methods of `clazz`. + * Returns `true` if any exception handler was eliminated. + */ + def removeEmptyExceptionHandlers(clazz: ClassNode): Boolean = { + clazz.methods.asScala.foldLeft(false) { + case (changed, method) => removeEmptyExceptionHandlers(method) || changed + } + } + + /** + * Remove exception handlers that cover empty code blocks. A block is considered empty if it + * consist only of labels, frames, line numbers, nops and gotos. + * + * Note that no instructions are eliminated. + * + * @return `true` if some exception handler was eliminated. + */ + def removeEmptyExceptionHandlers(method: MethodNode): Boolean = { + /** True if there exists code between start and end. */ + def containsExecutableCode(start: AbstractInsnNode, end: LabelNode): Boolean = { + start != end && (start.getOpcode match { + // FrameNode, LabelNode and LineNumberNode have opcode == -1. + case -1 | Opcodes.NOP | Opcodes.GOTO => containsExecutableCode(start.getNext, end) + case _ => true + }) + } + + val initialNumberHandlers = method.tryCatchBlocks.size + val handlersIter = method.tryCatchBlocks.iterator() + while(handlersIter.hasNext) { + val handler = handlersIter.next() + if (!containsExecutableCode(handler.start, handler.end)) handlersIter.remove() + } + method.tryCatchBlocks.size != initialNumberHandlers } + /** + * Remove all non-parameter entries from the local variable table which denote variables that are + * not actually read or written. + * + * Note that each entry in the local variable table has a start, end and index. Two entries with + * the same index, but distinct start / end ranges are different variables, they may have not the + * same type or name. + * + * TODO: also re-allocate locals to occupy fewer slots after eliminating unused ones + */ + def removeUnusedLocalVariableNodes(method: MethodNode): Boolean = { + def variableIsUsed(start: AbstractInsnNode, end: LabelNode, varIndex: Int): Boolean = { + start != end && (start match { + case v: VarInsnNode => v.`var` == varIndex + case _ => variableIsUsed(start.getNext, end, varIndex) + }) + } + + val initialNumVars = method.localVariables.size + val localsIter = method.localVariables.iterator() + // The parameters and `this` (for instance methods) have the lowest indices in the local variables + // table. Note that double / long fields occupy two slots, so we sum up the sizes. Since getSize + // returns 0 for void, we have to add `max 1`. + val paramsSize = scala.tools.asm.Type.getArgumentTypes(method.desc).map(_.getSize max 1).sum + val thisSize = if ((method.access & Opcodes.ACC_STATIC) == 0) 1 else 0 + val endParamIndex = paramsSize + thisSize + while (localsIter.hasNext) { + val local = localsIter.next() + // parameters and `this` have the lowest indices, starting at 0 + val used = local.index < endParamIndex || variableIsUsed(local.start, local.end, local.index) + if (!used) + localsIter.remove() + } + method.localVariables.size == initialNumVars + } + + /** * In order to run an Analyzer, the maxLocals / maxStack fields need to be available. The ASM * framework only computes these values during bytecode generation. diff --git a/src/partest-extras/scala/tools/partest/ASMConverters.scala b/src/partest-extras/scala/tools/partest/ASMConverters.scala index f4a90d9acf..d443a31112 100644 --- a/src/partest-extras/scala/tools/partest/ASMConverters.scala +++ b/src/partest-extras/scala/tools/partest/ASMConverters.scala @@ -3,7 +3,6 @@ package scala.tools.partest import scala.collection.JavaConverters._ import scala.tools.asm import asm.{tree => t} -import scala.tools.asm.tree.LabelNode /** Makes using ASM from ByteCodeTests more convenient. * @@ -15,13 +14,9 @@ object ASMConverters { /** * Transform the instructions of an ASM Method into a list of [[Instruction]]s. */ - def instructionsFromMethod(meth: t.MethodNode): List[Instruction] = { - val insns = meth.instructions - val asmToScala = new AsmToScala { - def labelIndex(l: t.LabelNode) = insns.indexOf(l) - } - asmToScala.convert(insns.iterator.asScala.toList) - } + def instructionsFromMethod(meth: t.MethodNode): List[Instruction] = new AsmToScala(meth).instructions + + def convertMethod(meth: t.MethodNode): Method = new AsmToScala(meth).method implicit class RichInstructionLists(val self: List[Instruction]) extends AnyVal { def === (other: List[Instruction]) = equivalentBytecode(self, other) @@ -61,6 +56,8 @@ object ASMConverters { } } + case class Method(instructions: List[Instruction], handlers: List[ExceptionHandler], localVars: List[LocalVariable]) + case class Field (opcode: Int, owner: String, name: String, desc: String) extends Instruction case class Incr (opcode: Int, `var`: Int, incr: Int) extends Instruction case class Op (opcode: Int) extends Instruction @@ -69,7 +66,7 @@ object ASMConverters { case class Ldc (opcode: Int, cst: Any) extends Instruction case class LookupSwitch(opcode: Int, dflt: Label, keys: List[Int], labels: List[Label]) extends Instruction case class TableSwitch (opcode: Int, min: Int, max: Int, dflt: Label, labels: List[Label]) extends Instruction - case class Method (opcode: Int, owner: String, name: String, desc: String, itf: Boolean) extends Instruction + case class Invoke (opcode: Int, owner: String, name: String, desc: String, itf: Boolean) extends Instruction case class NewArray (opcode: Int, desc: String, dims: Int) extends Instruction case class TypeOp (opcode: Int, desc: String) extends Instruction case class VarOp (opcode: Int, `var`: Int) extends Instruction @@ -77,13 +74,18 @@ object ASMConverters { case class FrameEntry (`type`: Int, local: List[Any], stack: List[Any]) extends Instruction { def opcode: Int = -1 } case class LineNumber (line: Int, start: Label) extends Instruction { def opcode: Int = -1 } - abstract class AsmToScala { + case class ExceptionHandler(start: Label, end: Label, handler: Label, desc: Option[String]) + case class LocalVariable(name: String, desc: String, signature: Option[String], start: Label, end: Label, index: Int) - def labelIndex(l: t.LabelNode): Int + class AsmToScala(asmMethod: t.MethodNode) { - def op(i: t.AbstractInsnNode): Int = i.getOpcode + def instructions: List[Instruction] = asmMethod.instructions.iterator.asScala.toList map apply - def convert(instructions: List[t.AbstractInsnNode]): List[Instruction] = instructions map apply + def method: Method = Method(instructions, convertHandlers(asmMethod), convertLocalVars(asmMethod)) + + private def labelIndex(l: t.LabelNode): Int = asmMethod.instructions.indexOf(l) + + private def op(i: t.AbstractInsnNode): Int = i.getOpcode private def lst[T](xs: java.util.List[T]): List[T] = if (xs == null) Nil else xs.asScala.toList @@ -108,7 +110,7 @@ object ASMConverters { case i: t.LdcInsnNode => Ldc (op(i), i.cst: Any) case i: t.LookupSwitchInsnNode => LookupSwitch (op(i), applyLabel(i.dflt), lst(i.keys) map (x => x: Int), lst(i.labels) map applyLabel) case i: t.TableSwitchInsnNode => TableSwitch (op(i), i.min, i.max, applyLabel(i.dflt), lst(i.labels) map applyLabel) - case i: t.MethodInsnNode => Method (op(i), i.owner, i.name, i.desc, i.itf) + case i: t.MethodInsnNode => Invoke (op(i), i.owner, i.name, i.desc, i.itf) case i: t.MultiANewArrayInsnNode => NewArray (op(i), i.desc, i.dims) case i: t.TypeInsnNode => TypeOp (op(i), i.desc) case i: t.VarInsnNode => VarOp (op(i), i.`var`) @@ -116,6 +118,14 @@ object ASMConverters { case i: t.FrameNode => FrameEntry (i.`type`, mapOverFrameTypes(lst(i.local)), mapOverFrameTypes(lst(i.stack))) case i: t.LineNumberNode => LineNumber (i.line, applyLabel(i.start)) } + + private def convertHandlers(method: t.MethodNode): List[ExceptionHandler] = { + method.tryCatchBlocks.asScala.map(h => ExceptionHandler(applyLabel(h.start), applyLabel(h.end), applyLabel(h.handler), Option(h.`type`)))(collection.breakOut) + } + + private def convertLocalVars(method: t.MethodNode): List[LocalVariable] = { + method.localVariables.asScala.map(v => LocalVariable(v.name, v.desc, Option(v.signature), applyLabel(v.start), applyLabel(v.end), v.index))(collection.breakOut) + } } import collection.mutable.{Map => MMap} @@ -159,15 +169,21 @@ object ASMConverters { }) && equivalentBytecode(as.tail, bs.tail, varMap, labelMap) } - /** - * Convert back a list of [[Instruction]]s to ASM land. The code is emitted into the parameter - * `method`. - */ def applyToMethod(method: t.MethodNode, instructions: List[Instruction]): Unit = { val asmLabel = createLabelNodes(instructions) instructions.foreach(visitMethod(method, _, asmLabel)) } + /** + * Convert back a [[Method]] to ASM land. The code is emitted into the parameter `asmMethod`. + */ + def applyToMethod(asmMethod: t.MethodNode, method: Method): Unit = { + val asmLabel = createLabelNodes(method.instructions) + method.instructions.foreach(visitMethod(asmMethod, _, asmLabel)) + method.handlers.foreach(h => asmMethod.visitTryCatchBlock(asmLabel(h.start), asmLabel(h.end), asmLabel(h.handler), h.desc.orNull)) + method.localVars.foreach(v => asmMethod.visitLocalVariable(v.name, v.desc, v.signature.orNull, asmLabel(v.start), asmLabel(v.end), v.index)) + } + private def createLabelNodes(instructions: List[Instruction]): Map[Label, asm.Label] = { val labels = instructions collect { case l: Label => l @@ -190,7 +206,7 @@ object ASMConverters { case Ldc(op, cst) => method.visitLdcInsn(cst) case LookupSwitch(op, dflt, keys, labels) => method.visitLookupSwitchInsn(asmLabel(dflt), keys.toArray, (labels map asmLabel).toArray) case TableSwitch(op, min, max, dflt, labels) => method.visitTableSwitchInsn(min, max, asmLabel(dflt), (labels map asmLabel).toArray: _*) - case Method(op, owner, name, desc, itf) => method.visitMethodInsn(op, owner, name, desc, itf) + case Invoke(op, owner, name, desc, itf) => method.visitMethodInsn(op, owner, name, desc, itf) case NewArray(op, desc, dims) => method.visitMultiANewArrayInsn(desc, dims) case TypeOp(op, desc) => method.visitTypeInsn(op, desc) case VarOp(op, vr) => method.visitVarInsn(op, vr) diff --git a/test/junit/scala/tools/nsc/backend/jvm/CodeGenTools.scala b/test/junit/scala/tools/nsc/backend/jvm/CodeGenTools.scala index fb677fc84b..15bc1f427d 100644 --- a/test/junit/scala/tools/nsc/backend/jvm/CodeGenTools.scala +++ b/test/junit/scala/tools/nsc/backend/jvm/CodeGenTools.scala @@ -1,5 +1,7 @@ package scala.tools.nsc.backend.jvm +import org.junit.Assert._ + import scala.reflect.internal.util.BatchSourceFile import scala.reflect.io.VirtualDirectory import scala.tools.asm.Opcodes @@ -16,9 +18,11 @@ object CodeGenTools { name: String = "m", descriptor: String = "()V", genericSignature: String = null, - throwsExceptions: Array[String] = null)(body: Instruction*): MethodNode = { + throwsExceptions: Array[String] = null, + handlers: List[ExceptionHandler] = Nil, + localVars: List[LocalVariable] = Nil)(body: Instruction*): MethodNode = { val node = new MethodNode(flags, name, descriptor, genericSignature, throwsExceptions) - applyToMethod(node, body.toList) + applyToMethod(node, Method(body.toList, handlers, localVars)) node } @@ -63,4 +67,13 @@ object CodeGenTools { val List(m) = compileMethods(compiler)(code) instructionsFromMethod(m) } + + def singleMethod(compiler: Global)(code: String): Method = { + val List(m) = compileMethods(compiler)(code) + convertMethod(m) + } + + def assertSameCode(actual: List[Instruction], expected: List[Instruction]): Unit = { + assertTrue(s"\nExpected: $expected\nActual : $actual", actual === expected) + } } diff --git a/test/junit/scala/tools/nsc/backend/jvm/DirectCompileTest.scala b/test/junit/scala/tools/nsc/backend/jvm/DirectCompileTest.scala index 640f22fc47..2fb5bb8052 100644 --- a/test/junit/scala/tools/nsc/backend/jvm/DirectCompileTest.scala +++ b/test/junit/scala/tools/nsc/backend/jvm/DirectCompileTest.scala @@ -58,7 +58,7 @@ class DirectCompileTest { List(IntOp(BIPUSH, 10), Op(IRETURN))) assertTrue(instructionsFromMethod(g).dropNonOp === - List(VarOp(ALOAD, 0), Method(INVOKEVIRTUAL, "C", "f", "()I", false), Op(IRETURN))) + List(VarOp(ALOAD, 0), Invoke(INVOKEVIRTUAL, "C", "f", "()I", false), Op(IRETURN))) } @Test diff --git a/test/junit/scala/tools/nsc/backend/jvm/opt/EmptyExceptionHandlersTest.scala b/test/junit/scala/tools/nsc/backend/jvm/opt/EmptyExceptionHandlersTest.scala new file mode 100644 index 0000000000..57fa1a7b66 --- /dev/null +++ b/test/junit/scala/tools/nsc/backend/jvm/opt/EmptyExceptionHandlersTest.scala @@ -0,0 +1,92 @@ +package scala.tools.nsc +package backend.jvm +package opt + +import org.junit.runner.RunWith +import org.junit.runners.JUnit4 +import org.junit.Test +import scala.tools.asm.Opcodes._ +import org.junit.Assert._ + +import CodeGenTools._ +import scala.tools.partest.ASMConverters +import ASMConverters._ + +@RunWith(classOf[JUnit4]) +class EmptyExceptionHandlersTest { + + val exceptionDescriptor = "java/lang/Exception" + + @Test + def eliminateEmpty(): Unit = { + val handlers = List(ExceptionHandler(Label(1), Label(2), Label(2), Some(exceptionDescriptor))) + val asmMethod = genMethod(handlers = handlers)( + Label(1), + Label(2), + Op(RETURN) + ) + assertTrue(convertMethod(asmMethod).handlers.length == 1) + LocalOpt.removeEmptyExceptionHandlers(asmMethod) + assertTrue(convertMethod(asmMethod).handlers.isEmpty) + } + + @Test + def eliminateHandlersGuardingNops(): Unit = { + val handlers = List(ExceptionHandler(Label(1), Label(2), Label(2), Some(exceptionDescriptor))) + val asmMethod = genMethod(handlers = handlers)( + Label(1), // nops only + Op(NOP), + Op(NOP), + Jump(GOTO, Label(3)), + Op(NOP), + Label(3), + Op(NOP), + Jump(GOTO, Label(4)), + + Label(2), // handler + Op(ACONST_NULL), + Op(ATHROW), + + Label(4), // return + Op(RETURN) + ) + assertTrue(convertMethod(asmMethod).handlers.length == 1) + LocalOpt.removeEmptyExceptionHandlers(asmMethod) + assertTrue(convertMethod(asmMethod).handlers.isEmpty) + } + + val noOptCompiler = newCompiler(extraArgs = "-Ybackend:GenBCode -Yopt:l:none") + val dceCompiler = newCompiler(extraArgs = "-Ybackend:GenBCode -Yopt:unreachable-code") + + @Test + def eliminateUnreachableHandler(): Unit = { + val code = "def f: Unit = try { } catch { case _: Exception => println(0) }; println(1)" + + assertTrue(singleMethod(noOptCompiler)(code).handlers.length == 1) + val optMethod = singleMethod(dceCompiler)(code) + assertTrue(optMethod.handlers.isEmpty) + + val code2 = + """def f: Unit = { + | println(0) + | return + | try { throw new Exception("") } // removed by dce, so handler will be removed as well + | catch { case _: Exception => println(1) } + | println(2) + |}""".stripMargin + + assertTrue(singleMethod(dceCompiler)(code2).handlers.isEmpty) + } + + @Test + def keepAliveHandlers(): Unit = { + val code = + """def f: Int = { + | println(0) + | try { 1 } + | catch { case _: Exception => 2 } + |}""".stripMargin + + assertTrue(singleMethod(dceCompiler)(code).handlers.length == 1) + } +} 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 ae08d10b79..a3bd7ae6fe 100644 --- a/test/junit/scala/tools/nsc/backend/jvm/opt/UnreachableCodeTest.scala +++ b/test/junit/scala/tools/nsc/backend/jvm/opt/UnreachableCodeTest.scala @@ -135,6 +135,26 @@ class UnreachableCodeTest { } @Test + def eliminateDeadCatchBlocks(): Unit = { + val code = "def f: Int = { return 0; try { 1 } catch { case _: Exception => 2 } }" + assertSameCode(singleMethodInstructions(dceCompiler)(code).dropNonOp, + List(Op(ICONST_0), Op(IRETURN))) + + val code2 = "def f: Unit = { try { } catch { case _: Exception => () }; () }" + // DCE only removes dead basic blocks, but not NOPs, and also not useless jumps + assertSameCode(singleMethodInstructions(dceCompiler)(code2).dropNonOp, + List(Op(NOP), Jump(GOTO, Label(33)), Label(33), Op(RETURN))) + + val code3 = "def f: Unit = { try { } catch { case _: Exception => try { } catch { case _: Exception => () } }; () }" + assertSameCode(singleMethodInstructions(dceCompiler)(code3).dropNonOp, + List(Op(NOP), Jump(GOTO, Label(33)), Label(33), Op(RETURN))) + + val code4 = "def f: Unit = { try { try { } catch { case _: Exception => () } } catch { case _: Exception => () }; () }" + assertSameCode(singleMethodInstructions(dceCompiler)(code4).dropNonOp, + List(Op(NOP), Jump(GOTO, Label(4)), Label(4), Jump(GOTO, Label(7)), Label(7), Op(RETURN))) + } + + @Test // test the dce-testing tools def metaTest(): Unit = { assertEliminateDead() // no instructions @@ -194,8 +214,4 @@ object UnreachableCodeTest { val expectedLive = code.filter(_._2).map(_._1).toList assertSameCode(nonEliminated, expectedLive) } - - def assertSameCode(actual: List[Instruction], expected: List[Instruction]): Unit = { - assertTrue(s"\nExpected: $expected\nActual : $actual", actual === expected) - } } diff --git a/test/junit/scala/tools/nsc/backend/jvm/opt/UnusedLocalVariablesTest.scala b/test/junit/scala/tools/nsc/backend/jvm/opt/UnusedLocalVariablesTest.scala new file mode 100644 index 0000000000..24a1f9d1c1 --- /dev/null +++ b/test/junit/scala/tools/nsc/backend/jvm/opt/UnusedLocalVariablesTest.scala @@ -0,0 +1,87 @@ +package scala.tools.nsc +package backend.jvm +package opt + +import org.junit.runner.RunWith +import org.junit.runners.JUnit4 +import org.junit.Test +import scala.tools.asm.Opcodes._ +import org.junit.Assert._ +import scala.collection.JavaConverters._ + +import CodeGenTools._ +import scala.tools.partest.ASMConverters +import ASMConverters._ + +@RunWith(classOf[JUnit4]) +class UnusedLocalVariablesTest { + val dceCompiler = newCompiler(extraArgs = "-Ybackend:GenBCode -Yopt:unreachable-code") + + @Test + def removeUnusedVar(): Unit = { + val code = """def f(a: Long, b: String, c: Double): Unit = { return; var x = a; var y = x + 10 }""" + assertLocalVarCount(code, 4) // `this, a, b, c` + + val code2 = """def f(): Unit = { var x = if (true) return else () }""" + assertLocalVarCount(code2, 1) // x is eliminated, constant folding in scalac removes the if + + val code3 = """def f: Unit = return""" // paramless method + assertLocalVarCount(code3, 1) // this + } + + @Test + def keepUsedVar(): Unit = { + val code = """def f(a: Long, b: String, c: Double): Unit = { val x = 10 + a; val y = x + 10 }""" + assertLocalVarCount(code, 6) + + val code2 = """def f(a: Long): Unit = { var x = if (a == 0l) return else () }""" + assertLocalVarCount(code2, 3) // remains + } + + @Test + def constructorLocals(): Unit = { + val code = """class C { + | def this(a: Int) = { + | this() + | throw new Exception("") + | val y = 0 + | } + |} + |""".stripMargin + val cls = compileClasses(dceCompiler)(code).head + val m = convertMethod(cls.methods.asScala.toList.find(_.desc == "(I)V").get) + assertTrue(m.localVars.length == 2) // this, a, but not y + + + val code2 = + """class C { + | { + | throw new Exception("") + | val a = 0 + | } + |} + | + |object C { + | { + | throw new Exception("") + | val b = 1 + | } + |} + """.stripMargin + + val clss2 = compileClasses(dceCompiler)(code2) + val cls2 = clss2.find(_.name == "C").get + val companion2 = clss2.find(_.name == "C$").get + + val clsConstr = convertMethod(cls2.methods.asScala.toList.find(_.name == "").get) + val companionConstr = convertMethod(companion2.methods.asScala.toList.find(_.name == "").get) + + assertTrue(clsConstr.localVars.length == 1) // this + assertTrue(companionConstr.localVars.length == 1) // this + } + + def assertLocalVarCount(code: String, numVars: Int): Unit = { + assertTrue(singleMethod(dceCompiler)(code).localVars.length == numVars) + } + +} -- cgit v1.2.3 From 9132efa4a8511e267c808c95df4d2e3de68277e6 Mon Sep 17 00:00:00 2001 From: Lukas Rytz Date: Thu, 11 Sep 2014 10:22:37 +0200 Subject: Address review feedback. --- src/compiler/scala/tools/nsc/backend/jvm/BCodeBodyBuilder.scala | 4 ++-- src/compiler/scala/tools/nsc/backend/jvm/BackendStats.scala | 6 ++---- src/partest-extras/scala/tools/partest/ASMConverters.scala | 4 ++-- src/partest-extras/scala/tools/partest/BytecodeTest.scala | 2 +- test/files/run/nothingTypeDce.scala | 4 +++- 5 files changed, 10 insertions(+), 10 deletions(-) (limited to 'test') diff --git a/src/compiler/scala/tools/nsc/backend/jvm/BCodeBodyBuilder.scala b/src/compiler/scala/tools/nsc/backend/jvm/BCodeBodyBuilder.scala index ba97cf3748..daf36ce374 100644 --- a/src/compiler/scala/tools/nsc/backend/jvm/BCodeBodyBuilder.scala +++ b/src/compiler/scala/tools/nsc/backend/jvm/BCodeBodyBuilder.scala @@ -824,7 +824,7 @@ abstract class BCodeBodyBuilder extends BCodeSkelBuilder { * 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 + * Note: The two verifiers (old: type inference, new: type checking) have different * requirements. Very briefly: * * Old (http://docs.oracle.com/javase/specs/jvms/se8/html/jvms-4.html#jvms-4.10.2.1): at @@ -832,7 +832,7 @@ abstract class BCodeBodyBuilder extends BCodeSkelBuilder { * - 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 + * instructions 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) diff --git a/src/compiler/scala/tools/nsc/backend/jvm/BackendStats.scala b/src/compiler/scala/tools/nsc/backend/jvm/BackendStats.scala index d96ee933b7..4b9383c67c 100644 --- a/src/compiler/scala/tools/nsc/backend/jvm/BackendStats.scala +++ b/src/compiler/scala/tools/nsc/backend/jvm/BackendStats.scala @@ -17,10 +17,8 @@ object BackendStats { val bcodeDceTimer = newSubTimer("dead code elimination", bcodeTimer) val bcodeWriteTimer = newSubTimer("classfile writing", bcodeTimer) - def timed[T](timer: Statistics.Timer)(body: => T) = { + def timed[T](timer: Statistics.Timer)(body: => T): T = { val start = Statistics.startTimer(timer) - val res = body - Statistics.stopTimer(timer, start) - res + try body finally Statistics.stopTimer(timer, start) } } diff --git a/src/partest-extras/scala/tools/partest/ASMConverters.scala b/src/partest-extras/scala/tools/partest/ASMConverters.scala index d443a31112..67a4e8ae01 100644 --- a/src/partest-extras/scala/tools/partest/ASMConverters.scala +++ b/src/partest-extras/scala/tools/partest/ASMConverters.scala @@ -141,9 +141,9 @@ object ASMConverters { } def sameVar(v1: Int, v2: Int) = same(v1, v2, varMap) def sameLabel(l1: Label, l2: Label) = same(l1.offset, l2.offset, labelMap) - def sameLabels(ls1: List[Label], ls2: List[Label]) = ls1.length == ls2.length && (ls1, ls2).zipped.forall(sameLabel) + def sameLabels(ls1: List[Label], ls2: List[Label]) = (ls1 corresponds ls2)(sameLabel) - def sameFrameTypes(ts1: List[Any], ts2: List[Any]) = ts1.length == ts2.length && (ts1, ts2).zipped.forall { + def sameFrameTypes(ts1: List[Any], ts2: List[Any]) = (ts1 corresponds ts2) { case (t1: Label, t2: Label) => sameLabel(t1, t2) case (x, y) => x == y } diff --git a/src/partest-extras/scala/tools/partest/BytecodeTest.scala b/src/partest-extras/scala/tools/partest/BytecodeTest.scala index c6fa279c50..3261cada37 100644 --- a/src/partest-extras/scala/tools/partest/BytecodeTest.scala +++ b/src/partest-extras/scala/tools/partest/BytecodeTest.scala @@ -84,7 +84,7 @@ abstract class BytecodeTest { /** * Compare the bytecodes of two methods. * - * The for the `similar` function, you probably want to pass [[ASMConverters.equivalentBytecode]]. + * For the `similar` function, you probably want to pass [[ASMConverters.equivalentBytecode]]. */ def similarBytecode(methA: MethodNode, methB: MethodNode, similar: (List[Instruction], List[Instruction]) => Boolean) = { val isa = instructionsFromMethod(methA) diff --git a/test/files/run/nothingTypeDce.scala b/test/files/run/nothingTypeDce.scala index 4b9d934eac..5f3692fd33 100644 --- a/test/files/run/nothingTypeDce.scala +++ b/test/files/run/nothingTypeDce.scala @@ -26,7 +26,7 @@ class C { } def f5(x: Boolean) = { - // stack heights need to be the smae. ??? looks to the jvm like returning a value of + // stack heights need to be the same. ??? looks to the jvm like returning a value of // type Nothing$, need to drop or throw it. println( if (x) { ???; 10 } @@ -57,5 +57,7 @@ class C { } object Test extends App { + // creating an instance is enough to trigger bytecode verification for all methods, + // no need to invoke the methods. new C() } -- cgit v1.2.3 From 1da69e82a1ccf2327b337068de1209d772cb6b52 Mon Sep 17 00:00:00 2001 From: Rex Kerr Date: Sun, 24 Aug 2014 17:55:20 -0700 Subject: SI-8815 mutable.LongMap makes different choices for splitAt vs etc. It turns out that take/drop/splitAt/takeWhile/dropWhile inherit a smattering of foreach vs. iterator-based implementations. These aren't consistent unless they iterate in the same order. This probably reflects an undesirable underlying weakness, but in this particular case it was easy to make LongMap's foreach order agree with iterator. Made traversal order of other foreach-like methods match also. Also fixed a bug where Long.MinValue wasn't iterated. Added unit test for iteration coverage of extreme values. --- src/library/scala/collection/mutable/LongMap.scala | 38 +++++++--------------- .../scala/collection/SetMapConsistencyTest.scala | 15 +++++++++ 2 files changed, 26 insertions(+), 27 deletions(-) (limited to 'test') diff --git a/src/library/scala/collection/mutable/LongMap.scala b/src/library/scala/collection/mutable/LongMap.scala index 984ae6f7cc..ef488a3697 100644 --- a/src/library/scala/collection/mutable/LongMap.scala +++ b/src/library/scala/collection/mutable/LongMap.scala @@ -388,12 +388,14 @@ extends AbstractMap[Long, V] nextPair = anotherPair anotherPair = null } - nextPair = null + else nextPair = null ans } } override def foreach[A](f: ((Long,V)) => A) { + if ((extraKeys & 1) == 1) f((0L, zeroValue.asInstanceOf[V])) + if ((extraKeys & 2) == 2) f((Long.MinValue, minValue.asInstanceOf[V])) var i,j = 0 while (i < _keys.length & j < _size) { val k = _keys(i) @@ -403,8 +405,6 @@ extends AbstractMap[Long, V] } i += 1 } - if ((extraKeys & 1) == 1) f((0L, zeroValue.asInstanceOf[V])) - if ((extraKeys & 2) == 2) f((Long.MinValue, minValue.asInstanceOf[V])) } override def clone(): LongMap[V] = { @@ -417,6 +417,8 @@ extends AbstractMap[Long, V] /** Applies a function to all keys of this map. */ def foreachKey[A](f: Long => A) { + if ((extraKeys & 1) == 1) f(0L) + if ((extraKeys & 2) == 2) f(Long.MinValue) var i,j = 0 while (i < _keys.length & j < _size) { val k = _keys(i) @@ -426,12 +428,12 @@ extends AbstractMap[Long, V] } i += 1 } - if ((extraKeys & 1) == 1) f(0L) - if ((extraKeys & 2) == 2) f(Long.MinValue) } /** Applies a function to all values of this map. */ def foreachValue[A](f: V => A) { + if ((extraKeys & 1) == 1) f(zeroValue.asInstanceOf[V]) + if ((extraKeys & 2) == 2) f(minValue.asInstanceOf[V]) var i,j = 0 while (i < _keys.length & j < _size) { val k = _keys(i) @@ -441,8 +443,6 @@ extends AbstractMap[Long, V] } i += 1 } - if ((extraKeys & 1) == 1) f(zeroValue.asInstanceOf[V]) - if ((extraKeys & 2) == 2) f(minValue.asInstanceOf[V]) } /** Creates a new `LongMap` with different values. @@ -450,6 +450,8 @@ extends AbstractMap[Long, V] * collection immediately. */ def mapValuesNow[V1](f: V => V1): LongMap[V1] = { + val zv = if ((extraKeys & 1) == 1) f(zeroValue.asInstanceOf[V]).asInstanceOf[AnyRef] else null + val mv = if ((extraKeys & 2) == 2) f(minValue.asInstanceOf[V]).asInstanceOf[AnyRef] else null val lm = new LongMap[V1](LongMap.exceptionDefault, 1, false) val kz = java.util.Arrays.copyOf(_keys, _keys.length) val vz = new Array[AnyRef](_values.length) @@ -462,8 +464,6 @@ extends AbstractMap[Long, V] } i += 1 } - val zv = if ((extraKeys & 1) == 1) f(zeroValue.asInstanceOf[V]).asInstanceOf[AnyRef] else null - val mv = if ((extraKeys & 2) == 2) f(minValue.asInstanceOf[V]).asInstanceOf[AnyRef] else null lm.initializeTo(mask, extraKeys, zv, mv, _size, _vacant, kz, vz) lm } @@ -472,6 +472,8 @@ extends AbstractMap[Long, V] * Note: the default, if any, is not transformed. */ def transformValues(f: V => V): this.type = { + if ((extraKeys & 1) == 1) zeroValue = f(zeroValue.asInstanceOf[V]).asInstanceOf[AnyRef] + if ((extraKeys & 2) == 2) minValue = f(minValue.asInstanceOf[V]).asInstanceOf[AnyRef] var i,j = 0 while (i < _keys.length & j < _size) { val k = _keys(i) @@ -481,26 +483,8 @@ extends AbstractMap[Long, V] } i += 1 } - if ((extraKeys & 1) == 1) zeroValue = f(zeroValue.asInstanceOf[V]).asInstanceOf[AnyRef] - if ((extraKeys & 2) == 2) minValue = f(minValue.asInstanceOf[V]).asInstanceOf[AnyRef] this } - - /* - override def toString = { - val sb = new StringBuilder("LongMap(") - var n = 0 - foreach{ case (k,v) => - if (n > 0) sb ++= ", " - sb ++= k.toString - sb ++= " -> " - sb ++= v.toString - n += 1 - } - sb += ')' - sb.result - } - */ } object LongMap { diff --git a/test/junit/scala/collection/SetMapConsistencyTest.scala b/test/junit/scala/collection/SetMapConsistencyTest.scala index eed6007eef..261c11a98b 100644 --- a/test/junit/scala/collection/SetMapConsistencyTest.scala +++ b/test/junit/scala/collection/SetMapConsistencyTest.scala @@ -514,4 +514,19 @@ class SetMapConsistencyTest { assert( hs.toList.toSet == hs ) assert( hs == hs.toList.toSet ) } + + @Test + def testSI8815() { + val lm = new scala.collection.mutable.LongMap[String] + lm += (Long.MinValue, "min") + lm += (-1, "neg-one") + lm += (0, "zero") + lm += (Long.MaxValue, "max") + var nit = 0 + lm.iterator.foreach(_ => nit += 1) + var nfe = 0 + lm.foreach(_ => nfe += 1) + assert(nit == 4) + assert(nfe == 4) + } } -- cgit v1.2.3 From 63207e115a46634d47446a87a7f4bc3c2651b0e7 Mon Sep 17 00:00:00 2001 From: Lukas Rytz Date: Wed, 10 Sep 2014 13:59:03 +0200 Subject: isAnonymousClass/Function for delambdafy classes is not true Ydelambdafy:method lambda classes are not anonymous classes, and not anonymous function classes either. They are somethig new, so there's a new predicate isDelambdafyFunction. They are not anonymous classes (or functions) because anonymous classes in Java speak are nested. Delambdafy classes are always top-level, they are just synthetic. Before this patch, isAnonymous was sometimes accidentailly true: if the lambda is nested in an anonymous class. Now it's always false. --- build.xml | 9 +++++-- .../tools/nsc/backend/jvm/BCodeAsmCommon.scala | 13 ++++------ .../scala/tools/nsc/transform/Delambdafy.scala | 28 ++++++++++++---------- src/reflect/scala/reflect/internal/StdNames.scala | 23 +++++++++--------- src/reflect/scala/reflect/internal/Symbols.scala | 1 + test/files/run/delambdafyLambdaClassNames.check | 1 + test/files/run/delambdafyLambdaClassNames.flags | 1 + .../files/run/delambdafyLambdaClassNames/A_1.scala | 5 ++++ .../run/delambdafyLambdaClassNames/Test.scala | 4 ++++ 9 files changed, 52 insertions(+), 33 deletions(-) create mode 100644 test/files/run/delambdafyLambdaClassNames.check create mode 100644 test/files/run/delambdafyLambdaClassNames.flags create mode 100644 test/files/run/delambdafyLambdaClassNames/A_1.scala create mode 100644 test/files/run/delambdafyLambdaClassNames/Test.scala (limited to 'test') diff --git a/build.xml b/build.xml index 6c750e530d..944feb2c10 100755 --- a/build.xml +++ b/build.xml @@ -1512,7 +1512,12 @@ TODO: - + + + + + + @@ -1530,7 +1535,7 @@ TODO: - + diff --git a/src/compiler/scala/tools/nsc/backend/jvm/BCodeAsmCommon.scala b/src/compiler/scala/tools/nsc/backend/jvm/BCodeAsmCommon.scala index e5b4c4a6c2..0c0d726630 100644 --- a/src/compiler/scala/tools/nsc/backend/jvm/BCodeAsmCommon.scala +++ b/src/compiler/scala/tools/nsc/backend/jvm/BCodeAsmCommon.scala @@ -22,14 +22,11 @@ final class BCodeAsmCommon[G <: Global](val global: G) { * null. */ def isAnonymousOrLocalClass(classSym: Symbol): Boolean = { - def isDelambdafyLambdaClass(classSym: Symbol): Boolean = { - classSym.isAnonymousFunction && (settings.Ydelambdafy.value == "method") - } - assert(classSym.isClass, s"not a class: $classSym") - - !isDelambdafyLambdaClass(classSym) && - (classSym.isAnonymousClass || !classSym.originalOwner.isClass) + val res = (classSym.isAnonymousClass || !classSym.originalOwner.isClass) + // lambda classes are always top-level classes. + if (res) assert(!classSym.isDelambdafyFunction) + res } /** @@ -81,7 +78,7 @@ final class BCodeAsmCommon[G <: Global](val global: G) { final case class EnclosingMethodEntry(owner: String, name: String, methodDescriptor: String) /** - * If data for emitting an EnclosingMethod attribute. None if `classSym` is a member class (not + * Data for emitting an EnclosingMethod attribute. None if `classSym` is a member class (not * an anonymous or local class). See doc in BTypes. * * The class is parametrized by two functions to obtain a bytecode class descriptor for a class diff --git a/src/compiler/scala/tools/nsc/transform/Delambdafy.scala b/src/compiler/scala/tools/nsc/transform/Delambdafy.scala index 9cc8d779a0..12e7b23f48 100644 --- a/src/compiler/scala/tools/nsc/transform/Delambdafy.scala +++ b/src/compiler/scala/tools/nsc/transform/Delambdafy.scala @@ -245,18 +245,22 @@ abstract class Delambdafy extends Transform with TypingTransformers with ast.Tre // - make `anonClass.isAnonymousClass` true. // - use `newAnonymousClassSymbol` or push the required variations into a similar factory method // - reinstate the assertion in `Erasure.resolveAnonymousBridgeClash` - val suffix = "$lambda$" + ( + val suffix = nme.DELAMBDAFY_LAMBDA_CLASS_NAME + "$" + ( if (funOwner.isPrimaryConstructor) "" else "$" + funOwner.name + "$" ) - val name = unit.freshTypeName(s"${oldClass.name.decode}$suffix") + val oldClassPart = oldClass.name.decode + // make sure the class name doesn't contain $anon, otherwsie isAnonymousClass/Function may be true + val name = unit.freshTypeName(s"$oldClassPart$suffix".replace("$anon", "$nestedInAnon")) - val anonClass = pkg newClassSymbol(name, originalFunction.pos, FINAL | SYNTHETIC) addAnnotation SerialVersionUIDAnnotation - anonClass setInfo ClassInfoType(parents, newScope, anonClass) + val lambdaClass = pkg newClassSymbol(name, originalFunction.pos, FINAL | SYNTHETIC) addAnnotation SerialVersionUIDAnnotation + lambdaClass setInfo ClassInfoType(parents, newScope, lambdaClass) + assert(!lambdaClass.isAnonymousClass && !lambdaClass.isAnonymousFunction, "anonymous class name: "+ lambdaClass.name) + assert(lambdaClass.isDelambdafyFunction, "not lambda class name: " + lambdaClass.name) val captureProxies2 = new LinkedHashMap[Symbol, TermSymbol] captures foreach {capture => - val sym = anonClass.newVariable(capture.name.toTermName, capture.pos, SYNTHETIC) + val sym = lambdaClass.newVariable(capture.name.toTermName, capture.pos, SYNTHETIC) sym setInfo capture.info captureProxies2 += ((capture, sym)) } @@ -266,30 +270,30 @@ abstract class Delambdafy extends Transform with TypingTransformers with ast.Tre val thisProxy = { val target = targetMethod(originalFunction) if (thisReferringMethods contains target) { - val sym = anonClass.newVariable(nme.FAKE_LOCAL_THIS, originalFunction.pos, SYNTHETIC) + val sym = lambdaClass.newVariable(nme.FAKE_LOCAL_THIS, originalFunction.pos, SYNTHETIC) sym.info = oldClass.tpe sym } else NoSymbol } - val decapturify = new DeCapturifyTransformer(captureProxies2, unit, oldClass, anonClass, originalFunction.symbol.pos, thisProxy) + val decapturify = new DeCapturifyTransformer(captureProxies2, unit, oldClass, lambdaClass, originalFunction.symbol.pos, thisProxy) val accessorMethod = createAccessorMethod(thisProxy, originalFunction) val decapturedFunction = decapturify.transform(originalFunction).asInstanceOf[Function] val members = (optionSymbol(thisProxy).toList ++ (captureProxies2 map (_._2))) map {member => - anonClass.info.decls enter member + lambdaClass.info.decls enter member ValDef(member, gen.mkZero(member.tpe)) setPos decapturedFunction.pos } // constructor - val constr = createConstructor(anonClass, members) + val constr = createConstructor(lambdaClass, members) // apply method with same arguments and return type as original lambda. - val applyMethodDef = createApplyMethod(anonClass, decapturedFunction, accessorMethod, thisProxy) + val applyMethodDef = createApplyMethod(lambdaClass, decapturedFunction, accessorMethod, thisProxy) - val bridgeMethod = createBridgeMethod(anonClass, originalFunction, applyMethodDef) + val bridgeMethod = createBridgeMethod(lambdaClass, originalFunction, applyMethodDef) def fulldef(sym: Symbol) = if (sym == NoSymbol) sym.toString @@ -305,7 +309,7 @@ abstract class Delambdafy extends Transform with TypingTransformers with ast.Tre val body = members ++ List(constr, applyMethodDef) ++ bridgeMethod // TODO if member fields are private this complains that they're not accessible - (localTyper.typedPos(decapturedFunction.pos)(ClassDef(anonClass, body)).asInstanceOf[ClassDef], thisProxy, accessorMethod) + (localTyper.typedPos(decapturedFunction.pos)(ClassDef(lambdaClass, body)).asInstanceOf[ClassDef], thisProxy, accessorMethod) } val (anonymousClassDef, thisProxy, accessorMethod) = makeAnonymousClass diff --git a/src/reflect/scala/reflect/internal/StdNames.scala b/src/reflect/scala/reflect/internal/StdNames.scala index d203218c09..99ff6a10b4 100644 --- a/src/reflect/scala/reflect/internal/StdNames.scala +++ b/src/reflect/scala/reflect/internal/StdNames.scala @@ -99,17 +99,18 @@ trait StdNames { val SINGLETON_SUFFIX: String = ".type" - val ANON_CLASS_NAME: NameType = "$anon" - val ANON_FUN_NAME: NameType = "$anonfun" - val EMPTY: NameType = "" - val EMPTY_PACKAGE_NAME: NameType = "" - val IMPL_CLASS_SUFFIX = "$class" - val IMPORT: NameType = "" - val MODULE_SUFFIX_NAME: NameType = MODULE_SUFFIX_STRING - val MODULE_VAR_SUFFIX: NameType = "$module" - val PACKAGE: NameType = "package" - val ROOT: NameType = "" - val SPECIALIZED_SUFFIX: NameType = "$sp" + val ANON_CLASS_NAME: NameType = "$anon" + val DELAMBDAFY_LAMBDA_CLASS_NAME: NameType = "$lambda" + val ANON_FUN_NAME: NameType = "$anonfun" + val EMPTY: NameType = "" + val EMPTY_PACKAGE_NAME: NameType = "" + val IMPL_CLASS_SUFFIX = "$class" + val IMPORT: NameType = "" + val MODULE_SUFFIX_NAME: NameType = MODULE_SUFFIX_STRING + val MODULE_VAR_SUFFIX: NameType = "$module" + val PACKAGE: NameType = "package" + val ROOT: NameType = "" + val SPECIALIZED_SUFFIX: NameType = "$sp" // value types (and AnyRef) are all used as terms as well // as (at least) arguments to the @specialize annotation. diff --git a/src/reflect/scala/reflect/internal/Symbols.scala b/src/reflect/scala/reflect/internal/Symbols.scala index 2670faa22d..44fce2c9ab 100644 --- a/src/reflect/scala/reflect/internal/Symbols.scala +++ b/src/reflect/scala/reflect/internal/Symbols.scala @@ -789,6 +789,7 @@ trait Symbols extends api.Symbols { self: SymbolTable => isMethod && owner.isDerivedValueClass && !isParamAccessor && !isConstructor && !hasFlag(SUPERACCESSOR) && !isMacro && !isSpecialized final def isAnonymousFunction = isSynthetic && (name containsName tpnme.ANON_FUN_NAME) + final def isDelambdafyFunction = isSynthetic && (name containsName tpnme.DELAMBDAFY_LAMBDA_CLASS_NAME) final def isDefinedInPackage = effectiveOwner.isPackageClass final def needsFlatClasses = phase.flatClasses && rawowner != NoSymbol && !rawowner.isPackageClass diff --git a/test/files/run/delambdafyLambdaClassNames.check b/test/files/run/delambdafyLambdaClassNames.check new file mode 100644 index 0000000000..d425d15dd0 --- /dev/null +++ b/test/files/run/delambdafyLambdaClassNames.check @@ -0,0 +1 @@ +A$$nestedInAnon$1$lambda$$run$1 diff --git a/test/files/run/delambdafyLambdaClassNames.flags b/test/files/run/delambdafyLambdaClassNames.flags new file mode 100644 index 0000000000..b10233d322 --- /dev/null +++ b/test/files/run/delambdafyLambdaClassNames.flags @@ -0,0 +1 @@ +-Ybackend:GenBCode -Ydelambdafy:method \ No newline at end of file diff --git a/test/files/run/delambdafyLambdaClassNames/A_1.scala b/test/files/run/delambdafyLambdaClassNames/A_1.scala new file mode 100644 index 0000000000..10489414b7 --- /dev/null +++ b/test/files/run/delambdafyLambdaClassNames/A_1.scala @@ -0,0 +1,5 @@ +class A { + def f = new Runnable { + def run(): Unit = List(1,2).foreach(println) + } +} diff --git a/test/files/run/delambdafyLambdaClassNames/Test.scala b/test/files/run/delambdafyLambdaClassNames/Test.scala new file mode 100644 index 0000000000..49a397d1d2 --- /dev/null +++ b/test/files/run/delambdafyLambdaClassNames/Test.scala @@ -0,0 +1,4 @@ +object Test extends App { + val c = Class.forName("A$$nestedInAnon$1$lambda$$run$1") + println(c.getName) +} -- cgit v1.2.3 From 039f3e3b35f73886f79f00bc578739002fe034cc Mon Sep 17 00:00:00 2001 From: Rex Kerr Date: Sat, 28 Jun 2014 03:24:39 -0700 Subject: SI-8680 Stream.addString is too eager Used the standard method of sending out two iterators, one twice as fast as the others, to avoid hanging on .force, .hasDefiniteSize, and .addString. .addString appends a "..." as the last element if it detects a cycle. It knows how to print the cycle length, but there's no good way to specify what you want right now, so it's not used. Added tests in t8680 that verify that cyclic streams give the expected results. Added to whitelist names of methods formerly used for recursion (now looping). --- bincompat-backward.whitelist.conf | 13 +++ bincompat-forward.whitelist.conf | 21 ++++ .../scala/collection/immutable/Stream.scala | 125 ++++++++++++++++++--- test/files/run/t8680.scala | 53 +++++++++ 4 files changed, 198 insertions(+), 14 deletions(-) create mode 100644 test/files/run/t8680.scala (limited to 'test') diff --git a/bincompat-backward.whitelist.conf b/bincompat-backward.whitelist.conf index 1c48887b63..6c98dc62a1 100644 --- a/bincompat-backward.whitelist.conf +++ b/bincompat-backward.whitelist.conf @@ -194,6 +194,19 @@ filter { { matchName="scala.collection.immutable.Stream.filteredTail" problemName=MissingMethodProblem + }, + // https://github.com/scala/scala/pull/3848 -- SI-8680 + { + matchName="scala.collection.immutable.Stream.scala$collection$immutable$Stream$$loop$6" + problemName=MissingMethodProblem + }, + { + matchName="scala.collection.immutable.Stream.scala$collection$immutable$Stream$$loop$5" + problemName=MissingMethodProblem + }, + { + matchName="scala.collection.immutable.Stream.scala$collection$immutable$Stream$$loop$4" + problemName=MissingMethodProblem } ] } diff --git a/bincompat-forward.whitelist.conf b/bincompat-forward.whitelist.conf index ec80e7cce9..87a59f2d53 100644 --- a/bincompat-forward.whitelist.conf +++ b/bincompat-forward.whitelist.conf @@ -368,6 +368,27 @@ filter { { matchName="scala.reflect.io.AbstractFile.filterImpl" problemName=MissingMethodProblem + }, + // https://github.com/scala/scala/pull/3848 -- SI-8680 + { + matchName="scala.collection.immutable.Stream.scala$collection$immutable$Stream$$loop$6" + problemName=MissingMethodProblem + }, + { + matchName="scala.collection.immutable.Stream.scala$collection$immutable$Stream$$loop$5" + problemName=MissingMethodProblem + }, + { + matchName="scala.collection.immutable.Stream.scala$collection$immutable$Stream$$loop$4" + problemName=MissingMethodProblem + }, + { + matchName="scala.collection.immutable.Stream.scala$collection$immutable$Stream$$loop$3" + problemName=MissingMethodProblem + }, + { + matchName="scala.collection.immutable.Stream.scala$collection$immutable$Stream$$loop$2" + problemName=MissingMethodProblem } ] } diff --git a/src/library/scala/collection/immutable/Stream.scala b/src/library/scala/collection/immutable/Stream.scala index 1f97c4c769..91a4e1c43d 100644 --- a/src/library/scala/collection/immutable/Stream.scala +++ b/src/library/scala/collection/immutable/Stream.scala @@ -176,6 +176,12 @@ import scala.language.implicitConversions * loop(1, 1) * } * }}} + * + * Note that `mkString` forces evaluation of a `Stream`, but `addString` does + * not. In both cases, a `Stream` that is or ends in a cycle + * (e.g. `lazy val s: Stream[Int] = 0 #:: s`) will convert additional trips + * through the cycle to `...`. Additionally, `addString` will display an + * un-memoized tail as `?`. * * @tparam A the type of the elements contained in this stream. * @@ -253,12 +259,22 @@ self => * @note Often we use `Stream`s to represent an infinite set or series. If * that's the case for your particular `Stream` then this function will never * return and will probably crash the VM with an `OutOfMemory` exception. + * This function will not hang on a finite cycle, however. * * @return The fully realized `Stream`. */ def force: Stream[A] = { - var these = this - while (!these.isEmpty) these = these.tail + // Use standard 2x 1x iterator trick for cycle detection ("those" is slow one) + var these, those = this + if (!these.isEmpty) these = these.tail + while (those ne these) { + if (these.isEmpty) return this + these = these.tail + if (these.isEmpty) return this + these = these.tail + if (these eq those) return this + those = those.tail + } this } @@ -309,9 +325,24 @@ self => override def toStream: Stream[A] = this - override def hasDefiniteSize = { - def loop(s: Stream[A]): Boolean = s.isEmpty || s.tailDefined && loop(s.tail) - loop(this) + override def hasDefiniteSize: Boolean = isEmpty || { + if (!tailDefined) false + else { + // Two-iterator trick (2x & 1x speed) for cycle detection. + var those = this + var these = tail + while (those ne these) { + if (these.isEmpty) return true + if (!these.tailDefined) return false + these = these.tail + if (these.isEmpty) return true + if (!these.tailDefined) return false + these = these.tail + if (those eq these) return false + those = those.tail + } + false // Cycle detected + } } /** Create a new stream which contains all elements of this stream followed by @@ -690,7 +721,8 @@ self => * `end`. Inside, the string representations of defined elements (w.r.t. * the method `toString()`) are separated by the string `sep`. The method will * not force evaluation of undefined elements. A tail of such elements will be - * represented by a `"?"` instead. + * represented by a `"?"` instead. A cyclic stream is represented by a `"..."` + * at the point where the cycle repeats. * * @param b The [[collection.mutable.StringBuilder]] factory to which we need * to add the string elements. @@ -701,16 +733,81 @@ self => * resulting string. */ override def addString(b: StringBuilder, start: String, sep: String, end: String): StringBuilder = { - def loop(pre: String, these: Stream[A]) { - if (these.isEmpty) b append end - else { - b append pre append these.head - if (these.tailDefined) loop(sep, these.tail) - else b append sep append "?" append end + b append start + if (!isEmpty) { + b append head + var cursor = this + var n = 1 + if (cursor.tailDefined) { // If tailDefined, also !isEmpty + var scout = tail + if (scout.isEmpty) { + // Single element. Bail out early. + b append end + return b + } + if ((cursor ne scout) && scout.tailDefined) { + cursor = scout + scout = scout.tail + // Use 2x 1x iterator trick for cycle detection; slow iterator can add strings + while ((cursor ne scout) && scout.tailDefined) { + b append sep append cursor.head + n += 1 + cursor = cursor.tail + scout = scout.tail + if (scout.tailDefined) scout = scout.tail + } + } + if (!scout.tailDefined) { // Not a cycle, scout hit an end + while (cursor ne scout) { + b append sep append cursor.head + n += 1 + cursor = cursor.tail + } + } + else { + // Cycle. + // If we have a prefix of length P followed by a cycle of length C, + // the scout will be at position (P%C) in the cycle when the cursor + // enters it at P. They'll then collide when the scout advances another + // C - (P%C) ahead of the cursor. + // If we run the scout P farther, then it will be at the start of + // the cycle: (C - (P%C) + (P%C)) == C == 0. So if another runner + // starts at the beginning of the prefix, they'll collide exactly at + // the start of the loop. + var runner = this + var k = 0 + while (runner ne scout) { + runner = runner.tail + scout = scout.tail + k += 1 + } + // Now runner and scout are at the beginning of the cycle. Advance + // cursor, adding to string, until it hits; then we'll have covered + // everything once. If cursor is already at beginning, we'd better + // advance one first unless runner didn't go anywhere (in which case + // we've already looped once). + if ((cursor eq scout) && (k > 0)) { + b append sep append cursor.head + n += 1 + cursor = cursor.tail + } + while (cursor ne scout) { + b append sep append cursor.head + n += 1 + cursor = cursor.tail + } + // Subtract prefix length from total length for cycle reporting. + // (Not currently used, but probably a good idea for the future.) + n -= k + } + } + if (!cursor.isEmpty) { + // Either undefined or cyclic; we can check with tailDefined + if (!cursor.tailDefined) b append sep append "?" + else b append sep append "..." } } - b append start - loop("", this) + b append end b } diff --git a/test/files/run/t8680.scala b/test/files/run/t8680.scala new file mode 100644 index 0000000000..2bce09c507 --- /dev/null +++ b/test/files/run/t8680.scala @@ -0,0 +1,53 @@ +object Test extends App { + def pre(n: Int) = (-n to -1).toStream + + def cyc(m: Int) = { + lazy val s: Stream[Int] = (0 until m).toStream #::: s + s + } + + def precyc(n: Int, m: Int) = pre(n) #::: cyc(m) + + def str(s: Stream[Int]) = { + val b = new StringBuilder + s.addString(b, "", "", "") + b.toString + } + + def goal(n: Int, m: Int) = (-n until m).mkString + "..." + + // Check un-forced cyclic and non-cyclic streams + assert(str(pre(2)) == pre(2).take(1).toList.mkString + "?") + assert(str(cyc(2)) == cyc(2).take(1).toList.mkString + "?") + assert(str(precyc(2,2)) == precyc(2,2).take(1).toList.mkString + "?") + assert(!pre(2).hasDefiniteSize) + assert(!cyc(2).hasDefiniteSize) + assert(!precyc(2,2).hasDefiniteSize) + + // Check forced cyclic and non-cyclic streams + assert(str(pre(2).force) == (-2 to -1).mkString) + assert(str(cyc(2).force) == (0 until 2).mkString + "...") + assert(str(precyc(2,2).force) == (-2 until 2).mkString + "...") + assert(pre(2).force.hasDefiniteSize) + assert(!cyc(2).force.hasDefiniteSize) + assert(!precyc(2,2).force.hasDefiniteSize) + + // Special cases + assert(str(cyc(1).force) == goal(0,1)) + assert(str(precyc(1,6).force) == goal(1,6)) + assert(str(precyc(6,1).force) == goal(6,1)) + + // Make sure there are no odd/even problems + for (n <- 3 to 4; m <- 3 to 4) { + assert(precyc(n,m).mkString == goal(n,m), s"mkString $n $m") + assert(!precyc(n,m).force.hasDefiniteSize, s"hasDef $n$m") + } + + // Make sure there are no cycle/prefix modulus problems + for (i <- 6 to 8) { + assert(precyc(i,3).mkString == goal(i,3), s"mkString $i 3") + assert(precyc(3,i).mkString == goal(3,i), s"mkString 3 $i") + assert(!precyc(i,3).force.hasDefiniteSize, s"hasDef $i 3") + assert(!precyc(3,i).force.hasDefiniteSize, s"hasDef 3 $i") + } +} -- cgit v1.2.3