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

@@ -67,7 +67,7 @@ std::unique_ptr<ir::Module> BuildModule(spv_target_env env,
   SpvTools t(env);
   t.SetMessageConsumer(consumer);
   std::vector<uint32_t> binary;
-  if (t.Assemble(text, &binary) != SPV_SUCCESS) return nullptr;
+  if (!t.Assemble(text, &binary)) return nullptr;
   return BuildModule(env, consumer, binary);
 }
 

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<uint32_t>* binary) {
+bool SpvTools::Assemble(const std::string& text,
+                        std::vector<uint32_t>* 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<uint32_t>& binary,
-                                   std::string* text, uint32_t options) {
+bool SpvTools::Disassemble(const std::vector<uint32_t>& 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<uint32_t>& binary) const {
+  spv_const_binary_t b = {binary.data(), binary.size()};
+  return spvValidate(impl_->context, &b, nullptr) == SPV_SUCCESS;
 }
 
 }  // namespace spvtools

source/opt/libspirv.hpp

@@ -15,6 +15,7 @@
 #ifndef SPIRV_TOOLS_LIBSPIRV_HPP_
 #define SPIRV_TOOLS_LIBSPIRV_HPP_
 
+#include <memory>
 #include <string>
 #include <vector>
 
@@ -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<uint32_t>* binary);
+  // Returns true on successful assembling. |binary| will be kept untouched if
+  // assembling is unsuccessful.
+  bool Assemble(const std::string& text, std::vector<uint32_t>* binary) const;
 
-  // 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<uint32_t>& binary, std::string* text,
-      uint32_t options = SPV_BINARY_TO_TEXT_OPTION_NO_HEADER |
-                         SPV_BINARY_TO_TEXT_OPTION_FRIENDLY_NAMES);
+  // 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<uint32_t>& 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<uint32_t>& 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> impl_;  // Unique pointer to implementation data.
 };
 
 }  // namespace spvtools

test/cpp_interface.cpp

@@ -12,6 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
+#include <gmock/gmock.h>
 #include <gtest/gtest.h>
 
 #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<uint32_t> 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<uint32_t> 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<uint32_t> binary;
-  EXPECT_EQ(SPV_ERROR_INVALID_TEXT, t.Assemble(input_text, &binary));
+  std::vector<uint32_t> binary = {42, 42};
+  EXPECT_FALSE(t.Assemble(input_text, &binary));
+  EXPECT_THAT(binary, ContainerEq(std::vector<uint32_t>{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<uint32_t> 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<uint32_t> 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

test/opt/pass_fixture.h

@@ -63,7 +63,7 @@ class PassTest : public TestT {
     std::vector<uint32_t> 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);
   }
 

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);
 }