Merge "Catch more cases of invalid input and give CompilationErrors."

4 files changed, 47 insertions, 49 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()) {