Reland "Find set of lock-candidates from const-class to disallow in merging"
This reverts commit 86e719c481d4dab3b1fbf329c83a22e405075c4d.
Bug: 142438687
Change-Id: I0521c64c85a8401fd34f422f4ccc13371ab69952
diff --git a/src/main/java/com/android/tools/r8/shaking/AppInfoWithLiveness.java b/src/main/java/com/android/tools/r8/shaking/AppInfoWithLiveness.java
index d061974..829c227 100644
--- a/src/main/java/com/android/tools/r8/shaking/AppInfoWithLiveness.java
+++ b/src/main/java/com/android/tools/r8/shaking/AppInfoWithLiveness.java
@@ -153,6 +153,8 @@
public final Set<DexType> neverClassInline;
/** All types that *must* never be merged due to a configuration directive (testing only). */
public final Set<DexType> neverMerge;
+ /** Set of const-class references. */
+ public final Set<DexType> constClassReferences;
/**
* All methods and fields whose value *must* never be propagated due to a configuration directive.
* (testing only).
@@ -223,7 +225,8 @@
Set<DexType> prunedTypes,
Map<DexField, Int2ReferenceMap<DexField>> switchMaps,
Map<DexType, Map<DexField, EnumValueInfo>> enumValueInfoMaps,
- Set<DexType> instantiatedLambdas) {
+ Set<DexType> instantiatedLambdas,
+ Set<DexType> constClassReferences) {
super(application);
this.liveTypes = liveTypes;
this.instantiatedAnnotationTypes = instantiatedAnnotationTypes;
@@ -264,6 +267,7 @@
this.switchMaps = switchMaps;
this.enumValueInfoMaps = enumValueInfoMaps;
this.instantiatedLambdas = instantiatedLambdas;
+ this.constClassReferences = constClassReferences;
}
public AppInfoWithLiveness(
@@ -304,7 +308,8 @@
Set<DexType> prunedTypes,
Map<DexField, Int2ReferenceMap<DexField>> switchMaps,
Map<DexType, Map<DexField, EnumValueInfo>> enumValueInfoMaps,
- Set<DexType> instantiatedLambdas) {
+ Set<DexType> instantiatedLambdas,
+ Set<DexType> constClassReferences) {
super(appInfoWithSubtyping);
this.liveTypes = liveTypes;
this.instantiatedAnnotationTypes = instantiatedAnnotationTypes;
@@ -345,6 +350,7 @@
this.switchMaps = switchMaps;
this.enumValueInfoMaps = enumValueInfoMaps;
this.instantiatedLambdas = instantiatedLambdas;
+ this.constClassReferences = constClassReferences;
}
private AppInfoWithLiveness(AppInfoWithLiveness previous) {
@@ -398,7 +404,8 @@
: CollectionUtils.mergeSets(previous.prunedTypes, removedClasses),
previous.switchMaps,
previous.enumValueInfoMaps,
- previous.instantiatedLambdas);
+ previous.instantiatedLambdas,
+ previous.constClassReferences);
copyMetadataFromPrevious(previous);
assert removedClasses == null || assertNoItemRemoved(previous.pinnedItems, removedClasses);
}
@@ -485,6 +492,7 @@
.collect(Collectors.toList()));
this.switchMaps = rewriteReferenceKeys(previous.switchMaps, lense::lookupField);
this.enumValueInfoMaps = rewriteReferenceKeys(previous.enumValueInfoMaps, lense::lookupType);
+ this.constClassReferences = previous.constClassReferences;
}
public AppInfoWithLiveness(
@@ -531,6 +539,7 @@
this.prunedTypes = previous.prunedTypes;
this.switchMaps = switchMaps;
this.enumValueInfoMaps = enumValueInfoMaps;
+ this.constClassReferences = previous.constClassReferences;
previous.markObsolete();
}
@@ -592,6 +601,16 @@
return interfaces;
}
+ /**
+ * Const-classes is a conservative set of types that may be lock-candidates and cannot be merged.
+ * When using synchronized blocks, we cannot ensure that const-class locks will not flow in. This
+ * can potentially cause incorrect behavior when merging classes. A conservative choice is to not
+ * merge any const-class classes. More info at b/142438687.
+ */
+ public boolean isLockCandidate(DexType type) {
+ return constClassReferences.contains(type);
+ }
+
public AppInfoWithLiveness withoutStaticFieldsWrites(Set<DexField> noLongerWrittenFields) {
assert checkIfObsolete();
if (noLongerWrittenFields.isEmpty()) {
diff --git a/src/main/java/com/android/tools/r8/shaking/Enqueuer.java b/src/main/java/com/android/tools/r8/shaking/Enqueuer.java
index 24686b5..2306b28 100644
--- a/src/main/java/com/android/tools/r8/shaking/Enqueuer.java
+++ b/src/main/java/com/android/tools/r8/shaking/Enqueuer.java
@@ -273,6 +273,12 @@
private final Set<DexReference> pinnedItems = Sets.newIdentityHashSet();
/**
+ * A set of seen const-class references that both serve as an initial lock-candidate set and will
+ * prevent statically merging the classes referenced.
+ */
+ private final Set<DexType> constClassReferences = Sets.newIdentityHashSet();
+
+ /**
* A map from classes to annotations that need to be processed should the classes ever become
* live.
*/
@@ -899,6 +905,15 @@
@Override
public boolean registerConstClass(DexType type) {
+ // We conservatively group T.class and T[].class to ensure that we do not merge T with S if
+ // potential locks on T[].class and S[].class exists.
+ DexType baseType = type.toBaseType(appView.dexItemFactory());
+ if (baseType.isClassType()) {
+ DexProgramClass baseClass = getProgramClassOrNull(baseType);
+ if (baseClass != null) {
+ constClassReferences.add(baseType);
+ }
+ }
return registerConstClassOrCheckCast(type);
}
@@ -2241,7 +2256,8 @@
Collections.emptyMap(),
Collections.emptyMap(),
SetUtils.mapIdentityHashSet(
- instantiatedInterfaceTypes.getItems(), DexProgramClass::getType));
+ instantiatedInterfaceTypes.getItems(), DexProgramClass::getType),
+ constClassReferences);
appInfo.markObsolete();
return appInfoWithLiveness;
}
diff --git a/src/main/java/com/android/tools/r8/shaking/StaticClassMerger.java b/src/main/java/com/android/tools/r8/shaking/StaticClassMerger.java
index 9326e31..bae5b40 100644
--- a/src/main/java/com/android/tools/r8/shaking/StaticClassMerger.java
+++ b/src/main/java/com/android/tools/r8/shaking/StaticClassMerger.java
@@ -342,6 +342,11 @@
// We are not allowed to merge synchronized classes with synchronized methods.
return false;
}
+ if (appView.appInfo().constClassReferences.contains(clazz.type)) {
+ // Since the type is const-class referenced (and the static merger does not create a lense
+ // to map the merged type) the class will likely remain and there is no gain from merging.
+ return false;
+ }
// Check if current candidate is a better choice depending on visibility. For package private
// or protected, the key is parameterized by the package name already, so we just have to
// check accessibility-flags. For global this is no-op.
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 159ca22..85da345 100644
--- a/src/main/java/com/android/tools/r8/shaking/VerticalClassMerger.java
+++ b/src/main/java/com/android/tools/r8/shaking/VerticalClassMerger.java
@@ -139,8 +139,8 @@
RESOLUTION_FOR_FIELDS_MAY_CHANGE,
RESOLUTION_FOR_METHODS_MAY_CHANGE,
SERVICE_LOADER,
+ SOURCE_AND_TARGET_LOCK_CANDIDATES,
STATIC_INITIALIZERS,
- STATIC_SYNCHRONIZED_METHODS,
UNHANDLED_INVOKE_DIRECT,
UNHANDLED_INVOKE_SUPER,
UNSAFE_INLINING,
@@ -189,6 +189,9 @@
case SERVICE_LOADER:
message = "it is used by a service loader";
break;
+ case SOURCE_AND_TARGET_LOCK_CANDIDATES:
+ message = "source and target are both lock-candidates";
+ break;
case STATIC_INITIALIZERS:
message = "merging of static initializers are not supported";
break;
@@ -204,9 +207,6 @@
case UNSUPPORTED_ATTRIBUTES:
message = "since inner-class attributes are not supported";
break;
- case STATIC_SYNCHRONIZED_METHODS:
- message = "since it has static synchronized methods which can lead to unwanted behavior";
- break;
default:
assert false;
}
@@ -480,6 +480,17 @@
}
return false;
}
+ boolean sourceCanBeSynchronizedOn =
+ appView.appInfo().isLockCandidate(clazz.type) || clazz.hasStaticSynchronizedMethods();
+ boolean targetCanBeSynchronizedOn =
+ appView.appInfo().isLockCandidate(targetClass.type)
+ || targetClass.hasStaticSynchronizedMethods();
+ if (sourceCanBeSynchronizedOn && targetCanBeSynchronizedOn) {
+ if (Log.ENABLED) {
+ AbortReason.SOURCE_AND_TARGET_LOCK_CANDIDATES.printLogMessageForClass(clazz);
+ }
+ return false;
+ }
if (targetClass.getEnclosingMethod() != null || !targetClass.getInnerClasses().isEmpty()) {
// TODO(herhut): Consider supporting merging of enclosing-method and inner-class attributes.
if (Log.ENABLED) {
@@ -817,14 +828,6 @@
assert isStillMergeCandidate(clazz);
}
- // Check for static synchronized methods on source
- if (clazz.hasStaticSynchronizedMethods()) {
- if (Log.ENABLED) {
- AbortReason.STATIC_SYNCHRONIZED_METHODS.printLogMessageForClass(clazz);
- }
- return;
- }
-
// Guard against the case where we have two methods that may get the same signature
// if we replace types. This is rare, so we approximate and err on the safe side here.
if (new CollisionDetector(clazz.type, targetClass.type).mayCollide()) {
diff --git a/src/test/java/com/android/tools/r8/classmerging/VerticalClassMergerSynchronizedBlockTest.java b/src/test/java/com/android/tools/r8/classmerging/VerticalClassMergerSynchronizedBlockTest.java
index 9b3c384..2828ec4 100644
--- a/src/test/java/com/android/tools/r8/classmerging/VerticalClassMergerSynchronizedBlockTest.java
+++ b/src/test/java/com/android/tools/r8/classmerging/VerticalClassMergerSynchronizedBlockTest.java
@@ -6,10 +6,8 @@
import static com.android.tools.r8.utils.codeinspector.Matchers.isPresent;
import static org.hamcrest.MatcherAssert.assertThat;
-import static org.hamcrest.core.StringContains.containsString;
import com.android.tools.r8.CompilationFailedException;
-import com.android.tools.r8.R8TestRunResult;
import com.android.tools.r8.TestBase;
import com.android.tools.r8.TestParameters;
import com.android.tools.r8.TestParametersCollection;
@@ -51,18 +49,13 @@
@Test
public void testNoMergingOfClassUsedInMonitor()
throws IOException, CompilationFailedException, ExecutionException {
- // TODO(b/142438687): Fix expectation when fixed.
- R8TestRunResult result =
- testForR8(parameters.getBackend())
- .addInnerClasses(VerticalClassMergerSynchronizedBlockTest.class)
- .addKeepMainRule(Main.class)
- .setMinApi(parameters.getApiLevel())
- .compile()
- .run(parameters.getRuntime(), Main.class)
- .assertFailureWithErrorThatMatches(containsString("DEADLOCKED!"));
- if (result.getExitCode() == 0) {
- result.inspect(inspector -> assertThat(inspector.clazz(LockOne.class), isPresent()));
- }
+ testForR8(parameters.getBackend())
+ .addInnerClasses(VerticalClassMergerSynchronizedBlockTest.class)
+ .addKeepMainRule(Main.class)
+ .setMinApi(parameters.getApiLevel())
+ .run(parameters.getRuntime(), Main.class)
+ .assertSuccessWithOutput("Hello World!")
+ .inspect(inspector -> assertThat(inspector.clazz(LockOne.class), isPresent()));
}
abstract static class LockOne {}
diff --git a/src/test/java/com/android/tools/r8/classmerging/VerticalClassMergerSynchronizedBlockWithArraysTest.java b/src/test/java/com/android/tools/r8/classmerging/VerticalClassMergerSynchronizedBlockWithArraysTest.java
new file mode 100644
index 0000000..b6039db
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/classmerging/VerticalClassMergerSynchronizedBlockWithArraysTest.java
@@ -0,0 +1,121 @@
+// 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.classmerging;
+
+import static com.android.tools.r8.utils.codeinspector.Matchers.isPresent;
+import static org.hamcrest.MatcherAssert.assertThat;
+
+import com.android.tools.r8.CompilationFailedException;
+import com.android.tools.r8.TestBase;
+import com.android.tools.r8.TestParameters;
+import com.android.tools.r8.TestParametersCollection;
+import com.android.tools.r8.ToolHelper.DexVm.Version;
+import java.io.IOException;
+import java.lang.Thread.State;
+import java.util.concurrent.ExecutionException;
+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 VerticalClassMergerSynchronizedBlockWithArraysTest extends TestBase {
+
+ private final TestParameters parameters;
+
+ @Parameters(name = "{0}")
+ public static TestParametersCollection data() {
+ // The 4.0.4 runtime will flakily mark threads as blocking and report DEADLOCKED.
+ return getTestParameters()
+ .withDexRuntimesStartingFromExcluding(Version.V4_0_4)
+ .withAllApiLevels()
+ .build();
+ }
+
+ public VerticalClassMergerSynchronizedBlockWithArraysTest(TestParameters parameters) {
+ this.parameters = parameters;
+ }
+
+ @Test
+ public void testOnRuntime() throws IOException, CompilationFailedException, ExecutionException {
+ testForRuntime(parameters.getRuntime(), parameters.getApiLevel())
+ .addInnerClasses(VerticalClassMergerSynchronizedBlockWithArraysTest.class)
+ .run(parameters.getRuntime(), Main.class)
+ .assertSuccessWithOutput("Hello World!");
+ }
+
+ @Test
+ public void testNoMergingOfClassUsedInMonitor()
+ throws IOException, CompilationFailedException, ExecutionException {
+ testForR8(parameters.getBackend())
+ .addInnerClasses(VerticalClassMergerSynchronizedBlockWithArraysTest.class)
+ .addKeepMainRule(Main.class)
+ .setMinApi(parameters.getApiLevel())
+ .run(parameters.getRuntime(), Main.class)
+ .assertSuccessWithOutput("Hello World!")
+ .inspect(inspector -> assertThat(inspector.clazz(LockOne.class), isPresent()));
+ }
+
+ abstract static class LockOne {}
+
+ static class LockTwo extends LockOne {}
+
+ static class LockThree {}
+
+ public static class Main {
+
+ private static volatile boolean inLockThreeCritical = false;
+ private static volatile boolean inLockTwoCritical = false;
+ private static volatile boolean arnoldWillNotBeBack = false;
+
+ private static volatile Thread t1 = new Thread(Main::lockThreeThenOne);
+ private static volatile Thread t2 = new Thread(Main::lockTwoThenThree);
+ private static volatile Thread t3 = new Thread(Main::arnold);
+
+ static void synchronizedAccessThroughLocks(String arg) {
+ System.out.print(arg);
+ }
+
+ public static void main(String[] args) {
+ t1.start();
+ t2.start();
+ // This thread is started to ensure termination in case we are rewriting incorrectly.
+ t3.start();
+
+ while (!arnoldWillNotBeBack) {}
+ }
+
+ static void lockThreeThenOne() {
+ synchronized (LockThree[].class) {
+ inLockThreeCritical = true;
+ while (!inLockTwoCritical) {}
+ synchronized (LockOne[].class) {
+ synchronizedAccessThroughLocks("Hello ");
+ }
+ }
+ }
+
+ static void lockTwoThenThree() {
+ synchronized (LockTwo[].class) {
+ inLockTwoCritical = true;
+ while (!inLockThreeCritical) {}
+ synchronized (LockThree[].class) {
+ synchronizedAccessThroughLocks("World!");
+ }
+ }
+ }
+
+ static void arnold() {
+ while (t1.getState() != State.TERMINATED || t2.getState() != State.TERMINATED) {
+ if (t1.getState() == State.BLOCKED && t2.getState() == State.BLOCKED) {
+ System.err.println("DEADLOCKED!");
+ System.exit(1);
+ break;
+ }
+ }
+ arnoldWillNotBeBack = true;
+ }
+ }
+}
diff --git a/src/test/java/com/android/tools/r8/kotlin/SimplifyIfNotNullKotlinTest.java b/src/test/java/com/android/tools/r8/kotlin/SimplifyIfNotNullKotlinTest.java
index 59aae43..b9ee37a 100644
--- a/src/test/java/com/android/tools/r8/kotlin/SimplifyIfNotNullKotlinTest.java
+++ b/src/test/java/com/android/tools/r8/kotlin/SimplifyIfNotNullKotlinTest.java
@@ -32,7 +32,10 @@
final String mainClassName = ex1.getClassName();
final String extraRules =
keepMainMethod(mainClassName) + neverInlineMethod(mainClassName, testMethodSignature);
- runTest(FOLDER, mainClassName, extraRules,
+ runTest(
+ FOLDER,
+ mainClassName,
+ extraRules,
app -> {
CodeInspector codeInspector = new CodeInspector(app);
ClassSubject clazz = checkClassIsKept(codeInspector, ex1.getClassName());
@@ -40,8 +43,7 @@
MethodSubject testMethod = checkMethodIsKept(clazz, testMethodSignature);
long ifzCount =
testMethod.streamInstructions().filter(i -> i.isIfEqz() || i.isIfNez()).count();
- long paramNullCheckCount =
- countCall(testMethod, "ArrayIteratorKt", "checkParameterIsNotNull");
+ long paramNullCheckCount = countCall(testMethod, "Intrinsics", "checkParameterIsNotNull");
// One after Iterator#hasNext, and another in the filter predicate: sinceYear != null.
assertEquals(2, ifzCount);
assertEquals(allowAccessModification ? 0 : 5, paramNullCheckCount);
