Avoid inlining monitors into code with monitors on M
We are seeing art vms on M that are seemingly messing up the
synchronization on some android 6 devices
This cl will disallow inlining code with monitors when the inlinee
already have monitors.
Bug: b/238399429
Change-Id: Ia32a8d81690adfde4a52c003008d7d335e7584b7
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/Inliner.java b/src/main/java/com/android/tools/r8/ir/optimize/Inliner.java
index 592b645..b2a3f3e 100644
--- a/src/main/java/com/android/tools/r8/ir/optimize/Inliner.java
+++ b/src/main/java/com/android/tools/r8/ir/optimize/Inliner.java
@@ -1022,6 +1022,15 @@
InlineeWithReason inlinee =
action.buildInliningIR(
appView, invoke, context, inliningIRProvider, lensCodeRewriter);
+
+ // b/238399429 Don't inline if we have monitor instructions in both
+ // inliner and inlinee if the Art runtime might mess up synchronization.
+ if (inlinee.code.metadata().mayHaveMonitorInstruction()
+ && code.metadata().mayHaveMonitorInstruction()
+ && appView.options().canHaveIssueWithInlinedMonitors()) {
+ continue;
+ }
+
if (strategy.willExceedBudget(
code, invoke, inlinee, block, whyAreYouNotInliningReporter)) {
assert whyAreYouNotInliningReporter.unsetReasonHasBeenReportedFlag();
diff --git a/src/main/java/com/android/tools/r8/utils/InternalOptions.java b/src/main/java/com/android/tools/r8/utils/InternalOptions.java
index 98d5a27..ae36b27 100644
--- a/src/main/java/com/android/tools/r8/utils/InternalOptions.java
+++ b/src/main/java/com/android/tools/r8/utils/InternalOptions.java
@@ -2815,4 +2815,10 @@
// TODO(b/246679983): Turned off while diagnosing b/246679983.
return false && isGeneratingDex() && minApiLevel.isGreaterThanOrEqualTo(AndroidApiLevel.L);
}
+
+ // b/238399429 Some art 6 vms have issues with multiple monitors in the same method
+ // Don't inline code with monitors into methods that already have monitors.
+ public boolean canHaveIssueWithInlinedMonitors() {
+ return canHaveBugPresentUntil(AndroidApiLevel.N);
+ }
}
diff --git a/src/test/java/com/android/tools/r8/ir/optimize/inliner/sync/InlineStaticSynchronizedMethodTest.java b/src/test/java/com/android/tools/r8/ir/optimize/inliner/sync/InlineStaticSynchronizedMethodTest.java
index 1a20851..569652a 100644
--- a/src/test/java/com/android/tools/r8/ir/optimize/inliner/sync/InlineStaticSynchronizedMethodTest.java
+++ b/src/test/java/com/android/tools/r8/ir/optimize/inliner/sync/InlineStaticSynchronizedMethodTest.java
@@ -12,6 +12,7 @@
import com.android.tools.r8.TestParameters;
import com.android.tools.r8.TestParametersCollection;
import com.android.tools.r8.ir.optimize.inliner.sync.InlineStaticSynchronizedMethodTest.TestClass.RunnableImpl;
+import com.android.tools.r8.utils.AndroidApiLevel;
import com.android.tools.r8.utils.codeinspector.ClassSubject;
import com.android.tools.r8.utils.codeinspector.CodeInspector;
import java.lang.Thread.State;
@@ -53,8 +54,17 @@
private void verifySynchronizedMethodsAreInlined(CodeInspector inspector) {
ClassSubject classSubject = inspector.clazz(RunnableImpl.class);
assertThat(classSubject, isPresent());
- assertThat(classSubject.uniqueMethodWithOriginalName("m1"), not(isPresent()));
- assertThat(classSubject.uniqueMethodWithOriginalName("m2"), not(isPresent()));
+ // On M we are seeing issues when inlining code with monitors which will trip up some art
+ // vms. See issue b/238399429 for details.
+ if (parameters.isCfRuntime()
+ || parameters.getApiLevel().isLessThanOrEqualTo(AndroidApiLevel.M)) {
+ assertThat(classSubject.uniqueMethodWithOriginalName("m1"), isPresent());
+ assertThat(classSubject.uniqueMethodWithOriginalName("m2"), not(isPresent()));
+
+ } else {
+ assertThat(classSubject.uniqueMethodWithOriginalName("m1"), not(isPresent()));
+ assertThat(classSubject.uniqueMethodWithOriginalName("m2"), not(isPresent()));
+ }
}
static class TestClass {
diff --git a/src/test/java/com/android/tools/r8/ir/optimize/inliner/sync/InlinerMonitorEnterValuesThresholdTest.java b/src/test/java/com/android/tools/r8/ir/optimize/inliner/sync/InlinerMonitorEnterValuesThresholdTest.java
index d6c07ca..c300549 100644
--- a/src/test/java/com/android/tools/r8/ir/optimize/inliner/sync/InlinerMonitorEnterValuesThresholdTest.java
+++ b/src/test/java/com/android/tools/r8/ir/optimize/inliner/sync/InlinerMonitorEnterValuesThresholdTest.java
@@ -10,6 +10,7 @@
import com.android.tools.r8.TestBase;
import com.android.tools.r8.TestParameters;
+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.google.common.collect.ImmutableList;
@@ -54,13 +55,22 @@
ClassSubject classSubject = inspector.clazz(TestClass.class);
assertThat(classSubject, isPresent());
assertThat(classSubject.mainMethod(), isPresent());
- assertThat(classSubject.uniqueMethodWithOriginalName("m1"), not(isPresent()));
- assertThat(classSubject.uniqueMethodWithOriginalName("m2"), not(isPresent()));
- if (threshold == 2) {
+ // On M we are seeing issues when inlining code with monitors which will trip up some art
+ // vms. See issue b/238399429 for details.
+ if (parameters.isCfRuntime()
+ || parameters.getApiLevel().isLessThanOrEqualTo(AndroidApiLevel.M)) {
+ assertThat(classSubject.uniqueMethodWithOriginalName("m1"), isPresent());
+ assertThat(classSubject.uniqueMethodWithOriginalName("m1"), isPresent());
assertThat(classSubject.uniqueMethodWithOriginalName("m3"), isPresent());
} else {
- assert threshold == 3;
- assertThat(classSubject.uniqueMethodWithOriginalName("m3"), not(isPresent()));
+ assertThat(classSubject.uniqueMethodWithOriginalName("m1"), not(isPresent()));
+ assertThat(classSubject.uniqueMethodWithOriginalName("m2"), not(isPresent()));
+ if (threshold == 2) {
+ assertThat(classSubject.uniqueMethodWithOriginalName("m3"), isPresent());
+ } else {
+ assert threshold == 3;
+ assertThat(classSubject.uniqueMethodWithOriginalName("m3"), not(isPresent()));
+ }
}
}
