Use both name and proto as key in internal naming state.
Once the given proto is used to search for the bound internal state, it
used only *name* to distinguish members. It was safe for javac-generated
bytecode, but that's not the case for general bytecode in compliance with
JVM-spec: the same name could be reused as long as member signatures are
different.
Internal naming states are chained according to the class hierarchy, and
then used to lookup how members in the super type were renamed. For
already aggressively overloaded input, name-based lookup was not able to
resolve members in the super type with the same name. To handle bytecode
in compliance with JVM-spec, we should use both name and proto as key.
One caveat is -useuniqueclassmembernames. To map members with the same
name to the same renamed name, we should use only name as key.
Bug: 73149686
Change-Id: Idb761ad0fa62c8a5fa84407f80fac7d1371aa6da
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 1fa8aaa..9e85188 100644
--- a/src/main/java/com/android/tools/r8/naming/MemberNameMinifier.java
+++ b/src/main/java/com/android/tools/r8/naming/MemberNameMinifier.java
@@ -32,8 +32,8 @@
this.dictionary = options.proguardConfiguration.getObfuscationDictionary();
this.useUniqueMemberNames = options.proguardConfiguration.isUseUniqueClassMemberNames();
this.overloadAggressively = options.proguardConfiguration.isOverloadAggressively();
- this.globalState =
- NamingState.createRoot(appInfo.dexItemFactory, dictionary, getKeyTransform());
+ this.globalState = NamingState.createRoot(
+ appInfo.dexItemFactory, dictionary, getKeyTransform(), useUniqueMemberNames);
}
abstract Function<StateType, ?> getKeyTransform();
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 3138d32..ce37763 100644
--- a/src/main/java/com/android/tools/r8/naming/MethodNameMinifier.java
+++ b/src/main/java/com/android/tools/r8/naming/MethodNameMinifier.java
@@ -343,7 +343,8 @@
computeStateIfAbsent(
libraryFrontier,
ignore -> parent == null
- ? NamingState.createRoot(appInfo.dexItemFactory, dictionary, getKeyTransform())
+ ? NamingState.createRoot(
+ appInfo.dexItemFactory, dictionary, getKeyTransform(), useUniqueMemberNames)
: parent.createChild());
if (holder != null) {
boolean keepAll = holder.isLibraryClass() || holder.accessFlags.isAnnotation();
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 b3ff544..abfa3c1 100644
--- a/src/main/java/com/android/tools/r8/naming/NamingState.java
+++ b/src/main/java/com/android/tools/r8/naming/NamingState.java
@@ -7,9 +7,11 @@
import com.android.tools.r8.graph.DexItemFactory;
import com.android.tools.r8.graph.DexString;
import com.android.tools.r8.utils.StringUtils;
-import com.google.common.collect.HashBiMap;
+import com.google.common.collect.HashBasedTable;
import com.google.common.collect.ImmutableList;
+import com.google.common.collect.Iterables;
import com.google.common.collect.Sets;
+import com.google.common.collect.Table;
import java.util.HashMap;
import java.util.Iterator;
import java.util.List;
@@ -20,80 +22,86 @@
class NamingState<ProtoType extends CachedHashValueDexItem, KeyType> {
private final NamingState<ProtoType, KeyType> parent;
- private final Map<KeyType, InternalState> usedNames = new HashMap<>();
+ private final Map<KeyType, InternalState<ProtoType>> usedNames = new HashMap<>();
private final DexItemFactory itemFactory;
private final ImmutableList<String> dictionary;
private final Function<ProtoType, KeyType> keyTransform;
+ private final boolean useUniqueMemberNames;
static <S, T extends CachedHashValueDexItem> NamingState<T, S> createRoot(
- DexItemFactory itemFactory, ImmutableList<String> dictionary, Function<T, S> keyTransform) {
- return new NamingState<>(null, itemFactory, dictionary, keyTransform);
+ DexItemFactory itemFactory,
+ ImmutableList<String> dictionary,
+ Function<T, S> keyTransform,
+ boolean useUniqueMemberNames) {
+ return new NamingState<>(null, itemFactory, dictionary, keyTransform, useUniqueMemberNames);
}
private NamingState(
NamingState<ProtoType, KeyType> parent,
DexItemFactory itemFactory,
ImmutableList<String> dictionary,
- Function<ProtoType, KeyType> keyTransform) {
+ Function<ProtoType, KeyType> keyTransform,
+ boolean useUniqueMemberNames) {
this.parent = parent;
this.itemFactory = itemFactory;
this.dictionary = dictionary;
this.keyTransform = keyTransform;
+ this.useUniqueMemberNames = useUniqueMemberNames;
}
public NamingState<ProtoType, KeyType> createChild() {
- return new NamingState<>(this, itemFactory, dictionary, keyTransform);
+ return new NamingState<>(this, itemFactory, dictionary, keyTransform, useUniqueMemberNames);
}
- private InternalState findInternalStateFor(ProtoType proto) {
+ private InternalState<ProtoType> findInternalStateFor(ProtoType proto) {
KeyType key = keyTransform.apply(proto);
- InternalState result = usedNames.get(key);
+ InternalState<ProtoType> result = usedNames.get(key);
if (result == null && parent != null) {
result = parent.findInternalStateFor(proto);
}
return result;
}
- private InternalState getOrCreateInternalStateFor(ProtoType proto) {
+ private InternalState<ProtoType> getOrCreateInternalStateFor(ProtoType proto) {
// TODO(herhut): Maybe allocate these sparsely and search via state chain.
KeyType key = keyTransform.apply(proto);
- InternalState result = usedNames.get(key);
+ InternalState<ProtoType> result = usedNames.get(key);
if (result == null) {
if (parent != null) {
- InternalState parentState = parent.getOrCreateInternalStateFor(proto);
+ InternalState<ProtoType> parentState = parent.getOrCreateInternalStateFor(proto);
result = parentState.createChild();
} else {
- result = new InternalState(itemFactory, null, dictionary);
+ result = new InternalState<>(itemFactory, null, dictionary);
}
usedNames.put(key, result);
}
return result;
}
- public DexString getAssignedNameFor(DexString name, ProtoType proto) {
- InternalState state = findInternalStateFor(proto);
+ private DexString getAssignedNameFor(DexString name, ProtoType proto) {
+ InternalState<ProtoType> state = findInternalStateFor(proto);
if (state == null) {
return null;
}
- return state.getAssignedNameFor(name);
+ return state.getAssignedNameFor(name, proto);
}
public DexString assignNewNameFor(DexString original, ProtoType proto, boolean markAsUsed) {
DexString result = getAssignedNameFor(original, proto);
if (result == null) {
- InternalState state = getOrCreateInternalStateFor(proto);
- result = state.getNameFor(original, markAsUsed);
+ InternalState<ProtoType> state = getOrCreateInternalStateFor(proto);
+ result = state.getNameFor(original, proto, markAsUsed);
}
return result;
}
public void reserveName(DexString name, ProtoType proto) {
- InternalState state = getOrCreateInternalStateFor(proto);
+ InternalState<ProtoType> state = getOrCreateInternalStateFor(proto);
state.reserveName(name);
}
public boolean isReserved(DexString name, ProtoType proto) {
- InternalState state = findInternalStateFor(proto);
+ InternalState<ProtoType> state = findInternalStateFor(proto);
if (state == null) {
return false;
}
@@ -101,32 +109,34 @@
}
public boolean isAvailable(DexString original, ProtoType proto, DexString candidate) {
- InternalState state = findInternalStateFor(proto);
+ InternalState<ProtoType> state = findInternalStateFor(proto);
if (state == null) {
return true;
}
- assert state.getAssignedNameFor(original) != candidate;
+ assert state.getAssignedNameFor(original, proto) != candidate;
return state.isAvailable(candidate);
}
public void addRenaming(DexString original, ProtoType proto, DexString newName) {
- InternalState state = getOrCreateInternalStateFor(proto);
- state.addRenaming(original, newName);
+ InternalState<ProtoType> state = getOrCreateInternalStateFor(proto);
+ state.addRenaming(original, proto, newName);
}
- private static class InternalState {
+ private class InternalState<InternalProtoType extends CachedHashValueDexItem> {
private static final int INITIAL_NAME_COUNT = 1;
- private final static char[] EMPTY_CHAR_ARRARY = new char[0];
+ private final char[] EMPTY_CHAR_ARRARY = new char[0];
protected final DexItemFactory itemFactory;
- private final InternalState parentInternalState;
+ private final InternalState<InternalProtoType> parentInternalState;
private Set<DexString> reservedNames = null;
- private Map<DexString, DexString> renamings = null;
+ private Table<DexString, InternalProtoType, DexString> renamings = null;
private int nameCount;
private final Iterator<String> dictionaryIterator;
- private InternalState(DexItemFactory itemFactory, InternalState parentInternalState,
+ private InternalState(
+ DexItemFactory itemFactory,
+ InternalState<InternalProtoType> parentInternalState,
Iterator<String> dictionaryIterator) {
this.itemFactory = itemFactory;
this.parentInternalState = parentInternalState;
@@ -135,7 +145,9 @@
this.dictionaryIterator = dictionaryIterator;
}
- private InternalState(DexItemFactory itemFactory, InternalState parentInternalState,
+ private InternalState(
+ DexItemFactory itemFactory,
+ InternalState<InternalProtoType> parentInternalState,
List<String> dictionary) {
this(itemFactory, parentInternalState, dictionary.iterator());
}
@@ -151,27 +163,40 @@
&& (parentInternalState == null || parentInternalState.isAvailable(name));
}
- public InternalState createChild() {
- return new InternalState(itemFactory, this, dictionaryIterator);
+ InternalState<InternalProtoType> createChild() {
+ return new InternalState<>(itemFactory, this, dictionaryIterator);
}
- public void reserveName(DexString name) {
+ void reserveName(DexString name) {
if (reservedNames == null) {
reservedNames = Sets.newIdentityHashSet();
}
reservedNames.add(name);
}
- public DexString getAssignedNameFor(DexString original) {
- DexString result = renamings == null ? null : renamings.get(original);
+ DexString getAssignedNameFor(DexString original, InternalProtoType proto) {
+ DexString result = null;
+ if (renamings != null) {
+ if (useUniqueMemberNames) {
+ Map<InternalProtoType, DexString> row = renamings.row(original);
+ if (row != null) {
+ // Either not renamed yet (0) or renamed (1). If renamed, return the same renamed name
+ // so that other members with the same name can be renamed to the same renamed name.
+ assert row.values().size() <= 1;
+ result = Iterables.getOnlyElement(row.values(), null);
+ }
+ } else {
+ result = renamings.get(original, proto);
+ }
+ }
if (result == null && parentInternalState != null) {
- result = parentInternalState.getAssignedNameFor(original);
+ result = parentInternalState.getAssignedNameFor(original, proto);
}
return result;
}
- public DexString getNameFor(DexString original, boolean markAsUsed) {
- DexString name = getAssignedNameFor(original);
+ DexString getNameFor(DexString original, InternalProtoType proto, boolean markAsUsed) {
+ DexString name = getAssignedNameFor(original, proto);
if (name != null) {
return name;
}
@@ -179,19 +204,19 @@
name = itemFactory.createString(nextSuggestedName());
} while (!isAvailable(name));
if (markAsUsed) {
- addRenaming(original, name);
+ addRenaming(original, proto, name);
}
return name;
}
- public void addRenaming(DexString original, DexString newName) {
+ void addRenaming(DexString original, InternalProtoType proto, DexString newName) {
if (renamings == null) {
- renamings = HashBiMap.create();
+ renamings = HashBasedTable.create();
}
- renamings.put(original, newName);
+ renamings.put(original, proto, newName);
}
- protected String nextSuggestedName() {
+ String nextSuggestedName() {
if (dictionaryIterator.hasNext()) {
return dictionaryIterator.next();
} else {
diff --git a/src/test/java/com/android/tools/r8/naming/overloadaggressively/ValidNameConflictTest.java b/src/test/java/com/android/tools/r8/naming/overloadaggressively/ValidNameConflictTest.java
index f949aea..a510cee 100644
--- a/src/test/java/com/android/tools/r8/naming/overloadaggressively/ValidNameConflictTest.java
+++ b/src/test/java/com/android/tools/r8/naming/overloadaggressively/ValidNameConflictTest.java
@@ -290,7 +290,6 @@
+ " static <methods>;"
+ "}\n",
keepMainProguardConfiguration(CLASS_NAME),
- "-useuniqueclassmembernames",
"-dontshrink");
AndroidApp app = compileWithR8(builder, pgConfigs, null);
@@ -478,7 +477,6 @@
+ " <methods>;"
+ "}\n",
keepMainProguardConfiguration(CLASS_NAME),
- "-useuniqueclassmembernames",
"-dontshrink");
AndroidApp app = compileWithR8(builder, pgConfigs, null);
@@ -631,15 +629,11 @@
MethodSubject subM2 = sub.method("java.lang.Object", "same", ImmutableList.of());
assertTrue(subM2.isPresent());
assertTrue(subM2.isRenamed());
- // TODO(b/73149686): R8 should be able to fix this conflict w/o -overloadaggressively.
- assertEquals(subM1.getFinalName(), subM2.getFinalName());
+ assertNotEquals(subM1.getFinalName(), subM2.getFinalName());
- // TODO(b/73149686): The current impl of method minifier visits classes in a hierarchical order.
- // Hence, already renamed same names in Super are not resolvable as the internal naming state
- // uses only _name_ to resolve methods in the hierarchy.
- // // No matter what, overloading methods should be renamed to the same name.
- // assertEquals(m1.getFinalName(), subM1.getFinalName());
- // assertEquals(m2.getFinalName(), subM2.getFinalName());
+ // No matter what, overloading methods should be renamed to the same name.
+ assertEquals(m1.getFinalName(), subM1.getFinalName());
+ assertEquals(m2.getFinalName(), subM2.getFinalName());
ProcessResult artOutput = runOnArtRaw(app, CLASS_NAME, dexVm);
assertEquals(0, artOutput.exitCode);
