Introduce notion of observable side effects for class initializers
Bug: 135918413
Change-Id: Icfa2ae3c345fad5039deb4daa7697f410c9ce0e1
diff --git a/src/main/java/com/android/tools/r8/graph/DexClass.java b/src/main/java/com/android/tools/r8/graph/DexClass.java
index 07f9e1c..e47fe4d 100644
--- a/src/main/java/com/android/tools/r8/graph/DexClass.java
+++ b/src/main/java/com/android/tools/r8/graph/DexClass.java
@@ -676,27 +676,6 @@
return getClassInitializer() != null;
}
- public boolean hasTrivialClassInitializer() {
- if (isLibraryClass()) {
- // We don't know for library classes in general but assume that java.lang.Object is safe.
- return superType == null;
- }
- DexEncodedMethod clinit = getClassInitializer();
- return clinit != null && clinit.getCode() != null && clinit.getCode().isEmptyVoidMethod();
- }
-
- public boolean hasNonTrivialClassInitializer() {
- if (isLibraryClass()) {
- // We don't know for library classes in general but assume that java.lang.Object is safe.
- return superType != null;
- }
- DexEncodedMethod clinit = getClassInitializer();
- if (clinit == null || clinit.getCode() == null) {
- return false;
- }
- return !clinit.getCode().isEmptyVoidMethod();
- }
-
public boolean hasDefaultInitializer() {
return getDefaultInitializer() != null;
}
@@ -760,7 +739,7 @@
return false;
}
}
- if (hasNonTrivialClassInitializer()) {
+ if (hasClassInitializerWithObservableSideEffects()) {
return true;
}
if (defaultValuesForStaticFieldsMayTriggerAllocation()) {
@@ -769,6 +748,18 @@
return initializationOfParentTypesMayHaveSideEffects(appView, ignore);
}
+ private boolean hasClassInitializerWithObservableSideEffects() {
+ if (isLibraryClass()) {
+ // We don't know for library classes in general but assume that java.lang.Object is safe.
+ return superType != null;
+ }
+ DexEncodedMethod clinit = getClassInitializer();
+ if (clinit == null || clinit.getCode() == null) {
+ return false;
+ }
+ return clinit.getOptimizationInfo().classInitializerMayHaveObservableSideEffects();
+ }
+
public Iterable<DexType> allImmediateSupertypes() {
Iterator<DexType> iterator =
superType != null
diff --git a/src/main/java/com/android/tools/r8/graph/DexEncodedMethod.java b/src/main/java/com/android/tools/r8/graph/DexEncodedMethod.java
index 234d322..64c5b9b 100644
--- a/src/main/java/com/android/tools/r8/graph/DexEncodedMethod.java
+++ b/src/main/java/com/android/tools/r8/graph/DexEncodedMethod.java
@@ -1092,6 +1092,11 @@
private DefaultMethodOptimizationInfoImpl() {}
@Override
+ public boolean classInitializerMayHaveObservableSideEffects() {
+ return true;
+ }
+
+ @Override
public TypeLatticeElement getDynamicReturnType() {
return UNKNOWN_TYPE;
}
@@ -1238,6 +1243,7 @@
public static class MethodOptimizationInfoImpl implements UpdatableMethodOptimizationInfo {
+ private boolean classInitializationMayHaveObservableSideEffects = true;
private boolean hasBeenInlinedIntoSingleCallSite = false;
private Set<DexType> initializedClassesOnNormalExit =
DefaultMethodOptimizationInfoImpl.UNKNOWN_INITIALIZED_CLASSES_ON_NORMAL_EXIT;
@@ -1320,6 +1326,16 @@
}
@Override
+ public boolean classInitializerMayHaveObservableSideEffects() {
+ return classInitializationMayHaveObservableSideEffects;
+ }
+
+ @Override
+ public void unsetClassInitializationMayHaveObservableSideEffects() {
+ classInitializationMayHaveObservableSideEffects = false;
+ }
+
+ @Override
public TypeLatticeElement getDynamicReturnType() {
return returnsObjectOfType;
}
diff --git a/src/main/java/com/android/tools/r8/graph/MethodOptimizationInfo.java b/src/main/java/com/android/tools/r8/graph/MethodOptimizationInfo.java
index 108779c..782ff34 100644
--- a/src/main/java/com/android/tools/r8/graph/MethodOptimizationInfo.java
+++ b/src/main/java/com/android/tools/r8/graph/MethodOptimizationInfo.java
@@ -19,6 +19,8 @@
Default
}
+ boolean classInitializerMayHaveObservableSideEffects();
+
TypeLatticeElement getDynamicReturnType();
ParameterUsage getParameterUsages(int parameter);
diff --git a/src/main/java/com/android/tools/r8/graph/UpdatableMethodOptimizationInfo.java b/src/main/java/com/android/tools/r8/graph/UpdatableMethodOptimizationInfo.java
index da39812..091a5a4 100644
--- a/src/main/java/com/android/tools/r8/graph/UpdatableMethodOptimizationInfo.java
+++ b/src/main/java/com/android/tools/r8/graph/UpdatableMethodOptimizationInfo.java
@@ -59,4 +59,6 @@
void unsetForceInline();
void markNeverInline();
+
+ void unsetClassInitializationMayHaveObservableSideEffects();
}
diff --git a/src/main/java/com/android/tools/r8/ir/code/Value.java b/src/main/java/com/android/tools/r8/ir/code/Value.java
index 52dcbd1..c170810 100644
--- a/src/main/java/com/android/tools/r8/ir/code/Value.java
+++ b/src/main/java/com/android/tools/r8/ir/code/Value.java
@@ -483,6 +483,17 @@
return false;
}
+ public boolean mayDependOnEnvironment() {
+ Value root = getAliasedValue();
+ if (root.isConstant()) {
+ return false;
+ }
+ if (root.isConstantArray()) {
+ return false;
+ }
+ return true;
+ }
+
public boolean usedInMonitorOperation() {
for (Instruction instruction : uniqueUsers()) {
if (instruction.isMonitor()) {
@@ -793,6 +804,11 @@
return definition.isOutConstant() && !hasLocalInfo();
}
+ public boolean isConstantArray() {
+ Value root = getAliasedValue();
+ return !root.isPhi() && definition.isNewArrayEmpty();
+ }
+
public boolean isPhi() {
return false;
}
diff --git a/src/main/java/com/android/tools/r8/ir/conversion/IRConverter.java b/src/main/java/com/android/tools/r8/ir/conversion/IRConverter.java
index 26ea6c5..fe76855 100644
--- a/src/main/java/com/android/tools/r8/ir/conversion/IRConverter.java
+++ b/src/main/java/com/android/tools/r8/ir/conversion/IRConverter.java
@@ -19,6 +19,7 @@
import com.android.tools.r8.graph.DexApplication.Builder;
import com.android.tools.r8.graph.DexCallSite;
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.DexItemFactory;
import com.android.tools.r8.graph.DexMethod;
@@ -42,6 +43,7 @@
import com.android.tools.r8.ir.code.InstructionListIterator;
import com.android.tools.r8.ir.code.InvokeStatic;
import com.android.tools.r8.ir.code.NumericType;
+import com.android.tools.r8.ir.code.StaticPut;
import com.android.tools.r8.ir.code.Value;
import com.android.tools.r8.ir.desugar.BackportedMethodRewriter;
import com.android.tools.r8.ir.desugar.CovariantReturnTypeAnnotationTransformer;
@@ -1359,22 +1361,71 @@
private void computeMayHaveSideEffects(
OptimizationFeedback feedback, DexEncodedMethod method, IRCode code) {
- if (options.enableSideEffectAnalysis
- && !appView.appInfo().withLiveness().mayHaveSideEffects.containsKey(method.method)) {
- boolean mayHaveSideEffects =
- // If the method is synchronized then it acquires a lock.
- method.accessFlags.isSynchronized()
- || (appView.dexItemFactory().isConstructor(method.method)
- && hasNonTrivialFinalizeMethod(method.method.holder))
- || Streams.stream(code.instructions())
- .anyMatch(
- instruction ->
- instruction.instructionMayHaveSideEffects(appView, method.method.holder));
- if (!mayHaveSideEffects) {
- // If the method is native, we don't know what could happen.
- assert !method.accessFlags.isNative();
- feedback.methodMayNotHaveSideEffects(method);
+ // If the method is native, we don't know what could happen.
+ assert !method.accessFlags.isNative();
+
+ if (!options.enableSideEffectAnalysis) {
+ return;
+ }
+
+ if (appView.appInfo().withLiveness().mayHaveSideEffects.containsKey(method.method)) {
+ return;
+ }
+
+ DexType context = method.method.holder;
+
+ if (method.isClassInitializer()) {
+ // For class initializers, we also wish to compute if the class initializer has observable
+ // side effects. A class initializer has observable side effects if it writes a static field
+ // of another class, or if any non-static-put instructions may have side effects.
+ boolean hasIgnoredStaticPut = false;
+ boolean mayHaveObservableSideEffects = false;
+ for (Instruction instruction : code.instructions()) {
+ if (instruction.isStaticPut()) {
+ StaticPut staticPut = instruction.asStaticPut();
+ DexEncodedField field = appView.appInfo().resolveField(staticPut.getField());
+ if (field == null
+ || field.field.holder != method.method.holder
+ || staticPut.inValue().mayDependOnEnvironment()
+ || instruction.instructionInstanceCanThrow(appView, context).isThrowing()) {
+ mayHaveObservableSideEffects = true;
+ break;
+ }
+ hasIgnoredStaticPut = true;
+ continue;
+ }
+ if (instruction.instructionMayHaveSideEffects(appView, context)) {
+ mayHaveObservableSideEffects = true;
+ break;
+ }
}
+ if (!mayHaveObservableSideEffects) {
+ feedback.unsetClassInitializerMayHaveObservableSideEffects(method);
+ if (!hasIgnoredStaticPut) {
+ feedback.methodMayNotHaveSideEffects(method);
+ }
+ }
+ return;
+ }
+
+ boolean mayHaveSideEffects;
+ if (method.accessFlags.isSynchronized()) {
+ // If the method is synchronized then it acquires a lock.
+ mayHaveSideEffects = true;
+ } else if (method.isInstanceInitializer() && hasNonTrivialFinalizeMethod(context)) {
+ // If a class T overrides java.lang.Object.finalize(), then treat the constructor as having
+ // side effects. This ensures that we won't remove instructions on the form `new-instance
+ // {v0}, T`.
+ mayHaveSideEffects = true;
+ } else {
+ // Otherwise, check if there is an instruction that has side effects.
+ mayHaveSideEffects =
+ Streams.stream(code.instructions())
+ .anyMatch(instruction -> instruction.instructionMayHaveSideEffects(appView, context));
+ }
+
+ if (!mayHaveSideEffects) {
+ feedback.methodMayNotHaveSideEffects(method);
}
}
diff --git a/src/main/java/com/android/tools/r8/ir/conversion/OptimizationFeedback.java b/src/main/java/com/android/tools/r8/ir/conversion/OptimizationFeedback.java
index 752f35e..6fac364 100644
--- a/src/main/java/com/android/tools/r8/ir/conversion/OptimizationFeedback.java
+++ b/src/main/java/com/android/tools/r8/ir/conversion/OptimizationFeedback.java
@@ -59,4 +59,6 @@
void setNonNullParamOrThrow(DexEncodedMethod method, BitSet facts);
void setNonNullParamOnNormalExits(DexEncodedMethod method, BitSet facts);
+
+ void unsetClassInitializerMayHaveObservableSideEffects(DexEncodedMethod method);
}
diff --git a/src/main/java/com/android/tools/r8/ir/conversion/OptimizationFeedbackDelayed.java b/src/main/java/com/android/tools/r8/ir/conversion/OptimizationFeedbackDelayed.java
index 82f6391..bcaee39 100644
--- a/src/main/java/com/android/tools/r8/ir/conversion/OptimizationFeedbackDelayed.java
+++ b/src/main/java/com/android/tools/r8/ir/conversion/OptimizationFeedbackDelayed.java
@@ -148,6 +148,12 @@
getOptimizationInfoForUpdating(method).setNonNullParamOnNormalExits(facts);
}
+ @Override
+ public synchronized void unsetClassInitializerMayHaveObservableSideEffects(
+ DexEncodedMethod method) {
+ getOptimizationInfoForUpdating(method).unsetClassInitializationMayHaveObservableSideEffects();
+ }
+
public void updateVisibleOptimizationInfo() {
// Remove methods that have become obsolete. A method may become obsolete, for example, as a
// result of the class staticizer, which aims to transform virtual methods on companion classes
diff --git a/src/main/java/com/android/tools/r8/ir/conversion/OptimizationFeedbackIgnore.java b/src/main/java/com/android/tools/r8/ir/conversion/OptimizationFeedbackIgnore.java
index c0655df..964983a 100644
--- a/src/main/java/com/android/tools/r8/ir/conversion/OptimizationFeedbackIgnore.java
+++ b/src/main/java/com/android/tools/r8/ir/conversion/OptimizationFeedbackIgnore.java
@@ -95,4 +95,7 @@
@Override
public void setNonNullParamOnNormalExits(DexEncodedMethod method, BitSet facts) {
}
+
+ @Override
+ public void unsetClassInitializerMayHaveObservableSideEffects(DexEncodedMethod method) {}
}
diff --git a/src/main/java/com/android/tools/r8/ir/conversion/OptimizationFeedbackSimple.java b/src/main/java/com/android/tools/r8/ir/conversion/OptimizationFeedbackSimple.java
index 1fc7a91..b30f830 100644
--- a/src/main/java/com/android/tools/r8/ir/conversion/OptimizationFeedbackSimple.java
+++ b/src/main/java/com/android/tools/r8/ir/conversion/OptimizationFeedbackSimple.java
@@ -124,4 +124,9 @@
public void setNonNullParamOnNormalExits(DexEncodedMethod method, BitSet facts) {
// Ignored.
}
+
+ @Override
+ public void unsetClassInitializerMayHaveObservableSideEffects(DexEncodedMethod method) {
+ // Ignored.
+ }
}
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 2c2ef08..090152a 100644
--- a/src/main/java/com/android/tools/r8/shaking/Enqueuer.java
+++ b/src/main/java/com/android/tools/r8/shaking/Enqueuer.java
@@ -898,9 +898,9 @@
// We also need to add the corresponding <clinit> to the set of live methods, as otherwise
// static field initialization (and other class-load-time sideeffects) will not happen.
KeepReason reason = KeepReason.reachableFromLiveType(type);
- if (holder.isProgramClass() && holder.hasNonTrivialClassInitializer()) {
+ if (holder.isProgramClass() && holder.hasClassInitializer()) {
DexEncodedMethod clinit = holder.getClassInitializer();
- if (clinit != null) {
+ if (clinit != null && clinit.getOptimizationInfo().mayHaveSideEffects()) {
assert clinit.method.holder == holder.type;
markDirectStaticOrConstructorMethodAsLive(clinit, reason);
}
diff --git a/src/test/java/com/android/tools/r8/ir/optimize/membervaluepropagation/B135918413.java b/src/test/java/com/android/tools/r8/ir/optimize/membervaluepropagation/B135918413.java
index c05847c..34bad33 100644
--- a/src/test/java/com/android/tools/r8/ir/optimize/membervaluepropagation/B135918413.java
+++ b/src/test/java/com/android/tools/r8/ir/optimize/membervaluepropagation/B135918413.java
@@ -5,8 +5,9 @@
package com.android.tools.r8.ir.optimize.membervaluepropagation;
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.assertFalse;
+import static org.junit.Assert.assertTrue;
import com.android.tools.r8.NeverInline;
import com.android.tools.r8.TestBase;
@@ -14,6 +15,7 @@
import com.android.tools.r8.TestParametersCollection;
import com.android.tools.r8.utils.codeinspector.ClassSubject;
import com.android.tools.r8.utils.codeinspector.CodeInspector;
+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 org.junit.Test;
@@ -52,18 +54,26 @@
ClassSubject classSubject = inspector.clazz(TestClass.class);
assertThat(classSubject, isPresent());
+ ClassSubject configClassSubject = inspector.clazz(Config.class);
+ assertThat(configClassSubject, isPresent());
+
+ FieldSubject alwaysEmptyFieldSubject = configClassSubject.uniqueFieldWithName("alwaysEmpty");
+ assertThat(alwaysEmptyFieldSubject, isPresent());
+
MethodSubject mainMethodSubject = classSubject.mainMethod();
assertThat(mainMethodSubject, isPresent());
- // TODO(b/135918413): Should be true.
- assertFalse(
+ assertTrue(
mainMethodSubject
.streamInstructions()
.filter(InstructionSubject::isStaticGet)
- .allMatch(instruction -> instruction.getField().name.toSourceString().equals("out")));
+ .map(InstructionSubject::getField)
+ .allMatch(
+ field ->
+ field.name.toSourceString().equals(alwaysEmptyFieldSubject.getFinalName())
+ || field.name.toSourceString().equals("out")));
MethodSubject deadMethodSubject = classSubject.uniqueMethodWithName("dead");
- // TODO(b/135918413): Should be absent.
- assertThat(deadMethodSubject, isPresent());
+ assertThat(deadMethodSubject, not(isPresent()));
}
static class TestClass {
