Disallow rewrite of invoke-super in merged class
When we merge a class B into another class C, the methods from B are moved into C, and then we subsequently rewrite the invoke-super instructions in C that hit a method in B, such that they use an invoke-direct instruction instead. In this process, we need to avoid rewriting the invoke-super instructions that originally was in the superclass B.
Example:
class A {
public void m() {}
}
class B extends A {
// this method will be made private and renamed to C.m$B when merged into C
public void m() {
// invoke must not be rewritten to "invoke-direct C.m$B"
// (this would lead to an infinite loop)
super.m();
}
}
class C extends B {
// invoke needs to be rewritten to "invoke-direct C.m$B"
public void m() { super.m(); }
}
Change-Id: Ic8698193aeeca7b9293d0214e0556c9407540cc9diff --git a/src/main/java/com/android/tools/r8/shaking/VerticalClassMerger.java b/src/main/java/com/android/tools/r8/shaking/VerticalClassMerger.java
index 4b57209..dbd67dd 100644
--- a/src/main/java/com/android/tools/r8/shaking/VerticalClassMerger.java
+++ b/src/main/java/com/android/tools/r8/shaking/VerticalClassMerger.java
@@ -432,6 +432,10 @@
directMethods.add(renameConstructor(directMethod));
} else {
directMethods.add(directMethod);
+
+ if (!directMethod.isStaticMethod()) {
+ blockRedirectionOfSuperCalls(directMethod.method);
+ }
}
}
@@ -467,6 +471,7 @@
// Record that invoke-super instructions in the target class should be redirected to the
// newly created direct method.
redirectSuperCallsInTarget(virtualMethod.method, resultingDirectMethod.method);
+ blockRedirectionOfSuperCalls(resultingDirectMethod.method);
if (shadowedBy == null) {
// In addition to the newly added direct method, create a virtual method such that we do
@@ -490,6 +495,7 @@
(existing, method) -> {
DexEncodedMethod renamedMethod = renameMethod(method, target.type, true);
deferredRenamings.map(method.method, renamedMethod.method);
+ blockRedirectionOfSuperCalls(renamedMethod.method);
return renamedMethod;
});
Collection<DexEncodedMethod> mergedVirtualMethods =
@@ -578,6 +584,26 @@
}
}
+ private void blockRedirectionOfSuperCalls(DexMethod method) {
+ // We are merging a class B into C. The methods from B are being moved into C, and then we
+ // subsequently rewrite the invoke-super instructions in C that hit a method in B, such that
+ // they use an invoke-direct instruction instead. In this process, we need to avoid rewriting
+ // the invoke-super instructions that originally was in the superclass B.
+ //
+ // Example:
+ // class A {
+ // public void m() {}
+ // }
+ // class B extends A {
+ // public void m() { super.m(); } <- invoke must not be rewritten to invoke-direct
+ // (this would lead to an infinite loop)
+ // }
+ // class C extends B {
+ // public void m() { super.m(); } <- invoke needs to be rewritten to invoke-direct
+ // }
+ deferredRenamings.markMethodAsMerged(method);
+ }
+
private boolean resolutionSucceeds(DexMethod targetMethod) {
DexClass enclosingClass = appInfo.definitionFor(targetMethod.holder);
return enclosingClass != null
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 ea57d1d..942a857 100644
--- a/src/main/java/com/android/tools/r8/shaking/VerticalClassMergerGraphLense.java
+++ b/src/main/java/com/android/tools/r8/shaking/VerticalClassMergerGraphLense.java
@@ -45,16 +45,19 @@
private final Map<DexField, DexField> fieldMap;
private final Map<DexMethod, DexMethod> methodMap;
+ private final Set<DexMethod> mergedMethods;
private final Map<DexType, Map<DexMethod, DexMethod>> contextualVirtualToDirectMethodMaps;
public VerticalClassMergerGraphLense(
Map<DexField, DexField> fieldMap,
Map<DexMethod, DexMethod> methodMap,
+ Set<DexMethod> mergedMethods,
Map<DexType, Map<DexMethod, DexMethod>> contextualVirtualToDirectMethodMaps,
GraphLense previousLense) {
this.previousLense = previousLense;
this.fieldMap = fieldMap;
this.methodMap = methodMap;
+ this.mergedMethods = mergedMethods;
this.contextualVirtualToDirectMethodMaps = contextualVirtualToDirectMethodMaps;
}
@@ -67,7 +70,7 @@
public DexMethod lookupMethod(DexMethod method, DexEncodedMethod context, Type type) {
assert isContextFreeForMethod(method) || (context != null && type != null);
DexMethod previous = previousLense.lookupMethod(method, context, type);
- if (type == Type.SUPER) {
+ if (type == Type.SUPER && !mergedMethods.contains(context.method)) {
Map<DexMethod, DexMethod> virtualToDirectMethodMap =
contextualVirtualToDirectMethodMaps.get(context.method.holder);
if (virtualToDirectMethodMap != null) {
@@ -127,6 +130,7 @@
private final ImmutableMap.Builder<DexField, DexField> fieldMapBuilder = ImmutableMap.builder();
private final ImmutableMap.Builder<DexMethod, DexMethod> methodMapBuilder =
ImmutableMap.builder();
+ private final ImmutableSet.Builder<DexMethod> mergedMethodsBuilder = ImmutableSet.builder();
private final Map<DexType, Map<DexMethod, DexMethod>> contextualVirtualToDirectMethodMaps =
new HashMap<>();
@@ -141,7 +145,15 @@
return previousLense;
}
return new VerticalClassMergerGraphLense(
- fieldMap, methodMap, contextualVirtualToDirectMethodMaps, previousLense);
+ fieldMap,
+ methodMap,
+ mergedMethodsBuilder.build(),
+ contextualVirtualToDirectMethodMaps,
+ previousLense);
+ }
+
+ public void markMethodAsMerged(DexMethod method) {
+ mergedMethodsBuilder.add(method);
}
public void map(DexField from, DexField to) {
@@ -161,6 +173,7 @@
public void merge(VerticalClassMergerGraphLense.Builder builder) {
fieldMapBuilder.putAll(builder.fieldMapBuilder.build());
methodMapBuilder.putAll(builder.methodMapBuilder.build());
+ mergedMethodsBuilder.addAll(builder.mergedMethodsBuilder.build());
for (DexType context : builder.contextualVirtualToDirectMethodMaps.keySet()) {
Map<DexMethod, DexMethod> current = contextualVirtualToDirectMethodMaps.get(context);
Map<DexMethod, DexMethod> other = builder.contextualVirtualToDirectMethodMaps.get(context);
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 58b87c0..1e71a82 100644
--- a/src/test/java/com/android/tools/r8/classmerging/ClassMergingTest.java
+++ b/src/test/java/com/android/tools/r8/classmerging/ClassMergingTest.java
@@ -289,7 +289,9 @@
"-keep public class classmerging.pkg.SimpleInterfaceImplRetriever"));
}
- @Ignore("b/73958515")
+ // TODO(christofferqa): This test checks that the invoke-super instruction in B is not rewritten
+ // into an invoke-direct instruction after it gets merged into class C. We should add a test that
+ // checks that this works with and without inlining of the method B.m().
@Test
public void testRewritePinnedMethod() throws Exception {
String main = "classmerging.RewritePinnedMethodTest";
