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: I116933e664d1cba5070414e85a4e3070a77e5ff1
diff --git a/src/main/java/com/android/tools/r8/graph/CfCode.java b/src/main/java/com/android/tools/r8/graph/CfCode.java
index 43e7099..535a891 100644
--- a/src/main/java/com/android/tools/r8/graph/CfCode.java
+++ b/src/main/java/com/android/tools/r8/graph/CfCode.java
@@ -16,6 +16,7 @@
import com.android.tools.r8.cf.code.CfInstruction;
import com.android.tools.r8.cf.code.CfLabel;
import com.android.tools.r8.cf.code.CfLoad;
+import com.android.tools.r8.cf.code.CfMonitor;
import com.android.tools.r8.cf.code.CfPosition;
import com.android.tools.r8.cf.code.CfReturnVoid;
import com.android.tools.r8.cf.code.CfTryCatch;
@@ -534,6 +535,16 @@
}
@Override
+ public boolean hasMonitorInstructions() {
+ for (CfInstruction instruction : getInstructions()) {
+ if (instruction instanceof CfMonitor) {
+ return true;
+ }
+ }
+ return false;
+ }
+
+ @Override
public IRCode buildIR(
ProgramMethod method,
AppView<?> appView,
diff --git a/src/main/java/com/android/tools/r8/graph/Code.java b/src/main/java/com/android/tools/r8/graph/Code.java
index 1077fb9..d0417b4 100644
--- a/src/main/java/com/android/tools/r8/graph/Code.java
+++ b/src/main/java/com/android/tools/r8/graph/Code.java
@@ -117,6 +117,10 @@
return false;
}
+ public boolean hasMonitorInstructions() {
+ return false;
+ }
+
public boolean isThrowExceptionCode() {
return false;
}
diff --git a/src/main/java/com/android/tools/r8/graph/DexCode.java b/src/main/java/com/android/tools/r8/graph/DexCode.java
index 7e55f8e..09db76d 100644
--- a/src/main/java/com/android/tools/r8/graph/DexCode.java
+++ b/src/main/java/com/android/tools/r8/graph/DexCode.java
@@ -9,6 +9,7 @@
import com.android.tools.r8.dex.MixedSectionCollection;
import com.android.tools.r8.dex.code.CfOrDexInstruction;
import com.android.tools.r8.dex.code.DexInstruction;
+import com.android.tools.r8.dex.code.DexMonitorEnter;
import com.android.tools.r8.dex.code.DexReturnVoid;
import com.android.tools.r8.dex.code.DexSwitchPayload;
import com.android.tools.r8.graph.DexCode.TryHandler.TypeAddrPair;
@@ -377,6 +378,16 @@
}
@Override
+ public boolean hasMonitorInstructions() {
+ for (DexInstruction instruction : instructions) {
+ if (instruction instanceof DexMonitorEnter) {
+ return true;
+ }
+ }
+ return false;
+ }
+
+ @Override
public Code asCode() {
return this;
}
diff --git a/src/main/java/com/android/tools/r8/graph/LazyCfCode.java b/src/main/java/com/android/tools/r8/graph/LazyCfCode.java
index 3ab128b..3abc2e4 100644
--- a/src/main/java/com/android/tools/r8/graph/LazyCfCode.java
+++ b/src/main/java/com/android/tools/r8/graph/LazyCfCode.java
@@ -229,6 +229,11 @@
}
@Override
+ public boolean hasMonitorInstructions() {
+ return asCfCode().hasMonitorInstructions();
+ }
+
+ @Override
public int estimatedSizeForInlining() {
return asCfCode().estimatedSizeForInlining();
}
diff --git a/src/main/java/com/android/tools/r8/horizontalclassmerging/PolicyScheduler.java b/src/main/java/com/android/tools/r8/horizontalclassmerging/PolicyScheduler.java
index b997f78..b6095d1 100644
--- a/src/main/java/com/android/tools/r8/horizontalclassmerging/PolicyScheduler.java
+++ b/src/main/java/com/android/tools/r8/horizontalclassmerging/PolicyScheduler.java
@@ -46,6 +46,7 @@
import com.android.tools.r8.horizontalclassmerging.policies.NoVirtualMethodMerging;
import com.android.tools.r8.horizontalclassmerging.policies.NoWeakerAccessPrivileges;
import com.android.tools.r8.horizontalclassmerging.policies.NotMatchedByNoHorizontalClassMerging;
+import com.android.tools.r8.horizontalclassmerging.policies.NotTwoInitsWithMonitors;
import com.android.tools.r8.horizontalclassmerging.policies.OnlyClassesWithStaticDefinitionsAndNoClassInitializer;
import com.android.tools.r8.horizontalclassmerging.policies.OnlyDirectlyConnectedOrUnrelatedInterfaces;
import com.android.tools.r8.horizontalclassmerging.policies.PreserveMethodCharacteristics;
@@ -292,6 +293,9 @@
new NoIndirectRuntimeTypeChecks(appView, runtimeTypeCheckInfo),
new NoWeakerAccessPrivileges(appView, immediateSubtypingInfo),
new PreventClassMethodAndDefaultMethodCollisions(appView, immediateSubtypingInfo));
+ if (appView.options().canHaveIssueWithInlinedMonitors()) {
+ builder.add(new NotTwoInitsWithMonitors());
+ }
}
private static void addMultiClassPoliciesForMergingNonSyntheticClasses(
diff --git a/src/main/java/com/android/tools/r8/horizontalclassmerging/policies/NotTwoInitsWithMonitors.java b/src/main/java/com/android/tools/r8/horizontalclassmerging/policies/NotTwoInitsWithMonitors.java
new file mode 100644
index 0000000..d5cebe2
--- /dev/null
+++ b/src/main/java/com/android/tools/r8/horizontalclassmerging/policies/NotTwoInitsWithMonitors.java
@@ -0,0 +1,28 @@
+// Copyright (c) 2022, 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.horizontalclassmerging.policies;
+
+import com.android.tools.r8.graph.DexEncodedMethod;
+import com.android.tools.r8.graph.DexProgramClass;
+import com.android.tools.r8.graph.ProgramMethod;
+
+public class NotTwoInitsWithMonitors extends AtMostOneClassThatMatchesPolicy {
+
+ @Override
+ public boolean atMostOneOf(DexProgramClass clazz) {
+ for (ProgramMethod initializer : clazz.programInstanceInitializers()) {
+ DexEncodedMethod definition = initializer.getDefinition();
+ if (definition.isSynchronized() || definition.getCode().hasMonitorInstructions()) {
+ return true;
+ }
+ }
+ return false;
+ }
+
+ @Override
+ public String getName() {
+ return "NotTwoInitsWithMonitors";
+ }
+}
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 69d51be..0db211d 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
@@ -15,6 +15,7 @@
import com.android.tools.r8.graph.AppView;
import com.android.tools.r8.graph.Code;
import com.android.tools.r8.graph.DexEncodedField;
+import com.android.tools.r8.graph.DexEncodedMethod;
import com.android.tools.r8.graph.DexField;
import com.android.tools.r8.graph.DexMethod;
import com.android.tools.r8.graph.DexType;
@@ -153,6 +154,10 @@
return false;
}
+ if (canHaveIssuesWithMonitors(singleTarget, method)) {
+ return false;
+ }
+
// We should never even try to inline something that is processed concurrently. It can lead
// to non-deterministic behaviour as the inlining IR could be built from either original output
// or optimized code. Right now this happens for the class class staticizer, as it just
@@ -199,6 +204,22 @@
return true;
}
+ private boolean canHaveIssuesWithMonitors(ProgramMethod singleTarget, ProgramMethod context) {
+ if (appView.options().canHaveIssueWithInlinedMonitors()) {
+ if (hasMonitorsOrIsSynchronized(singleTarget.getDefinition())) {
+ if (context.getOptimizationInfo().forceInline()
+ || hasMonitorsOrIsSynchronized(context.getDefinition())) {
+ return true;
+ }
+ }
+ }
+ return false;
+ }
+
+ public static boolean hasMonitorsOrIsSynchronized(DexEncodedMethod definition) {
+ return definition.isSynchronized() || definition.getCode().hasMonitorInstructions();
+ }
+
public boolean satisfiesRequirementsForSimpleInlining(InvokeMethod invoke, ProgramMethod target) {
// Code size modified by inlining, so only read for non-concurrent methods.
boolean deterministic = !methodProcessor.isProcessedConcurrently(target);
@@ -303,6 +324,17 @@
return null;
}
+ // Ensure that we don't introduce several monitors in the same method on old device that can
+ // choke on this. If a context is forceinline, e.g., from class merging, don't ever inline
+ // monitors, since that may conflict with a similar other constructor.
+ if (appView.options().canHaveIssueWithInlinedMonitors()) {
+ if (hasMonitorsOrIsSynchronized(singleTarget.getDefinition())
+ && (context.getOptimizationInfo().forceInline()
+ || code.metadata().mayHaveMonitorInstruction())) {
+ return null;
+ }
+ }
+
InlineAction action =
invoke.computeInlining(
singleTarget, reason, this, classInitializationAnalysis, whyAreYouNotInliningReporter);
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 b4099ee..3b87985 100644
--- a/src/main/java/com/android/tools/r8/utils/InternalOptions.java
+++ b/src/main/java/com/android/tools/r8/utils/InternalOptions.java
@@ -2824,4 +2824,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/InlineConstructorsWithMonitors.java b/src/test/java/com/android/tools/r8/ir/optimize/inliner/sync/InlineConstructorsWithMonitors.java
new file mode 100644
index 0000000..052398d
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/ir/optimize/inliner/sync/InlineConstructorsWithMonitors.java
@@ -0,0 +1,94 @@
+// Copyright (c) 2022, 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.ir.optimize.inliner.sync;
+
+import static com.android.tools.r8.utils.codeinspector.Matchers.hasDefaultConstructor;
+import static com.android.tools.r8.utils.codeinspector.Matchers.isPresent;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+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 java.util.List;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+import org.junit.runners.Parameterized.Parameters;
+
+@RunWith(Parameterized.class)
+public class InlineConstructorsWithMonitors extends TestBase {
+
+ private final TestParameters parameters;
+
+ @Parameters(name = "{0}")
+ public static List<Object[]> data() {
+ return buildParameters(getTestParameters().withAllRuntimesAndApiLevels().build());
+ }
+
+ public InlineConstructorsWithMonitors(TestParameters parameters) {
+ this.parameters = parameters;
+ }
+
+ @Test
+ public void test() throws Exception {
+ testForR8(parameters.getBackend())
+ .addInnerClasses(InlineConstructorsWithMonitors.class)
+ .addKeepMainRule(TestClass.class)
+ .setMinApi(parameters.getApiLevel())
+ .compile()
+ .run(parameters.getRuntime(), TestClass.class)
+ .assertSuccessWithOutputLines("monitor", "monitor2")
+ .inspect(this::inspect);
+ }
+
+ private void inspect(CodeInspector inspector) {
+ ClassSubject classSubject = inspector.clazz(TestClass.class);
+ assertThat(classSubject, isPresent());
+ ClassSubject fooClassSubject = inspector.clazz(Foo.class);
+ ClassSubject barClassSubject = inspector.clazz(Bar.class);
+ if (parameters.isCfRuntime()
+ || parameters.getApiLevel().isLessThanOrEqualTo(AndroidApiLevel.M)) {
+ // On M and below we don't want to merge constructors when both have monitors. See b/238399429
+ assertThat(fooClassSubject, hasDefaultConstructor());
+ assertThat(barClassSubject, hasDefaultConstructor());
+ } else {
+ assertTrue(
+ fooClassSubject.uniqueInstanceInitializer().isPresent()
+ || barClassSubject.uniqueInstanceInitializer().isPresent());
+ assertFalse(
+ fooClassSubject.uniqueInstanceInitializer().isPresent()
+ && barClassSubject.uniqueInstanceInitializer().isPresent());
+ }
+ }
+
+ static class Foo {
+ public Foo() {
+ Object o = new Object();
+ synchronized (o) {
+ System.out.println("monitor");
+ }
+ }
+ }
+
+ static class Bar {
+ public Bar() {
+ Object o = new String("Foo");
+ synchronized (o) {
+ System.out.println("monitor2");
+ }
+ }
+ }
+
+ static class TestClass {
+ public static void main(String[] args) {
+ new Foo();
+ new Bar();
+ }
+ }
+}
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/InlineWithMonitorInConstructorInline.java b/src/test/java/com/android/tools/r8/ir/optimize/inliner/sync/InlineWithMonitorInConstructorInline.java
new file mode 100644
index 0000000..96120c6
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/ir/optimize/inliner/sync/InlineWithMonitorInConstructorInline.java
@@ -0,0 +1,95 @@
+// Copyright (c) 2022, 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.ir.optimize.inliner.sync;
+
+import static com.android.tools.r8.utils.codeinspector.Matchers.isPresent;
+import static org.hamcrest.CoreMatchers.not;
+import static org.hamcrest.MatcherAssert.assertThat;
+
+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 java.util.List;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+import org.junit.runners.Parameterized.Parameters;
+
+@RunWith(Parameterized.class)
+public class InlineWithMonitorInConstructorInline extends TestBase {
+
+ private final TestParameters parameters;
+
+ @Parameters(name = "{0}")
+ public static List<Object[]> data() {
+ return buildParameters(getTestParameters().withAllRuntimesAndApiLevels().build());
+ }
+
+ public InlineWithMonitorInConstructorInline(TestParameters parameters) {
+ this.parameters = parameters;
+ }
+
+ @Test
+ public void test() throws Exception {
+ testForR8(parameters.getBackend())
+ .addInnerClasses(InlineWithMonitorInConstructorInline.class)
+ .addKeepMainRule(TestClass.class)
+ .setMinApi(parameters.getApiLevel())
+ .run(parameters.getRuntime(), TestClass.class)
+ .inspect(this::inspect)
+ .assertSuccessWithOutputLines("foo", "monitor", "bar", "monitor2");
+ }
+
+ private void inspect(CodeInspector inspector) {
+ ClassSubject classSubject = inspector.clazz(TestClass.class);
+ assertThat(classSubject, isPresent());
+ ClassSubject utilClassSubject = inspector.clazz(Util.class);
+ if (parameters.isCfRuntime()
+ || parameters.getApiLevel().isLessThanOrEqualTo(AndroidApiLevel.M)) {
+ // 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, not(isPresent()));
+ }
+ }
+
+ static class Foo {
+ public Foo() {
+ System.out.println("foo");
+ Util.useMonitor(this);
+ }
+ }
+
+ static class Bar {
+ public Bar() {
+ System.out.println("bar");
+ Util.useMonitor2(this);
+ }
+ }
+
+ static class Util {
+ public static void useMonitor(Object object) {
+ synchronized (object) {
+ System.out.println("monitor");
+ }
+ }
+
+ public static void useMonitor2(Object object) {
+ synchronized (object) {
+ System.out.println("monitor2");
+ }
+ }
+ }
+
+ static class TestClass {
+ public static void main(String[] args) {
+ new Foo();
+ new Bar();
+ }
+ }
+}
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..3ae6d3d 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;
@@ -44,9 +45,8 @@
.addOptionsModification(
options -> options.inlinerOptions().inliningMonitorEnterValuesAllowance = threshold)
.setMinApi(parameters.getApiLevel())
- .compile()
- .inspect(this::inspect)
.run(parameters.getRuntime(), TestClass.class)
+ .inspect(this::inspect)
.assertSuccessWithOutputLines("Hello world!");
}
@@ -54,13 +54,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("m2"), 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()));
+ }
}
}
