commit 2d8c93fe249ac143a8ae9966e85ea1d387b897bc
author Morten Krogh-Jespersen <mkroghj@google.com> Thu May 25 15:05:53 2023 +0200
committer Morten Krogh-Jespersen <mkroghj@google.com> Thu May 25 13:06:14 2023 +0000
tree e9a478ff13592858cb741fa101ff42ae8c268761
parent d4b1b5cde09d67a3b7cffed31c6d1cd5aba3ca5d

Only generate FilledNewArray when puts match the concrete array type

Bug: b/283715197
Change-Id: Ie24d90160cceeecd790a5074d8dbcbac116fb58e

src/main/java/com/android/tools/r8/ir/optimize/CodeRewriter.java

diff --git a/src/main/java/com/android/tools/r8/ir/optimize/CodeRewriter.java b/src/main/java/com/android/tools/r8/ir/optimize/CodeRewriter.java
index c0d3e6b..3988896 100644
--- a/src/main/java/com/android/tools/r8/ir/optimize/CodeRewriter.java
+++ b/src/main/java/com/android/tools/r8/ir/optimize/CodeRewriter.java
@@ -2263,7 +2263,7 @@
     }
   }
 
-  private FilledArrayCandidate computeFilledArrayCandiate(
+  private FilledArrayCandidate computeFilledArrayCandidate(
       Instruction instruction, RewriteArrayOptions options) {
     NewArrayEmpty newArrayEmpty = instruction.asNewArrayEmpty();
     if (newArrayEmpty == null) {
@@ -2284,6 +2284,18 @@
     if (!encodeAsFilledNewArray && !canUseFilledArrayData(arrayType, size, options)) {
       return null;
     }
+    // Check that all arguments to the array is the array type or that the array is type Object[].
+    if (!options.canUseSubTypesInFilledNewArray()
+        && arrayType != dexItemFactory.objectArrayType
+        && !arrayType.isPrimitiveArrayType()) {
+      DexType baseType = arrayType.toBaseType(dexItemFactory);
+      for (Instruction uniqueUser : newArrayEmpty.outValue().uniqueUsers()) {
+        if (uniqueUser.isArrayPut()
+            && !uniqueUser.asArrayPut().value().getType().isClassType(baseType)) {
+          return null;
+        }
+      }
+    }
     return new FilledArrayCandidate(newArrayEmpty, size, encodeAsFilledNewArray);
   }
 
@@ -2393,7 +2405,7 @@
     RewriteArrayOptions rewriteOptions = options.rewriteArrayOptions();
     InstructionListIterator it = block.listIterator(code);
     while (it.hasNext()) {
-      FilledArrayCandidate candidate = computeFilledArrayCandiate(it.next(), rewriteOptions);
+      FilledArrayCandidate candidate = computeFilledArrayCandidate(it.next(), rewriteOptions);
       if (candidate == null) {
         continue;
       }

src/main/java/com/android/tools/r8/utils/InternalOptions.java

diff --git a/src/main/java/com/android/tools/r8/utils/InternalOptions.java b/src/main/java/com/android/tools/r8/utils/InternalOptions.java
index 2f66e6f..b2ce050 100644
--- a/src/main/java/com/android/tools/r8/utils/InternalOptions.java
+++ b/src/main/java/com/android/tools/r8/utils/InternalOptions.java
@@ -1519,6 +1519,13 @@
       return hasFeaturePresentFrom(AndroidApiLevel.N);
     }
 
+    // When adding support for emitting filled-new-array for sub-types, ART 13 (Api-level 33) had
+    // issues. See b/283715197.
+    public boolean canUseSubTypesInFilledNewArray() {
+      assert isGeneratingDex();
+      return !canHaveBugPresentUntil(AndroidApiLevel.U);
+    }
+
     // Dalvik doesn't handle new-filled-array with arrays as values. It fails with:
     // W(629880) VFY: [Ljava/lang/Integer; is not instance of Ljava/lang/Integer;  (dalvikvm)
     public boolean canUseFilledNewArrayOfArrays() {

src/test/java/com/android/tools/r8/TestParameters.java

diff --git a/src/test/java/com/android/tools/r8/TestParameters.java b/src/test/java/com/android/tools/r8/TestParameters.java
index 904d325..c792710 100644
--- a/src/test/java/com/android/tools/r8/TestParameters.java
+++ b/src/test/java/com/android/tools/r8/TestParameters.java
@@ -116,6 +116,10 @@
     return isDexRuntime() && getApiLevel().isGreaterThanOrEqualTo(AndroidApiLevel.N);
   }
 
+  public boolean canUseSubTypesInFilledNewArray() {
+    return isDexRuntime() && getApiLevel().isGreaterThanOrEqualTo(AndroidApiLevel.U);
+  }
+
   public boolean runtimeWithClassValue() {
     assert isCfRuntime() || isDexRuntime();
     return isCfRuntime() || getDexRuntimeVersion().isNewerThanOrEqual(DexVm.Version.V14_0_0);

src/test/java/com/android/tools/r8/rewrite/arrays/FilledArrayDataRemoveCheckCastTest.java

diff --git a/src/test/java/com/android/tools/r8/rewrite/arrays/FilledArrayDataRemoveCheckCastTest.java b/src/test/java/com/android/tools/r8/rewrite/arrays/FilledArrayDataRemoveCheckCastTest.java
index 1f4e6fb..71639f7 100644
--- a/src/test/java/com/android/tools/r8/rewrite/arrays/FilledArrayDataRemoveCheckCastTest.java
+++ b/src/test/java/com/android/tools/r8/rewrite/arrays/FilledArrayDataRemoveCheckCastTest.java
@@ -68,9 +68,9 @@
               assertEquals(
                   parameters.canUseFilledNewArrayOnNonStringObjects(),
                   filledNewArrayInIterateBaseClasses.isPresent());
-              // TODO(b/283715197): It should not be present.
               assertEquals(
-                  parameters.canUseFilledNewArrayOnNonStringObjects(),
+                  parameters.canUseFilledNewArrayOnNonStringObjects()
+                      && parameters.canUseSubTypesInFilledNewArray(),
                   filledNewArrayInIterateSubClasses.isPresent());
             });
   }