From d24414a5d1780ce25179f3467b228f9a53863fb4 Mon Sep 17 00:00:00 2001 From: Dan Bornstein Date: Fri, 17 Apr 2009 13:30:51 -0700 Subject: [PATCH] Fix issue #1794388. In particular, when encountering a known-null being used as an array, always emit an instruction that's compatible with the element type expected/encountered in context. This will sometimes lead to surprising instruction choices, such as "aget-byte" when the original source used a boolean[], but in all of these cases the output is valid and behaves correctly, where "correctly" in this case means (a) passing verification, and (b) throwing a NullPointerException if ever executed. The test case (dx test #111) reflects unsurprising expectations and so needs updating, which I will do in a follow-up to this patch. I know this makes me an awful git user, but I fixed some comments and whitespace while I was in the territory and *didn't* turn these into separate commits. --- dx/src/com/android/dx/Version.java | 2 +- dx/src/com/android/dx/cf/code/RopperMachine.java | 39 +++++++------ dx/src/com/android/dx/cf/code/Simulator.java | 67 ++++++++++++++-------- .../com/android/dx/cf/code/ValueAwareMachine.java | 3 +- dx/src/com/android/dx/rop/code/Rops.java | 33 +++++++---- 5 files changed, 91 insertions(+), 53 deletions(-) diff --git a/dx/src/com/android/dx/Version.java b/dx/src/com/android/dx/Version.java index 9e68d922b..4950d39fe 100644 --- a/dx/src/com/android/dx/Version.java +++ b/dx/src/com/android/dx/Version.java @@ -21,5 +21,5 @@ package com.android.dx; */ public class Version { /** {@code non-null;} version string */ - public static final String VERSION = "1.2"; + public static final String VERSION = "1.3"; } diff --git a/dx/src/com/android/dx/cf/code/RopperMachine.java b/dx/src/com/android/dx/cf/code/RopperMachine.java index 887cec97c..dd7fcd46b 100644 --- a/dx/src/com/android/dx/cf/code/RopperMachine.java +++ b/dx/src/com/android/dx/cf/code/RopperMachine.java @@ -259,16 +259,17 @@ import java.util.ArrayList; } /** - * @return true if a RET has ben encountered since the last call to - * startBlock() + * @return {@code true} if a {@code ret} has ben encountered since + * the last call to {@code startBlock()} */ public boolean hasRet() { return returnAddress != null; } /** - * @return {@code null-ok;} return address of a ret instruction if encountered - * since last call to startBlock(). null if no ret instruction encountered. + * @return {@code null-ok;} return address of a {@code ret} + * instruction if encountered since last call to startBlock(). + * {@code null} if no ret instruction encountered. */ public ReturnAddress getReturnAddress() { return returnAddress; @@ -444,7 +445,7 @@ import java.util.ArrayList; catches, MULTIANEWARRAY_METHOD); insns.add(insn); - // Add a move-result + // Add a move-result. rop = Rops.opMoveResult(MULTIANEWARRAY_METHOD.getPrototype() .getReturnType()); insn = new PlainInsn(rop, pos, objectReg, RegisterSpecList.EMPTY); @@ -457,7 +458,6 @@ import java.util.ArrayList; opcode = ByteOps.CHECKCAST; sources = RegisterSpecList.make(objectReg); - } else if (opcode == ByteOps.JSR) { // JSR has no Rop instruction hasJsr = true; @@ -474,12 +474,14 @@ import java.util.ArrayList; } ropOpcode = jopToRopOpcode(opcode, cst); - rop = Rops.ropFor(ropOpcode, destType, sources, cst); Insn moveResult = null; if (dest != null && rop.isCallLike()) { - // We're going to want to have a move-result in the next basic block + /* + * We're going to want to have a move-result in the next + * basic block. + */ extraBlockCount++; moveResult = new PlainInsn( @@ -488,8 +490,10 @@ import java.util.ArrayList; dest = null; } else if (dest != null && rop.canThrow()) { - // We're going to want to have a move-result-pseudo - // in the next basic block + /* + * We're going to want to have a move-result-pseudo in the + * next basic block. + */ extraBlockCount++; moveResult = new PlainInsn( @@ -599,11 +603,12 @@ import java.util.ArrayList; } /* - * If initValues is non-null, it means that the parser has seen a group - * of compatible constant initialization bytecodes that are applied to - * the current newarray. The action we take here is to convert these - * initialization bytecodes into a single fill-array-data ROP which lays - * out all the constant values in a table. + * If initValues is non-null, it means that the parser has + * seen a group of compatible constant initialization + * bytecodes that are applied to the current newarray. The + * action we take here is to convert these initialization + * bytecodes into a single fill-array-data ROP which lays out + * all the constant values in a table. */ if (initValues != null) { extraBlockCount++; @@ -619,8 +624,8 @@ import java.util.ArrayList; * instruction. * * @param opcode the opcode being translated - * @param stackPointer {@code >= 0;} the stack pointer after the instruction's - * arguments have been popped + * @param stackPointer {@code >= 0;} the stack pointer after the + * instruction's arguments have been popped * @return {@code non-null;} the sources */ private RegisterSpecList getSources(int opcode, int stackPointer) { diff --git a/dx/src/com/android/dx/cf/code/Simulator.java b/dx/src/com/android/dx/cf/code/Simulator.java index 344759cb9..408e1261d 100644 --- a/dx/src/com/android/dx/cf/code/Simulator.java +++ b/dx/src/com/android/dx/cf/code/Simulator.java @@ -35,14 +35,17 @@ import java.util.ArrayList; /** * Class which knows how to simulate the effects of executing bytecode. - * + * *

