Catch more cases of invalid input and give CompilationErrors.
Previously these cases would lead to null pointer exceptions or
assertion errors.
R=sgjesse@google.com, vnukov@google.com
Change-Id: I9684379ffbc98106bd9c4a8753861f682cb13b83
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 2bef174..ab2056d 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
@@ -3,6 +3,7 @@
// BSD-style license that can be found in the LICENSE file.
package com.android.tools.r8.ir.code;
+import com.android.tools.r8.errors.CompilationError;
import com.android.tools.r8.graph.DexType;
import com.android.tools.r8.ir.conversion.DexBuilder;
import com.android.tools.r8.ir.conversion.IRBuilder;
@@ -119,7 +120,7 @@
public List<BasicBlock> getNormalPredecessors() {
ImmutableList.Builder<BasicBlock> normals = ImmutableList.builder();
for (BasicBlock predecessor : predecessors) {
- if (!predecessor.isCatchSuccessor(this)) {
+ if (!predecessor.hasCatchSuccessor(this)) {
normals.add(predecessor);
}
}
@@ -647,10 +648,10 @@
public EdgeType getEdgeType(BasicBlock successor) {
assert successors.indexOf(successor) >= 0;
- return isCatchSuccessor(successor) ? EdgeType.EXCEPTIONAL : EdgeType.NORMAL;
+ return hasCatchSuccessor(successor) ? EdgeType.EXCEPTIONAL : EdgeType.NORMAL;
}
- public boolean isCatchSuccessor(BasicBlock block) {
+ public boolean hasCatchSuccessor(BasicBlock block) {
if (!hasCatchHandlers()) {
return false;
}
@@ -658,7 +659,7 @@
}
public int guardsForCatchSuccessor(BasicBlock block) {
- assert isCatchSuccessor(block);
+ assert hasCatchSuccessor(block);
int index = successors.indexOf(block);
int count = 0;
for (int handler : catchHandlers.getAllTargets()) {
@@ -713,7 +714,7 @@
}
private String predecessorPostfix(BasicBlock block) {
- if (isCatchSuccessor(block)) {
+ if (hasCatchSuccessor(block)) {
return new String(new char[guardsForCatchSuccessor(block)]).replace("\0", "*");
}
return "";
@@ -1053,7 +1054,7 @@
// one new phi to merge the two exception values, and all other phis don't need
// to be changed.
for (BasicBlock catchSuccessor : catchSuccessors) {
- catchSuccessor.splitCriticalExceptioEdges(
+ catchSuccessor.splitCriticalExceptionEdges(
code.valueNumberGenerator,
newBlock -> {
newBlock.setNumber(code.blocks.size());
@@ -1062,15 +1063,6 @@
}
}
- private boolean allPredecessorsHaveCatchEdges() {
- for (BasicBlock predecessor : getPredecessors()) {
- if (!predecessor.isCatchSuccessor(this)) {
- assert false;
- }
- }
- return true;
- }
-
/**
* Assumes that `this` block is a catch handler target (note that it does not have to
* start with MoveException instruction, since the instruction can be removed by
@@ -1086,10 +1078,8 @@
*
* NOTE: onNewBlock must assign block number to the newly created block.
*/
- public void splitCriticalExceptioEdges(
+ public void splitCriticalExceptionEdges(
ValueNumberGenerator valueNumberGenerator, Consumer<BasicBlock> onNewBlock) {
- assert allPredecessorsHaveCatchEdges();
-
List<BasicBlock> predecessors = this.getPredecessors();
boolean hasMoveException = entry().isMoveException();
MoveException move = null;
@@ -1103,6 +1093,10 @@
List<BasicBlock> newPredecessors = new ArrayList<>();
List<Value> values = new ArrayList<>(predecessors.size());
for (BasicBlock predecessor : predecessors) {
+ if (!predecessor.hasCatchSuccessor(this)) {
+ throw new CompilationError(
+ "Invalid block structure: catch block reachable via non-exceptional flow.");
+ }
BasicBlock newBlock = new BasicBlock();
newPredecessors.add(newBlock);
if (hasMoveException) {
diff --git a/src/main/java/com/android/tools/r8/ir/code/Phi.java b/src/main/java/com/android/tools/r8/ir/code/Phi.java
index 5912f0f..de44e83 100644
--- a/src/main/java/com/android/tools/r8/ir/code/Phi.java
+++ b/src/main/java/com/android/tools/r8/ir/code/Phi.java
@@ -3,6 +3,7 @@
// BSD-style license that can be found in the LICENSE file.
package com.android.tools.r8.ir.code;
+import com.android.tools.r8.errors.CompilationError;
import com.android.tools.r8.graph.DebugLocalInfo;
import com.android.tools.r8.ir.code.BasicBlock.EdgeType;
import com.android.tools.r8.ir.conversion.IRBuilder;
@@ -60,6 +61,9 @@
// exactly once by adding the operands.
assert operands.isEmpty();
boolean canBeNull = false;
+ if (block.getPredecessors().size() == 0) {
+ throwUndefinedValueError();
+ }
for (BasicBlock pred : block.getPredecessors()) {
EdgeType edgeType = pred.getEdgeType(block);
// Since this read has been delayed we must provide the local info for the value.
@@ -79,6 +83,9 @@
// exactly once by adding the operands.
assert this.operands.isEmpty();
boolean canBeNull = false;
+ if (operands.size() == 0) {
+ throwUndefinedValueError();
+ }
for (Value operand : operands) {
canBeNull |= operand.canBeNull();
appendOperand(operand);
@@ -89,6 +96,13 @@
removeTrivialPhi();
}
+ private void throwUndefinedValueError() {
+ throw new CompilationError(
+ "Undefined value encountered during compilation. "
+ + "This is typically caused by invalid dex input that uses a register "
+ + "that is not define on all control-flow paths leading to the use.");
+ }
+
private void appendOperand(Value operand) {
operands.add(operand);
operand.addPhiUser(this);
@@ -174,9 +188,6 @@
same = op;
}
assert isTrivialPhi();
- if (same == null) {
- same = Value.UNDEFINED;
- }
// Removing this phi, so get rid of it as a phi user from all of the operands to avoid
// recursively getting back here with the same phi. If the phi has itself as an operand
// that also removes the self-reference.
diff --git a/src/main/java/com/android/tools/r8/ir/conversion/IRBuilder.java b/src/main/java/com/android/tools/r8/ir/conversion/IRBuilder.java
index b3d5266..d24c8b4 100644
--- a/src/main/java/com/android/tools/r8/ir/conversion/IRBuilder.java
+++ b/src/main/java/com/android/tools/r8/ir/conversion/IRBuilder.java
@@ -81,8 +81,6 @@
import com.android.tools.r8.ir.code.ValueNumberGenerator;
import com.android.tools.r8.ir.code.Xor;
import com.android.tools.r8.utils.InternalOptions;
-import com.android.tools.r8.utils.StringUtils;
-import com.android.tools.r8.utils.StringUtils.BraceType;
import it.unimi.dsi.fastutil.ints.Int2ReferenceAVLTreeMap;
import it.unimi.dsi.fastutil.ints.Int2ReferenceMap;
import it.unimi.dsi.fastutil.ints.Int2ReferenceSortedMap;
@@ -372,9 +370,6 @@
// necessary.
splitCriticalEdges();
- // Consistency check.
- assert phiOperandsAreConsistent();
-
// Package up the IR code.
IRCode ir = new IRCode(method, blocks, normalExitBlock, valueNumberGenerator);
@@ -808,7 +803,7 @@
public void addGoto(int targetOffset) {
addInstruction(new Goto());
BasicBlock targetBlock = getTarget(targetOffset);
- if (currentBlock.isCatchSuccessor(targetBlock)) {
+ if (currentBlock.hasCatchSuccessor(targetBlock)) {
needGotoToCatchBlocks.add(new BasicBlock.Pair(currentBlock, targetBlock));
} else {
currentBlock.link(targetBlock);
@@ -1086,7 +1081,12 @@
Value out = writeRegister(dest, MoveType.OBJECT, ThrowingInfo.NO_THROW);
MoveException instruction = new MoveException(out);
assert !instruction.instructionTypeCanThrow();
- assert currentBlock.getInstructions().isEmpty();
+ if (!currentBlock.getInstructions().isEmpty()) {
+ throw new CompilationError("Invalid MoveException instruction encountered. "
+ + "The MoveException instruction is not the first instruction in the block in "
+ + method.qualifiedName()
+ + ".");
+ }
addInstruction(instruction);
}
@@ -1698,7 +1698,7 @@
assert currentBlock != null;
flushCurrentDebugPosition();
currentBlock.add(new Goto());
- if (currentBlock.isCatchSuccessor(nextBlock)) {
+ if (currentBlock.hasCatchSuccessor(nextBlock)) {
needGotoToCatchBlocks.add(new BasicBlock.Pair(currentBlock, nextBlock));
} else {
currentBlock.link(nextBlock);
@@ -1778,8 +1778,8 @@
target.getPredecessors().add(newBlock);
// Check that the successor indexes are correct.
- assert source.isCatchSuccessor(newBlock);
- assert !source.isCatchSuccessor(target);
+ assert source.hasCatchSuccessor(newBlock);
+ assert !source.hasCatchSuccessor(target);
// Mark the filled predecessors to the blocks.
if (source.isFilled()) {
@@ -1790,22 +1790,6 @@
return blockNumber;
}
- private boolean phiOperandsAreConsistent() {
- for (BasicBlock block : blocks) {
- if (block.hasIncompletePhis()) {
- StringBuilder builder = new StringBuilder("Incomplete phis in ");
- builder.append(method);
- builder.append(". The following registers appear to be uninitialized: ");
- StringUtils.append(builder, block.getIncompletePhiRegisters(), ", ", BraceType.NONE);
- throw new CompilationError(builder.toString());
- }
- for (Phi phi : block.getPhis()) {
- assert phi.getOperands().size() == block.getPredecessors().size();
- }
- }
- return true;
- }
-
/**
* Change to control-flow graph to avoid repeated phi operands when all the same values
* flow in from multiple predecessors.
@@ -1837,6 +1821,15 @@
public void joinPredecessorsWithIdenticalPhis() {
List<BasicBlock> blocksToAdd = new ArrayList<>();
for (BasicBlock block : blocks) {
+ // Consistency check. At this point there should be no incomplete phis.
+ // If there are, the input is typically dex code that uses a register
+ // that is not defined on all control-flow paths.
+ if (block.hasIncompletePhis()) {
+ throw new CompilationError(
+ "Undefined value encountered during compilation. "
+ + "This is typically caused by invalid dex input that uses a register "
+ + "that is not define on all control-flow paths leading to the use.");
+ }
if (block.entry() instanceof MoveException) {
// TODO: Should we support joining in the presence of move-exception instructions?
continue;
@@ -1889,7 +1882,7 @@
// If any of the edges to the block are critical, we need to insert new blocks on each
// containing the move-exception instruction which must remain the first instruction.
if (block.entry() instanceof MoveException) {
- block.splitCriticalExceptioEdges(valueNumberGenerator,
+ block.splitCriticalExceptionEdges(valueNumberGenerator,
newBlock -> {
newBlock.setNumber(blocks.size() + newBlocks.size());
newBlocks.add(newBlock);
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 4bf5dc6..52b8ab0 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
@@ -1520,7 +1520,7 @@
// If we are processing an exception edge, we need to use the throwing instruction
// as the instruction we are coming from.
int fromInstruction = block.exit().getNumber();
- boolean isCatch = block.isCatchSuccessor(successor);
+ boolean isCatch = block.hasCatchSuccessor(successor);
if (isCatch) {
for (Instruction instruction : block.getInstructions()) {
if (instruction.instructionTypeCanThrow()) {
