From 9bc436160b4af99067973affb0b1008de9a2b04c Mon Sep 17 00:00:00 2001 From: David Brazdil Date: Thu, 5 Nov 2015 21:25:24 +0000 Subject: ART: Fix simplification of catch blocks in the presence of dead code Simplification of catch blocks transforms the code so that catch blocks have only exceptional predecessors. However, it is invoked before trivially dead code is eliminated which breaks simple assumptions such as the fact that a catch block cannot start with move-exception if it has non-exceptional predecessors. This patch fixes the algorithm to work under these relaxed conditions. Bug: 25494450 Bug: 25492628 Change-Id: Idc8d010102a4b8b9a6cd918b98d6e11d1838db0c --- compiler/optimizing/builder.cc | 16 +++-------- compiler/optimizing/graph_checker.cc | 15 ++++++++++ compiler/optimizing/graph_checker.h | 3 ++ compiler/optimizing/nodes.cc | 56 ++++++++++++++++++++++++++++++------ compiler/optimizing/nodes.h | 9 ++++++ 5 files changed, 78 insertions(+), 21 deletions(-) (limited to 'compiler/optimizing') diff --git a/compiler/optimizing/builder.cc b/compiler/optimizing/builder.cc index ed193c7b61..676e56477e 100644 --- a/compiler/optimizing/builder.cc +++ b/compiler/optimizing/builder.cc @@ -359,18 +359,10 @@ void HGraphBuilder::InsertTryBoundaryBlocks(const DexFile::CodeItem& code_item) // need a strategy for splitting exceptional edges. We split the block // after the move-exception (if present) and mark the first part not // throwing. The normal-flow edge between them will be split later. - HInstruction* first_insn = block->GetFirstInstruction(); - if (first_insn->IsLoadException()) { - // Catch block starts with a LoadException. Split the block after - // the StoreLocal and ClearException which must come after the load. - DCHECK(first_insn->GetNext()->IsStoreLocal()); - DCHECK(first_insn->GetNext()->GetNext()->IsClearException()); - throwing_block = block->SplitBefore(first_insn->GetNext()->GetNext()->GetNext()); - } else { - // Catch block does not load the exception. Split at the beginning - // to create an empty catch block. - throwing_block = block->SplitBefore(first_insn); - } + throwing_block = block->SplitCatchBlockAfterMoveException(); + // Move-exception does not throw and the block has throwing insructions + // so it must have been possible to split it. + DCHECK(throwing_block != nullptr); } try_block_info.Put(throwing_block->GetBlockId(), diff --git a/compiler/optimizing/graph_checker.cc b/compiler/optimizing/graph_checker.cc index 3de96b5d84..c32ef51988 100644 --- a/compiler/optimizing/graph_checker.cc +++ b/compiler/optimizing/graph_checker.cc @@ -188,6 +188,21 @@ void GraphChecker::VisitTryBoundary(HTryBoundary* try_boundary) { VisitInstruction(try_boundary); } +void GraphChecker::VisitLoadException(HLoadException* load) { + // Ensure that LoadException is the first instruction in a catch block. + if (!load->GetBlock()->IsCatchBlock()) { + AddError(StringPrintf("%s:%d is in a non-catch block %d.", + load->DebugName(), + load->GetId(), + load->GetBlock()->GetBlockId())); + } else if (load->GetBlock()->GetFirstInstruction() != load) { + AddError(StringPrintf("%s:%d is not the first instruction in catch block %d.", + load->DebugName(), + load->GetId(), + load->GetBlock()->GetBlockId())); + } +} + void GraphChecker::VisitInstruction(HInstruction* instruction) { if (seen_ids_.IsBitSet(instruction->GetId())) { AddError(StringPrintf("Instruction id %d is duplicate in graph.", diff --git a/compiler/optimizing/graph_checker.h b/compiler/optimizing/graph_checker.h index abf3659d91..d5ddbabc8c 100644 --- a/compiler/optimizing/graph_checker.h +++ b/compiler/optimizing/graph_checker.h @@ -50,6 +50,9 @@ class GraphChecker : public HGraphDelegateVisitor { // Check successors of blocks ending in TryBoundary. void VisitTryBoundary(HTryBoundary* try_boundary) OVERRIDE; + // Check that LoadException is the first instruction in a catch block. + void VisitLoadException(HLoadException* load) OVERRIDE; + // Check that HCheckCast and HInstanceOf have HLoadClass as second input. void VisitCheckCast(HCheckCast* check) OVERRIDE; void VisitInstanceOf(HInstanceOf* check) OVERRIDE; diff --git a/compiler/optimizing/nodes.cc b/compiler/optimizing/nodes.cc index 68fb0acf7f..af3d8f4a60 100644 --- a/compiler/optimizing/nodes.cc +++ b/compiler/optimizing/nodes.cc @@ -335,14 +335,24 @@ void HGraph::SimplifyCatchBlocks() { // instructions into `normal_block` and links the two blocks with a Goto. // Afterwards, incoming normal-flow edges are re-linked to `normal_block`, // leaving `catch_block` with the exceptional edges only. + // // Note that catch blocks with normal-flow predecessors cannot begin with - // a MOVE_EXCEPTION instruction, as guaranteed by the verifier. - DCHECK(!catch_block->GetFirstInstruction()->IsLoadException()); - HBasicBlock* normal_block = catch_block->SplitBefore(catch_block->GetFirstInstruction()); - for (size_t j = 0; j < catch_block->GetPredecessors().size(); ++j) { - if (!CheckIfPredecessorAtIsExceptional(*catch_block, j)) { - catch_block->GetPredecessors()[j]->ReplaceSuccessor(catch_block, normal_block); - --j; + // a move-exception instruction, as guaranteed by the verifier. However, + // trivially dead predecessors are ignored by the verifier and such code + // has not been removed at this stage. We therefore ignore the assumption + // and rely on GraphChecker to enforce it after initial DCE is run (b/25492628). + HBasicBlock* normal_block = catch_block->SplitCatchBlockAfterMoveException(); + if (normal_block == nullptr) { + // Catch block is either empty or only contains a move-exception. It must + // therefore be dead and will be removed during initial DCE. Do nothing. + DCHECK(!catch_block->EndsWithControlFlowInstruction()); + } else { + // Catch block was split. Re-link normal-flow edges to the new block. + for (size_t j = 0; j < catch_block->GetPredecessors().size(); ++j) { + if (!CheckIfPredecessorAtIsExceptional(*catch_block, j)) { + catch_block->GetPredecessors()[j]->ReplaceSuccessor(catch_block, normal_block); + --j; + } } } } @@ -1163,7 +1173,7 @@ void HInstruction::MoveBefore(HInstruction* cursor) { } HBasicBlock* HBasicBlock::SplitBefore(HInstruction* cursor) { - DCHECK(!graph_->IsInSsaForm()) << "Support for SSA form not implemented"; + DCHECK(!graph_->IsInSsaForm()) << "Support for SSA form not implemented."; DCHECK_EQ(cursor->GetBlock(), this); HBasicBlock* new_block = new (GetGraph()->GetArena()) HBasicBlock(GetGraph(), @@ -1193,7 +1203,7 @@ HBasicBlock* HBasicBlock::SplitBefore(HInstruction* cursor) { } HBasicBlock* HBasicBlock::CreateImmediateDominator() { - DCHECK(!graph_->IsInSsaForm()) << "Support for SSA form not implemented"; + DCHECK(!graph_->IsInSsaForm()) << "Support for SSA form not implemented."; DCHECK(!IsCatchBlock()) << "Support for updating try/catch information not implemented."; HBasicBlock* new_block = new (GetGraph()->GetArena()) HBasicBlock(GetGraph(), GetDexPc()); @@ -1209,6 +1219,34 @@ HBasicBlock* HBasicBlock::CreateImmediateDominator() { return new_block; } +HBasicBlock* HBasicBlock::SplitCatchBlockAfterMoveException() { + DCHECK(!graph_->IsInSsaForm()) << "Support for SSA form not implemented."; + DCHECK(IsCatchBlock()) << "This method is intended for catch blocks only."; + + HInstruction* first_insn = GetFirstInstruction(); + HInstruction* split_before = nullptr; + + if (first_insn != nullptr && first_insn->IsLoadException()) { + // Catch block starts with a LoadException. Split the block after + // the StoreLocal and ClearException which must come after the load. + DCHECK(first_insn->GetNext()->IsStoreLocal()); + DCHECK(first_insn->GetNext()->GetNext()->IsClearException()); + split_before = first_insn->GetNext()->GetNext()->GetNext(); + } else { + // Catch block does not load the exception. Split at the beginning + // to create an empty catch block. + split_before = first_insn; + } + + if (split_before == nullptr) { + // Catch block has no instructions after the split point (must be dead). + // Do not split it but rather signal error by returning nullptr. + return nullptr; + } else { + return SplitBefore(split_before); + } +} + HBasicBlock* HBasicBlock::SplitAfter(HInstruction* cursor) { DCHECK(!cursor->IsControlFlow()); DCHECK_NE(instructions_.last_instruction_, cursor); diff --git a/compiler/optimizing/nodes.h b/compiler/optimizing/nodes.h index 0f2c1cffee..ddd39a3898 100644 --- a/compiler/optimizing/nodes.h +++ b/compiler/optimizing/nodes.h @@ -837,6 +837,15 @@ class HBasicBlock : public ArenaObject { // blocks are consistent (for example ending with a control flow instruction). HBasicBlock* SplitAfter(HInstruction* cursor); + // Split catch block into two blocks after the original move-exception bytecode + // instruction, or at the beginning if not present. Returns the newly created, + // latter block, or nullptr if such block could not be created (must be dead + // in that case). Note that this method just updates raw block information, + // like predecessors, successors, dominators, and instruction list. It does not + // update the graph, reverse post order, loop information, nor make sure the + // blocks are consistent (for example ending with a control flow instruction). + HBasicBlock* SplitCatchBlockAfterMoveException(); + // Merge `other` at the end of `this`. Successors and dominated blocks of // `other` are changed to be successors and dominated blocks of `this`. Note // that this method does not update the graph, reverse post order, loop -- cgit v1.2.3-59-g8ed1b