Note: This class is not thread-safe. If multiple threads * need to use a single instance, they must synchronize access explicitly * between themselves.

*/ public class Simulator { - /** {@code non-null;} canned error message for local variable table mismatches */ - private static final String LOCAL_MISMATCH_ERROR = + /** + * {@code non-null;} canned error message for local variable + * table mismatches + */ + private static final String LOCAL_MISMATCH_ERROR = "This is symptomatic of .class transformation tools that ignore " + "local variable information."; @@ -131,7 +134,7 @@ public class Simulator { private class SimVisitor implements BytecodeArray.Visitor { /** * {@code non-null;} machine instance to use (just to avoid excessive - * cross-object field access) + * cross-object field access) */ private final Machine machine; @@ -255,25 +258,21 @@ public class Simulator { /* * Change the type (which is to be pushed) to * reflect the actual component type of the array - * being popped. + * being popped, unless it turns out to be a + * known-null, in which case we just use the type + * implied by the original instruction. */ - Type requireType = type.getArrayType(); - type = frame.getStack().peekType(1); - if (type == Type.KNOWN_NULL) { - /* - * The type is a known-null: Just treat the - * popped type as whatever is expected. In - * reality, unless this frame is revisited - * (due to a branch merge), execution will - * result in the throwing of a - * NullPointerException, but claiming the - * expected type at here should be good enough - * for the purposes at this level. - */ - type = requireType; + Type foundArrayType = frame.getStack().peekType(1); + Type requireArrayType; + + if (foundArrayType != Type.KNOWN_NULL) { + requireArrayType = foundArrayType; + type = foundArrayType.getComponentType(); + } else { + requireArrayType = type.getArrayType(); } - type = type.getComponentType(); - machine.popArgs(frame, requireType, Type.INT); + + machine.popArgs(frame, requireArrayType, Type.INT); break; } case ByteOps.IADD: @@ -292,7 +291,7 @@ public class Simulator { case ByteOps.IUSHR: { machine.popArgs(frame, type, Type.INT); break; - } + } case ByteOps.LCMP: { machine.popArgs(frame, Type.LONG, Type.LONG); break; @@ -308,8 +307,28 @@ public class Simulator { break; } case ByteOps.IASTORE: { - Type arrayType = type.getArrayType(); - machine.popArgs(frame, arrayType, Type.INT, type); + /* + * 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. + */ + Type foundArrayType = + frame.getStack().peekType(type.isCategory1() ? 2 : 3); + Type requireArrayType; + + if (foundArrayType != Type.KNOWN_NULL) { + requireArrayType = foundArrayType; + type = foundArrayType.getComponentType(); + } else { + requireArrayType = type.getArrayType(); + } + + machine.popArgs(frame, requireArrayType, Type.INT, type); break; } case ByteOps.POP2: diff --git a/dx/src/com/android/dx/cf/code/ValueAwareMachine.java b/dx/src/com/android/dx/cf/code/ValueAwareMachine.java index b0f993d81..43aab8a67 100644 --- a/dx/src/com/android/dx/cf/code/ValueAwareMachine.java +++ b/dx/src/com/android/dx/cf/code/ValueAwareMachine.java @@ -30,7 +30,8 @@ public class ValueAwareMachine extends BaseMachine { /** * Constructs an instance. * - * @param prototype {@code non-null;} the prototype for the associated method + * @param prototype {@code non-null;} the prototype for the associated + * method */ public ValueAwareMachine(Prototype prototype) { super(prototype); diff --git a/dx/src/com/android/dx/rop/code/Rops.java b/dx/src/com/android/dx/rop/code/Rops.java index 472212a84..15c2e17ed 100644 --- a/dx/src/com/android/dx/rop/code/Rops.java +++ b/dx/src/com/android/dx/rop/code/Rops.java @@ -1113,13 +1113,14 @@ public final class Rops { * match what is returned. TODO: Revisit this issue.

* * @param opcode the opcode - * @param dest {@code non-null;} destination type, or {@link Type#VOID} if none + * @param dest {@code non-null;} destination (result) type, or + * {@link Type#VOID} if none * @param sources {@code non-null;} list of source types * @param cst {@code null-ok;} associated constant, if any * @return {@code non-null;} an appropriate instance */ public static Rop ropFor(int opcode, TypeBearer dest, TypeList sources, - Constant cst) { + Constant cst) { switch (opcode) { case RegOps.NOP: return NOP; case RegOps.MOVE: return opMove(dest); @@ -1165,19 +1166,31 @@ public final class Rops { case RegOps.MONITOR_EXIT: return MONITOR_EXIT; case RegOps.AGET: { Type source = sources.getType(0); + Type componentType; if (source == Type.KNOWN_NULL) { - // Treat a known-null as an Object[] in this context. - source = Type.OBJECT_ARRAY; - } - return opAget(source.getComponentType()); + /* + * Treat a known-null as an array of the expected + * result type. + */ + componentType = dest.getType(); + } else { + componentType = source.getComponentType(); + } + return opAget(componentType); } case RegOps.APUT: { Type source = sources.getType(1); + Type componentType; if (source == Type.KNOWN_NULL) { - // Treat a known-null as an Object[] in this context. - source = Type.OBJECT_ARRAY; - } - return opAput(source.getComponentType()); + /* + * Treat a known-null as an array of the type being + * stored. + */ + componentType = sources.getType(0); + } else { + componentType = source.getComponentType(); + } + return opAput(componentType); } case RegOps.NEW_INSTANCE: return NEW_INSTANCE; case RegOps.NEW_ARRAY: return opNewArray(dest.getType()); -- 2.11.0