Handling roots removed after inlining of other roots.
Also adds check for methods w/o normal exit bloks which inliner
refuses to inline into blocks with catch handlers.
Bug: 110963213
Bug: 110736241
Change-Id: Ieececfb300713d3e0b6a6672ca938f7718198d72
diff --git a/src/main/java/com/android/tools/r8/ir/code/BasicBlock.java b/src/main/java/com/android/tools/r8/ir/code/BasicBlock.java
index 819c6f2..5bed061 100644
--- a/src/main/java/com/android/tools/r8/ir/code/BasicBlock.java
+++ b/src/main/java/com/android/tools/r8/ir/code/BasicBlock.java
@@ -659,6 +659,7 @@
for (Instruction instruction : getInstructions()) {
if (instruction.outValue != null) {
instruction.outValue.clearUsers();
+ instruction.setOutValue(null);
}
for (Value value : instruction.inValues) {
value.removeUser(instruction);
diff --git a/src/main/java/com/android/tools/r8/ir/code/BasicBlockInstructionIterator.java b/src/main/java/com/android/tools/r8/ir/code/BasicBlockInstructionIterator.java
index 58b61af..1adcd11 100644
--- a/src/main/java/com/android/tools/r8/ir/code/BasicBlockInstructionIterator.java
+++ b/src/main/java/com/android/tools/r8/ir/code/BasicBlockInstructionIterator.java
@@ -390,7 +390,7 @@
ImmutableList<BasicBlock> normalExits = inlinee.computeNormalExitBlocks();
if (normalExits.isEmpty()) {
assert inlineeCanThrow;
- // TODO(sgjesse): Remove this restriction.
+ // TODO(sgjesse): Remove this restriction (see b/64432527).
assert !invokeBlock.hasCatchHandlers();
blocksToRemove.addAll(
invokePredecessor.unlink(invokeBlock, new DominatorTree(code)));
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 8556905..b3bde90 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
@@ -444,6 +444,12 @@
return null;
}
+ // Don't inline code w/o normal returns into block with catch handlers (b/64432527).
+ if (initInvoke.getBlock().hasCatchHandlers() &&
+ definition.getOptimizationInfo().neverReturnsNormally()) {
+ return null;
+ }
+
// If the superclass of the initializer is NOT java.lang.Object, the super class
// initializer being called must be classified as TrivialInstanceInitializer.
//
@@ -476,16 +482,16 @@
if (invoke.inValues().lastIndexOf(eligibleInstance) > 0) {
return null; // Instance passed as an argument.
}
- return isEligibleMethodCall(invoke.getInvokedMethod(),
+ return isEligibleMethodCall(!invoke.getBlock().hasCatchHandlers(), invoke.getInvokedMethod(),
eligibility -> !eligibility.returnsReceiver ||
invoke.outValue() == null || invoke.outValue().numberOfAllUsers() == 0);
}
private InliningInfo isEligibleIndirectMethodCall(DexMethod callee) {
- return isEligibleMethodCall(callee, eligibility -> !eligibility.returnsReceiver);
+ return isEligibleMethodCall(false, callee, eligibility -> !eligibility.returnsReceiver);
}
- private InliningInfo isEligibleMethodCall(
+ private InliningInfo isEligibleMethodCall(boolean allowMethodsWithoutNormalReturns,
DexMethod callee, Predicate<ClassInlinerEligibility> eligibilityAcceptanceCheck) {
DexEncodedMethod singleTarget = findSingleTarget(callee);
@@ -496,8 +502,9 @@
return null; // Don't inline itself.
}
- ClassInlinerEligibility eligibility =
- singleTarget.getOptimizationInfo().getClassInlinerEligibility();
+ OptimizationInfo optimizationInfo = singleTarget.getOptimizationInfo();
+
+ ClassInlinerEligibility eligibility = optimizationInfo.getClassInlinerEligibility();
if (eligibility == null) {
return null;
}
@@ -508,6 +515,11 @@
return null;
}
+ // Don't inline code w/o normal returns into block with catch handlers (b/64432527).
+ if (!allowMethodsWithoutNormalReturns && optimizationInfo.neverReturnsNormally()) {
+ return null;
+ }
+
if (!singleTarget.isInliningCandidate(method, Reason.SIMPLE, appInfo)) {
// We won't be able to inline it here.
@@ -570,6 +582,11 @@
OptimizationInfo optimizationInfo = singleTarget.getOptimizationInfo();
+ // Don't inline code w/o normal returns into block with catch handlers (b/64432527).
+ if (invokeMethod.getBlock().hasCatchHandlers() && optimizationInfo.neverReturnsNormally()) {
+ return false;
+ }
+
// Go through all arguments, see if all usages of eligibleInstance are good.
for (int argIndex = 0; argIndex < arguments.size(); argIndex++) {
Value argument = arguments.get(argIndex);
diff --git a/src/test/java/com/android/tools/r8/ir/optimize/classinliner/ClassInlinerTest.java b/src/test/java/com/android/tools/r8/ir/optimize/classinliner/ClassInlinerTest.java
index 51a5262..3239191 100644
--- a/src/test/java/com/android/tools/r8/ir/optimize/classinliner/ClassInlinerTest.java
+++ b/src/test/java/com/android/tools/r8/ir/optimize/classinliner/ClassInlinerTest.java
@@ -9,6 +9,7 @@
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertThat;
+import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail;
import com.android.tools.r8.OutputMode;
@@ -27,6 +28,7 @@
import com.android.tools.r8.ir.optimize.classinliner.builders.Tuple;
import com.android.tools.r8.ir.optimize.classinliner.code.C;
import com.android.tools.r8.ir.optimize.classinliner.code.CodeTestClass;
+import com.android.tools.r8.ir.optimize.classinliner.invalidroot.InvalidRootsTestClass;
import com.android.tools.r8.ir.optimize.classinliner.trivial.ClassWithFinal;
import com.android.tools.r8.ir.optimize.classinliner.trivial.CycleReferenceAB;
import com.android.tools.r8.ir.optimize.classinliner.trivial.CycleReferenceBA;
@@ -254,6 +256,51 @@
assertFalse(inspector.clazz(C.F.class).isPresent());
}
+ @Test
+ public void testInvalidatedRoot() throws Exception {
+ String prefix = "com.android.tools.r8.ir.optimize.classinliner.invalidroot.";
+
+ byte[][] classes = {
+ ToolHelper.getClassAsBytes(InvalidRootsTestClass.class),
+ ToolHelper.getClassAsBytes(InvalidRootsTestClass.A.class),
+ ToolHelper.getClassAsBytes(InvalidRootsTestClass.B.class),
+ ToolHelper.getClassAsBytes(InvalidRootsTestClass.NeverReturnsNormally.class),
+ ToolHelper.getClassAsBytes(InvalidRootsTestClass.InitNeverReturnsNormally.class)
+ };
+ AndroidApp app = runR8(buildAndroidApp(classes), InvalidRootsTestClass.class);
+
+ String javaOutput = runOnJava(InvalidRootsTestClass.class);
+ String artOutput = runOnArt(app, InvalidRootsTestClass.class);
+ assertEquals(javaOutput, artOutput);
+
+ DexInspector inspector = new DexInspector(app);
+ ClassSubject clazz = inspector.clazz(InvalidRootsTestClass.class);
+
+ assertEquals(
+ Sets.newHashSet(prefix + "InvalidRootsTestClass$NeverReturnsNormally"),
+ collectTypes(clazz, "testExtraNeverReturnsNormally", "void"));
+
+ assertEquals(
+ Sets.newHashSet(prefix + "InvalidRootsTestClass$NeverReturnsNormally"),
+ collectTypes(clazz, "testDirectNeverReturnsNormally", "void"));
+
+ assertEquals(
+ Sets.newHashSet(prefix + "InvalidRootsTestClass$InitNeverReturnsNormally"),
+ collectTypes(clazz, "testInitNeverReturnsNormally", "void"));
+
+ assertTrue(inspector.clazz(InvalidRootsTestClass.NeverReturnsNormally.class).isPresent());
+ assertTrue(inspector.clazz(InvalidRootsTestClass.InitNeverReturnsNormally.class).isPresent());
+
+ assertEquals(
+ Sets.newHashSet(
+ "java.lang.StringBuilder",
+ "java.lang.RuntimeException"),
+ collectTypes(clazz, "testRootInvalidatesAfterInlining", "void"));
+
+ assertFalse(inspector.clazz(InvalidRootsTestClass.A.class).isPresent());
+ assertFalse(inspector.clazz(InvalidRootsTestClass.B.class).isPresent());
+ }
+
private Set<String> collectTypes(
ClassSubject clazz, String methodName, String retValue, String... params) {
return Stream.concat(
diff --git a/src/test/java/com/android/tools/r8/ir/optimize/classinliner/invalidroot/InvalidRootsTestClass.java b/src/test/java/com/android/tools/r8/ir/optimize/classinliner/invalidroot/InvalidRootsTestClass.java
new file mode 100644
index 0000000..26dc8cc
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/ir/optimize/classinliner/invalidroot/InvalidRootsTestClass.java
@@ -0,0 +1,129 @@
+// 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.classinliner.invalidroot;
+
+public class InvalidRootsTestClass {
+ private static int ID = 0;
+
+ private static String next() {
+ return Integer.toString(ID++);
+ }
+
+ public static void main(String[] args) {
+ InvalidRootsTestClass test = new InvalidRootsTestClass();
+ test.testExtraNeverReturnsNormally();
+ test.testDirectNeverReturnsNormally();
+ test.testInitNeverReturnsNormally();
+ test.testRootInvalidatesAfterInlining();
+ }
+
+ private synchronized void testExtraNeverReturnsNormally() {
+ testExtraNeverReturnsNormallyA();
+ testExtraNeverReturnsNormallyB();
+
+ try {
+ NeverReturnsNormally a = new NeverReturnsNormally();
+ neverReturnsNormallyExtra(next(), a);
+ } catch (RuntimeException re) {
+ System.out.println(re.toString());
+ }
+ }
+
+ private synchronized void testExtraNeverReturnsNormallyA() {
+ try {
+ neverReturnsNormallyExtra(next(), null);
+ } catch (RuntimeException re) {
+ System.out.println(re.toString());
+ }
+ }
+
+ private synchronized void testExtraNeverReturnsNormallyB() {
+ try {
+ neverReturnsNormallyExtra(next(), null);
+ } catch (RuntimeException re) {
+ System.out.println(re.toString());
+ }
+ }
+
+ private synchronized void testDirectNeverReturnsNormally() {
+ try {
+ NeverReturnsNormally a = new NeverReturnsNormally();
+ System.out.println(a.foo());
+ } catch (RuntimeException re) {
+ System.out.println(re.toString());
+ }
+ }
+
+ private synchronized void testInitNeverReturnsNormally() {
+ try {
+ new InitNeverReturnsNormally();
+ } catch (RuntimeException re) {
+ System.out.println(re.toString());
+ }
+ }
+
+ private void neverReturnsNormallyExtra(String prefix, NeverReturnsNormally a) {
+ throw new RuntimeException("neverReturnsNormallyExtra(" +
+ prefix + ", " + (a == null ? "null" : a.foo()) + "): " + next());
+ }
+
+ public static class NeverReturnsNormally {
+ public String foo() {
+ throw new RuntimeException("NeverReturnsNormally::foo(): " + next());
+ }
+ }
+
+ public static class InitNeverReturnsNormally {
+ public InitNeverReturnsNormally() {
+ throw new RuntimeException("InitNeverReturnsNormally::init(): " + next());
+ }
+
+ public String foo() {
+ return "InitNeverReturnsNormally::foo(): " + next();
+ }
+ }
+
+ private synchronized void testRootInvalidatesAfterInlining() {
+ A a = new A();
+ try {
+ notInlinedExtraMethod(next(), a);
+ System.out.println(new B().foo() + " " + next());
+ testRootInvalidatesAfterInliningA(a);
+ testRootInvalidatesAfterInliningB(a);
+ } catch (RuntimeException re) {
+ System.out.println(re.toString());
+ }
+ }
+
+ private void notInlinedExtraMethod(String prefix, A a) {
+ System.out.println("notInlinedExtraMethod(" +
+ prefix + ", " + (a == null ? "null" : a.foo()) + "): " + next());
+ if (a != null) {
+ throw new RuntimeException(
+ "notInlinedExtraMethod(" + prefix + ", " + a.foo() + "): " + next());
+ }
+ System.out.println("notInlinedExtraMethod(" + prefix + ", null): " + next());
+ }
+
+ private void testRootInvalidatesAfterInliningA(A a) {
+ notInlinedExtraMethod(next(), a);
+ }
+
+ private void testRootInvalidatesAfterInliningB(A a) {
+ notInlinedExtraMethod(next(), a);
+ }
+
+ public static class A {
+ public String foo() {
+ return "B::foo(" + next() + ")";
+ }
+ }
+
+ public static class B {
+ public String foo() {
+ return "B::foo(" + next() + ")";
+ }
+ }
+}
diff --git a/src/test/java/com/android/tools/r8/neverreturnsnormally/NeverReturnsNormallyTest.java b/src/test/java/com/android/tools/r8/neverreturnsnormally/NeverReturnsNormallyTest.java
index 3e46098..c929eaf 100644
--- a/src/test/java/com/android/tools/r8/neverreturnsnormally/NeverReturnsNormallyTest.java
+++ b/src/test/java/com/android/tools/r8/neverreturnsnormally/NeverReturnsNormallyTest.java
@@ -22,12 +22,12 @@
import com.google.common.collect.ImmutableList;
import java.util.Iterator;
import java.util.function.BiConsumer;
-import org.junit.Ignore;
import org.junit.Test;
public class NeverReturnsNormallyTest extends TestBase {
private void runTest(
- BiConsumer<DexInspector, CompilationMode> inspection, CompilationMode mode) throws Exception {
+ BiConsumer<DexInspector, CompilationMode> inspection,
+ boolean enableClassInliner, CompilationMode mode) throws Exception {
R8Command.Builder builder = R8Command.builder();
builder.addProgramFiles(ToolHelper.getClassFileForTestClass(TestClass.class));
builder.setProgramConsumer(DexIndexedConsumer.emptyConsumer());
@@ -49,7 +49,8 @@
"-allowaccessmodification"
),
Origin.unknown());
- AndroidApp app = ToolHelper.runR8(builder.build());
+ AndroidApp app = ToolHelper.runR8(builder.build(),
+ opts -> opts.enableClassInlining = enableClassInliner);
inspection.accept(new DexInspector(app), mode);
// Run on Art to check generated code against verifier.
@@ -128,10 +129,11 @@
return instructions.next();
}
- @Ignore("b/110736241")
@Test
public void test() throws Exception {
- runTest(this::validate, CompilationMode.DEBUG);
- runTest(this::validate, CompilationMode.RELEASE);
+ runTest(this::validate, true, CompilationMode.DEBUG);
+ runTest(this::validate, true, CompilationMode.RELEASE);
+ runTest(this::validate, false, CompilationMode.DEBUG);
+ runTest(this::validate, false, CompilationMode.RELEASE);
}
}
