backend: Emit calls using the correct receiver

This is a port of scala/scala@765eb29: When emitting a virtual call, the receiver in the bytecode cannot just be the method's owner (the class in which it is declared), because that class may not be accessible at the callsite. Instead we use the type of the receiver. This was basically done to fix - aladdin bug 455 (9954eaf) - SI-1430 (0bea2ab) - basically the same bug, slightly different - SI-4283 (8707c9e) - the same for field reads In Dotty, we rarely encountered this issue because under separate compilation, we see the bridge symbols generated by javac and use their owner as the receiver, but under joint compilation of .java and .scala sources this doesn't happen, in particular this commit fixes scala#6546. It also means that we should now be able to stop creating symbols in the ClassfileParser for Java bridges, this would make joint and separate compilation more similar and thus should reduce the number of bugs which appear in one but not the other as discussed in scala#6266. After this commit, some more changes are necessary to get the updated backend to work correctly with dotty, these are implemented in the later commits of this PR. Co-Authored-By: Lukas Rytz <[email protected]>
dotty-staging · Mar 12, 2020 · fa42b81 · fa42b81
1 parent bf283ee
commit fa42b81
Show file tree

Hide file tree

Showing 11 changed files with 422 additions and 159 deletions.
diff --git a/compiler/src/dotty/tools/backend/jvm/BCodeBodyBuilder.scala b/compiler/src/dotty/tools/backend/jvm/BCodeBodyBuilder.scala
diff --git a/compiler/src/dotty/tools/backend/jvm/BCodeIdiomatic.scala b/compiler/src/dotty/tools/backend/jvm/BCodeIdiomatic.scala
@@ -6,6 +6,7 @@ import scala.tools.asm
 import scala.annotation.switch
 import scala.collection.mutable
 import Primitives.{NE, EQ, TestOp, ArithmeticOp}
