From 72783ff32da5d3f025bbe1636cee84328f1135c6 Mon Sep 17 00:00:00 2001 From: David Brazdil Date: Thu, 9 Jul 2015 14:36:05 +0100 Subject: [PATCH] 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 --- compiler/optimizing/builder.cc | 109 +++++++++++---------- test/510-checker-try-catch/smali/Builder.smali | 23 +++-- test/523-checker-can-throw-regression/expected.txt | 0 test/523-checker-can-throw-regression/info.txt | 2 + .../smali/Test.smali | 53 ++++++++++ .../523-checker-can-throw-regression/src/Main.java | 35 +++++++ 6 files changed, 158 insertions(+), 64 deletions(-) create mode 100644 test/523-checker-can-throw-regression/expected.txt create mode 100644 test/523-checker-can-throw-regression/info.txt create mode 100644 test/523-checker-can-throw-regression/smali/Test.smali create mode 100644 test/523-checker-can-throw-regression/src/Main.java diff --git a/compiler/optimizing/builder.cc b/compiler/optimizing/builder.cc index bb1b40a1b..fe52c4430 100644 --- 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 87e4064de..2274ba4d4 100644 --- a/test/510-checker-try-catch/smali/Builder.smali +++ b/test/510-checker-try-catch/smali/Builder.smali @@ -914,6 +914,14 @@ ## CHECK: name "<>" ## CHECK: predecessors "<>" +## CHECK: name "{{B\d+}}" +## CHECK: Exit + +## CHECK: name "<>" +## CHECK: predecessors "<>" +## CHECK: successors "<>" +## CHECK: Div + ## CHECK: name "<>" ## CHECK: predecessors "B0" ## CHECK: successors "<>" @@ -926,11 +934,6 @@ ## CHECK: xhandlers "<>" ## CHECK: TryBoundary kind:exit -## CHECK: name "<>" -## CHECK: predecessors "<>" -## CHECK: successors "<>" -## CHECK: Div - ## CHECK: name "<>" ## CHECK: predecessors "<>" ## CHECK: successors "<>" @@ -987,6 +990,11 @@ ## CHECK: successors "<>" ## CHECK: Div +## CHECK: name "<>" +## CHECK: predecessors "<>" +## CHECK: successors "<>" +## CHECK: Div + ## CHECK: name "<>" ## CHECK: predecessors "<>" ## CHECK: successors "<>" @@ -999,11 +1007,6 @@ ## CHECK: xhandlers "<>" ## CHECK: TryBoundary kind:exit -## CHECK: name "<>" -## CHECK: predecessors "<>" -## CHECK: successors "<>" -## CHECK: Div - ## CHECK: name "<>" ## CHECK: predecessors "<>" ## CHECK: successors "<>" diff --git a/test/523-checker-can-throw-regression/expected.txt b/test/523-checker-can-throw-regression/expected.txt new file mode 100644 index 000000000..e69de29bb diff --git a/test/523-checker-can-throw-regression/info.txt b/test/523-checker-can-throw-regression/info.txt new file mode 100644 index 000000000..720dc8517 --- /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 index 000000000..87192ea12 --- /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 index 000000000..3ff48f3d4 --- /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[]) {} +} -- 2.11.0