Avoid merging classes where default methods collide
When checking merge groups ensure that default methods with the same
signature are declared on the same interface.
This fixes the ICCE caused by merging of lambda classes.
Bug: b/229951607
Change-Id: I769c9a4184c1620bbb4de940e019912e78dff32c
diff --git a/src/main/java/com/android/tools/r8/horizontalclassmerging/policies/NoDefaultInterfaceMethodMerging.java b/src/main/java/com/android/tools/r8/horizontalclassmerging/policies/NoDefaultInterfaceMethodMerging.java
index ac73202..043bd34 100644
--- a/src/main/java/com/android/tools/r8/horizontalclassmerging/policies/NoDefaultInterfaceMethodMerging.java
+++ b/src/main/java/com/android/tools/r8/horizontalclassmerging/policies/NoDefaultInterfaceMethodMerging.java
@@ -6,18 +6,21 @@
import com.android.tools.r8.graph.AppView;
import com.android.tools.r8.graph.DexEncodedMethod;
+import com.android.tools.r8.graph.DexMethodSignature;
import com.android.tools.r8.graph.DexProgramClass;
+import com.android.tools.r8.graph.DexType;
import com.android.tools.r8.horizontalclassmerging.HorizontalClassMerger.Mode;
import com.android.tools.r8.horizontalclassmerging.MergeGroup;
import com.android.tools.r8.horizontalclassmerging.MultiClassPolicy;
-import com.android.tools.r8.utils.InternalOptions;
-import com.android.tools.r8.utils.ListUtils;
+import com.android.tools.r8.utils.WorkList;
+import com.android.tools.r8.utils.collections.DexMethodSignatureMap;
import com.android.tools.r8.utils.collections.DexMethodSignatureSet;
import com.google.common.collect.Lists;
import java.util.Collection;
import java.util.LinkedHashMap;
import java.util.Map;
import java.util.Map.Entry;
+import java.util.function.Function;
/**
* For interfaces, we cannot introduce an instance field `int $r8$classId`. Therefore, we can't
@@ -29,42 +32,61 @@
*/
public class NoDefaultInterfaceMethodMerging extends MultiClassPolicy {
- private final Mode mode;
- private final InternalOptions options;
+ private final AppView<?> appView;
+ private final DexType MULTIPLE_SENTINEL;
public NoDefaultInterfaceMethodMerging(AppView<?> appView, Mode mode) {
- this.mode = mode;
- this.options = appView.options();
+ this.appView = appView;
+ // Use the java.lang.Object type to indicate more than one interface type, as that type
+ // itself is not an interface type.
+ this.MULTIPLE_SENTINEL = appView.dexItemFactory().objectType;
}
@Override
public Collection<MergeGroup> apply(MergeGroup group) {
- if (!group.isInterfaceGroup()) {
- return ListUtils.newLinkedList(group);
- }
-
// Split the group into smaller groups such that no default methods collide.
- Map<MergeGroup, DexMethodSignatureSet> newGroups = new LinkedHashMap<>();
+ // TODO(b/229951607): This fixes the ICCE issue for synthetic lambda classes, but a more
+ // general solution possibly extending the policy NoDefaultInterfaceMethodCollisions.
+ Map<MergeGroup, DexMethodSignatureMap<DexType>> newGroups = new LinkedHashMap<>();
for (DexProgramClass clazz : group) {
- addClassToGroup(clazz, newGroups);
+ addClassToGroup(
+ clazz,
+ newGroups,
+ group.isInterfaceGroup()
+ ? this::collectDefaultMethodsInInterfaces
+ : this::collectDefaultMethodsInImplementedInterfaces);
}
return removeTrivialGroups(Lists.newLinkedList(newGroups.keySet()));
}
private void addClassToGroup(
- DexProgramClass clazz, Map<MergeGroup, DexMethodSignatureSet> newGroups) {
- DexMethodSignatureSet classSignatures = DexMethodSignatureSet.create();
- classSignatures.addAllMethods(clazz.virtualMethods(DexEncodedMethod::isDefaultMethod));
+ DexProgramClass clazz,
+ Map<MergeGroup, DexMethodSignatureMap<DexType>> newGroups,
+ Function<DexProgramClass, DexMethodSignatureMap<DexType>> fn) {
+ DexMethodSignatureMap<DexType> classSignatures = fn.apply(clazz);
// Find a group that does not have any collisions with `clazz`.
- for (Entry<MergeGroup, DexMethodSignatureSet> entry : newGroups.entrySet()) {
+ nextGroup:
+ for (Entry<MergeGroup, DexMethodSignatureMap<DexType>> entry : newGroups.entrySet()) {
MergeGroup group = entry.getKey();
- DexMethodSignatureSet groupSignatures = entry.getValue();
- if (!groupSignatures.containsAnyOf(classSignatures)) {
- groupSignatures.addAll(classSignatures);
+ DexMethodSignatureMap<DexType> groupSignatures = entry.getValue();
+ if (!groupSignatures.containsAnyKeyOf(classSignatures.keySet())) {
+ groupSignatures.putAll(classSignatures);
group.add(clazz);
return;
+ } else {
+ DexMethodSignatureSet overlappingSignatures =
+ groupSignatures.intersectionWithKeys(classSignatures.keySet());
+ for (DexMethodSignature signature : overlappingSignatures) {
+ if ((groupSignatures.get(signature) != classSignatures.get(signature))
+ || (groupSignatures.get(signature) == MULTIPLE_SENTINEL)) {
+ continue nextGroup;
+ }
+ groupSignatures.putAll(classSignatures);
+ group.add(clazz);
+ return;
+ }
}
}
@@ -72,13 +94,59 @@
newGroups.put(new MergeGroup(clazz), classSignatures);
}
- @Override
- public String getName() {
- return "NoDefaultInterfaceMethodMerging";
+ private void addDefaultMethods(DexMethodSignatureMap<DexType> signatures, DexProgramClass iface) {
+ // When the same signature is added from several interfaces just move to the "multiple" state
+ // and do not keep track of the actual interfaces.
+ iface.forEachProgramVirtualMethodMatching(
+ DexEncodedMethod::isDefaultMethod,
+ method ->
+ signatures.merge(
+ method.getDefinition(),
+ iface.getType(),
+ (ignoreKey, current) -> current == iface.getType() ? current : MULTIPLE_SENTINEL));
+ }
+
+ private DexMethodSignatureMap<DexType> collectDefaultMethodsInInterfaces(DexProgramClass iface) {
+ assert iface.isInterface();
+ DexMethodSignatureMap<DexType> signatures = DexMethodSignatureMap.create();
+ WorkList<DexProgramClass> workList = WorkList.newIdentityWorkList();
+ workList.addIfNotSeen(iface);
+ while (workList.hasNext()) {
+ DexProgramClass item = workList.next();
+ assert item.isInterface();
+ addDefaultMethods(signatures, item);
+ addInterfacesToWorklist(item, workList);
+ }
+ return signatures;
+ }
+
+ // TODO(b/229951607): This only adresses the ICCE issue for synthetic lambda classes.
+ private DexMethodSignatureMap<DexType> collectDefaultMethodsInImplementedInterfaces(
+ DexProgramClass clazz) {
+ assert !clazz.isInterface();
+ DexMethodSignatureMap<DexType> signatures = DexMethodSignatureMap.create();
+ WorkList<DexProgramClass> workList = WorkList.newIdentityWorkList();
+ addInterfacesToWorklist(clazz, workList);
+ while (workList.hasNext()) {
+ DexProgramClass item = workList.next();
+ assert item.isInterface();
+ addDefaultMethods(signatures, item);
+ addInterfacesToWorklist(item, workList);
+ }
+ return signatures;
+ }
+
+ private void addInterfacesToWorklist(DexProgramClass clazz, WorkList<DexProgramClass> worklist) {
+ for (DexType iface : clazz.getInterfaces()) {
+ DexProgramClass ifaceDefinition = appView.programDefinitionFor(iface, clazz);
+ if (ifaceDefinition != null && ifaceDefinition.isInterface()) {
+ worklist.addIfNotSeen(ifaceDefinition);
+ }
+ }
}
@Override
- public boolean shouldSkipPolicy() {
- return !options.horizontalClassMergerOptions().isInterfaceMergingEnabled(mode);
+ public String getName() {
+ return "NoDefaultInterfaceMethodMerging";
}
}
diff --git a/src/main/java/com/android/tools/r8/utils/collections/DexMethodSignatureMap.java b/src/main/java/com/android/tools/r8/utils/collections/DexMethodSignatureMap.java
new file mode 100644
index 0000000..cff8be7
--- /dev/null
+++ b/src/main/java/com/android/tools/r8/utils/collections/DexMethodSignatureMap.java
@@ -0,0 +1,210 @@
+// Copyright (c) 2022, the R8 project authors. Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+package com.android.tools.r8.utils.collections;
+
+import com.android.tools.r8.graph.DexEncodedMethod;
+import com.android.tools.r8.graph.DexMethod;
+import com.android.tools.r8.graph.DexMethodSignature;
+import java.util.Collection;
+import java.util.HashMap;
+import java.util.LinkedHashMap;
+import java.util.Map;
+import java.util.Set;
+import java.util.function.BiConsumer;
+import java.util.function.BiFunction;
+import java.util.function.Function;
+import org.jetbrains.annotations.NotNull;
+
+public class DexMethodSignatureMap<T> implements Map<DexMethodSignature, T> {
+
+ private final Map<DexMethodSignature, T> backing;
+
+ private DexMethodSignatureMap(Map<DexMethodSignature, T> backing) {
+ this.backing = backing;
+ }
+
+ public static <T> DexMethodSignatureMap<T> create() {
+ return new DexMethodSignatureMap<>(new HashMap<>());
+ }
+
+ public static <T> DexMethodSignatureMap<T> createLinked() {
+ return new DexMethodSignatureMap<>(new LinkedHashMap<>());
+ }
+
+ @Override
+ public T put(DexMethodSignature signature, T value) {
+ return backing.put(signature, value);
+ }
+
+ public T put(DexMethod method, T value) {
+ return put(method.getSignature(), value);
+ }
+
+ public T put(DexEncodedMethod method, T value) {
+ return put(method.getReference(), value);
+ }
+
+ @Override
+ public void clear() {
+ backing.clear();
+ }
+
+ @Override
+ public Set<DexMethodSignature> keySet() {
+ return backing.keySet();
+ }
+
+ @Override
+ public Collection<T> values() {
+ return backing.values();
+ }
+
+ @Override
+ public Set<Entry<DexMethodSignature, T>> entrySet() {
+ return backing.entrySet();
+ }
+
+ @Override
+ public T getOrDefault(Object key, T defaultValue) {
+ return backing.getOrDefault(key, defaultValue);
+ }
+
+ @Override
+ public void forEach(BiConsumer<? super DexMethodSignature, ? super T> action) {
+ backing.forEach(action);
+ }
+
+ @Override
+ public void replaceAll(BiFunction<? super DexMethodSignature, ? super T, ? extends T> function) {
+ backing.replaceAll(function);
+ }
+
+ @Override
+ public T putIfAbsent(DexMethodSignature key, T value) {
+ return backing.putIfAbsent(key, value);
+ }
+
+ @Override
+ public boolean remove(Object key, Object value) {
+ return backing.remove(key, value);
+ }
+
+ @Override
+ public boolean replace(DexMethodSignature key, T oldValue, T newValue) {
+ return backing.replace(key, oldValue, newValue);
+ }
+
+ @Override
+ public T replace(DexMethodSignature key, T value) {
+ return backing.replace(key, value);
+ }
+
+ @Override
+ public T computeIfAbsent(
+ DexMethodSignature key,
+ @NotNull Function<? super DexMethodSignature, ? extends T> mappingFunction) {
+ return backing.computeIfAbsent(key, mappingFunction);
+ }
+
+ @Override
+ public T computeIfPresent(
+ DexMethodSignature key,
+ @NotNull BiFunction<? super DexMethodSignature, ? super T, ? extends T> remappingFunction) {
+ return backing.computeIfPresent(key, remappingFunction);
+ }
+
+ @Override
+ public T compute(
+ DexMethodSignature key,
+ @NotNull BiFunction<? super DexMethodSignature, ? super T, ? extends T> remappingFunction) {
+ return backing.compute(key, remappingFunction);
+ }
+
+ @Override
+ public T merge(
+ DexMethodSignature key,
+ @NotNull T value,
+ @NotNull BiFunction<? super T, ? super T, ? extends T> remappingFunction) {
+ return backing.merge(key, value, remappingFunction);
+ }
+
+ public T merge(
+ DexMethod method,
+ @NotNull T value,
+ @NotNull BiFunction<? super T, ? super T, ? extends T> remappingFunction) {
+ return merge(method.getSignature(), value, remappingFunction);
+ }
+
+ public T merge(
+ DexEncodedMethod method,
+ @NotNull T value,
+ @NotNull BiFunction<? super T, ? super T, ? extends T> remappingFunction) {
+ return merge(method.getReference(), value, remappingFunction);
+ }
+
+ @Override
+ public boolean containsKey(Object o) {
+ return backing.containsKey(o);
+ }
+
+ @Override
+ public boolean containsValue(Object value) {
+ return backing.containsValue(value);
+ }
+
+ @Override
+ public T get(Object key) {
+ return backing.get(key);
+ }
+
+ public boolean containsKey(DexMethodSignature signature) {
+ return backing.containsKey(signature);
+ }
+
+ public boolean containsAnyKeyOf(Iterable<DexMethodSignature> signatures) {
+ for (DexMethodSignature signature : signatures) {
+ if (containsKey(signature)) {
+ return true;
+ }
+ }
+ return false;
+ }
+
+ public DexMethodSignatureSet intersectionWithKeys(Iterable<DexMethodSignature> signatures) {
+ DexMethodSignatureSet result = DexMethodSignatureSet.create();
+ for (DexMethodSignature signature : signatures) {
+ if (containsKey(signature)) {
+ result.add(signature);
+ }
+ }
+ return result;
+ }
+
+ @Override
+ public boolean isEmpty() {
+ return backing.isEmpty();
+ }
+
+ @Override
+ public T remove(Object o) {
+ return backing.remove(o);
+ }
+
+ @Override
+ public void putAll(@NotNull Map<? extends DexMethodSignature, ? extends T> m) {}
+
+ public T remove(DexMethodSignature signature) {
+ return backing.remove(signature);
+ }
+
+ public T remove(DexEncodedMethod method) {
+ return remove(method.getSignature());
+ }
+
+ @Override
+ public int size() {
+ return backing.size();
+ }
+}
diff --git a/src/test/java/com/android/tools/r8/classmerging/horizontal/interfaces/CollisionWithDefaultMethodsTest.java b/src/test/java/com/android/tools/r8/classmerging/horizontal/interfaces/CollisionWithDefaultMethodsTest.java
index 072962b..55f5b52 100644
--- a/src/test/java/com/android/tools/r8/classmerging/horizontal/interfaces/CollisionWithDefaultMethodsTest.java
+++ b/src/test/java/com/android/tools/r8/classmerging/horizontal/interfaces/CollisionWithDefaultMethodsTest.java
@@ -42,12 +42,16 @@
.addKeepMainRule(TestClass.class)
.setMinApi(parameters.getApiLevel())
.addKeepRules("-keep class ** { *; }")
+ .addHorizontallyMergedClassesInspector(
+ inspector -> {
+ if (parameters.isCfRuntime()
+ || (parameters.isDexRuntime()
+ && parameters.canUseDefaultAndStaticInterfaceMethods())) {
+ inspector.assertNoClassesMerged();
+ }
+ })
.run(parameters.getRuntime(), TestClass.class)
- // TODO(b/229951607): This should never throw ICCE.
- .applyIf(
- parameters.isDexRuntime() && hasDefaultInterfaceMethodsSupport(parameters),
- r -> r.assertFailureWithErrorThatThrows(IncompatibleClassChangeError.class),
- r -> r.assertSuccessWithOutput(EXPECTED_OUTPUT));
+ .assertSuccessWithOutput(EXPECTED_OUTPUT);
}
static class Event {}
