Do not use locals debug information to compute actual types of locals.

Debug information doesn't have to be complete and can lead to
confusing type mismatches when propagated in type analysis. This change
ensures that debug information is not used to seed the type analysis.

R=zerny@google.com

Bug: b/64658224
Change-Id: I5cbaf1803e320339886e23f9d3201c0ea507b431

4 files changed, 123 insertions, 18 deletions

diff --git a/src/main/java/com/android/tools/r8/ir/conversion/JarSourceCode.java b/src/main/java/com/android/tools/r8/ir/conversion/JarSourceCode.java
index 484354a3a..8d36ef7d6 100644
--- a/src/main/java/com/android/tools/r8/ir/conversion/JarSourceCode.java
+++ b/src/main/java/com/android/tools/r8/ir/conversion/JarSourceCode.java
@@ -258,23 +258,15 @@ public class JarSourceCode implements SourceCode {
   @Override
   public void buildPrelude(IRBuilder builder) {
     Map<Integer, MoveType> initializedLocals = new HashMap<>(node.localVariables.size());
+    // Record types for arguments.
+    recordArgumentTypes(initializedLocals);
+    // Add debug information for all locals at the initial label.
     if (initialLabel != null) {
       state.openLocals(initialLabel);
     }
-    int argumentRegister = 0;
-    if (!isStatic()) {
-      Type thisType = Type.getType(clazz.descriptor.toString());
-      int register = state.writeLocal(argumentRegister++, thisType);
-      builder.addThisArgument(register);
-      initializedLocals.put(register, moveType(thisType));
-    }
-    for (Type type : parameterTypes) {
-      MoveType moveType = moveType(type);
-      int register = state.writeLocal(argumentRegister, type);
-      builder.addNonThisArgument(register, moveType);
-      argumentRegister += moveType.requiredRegisters();
-      initializedLocals.put(register, moveType);
-    }
+    // Build the actual argument instructions now that type and debug information is known
+    // for arguments.
+    buildArgumentInstructions(builder);
     if (isSynchronized()) {
       generatingMethodSynchronization = true;
       Type clazzType = Type.getType(clazz.toDescriptorString());
@@ -297,6 +289,23 @@ public class JarSourceCode implements SourceCode {
     for (Object o : node.localVariables) {
       LocalVariableNode local = (LocalVariableNode) o;
       Type localType = Type.getType(local.desc);
+      int sort = localType.getSort();
+      switch (sort) {
+        case Type.OBJECT:
+        case Type.ARRAY:
+          localType = JarState.NULL_TYPE;
+          break;
+        case Type.DOUBLE:
+        case Type.LONG:
+        case Type.FLOAT:
+          break;
+        case Type.VOID:
+        case Type.METHOD:
+          throw new Unreachable("Invalid local variable type: " + localType);
+        default:
+          localType = Type.INT_TYPE;
+          break;
+      }
       int localRegister = state.getLocalRegister(local.index, localType);
       MoveType exitingLocalType = initializedLocals.get(localRegister);
       assert exitingLocalType == null || exitingLocalType == moveType(localType);
@@ -311,6 +320,36 @@ public class JarSourceCode implements SourceCode {
     state.setBuilding();
   }
 
+  private void buildArgumentInstructions(IRBuilder builder) {
+    int argumentRegister = 0;
+    if (!isStatic()) {
+      Type thisType = Type.getType(clazz.descriptor.toString());
+      Slot slot = state.readLocal(argumentRegister++, thisType);
+      builder.addThisArgument(slot.register);
+    }
+    for (Type type : parameterTypes) {
+      MoveType moveType = moveType(type);
+      Slot slot = state.readLocal(argumentRegister, type);
+      builder.addNonThisArgument(slot.register, moveType);
+      argumentRegister += moveType.requiredRegisters();
+    }
+  }
+
+  private void recordArgumentTypes(Map<Integer, MoveType> initializedLocals) {
+    int argumentRegister = 0;
+    if (!isStatic()) {
+      Type thisType = Type.getType(clazz.descriptor.toString());
+      int register = state.writeLocal(argumentRegister++, thisType);
+      initializedLocals.put(register, moveType(thisType));
+    }
+    for (Type type : parameterTypes) {
+      MoveType moveType = moveType(type);
+      int register = state.writeLocal(argumentRegister, type);
+      argumentRegister += moveType.requiredRegisters();
+      initializedLocals.put(register, moveType);
+    }
+  }
+
   private void computeBlockEntryJarStates(IRBuilder builder) {
     Int2ReferenceSortedMap<BlockInfo> CFG = builder.getCFG();
     Queue<JarStateWorklistItem> worklist = new LinkedList<>();
diff --git a/src/main/java/com/android/tools/r8/ir/conversion/JarState.java b/src/main/java/com/android/tools/r8/ir/conversion/JarState.java
index 5009d5e2c..a6b1f2ce0 100644
--- a/src/main/java/com/android/tools/r8/ir/conversion/JarState.java
+++ b/src/main/java/com/android/tools/r8/ir/conversion/JarState.java
@@ -87,6 +87,9 @@ public class JarState {
       if (type == BYTE_OR_BOOL_TYPE) {
         type = Type.BYTE_TYPE;
       }
+      if (other == BYTE_OR_BOOL_TYPE) {
+        other = Type.BYTE_TYPE;
+      }
       int sort = type.getSort();
       int otherSort = other.getSort();
       if (isReferenceCompatible(type, other)) {
@@ -256,7 +259,7 @@ public class JarState {
     Collection<LocalVariableNode> nodes = localVariableStartPoints.get(label);
     ArrayList<Local> locals = new ArrayList<>(nodes.size());
     for (LocalVariableNode node : nodes) {
-      locals.add(setLocal(node.index, Type.getType(node.desc), localVariables.get(node)));
+      locals.add(setLocalInfo(node.index, Type.getType(node.desc), localVariables.get(node)));
     }
     // Sort to ensure deterministic instruction ordering (correctness is unaffected).
     locals.sort(Comparator.comparingInt(local -> local.slot.register));
@@ -335,6 +338,18 @@ public class JarState {
     return local;
   }
 
+  private Local setLocalInfo(int index, Type type, DebugLocalInfo info) {
+    return setLocalInfoForRegister(getLocalRegister(index, type), type, info);
+  }
+
+  private Local setLocalInfoForRegister(int register, Type type, DebugLocalInfo info) {
+    Local existingLocal = getLocalForRegister(register);
+    Local local = new Local(existingLocal.slot, info);
+    locals[register] = local;
+    return local;
+  }
+
+
   public int writeLocal(int index, Type type) {
     assert nonNullType(type);
     Local local = getLocal(index, type);
@@ -344,8 +359,9 @@ public class JarState {
     }
     // We cannot assume consistency for writes because we do not have complete information about the
     // scopes of locals. We assume the program to be verified and overwrite if the types mismatch.
-    if (local == null || (local.info == null && !typeEquals(local.slot.type, type))) {
-      local = setLocal(index, type, null);
+    if (local == null || !typeEquals(local.slot.type, type)) {
+      DebugLocalInfo info = local == null ? null : local.info;
+      local = setLocal(index, type, info);
     }
     return local.slot.register;
   }
diff --git a/src/test/java/com/android/tools/r8/jasmin/DebugLocalTests.java b/src/test/java/com/android/tools/r8/jasmin/DebugLocalTests.java
index 72bbaede8..2782c91b0 100644
--- a/src/test/java/com/android/tools/r8/jasmin/DebugLocalTests.java
+++ b/src/test/java/com/android/tools/r8/jasmin/DebugLocalTests.java
@@ -183,9 +183,9 @@ public class DebugLocalTests extends JasminTestBase {
         "MethodStart:",
         ".line 1",
 
-        "LabelXStart:",
         "  ldc 0",
         "  istore 1",
+        "LabelXStart:",
         ".line 2",
         "  invokestatic Test/ensureLine()V",
         "LabelXEnd:",
diff --git a/src/test/java/com/android/tools/r8/jasmin/Regress64658224.java b/src/test/java/com/android/tools/r8/jasmin/Regress64658224.java
new file mode 100644
index 000000000..8c1df5632
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/jasmin/Regress64658224.java
@@ -0,0 +1,50 @@
+// Copyright (c) 2017, 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.jasmin;
+
+import static org.junit.Assert.assertEquals;
+
+import com.google.common.collect.ImmutableList;
+import org.junit.Test;
+
+public class Regress64658224 extends JasminTestBase {
+
+  @Test
+  public void testInvalidTypeInfoFromLocals() throws Exception {
+    JasminBuilder builder = new JasminBuilder();
+    JasminBuilder.ClassBuilder clazz = builder.addClass("Test");
+
+    clazz.addStaticMethod("foo", ImmutableList.of("I"), "V",
+        ".limit stack 2",
+        ".limit locals 2",
+        ".var 1 is x Ljava/lang/Object; from L1 to L2",
+        "  aconst_null",
+        "  astore 1",
+        "L1:",
+        "  iload 0",
+        "  ifeq L3",
+        "L2:",
+        "  goto L5",
+        "L3:",
+        "  aload 1",
+        "  iconst_0",
+        "  aaload",
+        "  pop",
+        "L5:",
+        "  return");
+
+    clazz.addMainMethod(
+        ".limit stack 1",
+        ".limit locals 1",
+        "  ldc 2",
+        "  invokestatic Test/foo(I)V",
+        "  return");
+
+    String expected = "";
+    String javaResult = runOnJava(builder, clazz.name);
+    assertEquals(expected, javaResult);
+    String artResult = runOnArtD8(builder, clazz.name);
+    assertEquals(expected, artResult);
+  }
+}