Change ValidationState::diag to accept an Instruction. (#1749)

This CL changes the signature of diag() to accept an Instruction instead
of the instructions position.

A deprecated variant that accepts the position is available but will be
removed in the near future.

4 files changed, 32 insertions, 40 deletions

diff --git a/source/val/validate_cfg.cpp b/source/val/validate_cfg.cpp
index 37219d93..97dbcc0f 100644
--- a/source/val/validate_cfg.cpp
+++ b/source/val/validate_cfg.cpp
@@ -66,8 +66,7 @@ void printDominatorList(const BasicBlock& b) {
 
 spv_result_t FirstBlockAssert(ValidationState_t& _, uint32_t target) {
   if (_.current_function().IsFirstBlock(target)) {
-    return _.diag(SPV_ERROR_INVALID_CFG,
-                  _.FindDef(_.current_function().id())->InstructionPosition())
+    return _.diag(SPV_ERROR_INVALID_CFG, _.FindDef(_.current_function().id()))
            << "First block " << _.getIdName(target) << " of function "
            << _.getIdName(_.current_function().id()) << " is targeted by block "
            << _.getIdName(_.current_function().current_block()->id());
@@ -77,8 +76,7 @@ spv_result_t FirstBlockAssert(ValidationState_t& _, uint32_t target) {
 
 spv_result_t MergeBlockAssert(ValidationState_t& _, uint32_t merge_block) {
   if (_.current_function().IsBlockType(merge_block, kBlockTypeMerge)) {
-    return _.diag(SPV_ERROR_INVALID_CFG,
-                  _.FindDef(_.current_function().id())->InstructionPosition())
+    return _.diag(SPV_ERROR_INVALID_CFG, _.FindDef(_.current_function().id()))
            << "Block " << _.getIdName(merge_block)
            << " is already a merge block for another header";
   }
@@ -206,12 +204,8 @@ spv_result_t FindCaseFallThrough(
       if (*case_fall_through == 0u) {
         *case_fall_through = block->id();
       } else if (*case_fall_through != block->id()) {
-        uint32_t instruction_id =
-            target_block->label() ? target_block->label()->InstructionPosition()
-                                  : -1;
-
         // Case construct has at most one branch to another case construct.
-        return _.diag(SPV_ERROR_INVALID_CFG, instruction_id)
+        return _.diag(SPV_ERROR_INVALID_CFG, target_block->label())
                << "Case construct that targets "
                << _.getIdName(target_block->id())
                << " has branches to multiple other case construct targets "
@@ -298,8 +292,7 @@ spv_result_t StructuredSwitchChecks(const ValidationState_t& _,
         // must immediately precede T2 in the list of OpSwitch Target operands.
         if ((switch_inst->operands().size() < j + 2) ||
             (case_fall_through != switch_inst->GetOperandAs<uint32_t>(j + 2))) {
-          return _.diag(SPV_ERROR_INVALID_CFG,
-                        switch_inst->InstructionPosition())
+          return _.diag(SPV_ERROR_INVALID_CFG, switch_inst)
                  << "Case construct that targets " << _.getIdName(target)
                  << " has branches to the case construct that targets "
                  << _.getIdName(case_fall_through)
@@ -314,8 +307,7 @@ spv_result_t StructuredSwitchChecks(const ValidationState_t& _,
   // construct.
   for (const auto& pair : num_fall_through_targeted) {
     if (pair.second > 1) {
-      return _.diag(SPV_ERROR_INVALID_CFG,
-                    _.FindDef(pair.first)->InstructionPosition())
+      return _.diag(SPV_ERROR_INVALID_CFG, _.FindDef(pair.first))
              << "Multiple case constructs have branches to the case construct "
                 "that targets "
              << _.getIdName(pair.first);
@@ -353,8 +345,7 @@ spv_result_t StructuredControlFlowChecks(
     auto loop_header_id = loop_header->id();
     auto num_latch_blocks = loop_latch_blocks[loop_header_id].size();
     if (num_latch_blocks != 1) {
-      return _.diag(SPV_ERROR_INVALID_CFG,
-                    _.FindDef(loop_header_id)->InstructionPosition())
+      return _.diag(SPV_ERROR_INVALID_CFG, _.FindDef(loop_header_id))
              << "Loop header " << _.getIdName(loop_header_id)
              << " is targeted by " << num_latch_blocks
              << " back-edge blocks but the standard requires exactly one";
@@ -370,8 +361,7 @@ spv_result_t StructuredControlFlowChecks(
       string construct_name, header_name, exit_name;
       tie(construct_name, header_name, exit_name) =
           ConstructNames(construct.type());
-      return _.diag(SPV_ERROR_INTERNAL,
-                    _.FindDef(header->id())->InstructionPosition())
+      return _.diag(SPV_ERROR_INTERNAL, _.FindDef(header->id()))
              << "Construct " + construct_name + " with " + header_name + " " +
                     _.getIdName(header->id()) + " does not have a " +
                     exit_name + ". This may be a bug in the validator.";
@@ -381,8 +371,7 @@ spv_result_t StructuredControlFlowChecks(
     // header.
     if (merge && merge->reachable()) {
       if (!header->dominates(*merge)) {
-        return _.diag(SPV_ERROR_INVALID_CFG,
-                      _.FindDef(merge->id())->InstructionPosition())
+        return _.diag(SPV_ERROR_INVALID_CFG, _.FindDef(merge->id()))
                << ConstructErrorString(construct, _.getIdName(header->id()),
                                        _.getIdName(merge->id()),
                                        "does not dominate");
@@ -390,8 +379,7 @@ spv_result_t StructuredControlFlowChecks(
       // If it's really a merge block for a selection or loop, then it must be
       // *strictly* dominated by the header.
       if (construct.ExitBlockIsMergeBlock() && (header == merge)) {
-        return _.diag(SPV_ERROR_INVALID_CFG,
-                      _.FindDef(merge->id())->InstructionPosition())
+        return _.diag(SPV_ERROR_INVALID_CFG, _.FindDef(merge->id()))
                << ConstructErrorString(construct, _.getIdName(header->id()),
                                        _.getIdName(merge->id()),
                                        "does not strictly dominate");
@@ -401,8 +389,7 @@ spv_result_t StructuredControlFlowChecks(
     // post-dominance only make sense when the construct is reachable.
     if (header->reachable() && construct.type() == ConstructType::kContinue) {
       if (!merge->postdominates(*header)) {
-        return _.diag(SPV_ERROR_INVALID_CFG,
-                      _.FindDef(merge->id())->InstructionPosition())
+        return _.diag(SPV_ERROR_INVALID_CFG, _.FindDef(merge->id()))
                << ConstructErrorString(construct, _.getIdName(header->id()),
                                        _.getIdName(merge->id()),
                                        "is not post dominated by");
@@ -419,8 +406,7 @@ spv_result_t StructuredControlFlowChecks(
           string construct_name, header_name, exit_name;
           tie(construct_name, header_name, exit_name) =
               ConstructNames(construct.type());
-          return _.diag(SPV_ERROR_INVALID_CFG,
-                        _.FindDef(pred->id())->InstructionPosition())
+          return _.diag(SPV_ERROR_INVALID_CFG, _.FindDef(pred->id()))
                  << "block <ID> " << pred->id() << " branches to the "
                  << construct_name << " construct, but not to the "
                  << header_name << " <ID> " << header->id();
@@ -454,8 +440,7 @@ spv_result_t PerformCfgChecks(ValidationState_t& _) {
         }
         first = false;
       }
-      return _.diag(SPV_ERROR_INVALID_CFG,
-                    _.FindDef(function.id())->InstructionPosition())
+      return _.diag(SPV_ERROR_INVALID_CFG, _.FindDef(function.id()))
              << "Block(s) " << undef_blocks << "}"
              << " are referenced but not defined in function "
              << _.getIdName(function.id());
@@ -515,8 +500,7 @@ spv_result_t PerformCfgChecks(ValidationState_t& _) {
         if (auto idom = (*block)->immediate_dominator()) {
           if (idom != function.pseudo_entry_block() &&
               block == std::find(begin(blocks), block, idom)) {
-            return _.diag(SPV_ERROR_INVALID_CFG,
-                          _.FindDef(idom->id())->InstructionPosition())
+            return _.diag(SPV_ERROR_INVALID_CFG, _.FindDef(idom->id()))
                    << "Block " << _.getIdName((*block)->id())
                    << " appears in the binary before its dominator "
                    << _.getIdName(idom->id());
@@ -531,8 +515,7 @@ spv_result_t PerformCfgChecks(ValidationState_t& _) {
         for (auto block = begin(blocks); block != end(blocks); ++block) {
           if (function.GetBlockDepth(*block) >
               control_flow_nesting_depth_limit) {
-            return _.diag(SPV_ERROR_INVALID_CFG,
-                          _.FindDef((*block)->id())->InstructionPosition())
+            return _.diag(SPV_ERROR_INVALID_CFG, _.FindDef((*block)->id()))
                    << "Maximum Control Flow nesting depth exceeded.";
           }
         }
@@ -605,7 +588,7 @@ spv_result_t CfgPass(ValidationState_t& _, const Instruction* inst) {
       const Instruction* return_type_inst = _.FindDef(return_type);
       assert(return_type_inst);
       if (return_type_inst->opcode() != SpvOpTypeVoid)
-        return _.diag(SPV_ERROR_INVALID_CFG, inst->InstructionPosition())
+        return _.diag(SPV_ERROR_INVALID_CFG, inst)
                << "OpReturn can only be called from a function with void "
                << "return type.";
     }
diff --git a/source/val/validate_composites.cpp b/source/val/validate_composites.cpp
index 68bbdd23..766a3a39 100644
--- a/source/val/validate_composites.cpp
+++ b/source/val/validate_composites.cpp
@@ -424,7 +424,7 @@ spv_result_t ValidateVectorShuffle(ValidationState_t& _,
                                    const Instruction* inst) {
   auto resultType = _.FindDef(inst->type_id());
   if (!resultType || resultType->opcode() != SpvOpTypeVector) {
-    return _.diag(SPV_ERROR_INVALID_ID, inst->InstructionPosition())
+    return _.diag(SPV_ERROR_INVALID_ID, inst)
            << "The Result Type of OpVectorShuffle must be"
            << " OpTypeVector. Found Op"
            << spvOpcodeString(static_cast<SpvOp>(resultType->opcode())) << ".";
@@ -435,7 +435,7 @@ spv_result_t ValidateVectorShuffle(ValidationState_t& _,
   auto componentCount = inst->operands().size() - 4;
   auto resultVectorDimension = resultType->GetOperandAs<uint32_t>(2);
   if (componentCount != resultVectorDimension) {
-    return _.diag(SPV_ERROR_INVALID_ID, inst->InstructionPosition())
+    return _.diag(SPV_ERROR_INVALID_ID, inst)
            << "OpVectorShuffle component literals count does not match "
               "Result Type <id> '"
            << _.getIdName(resultType->id()) << "'s vector component count.";
@@ -448,21 +448,21 @@ spv_result_t ValidateVectorShuffle(ValidationState_t& _,
   auto vector2Object = _.FindDef(inst->GetOperandAs<uint32_t>(3));
   auto vector2Type = _.FindDef(vector2Object->type_id());
   if (!vector1Type || vector1Type->opcode() != SpvOpTypeVector) {
-    return _.diag(SPV_ERROR_INVALID_ID, inst->InstructionPosition())
+    return _.diag(SPV_ERROR_INVALID_ID, inst)
            << "The type of Vector 1 must be OpTypeVector.";
   }
   if (!vector2Type || vector2Type->opcode() != SpvOpTypeVector) {
-    return _.diag(SPV_ERROR_INVALID_ID, inst->InstructionPosition())
+    return _.diag(SPV_ERROR_INVALID_ID, inst)
            << "The type of Vector 2 must be OpTypeVector.";
   }
 
   auto resultComponentType = resultType->GetOperandAs<uint32_t>(1);
   if (vector1Type->GetOperandAs<uint32_t>(1) != resultComponentType) {
-    return _.diag(SPV_ERROR_INVALID_ID, inst->InstructionPosition())
+    return _.diag(SPV_ERROR_INVALID_ID, inst)
            << "The Component Type of Vector 1 must be the same as ResultType.";
   }
   if (vector2Type->GetOperandAs<uint32_t>(1) != resultComponentType) {
-    return _.diag(SPV_ERROR_INVALID_ID, inst->InstructionPosition())
+    return _.diag(SPV_ERROR_INVALID_ID, inst)
            << "The Component Type of Vector 2 must be the same as ResultType.";
   }
 
@@ -474,7 +474,7 @@ spv_result_t ValidateVectorShuffle(ValidationState_t& _,
   for (size_t i = firstLiteralIndex; i < inst->operands().size(); ++i) {
     auto literal = inst->GetOperandAs<uint32_t>(i);
     if (literal != 0xFFFFFFFF && literal >= N) {
-      return _.diag(SPV_ERROR_INVALID_ID, inst->InstructionPosition())
+      return _.diag(SPV_ERROR_INVALID_ID, inst)
              << "Component index " << literal << " is out of bounds for "
              << "combined (Vector1 + Vector2) size of " << N << ".";
     }
diff --git a/source/val/validation_state.cpp b/source/val/validation_state.cpp
index a2f146a0..94bd5922 100644
--- a/source/val/validation_state.cpp
+++ b/source/val/validation_state.cpp
@@ -283,6 +283,12 @@ DiagnosticStream ValidationState_t::diag(spv_result_t error_code) const {
 }
 
 DiagnosticStream ValidationState_t::diag(spv_result_t error_code,
+                                         const Instruction* inst) const {
+  int instruction_counter = inst ? inst->InstructionPosition() : -1;
+  return diag(error_code, instruction_counter);
+}
+
+DiagnosticStream ValidationState_t::diag(spv_result_t error_code,
                                          int instruction_counter) const {
   std::string disassembly;
   if (instruction_counter >= 0 && static_cast<size_t>(instruction_counter) <=
diff --git a/source/val/validation_state.h b/source/val/validation_state.h
index 6f38d28b..6195a4b4 100644
--- a/source/val/validation_state.h
+++ b/source/val/validation_state.h
@@ -154,7 +154,7 @@ class ValidationState_t {
   bool IsOpcodeInCurrentLayoutSection(SpvOp op);
 
   DiagnosticStream diag(spv_result_t error_code) const;
-  DiagnosticStream diag(spv_result_t error_code, int instruction_counter) const;
+  DiagnosticStream diag(spv_result_t error_code, const Instruction* inst) const;
 
   /// Returns the function states
   std::deque<Function>& functions();
@@ -508,6 +508,9 @@ class ValidationState_t {
  private:
   ValidationState_t(const ValidationState_t&);
 
+  /// Deprecated.  Use the Instruction variant instead.
+  DiagnosticStream diag(spv_result_t error_code, int instruction_counter) const;
+
   const spv_const_context context_;
 
   /// Stores the Validator command line options. Must be a valid options object.