Do not classinline when root is a static get and is used with a monitor
We can only remove monitor instructions in the class inliner, when the
root is a new instance. For static get roots, we cannot guarantee that
the instance is not shared.
Bug: 147411673
Change-Id: I4d5463ceecbde2941c3431510295551bfc2d9766
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/classinliner/ClassInlinerEligibilityInfo.java b/src/main/java/com/android/tools/r8/ir/optimize/classinliner/ClassInlinerEligibilityInfo.java
index bf06bc1..8d178a3 100644
--- a/src/main/java/com/android/tools/r8/ir/optimize/classinliner/ClassInlinerEligibilityInfo.java
+++ b/src/main/java/com/android/tools/r8/ir/optimize/classinliner/ClassInlinerEligibilityInfo.java
@@ -21,9 +21,14 @@
*/
final OptionalBool returnsReceiver;
+ final boolean hasMonitorOnReceiver;
+
public ClassInlinerEligibilityInfo(
- List<Pair<Invoke.Type, DexMethod>> callsReceiver, OptionalBool returnsReceiver) {
+ List<Pair<Invoke.Type, DexMethod>> callsReceiver,
+ OptionalBool returnsReceiver,
+ boolean hasMonitorOnReceiver) {
this.callsReceiver = callsReceiver;
this.returnsReceiver = returnsReceiver;
+ this.hasMonitorOnReceiver = hasMonitorOnReceiver;
}
}
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 157e7b4..fe1b092 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
@@ -819,12 +819,16 @@
}
if (root.isStaticGet()) {
- // If we are class inlining a singleton instance from a static-get, then we don't the value of
- // the fields.
+ // If we are class inlining a singleton instance from a static-get, then we don't know the
+ // value of the fields.
ParameterUsage receiverUsage = optimizationInfo.getParameterUsages(0);
if (receiverUsage == null || receiverUsage.hasFieldRead) {
return null;
}
+ if (eligibility.hasMonitorOnReceiver) {
+ // We will not be able to remove the monitor instruction afterwards.
+ return null;
+ }
}
// If the method returns receiver and the return value is actually
@@ -977,6 +981,10 @@
}
}
+ if (parameterUsage.isUsedInMonitor) {
+ return !root.isStaticGet();
+ }
+
if (!Sets.difference(parameterUsage.ifZeroTest, ALLOWED_ZERO_TEST_TYPES).isEmpty()) {
// Used in unsupported zero-check-if kinds.
return false;
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/info/MethodOptimizationInfoCollector.java b/src/main/java/com/android/tools/r8/ir/optimize/info/MethodOptimizationInfoCollector.java
index 1fb2e0c..8238723 100644
--- a/src/main/java/com/android/tools/r8/ir/optimize/info/MethodOptimizationInfoCollector.java
+++ b/src/main/java/com/android/tools/r8/ir/optimize/info/MethodOptimizationInfoCollector.java
@@ -166,12 +166,14 @@
List<Pair<Invoke.Type, DexMethod>> callsReceiver = new ArrayList<>();
boolean seenSuperInitCall = false;
+ boolean seenMonitor = false;
for (Instruction insn : receiver.aliasedUsers()) {
if (insn.isAssume()) {
continue;
}
if (insn.isMonitor()) {
+ seenMonitor = true;
continue;
}
@@ -246,11 +248,15 @@
return;
}
+ boolean synchronizedVirtualMethod =
+ method.accessFlags.isSynchronized() && method.isVirtualMethod();
+
feedback.setClassInlinerEligibility(
method,
new ClassInlinerEligibilityInfo(
callsReceiver,
- new ClassInlinerReceiverAnalysis(appView, method, code).computeReturnsReceiver()));
+ new ClassInlinerReceiverAnalysis(appView, method, code).computeReturnsReceiver(),
+ seenMonitor || synchronizedVirtualMethod));
}
private void identifyParameterUsages(
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/info/ParameterUsagesInfo.java b/src/main/java/com/android/tools/r8/ir/optimize/info/ParameterUsagesInfo.java
index ef87dcf..e911307 100644
--- a/src/main/java/com/android/tools/r8/ir/optimize/info/ParameterUsagesInfo.java
+++ b/src/main/java/com/android/tools/r8/ir/optimize/info/ParameterUsagesInfo.java
@@ -12,6 +12,7 @@
import com.android.tools.r8.ir.code.Instruction;
import com.android.tools.r8.ir.code.Invoke;
import com.android.tools.r8.ir.code.InvokeMethodWithReceiver;
+import com.android.tools.r8.ir.code.Monitor;
import com.android.tools.r8.ir.code.Return;
import com.android.tools.r8.ir.code.Value;
import com.android.tools.r8.utils.ListUtils;
@@ -86,6 +87,9 @@
// If this argument is returned: return arg.
public final boolean isReturned;
+ // If this argument is used in a monitor instruction.
+ public final boolean isUsedInMonitor;
+
ParameterUsage(
int index,
Set<Type> ifZeroTest,
@@ -93,7 +97,8 @@
boolean hasFieldAssignment,
boolean hasFieldRead,
boolean isAssignedToField,
- boolean isReturned) {
+ boolean isReturned,
+ boolean isUsedInMonitor) {
this.index = index;
this.ifZeroTest =
ifZeroTest.isEmpty() ? Collections.emptySet() : ImmutableSet.copyOf(ifZeroTest);
@@ -103,6 +108,7 @@
this.hasFieldRead = hasFieldRead;
this.isAssignedToField = isAssignedToField;
this.isReturned = isReturned;
+ this.isUsedInMonitor = isUsedInMonitor;
}
static ParameterUsage copyAndShift(ParameterUsage original, int shift) {
@@ -114,7 +120,8 @@
original.hasFieldAssignment,
original.hasFieldRead,
original.isAssignedToField,
- original.isReturned);
+ original.isReturned,
+ original.isUsedInMonitor);
}
public boolean notUsed() {
@@ -123,7 +130,8 @@
&& !hasFieldAssignment
&& !hasFieldRead
&& !isAssignedToField
- && !isReturned;
+ && !isReturned
+ && !isUsedInMonitor;
}
}
@@ -138,6 +146,7 @@
private boolean hasFieldRead = false;
private boolean isAssignedToField = false;
private boolean isReturned = false;
+ private boolean isUsedInMonitor = false;
ParameterUsageBuilder(Value arg, int index) {
this.arg = arg;
@@ -166,6 +175,9 @@
if (instruction.isReturn()) {
return note(instruction.asReturn());
}
+ if (instruction.isMonitor()) {
+ return note(instruction.asMonitor());
+ }
return false;
}
@@ -177,7 +189,8 @@
hasFieldAssignment,
hasFieldRead,
isAssignedToField,
- isReturned);
+ isReturned,
+ isUsedInMonitor);
}
private boolean note(If ifInstruction) {
@@ -231,5 +244,12 @@
isReturned = true;
return true;
}
+
+ private boolean note(Monitor monitorInstruction) {
+ assert monitorInstruction.inValues().size() == 1;
+ assert monitorInstruction.inValues().get(0).getAliasedValue() == arg;
+ isUsedInMonitor = true;
+ return true;
+ }
}
}
diff --git a/src/test/java/com/android/tools/r8/ir/optimize/classinliner/ClassInlinerStaticGetDirectMonitorTest.java b/src/test/java/com/android/tools/r8/ir/optimize/classinliner/ClassInlinerStaticGetDirectMonitorTest.java
new file mode 100644
index 0000000..f6252ea
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/ir/optimize/classinliner/ClassInlinerStaticGetDirectMonitorTest.java
@@ -0,0 +1,114 @@
+// Copyright (c) 2020, 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.classinliner;
+
+import static org.junit.Assert.assertTrue;
+
+import com.android.tools.r8.NeverInline;
+import com.android.tools.r8.TestBase;
+import com.android.tools.r8.TestParameters;
+import com.android.tools.r8.TestParametersCollection;
+import com.android.tools.r8.utils.codeinspector.CodeInspector;
+import com.android.tools.r8.utils.codeinspector.InstructionSubject;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+import org.junit.runners.Parameterized.Parameters;
+
+/**
+ * Currently, the class-inliner will not inline if the root is used in a monitor. This test just
+ * ensures, that if that ever changes, the monitor instructions will not be removed.
+ */
+@RunWith(Parameterized.class)
+public class ClassInlinerStaticGetDirectMonitorTest extends TestBase {
+
+ private final TestParameters parameters;
+
+ @Parameters(name = "{0}")
+ public static TestParametersCollection data() {
+ return getTestParameters().withAllRuntimesAndApiLevels().build();
+ }
+
+ public ClassInlinerStaticGetDirectMonitorTest(TestParameters parameters) {
+ this.parameters = parameters;
+ }
+
+ @Test
+ public void test() throws Exception {
+ testForR8(parameters.getBackend())
+ .addInnerClasses(ClassInlinerStaticGetDirectMonitorTest.class)
+ .addKeepMainRule(TestClass.class)
+ .enableInliningAnnotations()
+ .setMinApi(parameters.getApiLevel())
+ .noMinification()
+ .compile()
+ .inspect(this::inspect)
+ .run(parameters.getRuntime(), TestClass.class)
+ .assertSuccessWithOutputLines("20000");
+ }
+
+ private void inspect(CodeInspector inspector) {
+ assertTrue(
+ inspector
+ .clazz(TestClass.class)
+ .uniqueMethodWithName("produce1")
+ .streamInstructions()
+ .anyMatch(InstructionSubject::isMonitorEnter));
+ assertTrue(
+ inspector
+ .clazz(TestClass.class)
+ .uniqueMethodWithName("produce2")
+ .streamInstructions()
+ .anyMatch(InstructionSubject::isMonitorExit));
+ }
+
+ static class TestClass {
+
+ private static volatile Thread t1 = new Thread(TestClass::produce1);
+ private static volatile Thread t2 = new Thread(TestClass::produce2);
+
+ @NeverInline
+ static void produce1() {
+ Container instance = Container.getInstance();
+ for (int i = 0; i < 10000; i++) {
+ synchronized (instance) {
+ instance.increment();
+ }
+ }
+ }
+
+ @NeverInline
+ static void produce2() {
+ Container instance = Container.getInstance();
+ for (int i = 0; i < 10000; i++) {
+ synchronized (instance) {
+ instance.increment();
+ }
+ }
+ }
+
+ public static void main(String[] args) {
+ t1.start();
+ t2.start();
+ while (t1.isAlive() || t2.isAlive()) {}
+ System.out.println(Container.counter);
+ }
+ }
+
+ static class Container {
+
+ static Container INSTANCE = new Container();
+ public static int counter = 0;
+
+ static Container getInstance() {
+ return INSTANCE;
+ }
+
+ @NeverInline
+ final void increment() {
+ counter += 1;
+ }
+ }
+}
diff --git a/src/test/java/com/android/tools/r8/ir/optimize/classinliner/ClassInlinerStaticGetExtraMethodMonitorTest.java b/src/test/java/com/android/tools/r8/ir/optimize/classinliner/ClassInlinerStaticGetExtraMethodMonitorTest.java
new file mode 100644
index 0000000..0e0b260
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/ir/optimize/classinliner/ClassInlinerStaticGetExtraMethodMonitorTest.java
@@ -0,0 +1,105 @@
+// Copyright (c) 2020, 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.classinliner;
+
+import static com.android.tools.r8.utils.codeinspector.Matchers.isPresent;
+import static org.hamcrest.MatcherAssert.assertThat;
+
+import com.android.tools.r8.NeverInline;
+import com.android.tools.r8.TestBase;
+import com.android.tools.r8.TestParameters;
+import com.android.tools.r8.TestParametersCollection;
+import com.android.tools.r8.utils.codeinspector.CodeInspector;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+import org.junit.runners.Parameterized.Parameters;
+
+/**
+ * This is a reproduction of b/147411673 where we inline classes and remove monitor instructions.
+ */
+@RunWith(Parameterized.class)
+public class ClassInlinerStaticGetExtraMethodMonitorTest extends TestBase {
+
+ private final TestParameters parameters;
+
+ @Parameters(name = "{0}")
+ public static TestParametersCollection data() {
+ return getTestParameters().withAllRuntimesAndApiLevels().build();
+ }
+
+ public ClassInlinerStaticGetExtraMethodMonitorTest(TestParameters parameters) {
+ this.parameters = parameters;
+ }
+
+ @Test
+ public void test() throws Exception {
+ testForR8(parameters.getBackend())
+ .addInnerClasses(ClassInlinerStaticGetExtraMethodMonitorTest.class)
+ .addKeepMainRule(TestClass.class)
+ .enableInliningAnnotations()
+ .setMinApi(parameters.getApiLevel())
+ .noMinification()
+ .compile()
+ .inspect(this::inspect)
+ .run(parameters.getRuntime(), TestClass.class)
+ .assertSuccessWithOutputLines("20000");
+ }
+
+ private void inspect(CodeInspector inspector) {
+ assertThat(inspector.clazz(Container.class).uniqueMethodWithName("increment"), isPresent());
+ }
+
+ static class TestClass {
+
+ private static volatile Thread t1 = new Thread(TestClass::produce1);
+ private static volatile Thread t2 = new Thread(TestClass::produce2);
+
+ @NeverInline
+ static void produce1() {
+ Container instance = Container.getInstance();
+ for (int i = 0; i < 10000; i++) {
+ synchronizeOnExtraMethod(instance);
+ }
+ }
+
+ @NeverInline
+ static void produce2() {
+ Container instance = Container.getInstance();
+ for (int i = 0; i < 10000; i++) {
+ synchronizeOnExtraMethod(instance);
+ }
+ }
+
+ @NeverInline
+ static void synchronizeOnExtraMethod(Container container) {
+ synchronized (container) {
+ container.increment();
+ }
+ }
+
+ public static void main(String[] args) {
+ t1.start();
+ t2.start();
+ while (t1.isAlive() || t2.isAlive()) {}
+ System.out.println(Container.counter);
+ }
+ }
+
+ static class Container {
+
+ static Container INSTANCE = new Container();
+ public static int counter = 0;
+
+ static Container getInstance() {
+ return INSTANCE;
+ }
+
+ @NeverInline
+ final void increment() {
+ counter += 1;
+ }
+ }
+}
diff --git a/src/test/java/com/android/tools/r8/ir/optimize/classinliner/ClassInlinerStaticGetMonitorTest.java b/src/test/java/com/android/tools/r8/ir/optimize/classinliner/ClassInlinerStaticGetMonitorTest.java
new file mode 100644
index 0000000..216f34e
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/ir/optimize/classinliner/ClassInlinerStaticGetMonitorTest.java
@@ -0,0 +1,98 @@
+// 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.ir.optimize.classinliner;
+
+import static com.android.tools.r8.utils.codeinspector.Matchers.isPresent;
+import static org.hamcrest.MatcherAssert.assertThat;
+
+import com.android.tools.r8.NeverInline;
+import com.android.tools.r8.TestBase;
+import com.android.tools.r8.TestParameters;
+import com.android.tools.r8.TestParametersCollection;
+import com.android.tools.r8.utils.codeinspector.CodeInspector;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+import org.junit.runners.Parameterized.Parameters;
+
+/**
+ * This is a reproduction of b/147411673 where we inline classes and remove monitor instructions.
+ */
+@RunWith(Parameterized.class)
+public class ClassInlinerStaticGetMonitorTest extends TestBase {
+
+ private final TestParameters parameters;
+
+ @Parameters(name = "{0}")
+ public static TestParametersCollection data() {
+ return getTestParameters().withAllRuntimesAndApiLevels().build();
+ }
+
+ public ClassInlinerStaticGetMonitorTest(TestParameters parameters) {
+ this.parameters = parameters;
+ }
+
+ @Test
+ public void test() throws Exception {
+ testForR8(parameters.getBackend())
+ .addInnerClasses(ClassInlinerStaticGetMonitorTest.class)
+ .addKeepMainRule(TestClass.class)
+ .enableInliningAnnotations()
+ .setMinApi(parameters.getApiLevel())
+ .noMinification()
+ .compile()
+ .inspect(this::inspect)
+ .run(parameters.getRuntime(), TestClass.class)
+ .assertSuccessWithOutputLines("20000");
+ }
+
+ private void inspect(CodeInspector inspector) {
+ assertThat(inspector.clazz(Container.class).uniqueMethodWithName("increment"), isPresent());
+ }
+
+ static class TestClass {
+
+ private static volatile Thread t1 = new Thread(TestClass::produce1);
+ private static volatile Thread t2 = new Thread(TestClass::produce2);
+
+ @NeverInline
+ static void produce1() {
+ for (int i = 0; i < 10000; i++) {
+ Container.getInstance().increment();
+ }
+ }
+
+ @NeverInline
+ static void produce2() {
+ for (int i = 0; i < 10000; i++) {
+ Container.getInstance().increment();
+ }
+ }
+
+ public static void main(String[] args) {
+ t1.start();
+ t2.start();
+ while (t1.isAlive() || t2.isAlive()) {}
+ System.out.println(Container.counter);
+ }
+ }
+
+ static class Container {
+
+ static Container INSTANCE = new Container();
+ public static int counter = 0;
+
+ static Container getInstance() {
+ return INSTANCE;
+ }
+
+ @NeverInline
+ final void increment() {
+ synchronized (this) {
+ counter += 1;
+ }
+ }
+ }
+}
