diff options
author | Roland Levillain <rpl@google.com> | 2015-01-21 11:39:58 +0000 |
---|---|---|
committer | Roland Levillain <rpl@google.com> | 2015-01-21 11:39:58 +0000 |
commit | 5c4405e8ca4a0c1ee2d7759e650530c9aff77fd0 (patch) | |
tree | 02d05457c8810966018c1c852f78a16da78cddc6 | |
parent | 48aba6847270221c3f189732ecc69a0258d3aaca (diff) | |
download | art-5c4405e8ca4a0c1ee2d7759e650530c9aff77fd0.zip art-5c4405e8ca4a0c1ee2d7759e650530c9aff77fd0.tar.gz art-5c4405e8ca4a0c1ee2d7759e650530c9aff77fd0.tar.bz2 |
Improve error messages in art::GraphChecker and art::SSAChecker
- Add an art::GraphChecker::AddError helper.
- Use StringPrintf instead of std::stringstream.
- Rephrase some error messages.
Change-Id: Ia741e9e67cb5122f086a7383a2bc02d60ca637df
-rw-r--r-- | compiler/optimizing/graph_checker.cc | 341 | ||||
-rw-r--r-- | compiler/optimizing/graph_checker.h | 5 | ||||
-rw-r--r-- | runtime/primitive.cc | 5 | ||||
-rw-r--r-- | runtime/primitive.h | 2 |
4 files changed, 164 insertions, 189 deletions
diff --git a/compiler/optimizing/graph_checker.cc b/compiler/optimizing/graph_checker.cc index 291b14c..4d74c4e 100644 --- a/compiler/optimizing/graph_checker.cc +++ b/compiler/optimizing/graph_checker.cc @@ -17,10 +17,10 @@ #include "graph_checker.h" #include <map> -#include <sstream> #include <string> #include "base/bit_vector-inl.h" +#include "base/stringprintf.h" namespace art { @@ -45,15 +45,11 @@ void GraphChecker::VisitBasicBlock(HBasicBlock* block) { } } if (p_count_in_block_predecessors != block_count_in_p_successors) { - std::stringstream error; - error << "Block " << block->GetBlockId() - << " lists " << p_count_in_block_predecessors - << " occurrences of block " << p->GetBlockId() - << " in its predecessors, whereas block " << p->GetBlockId() - << " lists " << block_count_in_p_successors - << " occurrences of block " << block->GetBlockId() - << " in its successors."; - errors_.push_back(error.str()); + AddError(StringPrintf( + "Block %d lists %zu occurrences of block %d in its predecessors, whereas " + "block %d lists %zu occurrences of block %d in its successors.", + block->GetBlockId(), p_count_in_block_predecessors, p->GetBlockId(), + p->GetBlockId(), block_count_in_p_successors, block->GetBlockId())); } } @@ -75,35 +71,27 @@ void GraphChecker::VisitBasicBlock(HBasicBlock* block) { } } if (s_count_in_block_successors != block_count_in_s_predecessors) { - std::stringstream error; - error << "Block " << block->GetBlockId() - << " lists " << s_count_in_block_successors - << " occurrences of block " << s->GetBlockId() - << " in its successors, whereas block " << s->GetBlockId() - << " lists " << block_count_in_s_predecessors - << " occurrences of block " << block->GetBlockId() - << " in its predecessors."; - errors_.push_back(error.str()); + AddError(StringPrintf( + "Block %d lists %zu occurrences of block %d in its successors, whereas " + "block %d lists %zu occurrences of block %d in its predecessors.", + block->GetBlockId(), s_count_in_block_successors, s->GetBlockId(), + s->GetBlockId(), block_count_in_s_predecessors, block->GetBlockId())); } } // Ensure `block` ends with a branch instruction. HInstruction* last_inst = block->GetLastInstruction(); if (last_inst == nullptr || !last_inst->IsControlFlow()) { - std::stringstream error; - error << "Block " << block->GetBlockId() - << " does not end with a branch instruction."; - errors_.push_back(error.str()); + AddError(StringPrintf("Block %d does not end with a branch instruction.", + block->GetBlockId())); } // Visit this block's list of phis. for (HInstructionIterator it(block->GetPhis()); !it.Done(); it.Advance()) { // Ensure this block's list of phis contains only phis. if (!it.Current()->IsPhi()) { - std::stringstream error; - error << "Block " << current_block_->GetBlockId() - << " has a non-phi in its phi list."; - errors_.push_back(error.str()); + AddError(StringPrintf("Block %d has a non-phi in its phi list.", + current_block_->GetBlockId())); } it.Current()->Accept(this); } @@ -113,10 +101,8 @@ void GraphChecker::VisitBasicBlock(HBasicBlock* block) { it.Advance()) { // Ensure this block's list of instructions does not contains phis. if (it.Current()->IsPhi()) { - std::stringstream error; - error << "Block " << current_block_->GetBlockId() - << " has a phi in its non-phi list."; - errors_.push_back(error.str()); + AddError(StringPrintf("Block %d has a phi in its non-phi list.", + current_block_->GetBlockId())); } it.Current()->Accept(this); } @@ -124,30 +110,24 @@ void GraphChecker::VisitBasicBlock(HBasicBlock* block) { void GraphChecker::VisitInstruction(HInstruction* instruction) { if (seen_ids_.IsBitSet(instruction->GetId())) { - std::stringstream error; - error << "Duplicate id in graph " << instruction->GetId() << "."; - errors_.push_back(error.str()); + AddError(StringPrintf("Instruction id %d is duplicate in graph.", + instruction->GetId())); } else { seen_ids_.SetBit(instruction->GetId()); } // Ensure `instruction` is associated with `current_block_`. - if (instruction->GetBlock() != current_block_) { - std::stringstream error; - if (instruction->IsPhi()) { - error << "Phi "; - } else { - error << "Instruction "; - } - error << instruction->GetId() << " in block " - << current_block_->GetBlockId(); - if (instruction->GetBlock() != nullptr) { - error << " associated with block " - << instruction->GetBlock()->GetBlockId() << "."; - } else { - error << " not associated with any block."; - } - errors_.push_back(error.str()); + if (instruction->GetBlock() == nullptr) { + AddError(StringPrintf("%s %d in block %d not associated with any block.", + instruction->IsPhi() ? "Phi" : "Instruction", + instruction->GetId(), + current_block_->GetBlockId())); + } else if (instruction->GetBlock() != current_block_) { + AddError(StringPrintf("%s %d in block %d associated with block %d.", + instruction->IsPhi() ? "Phi" : "Instruction", + instruction->GetId(), + current_block_->GetBlockId(), + instruction->GetBlock()->GetBlockId())); } // Ensure the inputs of `instruction` are defined in a block of the graph. @@ -158,11 +138,10 @@ void GraphChecker::VisitInstruction(HInstruction* instruction) { ? input->GetBlock()->GetPhis() : input->GetBlock()->GetInstructions(); if (!list.Contains(input)) { - std::stringstream error; - error << "Input " << input->GetId() - << " of instruction " << instruction->GetId() - << " is not defined in a basic block of the control-flow graph."; - errors_.push_back(error.str()); + AddError(StringPrintf("Input %d of instruction %d is not defined " + "in a basic block of the control-flow graph.", + input->GetId(), + instruction->GetId())); } } @@ -174,11 +153,10 @@ void GraphChecker::VisitInstruction(HInstruction* instruction) { ? use->GetBlock()->GetPhis() : use->GetBlock()->GetInstructions(); if (!list.Contains(use)) { - std::stringstream error; - error << "User " << use->GetId() - << " of instruction " << instruction->GetId() - << " is not defined in a basic block of the control-flow graph."; - errors_.push_back(error.str()); + AddError(StringPrintf("User %d of instruction %d is not defined " + "in a basic block of the control-flow graph.", + use->GetId(), + instruction->GetId())); } } } @@ -193,10 +171,9 @@ void SSAChecker::VisitBasicBlock(HBasicBlock* block) { for (size_t j = 0; j < block->GetSuccessors().Size(); ++j) { HBasicBlock* successor = block->GetSuccessors().Get(j); if (successor->GetPredecessors().Size() > 1) { - std::stringstream error; - error << "Critical edge between blocks " << block->GetBlockId() - << " and " << successor->GetBlockId() << "."; - errors_.push_back(error.str()); + AddError(StringPrintf("Critical edge between blocks %d and %d.", + block->GetBlockId(), + successor->GetBlockId())); } } } @@ -212,47 +189,52 @@ void SSAChecker::CheckLoop(HBasicBlock* loop_header) { // Ensure the pre-header block is first in the list of // predecessors of a loop header. if (!loop_header->IsLoopPreHeaderFirstPredecessor()) { - std::stringstream error; - error << "Loop pre-header is not the first predecessor of the loop header " - << id << "."; - errors_.push_back(error.str()); + AddError(StringPrintf( + "Loop pre-header is not the first predecessor of the loop header %d.", + id)); } // Ensure the loop header has only two predecessors and that only the // second one is a back edge. - if (loop_header->GetPredecessors().Size() < 2) { - std::stringstream error; - error << "Loop header " << id << " has less than two predecessors."; - errors_.push_back(error.str()); - } else if (loop_header->GetPredecessors().Size() > 2) { - std::stringstream error; - error << "Loop header " << id << " has more than two predecessors."; - errors_.push_back(error.str()); + size_t num_preds = loop_header->GetPredecessors().Size(); + if (num_preds < 2) { + AddError(StringPrintf( + "Loop header %d has less than two predecessors: %zu.", + id, + num_preds)); + } else if (num_preds > 2) { + AddError(StringPrintf( + "Loop header %d has more than two predecessors: %zu.", + id, + num_preds)); } else { HLoopInformation* loop_information = loop_header->GetLoopInformation(); HBasicBlock* first_predecessor = loop_header->GetPredecessors().Get(0); if (loop_information->IsBackEdge(first_predecessor)) { - std::stringstream error; - error << "First predecessor of loop header " << id << " is a back edge."; - errors_.push_back(error.str()); + AddError(StringPrintf( + "First predecessor of loop header %d is a back edge.", + id)); } HBasicBlock* second_predecessor = loop_header->GetPredecessors().Get(1); if (!loop_information->IsBackEdge(second_predecessor)) { - std::stringstream error; - error << "Second predecessor of loop header " << id - << " is not a back edge."; - errors_.push_back(error.str()); + AddError(StringPrintf( + "Second predecessor of loop header %d is not a back edge.", + id)); } } // Ensure there is only one back edge per loop. size_t num_back_edges = loop_header->GetLoopInformation()->GetBackEdges().Size(); - if (num_back_edges != 1) { - std::stringstream error; - error << "Loop defined by header " << id << " has " - << num_back_edges << " back edge(s)."; - errors_.push_back(error.str()); + if (num_back_edges == 0) { + AddError(StringPrintf( + "Loop defined by header %d has no back edge.", + id)); + } else if (num_back_edges > 1) { + AddError(StringPrintf( + "Loop defined by header %d has several back edges: %zu.", + id, + num_back_edges)); } // Ensure all blocks in the loop are dominated by the loop header. @@ -261,10 +243,9 @@ void SSAChecker::CheckLoop(HBasicBlock* loop_header) { for (uint32_t i : loop_blocks.Indexes()) { HBasicBlock* loop_block = GetGraph()->GetBlocks().Get(i); if (!loop_header->Dominates(loop_block)) { - std::stringstream error; - error << "Loop block " << loop_block->GetBlockId() - << " not dominated by loop header " << id; - errors_.push_back(error.str()); + AddError(StringPrintf("Loop block %d not dominated by loop header %d.", + loop_block->GetBlockId(), + id)); } } } @@ -277,12 +258,10 @@ void SSAChecker::VisitInstruction(HInstruction* instruction) { !use_it.Done(); use_it.Advance()) { HInstruction* use = use_it.Current()->GetUser(); if (!use->IsPhi() && !instruction->StrictlyDominates(use)) { - std::stringstream error; - error << "Instruction " << instruction->GetId() - << " in block " << current_block_->GetBlockId() - << " does not dominate use " << use->GetId() - << " in block " << use->GetBlock()->GetBlockId() << "."; - errors_.push_back(error.str()); + AddError(StringPrintf("Instruction %d in block %d does not dominate " + "use %d in block %d.", + instruction->GetId(), current_block_->GetBlockId(), + use->GetId(), use->GetBlock()->GetBlockId())); } } @@ -294,13 +273,12 @@ void SSAChecker::VisitInstruction(HInstruction* instruction) { HInstruction* env_instruction = environment->GetInstructionAt(i); if (env_instruction != nullptr && !env_instruction->StrictlyDominates(instruction)) { - std::stringstream error; - error << "Instruction " << env_instruction->GetId() - << " in environment of instruction " << instruction->GetId() - << " from block " << current_block_->GetBlockId() - << " does not dominate instruction " << instruction->GetId() - << "."; - errors_.push_back(error.str()); + AddError(StringPrintf("Instruction %d in environment of instruction %d " + "from block %d does not dominate instruction %d.", + env_instruction->GetId(), + instruction->GetId(), + current_block_->GetBlockId(), + instruction->GetId())); } } } @@ -311,25 +289,21 @@ void SSAChecker::VisitPhi(HPhi* phi) { // Ensure the first input of a phi is not itself. if (phi->InputAt(0) == phi) { - std::stringstream error; - error << "Loop phi " << phi->GetId() - << " in block " << phi->GetBlock()->GetBlockId() - << " is its own first input."; - errors_.push_back(error.str()); + AddError(StringPrintf("Loop phi %d in block %d is its own first input.", + phi->GetId(), + phi->GetBlock()->GetBlockId())); } - // Ensure the number of phi inputs is the same as the number of + // Ensure the number of inputs of a phi is the same as the number of // its predecessors. const GrowableArray<HBasicBlock*>& predecessors = phi->GetBlock()->GetPredecessors(); if (phi->InputCount() != predecessors.Size()) { - std::stringstream error; - error << "Phi " << phi->GetId() - << " in block " << phi->GetBlock()->GetBlockId() - << " has " << phi->InputCount() << " inputs, but block " - << phi->GetBlock()->GetBlockId() << " has " - << predecessors.Size() << " predecessors."; - errors_.push_back(error.str()); + AddError(StringPrintf( + "Phi %d in block %d has %zu inputs, " + "but block %d has %zu predecessors.", + phi->GetId(), phi->GetBlock()->GetBlockId(), phi->InputCount(), + phi->GetBlock()->GetBlockId(), predecessors.Size())); } else { // Ensure phi input at index I either comes from the Ith // predecessor or from a block that dominates this predecessor. @@ -338,13 +312,11 @@ void SSAChecker::VisitPhi(HPhi* phi) { HBasicBlock* predecessor = predecessors.Get(i); if (!(input->GetBlock() == predecessor || input->GetBlock()->Dominates(predecessor))) { - std::stringstream error; - error << "Input " << input->GetId() << " at index " << i - << " of phi " << phi->GetId() - << " from block " << phi->GetBlock()->GetBlockId() - << " is not defined in predecessor number " << i - << " nor in a block dominating it."; - errors_.push_back(error.str()); + AddError(StringPrintf( + "Input %d at index %zu of phi %d from block %d is not defined in " + "predecessor number %zu nor in a block dominating it.", + input->GetId(), i, phi->GetId(), phi->GetBlock()->GetBlockId(), + i)); } } } @@ -369,69 +341,61 @@ void SSAChecker::VisitIf(HIf* instruction) { if (input->IsIntConstant()) { int value = input->AsIntConstant()->GetValue(); if (value != 0 && value != 1) { - std::stringstream error; - error << "If instruction " << instruction->GetId() - << " has a non-boolean constant input whose value is: " - << value << "."; - errors_.push_back(error.str()); + AddError(StringPrintf( + "If instruction %d has a non-Boolean constant input " + "whose value is: %d.", + instruction->GetId(), + value)); } } else if (instruction->InputAt(0)->GetType() != Primitive::kPrimBoolean) { - std::stringstream error; - error << "If instruction " << instruction->GetId() - << " has a non-boolean input type: " - << instruction->InputAt(0)->GetType() << "."; - errors_.push_back(error.str()); + AddError(StringPrintf( + "If instruction %d has a non-Boolean input type: %s.", + instruction->GetId(), + Primitive::PrettyDescriptor(instruction->InputAt(0)->GetType()))); } } void SSAChecker::VisitCondition(HCondition* op) { VisitInstruction(op); if (op->GetType() != Primitive::kPrimBoolean) { - std::stringstream error; - error << "Condition " << op->DebugName() << " " << op->GetId() - << " has a non-boolean result type: " - << op->GetType() << "."; - errors_.push_back(error.str()); + AddError(StringPrintf( + "Condition %s %d has a non-Boolean result type: %s.", + op->DebugName(), op->GetId(), + Primitive::PrettyDescriptor(op->GetType()))); } HInstruction* lhs = op->InputAt(0); HInstruction* rhs = op->InputAt(1); if (lhs->GetType() == Primitive::kPrimNot) { if (!op->IsEqual() && !op->IsNotEqual()) { - std::stringstream error; - error << "Condition " << op->DebugName() << " " << op->GetId() - << " uses an object as left-hand side input."; - errors_.push_back(error.str()); + AddError(StringPrintf( + "Condition %s %d uses an object as left-hand side input.", + op->DebugName(), op->GetId())); } if (rhs->IsIntConstant() && rhs->AsIntConstant()->GetValue() != 0) { - std::stringstream error; - error << "Condition " << op->DebugName() << " " << op->GetId() - << " compares an object with a non-0 integer: " - << rhs->AsIntConstant()->GetValue() - << "."; - errors_.push_back(error.str()); + AddError(StringPrintf( + "Condition %s %d compares an object with a non-zero integer: %d.", + op->DebugName(), op->GetId(), + rhs->AsIntConstant()->GetValue())); } } else if (rhs->GetType() == Primitive::kPrimNot) { if (!op->IsEqual() && !op->IsNotEqual()) { - std::stringstream error; - error << "Condition " << op->DebugName() << " " << op->GetId() - << " uses an object as right-hand side input."; - errors_.push_back(error.str()); + AddError(StringPrintf( + "Condition %s %d uses an object as right-hand side input.", + op->DebugName(), op->GetId())); } if (lhs->IsIntConstant() && lhs->AsIntConstant()->GetValue() != 0) { - std::stringstream error; - error << "Condition " << op->DebugName() << " " << op->GetId() - << " compares a non-0 integer with an object: " - << lhs->AsIntConstant()->GetValue() - << "."; - errors_.push_back(error.str()); + AddError(StringPrintf( + "Condition %s %d compares a non-zero integer with an object: %d.", + op->DebugName(), op->GetId(), + lhs->AsIntConstant()->GetValue())); } } else if (PrimitiveKind(lhs->GetType()) != PrimitiveKind(rhs->GetType())) { - std::stringstream error; - error << "Condition " << op->DebugName() << " " << op->GetId() - << " has inputs of different type: " - << lhs->GetType() << ", and " << rhs->GetType() - << "."; - errors_.push_back(error.str()); + AddError(StringPrintf( + "Condition %s %d has inputs of different types: " + "%s, and %s.", + op->DebugName(), op->GetId(), + Primitive::PrettyDescriptor(lhs->GetType()), + Primitive::PrettyDescriptor(rhs->GetType()))); } } @@ -439,41 +403,40 @@ void SSAChecker::VisitBinaryOperation(HBinaryOperation* op) { VisitInstruction(op); if (op->IsUShr() || op->IsShr() || op->IsShl()) { if (PrimitiveKind(op->InputAt(1)->GetType()) != Primitive::kPrimInt) { - std::stringstream error; - error << "Shift operation " << op->DebugName() << " " << op->GetId() - << " has a non-int kind second input: " - << op->InputAt(1)->DebugName() << " of type " << op->InputAt(1)->GetType() - << "."; - errors_.push_back(error.str()); + AddError(StringPrintf( + "Shift operation %s %d has a non-int kind second input: " + "%s of type %s.", + op->DebugName(), op->GetId(), + op->InputAt(1)->DebugName(), + Primitive::PrettyDescriptor(op->InputAt(1)->GetType()))); } } else { if (PrimitiveKind(op->InputAt(1)->GetType()) != PrimitiveKind(op->InputAt(0)->GetType())) { - std::stringstream error; - error << "Binary operation " << op->DebugName() << " " << op->GetId() - << " has inputs of different type: " - << op->InputAt(0)->GetType() << ", and " << op->InputAt(1)->GetType() - << "."; - errors_.push_back(error.str()); + AddError(StringPrintf( + "Binary operation %s %d has inputs of different types: " + "%s, and %s.", + op->DebugName(), op->GetId(), + Primitive::PrettyDescriptor(op->InputAt(0)->GetType()), + Primitive::PrettyDescriptor(op->InputAt(1)->GetType()))); } } if (op->IsCompare()) { if (op->GetType() != Primitive::kPrimInt) { - std::stringstream error; - error << "Compare operation " << op->GetId() - << " has a non-int result type: " - << op->GetType() << "."; - errors_.push_back(error.str()); + AddError(StringPrintf( + "Compare operation %d has a non-int result type: %s.", + op->GetId(), + Primitive::PrettyDescriptor(op->GetType()))); } } else { // Use the first input, so that we can also make this check for shift operations. if (PrimitiveKind(op->GetType()) != PrimitiveKind(op->InputAt(0)->GetType())) { - std::stringstream error; - error << "Binary operation " << op->DebugName() << " " << op->GetId() - << " has a result type different than its input type: " - << op->GetType() << ", and " << op->InputAt(1)->GetType() - << "."; - errors_.push_back(error.str()); + AddError(StringPrintf( + "Binary operation %s %d has a result type different " + "from its input type: %s vs %s.", + op->DebugName(), op->GetId(), + Primitive::PrettyDescriptor(op->GetType()), + Primitive::PrettyDescriptor(op->InputAt(1)->GetType()))); } } } diff --git a/compiler/optimizing/graph_checker.h b/compiler/optimizing/graph_checker.h index ae1557b..5ec3003 100644 --- a/compiler/optimizing/graph_checker.h +++ b/compiler/optimizing/graph_checker.h @@ -60,6 +60,11 @@ class GraphChecker : public HGraphDelegateVisitor { } protected: + // Report a new error. + void AddError(const std::string& error) { + errors_.push_back(error); + } + ArenaAllocator* const allocator_; // The block currently visited. HBasicBlock* current_block_ = nullptr; diff --git a/runtime/primitive.cc b/runtime/primitive.cc index a639f93..d29a060 100644 --- a/runtime/primitive.cc +++ b/runtime/primitive.cc @@ -31,6 +31,11 @@ static const char* kTypeNames[] = { "PrimVoid", }; +const char* Primitive::PrettyDescriptor(Primitive::Type type) { + CHECK(Primitive::kPrimNot <= type && type <= Primitive::kPrimVoid) << static_cast<int>(type); + return kTypeNames[type]; +} + std::ostream& operator<<(std::ostream& os, const Primitive::Type& type) { int32_t int_type = static_cast<int32_t>(type); if (type >= Primitive::kPrimNot && type <= Primitive::kPrimVoid) { diff --git a/runtime/primitive.h b/runtime/primitive.h index afcc64d..50d171c 100644 --- a/runtime/primitive.h +++ b/runtime/primitive.h @@ -146,6 +146,8 @@ class Primitive { } } + static const char* PrettyDescriptor(Type type); + private: DISALLOW_IMPLICIT_CONSTRUCTORS(Primitive); }; |