Permalink
Browse files

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 (9954eaf, 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 (0bea2ab), 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).
  • Loading branch information...
1 parent b7b4f87 commit 386a5bd68de3a95563d61b2b305e012c157f3b89 @Blaisorblade Blaisorblade committed Mar 15, 2013
View
3 src/compiler/scala/tools/nsc/backend/jvm/GenASM.scala
@@ -2260,15 +2260,16 @@ 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)
)
// whether to reference the type of the receiver or
// 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)
View
3 src/compiler/scala/tools/nsc/backend/jvm/GenJVM.scala
@@ -1197,15 +1197,16 @@ 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)
)
// whether to reference the type of the receiver or
// 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)
View
1 test/files/jvm/t7253.check
@@ -0,0 +1 @@
+bytecode identical
View
5 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
View
9 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();
+ }
+}
View
9 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()
+ }
+}
View
28 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")
+ }
+}

0 comments on commit 386a5bd

Please sign in to comment.