OSDN Git Service

Improve error messages in art::GraphChecker and art::SSAChecker
authorRoland Levillain <rpl@google.com>
Wed, 21 Jan 2015 11:39:58 +0000 (11:39 +0000)
committerRoland Levillain <rpl@google.com>
Wed, 21 Jan 2015 11:39:58 +0000 (11:39 +0000)
- Add an art::GraphChecker::AddError helper.
- Use StringPrintf instead of std::stringstream.
- Rephrase some error messages.

Change-Id: Ia741e9e67cb5122f086a7383a2bc02d60ca637df

compiler/optimizing/graph_checker.cc
compiler/optimizing/graph_checker.h
runtime/primitive.cc
runtime/primitive.h

index 291b14c..4d74c4e 100644 (file)
 #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())));
     }
   }
 }
index ae1557b..5ec3003 100644 (file)
@@ -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;
index a639f93..d29a060 100644 (file)
@@ -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) {
index afcc64d..50d171c 100644 (file)
@@ -146,6 +146,8 @@ class Primitive {
     }
   }
 
+  static const char* PrettyDescriptor(Type type);
+
  private:
   DISALLOW_IMPLICIT_CONSTRUCTORS(Primitive);
 };