Version 1.5.66
Cherry-pick:
Keep track of static-puts in clinit and remove them if necessary.
CL: https://r8-review.googlesource.com/c/r8/+/41280 (adaptively)
Cherry-pick: Add a test for incorrect member value propagation
CL: https://r8-review.googlesource.com/c/r8/+/41260 (partial)
Bug: 138912149
Change-Id: I3127f06489cb3e9c2e6ec84165b050c36245ee46
diff --git a/src/main/java/com/android/tools/r8/Version.java b/src/main/java/com/android/tools/r8/Version.java
index a49bad9..ad42fab 100644
--- a/src/main/java/com/android/tools/r8/Version.java
+++ b/src/main/java/com/android/tools/r8/Version.java
@@ -11,7 +11,7 @@
// This field is accessed from release scripts using simple pattern matching.
// Therefore, changing this field could break our release scripts.
- public static final String LABEL = "1.5.65";
+ public static final String LABEL = "1.5.66";
private Version() {
}
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/ClassInitializerDefaultsOptimization.java b/src/main/java/com/android/tools/r8/ir/optimize/ClassInitializerDefaultsOptimization.java
index 45ca27a..d1a8206 100644
--- a/src/main/java/com/android/tools/r8/ir/optimize/ClassInitializerDefaultsOptimization.java
+++ b/src/main/java/com/android/tools/r8/ir/optimize/ClassInitializerDefaultsOptimization.java
@@ -117,7 +117,7 @@
// the field initial value.
Set<StaticPut> unnecessaryStaticPuts = Sets.newIdentityHashSet();
Collection<StaticPut> finalFieldPuts =
- computeUnnecessaryStaticPuts(code, clazz, unnecessaryStaticPuts);
+ findFinalFieldPutsWhileCollectingUnnecessaryStaticPuts(code, clazz, unnecessaryStaticPuts);
// Return eagerly if there is nothing to optimize.
if (unnecessaryStaticPuts.isEmpty()) {
@@ -314,9 +314,10 @@
return null;
}
- private Collection<StaticPut> computeUnnecessaryStaticPuts(
- IRCode code, DexClass clazz, Set<StaticPut> puts) {
- Map<DexField, StaticPut> finalFieldPut = Maps.newIdentityHashMap();
+ private Collection<StaticPut> findFinalFieldPutsWhileCollectingUnnecessaryStaticPuts(
+ IRCode code, DexClass clazz, Set<StaticPut> unnecessaryStaticPuts) {
+ Map<DexField, StaticPut> finalFieldPuts = Maps.newIdentityHashMap();
+ Map<DexField, Set<StaticPut>> isWrittenBefore = Maps.newIdentityHashMap();
Set<DexField> isReadBefore = Sets.newIdentityHashSet();
final int color = code.reserveMarkingColor();
try {
@@ -333,14 +334,14 @@
isReadBefore.add(field.field);
} else if (instruction.instructionMayHaveSideEffects(appView, clazz.type)) {
// Reading another field is only OK if the read does not have side-effects.
- return finalFieldPut.values();
+ return validateFinalFieldPuts(finalFieldPuts, isWrittenBefore);
}
} else if (instruction.isStaticPut()) {
StaticPut put = instruction.asStaticPut();
if (put.getField().holder != clazz.type) {
// Can cause clinit on another class which can read uninitialized static fields
// of this class.
- return finalFieldPut.values();
+ return validateFinalFieldPuts(finalFieldPuts, isWrittenBefore);
}
DexField field = put.getField();
if (clazz.definesStaticField(field)) {
@@ -353,27 +354,54 @@
continue;
}
if (put.inValue().isConstant()) {
- if ((field.type.isClassType() || field.type.isArrayType())
- && put.inValue().isZero()) {
- finalFieldPut.put(put.getField(), put);
- puts.add(put);
+ if (field.type.isReferenceType() && put.inValue().isZero()) {
+ finalFieldPuts.put(field, put);
+ unnecessaryStaticPuts.add(put);
+ // If this field has been written before, those static-put's up to this point are
+ // redundant. We should remove them all together; otherwise, remaining static-put
+ // that is not constant can change the program semantics. See b/138912149.
+ if (isWrittenBefore.containsKey(field)) {
+ unnecessaryStaticPuts.addAll(isWrittenBefore.get(field));
+ isWrittenBefore.remove(field);
+ }
+ continue;
} else if (field.type.isPrimitiveType()
|| field.type == dexItemFactory.stringType) {
- finalFieldPut.put(put.getField(), put);
- puts.add(put);
+ finalFieldPuts.put(field, put);
+ unnecessaryStaticPuts.add(put);
+ if (isWrittenBefore.containsKey(field)) {
+ unnecessaryStaticPuts.addAll(isWrittenBefore.get(field));
+ isWrittenBefore.remove(field);
+ }
+ continue;
}
+ // Still constant, but not able to represent it as static encoded values, e.g.,
+ // const-class, const-method-handle, etc. This static-put can be redundant if the
+ // field is rewritten with another constant. Will fall through and track static-put.
} else if (isClassNameConstantOf(clazz, put)) {
// Collect put of class name constant as a potential default value.
- finalFieldPut.put(put.getField(), put);
- puts.add(put);
+ finalFieldPuts.put(field, put);
+ unnecessaryStaticPuts.add(put);
+ if (isWrittenBefore.containsKey(field)) {
+ unnecessaryStaticPuts.addAll(isWrittenBefore.get(field));
+ isWrittenBefore.remove(field);
+ }
+ continue;
}
+ // static-put that is reaching here can be redundant if the corresponding field is
+ // rewritten with another constant (of course before being read).
+ // However, if static-put is still remaining in `isWrittenBefore`, that indicates
+ // the previous candidate as final field put is no longer valid.
+ isWrittenBefore
+ .computeIfAbsent(field, ignore -> Sets.newIdentityHashSet())
+ .add(put);
} else {
// Writing another field is not OK.
- return finalFieldPut.values();
+ return validateFinalFieldPuts(finalFieldPuts, isWrittenBefore);
}
} else if (instruction.instructionMayHaveSideEffects(appView, clazz.type)) {
// Some other instruction that has side-effects. Stop here.
- return finalFieldPut.values();
+ return validateFinalFieldPuts(finalFieldPuts, isWrittenBefore);
} else {
// TODO(b/120138731): This check should be removed when the Class.get*Name()
// optimizations become enabled.
@@ -384,7 +412,7 @@
if (instruction.isInvoke() && instruction.asInvoke().outValue() != null) {
// This invoke could return a value that has been computed based on the value of one
// of the fields in the enclosing class, so give up.
- return finalFieldPut.values();
+ return validateFinalFieldPuts(finalFieldPuts, isWrittenBefore);
}
}
}
@@ -395,7 +423,33 @@
} finally {
code.returnMarkingColor(color);
}
- return finalFieldPut.values();
+ return validateFinalFieldPuts(finalFieldPuts, isWrittenBefore);
+ }
+
+ private Collection<StaticPut> validateFinalFieldPuts(
+ Map<DexField, StaticPut> finalFieldPuts,
+ Map<DexField, Set<StaticPut>> isWrittenBefore) {
+ // If a field is rewritten again with other values that we can't represent as static encoded
+ // values, that would be recorded at `isWrittenBefore`, which is used to collect and remove
+ // redundant static-puts. The remnant indicates that the candidate for final field put is not
+ // valid anymore, so remove it.
+ //
+ // For example,
+ // static String x;
+ //
+ // static {
+ // x = "constant"; // will be in finalFieldPut
+ // x = <non-constant> // will be added to isWrittenBefore
+ // // x = "another-constant"
+ // }
+ // If there is another static-put with a constant, static-put stored in `isWrittenBefore` is
+ // used to remove redundant static-put together. (And the previous constant is overwritten.)
+ // If not, `x` has "constant" as a default value, whereas static-put with non-constant is
+ // remaining. If other optimizations (most likely member value propagation) rely on encoded
+ // values, leaving it can cause incorrect optimizations. Thus, we invalidate candidates of
+ // final field puts at all.
+ isWrittenBefore.keySet().forEach(finalFieldPuts::remove);
+ return finalFieldPuts.values();
}
// Check if the static put is a constant derived from the class holding the method.
diff --git a/src/test/java/com/android/tools/r8/ir/optimize/membervaluepropagation/B138912149.java b/src/test/java/com/android/tools/r8/ir/optimize/membervaluepropagation/B138912149.java
new file mode 100644
index 0000000..bee06af
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/ir/optimize/membervaluepropagation/B138912149.java
@@ -0,0 +1,120 @@
+// 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.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.Assume.assumeTrue;
+
+import com.android.tools.r8.TestBase;
+import com.android.tools.r8.TestParameters;
+import com.android.tools.r8.TestParametersCollection;
+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 B138912149 extends TestBase {
+
+ private final TestParameters parameters;
+
+ @Parameters(name = "{0}")
+ public static TestParametersCollection data() {
+ return getTestParameters().withAllRuntimes().build();
+ }
+
+ public B138912149(TestParameters parameters) {
+ this.parameters = parameters;
+ }
+
+ @Test
+ public void test() throws Exception {
+ testForR8(parameters.getBackend())
+ .addInnerClasses(B138912149.class)
+ .addKeepMainRule(TestClass.class)
+ .setMinApi(parameters.getRuntime())
+ .noMinification()
+ .compile()
+ .run(parameters.getRuntime(), TestClass.class)
+ .assertSuccessWithOutputLines("The end")
+ .inspect(codeInspector -> {
+ // All <clinit>s are simplified and shrunk.
+ codeInspector.forAllClasses(classSubject -> {
+ assertThat(classSubject.clinit(), not(isPresent()));
+ });
+ });
+ }
+
+ @Test
+ public void testJvm() throws Exception {
+ assumeTrue(parameters.isCfRuntime());
+ testForJvm()
+ .addTestClasspath()
+ .run(parameters.getRuntime(), TestClass.class)
+ .assertSuccessWithOutputLines("The end");
+ }
+
+ static class TestClass {
+
+ public static void main(String... args) {
+ if (A.alwaysFalse) {
+ System.out.println("Dead code 1");
+ }
+ if (B.alwaysFalse || !B.alwaysTrue) {
+ System.out.println("Dead code 2");
+ }
+ if (C.alwaysNull != null) {
+ System.out.println("Dead code 3");
+ }
+ if (!N.name.endsWith("N")) {
+ System.out.println("Dead code: " + N.name);
+ }
+ System.out.println("The end");
+ }
+ }
+
+ static class A {
+ static boolean alwaysFalse;
+
+ static {
+ alwaysFalse = false;
+ }
+ }
+
+ static class B extends A {
+ static boolean alwaysTrue;
+
+ static {
+ alwaysTrue = alwaysFalse;
+ // Either keep this static-put or remove both static-put's and put `true` to `alwaysTrue`.
+ alwaysTrue = true;
+ }
+ }
+
+ static class C {
+ static Object alwaysNull;
+
+ static {
+ alwaysNull = C.class;
+ // Either keep this static-put or remove both static-put's and put `null` to `alwaysNull`.
+ alwaysNull = null;
+ }
+ }
+
+ static class N {
+ static String name;
+
+ static {
+ name = "dummy";
+ // Either keep this static-put or remove both static-put's and put the name, ...$N, to `name`.
+ name = N.class.getName();
+ // Note that we can't use other kinds of names, e.g., simple name, because the enclosing class
+ // is not provided as a program class, hence all attributes related to inner-name computation
+ // is gone in this test setting.
+ }
+ }
+}