+import scala.tools.asm.tree.MethodInsnNode
 
 /*
  *  A high-level facade to the ASM API for bytecode generation.
@@ -104,7 +105,7 @@ trait BCodeIdiomatic {
    */
   abstract class JCodeMethodN {
 
-    def jmethod: asm.MethodVisitor
+    def jmethod: asm.tree.MethodNode
 
     import asm.Opcodes;
 
@@ -394,21 +395,27 @@ trait BCodeIdiomatic {
 
     // can-multi-thread
     final def invokespecial(owner: String, name: String, desc: String, itf: Boolean): Unit = {
-      jmethod.visitMethodInsn(Opcodes.INVOKESPECIAL, owner, name, desc, itf)
+      emitInvoke(Opcodes.INVOKESPECIAL, owner, name, desc, itf)
     }
     // can-multi-thread
     final def invokestatic(owner: String, name: String, desc: String, itf: Boolean): Unit = {
-      jmethod.visitMethodInsn(Opcodes.INVOKESTATIC, owner, name, desc, itf)
+      emitInvoke(Opcodes.INVOKESTATIC, owner, name, desc, itf)
     }
     // can-multi-thread
     final def invokeinterface(owner: String, name: String, desc: String): Unit = {
-      jmethod.visitMethodInsn(Opcodes.INVOKEINTERFACE, owner, name, desc, true)
+      emitInvoke(Opcodes.INVOKEINTERFACE, owner, name, desc, itf = true)
     }
     // can-multi-thread
     final def invokevirtual(owner: String, name: String, desc: String): Unit = {
-      jmethod.visitMethodInsn(Opcodes.INVOKEVIRTUAL, owner, name, desc, false)
+      emitInvoke(Opcodes.INVOKEVIRTUAL, owner, name, desc, itf = false)
     }
 
+    def emitInvoke(opcode: Int, owner: String, name: String, desc: String, itf: Boolean): Unit = {
+      val node = new MethodInsnNode(opcode, owner, name, desc, itf)
+      jmethod.instructions.add(node)
+    }
+
+
     // can-multi-thread
     final def goTo(label: asm.Label): Unit = { jmethod.visitJumpInsn(Opcodes.GOTO, label) }
     // can-multi-thread

diff --git a/compiler/src/dotty/tools/backend/jvm/BackendInterface.scala b/compiler/src/dotty/tools/backend/jvm/BackendInterface.scala
@@ -442,6 +442,8 @@ abstract class BackendInterface extends BackendInterfaceDefinitions {
   }
 
   abstract class SymbolHelper {
+    def exists: Boolean
+
     // names
     def showFullName: String
     def javaSimpleName: String
@@ -534,6 +536,7 @@ abstract class BackendInterface extends BackendInterfaceDefinitions {
     def enclosingClassSym: Symbol
     def originalLexicallyEnclosingClass: Symbol
     def nextOverriddenSymbol: Symbol
+    def allOverriddenSymbols: List[Symbol]
 
 
     // members
@@ -603,6 +606,7 @@ abstract class BackendInterface extends BackendInterfaceDefinitions {
      */
     def sortedMembersBasedOnFlags(required: Flags, excluded: Flags): List[Symbol]
     def members: List[Symbol]
+    def decl(name: Name): Symbol
     def decls: List[Symbol]
     def underlying: Type
     def parents: List[Type]

diff --git a/compiler/src/dotty/tools/backend/jvm/DottyBackendInterface.scala b/compiler/src/dotty/tools/backend/jvm/DottyBackendInterface.scala
@@ -627,6 +627,8 @@ class DottyBackendInterface(outputDirectory: AbstractFile, val superCallsMap: Ma
   }
 
   implicit def symHelper(sym: Symbol): SymbolHelper = new SymbolHelper {
+    def exists: Boolean = sym.exists
+
     // names
     def showFullName: String = sym.showFullName
     def javaSimpleName: String = toDenot(sym).name.mangledString // addModuleSuffix(simpleName.dropLocal)
@@ -680,7 +682,7 @@ class DottyBackendInterface(outputDirectory: AbstractFile, val superCallsMap: Ma
       (sym.is(Flags.JavaStatic) || toDenot(sym).hasAnnotation(ctx.definitions.ScalaStaticAnnot))
       // guard against no sumbol cause this code is executed to select which call type(static\dynamic) to use to call array.clone
 
-    def isBottomClass: Boolean = (sym ne defn.NullClass) && (sym ne defn.NothingClass)
+    def isBottomClass: Boolean = (sym eq defn.NullClass) || (sym eq defn.NothingClass)
     def isBridge: Boolean = sym.is(Flags.Bridge)
     def isArtifact: Boolean = sym.is(Flags.Artifact)
     def hasEnumFlag: Boolean = sym.isAllOf(Flags.JavaEnumTrait)
@@ -759,12 +761,14 @@ class DottyBackendInterface(outputDirectory: AbstractFile, val superCallsMap: Ma
         toDenot(sym)(shiftedContext).lexicallyEnclosingClass(shiftedContext)
       } else NoSymbol
     def nextOverriddenSymbol: Symbol = toDenot(sym).nextOverriddenSymbol
+    def allOverriddenSymbols: List[Symbol] = toDenot(sym).allOverriddenSymbols.toList
 
     // members
     def primaryConstructor: Symbol = toDenot(sym).primaryConstructor
 
     /** For currently compiled classes: All locally defined classes including local classes.
      *  The empty list for classes that are not currently compiled.
+
      */
     def nestedClasses: List[Symbol] = definedClasses(ctx.flattenPhase)
 
@@ -876,6 +880,8 @@ class DottyBackendInterface(outputDirectory: AbstractFile, val superCallsMap: Ma
 
     def memberInfo(s: Symbol): Type = tp.memberInfo(s)
 
+    def decl(name: Name): Symbol = tp.decl(name).symbol
+
     def decls: List[Symbol] = tp.decls.toList
 
     def members: List[Symbol] = tp.allMembers.map(_.symbol).toList

diff --git a/compiler/test/dotty/tools/backend/jvm/DottyBytecodeTest.scala b/compiler/test/dotty/tools/backend/jvm/DottyBytecodeTest.scala
@@ -24,6 +24,7 @@ import java.io.{File => JFile, InputStream}
 trait DottyBytecodeTest {
   import AsmNode._
   import ASMConverters._
+  import DottyBytecodeTest._
 
   protected object Opcode {
     val newarray       = 188
@@ -81,6 +82,24 @@ trait DottyBytecodeTest {
     classNode.fields.asScala.find(_.name == name) getOrElse
       sys.error(s"Didn't find field '$name' in class '${classNode.name}'")
 
+  def getInstructions(c: ClassNode, name: String): List[Instruction] =
+    instructionsFromMethod(getMethod(c, name))
+
+  def assertSameCode(method: MethodNode, expected: List[Instruction]): Unit =
+    assertSameCode(instructionsFromMethod(method).dropNonOp, expected)
+  def assertSameCode(actual: List[Instruction], expected: List[Instruction]): Unit = {
+    assert(actual === expected, s"\nExpected: $expected\nActual  : $actual")
+  }
+
+  def assertInvoke(m: MethodNode, receiver: String, method: String): Unit =
+    assertInvoke(instructionsFromMethod(m), receiver, method)
+  def assertInvoke(l: List[Instruction], receiver: String, method: String): Unit = {
+    assert(l.exists {
+      case Invoke(_, `receiver`, `method`, _, _) => true
+      case _ => false
+    }, l.stringLines)
+  }
+
   def diffInstructions(isa: List[Instruction], isb: List[Instruction]): String = {
     val len = Math.max(isa.length, isb.length)
     val sb = new StringBuilder
@@ -194,3 +213,9 @@ trait DottyBytecodeTest {
     )
   }
 }
+object DottyBytecodeTest {
+  implicit class listStringLines[T](val l: List[T]) extends AnyVal {
+    def stringLines = l.mkString("\n")
+  }
+}
+
diff --git a/compiler/test/dotty/tools/backend/jvm/DottyBytecodeTests.scala b/compiler/test/dotty/tools/backend/jvm/DottyBytecodeTests.scala
@@ -8,6 +8,8 @@ import asm._
 import asm.tree._
 
 import scala.tools.asm.Opcodes
+import scala.jdk.CollectionConverters._
+
 
 class TestBCode extends DottyBytecodeTest {
   import ASMConverters._
@@ -783,4 +785,134 @@ class TestBCode extends DottyBytecodeTest {
       assertParamName(b2, "evidence$2")
     }
   }
+
+  @Test
+  def invocationReceivers(): Unit = {
+    import Opcodes._
+
+    checkBCode(List(invocationReceiversTestCode.definitions("Object"))) { dir =>
+      val c1 = loadClassNode(dir.lookupName("C1.class", directory = false).input)
+      val c2 = loadClassNode(dir.lookupName("C2.class", directory = false).input)
+      // Scala 2 uses "invokestatic T.clone$" here, see https://github.com/lampepfl/dotty/issues/5928
+      assertSameCode(getMethod(c1, "clone"), List(VarOp(ALOAD, 0), Invoke(INVOKESPECIAL, "T", "clone", "()Ljava/lang/Object;", true), Op(ARETURN)))
+      assertInvoke(getMethod(c1, "f1"), "T", "clone")
+      assertInvoke(getMethod(c1, "f2"), "T", "clone")
+      assertInvoke(getMethod(c1, "f3"), "C1", "clone")
+      assertInvoke(getMethod(c2, "f1"), "T", "clone")
+      assertInvoke(getMethod(c2, "f2"), "T", "clone")
+      assertInvoke(getMethod(c2, "f3"), "C1", "clone")
+    }
+    checkBCode(List(invocationReceiversTestCode.definitions("String"))) { dir =>
+      val c1b = loadClassNode(dir.lookupName("C1.class", directory = false).input)
+      val c2b = loadClassNode(dir.lookupName("C2.class", directory = false).input)
+      val tb = loadClassNode(dir.lookupName("T.class", directory = false).input)
+      val ub = loadClassNode(dir.lookupName("U.class", directory = false).input)
+
+      def ms(c: ClassNode, n: String) = c.methods.asScala.toList.filter(_.name == n)
+      assert(ms(tb, "clone").length == 1)
+      assert(ms(ub, "clone").isEmpty)
+      val List(c1Clone) = ms(c1b, "clone").filter(_.desc == "()Ljava/lang/Object;")
+      assert((c1Clone.access | Opcodes.ACC_BRIDGE) != 0)
+      assertSameCode(c1Clone, List(VarOp(ALOAD, 0), Invoke(INVOKEVIRTUAL, "C1", "clone", "()Ljava/lang/String;", false), Op(ARETURN)))
+
+      def iv(m: MethodNode) = getInstructions(c1b, "f1").collect({case i: Invoke => i})
+      assertSameCode(iv(getMethod(c1b, "f1")), List(Invoke(INVOKEINTERFACE, "T", "clone", "()Ljava/lang/String;", true)))
+      assertSameCode(iv(getMethod(c1b, "f2")), List(Invoke(INVOKEINTERFACE, "T", "clone", "()Ljava/lang/String;", true)))
+      // invokeinterface T.clone in C1 is OK here because it is not an override of Object.clone (different signature)
+      assertSameCode(iv(getMethod(c1b, "f3")), List(Invoke(INVOKEINTERFACE, "T", "clone", "()Ljava/lang/String;", true)))
+    }
+  }
+
+  @Test
+  def invocationReceiversProtected(): Unit = {
+    // http://lrytz.github.io/scala-aladdin-bugtracker/displayItem.do%3Fid=455.html / 9954eaf
+    // also https://github.com/scala/bug/issues/1430 / 0bea2ab (same but with interfaces)
+    val aC =
+      """package a;
+        |/*package private*/ abstract class A {
+        |  public int f() { return 1; }
+        |  public int t;
+        |}
+      """.stripMargin
+    val bC =
+      """package a;
+        |public class B extends A { }
+      """.stripMargin
+    val iC =
+      """package a;
+        |/*package private*/ interface I { int f(); }
+      """.stripMargin
+    val jC =
+      """package a;
+        |public interface J extends I { }
+      """.stripMargin
+    val cC =
+      """package b
+        |class C {
+        |  def f1(b: a.B) = b.f
+        |  def f2(b: a.B) = { b.t = b.t + 1 }
+        |  def f3(j: a.J) = j.f
+        |}
+      """.stripMargin
+
+    checkBCode(scalaSources = List(cC), javaSources = List(aC, bC, iC, jC)) { dir =>
+      val clsIn   = dir.subdirectoryNamed("b").lookupName("C.class", directory = false).input
+      val c = loadClassNode(clsIn)
+
+      assertInvoke(getMethod(c, "f1"), "a/B", "f") // receiver needs to be B (A is not accessible in class C, package b)
+      assertInvoke(getMethod(c, "f3"), "a/J", "f") // receiver needs to be J
+    }
+  }
+
+  @Test
+  def specialInvocationReceivers(): Unit = {
+    val code =
+      """class C {
+        |  def f1(a: Array[String]) = a.clone()
+        |  def f2(a: Array[Int]) = a.hashCode()
+        |  def f3(n: Nothing) = n.hashCode()
+        |  def f4(n: Null) = n.toString()
+        |
+        |}
+      """.stripMargin
+    checkBCode(List(code)) { dir =>
+      val c = loadClassNode(dir.lookupName("C.class", directory = false).input)
+
+      assertInvoke(getMethod(c, "f1"), "[Ljava/lang/String;", "clone") // array descriptor as receiver
+      assertInvoke(getMethod(c, "f2"), "java/lang/Object", "hashCode") // object receiver
+      assertInvoke(getMethod(c, "f3"), "java/lang/Object", "hashCode")
+      assertInvoke(getMethod(c, "f4"), "java/lang/Object", "toString")
+    }
+  }
+}
+
+object invocationReceiversTestCode {
+  // if cloneType is more specific than Object (e.g., String), a bridge method is generated.
+  def definitions(cloneType: String): String =
+    s"""trait T { override def clone(): $cloneType = "hi" }
+        |trait U extends T
+        |class C1 extends U with Cloneable {
+        |  // The comments below are true when `cloneType` is Object.
+        |  // C1 gets a forwarder for clone that invokes T.clone. this is needed because JVM method
+        |  // resolution always prefers class members, so it would resolve to Object.clone, even if
+        |  // C1 is a subtype of the interface T which has an overriding default method for clone.
+        |
+        |  // invokeinterface T.clone
+        |  def f1 = (this: T).clone()
+        |
+        |  // cannot invokeinterface U.clone (NoSuchMethodError). Object.clone would work here, but
+        |  // not in the example in C2 (illegal access to protected). T.clone works in all cases and
+        |  // resolves correctly.
+        |  def f2 = (this: U).clone()
+        |
+        |  // invokevirtual C1.clone()
+        |  def f3 = (this: C1).clone()
+        |}
+        |
+        |class C2 {
+        |  def f1(t: T) = t.clone()  // invokeinterface T.clone
+        |  def f2(t: U) = t.clone()  // invokeinterface T.clone -- Object.clone would be illegal (protected, explained in C1)
+        |  def f3(t: C1) = t.clone() // invokevirtual C1.clone -- Object.clone would be illegal
+        |}
+    """.stripMargin
 }
diff --git a/compiler/test/dotty/tools/backend/jvm/InlineBytecodeTests.scala b/compiler/test/dotty/tools/backend/jvm/InlineBytecodeTests.scala
@@ -341,7 +341,7 @@ class InlineBytecodeTests extends DottyBytecodeTest {
           VarOp(ALOAD, 0),
           Invoke(INVOKEVIRTUAL, "Test", "given_Int", "()I", false),
           Invoke(INVOKESTATIC, "scala/runtime/BoxesRunTime", "boxToInteger", "(I)Ljava/lang/Integer;", false),
-          Invoke(INVOKEINTERFACE, "scala/Function1", "apply", "(Ljava/lang/Object;)Ljava/lang/Object;", true),
+          Invoke(INVOKEINTERFACE, "dotty/runtime/function/JFunction1$mcZI$sp", "apply", "(Ljava/lang/Object;)Ljava/lang/Object;", true),
           Invoke(INVOKESTATIC, "scala/runtime/BoxesRunTime", "unboxToBoolean", "(Ljava/lang/Object;)Z", false),
           Op(IRETURN)
         )
@@ -352,7 +352,7 @@ class InlineBytecodeTests extends DottyBytecodeTest {
       }
 
       assert(instructions.tail == expected,
-        "`fg was not properly inlined in `test`\n" + diffInstructions(instructions, expected))
+        "`fg was not properly inlined in `test`\n" + diffInstructions(instructions.tail, expected))
 
     }
   }

diff --git a/tests/pos-java-interop/i6546/A.java b/tests/pos-java-interop/i6546/A.java
@@ -0,0 +1,8 @@
+package pkg;
+class P {
+  public void foo() {
+  }
+}
+
+public class A extends P {
+}
diff --git a/tests/pos-java-interop/i6546/B.scala b/tests/pos-java-interop/i6546/B.scala
@@ -0,0 +1,6 @@
+object B {
+  def main(args: Array[String]): Unit = {
+    val a = new pkg.A
+    a.foo()
+  }
+}
diff --git a/tests/run/invocationReceivers1.scala b/tests/run/invocationReceivers1.scala
@@ -0,0 +1,45 @@
+trait T { override def clone(): Object = "hi" }
+trait U extends T
+class C1 extends U with Cloneable {
+  // C1 gets a forwarder for clone that invokes T.clone. this is needed because JVM method
+  // resolution always prefers class members, so it would resolve to Object.clone, even if
+  // C1 is a subtype of the interface T which has an overriding default method for clone.
+
+  // invokeinterface T.clone
+  def f1 = (this: T).clone()
+
+  // cannot invokeinterface U.clone (NoSuchMethodError). Object.clone would work here, but
+  // not in the example in C2 (illegal access to protected). T.clone works in all cases and
+  // resolves correctly.
+  def f2 = (this: U).clone()
+
+  // invokevirtual C1.clone()
+  def f3 = (this: C1).clone()
+}
+
+class C2 {
+  def f1(t: T) = t.clone()  // invokeinterface T.clone
+  def f2(t: U) = t.clone()  // invokeinterface T.clone -- Object.clone would be illegal (protected, explained in C1)
+  def f3(t: C1) = t.clone() // invokevirtual C1.clone -- Object.clone would be illegal
+}
+
+object Test {
+  def main(arg: Array[String]): Unit = {
+    val r = new StringBuffer()
+    val c1 = new C1
+    r.append(c1.f1)
+    r.append(c1.f2)
+    r.append(c1.f3)
+    val t = new T { }
+    val u = new U { }
+    val c2 = new C2
+    r.append(c2.f1(t))
+    r.append(c2.f1(u))
+    r.append(c2.f1(c1))
+    r.append(c2.f2(u))
+    r.append(c2.f2(c1))
+    r.append(c2.f3(c1))
+    r.toString
+  }
+}
+