Fix collisions between fields
Bug: 169394640
Bug: 163311975
Change-Id: Ica076f10a001597c90d569511acefee8de5a689e
diff --git a/src/main/java/com/android/tools/r8/graph/DexField.java b/src/main/java/com/android/tools/r8/graph/DexField.java
index 43c065a..012030b 100644
--- a/src/main/java/com/android/tools/r8/graph/DexField.java
+++ b/src/main/java/com/android/tools/r8/graph/DexField.java
@@ -169,6 +169,10 @@
return dexItemFactory.createField(holder, type, name);
}
+ public DexField withName(DexString name, DexItemFactory dexItemFactory) {
+ return dexItemFactory.createField(holder, type, name);
+ }
+
public FieldReference asFieldReference() {
return Reference.field(
Reference.classFromDescriptor(holder.toDescriptorString()),
diff --git a/src/main/java/com/android/tools/r8/graph/DexItemFactory.java b/src/main/java/com/android/tools/r8/graph/DexItemFactory.java
index cdf97ab..cf30eed 100644
--- a/src/main/java/com/android/tools/r8/graph/DexItemFactory.java
+++ b/src/main/java/com/android/tools/r8/graph/DexItemFactory.java
@@ -1799,7 +1799,7 @@
* @param tryString callback to check if the method name is in use.
*/
public <T extends DexMember<?, ?>> T createFreshMember(
- String baseName, DexType holder, Function<DexString, Optional<T>> tryString) {
+ Function<DexString, Optional<T>> tryString, String baseName, DexType holder) {
int index = 0;
while (true) {
DexString name = createString(createMemberString(baseName, holder, index++));
@@ -1811,6 +1811,17 @@
}
/**
+ * Find a fresh method name that is not used by any other method. The method name takes the form
+ * "basename" or "basename$index".
+ *
+ * @param tryString callback to check if the method name is in use.
+ */
+ public <T extends DexMember<?, ?>> T createFreshMember(
+ Function<DexString, Optional<T>> tryString, String baseName) {
+ return createFreshMember(tryString, baseName, null);
+ }
+
+ /**
* Find a fresh method name that is not in the string pool. The name takes the form
* "basename$holdername" or "basename$holdername$index".
*/
@@ -1850,8 +1861,6 @@
Predicate<DexMethod> isFresh) {
DexMethod method =
createFreshMember(
- baseName,
- holder,
name -> {
DexMethod tryMethod = createMethod(target, proto, name);
if (isFresh.test(tryMethod)) {
@@ -1859,7 +1868,9 @@
} else {
return Optional.empty();
}
- });
+ },
+ baseName,
+ holder);
return method;
}
diff --git a/src/main/java/com/android/tools/r8/horizontalclassmerging/TreeFixer.java b/src/main/java/com/android/tools/r8/horizontalclassmerging/TreeFixer.java
index e33137c..7f12cc9 100644
--- a/src/main/java/com/android/tools/r8/horizontalclassmerging/TreeFixer.java
+++ b/src/main/java/com/android/tools/r8/horizontalclassmerging/TreeFixer.java
@@ -32,6 +32,7 @@
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Map;
+import java.util.Optional;
import java.util.Set;
import java.util.function.Consumer;
@@ -384,10 +385,26 @@
if (fields == null) {
return;
}
+ Set<DexField> existingFields = Sets.newIdentityHashSet();
+
for (int i = 0; i < fields.size(); i++) {
DexEncodedField encodedField = fields.get(i);
DexField field = encodedField.field;
DexField newField = fixupFieldReference(field);
+
+ // Rename the field if it already exists.
+ if (!existingFields.add(newField)) {
+ DexField template = newField;
+ newField =
+ dexItemFactory.createFreshMember(
+ tryName ->
+ Optional.of(template.withName(tryName, dexItemFactory))
+ .filter(tryMethod -> !existingFields.contains(tryMethod)),
+ newField.name.toSourceString());
+ boolean added = existingFields.add(newField);
+ assert added;
+ }
+
if (newField != encodedField.field) {
lensBuilder.mapField(field, newField);
setter.setField(i, encodedField.toTypeSubstitutedField(newField));
diff --git a/src/test/java/com/android/tools/r8/classmerging/horizontal/MergingProducesFieldCollisionTest.java b/src/test/java/com/android/tools/r8/classmerging/horizontal/MergingProducesFieldCollisionTest.java
new file mode 100644
index 0000000..d25d516
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/classmerging/horizontal/MergingProducesFieldCollisionTest.java
@@ -0,0 +1,110 @@
+// Copyright (c) 2020, 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.classmerging.horizontal;
+
+import static com.android.tools.r8.utils.codeinspector.Matchers.isPresent;
+import static com.android.tools.r8.utils.codeinspector.Matchers.notIf;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.junit.Assert.assertEquals;
+
+import com.android.tools.r8.NeverClassInline;
+import com.android.tools.r8.NeverInline;
+import com.android.tools.r8.NoVerticalClassMerging;
+import com.android.tools.r8.TestParameters;
+import com.android.tools.r8.utils.codeinspector.ClassSubject;
+import org.junit.Test;
+
+public class MergingProducesFieldCollisionTest extends HorizontalClassMergingTestBase {
+ public MergingProducesFieldCollisionTest(
+ TestParameters parameters, boolean enableHorizontalClassMerging) {
+ super(parameters, enableHorizontalClassMerging);
+ }
+
+ @Test
+ public void testR8() throws Exception {
+ byte[] transformedC = transformer(C.class).renameAndRemapField("v$1", "v").transform();
+
+ testForR8(parameters.getBackend())
+ .addKeepMainRule(Main.class)
+ .addProgramClassFileData(transformedC)
+ .addProgramClasses(Parent.class, A.class, B.class, Main.class)
+ .addOptionsModification(
+ options -> options.enableHorizontalClassMerging = enableHorizontalClassMerging)
+ .enableInliningAnnotations()
+ .enableNeverClassInliningAnnotations()
+ .setMinApi(parameters.getApiLevel())
+ .run(parameters.getRuntime(), Main.class)
+ .assertSuccess()
+ .inspect(
+ codeInspector -> {
+ assertThat(codeInspector.clazz(Parent.class), isPresent());
+
+ ClassSubject aClassSubject = codeInspector.clazz(A.class);
+ assertThat(aClassSubject, isPresent());
+
+ ClassSubject bClassSubject = codeInspector.clazz(B.class);
+ assertThat(bClassSubject, notIf(isPresent(), enableHorizontalClassMerging));
+
+ ClassSubject cClassSubject = codeInspector.clazz(C.class);
+ assertThat(cClassSubject, isPresent());
+
+ if (enableHorizontalClassMerging) {
+ assertEquals(
+ cClassSubject.allFields().get(0).type(),
+ cClassSubject.allFields().get(1).type());
+ }
+ });
+ }
+
+ @NoVerticalClassMerging
+ public static class Parent {
+ @NeverInline
+ public void foo() {
+ System.out.println("foo");
+ }
+ }
+
+ @NeverClassInline
+ public static class A extends Parent {
+ public A() {
+ System.out.println("b");
+ }
+ }
+
+ @NeverClassInline
+ public static class B extends Parent {
+ public B() {
+ System.out.println("b");
+ }
+
+ @NeverInline
+ public void foo() {
+ System.out.println("foo b");
+ }
+ }
+
+ @NeverClassInline
+ public static class C {
+ A v;
+ B v$1; // This field is renamed to v.
+
+ public C(A a, B b) {
+ v = a;
+ v$1 = b;
+ }
+
+ @NeverInline
+ public void foo() {
+ v.foo();
+ v$1.foo();
+ }
+ }
+
+ public static class Main {
+ public static void main(String[] args) {
+ new C(new A(), new B()).foo();
+ }
+ }
+}
diff --git a/src/test/java/com/android/tools/r8/transformers/ClassFileTransformer.java b/src/test/java/com/android/tools/r8/transformers/ClassFileTransformer.java
index 4779b8e..d9f3b1d 100644
--- a/src/test/java/com/android/tools/r8/transformers/ClassFileTransformer.java
+++ b/src/test/java/com/android/tools/r8/transformers/ClassFileTransformer.java
@@ -474,6 +474,11 @@
boolean test(int access, String name, String descriptor, String signature, Object value);
}
+ @FunctionalInterface
+ public interface FieldSignaturePredicate {
+ boolean test(String name, String typeDescriptor);
+ }
+
public ClassFileTransformer removeInnerClasses() {
return addClassTransformer(
new ClassTransformer() {
@@ -522,6 +527,42 @@
});
}
+ public ClassFileTransformer remapField(FieldSignaturePredicate predicate, String newName) {
+ return addMethodTransformer(
+ new MethodTransformer() {
+ @Override
+ public void visitFieldInsn(
+ final int opcode, final String owner, final String name, final String descriptor) {
+ if (predicate.test(name, descriptor)) {
+ super.visitFieldInsn(opcode, owner, newName, descriptor);
+ } else {
+ super.visitFieldInsn(opcode, owner, name, descriptor);
+ }
+ }
+ });
+ }
+
+ public ClassFileTransformer renameField(FieldSignaturePredicate predicate, String newName) {
+ return addClassTransformer(
+ new ClassTransformer() {
+ @Override
+ public FieldVisitor visitField(
+ int access, String name, String descriptor, String signature, Object value) {
+ if (predicate.test(name, descriptor)) {
+ return super.visitField(access, newName, descriptor, signature, value);
+ } else {
+ return super.visitField(access, name, descriptor, signature, value);
+ }
+ }
+ });
+ }
+
+ public ClassFileTransformer renameAndRemapField(String oldName, String newName) {
+ FieldSignaturePredicate matchPredicate = (name, signature) -> oldName.equals(name);
+ remapField(matchPredicate, newName);
+ return renameField(matchPredicate, newName);
+ }
+
/** Abstraction of the MethodVisitor.visitMethodInsn method with its sub visitor. */
@FunctionalInterface
public interface MethodInsnTransform {
