Merge "Reproduce interface method minification bug"
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 718ce78..a511b0a 100644
--- a/src/main/java/com/android/tools/r8/naming/InterfaceMethodNameMinifier.java
+++ b/src/main/java/com/android/tools/r8/naming/InterfaceMethodNameMinifier.java
@@ -19,9 +19,12 @@
import com.android.tools.r8.utils.Timing;
import com.google.common.base.Equivalence;
import com.google.common.base.Equivalence.Wrapper;
+import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Sets;
+import java.io.PrintStream;
import java.util.ArrayList;
import java.util.Collections;
+import java.util.Comparator;
import java.util.HashMap;
import java.util.HashSet;
import java.util.IdentityHashMap;
@@ -29,8 +32,24 @@
import java.util.Map;
import java.util.Map.Entry;
import java.util.Set;
+import java.util.stream.Collectors;
-class InterfaceMethodNameMinifier {
+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;
@@ -75,7 +94,6 @@
private void reserveNamesInInterfaces() {
for (DexType type : DexType.allInterfaces(appInfo.dexItemFactory)) {
assert type.isInterface();
- frontierState.put(type, type);
frontierState.allocateNamingStateAndReserve(type, type, null);
}
}
@@ -95,7 +113,7 @@
assert clazz.isInterface();
Set<NamingState<DexProto, ?>> collectedStates = getReachableStates(type);
for (DexEncodedMethod method : shuffleMethods(clazz.methods(), options)) {
- addStatesToGlobalMapForMethod(method, collectedStates, type);
+ addStatesToGlobalMapForMethod(method.method, collectedStates, type);
}
}
}
@@ -130,7 +148,7 @@
DexType iface = method.method.holder;
assert iface.isInterface();
Set<NamingState<DexProto, ?>> collectedStates = getReachableStates(iface);
- addStatesToGlobalMapForMethod(method, collectedStates, iface);
+ addStatesToGlobalMapForMethod(method.method, collectedStates, iface);
callSiteMethods.add(equivalence.wrap(method.method));
}
if (callSiteMethods.size() > 1) {
@@ -156,31 +174,18 @@
timing.end();
// Go over every method and assign a name.
timing.begin("Allocate names");
+
// Sort the methods by the number of dependent states, so that we use short names for methods
// that are referenced in many places.
- List<Wrapper<DexMethod>> methods = new ArrayList<>(globalStateMap.keySet());
- methods.sort((a, b) -> globalStateMap.get(b).size() - globalStateMap.get(a).size());
- for (Wrapper<DexMethod> key : methods) {
- if (!unificationParent.getOrDefault(key, key).equals(key)) {
- continue;
- }
- List<MethodNamingState> collectedStates = new ArrayList<>();
- Set<DexMethod> sourceMethods = Sets.newIdentityHashSet();
- for (Wrapper<DexMethod> k : unification.getOrDefault(key, Collections.singleton(key))) {
- DexMethod unifiedMethod = k.get();
- assert unifiedMethod != null;
- sourceMethods.addAll(sourceMethodsMap.get(k));
- for (NamingState<DexProto, ?> namingState : globalStateMap.get(k)) {
- collectedStates.add(
- new MethodNamingState(namingState, unifiedMethod.name, unifiedMethod.proto));
- }
- }
- DexMethod method = key.get();
- assert method != null;
- MethodNamingState originState =
- new MethodNamingState(originStates.get(key), method.name, method.proto);
- assignNameForInterfaceMethodInAllStates(collectedStates, sourceMethods, originState);
+ List<Wrapper<DexMethod>> interfaceMethods =
+ globalStateMap.keySet().stream()
+ .filter(wrapper -> unificationParent.getOrDefault(wrapper, wrapper).equals(wrapper))
+ .sorted(options.testing.minifier.interfaceMethodOrdering.getComparator(this))
+ .collect(Collectors.toList());
+ for (Wrapper<DexMethod> key : interfaceMethods) {
+ assignNameToInterfaceMethod(key, unification);
}
+
for (Entry<DexCallSite, DexMethod> entry : callSites.entrySet()) {
DexMethod method = entry.getValue();
DexString renamed = minifierState.getRenaming(method);
@@ -195,6 +200,35 @@
timing.end();
}
+ private void assignNameToInterfaceMethod(
+ Wrapper<DexMethod> key, Map<Wrapper<DexMethod>, Set<Wrapper<DexMethod>>> unification) {
+ List<MethodNamingState> collectedStates = new ArrayList<>();
+ Set<DexMethod> sourceMethods = Sets.newIdentityHashSet();
+ for (Wrapper<DexMethod> k : unification.getOrDefault(key, Collections.singleton(key))) {
+ DexMethod unifiedMethod = k.get();
+ assert unifiedMethod != null;
+ sourceMethods.addAll(sourceMethodsMap.get(k));
+ for (NamingState<DexProto, ?> namingState : globalStateMap.get(k)) {
+ collectedStates.add(
+ new MethodNamingState(namingState, unifiedMethod.name, unifiedMethod.proto));
+ }
+ }
+
+ DexMethod method = key.get();
+ assert method != null;
+
+ Set<String> loggingFilter = options.extensiveInterfaceMethodMinifierLoggingFilter;
+ if (!loggingFilter.isEmpty()) {
+ if (sourceMethods.stream().map(DexMethod::toSourceString).anyMatch(loggingFilter::contains)) {
+ print(method, sourceMethods, collectedStates, System.out);
+ }
+ }
+
+ MethodNamingState originState =
+ new MethodNamingState(originStates.get(key), method.name, method.proto);
+ assignNameForInterfaceMethodInAllStates(collectedStates, sourceMethods, originState);
+ }
+
private void assignNameForInterfaceMethodInAllStates(
List<MethodNamingState> collectedStates,
Set<DexMethod> sourceMethods,
@@ -213,6 +247,7 @@
DexString candidate;
do {
candidate = originState.assignNewName();
+
// If the state returns the same candidate for two consecutive trials, it should be this case:
// 1) an interface method with the same signature (name, param) but different return type
// has been already renamed; and 2) -useuniqueclassmembernames is set.
@@ -240,14 +275,10 @@
}
private void addStatesToGlobalMapForMethod(
- DexEncodedMethod method,
- Set<NamingState<DexProto, ?>> collectedStates,
- DexType originInterface) {
- Wrapper<DexMethod> key = equivalence.wrap(method.method);
- Set<NamingState<DexProto, ?>> stateSet =
- globalStateMap.computeIfAbsent(key, k -> new HashSet<>());
- stateSet.addAll(collectedStates);
- sourceMethodsMap.computeIfAbsent(key, k -> new HashSet<>()).add(method.method);
+ DexMethod method, Set<NamingState<DexProto, ?>> collectedStates, DexType originInterface) {
+ Wrapper<DexMethod> key = equivalence.wrap(method);
+ globalStateMap.computeIfAbsent(key, k -> new HashSet<>()).addAll(collectedStates);
+ sourceMethodsMap.computeIfAbsent(key, k -> new HashSet<>()).add(method);
originStates.putIfAbsent(key, minifierState.getState(originInterface));
}
@@ -268,17 +299,23 @@
}
private Set<NamingState<DexProto, ?>> getReachableStates(DexType type) {
- Set<DexType> interfaces = Sets.newIdentityHashSet();
- interfaces.add(type);
- collectSuperInterfaces(type, interfaces);
- collectSubInterfaces(type, interfaces);
+ if (minifierState.useUniqueMemberNames()) {
+ return ImmutableSet.of(minifierState.globalState());
+ }
+
+ Set<DexType> reachableInterfaces = Sets.newIdentityHashSet();
+ reachableInterfaces.add(type);
+ collectSuperInterfaces(type, reachableInterfaces);
+ collectSubInterfaces(type, reachableInterfaces);
+
Set<NamingState<DexProto, ?>> reachableStates = new HashSet<>();
- for (DexType iface : interfaces) {
- // Add the interface itself
- reachableStates.add(minifierState.getState(iface));
+ for (DexType reachableInterface : reachableInterfaces) {
+ // Add the interface itself.
+ reachableStates.add(minifierState.getState(reachableInterface));
+
// And the frontiers that correspond to the classes that implement the interface.
- for (DexType t : iface.allImplementsSubtypes()) {
- NamingState<DexProto, ?> state = minifierState.getState(frontierState.get(t));
+ for (DexType frontier : reachableInterface.allImplementsSubtypes()) {
+ NamingState<DexProto, ?> state = minifierState.getState(frontierState.get(frontier));
assert state != null;
reachableStates.add(state);
}
@@ -307,4 +344,21 @@
}
}
}
+
+ private void print(
+ DexMethod method,
+ Set<DexMethod> sourceMethods,
+ List<MethodNamingState> collectedStates,
+ PrintStream out) {
+ out.println("-----------------------------------------------------------------------");
+ out.println("assignNameToInterfaceMethod(`" + method.toSourceString() + "`)");
+ out.println("-----------------------------------------------------------------------");
+ out.println("Source methods:");
+ for (DexMethod sourceMethod : sourceMethods) {
+ out.println(" " + sourceMethod.toSourceString());
+ }
+ out.println("States:");
+ collectedStates.forEach(state -> state.print(" ", minifierState::getStateKey, out));
+ out.println();
+ }
}
diff --git a/src/main/java/com/android/tools/r8/naming/MemberNameMinifier.java b/src/main/java/com/android/tools/r8/naming/MemberNameMinifier.java
index 8117dd5..419bfbb 100644
--- a/src/main/java/com/android/tools/r8/naming/MemberNameMinifier.java
+++ b/src/main/java/com/android/tools/r8/naming/MemberNameMinifier.java
@@ -69,6 +69,14 @@
return useUniqueMemberNames ? globalState : states.get(type);
}
+ DexType getStateKey(NamingState<StateType, ?> state) {
+ return states.inverse().get(state);
+ }
+
+ NamingState<StateType, ?> globalState() {
+ return globalState;
+ }
+
boolean isReservedInGlobalState(DexString name, StateType state) {
return globalState.isReserved(name, state);
}
diff --git a/src/main/java/com/android/tools/r8/naming/MethodNameMinifier.java b/src/main/java/com/android/tools/r8/naming/MethodNameMinifier.java
index 4b67208..8864271 100644
--- a/src/main/java/com/android/tools/r8/naming/MethodNameMinifier.java
+++ b/src/main/java/com/android/tools/r8/naming/MethodNameMinifier.java
@@ -19,6 +19,7 @@
import com.google.common.base.Equivalence;
import com.google.common.base.Equivalence.Wrapper;
import com.google.common.collect.ImmutableMap;
+import java.io.PrintStream;
import java.util.HashMap;
import java.util.IdentityHashMap;
import java.util.Map;
@@ -132,8 +133,7 @@
// Phase 1: Reserve all the names that need to be kept and allocate linked state in the
// library part.
timing.begin("Phase 1");
- reserveNamesInClasses(
- appInfo.dexItemFactory.objectType, appInfo.dexItemFactory.objectType, null);
+ reserveNamesInClasses();
timing.end();
// Phase 2: Reserve all the names that are required for interfaces, and then assign names to
// interface methods. These are assigned by finding a name that is free in all naming
@@ -195,30 +195,24 @@
}
}
+ private void reserveNamesInClasses() {
+ reserveNamesInClasses(
+ appInfo.dexItemFactory.objectType, appInfo.dexItemFactory.objectType, null);
+ }
+
private void reserveNamesInClasses(
DexType type, DexType libraryFrontier, NamingState<DexProto, ?> parent) {
assert !type.isInterface();
NamingState<DexProto, ?> state =
frontierState.allocateNamingStateAndReserve(type, libraryFrontier, parent);
+
// If this is a library class (or effectively a library class as it is missing) move the
// frontier forward.
DexClass holder = appInfo.definitionFor(type);
- type.forAllExtendsSubtypes(
- subtype -> {
- assert !subtype.isInterface();
- reserveNamesInClasses(
- subtype,
- holder == null || holder.isLibraryClass() ? subtype : libraryFrontier,
- state);
- });
- }
-
- private void reserveNamesForMethod(
- DexEncodedMethod encodedMethod, boolean keepAll, NamingState<DexProto, ?> state) {
- DexMethod method = encodedMethod.method;
- if (keepAll || rootSet.noObfuscation.contains(method)) {
- state.reserveName(method.name, method.proto);
- globalState.reserveName(method.name, method.proto);
+ for (DexType subtype : type.allExtendsSubtypes()) {
+ assert !subtype.isInterface();
+ reserveNamesInClasses(
+ subtype, holder == null || holder.isLibraryClass() ? subtype : libraryFrontier, state);
}
}
@@ -227,12 +221,14 @@
private final Map<DexType, DexType> frontiers = new IdentityHashMap<>();
NamingState<DexProto, ?> allocateNamingStateAndReserve(
- DexType type, DexType libraryFrontier, NamingState<DexProto, ?> parent) {
- frontiers.put(type, libraryFrontier);
+ DexType type, DexType frontier, NamingState<DexProto, ?> parent) {
+ if (frontier != type) {
+ frontiers.put(type, frontier);
+ }
NamingState<DexProto, ?> state =
computeStateIfAbsent(
- libraryFrontier,
+ frontier,
ignore ->
parent == null
? NamingState.createRoot(
@@ -246,18 +242,28 @@
if (holder != null) {
boolean keepAll = holder.isLibraryClass() || holder.accessFlags.isAnnotation();
for (DexEncodedMethod method : shuffleMethods(holder.methods(), options)) {
- reserveNamesForMethod(method, keepAll, state);
+ // TODO(christofferqa): Wouldn't it be sufficient only to reserve names for non-private
+ // methods?
+ if (keepAll || rootSet.noObfuscation.contains(method.method)) {
+ reserveNamesForMethod(method.method, state);
+ }
}
}
return state;
}
+ private void reserveNamesForMethod(DexMethod method, NamingState<DexProto, ?> state) {
+ state.reserveName(method.name, method.proto);
+ globalState.reserveName(method.name, method.proto);
+ }
+
public DexType get(DexType type) {
- return frontiers.get(type);
+ return frontiers.getOrDefault(type, type);
}
public DexType put(DexType type, DexType frontier) {
+ assert frontier != type;
return frontiers.put(type, frontier);
}
}
@@ -307,6 +313,19 @@
DexProto getProto() {
return proto;
}
+
+ void print(
+ String indentation,
+ Function<NamingState<DexProto, ?>, DexType> stateKeyGetter,
+ PrintStream out) {
+ DexType stateKey = stateKeyGetter.apply(parent);
+ out.print(indentation);
+ out.print(stateKey != null ? stateKey.toSourceString() : "<?>");
+ out.print(".");
+ out.print(name.toSourceString());
+ out.println(proto.toSmaliString());
+ parent.printState(proto, indentation + " ", out);
+ }
}
// Shuffles the given methods if assertions are enabled and deterministic debugging is disabled.
diff --git a/src/main/java/com/android/tools/r8/naming/NamingState.java b/src/main/java/com/android/tools/r8/naming/NamingState.java
index f0f1720..d414b3b 100644
--- a/src/main/java/com/android/tools/r8/naming/NamingState.java
+++ b/src/main/java/com/android/tools/r8/naming/NamingState.java
@@ -11,10 +11,12 @@
import com.google.common.collect.Iterables;
import com.google.common.collect.Sets;
import com.google.common.collect.Table;
+import java.io.PrintStream;
import java.util.HashMap;
import java.util.Iterator;
import java.util.List;
import java.util.Map;
+import java.util.Map.Entry;
import java.util.Set;
import java.util.function.Function;
@@ -129,6 +131,18 @@
state.addRenaming(original, key, newName);
}
+ void printState(ProtoType proto, String indentation, PrintStream out) {
+ KeyType key = keyTransform.apply(proto);
+ InternalState state = findInternalStateFor(key);
+ if (state != null) {
+ state.printReservedNames(indentation, out);
+ state.printRenamings(indentation, out);
+ } else {
+ out.print(indentation);
+ out.println("<NO STATE>");
+ }
+ }
+
class InternalState {
private static final int INITIAL_NAME_COUNT = 1;
@@ -229,5 +243,49 @@
return StringUtils.numberToIdentifier(EMPTY_CHAR_ARRAY, nameCount++, false);
}
}
+
+ void printReservedNames(String indentation, PrintStream out) {
+ out.print(indentation);
+ out.print("Reserved names:");
+ if (reservedNames == null || reservedNames.isEmpty()) {
+ out.print(" <NO RESERVED NAMES>");
+ } else {
+ for (DexString reservedName : reservedNames) {
+ out.print(System.lineSeparator());
+ out.print(indentation);
+ out.print(" ");
+ out.print(reservedName.toSourceString());
+ }
+ }
+ out.println();
+ if (parentInternalState != null) {
+ parentInternalState.printReservedNames(indentation + " ", out);
+ }
+ }
+
+ void printRenamings(String indentation, PrintStream out) {
+ out.print(indentation);
+ out.print("Renamings:");
+ if (renamings == null || renamings.isEmpty()) {
+ out.print(" <NO RENAMINGS>");
+ } else {
+ for (DexString original : renamings.rowKeySet()) {
+ Map<KeyType, DexString> row = renamings.row(original);
+ for (Entry<KeyType, DexString> entry : row.entrySet()) {
+ out.print(System.lineSeparator());
+ out.print(indentation);
+ out.print(" ");
+ out.print(original.toSourceString());
+ out.print(entry.getKey().toString());
+ out.print(" -> ");
+ out.print(entry.getValue().toSourceString());
+ }
+ }
+ }
+ out.println();
+ if (parentInternalState != null) {
+ parentInternalState.printRenamings(indentation + " ", out);
+ }
+ }
}
}
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 a5854b6..a28c107 100644
--- a/src/main/java/com/android/tools/r8/utils/InternalOptions.java
+++ b/src/main/java/com/android/tools/r8/utils/InternalOptions.java
@@ -21,6 +21,8 @@
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.origin.Origin;
import com.android.tools.r8.shaking.ProguardConfiguration;
import com.android.tools.r8.shaking.ProguardConfigurationRule;
@@ -223,6 +225,9 @@
}
public Set<String> extensiveLoggingFilter = getExtensiveLoggingFilter();
+ public Set<String> extensiveInterfaceMethodMinifierLoggingFilter =
+ getExtensiveInterfaceMethodMinifierLoggingFilter();
+
public List<String> methodsFilter = ImmutableList.of();
public int minApiLevel = AndroidApiLevel.getDefault().getLevel();
// Skipping min_api check and compiling an intermediate result intended for later merging.
@@ -298,6 +303,19 @@
return ImmutableSet.of();
}
+ private static Set<String> getExtensiveInterfaceMethodMinifierLoggingFilter() {
+ String property =
+ System.getProperty("com.android.tools.r8.extensiveInterfaceMethodMinifierLoggingFilter");
+ if (property != null) {
+ ImmutableSet.Builder<String> builder = ImmutableSet.builder();
+ for (String method : property.split(";")) {
+ builder.add(method);
+ }
+ return builder.build();
+ }
+ return ImmutableSet.of();
+ }
+
public static class InvalidParameterAnnotationInfo {
final DexMethod method;
@@ -543,6 +561,13 @@
public boolean forceNameReflectionOptimization = false;
public boolean disallowLoadStoreOptimization = false;
public Consumer<IRCode> irModifier = null;
+
+ public MinifierTestingOptions minifier = new MinifierTestingOptions();
+
+ public static class MinifierTestingOptions {
+
+ public InterfaceMethodOrdering interfaceMethodOrdering = new DefaultInterfaceMethodOrdering();
+ }
}
private boolean hasMinApi(AndroidApiLevel level) {
diff --git a/src/test/java/com/android/tools/r8/R8TestRunResult.java b/src/test/java/com/android/tools/r8/R8TestRunResult.java
index 55730eb..ecf247c 100644
--- a/src/test/java/com/android/tools/r8/R8TestRunResult.java
+++ b/src/test/java/com/android/tools/r8/R8TestRunResult.java
@@ -57,7 +57,6 @@
return self();
}
-
public String proguardMap() {
return proguardMap;
}
diff --git a/src/test/java/com/android/tools/r8/naming/InterfaceMethodNameMinifierTest.java b/src/test/java/com/android/tools/r8/naming/InterfaceMethodNameMinifierTest.java
new file mode 100644
index 0000000..4315486
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/naming/InterfaceMethodNameMinifierTest.java
@@ -0,0 +1,79 @@
+// Copyright (c) 2019, 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.naming;
+
+import static org.junit.Assert.fail;
+
+import com.android.tools.r8.CompilationFailedException;
+import com.android.tools.r8.TestBase;
+import com.android.tools.r8.utils.FileUtils;
+import java.nio.file.Path;
+import org.junit.Test;
+
+/** Regression test for b/123730537. */
+public class InterfaceMethodNameMinifierTest extends TestBase {
+
+ @Test
+ public void test() throws Exception {
+ try {
+ 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();
+ }
+
+ interface I {}
+
+ interface J extends I {
+
+ // Will be renamed first, to a().
+ void a();
+
+ // Will be renamed secondly. Should be renamed to c(), and not b(), because `void K.b()` will
+ // be renamed to b() because `void L.b()` is reserved.
+ void c();
+ }
+
+ interface K extends I {
+
+ // Will be renamed thirdly, to b(), because `void L.b()` is reserved.
+ void b();
+ }
+
+ interface L {
+
+ // Reserved. Will be renamed together with `void K.b()`.
+ void b();
+ }
+
+ static class Implementation implements J, K {
+
+ @Override
+ public void a() {}
+
+ @Override
+ public void b() {}
+
+ @Override
+ public void c() {}
+ }
+}
