Enable constructor inlining for CF
This has been valid since b/166738818 was fixed by properly tracking the
"this" value to all non-normal exits in instance initializers.
This CL simplifies the regression test after manually verifying that it
still fails if the above mentioned fix is manually reverted.
Fixes: b/136458109
Change-Id: I77997cfe84729ecdba37f7d1a578c741ef8c2010
diff --git a/src/main/java/com/android/tools/r8/cf/code/CfFrameVerifier.java b/src/main/java/com/android/tools/r8/cf/code/CfFrameVerifier.java
index f16d699..9bb9a8a 100644
--- a/src/main/java/com/android/tools/r8/cf/code/CfFrameVerifier.java
+++ b/src/main/java/com/android/tools/r8/cf/code/CfFrameVerifier.java
@@ -306,9 +306,7 @@
: CfFrameState.error("No destination frame");
}
- @SuppressWarnings("UnusedVariable")
private TraversalContinuation<CfCodeDiagnostics, CfFrameState> computeInitialState() {
- DexItemFactory dexItemFactory = appView.dexItemFactory();
CfFrameState state = new ConcreteCfFrameState();
int localIndex = 0;
DexMethod context = previousMethod.orElse(method.getReference());
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/DefaultInliningOracle.java b/src/main/java/com/android/tools/r8/ir/optimize/DefaultInliningOracle.java
index c6bd2a2..af606fb 100644
--- a/src/main/java/com/android/tools/r8/ir/optimize/DefaultInliningOracle.java
+++ b/src/main/java/com/android/tools/r8/ir/optimize/DefaultInliningOracle.java
@@ -144,13 +144,6 @@
return false;
}
- // We don't inline into constructors when producing class files since this can mess up
- // the stackmap, see b/136250031
- if (method.getDefinition().isInstanceInitializer() && options.isGeneratingClassFiles()) {
- whyAreYouNotInliningReporter.reportNoInliningIntoConstructorsWhenGeneratingClassFiles();
- return false;
- }
-
if (method.isStructurallyEqualTo(singleTarget)) {
// Cannot handle recursive inlining at this point.
// Force inlined method should never be recursive.
diff --git a/src/test/java/com/android/tools/r8/classmerging/horizontal/ConstructorCantInlineTest.java b/src/test/java/com/android/tools/r8/classmerging/horizontal/ConstructorCantInlineTest.java
index 3e84ae4..f2db164 100644
--- a/src/test/java/com/android/tools/r8/classmerging/horizontal/ConstructorCantInlineTest.java
+++ b/src/test/java/com/android/tools/r8/classmerging/horizontal/ConstructorCantInlineTest.java
@@ -6,7 +6,6 @@
import static com.android.tools.r8.utils.codeinspector.Matchers.isAbsent;
import static com.android.tools.r8.utils.codeinspector.Matchers.isPresent;
-import static com.android.tools.r8.utils.codeinspector.Matchers.isPresentIf;
import static org.hamcrest.MatcherAssert.assertThat;
import com.android.tools.r8.NeverClassInline;
@@ -34,7 +33,7 @@
.inspect(
codeInspector -> {
assertThat(codeInspector.clazz(A.class), isAbsent());
- assertThat(codeInspector.clazz(B.class), isPresentIf(parameters.isCfRuntime()));
+ assertThat(codeInspector.clazz(B.class), isAbsent());
assertThat(codeInspector.clazz(C.class), isPresent());
assertThat(codeInspector.clazz(D.class), isAbsent());
});
diff --git a/src/test/java/com/android/tools/r8/classmerging/horizontal/ConstructorMergingOverlapTest.java b/src/test/java/com/android/tools/r8/classmerging/horizontal/ConstructorMergingOverlapTest.java
index e6e1d11..7dee4de 100644
--- a/src/test/java/com/android/tools/r8/classmerging/horizontal/ConstructorMergingOverlapTest.java
+++ b/src/test/java/com/android/tools/r8/classmerging/horizontal/ConstructorMergingOverlapTest.java
@@ -45,8 +45,7 @@
assertThat(firstInitSubject, isPresent());
assertThat(firstInitSubject, writesInstanceField(classIdFieldSubject.getDexField()));
- MethodSubject otherInitSubject =
- aClassSubject.init("int", parameters.isCfRuntime() ? "java.lang.Object" : "int");
+ MethodSubject otherInitSubject = aClassSubject.init("int", "int");
assertThat(otherInitSubject, isPresent());
assertThat(otherInitSubject, writesInstanceField(classIdFieldSubject.getDexField()));
diff --git a/src/test/java/com/android/tools/r8/classmerging/horizontal/ConstructorMergingTrivialOverlapTest.java b/src/test/java/com/android/tools/r8/classmerging/horizontal/ConstructorMergingTrivialOverlapTest.java
index ecb49d6..2be25d3 100644
--- a/src/test/java/com/android/tools/r8/classmerging/horizontal/ConstructorMergingTrivialOverlapTest.java
+++ b/src/test/java/com/android/tools/r8/classmerging/horizontal/ConstructorMergingTrivialOverlapTest.java
@@ -45,8 +45,7 @@
assertThat(firstInitSubject, isPresent());
assertThat(firstInitSubject, writesInstanceField(classIdFieldSubject.getDexField()));
- MethodSubject otherInitSubject =
- aClassSubject.init("int", parameters.isCfRuntime() ? "java.lang.Object" : "int");
+ MethodSubject otherInitSubject = aClassSubject.init("int", "int");
assertThat(otherInitSubject, isPresent());
assertThat(otherInitSubject, writesInstanceField(classIdFieldSubject.getDexField()));
diff --git a/src/test/java/com/android/tools/r8/classmerging/horizontal/EquivalentConstructorsWithClassIdAndDifferentArgumentAndAssignmentOrderMergingTest.java b/src/test/java/com/android/tools/r8/classmerging/horizontal/EquivalentConstructorsWithClassIdAndDifferentArgumentAndAssignmentOrderMergingTest.java
index c8b807f..2ad2931 100644
--- a/src/test/java/com/android/tools/r8/classmerging/horizontal/EquivalentConstructorsWithClassIdAndDifferentArgumentAndAssignmentOrderMergingTest.java
+++ b/src/test/java/com/android/tools/r8/classmerging/horizontal/EquivalentConstructorsWithClassIdAndDifferentArgumentAndAssignmentOrderMergingTest.java
@@ -53,10 +53,8 @@
ClassSubject aClassSubject = inspector.clazz(A.class);
assertThat(aClassSubject, isPresent());
// TODO(b/189296638): Enable constructor merging by changing the constructor
- // arguments.
assertEquals(
- parameters.isCfRuntime() ? 4 : 2,
- aClassSubject.allMethods(FoundMethodSubject::isInstanceInitializer).size());
+ 2, aClassSubject.allMethods(FoundMethodSubject::isInstanceInitializer).size());
})
.run(parameters.getRuntime(), Main.class)
.assertSuccessWithOutputLines("CD", "CD");
diff --git a/src/test/java/com/android/tools/r8/classmerging/horizontal/EquivalentConstructorsWithClassIdAndDifferentArgumentOrderMergingTest.java b/src/test/java/com/android/tools/r8/classmerging/horizontal/EquivalentConstructorsWithClassIdAndDifferentArgumentOrderMergingTest.java
index 19ed979..8c5633c 100644
--- a/src/test/java/com/android/tools/r8/classmerging/horizontal/EquivalentConstructorsWithClassIdAndDifferentArgumentOrderMergingTest.java
+++ b/src/test/java/com/android/tools/r8/classmerging/horizontal/EquivalentConstructorsWithClassIdAndDifferentArgumentOrderMergingTest.java
@@ -52,12 +52,9 @@
inspector -> {
ClassSubject aClassSubject = inspector.clazz(A.class);
assertThat(aClassSubject, isPresent());
-
// TODO(b/189296638): Enable constructor merging by changing the constructor
- // arguments.
assertEquals(
- parameters.isCfRuntime() ? 4 : 2,
- aClassSubject.allMethods(FoundMethodSubject::isInstanceInitializer).size());
+ 2, aClassSubject.allMethods(FoundMethodSubject::isInstanceInitializer).size());
})
.run(parameters.getRuntime(), Main.class)
.assertSuccessWithOutputLines("C0", "D1");
diff --git a/src/test/java/com/android/tools/r8/classmerging/horizontal/EquivalentConstructorsWithClassIdAndExtraNullsMergingTest.java b/src/test/java/com/android/tools/r8/classmerging/horizontal/EquivalentConstructorsWithClassIdAndExtraNullsMergingTest.java
index fa03ace..1d995fb 100644
--- a/src/test/java/com/android/tools/r8/classmerging/horizontal/EquivalentConstructorsWithClassIdAndExtraNullsMergingTest.java
+++ b/src/test/java/com/android/tools/r8/classmerging/horizontal/EquivalentConstructorsWithClassIdAndExtraNullsMergingTest.java
@@ -5,7 +5,6 @@
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.isPresentIf;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.junit.Assert.assertEquals;
@@ -54,13 +53,9 @@
ClassSubject aClassSubject = inspector.clazz(A.class);
assertThat(aClassSubject, isPresent());
assertEquals(
- parameters.isCfRuntime() ? 3 : 2,
- aClassSubject.allMethods(FoundMethodSubject::isInstanceInitializer).size());
+ 2, aClassSubject.allMethods(FoundMethodSubject::isInstanceInitializer).size());
assertThat(aClassSubject.init("java.lang.Object", "int"), isPresent());
assertThat(aClassSubject.init("java.lang.Object", "int", "int"), isPresent());
- assertThat(
- aClassSubject.init("java.lang.Object", "int", "java.lang.Object"),
- isPresentIf(parameters.isCfRuntime()));
})
.run(parameters.getRuntime(), Main.class)
.assertSuccessWithOutputLines("C", "0", "C", "D");
diff --git a/src/test/java/com/android/tools/r8/dagger/DaggerBasicSingletonUsingBindsTest.java b/src/test/java/com/android/tools/r8/dagger/DaggerBasicSingletonUsingBindsTest.java
index 0fc4708..627ad79 100644
--- a/src/test/java/com/android/tools/r8/dagger/DaggerBasicSingletonUsingBindsTest.java
+++ b/src/test/java/com/android/tools/r8/dagger/DaggerBasicSingletonUsingBindsTest.java
@@ -80,9 +80,6 @@
"basic.MainUsingBinds",
"dagger.internal.DoubleCheck",
"javax.inject.Provider");
- if (parameters.isCfRuntime()) {
- expectedClasses.add("basic.DaggerMainComponentUsingBinds");
- }
if (target.equals("1.8") || parameters.isDexRuntime()) {
expectedClasses.add("basic.DaggerMainComponentUsingBinds$Builder");
}
diff --git a/src/test/java/com/android/tools/r8/dagger/DaggerBasicSingletonUsingProvidesTest.java b/src/test/java/com/android/tools/r8/dagger/DaggerBasicSingletonUsingProvidesTest.java
index 16117c6..5c06a52 100644
--- a/src/test/java/com/android/tools/r8/dagger/DaggerBasicSingletonUsingProvidesTest.java
+++ b/src/test/java/com/android/tools/r8/dagger/DaggerBasicSingletonUsingProvidesTest.java
@@ -60,9 +60,6 @@
"basic.MainUsingProvides",
"dagger.internal.DoubleCheck",
"javax.inject.Provider");
- if (parameters.isCfRuntime()) {
- expectedClasses.add("basic.DaggerMainComponentUsingProvides");
- }
if (target.equals("1.8") || parameters.isDexRuntime()) {
expectedClasses.add("basic.DaggerMainComponentUsingProvides$Builder");
}
diff --git a/src/test/java/com/android/tools/r8/desugar/nestaccesscontrol/NestAttributesInDexShrinkingFieldsTest.java b/src/test/java/com/android/tools/r8/desugar/nestaccesscontrol/NestAttributesInDexShrinkingFieldsTest.java
index 121a0f3..4a1e82c 100644
--- a/src/test/java/com/android/tools/r8/desugar/nestaccesscontrol/NestAttributesInDexShrinkingFieldsTest.java
+++ b/src/test/java/com/android/tools/r8/desugar/nestaccesscontrol/NestAttributesInDexShrinkingFieldsTest.java
@@ -89,11 +89,7 @@
.setMinApi(parameters)
.addOptionsModification(options -> options.emitNestAnnotationsInDex = true)
.compile()
- // TODO(b/136250031, b/136458109): Allow inlining of constructors into constructors when
- // compiling to CF.
- .inspect(
- inspector ->
- assertEquals(parameters.isCfRuntime() ? 2 : 1, inspector.allClasses().size()))
+ .inspect(inspector -> assertEquals(1, inspector.allClasses().size()))
.run(parameters.getRuntime(), "Host")
.assertSuccessWithOutput(EXPECTED_OUTPUT);
}
diff --git a/src/test/java/com/android/tools/r8/ir/optimize/inliner/Regress136250031.java b/src/test/java/com/android/tools/r8/ir/optimize/inliner/Regress136250031.java
index eb6d129..c9f8098 100644
--- a/src/test/java/com/android/tools/r8/ir/optimize/inliner/Regress136250031.java
+++ b/src/test/java/com/android/tools/r8/ir/optimize/inliner/Regress136250031.java
@@ -4,9 +4,13 @@
package com.android.tools.r8.ir.optimize.inliner;
+import static org.junit.Assert.assertTrue;
+
import com.android.tools.r8.TestBase;
import com.android.tools.r8.TestParameters;
import com.android.tools.r8.TestParametersCollection;
+import com.android.tools.r8.utils.codeinspector.InstructionSubject;
+import com.android.tools.r8.utils.codeinspector.MethodSubject;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.Parameterized;
@@ -21,7 +25,19 @@
@Parameters(name = "{0}")
public static TestParametersCollection data() {
- return getTestParameters().withAllRuntimesAndApiLevels().build();
+ return TestParameters.builder()
+ .withCfRuntimes()
+ .withDefaultDexRuntime()
+ .withMaximumApiLevel()
+ .build();
+ }
+
+ @Test
+ public void testReference() throws Exception {
+ testForRuntime(parameters)
+ .addInnerClasses(Regress136250031.class)
+ .run(parameters.getRuntime(), TestClass.class)
+ .assertSuccessWithOutputLines("42");
}
@Test
@@ -34,12 +50,29 @@
inspector -> inspector.assertMergedIntoSubtype(A.class).assertNoOtherClassesMerged())
.setMinApi(parameters)
.run(parameters.getRuntime(), TestClass.class)
- .assertSuccessWithOutputLines("42");
+ .assertSuccessWithOutputLines("42")
+ .inspect(
+ inspector -> {
+ MethodSubject initMethod = inspector.clazz(B.class).init();
+ assertTrue(
+ "Expected 'throw null' in " + initMethod.getMethod().codeToString(),
+ initMethod.streamInstructions().anyMatch(InstructionSubject::isThrow));
+ });
}
static class TestClass {
+
+ // This method is inlined into the constructor for B between entry and the subsequent <init>
+ // on Object (after vertically merging A into B).
+ public static final String maybeAbort() {
+ if (System.currentTimeMillis() < 0) {
+ throw null;
+ }
+ return "42";
+ }
+
public static void main(String[] args) {
- new B(new C());
+ new B();
}
}
@@ -50,21 +83,10 @@
}
public static class B extends A {
- B(C c) {
- super(c.instance.toString());
- }
- }
-
- public static class C {
- public C instance;
-
- C() {
- instance = System.currentTimeMillis() > 0 ? this : null;
- }
-
- @Override
- public String toString() {
- return "42";
+ B() {
+ // The merge and inlining will result in a jump to an non-normal exit.
+ // The frame must retain the uninitialized this.
+ super(TestClass.maybeAbort());
}
}
}
diff --git a/src/test/java/com/android/tools/r8/ir/optimize/inliner/sync/InlineWithMonitorInConstructorInline.java b/src/test/java/com/android/tools/r8/ir/optimize/inliner/sync/InlineWithMonitorInConstructorInline.java
index a2b9b48..7ce443c 100644
--- a/src/test/java/com/android/tools/r8/ir/optimize/inliner/sync/InlineWithMonitorInConstructorInline.java
+++ b/src/test/java/com/android/tools/r8/ir/optimize/inliner/sync/InlineWithMonitorInConstructorInline.java
@@ -6,8 +6,8 @@
import static com.android.tools.r8.utils.codeinspector.CodeMatchers.containsConstString;
import static com.android.tools.r8.utils.codeinspector.CodeMatchers.invokesMethod;
+import static com.android.tools.r8.utils.codeinspector.Matchers.isAbsent;
import static com.android.tools.r8.utils.codeinspector.Matchers.isPresent;
-import static com.android.tools.r8.utils.codeinspector.Matchers.isPresentIf;
import static org.hamcrest.CoreMatchers.not;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.junit.Assert.assertEquals;
@@ -62,40 +62,36 @@
ClassSubject fooClassSubject = inspector.clazz(Bar.class);
assertThat(fooClassSubject, isPresent());
- // When compiling to CF we do not allow inlining methods into constructors.
- // TODO(b/136458109): Util class should be absent when compiling to CF.
ClassSubject utilClassSubject = inspector.clazz(Util.class);
- assertThat(utilClassSubject, isPresentIf(parameters.isCfRuntime()));
+ assertThat(utilClassSubject, isAbsent());
- // The Util class is fully inlined when compiling to DEX. Verify that the two monitor
- // instructions are not inlined into the same method.
- if (parameters.isDexRuntime()) {
- if (parameters.getApiLevel().isLessThanOrEqualTo(AndroidApiLevel.M)) {
- // Find the synthetic constructor with an added `int classId` parameter. Verify that only
- // Foo.<init> has been inlined this constructor.
- MethodSubject syntheticInit = fooClassSubject.init("int");
- assertThat(syntheticInit, isPresent());
- assertThat(syntheticInit, containsConstString("foo"));
- assertThat(syntheticInit, not(containsConstString("bar")));
- assertEquals(1, numberOfMonitorEnterInstructions(syntheticInit));
+ // Verify that the two monitor instructions are not inlined into the same method.
+ if (parameters.isCfRuntime()
+ || parameters.getApiLevel().isLessThanOrEqualTo(AndroidApiLevel.M)) {
+ // Find the synthetic constructor with an added `int classId` parameter. Verify that only
+ // Foo.<init> has been inlined this constructor.
+ MethodSubject syntheticInit = fooClassSubject.init("int");
+ assertThat(syntheticInit, isPresent());
+ assertThat(syntheticInit, containsConstString("foo"));
+ assertThat(syntheticInit, not(containsConstString("bar")));
+ assertEquals(1, numberOfMonitorEnterInstructions(syntheticInit));
- // Find the non-synthetic constructor corresponding to Bar.<init>.
- MethodSubject barInit = fooClassSubject.init();
- assertThat(barInit, isPresent());
- assertThat(barInit, containsConstString("bar"));
- assertThat(barInit, not(containsConstString("foo")));
- assertEquals(1, numberOfMonitorEnterInstructions(barInit));
+ // Find the non-synthetic constructor corresponding to Bar.<init>.
+ MethodSubject barInit = fooClassSubject.init();
+ assertThat(barInit, isPresent());
+ assertThat(barInit, containsConstString("bar"));
+ assertThat(barInit, not(containsConstString("foo")));
+ assertEquals(1, numberOfMonitorEnterInstructions(barInit));
- // Finally verify that the synthetic constructor calls the non-synthetic constructor, due to
- // inlining being prohibited.
- assertThat(syntheticInit, invokesMethod(barInit));
- } else {
- MethodSubject syntheticInit = fooClassSubject.uniqueInstanceInitializer();
- assertThat(syntheticInit, isPresent());
- assertThat(syntheticInit, containsConstString("bar"));
- assertThat(syntheticInit, containsConstString("foo"));
- assertEquals(2, numberOfMonitorEnterInstructions(syntheticInit));
- }
+ // Finally verify that the synthetic constructor calls the non-synthetic constructor, due to
+ // inlining being prohibited.
+ assertThat(syntheticInit, invokesMethod(barInit));
+ } else {
+ MethodSubject syntheticInit = fooClassSubject.uniqueInstanceInitializer();
+ assertThat(syntheticInit, isPresent());
+ assertThat(syntheticInit, containsConstString("bar"));
+ assertThat(syntheticInit, containsConstString("foo"));
+ assertEquals(2, numberOfMonitorEnterInstructions(syntheticInit));
}
}
diff --git a/src/test/java/com/android/tools/r8/naming/applymapping/ApplyMappingAfterVerticalMergingMethodTest.java b/src/test/java/com/android/tools/r8/naming/applymapping/ApplyMappingAfterVerticalMergingMethodTest.java
index 52a8deb..c1a3053 100644
--- a/src/test/java/com/android/tools/r8/naming/applymapping/ApplyMappingAfterVerticalMergingMethodTest.java
+++ b/src/test/java/com/android/tools/r8/naming/applymapping/ApplyMappingAfterVerticalMergingMethodTest.java
@@ -113,10 +113,9 @@
assertThat(inspector.clazz(LibrarySubclass.class), isPresent());
List<FoundMethodSubject> methods =
inspector.clazz(LibrarySubclass.class).allMethods();
- assertEquals(backend.isCf() ? 4 : 3, methods.size());
+ assertEquals(3, methods.size());
assertEquals(
- backend.isCf() ? 2 : 1,
- methods.stream().filter(FoundMethodSubject::isInstanceInitializer).count());
+ 1, methods.stream().filter(FoundMethodSubject::isInstanceInitializer).count());
assertEquals(
1, methods.stream().filter(m -> m.getFinalName().contains("main")).count());
assertEquals(
diff --git a/src/test/java/com/android/tools/r8/shaking/ifrule/verticalclassmerging/KeepIfPresentRuleWithVerticalClassMergingTest.java b/src/test/java/com/android/tools/r8/shaking/ifrule/verticalclassmerging/KeepIfPresentRuleWithVerticalClassMergingTest.java
index 01cf6ce..fbde552 100644
--- a/src/test/java/com/android/tools/r8/shaking/ifrule/verticalclassmerging/KeepIfPresentRuleWithVerticalClassMergingTest.java
+++ b/src/test/java/com/android/tools/r8/shaking/ifrule/verticalclassmerging/KeepIfPresentRuleWithVerticalClassMergingTest.java
@@ -54,7 +54,7 @@
assertThat(classBSubject, isPresent());
assertThat(classBSubject.init(), isPresent());
assertThat(classBSubject.uniqueMethodWithOriginalName("greet"), isPresent());
- assertEquals(parameters.isCfRuntime() ? 3 : 2, classBSubject.allMethods().size());
+ assertEquals(2, classBSubject.allMethods().size());
})
.run(parameters.getRuntime(), TestClass.class)
.assertSuccessWithOutputLines("Hello world!");
