diff options
5 files changed, 48 insertions, 50 deletions
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 2bef17484..ab2056d03 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 class BasicBlock { 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 class BasicBlock { 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 class BasicBlock { } 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 @@ public class BasicBlock { } 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 @@ public class BasicBlock { // 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 @@ public class BasicBlock { } } - 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 @@ public class BasicBlock { * * 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 @@ public class BasicBlock { 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 5912f0f3d..de44e83be 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 @@ public class Phi extends Value { // 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 @@ public class Phi extends Value { // 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 @@ public class Phi extends Value { 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 @@ public class Phi extends Value { 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 b3d526635..d24c8b4d6 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.Value.DebugInfo; 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 @@ public class IRBuilder { // necessary. splitCriticalEdges(); - // Consistency check. - assert phiOperandsAreConsistent(); - // Package up the IR code. IRCode ir = new IRCode(method, blocks, normalExitBlock, valueNumberGenerator); @@ -808,7 +803,7 @@ public class IRBuilder { 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 @@ public class IRBuilder { 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 @@ public class IRBuilder { 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 @@ public class IRBuilder { 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 @@ public class IRBuilder { 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 class IRBuilder { 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 @@ public class IRBuilder { // 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 908e15e43..06918870a 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 @@ -1521,7 +1521,7 @@ public class LinearScanRegisterAllocator implements RegisterAllocator { // 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()) { diff --git a/src/test/java/com/android/tools/r8/smali/CatchSuccessorFallthroughTest.java b/src/test/java/com/android/tools/r8/smali/CatchSuccessorFallthroughTest.java index 5e67ad157..ca30f766e 100644 --- a/src/test/java/com/android/tools/r8/smali/CatchSuccessorFallthroughTest.java +++ b/src/test/java/com/android/tools/r8/smali/CatchSuccessorFallthroughTest.java @@ -93,7 +93,7 @@ public class CatchSuccessorFallthroughTest extends SmaliTestBase { assertTrue( defBlock.getInstructions().get(defBlock.getInstructions().size() - 2).isInvoke()); for (BasicBlock returnPredecessor : block.getPredecessors()) { - if (defBlock.isCatchSuccessor(returnPredecessor)) { + if (defBlock.hasCatchSuccessor(returnPredecessor)) { hasExceptionalPredecessor = true; } else if (defBlock == returnPredecessor) { // Normal flow goes to return. |