From 6effeaa7f1eed3d6856ed5eb928d54d3ac2d29fc Mon Sep 17 00:00:00 2001 From: Lei Zhang Date: Wed, 21 Sep 2016 10:53:15 -0400 Subject: [PATCH] Change some asserts to normal errors in IrLoader. --- source/opt/build_module.cpp | 6 ++-- source/opt/ir_loader.cpp | 61 +++++++++++++++++++++++++++++++++-------- source/opt/ir_loader.h | 18 ++++++++---- test/opt/test_ir_loader.cpp | 67 +++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 133 insertions(+), 19 deletions(-) diff --git a/source/opt/build_module.cpp b/source/opt/build_module.cpp index 2800c09..2699a46 100644 --- 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(builder)->AddInstruction(inst); - return SPV_SUCCESS; + if (reinterpret_cast(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 --- 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_(""), + inst_index_(0) {} + +bool IrLoader::AddInstruction(const spv_parsed_instruction_t* inst) { + ++inst_index_; const auto opcode = static_cast(inst->opcode); if (IsDebugLineInst(opcode)) { dbg_line_info_.push_back(Instruction(*inst)); - return; + return true; } std::unique_ptr 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 --- 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_; // 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 --- 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 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: :2:0:0: function inside function\n"); +} + +TEST(IrBuilder, MismatchOpFunctionEnd) { + DoErrorMessageCheck("OpFunctionEnd", + "error: :1:0:0: OpFunctionEnd without " + "corresponding OpFunction\n"); +} + +TEST(IrBuilder, OpFunctionEndInsideBasicBlock) { + DoErrorMessageCheck( + "%2 = OpFunction %1 None %3\n" + "%4 = OpLabel\n" + "OpFunctionEnd", + "error: :3:0:0: OpFunctionEnd inside basic block\n"); +} + +TEST(IrBuilder, BasicBlockOutsideFunction) { + DoErrorMessageCheck("OpCapability Shader\n%1 = OpLabel", + "error: :2:0:0: OpLabel outside function\n"); +} + +TEST(IrBuilder, OpLabelInsideBasicBlock) { + DoErrorMessageCheck( + "%2 = OpFunction %1 None %3\n" + "%4 = OpLabel\n" + "%5 = OpLabel", + "error: :3:0:0: OpLabel inside basic block\n"); +} + +TEST(IrBuilder, TerminatorOutsideFunction) { + DoErrorMessageCheck( + "OpReturn", + "error: :1:0:0: terminator instruction outside function\n"); +} + +TEST(IrBuilder, TerminatorOutsideBasicBlock) { + DoErrorMessageCheck("%2 = OpFunction %1 None %3\nOpReturn", + "error: :2:0:0: terminator instruction " + "outside basic block\n"); +} + +TEST(IrBuilder, NotAllowedInstAppearingInFunction) { + DoErrorMessageCheck("%2 = OpFunction %1 None %3\n%5 = OpVariable %4 Function", + "error: :2:0:0: Non-OpFunctionParameter " + "(opcode: 59) found inside function but outside basic " + "block\n"); +} + } // anonymous namespace -- 2.7.4