Prevent inlining of monitors into callers with monitors
Fixes: b/322801694
Change-Id: Idccb1dae190450d971979f566f033ac8bbf0bb2d
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 734004b..c6bd2a2 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
@@ -134,6 +134,7 @@
@Override
public boolean passesInliningConstraints(
+ IRCode code,
SingleResolutionResult<?> resolutionResult,
ProgramMethod singleTarget,
WhyAreYouNotInliningReporter whyAreYouNotInliningReporter) {
@@ -158,7 +159,7 @@
return false;
}
- if (canHaveIssuesWithMonitors(singleTarget, method)) {
+ if (canHaveIssuesWithMonitors(code, singleTarget)) {
return false;
}
@@ -198,16 +199,20 @@
return true;
}
- private boolean canHaveIssuesWithMonitors(ProgramMethod singleTarget, ProgramMethod context) {
- if (options.canHaveIssueWithInlinedMonitors() && hasMonitorsOrIsSynchronized(singleTarget)) {
- return context.getOptimizationInfo().forceInline() || hasMonitorsOrIsSynchronized(context);
- }
- return false;
+ private boolean canHaveIssuesWithMonitors(IRCode code, ProgramMethod singleTarget) {
+ return options.canHaveIssueWithInlinedMonitors()
+ && hasMonitorsOrIsSynchronized(code)
+ && hasMonitorsOrIsSynchronized(singleTarget);
}
- public static boolean hasMonitorsOrIsSynchronized(ProgramMethod method) {
- return method.getAccessFlags().isSynchronized()
- || method.getDefinition().getCode().hasMonitorInstructions();
+ public static boolean hasMonitorsOrIsSynchronized(IRCode code) {
+ return code.context().getAccessFlags().isSynchronized()
+ || code.metadata().mayHaveMonitorInstruction();
+ }
+
+ public static boolean hasMonitorsOrIsSynchronized(ProgramMethod singleTarget) {
+ return singleTarget.getAccessFlags().isSynchronized()
+ || singleTarget.getDefinition().getCode().hasMonitorInstructions();
}
public boolean satisfiesRequirementsForSimpleInlining(
@@ -438,9 +443,7 @@
}
if (!passesInliningConstraints(
- resolutionResult,
- singleTarget,
- whyAreYouNotInliningReporter)) {
+ code, resolutionResult, singleTarget, whyAreYouNotInliningReporter)) {
return null;
}
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/ForcedInliningOracle.java b/src/main/java/com/android/tools/r8/ir/optimize/ForcedInliningOracle.java
index 5dc2341..a3fb7e7 100644
--- a/src/main/java/com/android/tools/r8/ir/optimize/ForcedInliningOracle.java
+++ b/src/main/java/com/android/tools/r8/ir/optimize/ForcedInliningOracle.java
@@ -44,6 +44,7 @@
@Override
public boolean passesInliningConstraints(
+ IRCode code,
SingleResolutionResult<?> resolutionResult,
ProgramMethod candidate,
WhyAreYouNotInliningReporter whyAreYouNotInliningReporter) {
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/InliningOracle.java b/src/main/java/com/android/tools/r8/ir/optimize/InliningOracle.java
index 69d5ecf..a684ab7 100644
--- a/src/main/java/com/android/tools/r8/ir/optimize/InliningOracle.java
+++ b/src/main/java/com/android/tools/r8/ir/optimize/InliningOracle.java
@@ -77,6 +77,7 @@
void markInlined(IRCode inlinee);
boolean passesInliningConstraints(
+ IRCode code,
SingleResolutionResult<?> resolutionResult,
ProgramMethod candidate,
WhyAreYouNotInliningReporter whyAreYouNotInliningReporter);
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/classinliner/ClassInliner.java b/src/main/java/com/android/tools/r8/ir/optimize/classinliner/ClassInliner.java
index b192f8a..a359197 100644
--- a/src/main/java/com/android/tools/r8/ir/optimize/classinliner/ClassInliner.java
+++ b/src/main/java/com/android/tools/r8/ir/optimize/classinliner/ClassInliner.java
@@ -153,12 +153,7 @@
while (rootsIterator.hasNext()) {
Instruction root = rootsIterator.next();
InlineCandidateProcessor processor =
- new InlineCandidateProcessor(
- appView,
- inliner,
- methodProcessor,
- method,
- root);
+ new InlineCandidateProcessor(appView, code, inliner, methodProcessor, method, root);
// Assess eligibility of instance and class.
EligibilityStatus status = processor.isInstanceEligible();
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 256abbb..46d4883 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
@@ -90,6 +90,7 @@
AssumeAndCheckCastAliasedValueConfiguration.getInstance();
private final AppView<AppInfoWithLiveness> appView;
+ private final IRCode code;
private final DexItemFactory dexItemFactory;
private final Inliner inliner;
private final MethodProcessor methodProcessor;
@@ -113,11 +114,13 @@
InlineCandidateProcessor(
AppView<AppInfoWithLiveness> appView,
+ IRCode code,
Inliner inliner,
MethodProcessor methodProcessor,
ProgramMethod method,
Instruction root) {
this.appView = appView;
+ this.code = code;
this.dexItemFactory = appView.dexItemFactory();
this.inliner = inliner;
this.method = method;
@@ -1167,9 +1170,7 @@
// Check if the method is inline-able by standard inliner.
InliningOracle oracle = defaultOracle.computeIfAbsent();
if (!oracle.passesInliningConstraints(
- resolutionResult,
- singleTarget,
- NopWhyAreYouNotInliningReporter.getInstance())) {
+ code, resolutionResult, singleTarget, NopWhyAreYouNotInliningReporter.getInstance())) {
return false;
}
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 b80dc58..a2b9b48 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
@@ -4,34 +4,39 @@
package com.android.tools.r8.ir.optimize.inliner.sync;
-import static com.android.tools.r8.utils.codeinspector.Matchers.isAbsent;
+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.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;
import com.android.tools.r8.TestBase;
import com.android.tools.r8.TestParameters;
import com.android.tools.r8.TestParametersCollection;
+import com.android.tools.r8.utils.AndroidApiLevel;
import com.android.tools.r8.utils.codeinspector.ClassSubject;
import com.android.tools.r8.utils.codeinspector.CodeInspector;
+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;
+import org.junit.runners.Parameterized.Parameter;
import org.junit.runners.Parameterized.Parameters;
@RunWith(Parameterized.class)
public class InlineWithMonitorInConstructorInline extends TestBase {
- private final TestParameters parameters;
+ @Parameter(0)
+ public TestParameters parameters;
@Parameters(name = "{0}")
public static TestParametersCollection data() {
return getTestParameters().withAllRuntimesAndApiLevels().build();
}
- public InlineWithMonitorInConstructorInline(TestParameters parameters) {
- this.parameters = parameters;
- }
-
@Test
public void test() throws Exception {
testForR8(parameters.getBackend())
@@ -48,19 +53,56 @@
.assertSuccessWithOutputLines("foo", "monitor", "bar", "monitor2");
}
+ // Check we at most inline one method with monitor instructions into synthetic constructors
+ // created by class merging. See also b/238399429.
private void inspect(CodeInspector inspector) {
ClassSubject classSubject = inspector.clazz(TestClass.class);
assertThat(classSubject, isPresent());
+
+ 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);
- if (parameters.isCfRuntime()) {
- // We disallow merging methods with monitors into constructors which will be class merged
- // on pre M devices, see b/238399429
- assertThat(utilClassSubject, isPresent());
- } else {
- assertThat(utilClassSubject, isAbsent());
+ assertThat(utilClassSubject, isPresentIf(parameters.isCfRuntime()));
+
+ // 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));
+
+ // 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));
+ }
}
}
+ private static long numberOfMonitorEnterInstructions(MethodSubject methodSubject) {
+ return methodSubject.streamInstructions().filter(InstructionSubject::isMonitorEnter).count();
+ }
+
static class Foo {
public Foo() {
System.out.println("foo");
diff --git a/src/test/java/com/android/tools/r8/utils/codeinspector/CodeMatchers.java b/src/test/java/com/android/tools/r8/utils/codeinspector/CodeMatchers.java
index d711ac1..8c5bcb6 100644
--- a/src/test/java/com/android/tools/r8/utils/codeinspector/CodeMatchers.java
+++ b/src/test/java/com/android/tools/r8/utils/codeinspector/CodeMatchers.java
@@ -151,6 +151,29 @@
};
}
+ public static Matcher<MethodSubject> containsConstString(String string) {
+ return new TypeSafeMatcher<MethodSubject>() {
+ @Override
+ protected boolean matchesSafely(MethodSubject subject) {
+ return subject.isPresent()
+ && subject.getMethod().hasCode()
+ && subject
+ .streamInstructions()
+ .anyMatch(instructionSubject -> instructionSubject.isConstString(string));
+ }
+
+ @Override
+ public void describeTo(Description description) {
+ description.appendText("contains const-string");
+ }
+
+ @Override
+ public void describeMismatchSafely(MethodSubject subject, Description description) {
+ description.appendText("method did not");
+ }
+ };
+ }
+
public static Matcher<MethodSubject> instantiatesClass(Class<?> clazz) {
return instantiatesClass(clazz.getTypeName());
}
