Rewrite super calls to previously merged classes in vertical class merger
Change-Id: Ieeae36c7e13a3a2fe013a7003334173c2bd13c8d
diff --git a/src/main/java/com/android/tools/r8/ir/conversion/IRConverter.java b/src/main/java/com/android/tools/r8/ir/conversion/IRConverter.java
index 1134686..f43106b 100644
--- a/src/main/java/com/android/tools/r8/ir/conversion/IRConverter.java
+++ b/src/main/java/com/android/tools/r8/ir/conversion/IRConverter.java
@@ -630,7 +630,7 @@
printMethod(code, "Initial IR (SSA)");
if (options.canHaveArtStringNewInitBug()) {
- codeRewriter.ensureDirectStringNewToInit(code);
+ CodeRewriter.ensureDirectStringNewToInit(code);
}
if (options.debug) {
diff --git a/src/main/java/com/android/tools/r8/shaking/VerticalClassMerger.java b/src/main/java/com/android/tools/r8/shaking/VerticalClassMerger.java
index f0519a1..d06c73a 100644
--- a/src/main/java/com/android/tools/r8/shaking/VerticalClassMerger.java
+++ b/src/main/java/com/android/tools/r8/shaking/VerticalClassMerger.java
@@ -71,10 +71,18 @@
private final DexApplication application;
private final AppInfoWithLiveness appInfo;
private final GraphLense graphLense;
- private final Map<DexType, DexType> mergedClasses = new HashMap<>();
private final Timing timing;
private Collection<DexMethod> invokes;
+ // Map from source class to target class.
+ private final Map<DexType, DexType> mergedClasses = new HashMap<>();
+
+ // Map from target class to the super classes that have been merged into the target class.
+ private final Map<DexType, Set<DexType>> mergedClassesInverse = new HashMap<>();
+
+ // The resulting graph lense that should be used after class merging.
+ private final VerticalClassMergerGraphLense.Builder renamedMembersLense;
+
public VerticalClassMerger(
DexApplication application,
AppInfoWithLiveness appInfo,
@@ -83,6 +91,7 @@
this.application = application;
this.appInfo = appInfo;
this.graphLense = graphLense;
+ this.renamedMembersLense = VerticalClassMergerGraphLense.builder(appInfo);
this.timing = timing;
}
@@ -319,10 +328,6 @@
// Types that are pinned (in addition to those where appInfo.isPinned returns true).
Set<DexType> pinnedTypes = getPinnedTypes(classes);
- // The resulting graph lense that should be used after class merging.
- VerticalClassMergerGraphLense.Builder renamedMembersLense =
- VerticalClassMergerGraphLense.builder(appInfo);
-
Iterator<DexProgramClass> classIterator = classes.iterator();
// Visit the program classes in a top-down order according to the class hierarchy.
@@ -533,7 +538,7 @@
continue;
}
} else {
- // The method is shadowed. If it is abstract, we can simply move it to the subclass.
+ // The method is not shadowed. If it is abstract, we can simply move it to the subclass.
// Non-abstract methods are handled below (they cannot simply be moved to the subclass as
// a virtual method, because they might be the target of an invoke-super instruction).
if (virtualMethod.accessFlags.isAbstract()) {
@@ -638,6 +643,8 @@
source.interfaces = DexTypeList.empty();
// Step 4: Record merging.
mergedClasses.put(source.type, target.type);
+ mergedClassesInverse.computeIfAbsent(target.type, key -> new HashSet<>()).add(source.type);
+ assert !abortMerge;
return true;
}
@@ -658,13 +665,38 @@
DexMethod signatureInHolder =
application.dexItemFactory.createMethod(holder.type, oldTarget.proto, oldTarget.name);
// Only rewrite the invoke-super call if it does not lead to a NoSuchMethodError.
- if (resolutionSucceeds(signatureInHolder)) {
+ boolean resolutionSucceeds =
+ holder.lookupVirtualMethod(signatureInHolder) != null
+ || appInfo.lookupSuperTarget(signatureInHolder, holder.type) != null;
+ if (resolutionSucceeds) {
deferredRenamings.mapVirtualMethodToDirectInType(
signatureInHolder, newTarget, target.type);
- holder = holder.superType != null ? appInfo.definitionFor(holder.superType) : null;
} else {
break;
}
+
+ // Consider that A gets merged into B and B's subclass C gets merged into D. Instructions
+ // on the form "invoke-super {B,C,D}.m()" in D are changed into "invoke-direct D.m$C()" by
+ // the code above. However, instructions on the form "invoke-super A.m()" should also be
+ // changed into "invoke-direct D.m$C()". This is achieved by also considering the classes
+ // that have been merged into [holder].
+ Set<DexType> mergedTypes = mergedClassesInverse.get(holder.type);
+ if (mergedTypes != null) {
+ for (DexType type : mergedTypes) {
+ DexMethod signatureInType =
+ application.dexItemFactory.createMethod(type, oldTarget.proto, oldTarget.name);
+ // Resolution would have succeeded if the method used to be in [type], or if one of
+ // its super classes declared the method.
+ boolean resolutionSucceededBeforeMerge =
+ renamedMembersLense.hasMappingForSignatureInContext(holder.type, signatureInType)
+ || appInfo.lookupSuperTarget(signatureInHolder, holder.type) != null;
+ if (resolutionSucceededBeforeMerge) {
+ deferredRenamings.mapVirtualMethodToDirectInType(
+ signatureInType, newTarget, target.type);
+ }
+ }
+ }
+ holder = holder.superType != null ? appInfo.definitionFor(holder.superType) : null;
}
}
@@ -688,13 +720,6 @@
deferredRenamings.markMethodAsMerged(method);
}
- private boolean resolutionSucceeds(DexMethod targetMethod) {
- DexClass enclosingClass = appInfo.definitionFor(targetMethod.holder);
- return enclosingClass != null
- && (enclosingClass.lookupVirtualMethod(targetMethod) != null
- || appInfo.lookupSuperTarget(targetMethod, enclosingClass.type) != null);
- }
-
private DexEncodedMethod buildBridgeMethod(
DexEncodedMethod signature, DexMethod invocationTarget) {
DexType holder = target.type;
diff --git a/src/main/java/com/android/tools/r8/shaking/VerticalClassMergerGraphLense.java b/src/main/java/com/android/tools/r8/shaking/VerticalClassMergerGraphLense.java
index 99fb63a..4cfe8db 100644
--- a/src/main/java/com/android/tools/r8/shaking/VerticalClassMergerGraphLense.java
+++ b/src/main/java/com/android/tools/r8/shaking/VerticalClassMergerGraphLense.java
@@ -162,6 +162,15 @@
previousLense);
}
+ public boolean hasMappingForSignatureInContext(DexType context, DexMethod signature) {
+ Map<DexMethod, DexMethod> virtualToDirectMethodMap =
+ contextualVirtualToDirectMethodMaps.get(context);
+ if (virtualToDirectMethodMap != null) {
+ return virtualToDirectMethodMap.containsKey(signature);
+ }
+ return false;
+ }
+
public void markMethodAsMerged(DexMethod method) {
mergedMethodsBuilder.add(method);
}
diff --git a/src/test/java/com/android/tools/r8/classmerging/ClassMergingTest.java b/src/test/java/com/android/tools/r8/classmerging/ClassMergingTest.java
index f141d36..40299cc 100644
--- a/src/test/java/com/android/tools/r8/classmerging/ClassMergingTest.java
+++ b/src/test/java/com/android/tools/r8/classmerging/ClassMergingTest.java
@@ -3,6 +3,7 @@
// BSD-style license that can be found in the LICENSE file.
package com.android.tools.r8.classmerging;
+import static com.android.tools.r8.smali.SmaliBuilder.buildCode;
import static com.android.tools.r8.utils.DexInspectorMatchers.isPresent;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
@@ -178,14 +179,12 @@
"classmerging.SubClassThatReferencesSuperMethod",
"classmerging.SuperClassWithReferencedMethod");
smaliBuilder.addInitializer(
- ImmutableList.of(),
0,
"invoke-direct {p0}, Lclassmerging/SuperClassWithReferencedMethod;-><init>()V",
"return-void");
smaliBuilder.addInstanceMethod(
"java.lang.String",
"referencedMethod",
- ImmutableList.of(),
2,
"sget-object v0, Ljava/lang/System;->out:Ljava/io/PrintStream;",
"const-string v1, \"In referencedMethod on SubClassThatReferencesSuperMethod\"",
@@ -209,6 +208,140 @@
getProguardConfig(EXAMPLE_KEEP, "-keep class *"));
}
+ // The following test checks that our rewriting of invoke-super instructions in a class F works
+ // if a super type of F has previously been merged into its sub class.
+ //
+ // class A {} <- A is kept to preserve the error
+ // class B extends A { in F.invokeMethodOnA().
+ // public void m() {}
+ // }
+ // class C extends B { <- C will be merged into D.
+ // @Override
+ // public void m() { "invoke-super B.m()" }
+ // }
+ // class D extends C {
+ // @Override
+ // public void m() { "invoke-super C.m()" }
+ // }
+ // class E extends D { <- E will be merged into F.
+ // @Override
+ // public void m() { "invoke-super D.m()" }
+ // }
+ // class F extends E {
+ // @Override
+ // public void m() { throw new Exception() } <- Should be dead code.
+ //
+ // public void invokeMethodOnA() { "invoke-super A.m()" } <- Should yield NoSuchMethodError.
+ // public void invokeMethodOnB() { "invoke-super B.m()" }
+ // public void invokeMethodOnC() { "invoke-super C.m()" }
+ // public void invokeMethodOnD() { "invoke-super D.m()" }
+ // public void invokeMethodOnE() { "invoke-super E.m()" }
+ // public void invokeMethodOnF() { "invoke-super F.m()" }
+ // }
+ @Test
+ public void testSuperCallToMergedClassIsRewritten() throws Exception {
+ String main = "classmerging.SuperCallToMergedClassIsRewrittenTest";
+ Set<String> preservedClassNames =
+ ImmutableSet.of(
+ "classmerging.SuperCallToMergedClassIsRewrittenTest",
+ "classmerging.A",
+ "classmerging.B",
+ "classmerging.D",
+ "classmerging.F");
+
+ SmaliBuilder smaliBuilder = new SmaliBuilder();
+
+ smaliBuilder.addClass(main);
+ smaliBuilder.addMainMethod(
+ 2,
+ // Instantiate B so that it is not merged into C.
+ "new-instance v1, Lclassmerging/B;",
+ "invoke-direct {v1}, Lclassmerging/B;-><init>()V",
+ "invoke-virtual {v1}, Lclassmerging/B;->m()V",
+ // Instantiate D so that it is not merged into E.
+ "new-instance v1, Lclassmerging/D;",
+ "invoke-direct {v1}, Lclassmerging/D;-><init>()V",
+ "invoke-virtual {v1}, Lclassmerging/D;->m()V",
+ // Start the actual testing.
+ "new-instance v1, Lclassmerging/F;",
+ "invoke-direct {v1}, Lclassmerging/F;-><init>()V",
+ "invoke-virtual {v1}, Lclassmerging/F;->invokeMethodOnB()V",
+ "invoke-virtual {v1}, Lclassmerging/F;->invokeMethodOnC()V",
+ "invoke-virtual {v1}, Lclassmerging/F;->invokeMethodOnD()V",
+ "invoke-virtual {v1}, Lclassmerging/F;->invokeMethodOnE()V",
+ "invoke-virtual {v1}, Lclassmerging/F;->invokeMethodOnF()V",
+ // The method invokeMethodOnA() should yield a NoSuchMethodError.
+ ":try_start",
+ "invoke-virtual {v1}, Lclassmerging/F;->invokeMethodOnA()V",
+ ":try_end",
+ "return-void",
+ ".catch Ljava/lang/NoSuchMethodError; {:try_start .. :try_end} :catch",
+ ":catch",
+ smaliCodeForPrinting("NoSuchMethodError", 0, 1),
+ "return-void");
+
+ // Class A deliberately has no method m. We need to make sure that the "invoke-super A.m()"
+ // instruction in class F is not rewritten into something that does not throw.
+ smaliBuilder.addClass("classmerging.A");
+
+ // Class B declares a virtual method m() that prints "In B.m()".
+ smaliBuilder.addClass("classmerging.B", "classmerging.A");
+ smaliBuilder.addInstanceMethod(
+ "void", "m", 2, smaliCodeForPrinting("In B.m()", 0, 1), "return-void");
+
+ // Class C, D, and E declare a virtual method m() that prints "In C.m()", "In D.m()", and
+ // "In E.m()", respectively.
+ String[][] pairs =
+ new String[][] {new String[] {"C", "B"}, new String[] {"D", "C"}, new String[] {"E", "D"}};
+ for (String[] pair : pairs) {
+ String name = pair[0], superName = pair[1];
+ smaliBuilder.addClass("classmerging." + name, "classmerging." + superName);
+ smaliBuilder.addInstanceMethod(
+ "void",
+ "m",
+ 2,
+ smaliCodeForPrinting("In " + name + ".m()", 0, 1),
+ buildCode("invoke-super {p0}, Lclassmerging/" + superName + ";->m()V", "return-void"));
+ }
+
+ // Class F declares a virtual method m that throws an exception (it is expected to be dead).
+ smaliBuilder.addClass("classmerging.F", "classmerging.E");
+ smaliBuilder.addInstanceMethod(
+ "void",
+ "m",
+ 1,
+ "new-instance v1, Ljava/lang/Exception;",
+ "invoke-direct {v1}, Ljava/lang/Exception;-><init>()V",
+ "throw v1");
+
+ // Add methods to F with an "invoke-super X.m()" instruction for X in {A, B, C, D, E, F}.
+ for (String type : ImmutableList.of("A", "B", "C", "D", "E", "F")) {
+ String code =
+ buildCode("invoke-super {p0}, Lclassmerging/" + type + ";->m()V", "return-void");
+ smaliBuilder.addInstanceMethod("void", "invokeMethodOn" + type, 0, code);
+ }
+
+ // Build app.
+ AndroidApp.Builder builder = AndroidApp.builder();
+ builder.addDexProgramData(smaliBuilder.compile(), Origin.unknown());
+
+ // Run test.
+ runTestOnInput(
+ main,
+ builder.build(),
+ preservedClassNames::contains,
+ String.format("-keep class %s { public static void main(...); }", main));
+ }
+
+ private static String smaliCodeForPrinting(String message, int reg0, int reg1) {
+ return buildCode(
+ String.format("sget-object v%d, Ljava/lang/System;->out:Ljava/io/PrintStream;", reg0),
+ String.format("const-string v1, \"%s\"", message),
+ String.format(
+ "invoke-virtual {v%d, v%d}, Ljava/io/PrintStream;->println(Ljava/lang/String;)V",
+ reg0, reg1));
+ }
+
@Test
public void testConflictingInterfaceSignatures() throws Exception {
String main = "classmerging.ConflictingInterfaceSignaturesTest";
diff --git a/src/test/java/com/android/tools/r8/smali/SmaliBuilder.java b/src/test/java/com/android/tools/r8/smali/SmaliBuilder.java
index 9a6e778..e818801 100644
--- a/src/test/java/com/android/tools/r8/smali/SmaliBuilder.java
+++ b/src/test/java/com/android/tools/r8/smali/SmaliBuilder.java
@@ -285,6 +285,16 @@
return addMethod("public abstract", returnType, name, parameters, -1, null);
}
+ public MethodSignature addInitializer(int locals, String... instructions) {
+ return addMethod(
+ "public constructor",
+ "void",
+ "<init>",
+ ImmutableList.of(),
+ locals,
+ buildCode(instructions));
+ }
+
public MethodSignature addInitializer(
List<String> parameters, int locals, String... instructions) {
return addMethod(
@@ -292,6 +302,11 @@
}
public MethodSignature addInstanceMethod(
+ String returnType, String name, int locals, String... instructions) {
+ return addInstanceMethod(returnType, name, ImmutableList.of(), locals, buildCode(instructions));
+ }
+
+ public MethodSignature addInstanceMethod(
String returnType, String name, List<String> parameters, int locals, String... instructions) {
return addInstanceMethod(returnType, name, parameters, locals, buildCode(instructions));
}
@@ -315,7 +330,7 @@
getSource(currentClassName).add(builder.toString());
}
- private static String buildCode(String... instructions) {
+ public static String buildCode(String... instructions) {
StringBuilder builder = new StringBuilder();
for (String instruction : instructions) {
builder.append(instruction);
