Compute local info at exceptional blocks based on its predecessor.
This CL ends the visibility of locals that are clobbered as part of spill-moves
in the header of an exceptional block. Also added regression test that observed
incorrect locals information on entry to an exceptional block.
R=ager
Change-Id: I50459fe34cf4ebc1c378e3ddfadba6a4b1de74cf
diff --git a/src/main/java/com/android/tools/r8/ir/code/BasicBlock.java b/src/main/java/com/android/tools/r8/ir/code/BasicBlock.java
index 16bc790..44721ae 100644
--- a/src/main/java/com/android/tools/r8/ir/code/BasicBlock.java
+++ b/src/main/java/com/android/tools/r8/ir/code/BasicBlock.java
@@ -4,6 +4,7 @@
package com.android.tools.r8.ir.code;
import com.android.tools.r8.errors.CompilationError;
+import com.android.tools.r8.errors.Unreachable;
import com.android.tools.r8.graph.DebugLocalInfo;
import com.android.tools.r8.graph.DexType;
import com.android.tools.r8.ir.conversion.DexBuilder;
@@ -378,6 +379,18 @@
return instructions.get(instructions.size() - 1).asJumpInstruction();
}
+ public Instruction exceptionalExit() {
+ assert hasCatchHandlers();
+ ListIterator<Instruction> it = listIterator(instructions.size());
+ while (it.hasPrevious()) {
+ Instruction instruction = it.previous();
+ if (instruction.instructionTypeCanThrow()) {
+ return instruction;
+ }
+ }
+ throw new Unreachable();
+ }
+
public void clearUserInfo() {
phis = null;
instructions.forEach(Instruction::clearUserInfo);
diff --git a/src/main/java/com/android/tools/r8/ir/code/DebugLocalsChange.java b/src/main/java/com/android/tools/r8/ir/code/DebugLocalsChange.java
index a08b044..0a1cc1b 100644
--- a/src/main/java/com/android/tools/r8/ir/code/DebugLocalsChange.java
+++ b/src/main/java/com/android/tools/r8/ir/code/DebugLocalsChange.java
@@ -18,6 +18,7 @@
public DebugLocalsChange(
Int2ReferenceMap<DebugLocalInfo> ending, Int2ReferenceMap<DebugLocalInfo> starting) {
super(null);
+ assert !ending.isEmpty() || !starting.isEmpty();
this.ending = ending;
this.starting = starting;
}
diff --git a/src/main/java/com/android/tools/r8/ir/regalloc/LinearScanRegisterAllocator.java b/src/main/java/com/android/tools/r8/ir/regalloc/LinearScanRegisterAllocator.java
index 1e45d52..3a77a52 100644
--- a/src/main/java/com/android/tools/r8/ir/regalloc/LinearScanRegisterAllocator.java
+++ b/src/main/java/com/android/tools/r8/ir/regalloc/LinearScanRegisterAllocator.java
@@ -29,9 +29,10 @@
import com.android.tools.r8.utils.StringUtils;
import com.google.common.collect.HashMultiset;
import com.google.common.collect.Multiset;
-import com.google.common.collect.Multiset.Entry;
import com.google.common.collect.Multisets;
import it.unimi.dsi.fastutil.ints.Int2ReferenceMap;
+import it.unimi.dsi.fastutil.ints.Int2ReferenceMap.Entry;
+import it.unimi.dsi.fastutil.ints.Int2ReferenceMaps;
import it.unimi.dsi.fastutil.ints.Int2ReferenceOpenHashMap;
import it.unimi.dsi.fastutil.ints.IntArraySet;
import it.unimi.dsi.fastutil.ints.IntIterator;
@@ -75,6 +76,7 @@
}
private static class LocalRange implements Comparable<LocalRange> {
+ final Value value;
final DebugLocalInfo local;
final int register;
final int start;
@@ -82,6 +84,7 @@
LocalRange(Value value, int register, int start, int end) {
assert value.getLocalInfo() != null;
+ this.value = value;
this.local = value.getLocalInfo();
this.register = register;
this.start = start;
@@ -252,7 +255,7 @@
// information, we always use the argument register whenever a local corresponds to an
// argument value. That avoids ending and restarting locals whenever we move arguments
// to lower register.
- int register = getRegisterForValue(value, value.isArgument() ? 0 : start);
+ int register = getArgumentOrAllocateRegisterForValue(value, start);
ranges.add(new LocalRange(value, register, start, nextEnd));
Integer nextStart = nextInRange(nextEnd, end, starts);
if (nextStart == null) {
@@ -262,7 +265,8 @@
start = nextStart;
}
if (start >= 0) {
- ranges.add(new LocalRange(value, getRegisterForValue(value, start), start, end));
+ ranges.add(new LocalRange(value, getArgumentOrAllocateRegisterForValue(value, start),
+ start, end));
}
}
}
@@ -287,7 +291,7 @@
}
for (BasicBlock block : blocks) {
- block.setLocalsAtEntry(new Int2ReferenceOpenHashMap<>(currentLocals));
+ boolean blockEntry = true;
ListIterator<Instruction> instructionIterator = block.listIterator();
while (instructionIterator.hasNext()) {
Instruction instruction = instructionIterator.next();
@@ -316,15 +320,25 @@
}
nextStartingRange = rangeIterator.hasNext() ? rangeIterator.next() : null;
}
- if (localsChanged && shouldEmitChangesAtInstruction(instruction)) {
- DebugLocalsChange change = createLocalsChange(ending, starting);
- if (change != null) {
- if (instruction.isDebugPosition() || instruction.isJumpInstruction()) {
- instructionIterator.previous();
- instructionIterator.add(new DebugLocalsChange(ending, starting));
- instructionIterator.next();
- } else {
- instructionIterator.add(new DebugLocalsChange(ending, starting));
+
+ if (blockEntry) {
+ blockEntry = false;
+ if (instruction.isMoveException()) {
+ fixupSpillMovesAtMoveException(block, instructionIterator, openRanges, currentLocals);
+ } else {
+ block.setLocalsAtEntry(new Int2ReferenceOpenHashMap<>(currentLocals));
+ }
+ } else {
+ if (localsChanged && shouldEmitChangesAtInstruction(instruction)) {
+ DebugLocalsChange change = createLocalsChange(ending, starting);
+ if (change != null) {
+ if (instruction.isDebugPosition() || instruction.isJumpInstruction()) {
+ instructionIterator.previous();
+ instructionIterator.add(change);
+ instructionIterator.next();
+ } else {
+ instructionIterator.add(change);
+ }
}
}
}
@@ -333,13 +347,83 @@
}
}
+ private void fixupSpillMovesAtMoveException(
+ BasicBlock block,
+ ListIterator<Instruction> instructionIterator,
+ List<LocalRange> openRanges,
+ Int2ReferenceMap<DebugLocalInfo> finalLocals) {
+ Int2ReferenceMap<DebugLocalInfo> initialLocals = new Int2ReferenceOpenHashMap<>();
+ int exceptionalIndex = block.getPredecessors().get(0).exceptionalExit().getNumber();
+ for (LocalRange open : openRanges) {
+ int exceptionalRegister = getArgumentOrAllocateRegisterForValue(open.value, exceptionalIndex);
+ initialLocals.put(exceptionalRegister, open.local);
+ }
+ block.setLocalsAtEntry(new Int2ReferenceOpenHashMap<>(initialLocals));
+ Int2ReferenceMap<DebugLocalInfo> clobberedLocals = new Int2ReferenceOpenHashMap<>();
+ Iterator<Instruction> moveIterator = block.iterator();
+ assert block.entry().isMoveException();
+ int index = block.entry().getNumber();
+ moveIterator.next();
+ while (moveIterator.hasNext()) {
+ Instruction next = moveIterator.next();
+ if (next.getNumber() != -1) {
+ break;
+ }
+ if (clobberedLocals.isEmpty()) {
+ // Advance the iterator so it ends up at the first move that clobbers a local.
+ instructionIterator.next();
+ }
+ if (next.isMove()) {
+ Move move = next.asMove();
+ int dstRegister = getArgumentOrAllocateRegisterForValue(move.dest(), index);
+ DebugLocalInfo dstInitialLocal = initialLocals.get(dstRegister);
+ DebugLocalInfo dstFinalLocal = finalLocals.get(dstRegister);
+ if (dstInitialLocal != null && dstInitialLocal != dstFinalLocal) {
+ initialLocals.remove(dstRegister);
+ clobberedLocals.put(dstRegister, dstInitialLocal);
+ }
+ }
+ }
+ // Add an initial local change for all clobbered locals after the first clobbered local.
+ if (!clobberedLocals.isEmpty()) {
+ instructionIterator.add(new DebugLocalsChange(
+ clobberedLocals, Int2ReferenceMaps.emptyMap()));
+ }
+ // Compute the final change in locals and emit it after all spill moves.
+ while (instructionIterator.hasNext()) {
+ if (instructionIterator.next().getNumber() != -1) {
+ instructionIterator.previous();
+ break;
+ }
+ }
+ Int2ReferenceMap<DebugLocalInfo> ending = new Int2ReferenceOpenHashMap<>();
+ Int2ReferenceMap<DebugLocalInfo> starting = new Int2ReferenceOpenHashMap<>();
+ for (Entry<DebugLocalInfo> initialLocal : initialLocals.int2ReferenceEntrySet()) {
+ if (finalLocals.get(initialLocal.getIntKey()) != initialLocal.getValue()) {
+ ending.put(initialLocal.getIntKey(), initialLocal.getValue());
+ }
+ }
+ for (Entry<DebugLocalInfo> finalLocal : finalLocals.int2ReferenceEntrySet()) {
+ if (initialLocals.get(finalLocal.getIntKey()) != finalLocal.getValue()) {
+ starting.put(finalLocal.getIntKey(), finalLocal.getValue());
+ }
+ }
+ DebugLocalsChange change = createLocalsChange(ending, starting);
+ if (change != null) {
+ instructionIterator.add(change);
+ }
+ }
+
private DebugLocalsChange createLocalsChange(
Int2ReferenceMap<DebugLocalInfo> ending, Int2ReferenceMap<DebugLocalInfo> starting) {
+ if (ending.isEmpty() && starting.isEmpty()) {
+ return null;
+ }
if (ending.isEmpty() || starting.isEmpty()) {
return new DebugLocalsChange(ending, starting);
}
IntSet unneeded = new IntArraySet(Math.min(ending.size(), starting.size()));
- for (Int2ReferenceMap.Entry<DebugLocalInfo> entry : ending.int2ReferenceEntrySet()) {
+ for (Entry<DebugLocalInfo> entry : ending.int2ReferenceEntrySet()) {
if (starting.get(entry.getIntKey()) == entry.getValue()) {
unneeded.add(entry.getIntKey());
}
@@ -1112,7 +1196,7 @@
map.add(operandRegister);
}
}
- for (Entry<Integer> entry : Multisets.copyHighestCountFirst(map).entrySet()) {
+ for (Multiset.Entry<Integer> entry : Multisets.copyHighestCountFirst(map).entrySet()) {
int register = entry.getElement();
if (tryHint(unhandledInterval, registerConstraint, freePositions, needsRegisterPair,
register)) {
diff --git a/src/test/java/com/android/tools/r8/debug/DebugTestBase.java b/src/test/java/com/android/tools/r8/debug/DebugTestBase.java
index be25097..6a5f738 100644
--- a/src/test/java/com/android/tools/r8/debug/DebugTestBase.java
+++ b/src/test/java/com/android/tools/r8/debug/DebugTestBase.java
@@ -325,6 +325,10 @@
return new JUnit3Wrapper.Command.SetLocalCommand(localName, newValue);
}
+ protected final JUnit3Wrapper.Command getLocal(String localName, Consumer<Value> inspector) {
+ return t -> inspector.accept(t.debuggeeState.getLocalValues().get(localName));
+ }
+
@Ignore("Prevents Gradle from running the wrapper as a test.")
static class JUnit3Wrapper extends JDWPTestCase {
@@ -643,7 +647,8 @@
assert valuesCount == 1;
Value localValue = replyPacket.getNextValueAsValue();
- Assert.assertEquals(expectedValue, localValue);
+ Assert.assertEquals("Incorrect value for local '" + localName + "'",
+ expectedValue, localValue);
}
public String getClassName() {
diff --git a/src/test/java/com/android/tools/r8/debug/LocalsTest.java b/src/test/java/com/android/tools/r8/debug/LocalsTest.java
index 6a6a20f..1fd4bc9 100644
--- a/src/test/java/com/android/tools/r8/debug/LocalsTest.java
+++ b/src/test/java/com/android/tools/r8/debug/LocalsTest.java
@@ -317,4 +317,35 @@
run());
}
+ @Test
+ public void testInvokeRangeLongThrowOnDiv() throws Throwable {
+ final int initialValueOfX = 21;
+ final long expectedValueOfL = (long) initialValueOfX * 2;
+ runDebugTest("Locals",
+ breakpoint("Locals", "foo"),
+ run(),
+ // Initialize obj to 42 using original value of x.
+ stepOver(),
+ // Set value of x to zero which will cause a div-by-zero arithmetic exception below.
+ checkLocal("x", Value.createInt(initialValueOfX)),
+ setLocal("x", Value.createInt(0)),
+ // Single step until the catch handler triggers.
+ checkLine(SOURCE_FILE, 166), stepOver(),
+ checkLine(SOURCE_FILE, 168), stepOver(),
+ checkLine(SOURCE_FILE, 169), stepOver(),
+ // At the catch handler, inspect the initial state of locals.
+ checkLine(SOURCE_FILE, 172),
+ checkLocal("x", Value.createInt(0)),
+ getLocal("obj", value -> Assert.assertEquals(Tag.OBJECT_TAG, value.getTag())),
+ checkLocal("l", Value.createLong(expectedValueOfL)),
+ // Step onto first line of catch handler and inspect again, including the exception local.
+ stepOver(),
+ checkLine(SOURCE_FILE, 173),
+ getLocal("e", value -> Assert.assertEquals(Tag.OBJECT_TAG, value.getTag())),
+ checkLocal("x", Value.createInt(0)),
+ getLocal("obj", value -> Assert.assertEquals(Tag.OBJECT_TAG, value.getTag())),
+ checkLocal("l", Value.createLong(expectedValueOfL)),
+ run());
+ }
+
}
