The failure of dx test 105 wasn't actually spurious. Fixed the code.
In particular, for array load and store instructions, dx was being too
aggressive about letting the contents of the stack override the
implied type expected by the instructions. This led to cases where dx
would emit code instead of reporting an error.
The failure on the array load side of things implied that there also
needed to be a new regression test on the store side, since the
existing array store test didn't catch this. So, I added that too in
this change.
Finally, I took this opportunity to do minor whitespace-type cleanups
in a couple of related files that I opened during the course of the
investigation.
Change-Id: I72c644f66afb1108ae43a129ac81b010d072155a
diff --git a/dx/src/com/android/dx/cf/code/BytecodeArray.java b/dx/src/com/android/dx/cf/code/BytecodeArray.java
index 80e94bf..e942b13 100644
--- a/dx/src/com/android/dx/cf/code/BytecodeArray.java
+++ b/dx/src/com/android/dx/cf/code/BytecodeArray.java
@@ -41,15 +41,18 @@
/** {@code non-null;} underlying bytes */
private final ByteArray bytes;
- /** {@code non-null;} constant pool to use when resolving constant pool indices */
+ /**
+ * {@code non-null;} constant pool to use when resolving constant
+ * pool indices
+ */
private final ConstantPool pool;
/**
* Constructs an instance.
*
* @param bytes {@code non-null;} underlying bytes
- * @param pool {@code non-null;} constant pool to use when resolving constant
- * pool indices
+ * @param pool {@code non-null;} constant pool to use when
+ * resolving constant pool indices
*/
public BytecodeArray(ByteArray bytes, ConstantPool pool) {
if (bytes == null) {
@@ -96,7 +99,8 @@
/**
* Parses each instruction in the array, in order.
*
- * @param visitor {@code null-ok;} visitor to call back to for each instruction
+ * @param visitor {@code null-ok;} visitor to call back to for
+ * each instruction
*/
public void forEach(Visitor visitor) {
int sz = bytes.size();
@@ -140,7 +144,8 @@
* set new bits in the work set during the process.
*
* @param workSet {@code non-null;} the work set to process
- * @param visitor {@code non-null;} visitor to call back to for each instruction
+ * @param visitor {@code non-null;} visitor to call back to for
+ * each instruction
*/
public void processWorkSet(int[] workSet, Visitor visitor) {
if (visitor == null) {
@@ -431,6 +436,10 @@
return 1;
}
case ByteOps.BALOAD: {
+ /*
+ * Note: This is a load from either a byte[] or a
+ * boolean[].
+ */
visitor.visitNoArgs(ByteOps.IALOAD, offset, 1, Type.BYTE);
return 1;
}
@@ -543,6 +552,10 @@
return 1;
}
case ByteOps.BASTORE: {
+ /*
+ * Note: This is a load from either a byte[] or a
+ * boolean[].
+ */
visitor.visitNoArgs(ByteOps.IASTORE, offset, 1,
Type.BYTE);
return 1;
@@ -964,22 +977,28 @@
while (true) {
boolean punt = false;
- // First check if the next bytecode is dup
+ // First, check if the next bytecode is dup.
int nextByte = bytes.getUnsignedByte(curOffset++);
if (nextByte != ByteOps.DUP)
break;
- // Next check if the expected array index is pushed to the stack
+ /*
+ * Next, check if the expected array index is pushed to
+ * the stack.
+ */
parseInstruction(curOffset, constantVisitor);
if (constantVisitor.length == 0 ||
!(constantVisitor.cst instanceof CstInteger) ||
constantVisitor.value != nInit)
break;
- // Next, fetch the init value and record it
+ // Next, fetch the init value and record it.
curOffset += constantVisitor.length;
- // Next find out what kind of constant is pushed onto the stack
+ /*
+ * Next, find out what kind of constant is pushed onto
+ * the stack.
+ */
parseInstruction(curOffset, constantVisitor);
if (constantVisitor.length == 0 ||
!(constantVisitor.cst instanceof CstLiteralBits))
@@ -989,7 +1008,7 @@
initVals.add(constantVisitor.cst);
nextByte = bytes.getUnsignedByte(curOffset++);
- // Now, check if the value is stored to the array properly
+ // Now, check if the value is stored to the array properly.
switch (value) {
case ByteOps.NEWARRAY_BYTE:
case ByteOps.NEWARRAY_BOOLEAN: {
@@ -1222,8 +1241,8 @@
* @param opcode the opcode
* @param offset offset to the instruction
* @param length length of the instruction, in bytes
- * @param cases {@code non-null;} list of (value, target) pairs, plus the
- * default target
+ * @param cases {@code non-null;} list of (value, target)
+ * pairs, plus the default target
* @param padding the bytes found in the padding area (if any),
* packed
*/
@@ -1236,7 +1255,8 @@
* @param offset offset to the instruction
* @param length length of the instruction, in bytes
* @param type {@code non-null;} the type of the array
- * @param initVals {@code non-null;} list of bytecode offsets for init values
+ * @param initVals {@code non-null;} list of bytecode offsets
+ * for init values
*/
public void visitNewarray(int offset, int length, CstType type,
ArrayList<Constant> initVals);
diff --git a/dx/src/com/android/dx/cf/code/Simulator.java b/dx/src/com/android/dx/cf/code/Simulator.java
index f96699e..0812305 100644
--- a/dx/src/com/android/dx/cf/code/Simulator.java
+++ b/dx/src/com/android/dx/cf/code/Simulator.java
@@ -129,6 +129,64 @@
}
/**
+ * Returns the required array type for an array load or store
+ * instruction, based on a given implied type and an observed
+ * actual array type.
+ *
+ * <p>The interesting cases here have to do with object arrays,
+ * <code>byte[]</code>s, <code>boolean[]</code>s, and
+ * known-nulls.</p>
+ *
+ * <p>In the case of arrays of objects, we want to narrow the type
+ * to the actual array present on the stack, as long as what is
+ * present is an object type. Similarly, due to a quirk of the
+ * original bytecode representation, the instructions for dealing
+ * with <code>byte[]</code> and <code>boolean[]</code> are
+ * undifferentiated, and we aim here to return whichever one was
+ * actually present on the stack.</p>
+ *
+ * <p>In the case where there is a known-null on the stack where
+ * an array is expected, we just fall back to the implied type of
+ * the instruction. Due to the quirk described above, this means
+ * that source code that uses <code>boolean[]</code> might get
+ * translated surprisingly -- but correctly -- into an instruction
+ * that specifies a <code>byte[]</code>. It will be correct,
+ * because should the code actually execute, it will necessarily
+ * throw a <code>NullPointerException</code>, and it won't matter
+ * what opcode variant is used to achieve that result.</p>
+ *
+ * @param impliedType {@code non-null;} type implied by the
+ * instruction; is <i>not</i> an array type
+ * @param foundArrayType {@code non-null;} type found on the
+ * stack; is either an array type or a known-null
+ * @return {@code non-null;} the array type that should be
+ * required in this context
+ */
+ private static Type requiredArrayTypeFor(Type impliedType,
+ Type foundArrayType) {
+ if (foundArrayType == Type.KNOWN_NULL) {
+ return impliedType.getArrayType();
+ }
+
+ if ((impliedType == Type.OBJECT)
+ && foundArrayType.isArray()
+ && foundArrayType.getComponentType().isReference()) {
+ return foundArrayType;
+ }
+
+ if ((impliedType == Type.BYTE)
+ && (foundArrayType == Type.BOOLEAN_ARRAY)) {
+ /*
+ * Per above, an instruction with implied byte[] is also
+ * allowed to be used on boolean[].
+ */
+ return Type.BOOLEAN_ARRAY;
+ }
+
+ return impliedType.getArrayType();
+ }
+
+ /**
* Bytecode visitor used during simulation.
*/
private class SimVisitor implements BytecodeArray.Visitor {
@@ -256,23 +314,17 @@
}
case ByteOps.IALOAD: {
/*
- * Change the type (which is to be pushed) to
- * reflect the actual component type of the array
- * being popped, unless it turns out to be a
- * known-null, in which case we just use the type
- * implied by the original instruction.
+ * See comment on requiredArrayTypeFor() for explanation
+ * about what's going on here.
*/
Type foundArrayType = frame.getStack().peekType(1);
- Type requireArrayType;
+ Type requiredArrayType =
+ requiredArrayTypeFor(type, foundArrayType);
- if (foundArrayType != Type.KNOWN_NULL) {
- requireArrayType = foundArrayType;
- type = foundArrayType.getComponentType();
- } else {
- requireArrayType = type.getArrayType();
- }
+ // Make type agree with the discovered requiredArrayType.
+ type = requiredArrayType.getComponentType();
- machine.popArgs(frame, requireArrayType, Type.INT);
+ machine.popArgs(frame, requiredArrayType, Type.INT);
break;
}
case ByteOps.IADD:
@@ -308,27 +360,22 @@
}
case ByteOps.IASTORE: {
/*
- * Change the type (which is the type of the
- * element) to reflect the actual component type
- * of the array being popped, unless it turns out
- * to be a known-null, in which case we just use
- * the type implied by the original instruction.
- * The category 1 vs. 2 thing here is that, if the
- * element type is category 2, we have to skip over
- * one extra stack slot to find the array.
+ * See comment on requiredArrayTypeFor() for
+ * explanation about what's going on here. In
+ * addition to that, the category 1 vs. 2 thing
+ * below is to deal with the fact that, if the
+ * element type is category 2, we have to skip
+ * over one extra stack slot to find the array.
*/
Type foundArrayType =
frame.getStack().peekType(type.isCategory1() ? 2 : 3);
- Type requireArrayType;
+ Type requiredArrayType =
+ requiredArrayTypeFor(type, foundArrayType);
- if (foundArrayType != Type.KNOWN_NULL) {
- requireArrayType = foundArrayType;
- type = foundArrayType.getComponentType();
- } else {
- requireArrayType = type.getArrayType();
- }
+ // Make type agree with the discovered requiredArrayType.
+ type = requiredArrayType.getComponentType();
- machine.popArgs(frame, requireArrayType, Type.INT, type);
+ machine.popArgs(frame, requiredArrayType, Type.INT, type);
break;
}
case ByteOps.POP2:
@@ -367,8 +414,8 @@
case ByteOps.DUP_X1: {
ExecutionStack stack = frame.getStack();
- if (! (stack.peekType(0).isCategory1() &&
- stack.peekType(1).isCategory1())) {
+ if (!(stack.peekType(0).isCategory1() &&
+ stack.peekType(1).isCategory1())) {
throw illegalTos();
}
@@ -452,8 +499,8 @@
case ByteOps.SWAP: {
ExecutionStack stack = frame.getStack();
- if (! (stack.peekType(0).isCategory1() &&
- stack.peekType(1).isCategory1())) {
+ if (!(stack.peekType(0).isCategory1() &&
+ stack.peekType(1).isCategory1())) {
throw illegalTos();
}
@@ -486,7 +533,7 @@
* takes care of all the salient cases (types are the same,
* they're compatible primitive types, etc.).
*/
- if (! Merger.isPossiblyAssignableFrom(returnType, encountered)) {
+ if (!Merger.isPossiblyAssignableFrom(returnType, encountered)) {
throw new SimException("return type mismatch: prototype " +
"indicates " + returnType.toHuman() +
", but encountered type " + encountered.toHuman());
diff --git a/dx/src/com/android/dx/rop/type/Type.java b/dx/src/com/android/dx/rop/type/Type.java
index 16b16f5..244584d 100644
--- a/dx/src/com/android/dx/rop/type/Type.java
+++ b/dx/src/com/android/dx/rop/type/Type.java
@@ -27,7 +27,10 @@
* other using {@code ==}.
*/
public final class Type implements TypeBearer, Comparable<Type> {
- /** {@code non-null;} intern table mapping string descriptors to instances */
+ /**
+ * {@code non-null;} intern table mapping string descriptors to
+ * instances
+ */
private static final HashMap<String, Type> internTable =
new HashMap<String, Type>(500);
@@ -252,9 +255,9 @@
private final int newAt;
/**
- * {@code null-ok;} the internal-form class name corresponding to this type, if
- * calculated; only valid if {@code this} is a reference type and
- * additionally not a return address
+ * {@code null-ok;} the internal-form class name corresponding to
+ * this type, if calculated; only valid if {@code this} is a
+ * reference type and additionally not a return address
*/
private String className;
@@ -392,7 +395,8 @@
* with {@code "["} and calling {@code intern("L" + name + ";")}
* in all other cases.
*
- * @param name {@code non-null;} the name of the class whose type is desired
+ * @param name {@code non-null;} the name of the class whose type
+ * is desired
* @return {@code non-null;} the corresponding type
* @throws IllegalArgumentException thrown if the name has
* invalid syntax
diff --git a/dx/tests/105-verify-load-store-ops/expected.txt b/dx/tests/105-verify-load-store-ops/expected.txt
index 409ead0..b5db406 100644
--- a/dx/tests/105-verify-load-store-ops/expected.txt
+++ b/dx/tests/105-verify-load-store-ops/expected.txt
@@ -2,6 +2,8 @@
aaload: expected failure occurred
Generated: ./op_aastore.class
aastore: expected failure occurred
+Generated: ./op_aastore2.class
+aastore2: expected failure occurred
Generated: ./op_astore.class
astore: expected failure occurred
Generated: ./op_astore_0.class
diff --git a/dx/tests/105-verify-load-store-ops/op_aastore2.j b/dx/tests/105-verify-load-store-ops/op_aastore2.j
new file mode 100644
index 0000000..0083800
--- /dev/null
+++ b/dx/tests/105-verify-load-store-ops/op_aastore2.j
@@ -0,0 +1,27 @@
+; Copyright (C) 2010 The Android Open Source Project
+;
+; Licensed under the Apache License, Version 2.0 (the "License");
+; you may not use this file except in compliance with the License.
+; You may obtain a copy of the License at
+;
+; http://www.apache.org/licenses/LICENSE-2.0
+;
+; Unless required by applicable law or agreed to in writing, software
+; distributed under the License is distributed on an "AS IS" BASIS,
+; WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+; See the License for the specific language governing permissions and
+; limitations under the License.
+
+.class op_aastore2
+.super java/lang/Object
+
+.method public static test([I)V
+ .limit locals 2
+ .limit stack 4
+
+ aload_0
+ iconst_0
+ iconst_0
+ aastore
+ return
+.end method
diff --git a/dx/tests/105-verify-load-store-ops/run b/dx/tests/105-verify-load-store-ops/run
index adf987c..a6c5acc 100644
--- a/dx/tests/105-verify-load-store-ops/run
+++ b/dx/tests/105-verify-load-store-ops/run
@@ -27,6 +27,7 @@
oneop aaload
oneop aastore
+oneop aastore2 # second aastore test
oneop astore
oneop astore_0
oneop astore_1
