From 8654caa5657ef0e12367d9e705e6c31e70f68668 Mon Sep 17 00:00:00 2001 From: Lei Zhang Date: Fri, 9 Sep 2016 10:46:23 -0400 Subject: [PATCH] Prepare the C++ interface for publication. * Use PIMPL idiom in the C++ interface. * Clean up interface for assembling and disassembling. * Add validation into C++ interface. * Add more tests for the C++ interface. --- source/opt/build_module.cpp | 2 +- source/opt/libspirv.cpp | 52 +++++++++++-------- source/opt/libspirv.hpp | 63 +++++++++++++++-------- test/cpp_interface.cpp | 118 +++++++++++++++++++++++++++++++++++++++++--- test/opt/pass_fixture.h | 4 +- test/opt/test_ir_loader.cpp | 4 +- 6 files changed, 191 insertions(+), 52 deletions(-) diff --git a/source/opt/build_module.cpp b/source/opt/build_module.cpp index 5686275..d42c7f8 100644 --- a/source/opt/build_module.cpp +++ b/source/opt/build_module.cpp @@ -67,7 +67,7 @@ std::unique_ptr BuildModule(spv_target_env env, SpvTools t(env); t.SetMessageConsumer(consumer); std::vector binary; - if (t.Assemble(text, &binary) != SPV_SUCCESS) return nullptr; + if (!t.Assemble(text, &binary)) return nullptr; return BuildModule(env, consumer, binary); } diff --git a/source/opt/libspirv.cpp b/source/opt/libspirv.cpp index 2b7850b..b3f89f1 100644 --- a/source/opt/libspirv.cpp +++ b/source/opt/libspirv.cpp @@ -16,46 +16,58 @@ #include "ir_loader.h" #include "make_unique.h" +#include "message.h" #include "table.h" namespace spvtools { +// Structs for holding the data members for SpvTools. +struct SpvTools::Impl { + explicit Impl(spv_target_env env) : context(spvContextCreate(env)) { + // The default consumer in spv_context_t is a null consumer, which provides + // equivalent functionality (from the user's perspective) as a real consumer + // does nothing. + } + ~Impl() { spvContextDestroy(context); } + + spv_context context; // C interface context object. +}; + +SpvTools::SpvTools(spv_target_env env) : impl_(new Impl(env)) {} + +SpvTools::~SpvTools() {} + void SpvTools::SetMessageConsumer(MessageConsumer consumer) { - SetContextMessageConsumer(context_, std::move(consumer)); + SetContextMessageConsumer(impl_->context, std::move(consumer)); } -spv_result_t SpvTools::Assemble(const std::string& text, - std::vector* binary) { +bool SpvTools::Assemble(const std::string& text, + std::vector* binary) const { spv_binary spvbinary = nullptr; - spv_diagnostic diagnostic = nullptr; - - spv_result_t status = spvTextToBinary(context_, text.data(), text.size(), - &spvbinary, &diagnostic); + spv_result_t status = spvTextToBinary(impl_->context, text.data(), + text.size(), &spvbinary, nullptr); if (status == SPV_SUCCESS) { binary->assign(spvbinary->code, spvbinary->code + spvbinary->wordCount); } - - spvDiagnosticDestroy(diagnostic); spvBinaryDestroy(spvbinary); - - return status; + return status == SPV_SUCCESS; } -spv_result_t SpvTools::Disassemble(const std::vector& binary, - std::string* text, uint32_t options) { +bool SpvTools::Disassemble(const std::vector& binary, + std::string* text, uint32_t options) const { spv_text spvtext = nullptr; - spv_diagnostic diagnostic = nullptr; - - spv_result_t status = spvBinaryToText(context_, binary.data(), binary.size(), - options, &spvtext, &diagnostic); + spv_result_t status = spvBinaryToText( + impl_->context, binary.data(), binary.size(), options, &spvtext, nullptr); if (status == SPV_SUCCESS) { text->assign(spvtext->str, spvtext->str + spvtext->length); } - - spvDiagnosticDestroy(diagnostic); spvTextDestroy(spvtext); + return status == SPV_SUCCESS; +} - return status; +bool SpvTools::Validate(const std::vector& binary) const { + spv_const_binary_t b = {binary.data(), binary.size()}; + return spvValidate(impl_->context, &b, nullptr) == SPV_SUCCESS; } } // namespace spvtools diff --git a/source/opt/libspirv.hpp b/source/opt/libspirv.hpp index fed408a..63dde2f 100644 --- a/source/opt/libspirv.hpp +++ b/source/opt/libspirv.hpp @@ -15,6 +15,7 @@ #ifndef SPIRV_TOOLS_LIBSPIRV_HPP_ #define SPIRV_TOOLS_LIBSPIRV_HPP_ +#include #include #include @@ -25,36 +26,58 @@ namespace spvtools { // C++ interface for SPIRV-Tools functionalities. It wraps the context // (including target environment and the corresponding SPIR-V grammar) and -// provides methods for assembling, disassembling, validating, and optimizing. +// provides methods for assembling, disassembling, and validating. // -// Instances of this class are thread-safe. +// Instances of this class provide basic thread-safety guarantee. class SpvTools { public: - // Creates an instance targeting the given environment |env|. - SpvTools(spv_target_env env) : context_(spvContextCreate(env)) {} + enum { + // Default disassembling option used by Disassemble(): + // * Avoid prefix comments from decoding the SPIR-V module header, and + // * Use friendly names for variables. + kDefaultDisassembleOption = SPV_BINARY_TO_TEXT_OPTION_NO_HEADER | + SPV_BINARY_TO_TEXT_OPTION_FRIENDLY_NAMES + }; - ~SpvTools() { spvContextDestroy(context_); } + // Constructs an instance targeting the given environment |env|. + // + // The constructed instance will have an empty message consumer, which just + // ignores all messages from the library. Use SetMessageConsumer() to supply + // one if messages are of concern. + explicit SpvTools(spv_target_env env); - // Sets the message consumer to the given |consumer|. + // Disables copy/move constructor/assignment operations. + SpvTools(const SpvTools&) = delete; + SpvTools(SpvTools&&) = delete; + SpvTools& operator=(const SpvTools&) = delete; + SpvTools& operator=(SpvTools&&) = delete; + + // Destructs this instance. + ~SpvTools(); + + // Sets the message consumer to the given |consumer|. The |consumer| will be + // invoked once for each message communicated from the library. void SetMessageConsumer(MessageConsumer consumer); // Assembles the given assembly |text| and writes the result to |binary|. - // Returns SPV_SUCCESS on successful assembling. - spv_result_t Assemble(const std::string& text, std::vector* binary); - - // Disassembles the given SPIR-V |binary| with the given options and returns - // the assembly. By default the options are set to generate assembly with - // friendly variable names and no SPIR-V assembly header. Returns SPV_SUCCESS - // on successful disassembling. - spv_result_t Disassemble( - const std::vector& binary, std::string* text, - uint32_t options = SPV_BINARY_TO_TEXT_OPTION_NO_HEADER | - SPV_BINARY_TO_TEXT_OPTION_FRIENDLY_NAMES); + // Returns true on successful assembling. |binary| will be kept untouched if + // assembling is unsuccessful. + bool Assemble(const std::string& text, std::vector* binary) const; + + // Disassembles the given SPIR-V |binary| with the given |options| and writes + // the assembly to |text|. Returns ture on successful disassembling. |text| + // will be kept untouched if diassembling is unsuccessful. + bool Disassemble(const std::vector& binary, std::string* text, + uint32_t options = kDefaultDisassembleOption) const; + + // Validates the given SPIR-V |binary|. Returns true if no issues are found. + // Otherwise, returns false and communicates issues via the message consumer + // registered. + bool Validate(const std::vector& binary) const; private: - // Context for the current invocation. Thread-safety of this class depends on - // the constness of this field. - spv_context context_; + struct Impl; // Opaque struct for holding the data fields used by this class. + std::unique_ptr impl_; // Unique pointer to implementation data. }; } // namespace spvtools diff --git a/test/cpp_interface.cpp b/test/cpp_interface.cpp index 104958d..a585951 100644 --- a/test/cpp_interface.cpp +++ b/test/cpp_interface.cpp @@ -12,6 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. +#include #include #include "opt/libspirv.hpp" @@ -20,40 +21,143 @@ namespace { using namespace spvtools; +using ::testing::ContainerEq; TEST(CppInterface, SuccessfulRoundTrip) { const std::string input_text = "%2 = OpSizeOf %1 %3\n"; SpvTools t(SPV_ENV_UNIVERSAL_1_1); std::vector binary; - EXPECT_EQ(SPV_SUCCESS, t.Assemble(input_text, &binary)); + EXPECT_TRUE(t.Assemble(input_text, &binary)); EXPECT_TRUE(binary.size() > 5u); EXPECT_EQ(SpvMagicNumber, binary[0]); EXPECT_EQ(SpvVersion, binary[1]); + // This cannot pass validation since %1 is not defined. + t.SetMessageConsumer([](MessageLevel level, const char* source, + const spv_position_t& position, const char* message) { + EXPECT_EQ(MessageLevel::Error, level); + EXPECT_STREQ("", source); + EXPECT_EQ(0u, position.line); + EXPECT_EQ(0u, position.column); + EXPECT_EQ(1u, position.index); + EXPECT_STREQ("ID 1 has not been defined", message); + }); + EXPECT_FALSE(t.Validate(binary)); + std::string output_text; - EXPECT_EQ(SPV_SUCCESS, t.Disassemble(binary, &output_text)); + EXPECT_TRUE(t.Disassemble(binary, &output_text)); EXPECT_EQ(input_text, output_text); } +TEST(CppInterface, AssembleEmptyModule) { + std::vector binary(10, 42); + SpvTools t(SPV_ENV_UNIVERSAL_1_1); + EXPECT_TRUE(t.Assemble("", &binary)); + // We only have the header. + EXPECT_EQ(5u, binary.size()); + EXPECT_EQ(SpvMagicNumber, binary[0]); + EXPECT_EQ(SpvVersion, binary[1]); +} + TEST(CppInterface, AssembleWithWrongTargetEnv) { const std::string input_text = "%r = OpSizeOf %type %pointer"; SpvTools t(SPV_ENV_UNIVERSAL_1_0); + int invocation_count = 0; + t.SetMessageConsumer( + [&invocation_count](MessageLevel level, const char* source, + const spv_position_t& position, const char* message) { + ++invocation_count; + EXPECT_EQ(MessageLevel::Error, level); + EXPECT_STREQ("", source); + EXPECT_EQ(0u, position.line); + EXPECT_EQ(5u, position.column); + EXPECT_EQ(5u, position.index); + EXPECT_STREQ("Invalid Opcode name 'OpSizeOf'", message); + }); - std::vector binary; - EXPECT_EQ(SPV_ERROR_INVALID_TEXT, t.Assemble(input_text, &binary)); + std::vector binary = {42, 42}; + EXPECT_FALSE(t.Assemble(input_text, &binary)); + EXPECT_THAT(binary, ContainerEq(std::vector{42, 42})); + EXPECT_EQ(1, invocation_count); +} + +TEST(CppInterface, DisassembleEmptyModule) { + std::string text(10, 'x'); + SpvTools t(SPV_ENV_UNIVERSAL_1_1); + int invocation_count = 0; + t.SetMessageConsumer( + [&invocation_count](MessageLevel level, const char* source, + const spv_position_t& position, const char* message) { + ++invocation_count; + EXPECT_EQ(MessageLevel::Error, level); + EXPECT_STREQ("", source); + EXPECT_EQ(0u, position.line); + EXPECT_EQ(0u, position.column); + EXPECT_EQ(0u, position.index); + EXPECT_STREQ("Missing module.", message); + }); + EXPECT_FALSE(t.Disassemble({}, &text)); + EXPECT_EQ("xxxxxxxxxx", text); // The original string is unmodified. + EXPECT_EQ(1, invocation_count); } TEST(CppInterface, DisassembleWithWrongTargetEnv) { const std::string input_text = "%r = OpSizeOf %type %pointer"; SpvTools t11(SPV_ENV_UNIVERSAL_1_1); SpvTools t10(SPV_ENV_UNIVERSAL_1_0); + int invocation_count = 0; + t10.SetMessageConsumer( + [&invocation_count](MessageLevel level, const char* source, + const spv_position_t& position, const char* message) { + ++invocation_count; + EXPECT_EQ(MessageLevel::Error, level); + EXPECT_STREQ("", source); + EXPECT_EQ(0u, position.line); + EXPECT_EQ(0u, position.column); + EXPECT_EQ(5u, position.index); + EXPECT_STREQ("Invalid opcode: 321", message); + }); std::vector binary; - EXPECT_EQ(SPV_SUCCESS, t11.Assemble(input_text, &binary)); + EXPECT_TRUE(t11.Assemble(input_text, &binary)); - std::string output_text; - EXPECT_EQ(SPV_ERROR_INVALID_BINARY, t10.Disassemble(binary, &output_text)); + std::string output_text(10, 'x'); + EXPECT_FALSE(t10.Disassemble(binary, &output_text)); + EXPECT_EQ("xxxxxxxxxx", output_text); // The original string is unmodified. +} + +TEST(CppInterface, SuccessfulValidation) { + const std::string input_text = + "OpCapability Shader\nOpMemoryModel Logical GLSL450"; + SpvTools t(SPV_ENV_UNIVERSAL_1_1); + int invocation_count = 0; + t.SetMessageConsumer( + [&invocation_count](MessageLevel, const char*, const spv_position_t&, + const char*) { ++invocation_count; }); + + std::vector binary; + EXPECT_TRUE(t.Assemble(input_text, &binary)); + EXPECT_TRUE(t.Validate(binary)); + EXPECT_EQ(0, invocation_count); +} + +TEST(CppInterface, ValidateEmptyModule) { + SpvTools t(SPV_ENV_UNIVERSAL_1_1); + int invocation_count = 0; + t.SetMessageConsumer( + [&invocation_count](MessageLevel level, const char* source, + const spv_position_t& position, const char* message) { + ++invocation_count; + EXPECT_EQ(MessageLevel::Error, level); + EXPECT_STREQ("", source); + EXPECT_EQ(0u, position.line); + EXPECT_EQ(0u, position.column); + EXPECT_EQ(0u, position.index); + EXPECT_STREQ("Invalid SPIR-V magic number.", message); + }); + EXPECT_FALSE(t.Validate({})); + EXPECT_EQ(1, invocation_count); } } // anonymous namespace diff --git a/test/opt/pass_fixture.h b/test/opt/pass_fixture.h index df41ad5..9cc831c 100644 --- a/test/opt/pass_fixture.h +++ b/test/opt/pass_fixture.h @@ -63,7 +63,7 @@ class PassTest : public TestT { std::vector binary; module->ToBinary(&binary, skip_nop); std::string optimized; - EXPECT_EQ(SPV_SUCCESS, tools_.Disassemble(binary, &optimized)) + EXPECT_TRUE(tools_.Disassemble(binary, &optimized)) << "Disassembling failed for shader:\n" << original << std::endl; return std::make_tuple(optimized, modified); @@ -122,7 +122,7 @@ class PassTest : public TestT { module->ToBinary(&binary, /* skip_nop = */ false); std::string optimized; - EXPECT_EQ(SPV_SUCCESS, tools_.Disassemble(binary, &optimized)); + EXPECT_TRUE(tools_.Disassemble(binary, &optimized)); EXPECT_EQ(expected, optimized); } diff --git a/test/opt/test_ir_loader.cpp b/test/opt/test_ir_loader.cpp index 27860c7..b989380 100644 --- a/test/opt/test_ir_loader.cpp +++ b/test/opt/test_ir_loader.cpp @@ -32,7 +32,7 @@ void DoRoundTripCheck(const std::string& text) { module->ToBinary(&binary, /* skip_nop = */ false); std::string disassembled_text; - EXPECT_EQ(SPV_SUCCESS, t.Disassemble(binary, &disassembled_text)); + EXPECT_TRUE(t.Disassemble(binary, &disassembled_text)); EXPECT_EQ(text, disassembled_text); } @@ -224,7 +224,7 @@ TEST(IrBuilder, OpUndefOutsideFunction) { module->ToBinary(&binary, /* skip_nop = */ false); std::string disassembled_text; - EXPECT_EQ(SPV_SUCCESS, t.Disassemble(binary, &disassembled_text)); + EXPECT_TRUE(t.Disassemble(binary, &disassembled_text)); EXPECT_EQ(text, disassembled_text); } -- 2.7.4