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
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 484354a..8d36ef7 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 @@
@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 @@
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 @@
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 5009d5e..a6b1f2c 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 @@
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 @@
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 @@
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 @@
}
// 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 72bbaed..2782c91 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 @@
"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 0000000..8c1df56
--- /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);
+ }
+}
