Fixing inliningConstraintForVirtualInvoke for cases when no targets were found
Apparently there is something wrong with lookupTargets(...) called on
interface invocation. None of testInterface* methods in added tests are
inlined because we failed to find targets. Even such straightforward one
as testInterfaceD.
Note that not finding targets for interface invocations is not a big
problem, since in valid code all interface method implementations are
required to be public, so the check for targets being accessible (below)
should always succeed unless the input is erroneous.
Bug:
Change-Id: Ia5fcac3aa026739c73ff8fa1c4deac19c0ae7df0
diff --git a/src/main/java/com/android/tools/r8/ir/code/InvokeMethod.java b/src/main/java/com/android/tools/r8/ir/code/InvokeMethod.java
index 44fb6b0..5fe8972 100644
--- a/src/main/java/com/android/tools/r8/ir/code/InvokeMethod.java
+++ b/src/main/java/com/android/tools/r8/ir/code/InvokeMethod.java
@@ -116,7 +116,7 @@
return Constraint.ALWAYS;
}
Collection<DexEncodedMethod> targets = lookupTargets(info, invocationContext);
- if (targets == null || targets.isEmpty()) {
+ if (targets == null) {
return Constraint.NEVER;
}
diff --git a/src/test/java/com/android/tools/r8/ir/optimize/inliner/InlinerTest.java b/src/test/java/com/android/tools/r8/ir/optimize/inliner/InlinerTest.java
new file mode 100644
index 0000000..adb87e6
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/ir/optimize/inliner/InlinerTest.java
@@ -0,0 +1,117 @@
+// 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 com.android.tools.r8.ir.optimize.inliner;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.fail;
+
+import com.android.tools.r8.OutputMode;
+import com.android.tools.r8.TestBase;
+import com.android.tools.r8.ToolHelper;
+import com.android.tools.r8.ToolHelper.ProcessResult;
+import com.android.tools.r8.VmTestRunner;
+import com.android.tools.r8.ir.optimize.inliner.interfaces.InterfaceTargetsTestClass;
+import com.android.tools.r8.ir.optimize.inliner.interfaces.InterfaceTargetsTestClass.IfaceA;
+import com.android.tools.r8.ir.optimize.inliner.interfaces.InterfaceTargetsTestClass.IfaceB;
+import com.android.tools.r8.ir.optimize.inliner.interfaces.InterfaceTargetsTestClass.IfaceC;
+import com.android.tools.r8.ir.optimize.inliner.interfaces.InterfaceTargetsTestClass.IfaceD;
+import com.android.tools.r8.ir.optimize.inliner.interfaces.InterfaceTargetsTestClass.IfaceNoImpl;
+import com.android.tools.r8.naming.MemberNaming.MethodSignature;
+import com.android.tools.r8.utils.AndroidApp;
+import com.android.tools.r8.utils.DexInspector;
+import com.android.tools.r8.utils.DexInspector.ClassSubject;
+import com.android.tools.r8.utils.DexInspector.MethodSubject;
+import com.android.tools.r8.utils.InternalOptions;
+import java.nio.file.Path;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+
+@RunWith(VmTestRunner.class)
+public class InlinerTest extends TestBase {
+ @Test
+ public void testInterfacesWithoutTargets() throws Exception {
+ byte[][] classes = {
+ ToolHelper.getClassAsBytes(InterfaceTargetsTestClass.class),
+ ToolHelper.getClassAsBytes(InterfaceTargetsTestClass.IfaceNoImpl.class),
+ ToolHelper.getClassAsBytes(InterfaceTargetsTestClass.IfaceA.class),
+ ToolHelper.getClassAsBytes(InterfaceTargetsTestClass.BaseA.class),
+ ToolHelper.getClassAsBytes(InterfaceTargetsTestClass.DerivedA.class),
+ ToolHelper.getClassAsBytes(InterfaceTargetsTestClass.IfaceB.class),
+ ToolHelper.getClassAsBytes(InterfaceTargetsTestClass.BaseB.class),
+ ToolHelper.getClassAsBytes(InterfaceTargetsTestClass.DerivedB.class),
+ ToolHelper.getClassAsBytes(InterfaceTargetsTestClass.IfaceC.class),
+ ToolHelper.getClassAsBytes(InterfaceTargetsTestClass.IfaceC2.class),
+ ToolHelper.getClassAsBytes(InterfaceTargetsTestClass.BaseC.class),
+ ToolHelper.getClassAsBytes(InterfaceTargetsTestClass.DerivedC.class),
+ ToolHelper.getClassAsBytes(InterfaceTargetsTestClass.IfaceD.class),
+ ToolHelper.getClassAsBytes(InterfaceTargetsTestClass.BaseD.class)
+ };
+ AndroidApp app = runR8(buildAndroidApp(classes), InterfaceTargetsTestClass.class);
+
+ String javaOutput = runOnJava(InterfaceTargetsTestClass.class);
+ String artOutput = runOnArt(app, InterfaceTargetsTestClass.class);
+ assertEquals(javaOutput, artOutput);
+
+ DexInspector inspector = new DexInspector(app);
+ ClassSubject clazz = inspector.clazz(InterfaceTargetsTestClass.class);
+
+ assertFalse(getMethodSubject(clazz,
+ "testInterfaceNoImpl", String.class, IfaceNoImpl.class).isPresent());
+ assertFalse(getMethodSubject(clazz,
+ "testInterfaceA", String.class, IfaceA.class).isPresent());
+ assertFalse(getMethodSubject(clazz,
+ "testInterfaceB", String.class, IfaceB.class).isPresent());
+ assertFalse(getMethodSubject(clazz,
+ "testInterfaceD", String.class, IfaceC.class).isPresent());
+ assertFalse(getMethodSubject(clazz,
+ "testInterfaceD", String.class, IfaceD.class).isPresent());
+ }
+
+ private MethodSubject getMethodSubject(
+ ClassSubject clazz, String methodName, Class retValue, Class... params) {
+ return clazz.method(new MethodSignature(methodName, retValue.getTypeName(),
+ Stream.of(params).map(Class::getTypeName).collect(Collectors.toList())));
+ }
+
+ private AndroidApp runR8(AndroidApp app, Class mainClass) throws Exception {
+ AndroidApp compiled =
+ compileWithR8(app, getProguardConfig(mainClass.getCanonicalName()), this::configure);
+
+ // Materialize file for execution.
+ Path generatedDexFile = temp.getRoot().toPath().resolve("classes.jar");
+ compiled.writeToZip(generatedDexFile, OutputMode.DexIndexed);
+
+ // Run with ART.
+ String artOutput = ToolHelper.runArtNoVerificationErrors(
+ generatedDexFile.toString(), mainClass.getCanonicalName());
+
+ // Compare with Java.
+ ProcessResult javaResult = ToolHelper.runJava(
+ ToolHelper.getClassPathForTests(), mainClass.getCanonicalName());
+
+ if (javaResult.exitCode != 0) {
+ System.out.println(javaResult.stdout);
+ System.err.println(javaResult.stderr);
+ fail("JVM failed for: " + mainClass);
+ }
+ assertEquals("JVM and ART output differ", javaResult.stdout, artOutput);
+
+ return compiled;
+ }
+
+ private String getProguardConfig(String main) {
+ return keepMainProguardConfiguration(main)
+ + "\n"
+ + "-dontobfuscate\n"
+ + "-allowaccessmodification";
+ }
+
+ private void configure(InternalOptions options) {
+ options.enableClassInlining = false;
+ }
+}
diff --git a/src/test/java/com/android/tools/r8/ir/optimize/inliner/interfaces/InterfaceTargetsTestClass.java b/src/test/java/com/android/tools/r8/ir/optimize/inliner/interfaces/InterfaceTargetsTestClass.java
new file mode 100644
index 0000000..d132833
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/ir/optimize/inliner/interfaces/InterfaceTargetsTestClass.java
@@ -0,0 +1,102 @@
+// 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 com.android.tools.r8.ir.optimize.inliner.interfaces;
+
+public class InterfaceTargetsTestClass {
+ public static void main(String[] args) {
+ System.out.println(testInterfaceNoImpl(null));
+ System.out.println(testInterfaceA(null));
+ System.out.println(testInterfaceB(null));
+ System.out.println(testInterfaceC(null));
+ System.out.println(testInterfaceD(null));
+ }
+
+ public interface IfaceNoImpl {
+ void foo();
+ }
+
+ public static String testInterfaceNoImpl(IfaceNoImpl iface) {
+ if (iface != null) {
+ iface.foo();
+ }
+ return "testInterfaceNoImpl::OK";
+ }
+
+ public interface IfaceA {
+ void foo();
+ }
+
+ public static class BaseA {
+ public void foo() {
+ }
+ }
+
+ public static class DerivedA extends BaseA implements IfaceA {
+ }
+
+ public static String testInterfaceA(IfaceA iface) {
+ if (iface != null) {
+ iface.foo();
+ }
+ return "testInterfaceA::OK";
+ }
+
+ public interface IfaceB {
+ void foo();
+ }
+
+ public abstract static class BaseB implements IfaceB {
+ }
+
+ public static class DerivedB extends BaseB {
+ public void foo() {
+ }
+ }
+
+ public static String testInterfaceB(IfaceB iface) {
+ if (iface != null) {
+ iface.foo();
+ }
+ return "testInterfaceB::OK";
+ }
+
+ public interface IfaceC {
+ void foo();
+ }
+
+ public interface IfaceC2 extends IfaceC {
+ default void foo() {
+ }
+ }
+
+ public abstract static class BaseC implements IfaceC {
+ }
+
+ public static class DerivedC extends BaseC implements IfaceC2 {
+ }
+
+ public static String testInterfaceC(IfaceC iface) {
+ if (iface != null) {
+ iface.foo();
+ }
+ return "testInterfaceC::OK";
+ }
+
+ public interface IfaceD {
+ void foo();
+ }
+
+ public static class BaseD implements IfaceD {
+ public void foo() {
+ }
+ }
+
+ public static String testInterfaceD(IfaceD iface) {
+ if (iface != null) {
+ iface.foo();
+ }
+ return "testInterfaceD::OK";
+ }
+}
