Version 1.6.42
Cherry pick: Add test showing lack of inlining of constructors in
vertical class merger
CL: https://r8-review.googlesource.com/c/r8/+/44553
Cherry pick: Disable vertical merging if base has dependency outside
main dex
CL: https://r8-review.googlesource.com/c/r8/+/44625
Bug: 142909854
Change-Id: Ibcc03c75a5d6999df9377dd58fdb8cb81eea300e
diff --git a/src/main/java/com/android/tools/r8/Version.java b/src/main/java/com/android/tools/r8/Version.java
index d8c530b..e337dd1 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.6.41";
+ public static final String LABEL = "1.6.42";
private Version() {
}
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 f90c1b6..664cd90 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
@@ -269,23 +269,28 @@
return false;
}
- if (!inliner.mainDexClasses.isEmpty()) {
- // Don't inline code with references beyond root main dex classes into a root main dex class.
- // If we do this it can increase the size of the main dex dependent classes.
- if (inliner.mainDexClasses.getRoots().contains(method.method.holder)
- && MainDexDirectReferenceTracer.hasReferencesOutsideFromCode(
- appView.appInfo(), candidate, inliner.mainDexClasses.getRoots())) {
- if (info != null) {
- info.exclude(invoke, "target has references beyond main dex");
- }
- return false;
+ // Don't inline code with references beyond root main dex classes into a root main dex class.
+ // If we do this it can increase the size of the main dex dependent classes.
+ if (reason != Reason.FORCE
+ && inlineeRefersToClassesNotInMainDex(method.method.holder, candidate)) {
+ if (info != null) {
+ info.exclude(invoke, "target has references beyond main dex");
}
- // Allow inlining into the classes in the main dex dependent set without restrictions.
+ return false;
}
-
+ assert reason != Reason.FORCE
+ || !inlineeRefersToClassesNotInMainDex(method.method.holder, candidate);
return true;
}
+ private boolean inlineeRefersToClassesNotInMainDex(DexType holder, DexEncodedMethod target) {
+ if (inliner.mainDexClasses.isEmpty() || !inliner.mainDexClasses.getRoots().contains(holder)) {
+ return false;
+ }
+ return MainDexDirectReferenceTracer.hasReferencesOutsideFromCode(
+ appView.appInfo(), target, inliner.mainDexClasses.getRoots());
+ }
+
private boolean satisfiesRequirementsForSimpleInlining(
InvokeMethod invoke, DexEncodedMethod target) {
// If we are looking for a simple method, only inline if actually simple.
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 53d2179..c133ec2 100644
--- a/src/main/java/com/android/tools/r8/shaking/VerticalClassMerger.java
+++ b/src/main/java/com/android/tools/r8/shaking/VerticalClassMerger.java
@@ -130,6 +130,7 @@
ALWAYS_INLINE,
CONFLICT,
ILLEGAL_ACCESS,
+ MAIN_DEX_ROOT_OUTSIDE_REFERENCE,
MERGE_ACROSS_NESTS,
NATIVE_METHOD,
NO_SIDE_EFFECTS,
@@ -163,6 +164,9 @@
case ILLEGAL_ACCESS:
message = "it could lead to illegal accesses";
break;
+ case MAIN_DEX_ROOT_OUTSIDE_REFERENCE:
+ message = "contains a constructor with a reference outside the main dex classes";
+ break;
case MERGE_ACROSS_NESTS:
message = "cannot merge across nests, or from nest to non-nest";
break;
@@ -409,16 +413,21 @@
return false;
}
}
- for (DexEncodedMethod method : clazz.methods()) {
+ for (DexEncodedMethod method : clazz.directMethods()) {
if (appInfo.isPinned(method.method)) {
return false;
}
- if (method.isInstanceInitializer() && disallowInlining(method, singleSubtype)) {
- // Cannot guarantee that markForceInline() will work.
- if (Log.ENABLED) {
- AbortReason.UNSAFE_INLINING.printLogMessageForClass(clazz);
+ // We rename constructors to private methods and mark them to be forced-inlined, so we have to
+ // check if we can force-inline all constructors.
+ if (method.isInstanceInitializer()) {
+ AbortReason reason = disallowInlining(method, singleSubtype);
+ if (reason != null) {
+ // Cannot guarantee that markForceInline() will work.
+ if (Log.ENABLED) {
+ reason.printLogMessageForClass(clazz);
+ }
+ return false;
}
- return false;
}
}
if (clazz.getEnclosingMethod() != null || !clazz.getInnerClasses().isEmpty()) {
@@ -1662,7 +1671,7 @@
}
}
- private boolean disallowInlining(DexEncodedMethod method, DexType invocationContext) {
+ private AbortReason disallowInlining(DexEncodedMethod method, DexType invocationContext) {
if (appView.options().enableInlining) {
if (method.getCode().isCfCode()) {
CfCode code = method.getCode().asCfCode();
@@ -1672,11 +1681,21 @@
appView,
new SingleTypeMapperGraphLense(method.method.holder, invocationContext),
invocationContext);
- return constraint == ConstraintWithTarget.NEVER;
+ if (constraint == ConstraintWithTarget.NEVER) {
+ return AbortReason.UNSAFE_INLINING;
+ }
+ // Constructors can have references beyond the root main dex classes. This can increase the
+ // size of the main dex dependent classes and we should bail out.
+ if (mainDexClasses.getRoots().contains(invocationContext)
+ && MainDexDirectReferenceTracer.hasReferencesOutsideFromCode(
+ appView.appInfo(), method, mainDexClasses.getRoots())) {
+ return AbortReason.MAIN_DEX_ROOT_OUTSIDE_REFERENCE;
+ }
+ return null;
}
// For non-jar/cf code we currently cannot guarantee that markForceInline() will succeed.
}
- return true;
+ return AbortReason.UNSAFE_INLINING;
}
private class SingleTypeMapperGraphLense extends GraphLense {
diff --git a/src/test/java/com/android/tools/r8/classmerging/VerticalClassMergerInitTest.java b/src/test/java/com/android/tools/r8/classmerging/VerticalClassMergerInitTest.java
new file mode 100644
index 0000000..1c4f406
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/classmerging/VerticalClassMergerInitTest.java
@@ -0,0 +1,103 @@
+// 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.classmerging;
+
+import static com.android.tools.r8.utils.codeinspector.Matchers.isPresent;
+import static org.hamcrest.MatcherAssert.assertThat;
+
+import com.android.tools.r8.CompilationFailedException;
+import com.android.tools.r8.TestBase;
+import com.android.tools.r8.TestParameters;
+import com.android.tools.r8.TestParametersCollection;
+import com.android.tools.r8.ir.optimize.Inliner.Reason;
+import com.android.tools.r8.utils.AndroidApiLevel;
+import com.google.common.collect.ImmutableSet;
+import java.io.IOException;
+import java.util.concurrent.ExecutionException;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+import org.junit.runners.Parameterized.Parameters;
+
+@RunWith(Parameterized.class)
+// This is a reproduction of b/142909854 where we vertically merge a class with a constructor that
+// references a class outside the main-dex collection. We did not inline those, even when
+// force-inlining, so the renamed constructor broke the init chain.
+public class VerticalClassMergerInitTest extends TestBase {
+
+ private final TestParameters parameters;
+
+ @Parameters(name = "{0}")
+ public static TestParametersCollection data() {
+ return getTestParameters().withDexRuntimes().build();
+ }
+
+ public VerticalClassMergerInitTest(TestParameters parameters) {
+ this.parameters = parameters;
+ }
+
+ @Test
+ public void testMergingClassWithConstructorNotInMainDex()
+ throws IOException, CompilationFailedException, ExecutionException {
+ testForR8(parameters.getBackend())
+ .addInnerClasses(VerticalClassMergerInitTest.class)
+ .addKeepMainRule(Main.class)
+ .addMainDexRules("-keep class " + Main.class.getTypeName())
+ .setMinApi(AndroidApiLevel.K_WATCH)
+ .addOptionsModification(
+ options -> {
+ options.forceProguardCompatibility = true;
+ // The initializer is small in this example so only allow FORCE inlining.
+ options.testing.validInliningReasons = ImmutableSet.of(Reason.FORCE);
+ })
+ .compile()
+ .inspect(
+ inspector -> {
+ assertThat(inspector.clazz(Base.class), isPresent());
+ assertThat(inspector.clazz(Child.class), isPresent());
+ })
+ .run(parameters.getRuntime(), Main.class)
+ .assertSuccessWithOutputLines("Base.init()", "Outside.init()", "Child.init()");
+ }
+
+ public static class Base {
+
+ public Base() {
+ System.out.println("Base.init()");
+ new Outside();
+ }
+ }
+
+ private static class Outside {
+ Outside() {
+ System.out.println("Outside.init()");
+ }
+ }
+
+ public static class Child extends Base {
+
+ // We need a static member to force the main-dex tracing to include Child and Base.
+ public static Object foo() {
+ return null;
+ }
+
+ public Child() {
+ System.out.println("Child.init()");
+ }
+ }
+
+ public static class Main {
+
+ {
+ if (Child.foo() == null) {
+ System.out.println("Main.clinit()");
+ }
+ }
+
+ public static void main(String[] args) {
+ new Child();
+ }
+ }
+}
