Version 1.3.25
Merge: Add test for array-return-type collision
CL: https://r8-review.googlesource.com/c/r8/+/29345
Merge: Handle array type collisions in vertical class merger
CL: https://r8-review.googlesource.com/c/r8/+/29342
Partial merge: Static, horizontal class merging
CL: https://r8-review.googlesource.com/c/r8/+/28780
The commit "Static, horizontal class merging" contains a fix for the vertical class merger. This CL only merges that fix from https://r8-review.googlesource.com/c/r8/+/28780 (in particular, this merge does not include the horizontal class merger).
Bug: 117399364
Change-Id: If382ec986fdae0e44b2d37b6d509385ddd58bc06
diff --git a/src/main/java/com/android/tools/r8/Version.java b/src/main/java/com/android/tools/r8/Version.java
index 1c1e02a..5ee47e8 100644
--- a/src/main/java/com/android/tools/r8/Version.java
+++ b/src/main/java/com/android/tools/r8/Version.java
@@ -11,7 +11,7 @@
// This field is accessed from release scripts using simple pattern matching.
// Therefore, changing this field could break our release scripts.
- public static final String LABEL = "1.3.24";
+ public static final String LABEL = "1.3.25";
private Version() {
}
diff --git a/src/main/java/com/android/tools/r8/shaking/VerticalClassMerger.java b/src/main/java/com/android/tools/r8/shaking/VerticalClassMerger.java
index 5e03071..ce5a358 100644
--- a/src/main/java/com/android/tools/r8/shaking/VerticalClassMerger.java
+++ b/src/main/java/com/android/tools/r8/shaking/VerticalClassMerger.java
@@ -1495,12 +1495,13 @@
result = 0;
int bitsUsed = 0;
int accumulator = 0;
- for (DexType aType : proto.parameters.values) {
+ for (DexType parameterType : proto.parameters.values) {
+ DexType parameterBaseType = parameterType.toBaseType(appInfo.dexItemFactory);
// Substitute the type with the already merged class to estimate what it will look like.
- aType = mergedClasses.getOrDefault(aType, aType);
+ DexType mappedType = mergedClasses.getOrDefault(parameterBaseType, parameterBaseType);
accumulator <<= 1;
bitsUsed++;
- if (aType == type) {
+ if (mappedType == type) {
accumulator |= 1;
}
// Handle overflow on 31 bit boundary.
@@ -1511,9 +1512,10 @@
}
}
// We also take the return type into account for potential conflicts.
- DexType returnType = mergedClasses.getOrDefault(proto.returnType, proto.returnType);
+ DexType returnBaseType = proto.returnType.toBaseType(appInfo.dexItemFactory);
+ DexType mappedReturnType = mergedClasses.getOrDefault(returnBaseType, returnBaseType);
accumulator <<= 1;
- if (returnType == type) {
+ if (mappedReturnType == type) {
accumulator |= 1;
}
result |= accumulator;
@@ -1628,7 +1630,8 @@
private boolean checkFieldReference(DexField field) {
if (!foundIllegalAccess) {
- if (field.clazz.isSamePackage(source.type)) {
+ DexType baseType = field.clazz.toBaseType(appInfo.dexItemFactory);
+ if (baseType.isClassType() && baseType.isSamePackage(source.type)) {
checkTypeReference(field.clazz);
checkTypeReference(field.type);
@@ -1643,7 +1646,8 @@
private boolean checkMethodReference(DexMethod method) {
if (!foundIllegalAccess) {
- if (method.holder.isSamePackage(source.type)) {
+ DexType baseType = method.holder.toBaseType(appInfo.dexItemFactory);
+ if (baseType.isClassType() && baseType.isSamePackage(source.type)) {
checkTypeReference(method.holder);
checkTypeReference(method.proto.returnType);
for (DexType type : method.proto.parameters.values) {
@@ -1660,8 +1664,9 @@
private boolean checkTypeReference(DexType type) {
if (!foundIllegalAccess) {
- if (type.isClassType() && type.isSamePackage(source.type)) {
- DexClass clazz = appInfo.definitionFor(type);
+ DexType baseType = type.toBaseType(appInfo.dexItemFactory);
+ if (baseType.isClassType() && baseType.isSamePackage(source.type)) {
+ DexClass clazz = appInfo.definitionFor(baseType);
if (clazz == null || !clazz.accessFlags.isPublic()) {
foundIllegalAccess = true;
}
diff --git a/src/test/examples/classmerging/ArrayTypeCollisionTest.java b/src/test/examples/classmerging/ArrayTypeCollisionTest.java
new file mode 100644
index 0000000..9e4c9fd
--- /dev/null
+++ b/src/test/examples/classmerging/ArrayTypeCollisionTest.java
@@ -0,0 +1,26 @@
+// Copyright (c) 2018, 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 classmerging;
+
+public class ArrayTypeCollisionTest {
+
+ public static void main(String[] args) {
+ method(new A[] {});
+ method(new B[] {});
+ }
+
+ private static void method(A[] obj) {
+ System.out.println("In method(A[])");
+ }
+
+ private static void method(B[] obj) {
+ System.out.println("In method(B[])");
+ }
+
+ // A cannot be merged into B because that would lead to a collision.
+ public static class A {}
+
+ public static class B extends A {}
+}
diff --git a/src/test/examples/classmerging/keep-rules.txt b/src/test/examples/classmerging/keep-rules.txt
index 97be613..e19eb97 100644
--- a/src/test/examples/classmerging/keep-rules.txt
+++ b/src/test/examples/classmerging/keep-rules.txt
@@ -7,6 +7,9 @@
-keep public class classmerging.Test {
public static void main(...);
}
+-keep public class classmerging.ArrayTypeCollisionTest {
+ public static void main(...);
+}
-keep public class classmerging.CallGraphCycleTest {
public static void main(...);
}
diff --git a/src/test/java/com/android/tools/r8/classmerging/ClassMergingTest.java b/src/test/java/com/android/tools/r8/classmerging/ClassMergingTest.java
index 7a1f831..1ac4d8e 100644
--- a/src/test/java/com/android/tools/r8/classmerging/ClassMergingTest.java
+++ b/src/test/java/com/android/tools/r8/classmerging/ClassMergingTest.java
@@ -37,6 +37,7 @@
import com.android.tools.r8.utils.AndroidApiLevel;
import com.android.tools.r8.utils.AndroidApp;
import com.android.tools.r8.utils.InternalOptions;
+import com.android.tools.r8.utils.StringUtils;
import com.android.tools.r8.utils.codeinspector.ClassSubject;
import com.android.tools.r8.utils.codeinspector.CodeInspector;
import com.android.tools.r8.utils.codeinspector.FoundClassSubject;
@@ -126,6 +127,92 @@
}
}
+ @Test
+ public void testArrayTypeCollision() throws Throwable {
+ String main = "classmerging.ArrayTypeCollisionTest";
+ Path[] programFiles =
+ new Path[] {
+ CF_DIR.resolve("ArrayTypeCollisionTest.class"),
+ CF_DIR.resolve("ArrayTypeCollisionTest$A.class"),
+ CF_DIR.resolve("ArrayTypeCollisionTest$B.class")
+ };
+ Set<String> preservedClassNames =
+ ImmutableSet.of(
+ "classmerging.ArrayTypeCollisionTest",
+ "classmerging.ArrayTypeCollisionTest$A",
+ "classmerging.ArrayTypeCollisionTest$B");
+ runTest(
+ main,
+ programFiles,
+ preservedClassNames::contains,
+ getProguardConfig(
+ EXAMPLE_KEEP,
+ "-neverinline public class classmerging.ArrayTypeCollisionTest {",
+ " static void method(...);",
+ "}"));
+ }
+
+ /**
+ * Tests that two classes A and B are not merged when there are two methods in the same class that
+ * returns an array type and would become conflicting due to the renaming of their return types,
+ * as in the following class.
+ *
+ * <pre>
+ * class ArrayReturnTypeCollisionTest {
+ * public static A[] method() { ... }
+ * public static B[] method() { ... }
+ * }
+ * </pre>
+ */
+ @Test
+ public void testArrayReturnTypeCollision() throws Throwable {
+ String main = "classmerging.ArrayReturnTypeCollisionTest";
+ Set<String> preservedClassNames =
+ ImmutableSet.of(
+ "classmerging.ArrayReturnTypeCollisionTest", "classmerging.A", "classmerging.B");
+
+ JasminBuilder jasminBuilder = new JasminBuilder();
+
+ ClassBuilder classBuilder = jasminBuilder.addClass(main);
+ classBuilder.addMainMethod(
+ ".limit locals 1",
+ ".limit stack 2",
+ "invokestatic classmerging/ArrayReturnTypeCollisionTest/method()[Lclassmerging/A;",
+ "pop",
+ "invokestatic classmerging/ArrayReturnTypeCollisionTest/method()[Lclassmerging/B;",
+ "pop",
+ "return");
+
+ // Add two methods with the same name that have return types A[] and B[], respectively.
+ classBuilder.addStaticMethod(
+ "method", ImmutableList.of(), "[Lclassmerging/A;",
+ ".limit stack 1", ".limit locals 1", "iconst_0", "anewarray classmerging/A", "areturn");
+ classBuilder.addStaticMethod(
+ "method", ImmutableList.of(), "[Lclassmerging/B;",
+ ".limit stack 1", ".limit locals 1", "iconst_0", "anewarray classmerging/B", "areturn");
+
+ // Class A is empty so that it can easily be merged into B.
+ classBuilder = jasminBuilder.addClass("classmerging.A");
+ classBuilder.addDefaultConstructor();
+
+ // Class B inherits from A and is also empty.
+ classBuilder = jasminBuilder.addClass("classmerging.B", "classmerging.A");
+ classBuilder.addDefaultConstructor();
+
+ // Run test.
+ runTestOnInput(
+ main,
+ jasminBuilder.build(),
+ preservedClassNames::contains,
+ StringUtils.joinLines(
+ "-keep class " + main + " {",
+ " public static void main(...);",
+ "}",
+ "-neverinline class " + main + " {",
+ " static void method(...);",
+ "}"));
+ }
+
// This test has a cycle in the call graph consisting of the methods A.<init> and B.<init>.
// When nondeterministicCycleElimination is enabled, we shuffle the nodes in the call graph
// before the cycle elimination. Therefore, it is nondeterministic if the cycle detector will
