Resolve field name conflicts in aggressively overloaded input.
MemberMinifier has two-layered lookup:
1) from proto (either field type or method signature) to internal state,
2) inside internal state, original name to newly assigned (re)name.
-overloadaggressively tweak what to use as a key at the 1st layer.
If enabled, field type is always mapped to void, and thus fields with
different type can be named to the same rename.
(For methods, return type is ignored.)
-useuniqueclassmembernames determines whether to remember name mapping
inside the internal state, i.e., entry in the 2nd layer.
------
This CL clearly settled the renaming behavior regarding the 2nd layer.
With -useuniqueclassmembernames, we should keep renaming mappings in an
internal state. On the other hand, if it's not set, by not maintaining
such renaming, we can assign different names to already aggressively
overloaded fields.
------
Method renaming has a different story, since overloading methods in
subtypes should use the same renamed names. Will handle that in a
separate CL.
Bug: 73149686
Change-Id: I79ce71b4da485bdb4e5523e60f149e39e3c1d6a9
diff --git a/src/main/java/com/android/tools/r8/naming/FieldNameMinifier.java b/src/main/java/com/android/tools/r8/naming/FieldNameMinifier.java
index e517da9..53416c6 100644
--- a/src/main/java/com/android/tools/r8/naming/FieldNameMinifier.java
+++ b/src/main/java/com/android/tools/r8/naming/FieldNameMinifier.java
@@ -9,7 +9,6 @@
import com.android.tools.r8.graph.DexField;
import com.android.tools.r8.graph.DexString;
import com.android.tools.r8.graph.DexType;
-import com.android.tools.r8.shaking.ProguardConfiguration;
import com.android.tools.r8.shaking.RootSetBuilder.RootSet;
import com.android.tools.r8.utils.InternalOptions;
import com.android.tools.r8.utils.Timing;
@@ -23,8 +22,8 @@
}
@Override
- Function<DexType, ?> getKeyTransform(ProguardConfiguration config) {
- if (config.isOverloadAggressively()) {
+ Function<DexType, ?> getKeyTransform() {
+ if (overloadAggressively) {
// Use the type as the key, hence reuse names per type.
return a -> a;
} else {
@@ -88,9 +87,9 @@
private void renameField(DexEncodedField encodedField, NamingState<DexType, ?> state) {
DexField field = encodedField.field;
if (!state.isReserved(field.name, field.type)) {
- DexString candidate = state.assignNewNameFor(field.name, field.type, false);
- renaming.put(field, candidate);
- state.addRenaming(field.name, field.type, candidate);
+ renaming.put(
+ field,
+ state.assignNewNameFor(field.name, field.type, useUniqueMemberNames));
}
}
}
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 4a288be..1fa8aaa 100644
--- a/src/main/java/com/android/tools/r8/naming/MemberNameMinifier.java
+++ b/src/main/java/com/android/tools/r8/naming/MemberNameMinifier.java
@@ -7,7 +7,6 @@
import com.android.tools.r8.graph.CachedHashValueDexItem;
import com.android.tools.r8.graph.DexString;
import com.android.tools.r8.graph.DexType;
-import com.android.tools.r8.shaking.ProguardConfiguration;
import com.android.tools.r8.shaking.RootSetBuilder.RootSet;
import com.android.tools.r8.utils.InternalOptions;
import com.google.common.collect.ImmutableList;
@@ -25,17 +24,19 @@
protected final Map<DexType, NamingState<StateType, ?>> states = new IdentityHashMap<>();
protected final NamingState<StateType, ?> globalState;
protected final boolean useUniqueMemberNames;
+ protected final boolean overloadAggressively;
MemberNameMinifier(AppInfoWithSubtyping appInfo, RootSet rootSet, InternalOptions options) {
this.appInfo = appInfo;
this.rootSet = rootSet;
this.dictionary = options.proguardConfiguration.getObfuscationDictionary();
- this.globalState = NamingState.createRoot(appInfo.dexItemFactory, dictionary,
- getKeyTransform(options.proguardConfiguration));
this.useUniqueMemberNames = options.proguardConfiguration.isUseUniqueClassMemberNames();
+ this.overloadAggressively = options.proguardConfiguration.isOverloadAggressively();
+ this.globalState =
+ NamingState.createRoot(appInfo.dexItemFactory, dictionary, getKeyTransform());
}
- abstract Function<StateType, ?> getKeyTransform(ProguardConfiguration config);
+ abstract Function<StateType, ?> getKeyTransform();
protected NamingState<StateType, ?> computeStateIfAbsent(
DexType type, Function<DexType, NamingState<StateType, ?>> f) {
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 7b939ae..936a06a 100644
--- a/src/main/java/com/android/tools/r8/naming/MethodNameMinifier.java
+++ b/src/main/java/com/android/tools/r8/naming/MethodNameMinifier.java
@@ -10,7 +10,6 @@
import com.android.tools.r8.graph.DexProto;
import com.android.tools.r8.graph.DexString;
import com.android.tools.r8.graph.DexType;
-import com.android.tools.r8.shaking.ProguardConfiguration;
import com.android.tools.r8.shaking.RootSetBuilder.RootSet;
import com.android.tools.r8.utils.InternalOptions;
import com.android.tools.r8.utils.MethodJavaSignatureEquivalence;
@@ -90,19 +89,17 @@
class MethodNameMinifier extends MemberNameMinifier<DexMethod, DexProto> {
private final Equivalence<DexMethod> equivalence;
- private final ProguardConfiguration config;
MethodNameMinifier(AppInfoWithSubtyping appInfo, RootSet rootSet, InternalOptions options) {
super(appInfo, rootSet, options);
- this.config = options.proguardConfiguration;
- equivalence = config.isOverloadAggressively()
+ equivalence = overloadAggressively
? MethodSignatureEquivalence.get()
: MethodJavaSignatureEquivalence.get();
}
@Override
- Function<DexProto, ?> getKeyTransform(ProguardConfiguration config) {
- if (config.isOverloadAggressively()) {
+ Function<DexProto, ?> getKeyTransform() {
+ if (overloadAggressively) {
// Use the full proto as key, hence reuse names based on full signature.
return a -> a;
} else {
@@ -334,8 +331,7 @@
computeStateIfAbsent(
libraryFrontier,
ignore -> parent == null
- ? NamingState
- .createRoot(appInfo.dexItemFactory, dictionary, getKeyTransform(config))
+ ? NamingState.createRoot(appInfo.dexItemFactory, dictionary, getKeyTransform())
: parent.createChild());
if (holder != null) {
boolean keepAll = holder.isLibraryClass() || holder.accessFlags.isAnnotation();
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 365ae83..8fb2622 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
@@ -90,7 +90,7 @@
}
@Test
- public void remainFieldNameConflictDueToKeepRules() throws Exception {
+ public void remainFieldNameConflict_keepRules() throws Exception {
Assume.assumeTrue(ToolHelper.artSupported());
JasminBuilder builder = buildFieldNameConflictClassFile();
ProcessResult javaOutput = runOnJavaNoVerifyRaw(builder, CLASS_NAME);
@@ -123,7 +123,7 @@
@Test
- public void remainFieldNameConflictWithUseUniqueClassMemberNames() throws Exception {
+ public void remainFieldNameConflict_useuniqueclassmembernames() throws Exception {
Assume.assumeTrue(ToolHelper.artSupported());
JasminBuilder builder = buildFieldNameConflictClassFile();
ProcessResult javaOutput = runOnJavaNoVerifyRaw(builder, CLASS_NAME);
@@ -152,7 +152,38 @@
}
@Test
- public void resolveFieldNameConflictWithoutAnyOption() throws Exception {
+ public void remainFieldNameConflict_useuniqueclassmembernames_overloadaggressively()
+ throws Exception {
+ Assume.assumeTrue(ToolHelper.artSupported());
+ JasminBuilder builder = buildFieldNameConflictClassFile();
+ ProcessResult javaOutput = runOnJavaNoVerifyRaw(builder, CLASS_NAME);
+ assertEquals(0, javaOutput.exitCode);
+
+ List<String> pgConfigs = ImmutableList.of(
+ keepMainProguardConfiguration(CLASS_NAME),
+ "-useuniqueclassmembernames",
+ "-overloadaggressively", // no-op
+ "-dontshrink");
+ AndroidApp app = compileWithR8(builder, pgConfigs, null);
+
+ DexInspector dexInspector = new DexInspector(app);
+ ClassSubject clazz = dexInspector.clazz(CLASS_NAME);
+ assertTrue(clazz.isPresent());
+ FieldSubject f1 = clazz.field("java.lang.String", "same");
+ assertTrue(f1.isPresent());
+ assertTrue(f1.isRenamed());
+ FieldSubject f2 = clazz.field("java.lang.Object", "same");
+ assertTrue(f2.isPresent());
+ assertTrue(f2.isRenamed());
+ assertEquals(f1.getField().field.name, f2.getField().field.name);
+
+ ProcessResult artOutput = runOnArtRaw(app, CLASS_NAME, dexVm);
+ assertEquals(0, artOutput.exitCode);
+ assertEquals(javaOutput.stdout, artOutput.stdout);
+ }
+
+ @Test
+ public void resolveFieldNameConflict_no_options() throws Exception {
Assume.assumeTrue(ToolHelper.artSupported());
JasminBuilder builder = buildFieldNameConflictClassFile();
ProcessResult javaOutput = runOnJavaNoVerifyRaw(builder, CLASS_NAME);
@@ -172,8 +203,7 @@
FieldSubject f2 = clazz.field("java.lang.Object", "same");
assertTrue(f2.isPresent());
assertTrue(f2.isRenamed());
- // TODO(b/73149686): R8 should be able to fix this conflict w/o -overloadaggressively.
- assertEquals(f1.getField().field.name, f2.getField().field.name);
+ assertNotEquals(f1.getField().field.name, f2.getField().field.name);
ProcessResult artOutput = runOnArtRaw(app, CLASS_NAME, dexVm);
assertEquals(0, artOutput.exitCode);
@@ -181,7 +211,7 @@
}
@Test
- public void resolveFieldNameConflictEvenWithOverloadAggressively() throws Exception {
+ public void remainFieldNameConflict_overloadaggressively() throws Exception {
Assume.assumeTrue(ToolHelper.artSupported());
JasminBuilder builder = buildFieldNameConflictClassFile();
ProcessResult javaOutput = runOnJavaNoVerifyRaw(builder, CLASS_NAME);
@@ -202,7 +232,6 @@
FieldSubject f2 = clazz.field("java.lang.Object", "same");
assertTrue(f2.isPresent());
assertTrue(f2.isRenamed());
- // TODO(b/72858955): R8 should resolve this field name conflict.
assertEquals(f1.getField().field.name, f2.getField().field.name);
ProcessResult artOutput = runOnArtRaw(app, CLASS_NAME, dexVm);
@@ -255,7 +284,7 @@
}
@Test
- public void remainMethodNameConflictDueToKeepRules() throws Exception {
+ public void remainMethodNameConflict_keepRules() throws Exception {
Assume.assumeTrue(ToolHelper.artSupported());
JasminBuilder builder = buildMethodNameConflictClassFile();
ProcessResult javaOutput = runOnJavaNoVerifyRaw(builder, CLASS_NAME);
@@ -287,7 +316,7 @@
}
@Test
- public void remainMethodNameConflictWithUseUniqueClassMemberNames() throws Exception {
+ public void remainMethodNameConflict_useuniqueclassmembernames() throws Exception {
Assume.assumeTrue(ToolHelper.artSupported());
JasminBuilder builder = buildMethodNameConflictClassFile();
ProcessResult javaOutput = runOnJavaNoVerifyRaw(builder, CLASS_NAME);
@@ -316,7 +345,39 @@
}
@Test
- public void resolveMethodNameConflictWithoutAnyOption() throws Exception {
+ public void remainMethodNameConflict_useuniqueclassmembernames_overloadaggressively()
+ throws Exception {
+ Assume.assumeTrue(ToolHelper.artSupported());
+ JasminBuilder builder = buildMethodNameConflictClassFile();
+ ProcessResult javaOutput = runOnJavaNoVerifyRaw(builder, CLASS_NAME);
+ assertEquals(0, javaOutput.exitCode);
+
+ List<String> pgConfigs = ImmutableList.of(
+ keepMainProguardConfiguration(CLASS_NAME),
+ "-useuniqueclassmembernames",
+ "-overloadaggressively", // no-op
+ "-dontshrink");
+ AndroidApp app = compileWithR8(builder, pgConfigs, null);
+
+ DexInspector dexInspector = new DexInspector(app);
+ ClassSubject clazz = dexInspector.clazz(ANOTHER_CLASS);
+ assertTrue(clazz.isPresent());
+ MethodSubject m1 = clazz.method("java.lang.String", "same", ImmutableList.of());
+ assertTrue(m1.isPresent());
+ assertTrue(m1.isRenamed());
+ MethodSubject m2 = clazz.method("java.lang.Object", "same", ImmutableList.of());
+ assertTrue(m2.isPresent());
+ assertTrue(m2.isRenamed());
+ assertEquals(m1.getMethod().method.name, m2.getMethod().method.name);
+
+ ProcessResult artOutput = runOnArtRaw(app, CLASS_NAME, dexVm);
+ assertEquals(0, artOutput.exitCode);
+ assertEquals(javaOutput.stdout, artOutput.stdout);
+ }
+
+
+ @Test
+ public void resolveMethodNameConflict_no_options() throws Exception {
Assume.assumeTrue(ToolHelper.artSupported());
JasminBuilder builder = buildMethodNameConflictClassFile();
ProcessResult javaOutput = runOnJavaNoVerifyRaw(builder, CLASS_NAME);
@@ -345,7 +406,7 @@
}
@Test
- public void resolveMethodNameConflictEvenWithOverloadAggressively() throws Exception {
+ public void remainMethodNameConflict_overloadaggressively() throws Exception {
Assume.assumeTrue(ToolHelper.artSupported());
JasminBuilder builder = buildMethodNameConflictClassFile();
ProcessResult javaOutput = runOnJavaNoVerifyRaw(builder, CLASS_NAME);
@@ -366,7 +427,6 @@
MethodSubject m2 = clazz.method("java.lang.Object", "same", ImmutableList.of());
assertTrue(m2.isPresent());
assertTrue(m2.isRenamed());
- // TODO(b/73149686): R8 should be able to fix this conflict even w/ -overloadaggressively.
assertEquals(m1.getMethod().method.name, m2.getMethod().method.name);
ProcessResult artOutput = runOnArtRaw(app, CLASS_NAME, dexVm);
