Change some asserts to normal errors in IrLoader.

diff --git a/source/opt/build_module.cpp b/source/opt/build_module.cpp
index 2800c09..2699a46 100644 (file)
--- a/source/opt/build_module.cpp
+++ b/source/opt/build_module.cpp
@@ -35,8 +35,10 @@ spv_result_t SetSpvHeader(void* builder, spv_endianness_t, uint32_t magic,
 // Processes a parsed instruction for IrLoader. Meets the interface requirement
 // of spvBinaryParse().
 spv_result_t SetSpvInst(void* builder, const spv_parsed_instruction_t* inst) {
-  reinterpret_cast<ir::IrLoader*>(builder)->AddInstruction(inst);
-  return SPV_SUCCESS;
+  if (reinterpret_cast<ir::IrLoader*>(builder)->AddInstruction(inst)) {
+    return SPV_SUCCESS;
+  }
+  return SPV_ERROR_INVALID_BINARY;
 };
 
 }  // annoymous namespace

diff --git a/source/opt/ir_loader.cpp b/source/opt/ir_loader.cpp
index 09106a3..d358e97 100644 (file)
--- a/source/opt/ir_loader.cpp
+++ b/source/opt/ir_loader.cpp
@@ -20,35 +20,67 @@
 namespace spvtools {
 namespace ir {
 
-void IrLoader::AddInstruction(const spv_parsed_instruction_t* inst) {
+IrLoader::IrLoader(const MessageConsumer& consumer, Module* module)
+    : consumer_(consumer),
+      module_(module),
+      source_("<instruction>"),
+      inst_index_(0) {}
+
+bool IrLoader::AddInstruction(const spv_parsed_instruction_t* inst) {
+  ++inst_index_;
   const auto opcode = static_cast<SpvOp>(inst->opcode);
   if (IsDebugLineInst(opcode)) {
     dbg_line_info_.push_back(Instruction(*inst));
-    return;
+    return true;
   }
 
   std::unique_ptr<Instruction> spv_inst(
       new Instruction(*inst, std::move(dbg_line_info_)));
   dbg_line_info_.clear();
+
+  const char* src = source_.c_str();
+  spv_position_t loc = {inst_index_, 0, 0};
+
   // Handle function and basic block boundaries first, then normal
   // instructions.
   if (opcode == SpvOpFunction) {
-    SPIRV_ASSERT(consumer_, function_ == nullptr);
-    SPIRV_ASSERT(consumer_, block_ == nullptr);
+    if (function_ != nullptr) {
+      Error(consumer_, src, loc, "function inside function");
+      return false;
+    }
     function_.reset(new Function(std::move(spv_inst)));
   } else if (opcode == SpvOpFunctionEnd) {
-    SPIRV_ASSERT(consumer_, function_ != nullptr);
-    SPIRV_ASSERT(consumer_, block_ == nullptr);
+    if (function_ == nullptr) {
+      Error(consumer_, src, loc,
+            "OpFunctionEnd without corresponding OpFunction");
+      return false;
+    }
+    if (block_ != nullptr) {
+      Error(consumer_, src, loc, "OpFunctionEnd inside basic block");
+      return false;
+    }
     function_->SetFunctionEnd(std::move(spv_inst));
     module_->AddFunction(std::move(function_));
     function_ = nullptr;
   } else if (opcode == SpvOpLabel) {
-    SPIRV_ASSERT(consumer_, function_ != nullptr);
-    SPIRV_ASSERT(consumer_, block_ == nullptr);
+    if (function_ == nullptr) {
+      Error(consumer_, src, loc, "OpLabel outside function");
+      return false;
+    }
+    if (block_ != nullptr) {
+      Error(consumer_, src, loc, "OpLabel inside basic block");
+      return false;
+    }
     block_.reset(new BasicBlock(std::move(spv_inst)));
   } else if (IsTerminatorInst(opcode)) {
-    SPIRV_ASSERT(consumer_, function_ != nullptr);
-    SPIRV_ASSERT(consumer_, block_ != nullptr);
+    if (function_ == nullptr) {
+      Error(consumer_, src, loc, "terminator instruction outside function");
+      return false;
+    }
+    if (block_ == nullptr) {
+      Error(consumer_, src, loc, "terminator instruction outside basic block");
+      return false;
+    }
     block_->AddInstruction(std::move(spv_inst));
     function_->AddBasicBlock(std::move(block_));
     block_ = nullptr;
@@ -82,13 +114,20 @@ void IrLoader::AddInstruction(const spv_parsed_instruction_t* inst) {
       }
     } else {
       if (block_ == nullptr) {  // Inside function but outside blocks
-        SPIRV_ASSERT(consumer_, opcode == SpvOpFunctionParameter);
+        if (opcode != SpvOpFunctionParameter) {
+          Errorf(consumer_, src, loc,
+                 "Non-OpFunctionParameter (opcode: %d) found inside "
+                 "function but outside basic block",
+                 opcode);
+          return false;
+        }
         function_->AddParameter(std::move(spv_inst));
       } else {
         block_->AddInstruction(std::move(spv_inst));
       }
     }
   }
+  return true;
 }
 
 // Resolves internal references among the module, functions, basic blocks, etc.

diff --git a/source/opt/ir_loader.h b/source/opt/ir_loader.h
index ce971d0..c5c7359 100644 (file)
--- a/source/opt/ir_loader.h
+++ b/source/opt/ir_loader.h
@@ -40,18 +40,20 @@ class IrLoader {
   // All internal messages will be communicated to the outside via the given
   // message |consumer|. This instance only keeps a reference to the |consumer|,
   // so the |consumer| should outlive this instance.
-  IrLoader(const MessageConsumer& consumer, Module* module)
-      : consumer_(consumer), module_(module) {}
+  IrLoader(const MessageConsumer& consumer, Module* module);
+
+  // Sets the source name of the module.
+  void SetSource(const std::string& src) { source_ = src; }
 
   // Sets the fields in the module's header to the given parameters.
   void SetModuleHeader(uint32_t magic, uint32_t version, uint32_t generator,
                        uint32_t bound, uint32_t reserved) {
     module_->SetHeader({magic, version, generator, bound, reserved});
   }
-  // Adds an instruction to the module. This method will properly capture and
-  // store the data provided in |inst| so that |inst| is no longer needed after
-  // returning.
-  void AddInstruction(const spv_parsed_instruction_t* inst);
+  // Adds an instruction to the module. Returns true if no error occurs. This
+  // method will properly capture and store the data provided in |inst| so that
+  // |inst| is no longer needed after returning.
+  bool AddInstruction(const spv_parsed_instruction_t* inst);
   // Finalizes the module construction. This must be called after the module
   // header has been set and all instructions have been added.  This is
   // forgiving in the case of a missing terminator instruction on a basic block,
@@ -63,6 +65,10 @@ class IrLoader {
   const MessageConsumer& consumer_;
   // The module to be built.
   Module* module_;
+  // The source name of the module.
+  std::string source_;
+  // The last used instruction index.
+  uint32_t inst_index_;
   // The current Function under construction.
   std::unique_ptr<Function> function_;
   // The current BasicBlock under construction.

diff --git a/test/opt/test_ir_loader.cpp b/test/opt/test_ir_loader.cpp
index 11fe019..87e4fb4 100644 (file)
--- a/test/opt/test_ir_loader.cpp
+++ b/test/opt/test_ir_loader.cpp
@@ -298,4 +298,71 @@ TEST(IrBuilder, KeepLineDebugInfoBeforeFunctionEnd) {
   // clang-format on
 }
 
+// Checks the given |error_message| is reported when trying to build a module
+// from the given |assembly|.
+void DoErrorMessageCheck(const std::string& assembly,
+                         const std::string& error_message) {
+  auto consumer = [error_message](spv_message_level_t level, const char* source,
+                                  const spv_position_t& position,
+                                  const char* m) {
+    EXPECT_EQ(error_message, StringifyMessage(level, source, position, m));
+  };
+
+  SpirvTools t(SPV_ENV_UNIVERSAL_1_1);
+  std::unique_ptr<ir::Module> module =
+      BuildModule(SPV_ENV_UNIVERSAL_1_1, std::move(consumer), assembly);
+  EXPECT_EQ(nullptr, module);
+}
+
+TEST(IrBuilder, FunctionInsideFunction) {
+  DoErrorMessageCheck("%2 = OpFunction %1 None %3\n%5 = OpFunction %4 None %6",
+                      "error: <instruction>:2:0:0: function inside function\n");
+}
+
+TEST(IrBuilder, MismatchOpFunctionEnd) {
+  DoErrorMessageCheck("OpFunctionEnd",
+                      "error: <instruction>:1:0:0: OpFunctionEnd without "
+                      "corresponding OpFunction\n");
+}
+
+TEST(IrBuilder, OpFunctionEndInsideBasicBlock) {
+  DoErrorMessageCheck(
+      "%2 = OpFunction %1 None %3\n"
+      "%4 = OpLabel\n"
+      "OpFunctionEnd",
+      "error: <instruction>:3:0:0: OpFunctionEnd inside basic block\n");
+}
+
+TEST(IrBuilder, BasicBlockOutsideFunction) {
+  DoErrorMessageCheck("OpCapability Shader\n%1 = OpLabel",
+                      "error: <instruction>:2:0:0: OpLabel outside function\n");
+}
+
+TEST(IrBuilder, OpLabelInsideBasicBlock) {
+  DoErrorMessageCheck(
+      "%2 = OpFunction %1 None %3\n"
+      "%4 = OpLabel\n"
+      "%5 = OpLabel",
+      "error: <instruction>:3:0:0: OpLabel inside basic block\n");
+}
+
+TEST(IrBuilder, TerminatorOutsideFunction) {
+  DoErrorMessageCheck(
+      "OpReturn",
+      "error: <instruction>:1:0:0: terminator instruction outside function\n");
+}
+
+TEST(IrBuilder, TerminatorOutsideBasicBlock) {
+  DoErrorMessageCheck("%2 = OpFunction %1 None %3\nOpReturn",
+                      "error: <instruction>:2:0:0: terminator instruction "
+                      "outside basic block\n");
+}
+
+TEST(IrBuilder, NotAllowedInstAppearingInFunction) {
+  DoErrorMessageCheck("%2 = OpFunction %1 None %3\n%5 = OpVariable %4 Function",
+                      "error: <instruction>:2:0:0: Non-OpFunctionParameter "
+                      "(opcode: 59) found inside function but outside basic "
+                      "block\n");
+}
+
 }  // anonymous namespace