Skip to content

Commit 765eb29

Browse files
lrytzlrytz
committed
Clean up code gen for method invocations
The code was patched many times in the history and became a bit scattered. 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 this patch we extend the fix to field writes, and clean up the code. This patch basically reverts 6eb55d4, the fix for SI-4560, which was rather a workaround than a fix. The underlying problem was that in some cases, in a method invocation `foo.bar()`, the method `bar` was not actually a member of `foo.tpe`, causing a NoSuchMethodErrors. The issue was related to trait implementation classes. The idea of the fix was to check, at code-gen time, `foo.tpe.member("bar")`, and if that returns `NoSymbol`, use `barSym.owner`. With the new trait encoding the underlying problem seems to be fixed - all tests still pass (run/t4560.scala and run/t4560b.scala).
1 parent d176f10 commit 765eb29

File tree

7 files changed

+324
-218
lines changed

7 files changed

+324
-218
lines changed

‎src/compiler/scala/tools/nsc/backend/jvm/BCodeBodyBuilder.scala‎

Lines changed: 162 additions & 195 deletions
Large diffs are not rendered by default.

‎src/compiler/scala/tools/nsc/backend/jvm/BCodeSkelBuilder.scala‎

Lines changed: 13 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ abstract class BCodeSkelBuilder extends BCodeHelpers {
5959

6060
// current class
6161
var cnode: asm.tree.ClassNode = null
62-
var thisName: String = null // the internal name of the class being emitted
62+
var thisBType: ClassBType = null
6363

6464
var claszSymbol: Symbol = null
6565
var isCZParcelable = false
@@ -91,9 +91,7 @@ abstract class BCodeSkelBuilder extends BCodeHelpers {
9191
isCZParcelable = isAndroidParcelableClass(claszSymbol)
9292
isCZStaticModule = isStaticModuleClass(claszSymbol)
9393
isCZRemote = isRemote(claszSymbol)
94-
thisName = internalName(claszSymbol)
95-
96-
val classBType = classBTypeFromSymbol(claszSymbol)
94+
thisBType = classBTypeFromSymbol(claszSymbol)
9795

9896
cnode = new asm.tree.ClassNode()
9997

@@ -123,7 +121,7 @@ abstract class BCodeSkelBuilder extends BCodeHelpers {
123121
if (shouldAddLambdaDeserialize)
124122
backendUtils.addLambdaDeserialize(cnode)
125123

126-
cnode.visitAttribute(classBType.inlineInfoAttribute.get)
124+
cnode.visitAttribute(thisBType.inlineInfoAttribute.get)
127125

128126
if (AsmUtils.traceClassEnabled && cnode.name.contains(AsmUtils.traceClassPattern))
129127
AsmUtils.traceClass(cnode)
@@ -144,7 +142,7 @@ abstract class BCodeSkelBuilder extends BCodeHelpers {
144142

145143
val thisSignature = getGenericSignature(claszSymbol, claszSymbol.owner)
146144
cnode.visit(classfileVersion, flags,
147-
thisName, thisSignature,
145+
thisBType.internalName, thisSignature,
148146
superClass, interfaceNames.toArray)
149147

150148
if (emitSource) {
@@ -157,7 +155,7 @@ abstract class BCodeSkelBuilder extends BCodeHelpers {
157155
case _ => ()
158156
}
159157

160-
val ssa = getAnnotPickle(thisName, claszSymbol)
158+
val ssa = getAnnotPickle(thisBType.internalName, claszSymbol)
161159
cnode.visitAttribute(if (ssa.isDefined) pickleMarkerLocal else pickleMarkerForeign)
162160
emitAnnotations(cnode, claszSymbol.annotations ++ ssa)
163161

@@ -178,7 +176,7 @@ abstract class BCodeSkelBuilder extends BCodeHelpers {
178176
}
179177
if (isCandidateForForwarders) {
180178
log(s"Adding static forwarders from '$claszSymbol' to implementations in '$lmoc'")
181-
addForwarders(isRemote(claszSymbol), cnode, thisName, lmoc.moduleClass)
179+
addForwarders(isRemote(claszSymbol), cnode, thisBType.internalName, lmoc.moduleClass)
182180
}
183181
}
184182
}
@@ -196,7 +194,7 @@ abstract class BCodeSkelBuilder extends BCodeHelpers {
196194
val fv =
197195
cnode.visitField(GenBCode.PublicStaticFinal, // TODO confirm whether we really don't want ACC_SYNTHETIC nor ACC_DEPRECATED
198196
strMODULE_INSTANCE_FIELD,
199-
"L" + thisName + ";",
197+
thisBType.descriptor,
200198
null, // no java-generic-signature
201199
null // no initial value
202200
)
@@ -220,11 +218,11 @@ abstract class BCodeSkelBuilder extends BCodeHelpers {
220218

221219
/* "legacy static initialization" */
222220
if (isCZStaticModule) {
223-
clinit.visitTypeInsn(asm.Opcodes.NEW, thisName)
221+
clinit.visitTypeInsn(asm.Opcodes.NEW, thisBType.internalName)
224222
clinit.visitMethodInsn(asm.Opcodes.INVOKESPECIAL,
225-
thisName, INSTANCE_CONSTRUCTOR_NAME, "()V", false)
223+
thisBType.internalName, INSTANCE_CONSTRUCTOR_NAME, "()V", false)
226224
}
227-
if (isCZParcelable) { legacyAddCreatorCode(clinit, cnode, thisName) }
225+
if (isCZParcelable) { legacyAddCreatorCode(clinit, cnode, thisBType.internalName) }
228226
clinit.visitInsn(asm.Opcodes.RETURN)
229227

230228
clinit.visitMaxs(0, 0) // just to follow protocol, dummy arguments
@@ -604,7 +602,7 @@ abstract class BCodeSkelBuilder extends BCodeHelpers {
604602
if (!hasStaticBitSet) {
605603
mnode.visitLocalVariable(
606604
"this",
607-
"L" + thisName + ";",
605+
thisBType.descriptor,
608606
null,
609607
veryFirstProgramPoint,
610608
onePastLastProgramPoint,
@@ -686,8 +684,8 @@ abstract class BCodeSkelBuilder extends BCodeHelpers {
686684
val jname = callee.javaSimpleName.toString
687685
val jtype = methodBTypeFromSymbol(callee).descriptor
688686
insnParcA = new asm.tree.MethodInsnNode(asm.Opcodes.INVOKESTATIC, jowner, jname, jtype, false)
689-
// PUTSTATIC `thisName`.CREATOR;
690-
insnParcB = new asm.tree.FieldInsnNode(asm.Opcodes.PUTSTATIC, thisName, "CREATOR", andrFieldDescr)
687+
// PUTSTATIC `thisBType.internalName`.CREATOR;
688+
insnParcB = new asm.tree.FieldInsnNode(asm.Opcodes.PUTSTATIC, thisBType.internalName, "CREATOR", andrFieldDescr)
691689
}
692690

693691
// insert a few instructions for initialization before each return instruction

‎src/compiler/scala/tools/nsc/backend/jvm/GenBCode.scala‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -187,7 +187,7 @@ abstract class GenBCode extends BCodeSyncAndTry {
187187
// -------------- "plain" class --------------
188188
val pcb = new PlainClassBuilder(cunit)
189189
pcb.genPlainClass(cd)
190-
val outF = if (needsOutFolder) getOutFolder(claszSymbol, pcb.thisName, cunit) else null
190+
val outF = if (needsOutFolder) getOutFolder(claszSymbol, pcb.thisBType.internalName, cunit) else null
191191
val plainC = pcb.cnode
192192

193193
// -------------- bean info class, if needed --------------

‎test/files/run/t2106.check‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
t2106.scala:7: warning: A::foo()Ljava/lang/Object; is annotated @inline but could not be inlined:
2-
The callee A::foo()Ljava/lang/Object; contains the instruction INVOKEVIRTUAL java/lang/Object.clone ()Ljava/lang/Object;
2+
The callee A::foo()Ljava/lang/Object; contains the instruction INVOKEVIRTUAL A.clone ()Ljava/lang/Object;
33
that would cause an IllegalAccessError when inlined into class Test$.
44
def main(args: Array[String]): Unit = x.foo
55
^

‎test/junit/scala/issues/BytecodeTest.scala‎

Lines changed: 134 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import scala.tools.nsc.backend.jvm.CodeGenTools._
1010
import org.junit.Assert._
1111

1212
import scala.collection.JavaConverters._
13+
import scala.tools.asm.Opcodes
1314
import scala.tools.asm.tree.ClassNode
1415
import scala.tools.partest.ASMConverters._
1516
import scala.tools.testing.ClearAfterClass
@@ -341,4 +342,137 @@ class BytecodeTest extends ClearAfterClass {
341342

342343
checkForwarder('K12, "T2")
343344
}
345+
346+
@Test
347+
def invocationReceivers(): Unit = {
348+
val List(c1, c2, t, u) = compileClasses(compiler)(invocationReceiversTestCode.definitions("Object"))
349+
// mixin forwarder in C1
350+
assertSameCode(getSingleMethod(c1, "clone"), List(VarOp(ALOAD, 0), Invoke(INVOKESPECIAL, "T", "clone", "()Ljava/lang/Object;", false), Op(ARETURN)))
351+
assertInvoke(getSingleMethod(c1, "f1"), "T", "clone")
352+
assertInvoke(getSingleMethod(c1, "f2"), "T", "clone")
353+
assertInvoke(getSingleMethod(c1, "f3"), "C1", "clone")
354+
assertInvoke(getSingleMethod(c2, "f1"), "T", "clone")
355+
assertInvoke(getSingleMethod(c2, "f2"), "T", "clone")
356+
assertInvoke(getSingleMethod(c2, "f3"), "C1", "clone")
357+
358+
val List(c1b, c2b, tb, ub) = compileClasses(compiler)(invocationReceiversTestCode.definitions("String"))
359+
def ms(c: ClassNode, n: String) = c.methods.asScala.toList.filter(_.name == n)
360+
assert(ms(tb, "clone").length == 1)
361+
assert(ms(ub, "clone").isEmpty)
362+
val List(c1Clone) = ms(c1b, "clone")
363+
assertEquals(c1Clone.desc, "()Ljava/lang/Object;")
364+
assert((c1Clone.access | Opcodes.ACC_BRIDGE) != 0)
365+
assertSameCode(convertMethod(c1Clone), List(VarOp(ALOAD, 0), Invoke(INVOKEVIRTUAL, "C1", "clone", "()Ljava/lang/String;", false), Op(ARETURN)))
366+
367+
def iv(m: Method) = getSingleMethod(c1b, "f1").instructions.collect({case i: Invoke => i})
368+
assertSameCode(iv(getSingleMethod(c1b, "f1")), List(Invoke(INVOKEINTERFACE, "T", "clone", "()Ljava/lang/String;", true)))
369+
assertSameCode(iv(getSingleMethod(c1b, "f2")), List(Invoke(INVOKEINTERFACE, "T", "clone", "()Ljava/lang/String;", true)))
370+
// invokeinterface T.clone in C1 is OK here because it is not an override of Object.clone (different siganture)
371+
assertSameCode(iv(getSingleMethod(c1b, "f3")), List(Invoke(INVOKEINTERFACE, "T", "clone", "()Ljava/lang/String;", true)))
372+
}
373+
374+
@Test
375+
def invocationReceiversProtected(): Unit = {
376+
// http://lrytz.github.io/scala-aladdin-bugtracker/displayItem.do%3Fid=455.html / 9954eaf
377+
// also https://issues.scala-lang.org/browse/SI-1430 / 0bea2ab (same but with interfaces)
378+
val aC =
379+
"""package a;
380+
|/*package private*/ abstract class A {
381+
| public int f() { return 1; }
382+
| public int t;
383+
|}
384+
""".stripMargin
385+
val bC =
386+
"""package a;
387+
|public class B extends A { }
388+
""".stripMargin
389+
val iC =
390+
"""package a;
391+
|/*package private*/ interface I { int f(); }
392+
""".stripMargin
393+
val jC =
394+
"""package a;
395+
|public interface J extends I { }
396+
""".stripMargin
397+
val cC =
398+
"""package b
399+
|class C {
400+
| def f1(b: a.B) = b.f
401+
| def f2(b: a.B) = { b.t = b.t + 1 }
402+
| def f3(j: a.J) = j.f
403+
|}
404+
""".stripMargin
405+
val List(c) = compileClasses(compiler)(cC, javaCode = List((aC, "A.java"), (bC, "B.java"), (iC, "I.java"), (jC, "J.java")))
406+
assertInvoke(getSingleMethod(c, "f1"), "a/B", "f") // receiver needs to be B (A is not accessible in class C, package b)
407+
println(getSingleMethod(c, "f2").instructions.stringLines)
408+
assertInvoke(getSingleMethod(c, "f3"), "a/J", "f") // receiver needs to be J
409+
}
410+
411+
@Test
412+
def specialInvocationReceivers(): Unit = {
413+
val code =
414+
"""class C {
415+
| def f1(a: Array[String]) = a.clone()
416+
| def f2(a: Array[Int]) = a.hashCode()
417+
| def f3(n: Nothing) = n.hashCode()
418+
| def f4(n: Null) = n.toString()
419+
|
420+
|}
421+
""".stripMargin
422+
val List(c) = compileClasses(compiler)(code)
423+
assertInvoke(getSingleMethod(c, "f1"), "[Ljava/lang/String;", "clone") // array descriptor as receiver
424+
assertInvoke(getSingleMethod(c, "f2"), "java/lang/Object", "hashCode") // object receiver
425+
assertInvoke(getSingleMethod(c, "f3"), "java/lang/Object", "hashCode")
426+
assertInvoke(getSingleMethod(c, "f4"), "java/lang/Object", "toString")
427+
}
428+
}
429+
430+
object invocationReceiversTestCode {
431+
// if cloneType is more specific than Object (e.g., String), a bridge method is generated.
432+
def definitions(cloneType: String) =
433+
s"""trait T { override def clone(): $cloneType = "hi" }
434+
|trait U extends T
435+
|class C1 extends U with Cloneable {
436+
| // The comments below are true when $cloneType is Object.
437+
| // C1 gets a forwarder for clone that invokes T.clone. this is needed because JVM method
438+
| // resolution always prefers class members, so it would resolve to Object.clone, even if
439+
| // C1 is a subtype of the interface T which has an overriding default method for clone.
440+
|
441+
| // invokeinterface T.clone
442+
| def f1 = (this: T).clone()
443+
|
444+
| // cannot invokeinterface U.clone (NoSuchMethodError). Object.clone would work here, but
445+
| // not in the example in C2 (illegal access to protected). T.clone works in all cases and
446+
| // resolves correctly.
447+
| def f2 = (this: U).clone()
448+
|
449+
| // invokevirtual C1.clone()
450+
| def f3 = (this: C1).clone()
451+
|}
452+
|
453+
|class C2 {
454+
| def f1(t: T) = t.clone() // invokeinterface T.clone
455+
| def f2(t: U) = t.clone() // invokeinterface T.clone -- Object.clone would be illegal (protected, explained in C1)
456+
| def f3(t: C1) = t.clone() // invokevirtual C1.clone -- Object.clone would be illegal
457+
|}
458+
""".stripMargin
459+
460+
val runCode =
461+
"""
462+
|val r = new StringBuffer()
463+
|val c1 = new C1
464+
|r.append(c1.f1)
465+
|r.append(c1.f2)
466+
|r.append(c1.f3)
467+
|val t = new T { }
468+
|val u = new U { }
469+
|val c2 = new C2
470+
|r.append(c2.f1(t))
471+
|r.append(c2.f1(u))
472+
|r.append(c2.f1(c1))
473+
|r.append(c2.f2(u))
474+
|r.append(c2.f2(c1))
475+
|r.append(c2.f3(c1))
476+
|r.toString
477+
""".stripMargin
344478
}

‎test/junit/scala/issues/RunTest.scala‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -140,4 +140,11 @@ class RunTest extends ClearAfterClass {
140140
val i = Integer.TYPE
141141
assertEquals(run[List[Class[_]]](code), List(i, i))
142142
}
143+
144+
@Test
145+
def invocationReceivers(): Unit = {
146+
import invocationReceiversTestCode._
147+
assertEquals(run[String](definitions("Object") + runCode), "hi" * 9)
148+
assertEquals(run[String](definitions("String") + runCode), "hi" * 9) // bridge method for clone generated
149+
}
143150
}

‎test/junit/scala/tools/nsc/backend/jvm/analysis/NullnessAnalyzerTest.scala‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -68,12 +68,12 @@ class NullnessAnalyzerTest extends ClearAfterClass {
6868
// So in the frame for `ALOAD 0`, the stack is still empty.
6969

7070
val res =
71-
""" L0: 0: NotNull
72-
| LINENUMBER 1 L0: 0: NotNull
73-
| ALOAD 0: 0: NotNull
74-
|INVOKEVIRTUAL java/lang/Object.toString ()Ljava/lang/String;: 0: NotNull, 1: NotNull
75-
| ARETURN: 0: NotNull, 1: Unknown1
76-
| L0: null""".stripMargin
71+
""" L0: 0: NotNull
72+
| LINENUMBER 1 L0: 0: NotNull
73+
| ALOAD 0: 0: NotNull
74+
|INVOKEVIRTUAL C.toString ()Ljava/lang/String;: 0: NotNull, 1: NotNull
75+
| ARETURN: 0: NotNull, 1: Unknown1
76+
| L0: null""".stripMargin
7777
// println(showAllNullnessFrames(newNullnessAnalyzer(m), m))
7878
assertEquals(showAllNullnessFrames(newNullnessAnalyzer(m), m), res)
7979
}

0 commit comments

Comments
 (0)