Reland "Treat static-puts that store objects with a non-default finalize() method as having side effects"
This reverts commit d9c5a2c17319f26cdc08ac79e190eddcbae15cd7.
Bug: 137836288
Change-Id: I9f03c7cf0ce4410efb208596034274a2a5c33340
diff --git a/src/main/java/com/android/tools/r8/graph/AppInfoWithSubtyping.java b/src/main/java/com/android/tools/r8/graph/AppInfoWithSubtyping.java
index 2e37930..9befcc6 100644
--- a/src/main/java/com/android/tools/r8/graph/AppInfoWithSubtyping.java
+++ b/src/main/java/com/android/tools/r8/graph/AppInfoWithSubtyping.java
@@ -3,6 +3,7 @@
// BSD-style license that can be found in the LICENSE file.
package com.android.tools.r8.graph;
+import com.android.tools.r8.OptionalBool;
import com.android.tools.r8.errors.CompilationError;
import com.android.tools.r8.ir.desugar.LambdaDescriptor;
import com.android.tools.r8.origin.Origin;
@@ -34,7 +35,8 @@
private static final Set<DexType> NO_DIRECT_SUBTYPE = ImmutableSet.of();
private static class TypeInfo {
- final DexType type;
+
+ private final DexType type;
int hierarchyLevel = UNKNOWN_LEVEL;
/**
@@ -46,6 +48,10 @@
// Caching what interfaces this type is implementing. This includes super-interface hierarchy.
Set<DexType> implementedInterfaces = null;
+ // Caches which static types that may store an object that has a non-default finalize() method.
+ // E.g., `java.lang.Object -> TRUE` if there is a subtype of Object that overrides finalize().
+ Map<DexType, Boolean> mayHaveFinalizeMethodDirectlyOrIndirectlyCache = new IdentityHashMap<>();
+
public TypeInfo(DexType type) {
this.type = type;
}
@@ -87,7 +93,7 @@
setLevel(ROOT_LEVEL);
}
- void tagAsInteface() {
+ void tagAsInterface() {
setLevel(INTERFACE_LEVEL);
}
@@ -108,6 +114,17 @@
ensureDirectSubTypeSet();
directSubtypes.add(type);
}
+
+ synchronized OptionalBool mayHaveFinalizeMethodDirectlyOrIndirectly() {
+ Boolean cache = mayHaveFinalizeMethodDirectlyOrIndirectlyCache.get(type);
+ return cache != null ? OptionalBool.of(cache.booleanValue()) : OptionalBool.unknown();
+ }
+
+ synchronized boolean cacheMayHaveFinalizeMethodDirectlyOrIndirectly(boolean value) {
+ assert mayHaveFinalizeMethodDirectlyOrIndirectlyCache.getOrDefault(type, value) == value;
+ mayHaveFinalizeMethodDirectlyOrIndirectlyCache.put(type, value);
+ return value;
+ }
}
// Set of missing classes, discovered during subtypeMap computation.
@@ -225,7 +242,7 @@
getTypeInfo(inter).addInterfaceSubtype(holder);
}
if (holderClass.isInterface()) {
- getTypeInfo(holder).tagAsInteface();
+ getTypeInfo(holder).tagAsInterface();
}
} else {
if (baseClass.isProgramClass() || baseClass.isClasspathClass()) {
@@ -708,4 +725,41 @@
public boolean inDifferentHierarchy(DexType type1, DexType type2) {
return !isSubtype(type1, type2) && !isSubtype(type2, type1);
}
+
+ public boolean mayHaveFinalizeMethodDirectlyOrIndirectly(DexType type) {
+ return computeMayHaveFinalizeMethodDirectlyOrIndirectlyIfAbsent(type, true);
+ }
+
+ private boolean computeMayHaveFinalizeMethodDirectlyOrIndirectlyIfAbsent(
+ DexType type, boolean lookUpwards) {
+ assert type.isClassType();
+ TypeInfo typeInfo = getTypeInfo(type);
+ OptionalBool result = typeInfo.mayHaveFinalizeMethodDirectlyOrIndirectly();
+ if (!result.isUnknown()) {
+ return result.isTrue();
+ }
+ DexClass clazz = definitionFor(type);
+ if (clazz == null) {
+ return typeInfo.cacheMayHaveFinalizeMethodDirectlyOrIndirectly(true);
+ }
+ if (clazz.isProgramClass()) {
+ if (lookUpwards) {
+ DexEncodedMethod resolutionResult =
+ resolveMethod(type, dexItemFactory().objectMethods.finalize).asSingleTarget();
+ if (resolutionResult != null && resolutionResult.isProgramMethod(this)) {
+ return typeInfo.cacheMayHaveFinalizeMethodDirectlyOrIndirectly(true);
+ }
+ } else {
+ if (clazz.lookupVirtualMethod(dexItemFactory().objectMethods.finalize) != null) {
+ return typeInfo.cacheMayHaveFinalizeMethodDirectlyOrIndirectly(true);
+ }
+ }
+ }
+ for (DexType subtype : allImmediateSubtypes(type)) {
+ if (computeMayHaveFinalizeMethodDirectlyOrIndirectlyIfAbsent(subtype, false)) {
+ return typeInfo.cacheMayHaveFinalizeMethodDirectlyOrIndirectly(true);
+ }
+ }
+ return typeInfo.cacheMayHaveFinalizeMethodDirectlyOrIndirectly(false);
+ }
}
diff --git a/src/main/java/com/android/tools/r8/ir/code/StaticPut.java b/src/main/java/com/android/tools/r8/ir/code/StaticPut.java
index 6db965b..5d8fb67 100644
--- a/src/main/java/com/android/tools/r8/ir/code/StaticPut.java
+++ b/src/main/java/com/android/tools/r8/ir/code/StaticPut.java
@@ -15,12 +15,16 @@
import com.android.tools.r8.dex.Constants;
import com.android.tools.r8.errors.Unreachable;
import com.android.tools.r8.graph.AppView;
+import com.android.tools.r8.graph.DexClass;
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.DexItemFactory;
import com.android.tools.r8.graph.DexType;
import com.android.tools.r8.ir.analysis.ClassInitializationAnalysis;
import com.android.tools.r8.ir.analysis.ClassInitializationAnalysis.AnalysisAssumption;
import com.android.tools.r8.ir.analysis.ClassInitializationAnalysis.Query;
+import com.android.tools.r8.ir.analysis.type.TypeLatticeElement;
import com.android.tools.r8.ir.conversion.CfBuilder;
import com.android.tools.r8.ir.conversion.DexBuilder;
import com.android.tools.r8.ir.optimize.Inliner.ConstraintWithTarget;
@@ -120,13 +124,48 @@
return false;
}
- return appInfoWithLiveness.isFieldRead(encodedField);
+ return appInfoWithLiveness.isFieldRead(encodedField)
+ || isStoringObjectWithFinalizer(appInfoWithLiveness);
}
// In D8, we always have to assume that the field can be read, and thus have side effects.
return true;
}
+ /**
+ * Returns {@code true} if this instruction may store a value that has a non-default finalize()
+ * method in a static field. In that case, it is not safe to remove this instruction, since that
+ * could change the lifetime of the value.
+ */
+ private boolean isStoringObjectWithFinalizer(AppInfoWithLiveness appInfo) {
+ TypeLatticeElement type = value().getTypeLattice();
+ TypeLatticeElement baseType =
+ type.isArrayType() ? type.asArrayTypeLatticeElement().getArrayBaseTypeLattice() : type;
+ if (baseType.isClassType()) {
+ Value root = value().getAliasedValue();
+ if (!root.isPhi() && root.definition.isNewInstance()) {
+ DexClass clazz = appInfo.definitionFor(root.definition.asNewInstance().clazz);
+ if (clazz == null) {
+ return true;
+ }
+ if (clazz.superType == null) {
+ return false;
+ }
+ DexItemFactory dexItemFactory = appInfo.dexItemFactory();
+ DexEncodedMethod resolutionResult =
+ appInfo
+ .resolveMethod(clazz.type, dexItemFactory.objectMethods.finalize)
+ .asSingleTarget();
+ return resolutionResult != null && resolutionResult.isProgramMethod(appInfo);
+ }
+
+ return appInfo.mayHaveFinalizeMethodDirectlyOrIndirectly(
+ baseType.asClassTypeLatticeElement().getClassType());
+ }
+
+ return false;
+ }
+
@Override
public boolean canBeDeadCode(AppView<?> appView, IRCode code) {
// static-put can be dead as long as it cannot have any of the following:
diff --git a/src/test/java/com/android/tools/r8/ir/analysis/sideeffect/PutObjectWithFinalizeTest.java b/src/test/java/com/android/tools/r8/ir/analysis/sideeffect/PutObjectWithFinalizeTest.java
new file mode 100644
index 0000000..d133adf
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/ir/analysis/sideeffect/PutObjectWithFinalizeTest.java
@@ -0,0 +1,151 @@
+// 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.analysis.sideeffect;
+
+import static com.android.tools.r8.utils.codeinspector.Matchers.isPresent;
+import static org.hamcrest.CoreMatchers.not;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.junit.Assert.assertTrue;
+
+import com.android.tools.r8.NeverInline;
+import com.android.tools.r8.NeverMerge;
+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.ClassSubject;
+import com.android.tools.r8.utils.codeinspector.FieldSubject;
+import com.android.tools.r8.utils.codeinspector.InstructionSubject;
+import com.android.tools.r8.utils.codeinspector.MethodSubject;
+import com.google.common.collect.ImmutableList;
+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 PutObjectWithFinalizeTest extends TestBase {
+
+ private final TestParameters parameters;
+
+ @Parameters(name = "{0}")
+ public static TestParametersCollection data() {
+ return getTestParameters().withAllRuntimes().build();
+ }
+
+ public PutObjectWithFinalizeTest(TestParameters parameters) {
+ this.parameters = parameters;
+ }
+
+ @Test
+ public void test() throws Exception {
+ testForR8(parameters.getBackend())
+ .addInnerClasses(PutObjectWithFinalizeTest.class)
+ .addKeepMainRule(TestClass.class)
+ // The class staticizer does not consider the finalize() method.
+ .addOptionsModification(options -> options.enableClassStaticizer = false)
+ .enableInliningAnnotations()
+ .enableMergeAnnotations()
+ .setMinApi(parameters.getRuntime())
+ .compile()
+ .inspect(
+ inspector -> {
+ ClassSubject classSubject = inspector.clazz(TestClass.class);
+ assertThat(classSubject, isPresent());
+
+ MethodSubject mainSubject = classSubject.clinit();
+ assertThat(mainSubject, isPresent());
+
+ List<String> presentFields =
+ ImmutableList.of(
+ "directInstanceWithDirectFinalizer",
+ "directInstanceWithIndirectFinalizer",
+ "indirectInstanceWithDirectFinalizer",
+ "indirectInstanceWithIndirectFinalizer",
+ "otherIndirectInstanceWithoutFinalizer",
+ "arrayWithDirectFinalizer",
+ "arrayWithIndirectFinalizer",
+ "otherArrayInstanceWithoutFinalizer");
+ for (String name : presentFields) {
+ FieldSubject fieldSubject = classSubject.uniqueFieldWithName(name);
+ assertThat(fieldSubject, isPresent());
+ assertTrue(
+ mainSubject
+ .streamInstructions()
+ .filter(InstructionSubject::isStaticPut)
+ .map(InstructionSubject::getField)
+ .map(field -> field.name.toSourceString())
+ .anyMatch(fieldSubject.getFinalName()::equals));
+ }
+
+ List<String> absentFields =
+ ImmutableList.of(
+ "directInstanceWithoutFinalizer",
+ "otherDirectInstanceWithoutFinalizer",
+ "indirectInstanceWithoutFinalizer",
+ "arrayWithoutFinalizer");
+ for (String name : absentFields) {
+ assertThat(classSubject.uniqueFieldWithName(name), not(isPresent()));
+ }
+ })
+ .run(parameters.getRuntime(), TestClass.class)
+ .assertSuccessWithOutputLines("Hello world!");
+ }
+
+ static class TestClass {
+
+ static A directInstanceWithDirectFinalizer = new A();
+ static A directInstanceWithIndirectFinalizer = new B();
+ static Object directInstanceWithoutFinalizer = new C();
+ static Object otherDirectInstanceWithoutFinalizer = new Object();
+
+ static A indirectInstanceWithDirectFinalizer = createA();
+ static A indirectInstanceWithIndirectFinalizer = createB();
+ static Object indirectInstanceWithoutFinalizer = createC();
+ static Object otherIndirectInstanceWithoutFinalizer = createObject();
+
+ static A[] arrayWithDirectFinalizer = new A[42];
+ static A[] arrayWithIndirectFinalizer = new B[42];
+ static Object[] arrayWithoutFinalizer = new C[42];
+ static Object[] otherArrayInstanceWithoutFinalizer = new Object[42];
+
+ public static void main(String[] args) {
+ System.out.println("Hello world!");
+ }
+
+ @NeverInline
+ static A createA() {
+ return new A();
+ }
+
+ @NeverInline
+ static B createB() {
+ return new B();
+ }
+
+ @NeverInline
+ static C createC() {
+ return new C();
+ }
+
+ @NeverInline
+ static Object createObject() {
+ return new Object();
+ }
+ }
+
+ @NeverMerge
+ static class A {
+
+ @Override
+ public void finalize() {
+ System.out.println("Finalize!");
+ }
+ }
+
+ static class B extends A {}
+
+ static class C {}
+}
