Fix interface method minification bug
Bug: 123730537
Change-Id: I0596457e48ff1f6bd03acea76442f9ffd12f615a
diff --git a/src/main/java/com/android/tools/r8/naming/InterfaceMethodNameMinifier.java b/src/main/java/com/android/tools/r8/naming/InterfaceMethodNameMinifier.java
index a511b0a..b643be3 100644
--- a/src/main/java/com/android/tools/r8/naming/InterfaceMethodNameMinifier.java
+++ b/src/main/java/com/android/tools/r8/naming/InterfaceMethodNameMinifier.java
@@ -36,21 +36,6 @@
public class InterfaceMethodNameMinifier {
- public interface InterfaceMethodOrdering {
-
- Comparator<Wrapper<DexMethod>> getComparator(InterfaceMethodNameMinifier minifier);
- }
-
- public static class DefaultInterfaceMethodOrdering implements InterfaceMethodOrdering {
-
- @Override
- public Comparator<Wrapper<DexMethod>> getComparator(InterfaceMethodNameMinifier minifier) {
- Map<Wrapper<DexMethod>, Set<NamingState<DexProto, ?>>> globalStateMap =
- minifier.globalStateMap;
- return (a, b) -> globalStateMap.get(b).size() - globalStateMap.get(a).size();
- }
- }
-
private final AppInfoWithLiveness appInfo;
private final Set<DexCallSite> desugaredCallSites;
private final Equivalence<DexMethod> equivalence;
@@ -87,6 +72,10 @@
this.options = options;
}
+ public Comparator<Wrapper<DexMethod>> createDefaultInterfaceMethodOrdering() {
+ return (a, b) -> globalStateMap.get(b).size() - globalStateMap.get(a).size();
+ }
+
Map<DexCallSite, DexString> getCallSiteRenamings() {
return callSiteRenamings;
}
@@ -180,10 +169,27 @@
List<Wrapper<DexMethod>> interfaceMethods =
globalStateMap.keySet().stream()
.filter(wrapper -> unificationParent.getOrDefault(wrapper, wrapper).equals(wrapper))
- .sorted(options.testing.minifier.interfaceMethodOrdering.getComparator(this))
+ .sorted(options.testing.minifier.createInterfaceMethodOrdering(this))
.collect(Collectors.toList());
+
+ // Propagate reserved names to all states.
+ List<Wrapper<DexMethod>> reservedInterfaceMethods =
+ interfaceMethods.stream()
+ .filter(wrapper -> anyIsReserved(wrapper, unification))
+ .collect(Collectors.toList());
+ for (Wrapper<DexMethod> key : reservedInterfaceMethods) {
+ propagateReservedNames(key, unification);
+ }
+
+ // Verify that there is no more to propagate.
+ assert reservedInterfaceMethods.stream()
+ .noneMatch(key -> propagateReservedNames(key, unification));
+
+ // Assign names to unreserved interface methods.
for (Wrapper<DexMethod> key : interfaceMethods) {
- assignNameToInterfaceMethod(key, unification);
+ if (!reservedInterfaceMethods.contains(key)) {
+ assignNameToInterfaceMethod(key, unification);
+ }
}
for (Entry<DexCallSite, DexMethod> entry : callSites.entrySet()) {
@@ -200,6 +206,24 @@
timing.end();
}
+ private boolean propagateReservedNames(
+ Wrapper<DexMethod> key, Map<Wrapper<DexMethod>, Set<Wrapper<DexMethod>>> unification) {
+ Set<Wrapper<DexMethod>> unifiedMethods =
+ unification.getOrDefault(key, Collections.singleton(key));
+ boolean changed = false;
+ for (Wrapper<DexMethod> wrapper : unifiedMethods) {
+ DexMethod unifiedMethod = wrapper.get();
+ assert unifiedMethod != null;
+ for (NamingState<DexProto, ?> namingState : globalStateMap.get(wrapper)) {
+ if (!namingState.isReserved(unifiedMethod.name, unifiedMethod.proto)) {
+ namingState.reserveName(unifiedMethod.name, unifiedMethod.proto);
+ changed = true;
+ }
+ }
+ }
+ return changed;
+ }
+
private void assignNameToInterfaceMethod(
Wrapper<DexMethod> key, Map<Wrapper<DexMethod>, Set<Wrapper<DexMethod>>> unification) {
List<MethodNamingState> collectedStates = new ArrayList<>();
@@ -233,13 +257,8 @@
List<MethodNamingState> collectedStates,
Set<DexMethod> sourceMethods,
MethodNamingState originState) {
- if (anyIsReserved(collectedStates)) {
- // This method's name is reserved in at least one naming state, so reserve it everywhere.
- for (MethodNamingState state : collectedStates) {
- state.reserveName();
- }
- return;
- }
+ assert !anyIsReserved(collectedStates);
+
// We use the origin state to allocate a name here so that we can reuse names between different
// unrelated interfaces. This saves some space. The alternative would be to use a global state
// for allocating names, which would save the work to search here.
@@ -282,6 +301,23 @@
originStates.putIfAbsent(key, minifierState.getState(originInterface));
}
+ private boolean anyIsReserved(
+ Wrapper<DexMethod> key, Map<Wrapper<DexMethod>, Set<Wrapper<DexMethod>>> unification) {
+ Set<Wrapper<DexMethod>> unifiedMethods =
+ unification.getOrDefault(key, Collections.singleton(key));
+ for (Wrapper<DexMethod> wrapper : unifiedMethods) {
+ DexMethod unifiedMethod = wrapper.get();
+ assert unifiedMethod != null;
+
+ for (NamingState<DexProto, ?> namingState : globalStateMap.get(wrapper)) {
+ if (namingState.isReserved(unifiedMethod.name, unifiedMethod.proto)) {
+ return true;
+ }
+ }
+ }
+ return false;
+ }
+
private boolean anyIsReserved(List<MethodNamingState> collectedStates) {
DexString name = collectedStates.get(0).getName();
Map<DexProto, Boolean> globalStateCache = new IdentityHashMap<>();
diff --git a/src/main/java/com/android/tools/r8/utils/InternalOptions.java b/src/main/java/com/android/tools/r8/utils/InternalOptions.java
index a28c107..85c4876 100644
--- a/src/main/java/com/android/tools/r8/utils/InternalOptions.java
+++ b/src/main/java/com/android/tools/r8/utils/InternalOptions.java
@@ -21,17 +21,18 @@
import com.android.tools.r8.graph.DexType;
import com.android.tools.r8.ir.code.IRCode;
import com.android.tools.r8.ir.optimize.Inliner;
-import com.android.tools.r8.naming.InterfaceMethodNameMinifier.DefaultInterfaceMethodOrdering;
-import com.android.tools.r8.naming.InterfaceMethodNameMinifier.InterfaceMethodOrdering;
+import com.android.tools.r8.naming.InterfaceMethodNameMinifier;
import com.android.tools.r8.origin.Origin;
import com.android.tools.r8.shaking.ProguardConfiguration;
import com.android.tools.r8.shaking.ProguardConfigurationRule;
import com.android.tools.r8.utils.IROrdering.IdentityIROrdering;
import com.android.tools.r8.utils.IROrdering.NondeterministicIROrdering;
+import com.google.common.base.Equivalence.Wrapper;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import java.nio.file.Path;
import java.util.ArrayList;
+import java.util.Comparator;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
@@ -566,7 +567,15 @@
public static class MinifierTestingOptions {
- public InterfaceMethodOrdering interfaceMethodOrdering = new DefaultInterfaceMethodOrdering();
+ public Comparator<DexMethod> interfaceMethodOrdering = null;
+
+ public Comparator<Wrapper<DexMethod>> createInterfaceMethodOrdering(
+ InterfaceMethodNameMinifier minifier) {
+ if (interfaceMethodOrdering != null) {
+ return (a, b) -> interfaceMethodOrdering.compare(a.get(), b.get());
+ }
+ return minifier.createDefaultInterfaceMethodOrdering();
+ }
}
}
diff --git a/src/test/java/com/android/tools/r8/naming/InterfaceMethodNameMinifierTest.java b/src/test/java/com/android/tools/r8/naming/InterfaceMethodNameMinifierTest.java
index 4315486..f2e1a60 100644
--- a/src/test/java/com/android/tools/r8/naming/InterfaceMethodNameMinifierTest.java
+++ b/src/test/java/com/android/tools/r8/naming/InterfaceMethodNameMinifierTest.java
@@ -4,10 +4,8 @@
package com.android.tools.r8.naming;
-import static org.junit.Assert.fail;
-
-import com.android.tools.r8.CompilationFailedException;
import com.android.tools.r8.TestBase;
+import com.android.tools.r8.graph.DexMethod;
import com.android.tools.r8.utils.FileUtils;
import java.nio.file.Path;
import org.junit.Test;
@@ -17,28 +15,20 @@
@Test
public void test() throws Exception {
- try {
- Path dictionary = temp.getRoot().toPath().resolve("dictionary.txt");
- FileUtils.writeTextFile(dictionary, "a", "b", "c");
+ Path dictionary = temp.getRoot().toPath().resolve("dictionary.txt");
+ FileUtils.writeTextFile(dictionary, "a", "b", "c");
- testForR8(Backend.DEX)
- .addInnerClasses(InterfaceMethodNameMinifierTest.class)
- .addKeepRules(
- "-keep,allowobfuscation interface * { <methods>; }",
- "-keep,allowobfuscation class * { <methods>; }",
- "-keep interface " + L.class.getTypeName() + " { <methods>; }",
- "-obfuscationdictionary " + dictionary.toString())
- // Minify the interface methods in alphabetic order.
- .addOptionsModification(
- options ->
- options.testing.minifier.interfaceMethodOrdering =
- minifier -> (a, b) -> a.get().slowCompareTo(b.get()))
- .compile();
- } catch (CompilationFailedException e) {
- // TODO(b/123730537): Fails with duplicate methods.
- return;
- }
- fail();
+ testForR8(Backend.DEX)
+ .addInnerClasses(InterfaceMethodNameMinifierTest.class)
+ .addKeepRules(
+ "-keep,allowobfuscation interface * { <methods>; }",
+ "-keep,allowobfuscation class * { <methods>; }",
+ "-keep interface " + L.class.getTypeName() + " { <methods>; }",
+ "-obfuscationdictionary " + dictionary.toString())
+ // Minify the interface methods in alphabetic order.
+ .addOptionsModification(
+ options -> options.testing.minifier.interfaceMethodOrdering = DexMethod::slowCompareTo)
+ .compile();
}
interface I {}
