Assert that definitionFor is only called on non-null class types.
Change-Id: Ia95759020fd18310a2970ff054d271f6c4ba1274
diff --git a/src/main/java/com/android/tools/r8/graph/AppInfoWithSubtyping.java b/src/main/java/com/android/tools/r8/graph/AppInfoWithSubtyping.java
index a45064e..e415b0f 100644
--- a/src/main/java/com/android/tools/r8/graph/AppInfoWithSubtyping.java
+++ b/src/main/java/com/android/tools/r8/graph/AppInfoWithSubtyping.java
@@ -119,8 +119,10 @@
// For mapping invoke virtual instruction to target methods.
public Set<DexEncodedMethod> lookupVirtualTargets(DexMethod method) {
- Set<DexEncodedMethod> result = new HashSet<>();
- // First add the target for receiver type method.type.
+ if (method.holder.isArrayType()) {
+ assert method.name == dexItemFactory.cloneMethodName;
+ return null;
+ }
DexClass root = definitionFor(method.holder);
if (root == null) {
// type specified in method does not have a materialized class.
@@ -131,6 +133,8 @@
// This will fail at runtime.
return null;
}
+ // First add the target for receiver type method.type.
+ Set<DexEncodedMethod> result = new HashSet<>();
topTargets.forEachTarget(result::add);
// Add all matching targets from the subclass hierarchy.
for (DexType type : subtypes(method.holder)) {
diff --git a/src/main/java/com/android/tools/r8/graph/DirectMappedDexApplication.java b/src/main/java/com/android/tools/r8/graph/DirectMappedDexApplication.java
index 012c4a4..f070078 100644
--- a/src/main/java/com/android/tools/r8/graph/DirectMappedDexApplication.java
+++ b/src/main/java/com/android/tools/r8/graph/DirectMappedDexApplication.java
@@ -45,6 +45,7 @@
@Override
public DexClass definitionFor(DexType type) {
+ assert type.isClassType() : "Cannot lookup definition for type: " + type;
DexClass result = programClasses.get(type);
if (result == null) {
result = libraryClasses.get(type);
diff --git a/src/main/java/com/android/tools/r8/graph/LazyLoadedDexApplication.java b/src/main/java/com/android/tools/r8/graph/LazyLoadedDexApplication.java
index 6dea825..b236428 100644
--- a/src/main/java/com/android/tools/r8/graph/LazyLoadedDexApplication.java
+++ b/src/main/java/com/android/tools/r8/graph/LazyLoadedDexApplication.java
@@ -42,9 +42,7 @@
@Override
public DexClass definitionFor(DexType type) {
- if (type == null) {
- return null;
- }
+ assert type.isClassType() : "Cannot lookup definition for type: " + type;
DexClass clazz = programClasses.get(type);
if (clazz == null && classpathClasses != null) {
clazz = classpathClasses.get(type);
diff --git a/src/main/java/com/android/tools/r8/ir/analysis/TypeChecker.java b/src/main/java/com/android/tools/r8/ir/analysis/TypeChecker.java
index 53a6245..75a9726 100644
--- a/src/main/java/com/android/tools/r8/ir/analysis/TypeChecker.java
+++ b/src/main/java/com/android/tools/r8/ir/analysis/TypeChecker.java
@@ -78,7 +78,7 @@
return true;
}
- if (valueType.isReference()) {
+ if (fieldType.isClassType() && valueType.isReference()) {
// Interface types are treated like Object according to the JVM spec.
// https://docs.oracle.com/javase/specs/jvms/se9/html/jvms-4.html#jvms-4.10.1.2-100
DexClass clazz = appInfo.definitionFor(instruction.getField().type);
diff --git a/src/main/java/com/android/tools/r8/ir/code/ConstClass.java b/src/main/java/com/android/tools/r8/ir/code/ConstClass.java
index bd57f29..7610f9c 100644
--- a/src/main/java/com/android/tools/r8/ir/code/ConstClass.java
+++ b/src/main/java/com/android/tools/r8/ir/code/ConstClass.java
@@ -91,6 +91,9 @@
// A const-class instruction can be dead code only if the resulting program is known to contain
// the class mentioned.
DexType baseType = clazz.toBaseType(appInfo.dexItemFactory);
+ if (baseType.isPrimitiveType()) {
+ return true;
+ }
DexClass holder = appInfo.definitionFor(baseType);
return holder != null && holder.isProgramClass();
}
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/CodeRewriter.java b/src/main/java/com/android/tools/r8/ir/optimize/CodeRewriter.java
index 3227d45..3091c36 100644
--- a/src/main/java/com/android/tools/r8/ir/optimize/CodeRewriter.java
+++ b/src/main/java/com/android/tools/r8/ir/optimize/CodeRewriter.java
@@ -2147,6 +2147,9 @@
private boolean isTypeInaccessibleInCurrentContext(DexType type, DexEncodedMethod context) {
DexType baseType = type.toBaseType(appInfo.dexItemFactory);
+ if (baseType.isPrimitiveType()) {
+ return false;
+ }
DexClass clazz = definitionFor(baseType);
if (clazz == null) {
// Conservatively say yes.
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/classinliner/InlineCandidateProcessor.java b/src/main/java/com/android/tools/r8/ir/optimize/classinliner/InlineCandidateProcessor.java
index 7375303..cdf37c4 100644
--- a/src/main/java/com/android/tools/r8/ir/optimize/classinliner/InlineCandidateProcessor.java
+++ b/src/main/java/com/android/tools/r8/ir/optimize/classinliner/InlineCandidateProcessor.java
@@ -113,6 +113,9 @@
eligibleClass =
root.isNewInstance() ? root.asNewInstance().clazz : root.asStaticGet().getField().type;
+ if (!eligibleClass.isClassType()) {
+ return false;
+ }
eligibleClassDefinition = appInfo.definitionFor(eligibleClass);
if (eligibleClassDefinition == null && lambdaRewriter != null) {
// Check if the class is synthesized for a desugared lambda
diff --git a/src/main/java/com/android/tools/r8/shaking/Enqueuer.java b/src/main/java/com/android/tools/r8/shaking/Enqueuer.java
index adc6979..8560ba7 100644
--- a/src/main/java/com/android/tools/r8/shaking/Enqueuer.java
+++ b/src/main/java/com/android/tools/r8/shaking/Enqueuer.java
@@ -2414,6 +2414,10 @@
// https://docs.oracle.com/javase/specs/jvms/se9/html/jvms-6.html#jvms-6.5.invokevirtual
assert method != null;
assert refinedReceiverType.isSubtypeOf(method.holder, this);
+ if (method.holder.isArrayType()) {
+ assert method.name == dexItemFactory.cloneMethodName;
+ return null;
+ }
DexClass holder = definitionFor(method.holder);
if (holder == null || holder.isLibraryClass() || holder.isInterface()) {
return null;
diff --git a/src/main/java/com/android/tools/r8/shaking/MainDexDirectReferenceTracer.java b/src/main/java/com/android/tools/r8/shaking/MainDexDirectReferenceTracer.java
index 543dcc4..71b3d53 100644
--- a/src/main/java/com/android/tools/r8/shaking/MainDexDirectReferenceTracer.java
+++ b/src/main/java/com/android/tools/r8/shaking/MainDexDirectReferenceTracer.java
@@ -19,7 +19,6 @@
import com.android.tools.r8.graph.DexString;
import com.android.tools.r8.graph.DexType;
import com.android.tools.r8.graph.UseRegistry;
-import com.google.common.collect.ImmutableSet;
import java.util.Set;
import java.util.function.Consumer;
@@ -61,35 +60,23 @@
boolean value = false;
}
- public static boolean hasReferencesOutside(
- AppInfoWithSubtyping appInfo, DexProgramClass clazz, Set<DexType> types) {
- BooleanBox result = new BooleanBox();
-
- new MainDexDirectReferenceTracer(appInfo, type -> {
- if (!types.contains(type)) {
- DexClass cls = appInfo.definitionFor(type);
- if (cls != null && !cls.isLibraryClass()) {
- result.value = true;
- }
- }
- }).run(ImmutableSet.of(clazz.type));
-
- return result.value;
- }
-
public static boolean hasReferencesOutsideFromCode(
AppInfoWithSubtyping appInfo, DexEncodedMethod method, Set<DexType> classes) {
BooleanBox result = new BooleanBox();
- new MainDexDirectReferenceTracer(appInfo, type -> {
- if (!classes.contains(type)) {
- DexClass cls = appInfo.definitionFor(type);
- if (cls != null && !cls.isLibraryClass()) {
- result.value = true;
- }
- }
- }).runOnCode(method);
+ new MainDexDirectReferenceTracer(
+ appInfo,
+ type -> {
+ DexType baseType = type.toBaseType(appInfo.dexItemFactory);
+ if (baseType.isClassType() && !classes.contains(baseType)) {
+ DexClass cls = appInfo.definitionFor(baseType);
+ if (cls != null && !cls.isLibraryClass()) {
+ result.value = true;
+ }
+ }
+ })
+ .runOnCode(method);
return result.value;
}
diff --git a/src/main/java/com/android/tools/r8/shaking/RootSetBuilder.java b/src/main/java/com/android/tools/r8/shaking/RootSetBuilder.java
index 898d06c..ff196e3 100644
--- a/src/main/java/com/android/tools/r8/shaking/RootSetBuilder.java
+++ b/src/main/java/com/android/tools/r8/shaking/RootSetBuilder.java
@@ -843,6 +843,9 @@
}
private void includeDescriptor(DexDefinition item, DexType type, ProguardKeepRule context) {
+ if (type.isVoidType()) {
+ return;
+ }
if (type.isArrayType()) {
type = type.toBaseType(application.dexItemFactory);
}
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 b9f7554..2d8392b 100644
--- a/src/main/java/com/android/tools/r8/shaking/VerticalClassMerger.java
+++ b/src/main/java/com/android/tools/r8/shaking/VerticalClassMerger.java
@@ -336,15 +336,16 @@
}
private void markTypeAsPinned(DexType type, AbortReason reason) {
- if (appInfo.isPinned(type)) {
+ DexType baseType = type.toBaseType(appInfo.dexItemFactory);
+ if (!baseType.isClassType() || appInfo.isPinned(baseType)) {
// We check for the case where the type is pinned according to appInfo.isPinned,
// so we only need to add it here if it is not the case.
return;
}
- DexClass clazz = appInfo.definitionFor(type);
+ DexClass clazz = appInfo.definitionFor(baseType);
if (clazz != null && clazz.isProgramClass()) {
- boolean changed = pinnedTypes.add(type);
+ boolean changed = pinnedTypes.add(baseType);
if (Log.ENABLED) {
if (changed && isMergeCandidate(clazz.asProgramClass(), ImmutableSet.of())) {
@@ -742,10 +743,6 @@
return false;
}
- private boolean hasReferencesOutside(DexProgramClass clazz, Set<DexType> types) {
- return MainDexDirectReferenceTracer.hasReferencesOutside(appInfo, clazz, types);
- }
-
private void mergeClassIfPossible(DexProgramClass clazz) {
if (!mergeCandidates.contains(clazz)) {
return;
diff --git a/src/test/examples/classmerging/PinnedArrayParameterTypesTest.java b/src/test/examples/classmerging/PinnedArrayParameterTypesTest.java
new file mode 100644
index 0000000..4a8b5be
--- /dev/null
+++ b/src/test/examples/classmerging/PinnedArrayParameterTypesTest.java
@@ -0,0 +1,47 @@
+// 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 classmerging;
+
+import java.lang.reflect.Method;
+
+public class PinnedArrayParameterTypesTest {
+
+ public static void main(String[] args) throws Exception {
+ for (Method method : TestClass.class.getMethods()) {
+ if (method.getName().equals("method")) {
+ Class<?> parameterType = method.getParameterTypes()[0];
+
+ // Should print classmerging.PinnedArrayParameterTypesTest$Interface when
+ // -keepparameternames is used.
+ System.out.println(parameterType.getName());
+
+ method.invoke(null, new Object[]{ new InterfaceImpl[]{ new InterfaceImpl() } });
+ break;
+ }
+ }
+ }
+
+ public interface Interface {
+
+ void foo();
+ }
+
+ public static class InterfaceImpl implements Interface {
+
+ @Override
+ public void foo() {
+ System.out.println("In InterfaceImpl.foo()");
+ }
+ }
+
+ public static class TestClass {
+
+ // This method has been kept explicitly by a keep rule. Therefore, since -keepparameternames is
+ // used, Interface must not be merged into InterfaceImpl.
+ public static void method(Interface[] obj) {
+ obj[0].foo();
+ }
+ }
+}
diff --git a/src/test/examples/classmerging/keep-rules.txt b/src/test/examples/classmerging/keep-rules.txt
index e3e0006..c75058d 100644
--- a/src/test/examples/classmerging/keep-rules.txt
+++ b/src/test/examples/classmerging/keep-rules.txt
@@ -43,6 +43,12 @@
-keep public class classmerging.PinnedParameterTypesTest$TestClass {
public static void method(...);
}
+-keep public class classmerging.PinnedArrayParameterTypesTest {
+ public static void main(...);
+}
+-keep public class classmerging.PinnedArrayParameterTypesTest$TestClass {
+ public static void method(...);
+}
-keep public class classmerging.ProguardFieldMapTest {
public static void main(...);
}
diff --git a/src/test/java/com/android/tools/r8/D8TestBuilder.java b/src/test/java/com/android/tools/r8/D8TestBuilder.java
index 88d077b..37e4dec 100644
--- a/src/test/java/com/android/tools/r8/D8TestBuilder.java
+++ b/src/test/java/com/android/tools/r8/D8TestBuilder.java
@@ -3,13 +3,18 @@
// BSD-style license that can be found in the LICENSE file.
package com.android.tools.r8;
+import com.android.tools.r8.ClassFileConsumer.ArchiveConsumer;
import com.android.tools.r8.D8Command.Builder;
import com.android.tools.r8.TestBase.Backend;
import com.android.tools.r8.utils.AndroidApp;
+import com.android.tools.r8.utils.DescriptorUtils;
import com.android.tools.r8.utils.InternalOptions;
+import java.io.IOException;
import java.nio.file.Path;
+import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
+import java.util.List;
import java.util.function.Consumer;
import java.util.function.Supplier;
@@ -17,6 +22,9 @@
extends TestCompilerBuilder<
D8Command, Builder, D8TestCompileResult, D8TestRunResult, D8TestBuilder> {
+ // Consider an in-order collection of both class and files on the classpath.
+ private List<Class<?>> classpathClasses = new ArrayList<>();
+
private D8TestBuilder(TestState state, Builder builder) {
super(state, builder, Backend.DEX);
}
@@ -34,6 +42,29 @@
D8TestCompileResult internalCompile(
Builder builder, Consumer<InternalOptions> optionsConsumer, Supplier<AndroidApp> app)
throws CompilationFailedException {
+ if (!classpathClasses.isEmpty()) {
+ Path cp;
+ try {
+ cp = getState().getNewTempFolder().resolve("cp.jar");
+ } catch (IOException e) {
+ throw builder.getReporter().fatalError("Failed to create temp file for classpath archive");
+ }
+ ArchiveConsumer archiveConsumer = new ArchiveConsumer(cp);
+ for (Class<?> classpathClass : classpathClasses) {
+ try {
+ archiveConsumer.accept(
+ ByteDataView.of(ToolHelper.getClassAsBytes(classpathClass)),
+ DescriptorUtils.javaTypeToDescriptor(classpathClass.getTypeName()),
+ builder.getReporter());
+ } catch (IOException e) {
+ builder
+ .getReporter()
+ .error("Failed to read bytes for classpath class: " + classpathClass.getTypeName());
+ }
+ }
+ archiveConsumer.finished(builder.getReporter());
+ builder.addClasspathFiles(cp);
+ }
ToolHelper.runD8(builder, optionsConsumer);
return new D8TestCompileResult(getState(), app.get());
}
@@ -43,7 +74,8 @@
}
public D8TestBuilder addClasspathClasses(Collection<Class<?>> classes) {
- return addClasspathFiles(getFilesForClasses(classes));
+ classpathClasses.addAll(classes);
+ return self();
}
public D8TestBuilder addClasspathFiles(Path... files) {
diff --git a/src/test/java/com/android/tools/r8/RegressionForPrimitiveDefinitionForLookup.java b/src/test/java/com/android/tools/r8/RegressionForPrimitiveDefinitionForLookup.java
new file mode 100644
index 0000000..b43bb8b
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/RegressionForPrimitiveDefinitionForLookup.java
@@ -0,0 +1,43 @@
+// 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;
+
+import com.android.tools.r8.utils.StringUtils;
+import org.junit.Test;
+
+class Tester {
+ public int foo() {
+ float[][][] fs = new float[1][2][3];
+ return fs.length;
+ }
+
+ public static void main(String[] args) {
+ System.out.println(new Tester().foo());
+ }
+}
+
+// The DirectoryClasspathProvider asserts lookups are reference types which witnessed the issue.
+public class RegressionForPrimitiveDefinitionForLookup extends TestBase {
+
+ public final Class<Tester> CLASS = Tester.class;
+ public String EXPECTED = StringUtils.lines("1");
+
+ @Test
+ public void testWithArchiveClasspath() throws Exception {
+ testForD8()
+ .addClasspathClasses(CLASS)
+ .addProgramClasses(CLASS)
+ .run(CLASS)
+ .assertSuccessWithOutput(EXPECTED);
+ }
+
+ @Test
+ public void testWithDirectoryClasspath() throws Exception {
+ testForD8()
+ .addClasspathFiles(ToolHelper.getClassPathForTests())
+ .addProgramClasses(CLASS)
+ .run(CLASS)
+ .assertSuccessWithOutput(EXPECTED);
+ }
+}
diff --git a/src/test/java/com/android/tools/r8/classmerging/VerticalClassMergerTest.java b/src/test/java/com/android/tools/r8/classmerging/VerticalClassMergerTest.java
index 2284fa5..bb89443 100644
--- a/src/test/java/com/android/tools/r8/classmerging/VerticalClassMergerTest.java
+++ b/src/test/java/com/android/tools/r8/classmerging/VerticalClassMergerTest.java
@@ -466,6 +466,29 @@
}
@Test
+ public void testPinnedArrayParameterTypes() throws Throwable {
+ String main = "classmerging.PinnedArrayParameterTypesTest";
+ Path[] programFiles =
+ new Path[] {
+ CF_DIR.resolve("PinnedArrayParameterTypesTest.class"),
+ CF_DIR.resolve("PinnedArrayParameterTypesTest$Interface.class"),
+ CF_DIR.resolve("PinnedArrayParameterTypesTest$InterfaceImpl.class"),
+ CF_DIR.resolve("PinnedArrayParameterTypesTest$TestClass.class")
+ };
+ Set<String> preservedClassNames =
+ ImmutableSet.of(
+ "classmerging.PinnedArrayParameterTypesTest",
+ "classmerging.PinnedArrayParameterTypesTest$Interface",
+ "classmerging.PinnedArrayParameterTypesTest$InterfaceImpl",
+ "classmerging.PinnedArrayParameterTypesTest$TestClass");
+ runTest(
+ main,
+ programFiles,
+ preservedClassNames::contains,
+ getProguardConfig(EXAMPLE_KEEP, "-keepparameternames"));
+ }
+
+ @Test
public void testProguardFieldMap() throws Throwable {
String main = "classmerging.ProguardFieldMapTest";
Path[] programFiles =
