summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorRoland Levillain <rpl@google.com>2015-01-21 11:39:58 +0000
committerRoland Levillain <rpl@google.com>2015-01-21 11:39:58 +0000
commit5c4405e8ca4a0c1ee2d7759e650530c9aff77fd0 (patch)
tree02d05457c8810966018c1c852f78a16da78cddc6
parent48aba6847270221c3f189732ecc69a0258d3aaca (diff)
downloadart-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.cc341
-rw-r--r--compiler/optimizing/graph_checker.h5
-rw-r--r--runtime/primitive.cc5
-rw-r--r--runtime/primitive.h2
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);
};