Disable vertical merging if base has dependency outside main dex
This CL will add an additional check in the vertical class merger when
merging bases with constructors if compiling with a main dex list.
The check will look through constructors for references outside the
main dex and disable merging if a dependency is found.
The check requires scanning the code of constructors so the check is
placed as the last one.
Bug: 142909854
Change-Id: I8d046f7666290fc8e358b28c3f030e4c3093df04
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 2649e62..0251620 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
@@ -290,21 +290,26 @@
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(), singleTarget, inliner.mainDexClasses.getRoots())) {
- whyAreYouNotInliningReporter.reportInlineeRefersToClassesNotInMainDex();
- return false;
- }
- // Allow inlining into the classes in the main dex dependent set without restrictions.
+ // 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, singleTarget)) {
+ whyAreYouNotInliningReporter.reportInlineeRefersToClassesNotInMainDex();
+ return false;
}
-
+ assert reason != Reason.FORCE
+ || !inlineeRefersToClassesNotInMainDex(method.method.holder, singleTarget);
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 1e6b0ee..159ca22 100644
--- a/src/main/java/com/android/tools/r8/shaking/VerticalClassMerger.java
+++ b/src/main/java/com/android/tools/r8/shaking/VerticalClassMerger.java
@@ -131,6 +131,7 @@
ALWAYS_INLINE,
CONFLICT,
ILLEGAL_ACCESS,
+ MAIN_DEX_ROOT_OUTSIDE_REFERENCE,
MERGE_ACROSS_NESTS,
NATIVE_METHOD,
NO_SIDE_EFFECTS,
@@ -164,6 +165,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;
@@ -417,12 +421,17 @@
return false;
}
for (DexEncodedMethod method : clazz.directMethods()) {
- 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()) {
@@ -1663,7 +1672,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();
@@ -1673,11 +1682,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
index 3285930..1c4f406 100644
--- a/src/test/java/com/android/tools/r8/classmerging/VerticalClassMergerInitTest.java
+++ b/src/test/java/com/android/tools/r8/classmerging/VerticalClassMergerInitTest.java
@@ -5,9 +5,7 @@
package com.android.tools.r8.classmerging;
import static com.android.tools.r8.utils.codeinspector.Matchers.isPresent;
-import static org.hamcrest.CoreMatchers.containsString;
import static org.hamcrest.MatcherAssert.assertThat;
-import static org.hamcrest.core.IsNot.not;
import com.android.tools.r8.CompilationFailedException;
import com.android.tools.r8.TestBase;
@@ -43,7 +41,6 @@
@Test
public void testMergingClassWithConstructorNotInMainDex()
throws IOException, CompilationFailedException, ExecutionException {
- // TODO(b/142909854): Fix test expectation when bug has been resolved.
testForR8(parameters.getBackend())
.addInnerClasses(VerticalClassMergerInitTest.class)
.addKeepMainRule(Main.class)
@@ -58,11 +55,11 @@
.compile()
.inspect(
inspector -> {
- assertThat(inspector.clazz(Base.class), not(isPresent()));
+ assertThat(inspector.clazz(Base.class), isPresent());
assertThat(inspector.clazz(Child.class), isPresent());
})
.run(parameters.getRuntime(), Main.class)
- .assertFailureWithErrorThatMatches(containsString("initialized"));
+ .assertSuccessWithOutputLines("Base.init()", "Outside.init()", "Child.init()");
}
public static class Base {
