From fc600dccd7797a9a10cdd457034ea8e148ccd631 Mon Sep 17 00:00:00 2001 From: Roland Levillain Date: Tue, 2 Dec 2014 17:16:31 +0000 Subject: [PATCH] Fix a compiler bug related to a catch-less try-finally statement. Ensure a dead basic block produced in this case is properly removed. Change-Id: I7c88e26aaa6c6378892f7c7c299494fa42312db2 --- compiler/optimizing/nodes.cc | 57 +++++++++++++++++------- compiler/optimizing/nodes.h | 1 + test/435-try-finally-without-catch/expected.txt | 3 ++ test/435-try-finally-without-catch/info.txt | 26 +++++++++++ test/435-try-finally-without-catch/src/Main.java | 41 +++++++++++++++++ 5 files changed, 111 insertions(+), 17 deletions(-) create mode 100644 test/435-try-finally-without-catch/expected.txt create mode 100644 test/435-try-finally-without-catch/info.txt create mode 100644 test/435-try-finally-without-catch/src/Main.java diff --git a/compiler/optimizing/nodes.cc b/compiler/optimizing/nodes.cc index 7584f1bac..ba4dccf59 100644 --- a/compiler/optimizing/nodes.cc +++ b/compiler/optimizing/nodes.cc @@ -30,6 +30,36 @@ void HGraph::FindBackEdges(ArenaBitVector* visited) { VisitBlockForBackEdges(entry_block_, visited, &visiting); } +static void RemoveAsUser(HInstruction* instruction) { + for (size_t i = 0; i < instruction->InputCount(); i++) { + instruction->InputAt(i)->RemoveUser(instruction, i); + } + + HEnvironment* environment = instruction->GetEnvironment(); + if (environment != nullptr) { + for (size_t i = 0, e = environment->Size(); i < e; ++i) { + HInstruction* vreg = environment->GetInstructionAt(i); + if (vreg != nullptr) { + vreg->RemoveEnvironmentUser(environment, i); + } + } + } +} + +void HGraph::RemoveInstructionsAsUsersFromDeadBlocks(const ArenaBitVector& visited) const { + for (size_t i = 0; i < blocks_.Size(); ++i) { + if (!visited.IsBitSet(i)) { + HBasicBlock* block = blocks_.Get(i); + for (HInstructionIterator it(block->GetPhis()); !it.Done(); it.Advance()) { + RemoveAsUser(it.Current()); + } + for (HInstructionIterator it(block->GetInstructions()); !it.Done(); it.Advance()) { + RemoveAsUser(it.Current()); + } + } + } +} + void HGraph::RemoveDeadBlocks(const ArenaBitVector& visited) const { for (size_t i = 0; i < blocks_.Size(); ++i) { if (!visited.IsBitSet(i)) { @@ -72,16 +102,21 @@ void HGraph::BuildDominatorTree() { // (1) Find the back edges in the graph doing a DFS traversal. FindBackEdges(&visited); - // (2) Remove blocks not visited during the initial DFS. - // Step (3) requires dead blocks to be removed from the + // (2) Remove instructions and phis from blocks not visited during + // the initial DFS as users from other instructions, so that + // users can be safely removed before uses later. + RemoveInstructionsAsUsersFromDeadBlocks(visited); + + // (3) Remove blocks not visited during the initial DFS. + // Step (4) requires dead blocks to be removed from the // predecessors list of live blocks. RemoveDeadBlocks(visited); - // (3) Simplify the CFG now, so that we don't need to recompute + // (4) Simplify the CFG now, so that we don't need to recompute // dominators and the reverse post order. SimplifyCFG(); - // (4) Compute the immediate dominator of each block. We visit + // (5) Compute the immediate dominator of each block. We visit // the successors of a block only when all its forward branches // have been processed. GrowableArray visits(arena_, blocks_.Size()); @@ -391,19 +426,7 @@ static void Remove(HInstructionList* instruction_list, instruction->SetBlock(nullptr); instruction_list->RemoveInstruction(instruction); - for (size_t i = 0; i < instruction->InputCount(); i++) { - instruction->InputAt(i)->RemoveUser(instruction, i); - } - - HEnvironment* environment = instruction->GetEnvironment(); - if (environment != nullptr) { - for (size_t i = 0, e = environment->Size(); i < e; ++i) { - HInstruction* vreg = environment->GetInstructionAt(i); - if (vreg != nullptr) { - vreg->RemoveEnvironmentUser(environment, i); - } - } - } + RemoveAsUser(instruction); } void HBasicBlock::RemoveInstruction(HInstruction* instruction) { diff --git a/compiler/optimizing/nodes.h b/compiler/optimizing/nodes.h index 9d0b4a971..0054d25af 100644 --- a/compiler/optimizing/nodes.h +++ b/compiler/optimizing/nodes.h @@ -173,6 +173,7 @@ class HGraph : public ArenaObject { void VisitBlockForBackEdges(HBasicBlock* block, ArenaBitVector* visited, ArenaBitVector* visiting); + void RemoveInstructionsAsUsersFromDeadBlocks(const ArenaBitVector& visited) const; void RemoveDeadBlocks(const ArenaBitVector& visited) const; ArenaAllocator* const arena_; diff --git a/test/435-try-finally-without-catch/expected.txt b/test/435-try-finally-without-catch/expected.txt new file mode 100644 index 000000000..8a67802d7 --- /dev/null +++ b/test/435-try-finally-without-catch/expected.txt @@ -0,0 +1,3 @@ +In finally +In finally +In finally diff --git a/test/435-try-finally-without-catch/info.txt b/test/435-try-finally-without-catch/info.txt new file mode 100644 index 000000000..46217c516 --- /dev/null +++ b/test/435-try-finally-without-catch/info.txt @@ -0,0 +1,26 @@ +Exercise a method containing a `try' statement with several +instructions with a `finally' clause but without any `catch' block, +enclosed in a loop. + +When dx processes an integer division (or modulo) enclosing a `try' +block and whose result is assigned to a local value, it is smart +enough not to emit a `div-int' (or `rem-int') instruction when the +divisor is non-null, as it wouldn't be used. However, dx is not +that clever regarding exception handling: if the divisor is known to +be non-null at compile-time (as is the case in this test), it will +still emit a block with the exception catching and rethrowing +mechanism, even if it is not used. + +This used to be a problem for a `try' block followed by a `finally' +clause but with no `catch' block: in that case, the generated Dex code +item would list zero catch block for this method (see +art::CodeItem::tries_size_) and the optimizing compiler would have no +clue that it contains a `try' statement, which it cannot optimize +(yet). With no hint that this method might contain one (or several) +special block(s) related to `catch'-less `try' statement(s), the +optimizing compiler considered this (these) as dead block(s) and +improperly tried to remove its (their) instructions, sometimes +removing instructions used by others instructions, thus triggering +assertions. The optimizing compiler was thus adjusted to remove these +instructions in a proper fashion, by removing them as users first, and +then by suppressing them for good. diff --git a/test/435-try-finally-without-catch/src/Main.java b/test/435-try-finally-without-catch/src/Main.java new file mode 100644 index 000000000..3c29ce847 --- /dev/null +++ b/test/435-try-finally-without-catch/src/Main.java @@ -0,0 +1,41 @@ +/* + * Copyright (C) 2014 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. + */ + +public class Main { + + public static void main(String[] args){ + foo(); + } + + // Reduced test case inspired by constantPropagationTest() from + // test/083-compiler-regressions. + static void foo() { + int a = 0; + int b = 1; + + for (int i = 0; i < 3; i++) { + try { + a = 1; + // Would throw an ArithmeticException if b were null (hence + // the enclosing `try' statement). + int c = a % b; + } + finally { + System.out.println("In finally"); + } + } + } +} -- 2.11.0