commit 9766b22b33ef0b8af8ce4d8c5e78d4397a2d2ca9
author JasperNV <46733637+JasperNV@users.noreply.github.com> Fri Apr 05 10:36:42 2019 -0700
committer Steven Perron <stevenperron@google.com> Fri Apr 05 13:36:42 2019 -0400
tree 5269f68e78d95989a59305922d8975f143c02948
parent 3a0bc9e72495f3727c402064a1d9f70dcc33c9fe

spirv-opt: Behave a bit better in the face of unknown instructions (#2487)

* opt/ir_loader: Don't silently drop unknown instructions on the floor

Currently, if spirv-opt sees an instruction it does not know, it will
silently ignore it and move to the next one. This changes it
to be an error, as dropping it on the floor is likely to generate
invalid SPIR-V output.

* opt/optimizer: Complain a bit louder for unexpected binary changes

If a binary change happens despite a pass saying that the binaries
should be identical, this is indicative of a bug in the pass itself.

This does not change behavior for it to be an error, but simply emits a warning in this case.

source/opt/ir_loader.cpp

diff --git a/source/opt/ir_loader.cpp b/source/opt/ir_loader.cpp
index 46e2bee..edd4832 100644
--- a/source/opt/ir_loader.cpp
+++ b/source/opt/ir_loader.cpp
@@ -116,8 +116,10 @@
                  opcode == SpvOpUndef) {
         module_->AddGlobalValue(std::move(spv_inst));
       } else {
-        SPIRV_UNIMPLEMENTED(consumer_,
-                            "unhandled inst type outside function definition");
+        Errorf(consumer_, src, loc,
+               "Unhandled inst type (opcode: %d) found outside function definition.",
+               opcode);
+        return false;
       }
     } else {
       if (block_ == nullptr) {  // Inside function but outside blocks

source/opt/optimizer.cpp

diff --git a/source/opt/optimizer.cpp b/source/opt/optimizer.cpp
index b9e73ab..c17c05f 100644
--- a/source/opt/optimizer.cpp
+++ b/source/opt/optimizer.cpp
@@ -518,10 +518,20 @@
   impl_->pass_manager.SetValidatorOptions(&opt_options->val_options_);
   impl_->pass_manager.SetTargetEnv(impl_->target_env);
   auto status = impl_->pass_manager.Run(context.get());
-  if (status == opt::Pass::Status::SuccessWithChange ||
-      (status == opt::Pass::Status::SuccessWithoutChange &&
-       (optimized_binary->data() != original_binary ||
-        optimized_binary->size() != original_binary_size))) {
+
+  bool binary_changed = false;
+  if (status == opt::Pass::Status::SuccessWithChange) {
+    binary_changed = true;
+  } else if (status == opt::Pass::Status::SuccessWithoutChange) {
+    if (optimized_binary->size() != original_binary_size ||
+        (memcmp(optimized_binary->data(), original_binary, original_binary_size) != 0)) {
+      binary_changed = true;
+      Logf(consumer(), SPV_MSG_WARNING, nullptr, {},
+           "Binary unexpectedly changed despite optimizer saying there was no change");
+    }
+  }
+
+  if (binary_changed) {
     optimized_binary->clear();
     context->module()->ToBinary(optimized_binary, /* skip_nop = */ true);
   }