ART: Fix bug in GraphBuilder

This fixes a bug where the GraphBuilder would split a throwing catch
block but would not update info about which blocks throw.

Change-Id: If5415f0c320aa488e06eb042e8fea6f03e30246a

diff --git a/compiler/optimizing/builder.cc b/compiler/optimizing/builder.cc
index bb1b40a..fe52c44 100644 (file)
--- a/compiler/optimizing/builder.cc
+++ b/compiler/optimizing/builder.cc
@@ -333,18 +333,41 @@ void HGraphBuilder::InsertTryBoundaryBlocks(const DexFile::CodeItem& code_item)
     return;
   }
 
-  const size_t num_blocks = graph_->GetBlocks().Size();
-  ArenaBitVector can_block_throw(arena_, num_blocks, false);
+  // Bit vector stores information on which blocks contain throwing instructions.
+  // Must be expandable because catch blocks may be split into two.
+  ArenaBitVector can_block_throw(arena_, graph_->GetBlocks().Size(), /* expandable */ true);
 
   // Scan blocks and mark those which contain throwing instructions.
-  for (size_t block_id = 0; block_id < num_blocks; ++block_id) {
+  for (size_t block_id = 0, e = graph_->GetBlocks().Size(); block_id < e; ++block_id) {
     HBasicBlock* block = graph_->GetBlocks().Get(block_id);
+    bool can_throw = false;
     for (HInstructionIterator insn(block->GetInstructions()); !insn.Done(); insn.Advance()) {
       if (insn.Current()->CanThrow()) {
-        can_block_throw.SetBit(block_id);
+        can_throw = true;
         break;
       }
     }
+
+    if (can_throw) {
+      if (block->IsCatchBlock()) {
+        // Catch blocks are always considered an entry point into the TryItem in
+        // order to avoid splitting exceptional edges. We split the block after
+        // the move-exception (if present) and mark the first part non-throwing.
+        // Later on, a TryBoundary will be inserted between the two blocks.
+        HInstruction* first_insn = block->GetFirstInstruction();
+        if (first_insn->IsLoadException()) {
+          // Catch block starts with a LoadException. Split the block after the
+          // StoreLocal that must come after the load.
+          DCHECK(first_insn->GetNext()->IsStoreLocal());
+          block = block->SplitBefore(first_insn->GetNext()->GetNext());
+        } else {
+          // Catch block does not load the exception. Split at the beginning to
+          // create an empty catch block.
+          block = block->SplitBefore(first_insn);
+        }
+      }
+      can_block_throw.SetBit(block->GetBlockId());
+    }
   }
 
   // Iterate over all blocks, find those covered by some TryItem and:
@@ -353,7 +376,7 @@ void HGraphBuilder::InsertTryBoundaryBlocks(const DexFile::CodeItem& code_item)
   //   (c) link the new blocks to corresponding exception handlers.
   // We cannot iterate only over blocks in `branch_targets_` because switch-case
   // blocks share the same dex_pc.
-  for (size_t block_id = 0; block_id < num_blocks; ++block_id) {
+  for (size_t block_id = 0, e = graph_->GetBlocks().Size(); block_id < e; ++block_id) {
     HBasicBlock* try_block = graph_->GetBlocks().Get(block_id);
 
     // TryBoundary blocks are added at the end of the list and not iterated over.
@@ -365,57 +388,35 @@ void HGraphBuilder::InsertTryBoundaryBlocks(const DexFile::CodeItem& code_item)
       continue;
     }
 
-    if (try_block->IsCatchBlock()) {
-      // Catch blocks are always considered an entry point into the TryItem in
-      // order to avoid splitting exceptional edges (they might not have been
-      // created yet). We separate the move-exception (if present) from the
-      // rest of the block and insert a TryBoundary after it, creating a
-      // landing pad for the exceptional edges.
-      HInstruction* first_insn = try_block->GetFirstInstruction();
-      HInstruction* split_position = nullptr;
-      if (first_insn->IsLoadException()) {
-        // Catch block starts with a LoadException. Split the block after the
-        // StoreLocal that must come after the load.
-        DCHECK(first_insn->GetNext()->IsStoreLocal());
-        split_position = first_insn->GetNext()->GetNext();
-      } else {
-        // Catch block does not obtain the exception. Split at the beginning
-        // to create an empty catch block.
-        split_position = first_insn;
-      }
-      DCHECK(split_position != nullptr);
-      HBasicBlock* catch_block = try_block;
-      try_block = catch_block->SplitBefore(split_position);
-      SplitTryBoundaryEdge(catch_block, try_block, HTryBoundary::kEntry, code_item, *try_item);
-    } else {
-      // For non-catch blocks, find predecessors which are not covered by the
-      // same TryItem range. Such edges enter the try block and will have
-      // a TryBoundary inserted.
-      for (size_t i = 0; i < try_block->GetPredecessors().Size(); ++i) {
-        HBasicBlock* predecessor = try_block->GetPredecessors().Get(i);
-        if (predecessor->IsSingleTryBoundary()) {
-          // The edge was already split because of an exit from a neighbouring
-          // TryItem. We split it again and insert an entry point.
-          if (kIsDebugBuild) {
-            HTryBoundary* last_insn = predecessor->GetLastInstruction()->AsTryBoundary();
-            const DexFile::TryItem* predecessor_try_item =
-                GetTryItem(predecessor->GetSinglePredecessor(), code_item, can_block_throw);
-            DCHECK(!last_insn->IsEntry());
-            DCHECK_EQ(last_insn->GetNormalFlowSuccessor(), try_block);
-            DCHECK(try_block->IsFirstIndexOfPredecessor(predecessor, i));
-            DCHECK_NE(try_item, predecessor_try_item);
-          }
-        } else if (GetTryItem(predecessor, code_item, can_block_throw) != try_item) {
-          // This is an entry point into the TryItem and the edge has not been
-          // split yet. That means that `predecessor` is not in a TryItem, or
-          // it is in a different TryItem and we happened to iterate over this
-          // block first. We split the edge and insert an entry point.
-        } else {
-          // Not an edge on the boundary of the try block.
-          continue;
+    // Catch blocks were split earlier and cannot throw.
+    DCHECK(!try_block->IsCatchBlock());
+
+    // Find predecessors which are not covered by the same TryItem range. Such
+    // edges enter the try block and will have a TryBoundary inserted.
+    for (size_t i = 0; i < try_block->GetPredecessors().Size(); ++i) {
+      HBasicBlock* predecessor = try_block->GetPredecessors().Get(i);
+      if (predecessor->IsSingleTryBoundary()) {
+        // The edge was already split because of an exit from a neighbouring
+        // TryItem. We split it again and insert an entry point.
+        if (kIsDebugBuild) {
+          HTryBoundary* last_insn = predecessor->GetLastInstruction()->AsTryBoundary();
+          const DexFile::TryItem* predecessor_try_item =
+              GetTryItem(predecessor->GetSinglePredecessor(), code_item, can_block_throw);
+          DCHECK(!last_insn->IsEntry());
+          DCHECK_EQ(last_insn->GetNormalFlowSuccessor(), try_block);
+          DCHECK(try_block->IsFirstIndexOfPredecessor(predecessor, i));
+          DCHECK_NE(try_item, predecessor_try_item);
         }
-        SplitTryBoundaryEdge(predecessor, try_block, HTryBoundary::kEntry, code_item, *try_item);
+      } else if (GetTryItem(predecessor, code_item, can_block_throw) != try_item) {
+        // This is an entry point into the TryItem and the edge has not been
+        // split yet. That means that `predecessor` is not in a TryItem, or
+        // it is in a different TryItem and we happened to iterate over this
+        // block first. We split the edge and insert an entry point.
+      } else {
+        // Not an edge on the boundary of the try block.
+        continue;
       }
+      SplitTryBoundaryEdge(predecessor, try_block, HTryBoundary::kEntry, code_item, *try_item);
     }
 
     // Find successors which are not covered by the same TryItem range. Such

diff --git a/test/510-checker-try-catch/smali/Builder.smali b/test/510-checker-try-catch/smali/Builder.smali
index 87e4064..2274ba4 100644 (file)
--- a/test/510-checker-try-catch/smali/Builder.smali
+++ b/test/510-checker-try-catch/smali/Builder.smali
@@ -914,6 +914,14 @@
 ## CHECK:  name             "<<BReturn:B\d+>>"
 ## CHECK:  predecessors     "<<BExitTry2>>"
 
+## CHECK:  name             "{{B\d+}}"
+## CHECK:  Exit
+
+## CHECK:  name             "<<BTry2:B\d+>>"
+## CHECK:  predecessors     "<<BEnterTry2>>"
+## CHECK:  successors       "<<BExitTry2>>"
+## CHECK:  Div
+
 ## CHECK:  name             "<<BEnterTry1>>"
 ## CHECK:  predecessors     "B0"
 ## CHECK:  successors       "<<BTry1>>"
@@ -926,11 +934,6 @@
 ## CHECK:  xhandlers        "<<BCatch>>"
 ## CHECK:  TryBoundary      kind:exit
 
-## CHECK:  name             "<<BTry2:B\d+>>"
-## CHECK:  predecessors     "<<BEnterTry2>>"
-## CHECK:  successors       "<<BExitTry2>>"
-## CHECK:  Div
-
 ## CHECK:  name             "<<BEnterTry2>>"
 ## CHECK:  predecessors     "<<BCatch>>"
 ## CHECK:  successors       "<<BTry2>>"
@@ -987,6 +990,11 @@
 ## CHECK:  successors       "<<BExitTry1>>"
 ## CHECK:  Div
 
+## CHECK:  name             "<<BTry2:B\d+>>"
+## CHECK:  predecessors     "<<BEnterTry2>>"
+## CHECK:  successors       "<<BExitTry2>>"
+## CHECK:  Div
+
 ## CHECK:  name             "<<BEnterTry1>>"
 ## CHECK:  predecessors     "<<BCatch1>>"
 ## CHECK:  successors       "<<BTry1>>"
@@ -999,11 +1007,6 @@
 ## CHECK:  xhandlers        "<<BCatch2>>"
 ## CHECK:  TryBoundary      kind:exit
 
-## CHECK:  name             "<<BTry2:B\d+>>"
-## CHECK:  predecessors     "<<BEnterTry2>>"
-## CHECK:  successors       "<<BExitTry2>>"
-## CHECK:  Div
-
 ## CHECK:  name             "<<BEnterTry2>>"
 ## CHECK:  predecessors     "<<BCatch2>>"
 ## CHECK:  successors       "<<BTry2>>"

diff --git a/test/523-checker-can-throw-regression/expected.txt b/test/523-checker-can-throw-regression/expected.txt
new file mode 100644 (file)
index 0000000..e69de29

diff --git a/test/523-checker-can-throw-regression/info.txt b/test/523-checker-can-throw-regression/info.txt
new file mode 100644 (file)
index 0000000..720dc85
--- /dev/null
+++ b/test/523-checker-can-throw-regression/info.txt
@@ -0,0 +1,2 @@
+Regression test for the HGraphBuilder which would split a throwing catch block
+but would not update information about which blocks throw.
\ No newline at end of file

diff --git a/test/523-checker-can-throw-regression/smali/Test.smali b/test/523-checker-can-throw-regression/smali/Test.smali
new file mode 100644 (file)
index 0000000..87192ea
--- /dev/null
+++ b/test/523-checker-can-throw-regression/smali/Test.smali
@@ -0,0 +1,53 @@
+#
+# Copyright (C) 2015 The Android Open Source Project
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+.class public LTest;
+
+.super Ljava/lang/Object;
+
+## CHECK-START: int Test.testCase(int, int, int) builder (after)
+## CHECK:         TryBoundary kind:entry
+## CHECK:         TryBoundary kind:entry
+## CHECK-NOT:     TryBoundary kind:entry
+
+## CHECK-START: int Test.testCase(int, int, int) builder (after)
+## CHECK:         TryBoundary kind:exit
+## CHECK:         TryBoundary kind:exit
+## CHECK-NOT:     TryBoundary kind:exit
+
+.method public static testCase(III)I
+  .registers 4
+
+  :try_start_1
+  div-int/2addr p0, p1
+  return p0
+  :try_end_1
+  .catchall {:try_start_1 .. :try_end_1} :catchall
+
+  :catchall
+  :try_start_2
+  move-exception v0
+  # Block would be split here but second part not marked as throwing.
+  div-int/2addr p0, p1
+  if-eqz p2, :else
+
+  div-int/2addr p0, p1
+  :else
+  div-int/2addr p0, p2
+  return p0
+  :try_end_2
+  .catchall {:try_start_2 .. :try_end_2} :catchall
+
+.end method

diff --git a/test/523-checker-can-throw-regression/src/Main.java b/test/523-checker-can-throw-regression/src/Main.java
new file mode 100644 (file)
index 0000000..3ff48f3
--- /dev/null
+++ b/test/523-checker-can-throw-regression/src/Main.java
@@ -0,0 +1,35 @@
+/*
+ * Copyright (C) 2015 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import java.lang.reflect.Method;
+import java.lang.reflect.Type;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.concurrent.Callable;
+import java.util.concurrent.Executors;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Future;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.CancellationException;
+import java.util.concurrent.TimeoutException;
+
+public class Main {
+
+  // Workaround for b/18051191.
+  class InnerClass {}
+
+  public static void main(String args[]) {}
+}