At least mostly-fix internal issue #1898095 / public issue #2868.
The problem is that Scala code may have two variables with the same
name that are simultaneously active, and dx didn't expect that ever to
be the case. The fix is simply to catch another case where a local
variable might be considered to become superfluous due to having an
empty extent. However, this is really more of a workaround, as the
result will be that such code when attached to a debugger will only
show one of the two (or more) same-named variables as being active at
any given time.
The thing that prevents a more satisfactory solution is that the
optimization code can end up "splitting up" a single local into
multiple registers, and the only way we have to track them is by
name/type. So, even without Scala's rules, it was already possible to
find a same-named local in multiple registers, but with the status
quo, finding that really meant that a local was moving from one
register to another. The local variable table code handles that case
by automatically "ending" variables that appear to move, and the Scala
case looks just like that.
In the long run, we probably want to have a way to tag local variables
that isn't just name/type, but that will have to come later.
(As a bonus, I did some minor renaming for clarity while I was in the
territory.)
diff --git a/dx/src/com/android/dx/dex/code/LocalList.java b/dx/src/com/android/dx/dex/code/LocalList.java
index 326a6c6..93e7c3f 100644
--- a/dx/src/com/android/dx/dex/code/LocalList.java
+++ b/dx/src/com/android/dx/dex/code/LocalList.java
@@ -38,10 +38,10 @@
/** whether to run the self-check code */
private static final boolean DEBUG = false;
-
+
/**
* Constructs an instance. All indices initially contain {@code null}.
- *
+ *
* @param size {@code >= 0;} the size of the list
*/
public LocalList(int size) {
@@ -52,7 +52,7 @@
* Gets the element at the given index. It is an error to call
* this with the index for an element which was never set; if you
* do that, this will throw {@code NullPointerException}.
- *
+ *
* @param n {@code >= 0, < size();} which index
* @return {@code non-null;} element at that index
*/
@@ -62,7 +62,7 @@
/**
* Sets the entry at the given index.
- *
+ *
* @param n {@code >= 0, < size();} which index
* @param entry {@code non-null;} the entry to set at {@code n}
*/
@@ -72,7 +72,7 @@
/**
* Does a human-friendly dump of this instance.
- *
+ *
* @param out {@code non-null;} where to dump
* @param prefix {@code non-null;} prefix to attach to each line of output
*/
@@ -90,7 +90,7 @@
*/
public static enum Disposition {
/** local started (introduced) */
- START,
+ START,
/** local ended without being replaced */
END_SIMPLY,
@@ -129,11 +129,11 @@
/** {@code non-null;} variable type (derived from {@code spec}) */
private final CstType type;
-
+
/**
* Constructs an instance.
- *
- * @param address {@code >= 0;} address
+ *
+ * @param address {@code >= 0;} address
* @param disposition {@code non-null;} disposition of the local
* @param spec {@code non-null;} register spec representing
* the variable
@@ -182,7 +182,7 @@
* Compares by (in priority order) address, end then start
* disposition (variants of end are all consistered
* equivalent), and spec.
- *
+ *
* @param other {@code non-null;} entry to compare to
* @return {@code -1..1;} standard result of comparison
*/
@@ -195,7 +195,7 @@
boolean thisIsStart = isStart();
boolean otherIsStart = other.isStart();
-
+
if (thisIsStart != otherIsStart) {
return thisIsStart ? 1 : -1;
}
@@ -205,7 +205,7 @@
/**
* Gets the address.
- *
+ *
* @return {@code >= 0;} the address
*/
public int getAddress() {
@@ -214,7 +214,7 @@
/**
* Gets the disposition.
- *
+ *
* @return {@code non-null;} the disposition
*/
public Disposition getDisposition() {
@@ -224,7 +224,7 @@
/**
* Gets whether this is a local start. This is just shorthand for
* {@code getDisposition() == Disposition.START}.
- *
+ *
* @return {@code true} iff this is a start
*/
public boolean isStart() {
@@ -233,7 +233,7 @@
/**
* Gets the variable name.
- *
+ *
* @return {@code null-ok;} the variable name
*/
public CstUtf8 getName() {
@@ -251,7 +251,7 @@
/**
* Gets the variable's type.
- *
+ *
* @return {@code non-null;} the type
*/
public CstType getType() {
@@ -260,7 +260,7 @@
/**
* Gets the number of the register holding the variable.
- *
+ *
* @return {@code >= 0;} the number of the register holding
* the variable
*/
@@ -279,7 +279,7 @@
/**
* Returns whether or not this instance matches the given spec.
- *
+ *
* @param spec {@code non-null;} the spec in question
* @return {@code true} iff this instance matches
* {@code spec}
@@ -302,8 +302,8 @@
/**
* Returns an instance just like this one but with the disposition
- * set as given
- *
+ * set as given.
+ *
* @param disposition {@code non-null;} the new disposition
* @return {@code non-null;} an appropriately-constructed instance
*/
@@ -311,17 +311,17 @@
if (disposition == this.disposition) {
return this;
}
-
+
return new Entry(address, disposition, spec);
}
}
-
+
/**
* Constructs an instance for the given method, based on the given
* block order and intermediate local information.
- *
+ *
* @param insns {@code non-null;} instructions to convert
- * @return {@code non-null;} the constructed list
+ * @return {@code non-null;} the constructed list
*/
public static LocalList make(DalvInsnList insns) {
int sz = insns.size();
@@ -332,8 +332,8 @@
* into separate per-variable starts, adding explicit ends
* wherever a variable is replaced or moved, and collecting
* these and all the other local variable "activity"
- * together into an output list (without the other insns).
- *
+ * together into an output list (without the other insns).
+ *
* Note: As of this writing, this method won't be handed any
* insn lists that contain local ends, but I (danfuzz) expect
* that to change at some point, when we start feeding that
@@ -384,9 +384,9 @@
}
throw ex;
}
-
+
}
-
+
/**
* Helper for {@link #debugVerify} which does most of the work.
*/
@@ -413,7 +413,7 @@
throw new RuntimeException("redundant end at " +
Integer.toHexString(e.getAddress()));
}
-
+
int addr = e.getAddress();
boolean foundStart = false;
@@ -435,7 +435,7 @@
throw new RuntimeException(
"redundant end at " +
Integer.toHexString(addr));
- }
+ }
}
}
@@ -445,7 +445,7 @@
"improper end replacement claim at " +
Integer.toHexString(addr));
}
-
+
active[reg] = null;
}
}
@@ -493,7 +493,7 @@
*/
private void aboutToProcess(int address, int reg) {
boolean first = (endIndices == null);
-
+
if ((address == lastAddress) && !first) {
return;
}
@@ -529,11 +529,15 @@
* Sets the local state at the given address to the given snapshot.
* The first call on this instance must be to this method, so that
* the register state can be properly sized.
- *
+ *
* @param address {@code >= 0;} the address
* @param specs {@code non-null;} spec set representing the locals
*/
public void snapshot(int address, RegisterSpecSet specs) {
+ if (DEBUG) {
+ System.err.printf("%04x snapshot %s\n", address, specs);
+ }
+
int sz = specs.getMaxSize();
aboutToProcess(address, sz - 1);
@@ -552,16 +556,24 @@
startLocal(address, newSpec);
}
}
+
+ if (DEBUG) {
+ System.err.printf("%04x snapshot done\n", address);
+ }
}
-
+
/**
* Starts a local at the given address.
- *
+ *
* @param address {@code >= 0;} the address
* @param startedLocal {@code non-null;} spec representing the
* started local
*/
public void startLocal(int address, RegisterSpec startedLocal) {
+ if (DEBUG) {
+ System.err.printf("%04x start %s\n", address, startedLocal);
+ }
+
int regNum = startedLocal.getReg();
startedLocal = filterSpec(startedLocal);
@@ -584,7 +596,7 @@
}
int endAt = endIndices[regNum];
-
+
if (existingLocal != null) {
/*
* There is an existing (but non-matching) local.
@@ -629,8 +641,8 @@
}
}
}
-
- /*
+
+ /*
* The code above didn't find and remove an unnecessary
* local end, so we now have to add one or more entries to
* the output to capture the transition.
@@ -668,18 +680,36 @@
* if any (that is, if the local migrates from vX to vY,
* we should note that as a local end in vX).
*/
-
+
add(address, Disposition.START, startedLocal);
}
/**
+ * Ends a local at the given address, using the disposition
+ * {@code END_SIMPLY}.
+ *
+ * @param address {@code >= 0;} the address
+ * @param endedLocal {@code non-null;} spec representing the
+ * local being ended
+ */
+ public void endLocal(int address, RegisterSpec endedLocal) {
+ endLocal(address, endedLocal, Disposition.END_SIMPLY);
+ }
+
+ /**
* Ends a local at the given address.
*
* @param address {@code >= 0;} the address
* @param endedLocal {@code non-null;} spec representing the
* local being ended
+ * @param disposition reason for the end
*/
- public void endLocal(int address, RegisterSpec endedLocal) {
+ public void endLocal(int address, RegisterSpec endedLocal,
+ Disposition disposition) {
+ if (DEBUG) {
+ System.err.printf("%04x end %s\n", address, endedLocal);
+ }
+
int regNum = endedLocal.getReg();
endedLocal = filterSpec(endedLocal);
@@ -700,7 +730,7 @@
return;
}
- add(address, Disposition.END_SIMPLY, endedLocal);
+ add(address, disposition, endedLocal);
}
/**
@@ -711,7 +741,7 @@
* active), update the {@link #endIndices} to be accurate, and
* if needed update the newly-active end to reflect an altered
* disposition.
- *
+ *
* @param address {@code >= 0;} the address
* @param endedLocal {@code non-null;} spec representing the
* local being ended
@@ -745,7 +775,7 @@
* In fact, we found that the endedLocal had started at the
* same address, so do all the requisite cleanup.
*/
-
+
regs.remove(endedLocal);
result.set(at, null);
nullResultCount++;
@@ -771,7 +801,7 @@
if (found) {
// We found an end for the same register.
endIndices[regNum] = at;
-
+
if (entry.getAddress() == address) {
/*
* It's still the same address, so update the
@@ -791,11 +821,11 @@
* null" type into simply {@code Object}. This method needs to
* be called for any spec that is on its way into a locals
* list.
- *
+ *
* <p>This isn't necessarily the cleanest way to achieve the
* goal of not representing known nulls in a locals list, but
* it gets the job done.</p>
- *
+ *
* @param orig {@code null-ok;} the original spec
* @return {@code null-ok;} an appropriately modified spec, or the
* original if nothing needs to be done
@@ -832,8 +862,10 @@
}
/**
- * Adds or updates an end local (changing its disposition).
- *
+ * Adds or updates an end local (changing its disposition). If
+ * this would cause an empty range for a local, this instead
+ * removes the local entirely.
+ *
* @param address {@code >= 0;} the address
* @param disposition {@code non-null;} the disposition
* @param spec {@code non-null;} spec representing the local
@@ -848,21 +880,26 @@
int endAt = endIndices[regNum];
if (endAt >= 0) {
+ // There is a previous end.
Entry endEntry = result.get(endAt);
if ((endEntry.getAddress() == address) &&
endEntry.getRegisterSpec().equals(spec)) {
+ /*
+ * The end is for the right address and variable, so
+ * update it.
+ */
result.set(endAt, endEntry.withDisposition(disposition));
- regs.remove(spec);
+ regs.remove(spec); // TODO: Is this line superfluous?
return;
}
}
-
- add(address, disposition, spec);
+
+ endLocal(address, spec, disposition);
}
/**
* Finishes processing altogether and gets the result.
- *
+ *
* @return {@code non-null;} the result list
*/
public LocalList finish() {
@@ -870,7 +907,7 @@
int resultSz = result.size();
int finalSz = resultSz - nullResultCount;
-
+
if (finalSz == 0) {
return EMPTY;
}
@@ -907,5 +944,5 @@
resultList.setImmutable();
return resultList;
}
- }
+ }
}
diff --git a/dx/src/com/android/dx/dex/file/DebugInfoEncoder.java b/dx/src/com/android/dx/dex/file/DebugInfoEncoder.java
index e4bed65..1cd24af 100644
--- a/dx/src/com/android/dx/dex/file/DebugInfoEncoder.java
+++ b/dx/src/com/android/dx/dex/file/DebugInfoEncoder.java
@@ -96,8 +96,8 @@
/**
* Creates an instance.
*
- * @param pl {@code null-ok;} positions (line numbers) to encode
- * @param ll {@code null-ok;} local variables to encode
+ * @param positions {@code null-ok;} positions (line numbers) to encode
+ * @param locals {@code null-ok;} local variables to encode
* @param file {@code null-ok;} may only be {@code null} if simply using
* this class to do a debug print
* @param codeSize
@@ -105,19 +105,18 @@
* @param isStatic
* @param ref
*/
- public DebugInfoEncoder(PositionList pl, LocalList ll,
+ public DebugInfoEncoder(PositionList positions, LocalList locals,
DexFile file, int codeSize, int regSize,
boolean isStatic, CstMethodRef ref) {
- this.positions = pl;
- this.locals = ll;
+ this.positions = positions;
+ this.locals = locals;
this.file = file;
- output = new ByteArrayAnnotatedOutput();
this.desc = ref.getPrototype();
this.isStatic = isStatic;
-
this.codeSize = codeSize;
this.regSize = regSize;
+ output = new ByteArrayAnnotatedOutput();
lastEntryForReg = new LocalList.Entry[regSize];
}
@@ -190,7 +189,7 @@
return result;
}
-
+
private byte[] convert0() throws IOException {
ArrayList<PositionList.Entry> sortedPositions = buildSortedPositions();
ArrayList<LocalList.Entry> methodArgs = extractMethodArguments();
@@ -204,21 +203,22 @@
annotate(1, String.format("%04x: prologue end",address));
}
- int szp = sortedPositions.size();
- int szl = locals.size();
+ int positionsSz = sortedPositions.size();
+ int localsSz = locals.size();
// Current index in sortedPositions
- int curp = 0;
+ int curPositionIdx = 0;
// Current index in locals
- int curl = 0;
+ int curLocalIdx = 0;
for (;;) {
/*
* Emit any information for the current address.
*/
- curl = emitLocalsAtAddress(curl);
- curp = emitPositionsAtAddress(curp, sortedPositions);
+ curLocalIdx = emitLocalsAtAddress(curLocalIdx);
+ curPositionIdx =
+ emitPositionsAtAddress(curPositionIdx, sortedPositions);
/*
* Figure out what the next important address is.
@@ -227,12 +227,12 @@
int nextAddrL = Integer.MAX_VALUE; // local variable
int nextAddrP = Integer.MAX_VALUE; // position (line number)
- if (curl < szl) {
- nextAddrL = locals.get(curl).getAddress();
+ if (curLocalIdx < localsSz) {
+ nextAddrL = locals.get(curLocalIdx).getAddress();
}
- if (curp < szp) {
- nextAddrP = sortedPositions.get(curp).getAddress();
+ if (curPositionIdx < positionsSz) {
+ nextAddrP = sortedPositions.get(curPositionIdx).getAddress();
}
int next = Math.min(nextAddrP, nextAddrL);
@@ -249,12 +249,12 @@
if (next == codeSize
&& nextAddrL == Integer.MAX_VALUE
&& nextAddrP == Integer.MAX_VALUE) {
- break;
+ break;
}
if (next == nextAddrP) {
// Combined advance PC + position entry
- emitPosition(sortedPositions.get(curp++));
+ emitPosition(sortedPositions.get(curPositionIdx++));
} else {
emitAdvancePc(next - address);
}
@@ -271,96 +271,96 @@
* locals} and including all subsequent activity at the same
* address.
*
- * @param curl Current index in locals
- * @return new value for {@code curl}
+ * @param curLocalIdx Current index in locals
+ * @return new value for {@code curLocalIdx}
* @throws IOException
*/
- private int emitLocalsAtAddress(int curl)
+ private int emitLocalsAtAddress(int curLocalIdx)
throws IOException {
int sz = locals.size();
// TODO: Don't emit ends implied by starts.
- while ((curl < sz)
- && (locals.get(curl).getAddress() == address)) {
- LocalList.Entry lle = locals.get(curl++);
- int reg = lle.getRegister();
- LocalList.Entry prevlle = lastEntryForReg[reg];
+ while ((curLocalIdx < sz)
+ && (locals.get(curLocalIdx).getAddress() == address)) {
+ LocalList.Entry entry = locals.get(curLocalIdx++);
+ int reg = entry.getRegister();
+ LocalList.Entry prevEntry = lastEntryForReg[reg];
- if (lle == prevlle) {
+ if (entry == prevEntry) {
/*
* Here we ignore locals entries for parameters,
* which have already been represented and placed in the
* lastEntryForReg array.
*/
continue;
- }
+ }
// At this point we have a new entry one way or another.
- lastEntryForReg[reg] = lle;
+ lastEntryForReg[reg] = entry;
- if (lle.isStart()) {
- if ((prevlle != null) && lle.matches(prevlle)) {
+ if (entry.isStart()) {
+ if ((prevEntry != null) && entry.matches(prevEntry)) {
/*
* The previous local in this register has the same
* name and type as the one being introduced now, so
* use the more efficient "restart" form.
*/
- if (prevlle.isStart()) {
+ if (prevEntry.isStart()) {
/*
* We should never be handed a start when a
* a matching local is already active.
*/
throw new RuntimeException("shouldn't happen");
}
- emitLocalRestart(lle);
+ emitLocalRestart(entry);
} else {
- emitLocalStart(lle);
+ emitLocalStart(entry);
}
} else {
/*
* Only emit a local end if it is *not* due to a direct
* replacement. Direct replacements imply an end of the
* previous local in the same register.
- *
+ *
* TODO: Make sure the runtime can deal with implied
* local ends from category-2 interactions, and when so,
* also stop emitting local ends for those cases.
*/
- if (lle.getDisposition()
+ if (entry.getDisposition()
!= LocalList.Disposition.END_REPLACED) {
- emitLocalEnd(lle);
+ emitLocalEnd(entry);
}
}
}
- return curl;
+ return curLocalIdx;
}
/**
* Emits all positions that occur at the current {@code address}
*
- * @param curp Current index in sortedPositions
+ * @param curPositionIdx Current index in sortedPositions
* @param sortedPositions positions, sorted by ascending address
- * @return new value for {@code curp}
+ * @return new value for {@code curPositionIdx}
* @throws IOException
*/
- private int emitPositionsAtAddress(int curp,
+ private int emitPositionsAtAddress(int curPositionIdx,
ArrayList<PositionList.Entry> sortedPositions)
throws IOException {
-
- int szp = sortedPositions.size();
- while (curp < szp
- && sortedPositions.get(curp).getAddress() == address) {
- emitPosition(sortedPositions.get(curp++));
+ int positionsSz = sortedPositions.size();
+ while ((curPositionIdx < positionsSz)
+ && (sortedPositions.get(curPositionIdx).getAddress()
+ == address)) {
+ emitPosition(sortedPositions.get(curPositionIdx++));
}
- return curp;
+ return curPositionIdx;
}
/**
* Emits the header sequence, which consists of LEB128-encoded initial
* line number and string indicies for names of all non-"this" arguments.
- *
+ *
* @param sortedPositions positions, sorted by ascending address
* @param methodArgs local list entries for method argumens arguments,
* in left-to-right order omitting "this"
@@ -406,7 +406,7 @@
output.writeUnsignedLeb128(szParamTypes);
if (annotate) {
- annotate(output.getCursor() - mark,
+ annotate(output.getCursor() - mark,
String.format("parameters_size: %04x", szParamTypes));
}
@@ -833,7 +833,7 @@
if (deltaLines < DBG_LINE_BASE
|| deltaLines > (DBG_LINE_BASE + DBG_LINE_RANGE -1)) {
- throw new RuntimeException("Parameter out of range");
+ throw new RuntimeException("Parameter out of range");
}
return (deltaLines - DBG_LINE_BASE)
@@ -865,7 +865,7 @@
}
/**
- * Emits an {@link DebugInfoConstants#DBG_ADVANCE_PC DBG_ADVANCE_PC}
+ * Emits an {@link DebugInfoConstants#DBG_ADVANCE_PC DBG_ADVANCE_PC}
* sequence.
*
* @param deltaAddress {@code >= 0;} amount to change program counter by