From 386a5bd68de3a95563d61b2b305e012c157f3b89 Mon Sep 17 00:00:00 2001 From: "Paolo G. Giarrusso" Date: Fri, 15 Mar 2013 21:42:15 +0100 Subject: SI-7253: respect binary compatibility constraints From the JLS one can prove that moving a method to a superclass is a binary compatible change, both forward and backward. That's because when compiling a method call `c.foo()`, where c: C, the output descriptor *must* refer to `C` and not to the class where `foo()` is actually defined. This patch just ensures that, and adds a test comparing generated descriptors against the Javac output. The sample code is from Paul Philipps, the fix and the bytecode comparison code from me. From 2006 (9954eafffd5e60676238369ab0ed5797c92b4a7b, a fix for bug 455 in the old bug tracker http://www.scala-lang.org/sites/default/files/aladdin/displayItem.do%3Fid=455.html) until 2.9, Scalac has followed this rule "often" (that is, when C is *not* an interface). This behavior was wrong, but the bug was hard to trigger. AFAICS, this can create problems only when moving a method to a super interface in a library and expecting forward binary compatibility - that is, compiling some Scala client code against the new version of the library, and trying to run this code against the old version of the library. This change grows an interface, so it is valid only if clients are supposed to *not* implement the library. Apparently, this is so rare that nobody noticed. Since 2.10 (0bea2ab5f6b211a83bbf14ea46fe57b8163c6334), Scalac follows this rule *only* when C is an interface (I assume by oversight, since the main change was an accessibility check), so the bug was finally triggered. The new code will have to emit INVOKEINTERFACE instead of INVOKEVIRTUAL a bit more often, compared to 2.9 (but not to 2.10). I don't know whether INVOKEINTERFACE is noticeably slower (it shouldn't be); but this is the safest fix since this behavior is mandated by the JLS. If somebody disagrees and believes the 2.9 is significantly faster, IMHO he should send a separate pull request (although ProGuard is probably a better place for the change). --- .../scala/tools/nsc/backend/jvm/GenASM.scala | 3 ++- .../scala/tools/nsc/backend/jvm/GenJVM.scala | 3 ++- test/files/jvm/t7253.check | 1 + test/files/jvm/t7253/Base_1.scala | 5 ++++ test/files/jvm/t7253/JavaClient_1.java | 9 +++++++ test/files/jvm/t7253/ScalaClient_1.scala | 9 +++++++ test/files/jvm/t7253/test.scala | 28 ++++++++++++++++++++++ 7 files changed, 56 insertions(+), 2 deletions(-) create mode 100644 test/files/jvm/t7253.check create mode 100644 test/files/jvm/t7253/Base_1.scala create mode 100644 test/files/jvm/t7253/JavaClient_1.java create mode 100644 test/files/jvm/t7253/ScalaClient_1.scala create mode 100644 test/files/jvm/t7253/test.scala diff --git a/src/compiler/scala/tools/nsc/backend/jvm/GenASM.scala b/src/compiler/scala/tools/nsc/backend/jvm/GenASM.scala index 9b16327ffc..55b20154de 100644 --- a/src/compiler/scala/tools/nsc/backend/jvm/GenASM.scala +++ b/src/compiler/scala/tools/nsc/backend/jvm/GenASM.scala @@ -2260,6 +2260,7 @@ abstract class GenASM extends SubComponent with BytecodeWriters with GenJVMASM { hostSymbol.info ; methodOwner.info def isInterfaceCall(sym: Symbol) = ( + //XXX remove the test for ObjectClass. sym.isInterface && methodOwner != ObjectClass || sym.isJavaDefined && sym.isNonBottomSubClass(ClassfileAnnotationClass) ) @@ -2267,8 +2268,8 @@ abstract class GenASM extends SubComponent with BytecodeWriters with GenJVMASM { // the type of the method owner (if not an interface!) val useMethodOwner = ( style != Dynamic - || !isInterfaceCall(hostSymbol) && isAccessibleFrom(methodOwner, siteSymbol) || hostSymbol.isBottomClass + || methodOwner == ObjectClass ) val receiver = if (useMethodOwner) methodOwner else hostSymbol val jowner = javaName(receiver) diff --git a/src/compiler/scala/tools/nsc/backend/jvm/GenJVM.scala b/src/compiler/scala/tools/nsc/backend/jvm/GenJVM.scala index 598965b982..ed0d8144ca 100644 --- a/src/compiler/scala/tools/nsc/backend/jvm/GenJVM.scala +++ b/src/compiler/scala/tools/nsc/backend/jvm/GenJVM.scala @@ -1197,6 +1197,7 @@ abstract class GenJVM extends SubComponent with GenJVMUtil with GenAndroid with hostSymbol.info ; methodOwner.info def isInterfaceCall(sym: Symbol) = ( + //XXX remove the test for ObjectClass. sym.isInterface && methodOwner != ObjectClass || sym.isJavaDefined && sym.isNonBottomSubClass(ClassfileAnnotationClass) ) @@ -1204,8 +1205,8 @@ abstract class GenJVM extends SubComponent with GenJVMUtil with GenAndroid with // the type of the method owner (if not an interface!) val useMethodOwner = ( style != Dynamic - || !isInterfaceCall(hostSymbol) && isAccessibleFrom(methodOwner, siteSymbol) || hostSymbol.isBottomClass + || methodOwner == ObjectClass ) val receiver = if (useMethodOwner) methodOwner else hostSymbol val jowner = javaName(receiver) diff --git a/test/files/jvm/t7253.check b/test/files/jvm/t7253.check new file mode 100644 index 0000000000..43f53aba12 --- /dev/null +++ b/test/files/jvm/t7253.check @@ -0,0 +1 @@ +bytecode identical diff --git a/test/files/jvm/t7253/Base_1.scala b/test/files/jvm/t7253/Base_1.scala new file mode 100644 index 0000000000..a531ebb69d --- /dev/null +++ b/test/files/jvm/t7253/Base_1.scala @@ -0,0 +1,5 @@ +trait A { def f(): Int } +trait B1 extends A +abstract class B2 extends A +class B3 extends A { def f(): Int = 1 } +class B4 extends B3 diff --git a/test/files/jvm/t7253/JavaClient_1.java b/test/files/jvm/t7253/JavaClient_1.java new file mode 100644 index 0000000000..43475de2f5 --- /dev/null +++ b/test/files/jvm/t7253/JavaClient_1.java @@ -0,0 +1,9 @@ +public class JavaClient_1 { + int foo() { + ((A) null).f(); + ((B1) null).f(); + ((B2) null).f(); + ((B3) null).f(); + return ((B4) null).f(); + } +} diff --git a/test/files/jvm/t7253/ScalaClient_1.scala b/test/files/jvm/t7253/ScalaClient_1.scala new file mode 100644 index 0000000000..d244b326a8 --- /dev/null +++ b/test/files/jvm/t7253/ScalaClient_1.scala @@ -0,0 +1,9 @@ +class ScalaClient_1 { + def foo() = { + (null: A).f() + (null: B1).f() + (null: B2).f() + (null: B3).f() + (null: B4).f() + } +} diff --git a/test/files/jvm/t7253/test.scala b/test/files/jvm/t7253/test.scala new file mode 100644 index 0000000000..7fe08e8813 --- /dev/null +++ b/test/files/jvm/t7253/test.scala @@ -0,0 +1,28 @@ +import scala.tools.partest.BytecodeTest + +import scala.tools.nsc.util.JavaClassPath +import java.io.InputStream +import scala.tools.asm +import asm.ClassReader +import asm.tree.{ClassNode, InsnList} +import scala.collection.JavaConverters._ + +object Test extends BytecodeTest { + import instructions._ + + def show: Unit = { + val instrBaseSeqs = Seq("ScalaClient_1", "JavaClient_1") map (name => instructions.fromMethod(getMethod(loadClassNode(name), "foo"))) + val instrSeqs = instrBaseSeqs map (_ filter isInvoke) + cmpInstructions(instrSeqs(0), instrSeqs(1)) + } + + def cmpInstructions(isa: List[Instruction], isb: List[Instruction]) = { + if (isa == isb) println("bytecode identical") + else diffInstructions(isa, isb) + } + + def isInvoke(node: Instruction): Boolean = { + val opcode = node.opcode + (opcode == "INVOKEVIRTUAL") || (opcode == "INVOKEINTERFACE") + } +} -- cgit v1.2.3