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.
+    }
+  }
+}