Avoid vertical class merging if virtual method is hit by invoke-direct
It is valid to have an invoke-direct instruction in a default interface method that targets another default method in the same interface (see InterfaceMethodDesugaringTests.testInvokeSpecialToDefaultMethod). However, in a class, that would lead to a verification error. Therefore, this CL disallows merging such interfaces into their subtypes.
Note that, during vertical class merging, we create a private method for each virtual method that is moved to the subtype. Therefore, it would be possible to redirect the invoke-direct instructions that target a virtual method to invoke-direct instructions that target the newly created private methods. However, this would require maintaining potentially many mappings in the graph lense that would likely never be used in practice.
Change-Id: I24ec192c25d4357fa8582edffc6db80ff628e7e0
diff --git a/src/main/java/com/android/tools/r8/shaking/Enqueuer.java b/src/main/java/com/android/tools/r8/shaking/Enqueuer.java
index 061b9bf..7df9d9c 100644
--- a/src/main/java/com/android/tools/r8/shaking/Enqueuer.java
+++ b/src/main/java/com/android/tools/r8/shaking/Enqueuer.java
@@ -153,6 +153,8 @@
* its implementation may be removed and it may be marked abstract.
*/
private final SetWithReason<DexEncodedMethod> targetedMethods = new SetWithReason<>();
+ /** Set of virtual methods that are the immediate target of an invoke-direct. */
+ private final Set<DexMethod> virtualMethodsTargetedByInvokeDirect = Sets.newIdentityHashSet();
/**
* Set of methods that belong to live classes and can be reached by invokes. These need to be
* kept.
@@ -601,6 +603,13 @@
DexEncodedMethod target = appInfo.dispatchDirectInvoke(resolutionResult);
if (target != null) {
markDirectStaticOrConstructorMethodAsLive(target, reason);
+
+ // It is valid to have an invoke-direct instruction in a default interface method that
+ // targets another default method in the same interface (see testInvokeSpecialToDefault-
+ // Method). In a class, that would lead to a verification error.
+ if (target.isVirtualMethod()) {
+ virtualMethodsTargetedByInvokeDirect.add(target.method);
+ }
}
}
@@ -1479,6 +1488,8 @@
* removed.
*/
final SortedSet<DexMethod> targetedMethods;
+ /** Set of virtual methods that are the immediate target of an invoke-direct. */
+ final SortedSet<DexMethod> virtualMethodsTargetedByInvokeDirect;
/**
* Set of methods that belong to live classes and can be reached by invokes. These need to be
* kept.
@@ -1596,6 +1607,9 @@
ImmutableSortedSet.copyOf(
PresortedComparable<DexType>::slowCompareTo, enqueuer.instantiatedLambdas.getItems());
this.targetedMethods = toSortedDescriptorSet(enqueuer.targetedMethods.getItems());
+ this.virtualMethodsTargetedByInvokeDirect =
+ ImmutableSortedSet.copyOf(
+ DexMethod::slowCompareTo, enqueuer.virtualMethodsTargetedByInvokeDirect);
this.liveMethods = toSortedDescriptorSet(enqueuer.liveMethods.getItems());
this.liveFields = toSortedDescriptorSet(enqueuer.liveFields.getItems());
this.instanceFieldReads = enqueuer.collectInstanceFieldsRead();
@@ -1636,6 +1650,7 @@
this.instantiatedTypes = previous.instantiatedTypes;
this.instantiatedLambdas = previous.instantiatedLambdas;
this.targetedMethods = previous.targetedMethods;
+ this.virtualMethodsTargetedByInvokeDirect = previous.virtualMethodsTargetedByInvokeDirect;
this.liveMethods = previous.liveMethods;
this.liveFields = previous.liveFields;
this.instanceFieldReads = previous.instanceFieldReads;
@@ -1675,6 +1690,8 @@
this.instantiatedTypes = rewriteItems(previous.instantiatedTypes, lense::lookupType);
this.instantiatedLambdas = rewriteItems(previous.instantiatedLambdas, lense::lookupType);
this.targetedMethods = rewriteMethodsConservatively(previous.targetedMethods, lense);
+ this.virtualMethodsTargetedByInvokeDirect =
+ rewriteMethodsConservatively(previous.virtualMethodsTargetedByInvokeDirect, lense);
this.liveMethods = rewriteMethodsConservatively(previous.liveMethods, lense);
this.liveFields = rewriteItems(previous.liveFields, lense::lookupField);
this.instanceFieldReads =
@@ -1726,6 +1743,7 @@
this.instantiatedTypes = previous.instantiatedTypes;
this.instantiatedLambdas = previous.instantiatedLambdas;
this.targetedMethods = previous.targetedMethods;
+ this.virtualMethodsTargetedByInvokeDirect = previous.virtualMethodsTargetedByInvokeDirect;
this.liveMethods = previous.liveMethods;
this.liveFields = previous.liveFields;
this.instanceFieldReads = previous.instanceFieldReads;
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 0840d91..38473f3 100644
--- a/src/main/java/com/android/tools/r8/shaking/VerticalClassMerger.java
+++ b/src/main/java/com/android/tools/r8/shaking/VerticalClassMerger.java
@@ -81,6 +81,8 @@
RESOLUTION_FOR_FIELDS_MAY_CHANGE,
RESOLUTION_FOR_METHODS_MAY_CHANGE,
STATIC_INITIALIZERS,
+ UNHANDLED_INVOKE_DIRECT,
+ UNHANDLED_INVOKE_SUPER,
UNSAFE_INLINING,
UNSUPPORTED_ATTRIBUTES;
@@ -121,6 +123,12 @@
case STATIC_INITIALIZERS:
message = "merging of static initializers are not supported";
break;
+ case UNHANDLED_INVOKE_DIRECT:
+ message = "a virtual method is targeted by an invoke-direct instruction";
+ break;
+ case UNHANDLED_INVOKE_SUPER:
+ message = "it may change the semantics of an invoke-super instruction";
+ break;
case UNSAFE_INLINING:
message = "force-inlining might fail";
break;
@@ -217,10 +225,15 @@
// }
// }
for (DexMethod signature : appInfo.brokenSuperInvokes) {
- DexClass targetClass = appInfo.definitionFor(signature.holder);
- if (targetClass != null && targetClass.isProgramClass()) {
- pinnedTypes.add(signature.holder);
- }
+ markTypeAsPinned(signature.holder, AbortReason.UNHANDLED_INVOKE_SUPER);
+ }
+
+ // It is valid to have an invoke-direct instruction in a default interface method that targets
+ // another default method in the same interface (see InterfaceMethodDesugaringTests.testInvoke-
+ // SpecialToDefaultMethod). However, in a class, that would lead to a verification error.
+ // Therefore, we disallow merging such interfaces into their subtypes.
+ for (DexMethod signature : appInfo.virtualMethodsTargetedByInvokeDirect) {
+ markTypeAsPinned(signature.holder, AbortReason.UNHANDLED_INVOKE_DIRECT);
}
}
