spirv-val: Add better error code for invalid operand (#4753)

source/assembly_grammar.cpp

@@ -62,9 +62,9 @@ spv_result_t spvTextParseMaskOperand(spv_target_env env,
     end = std::find(begin, text_end, separator);
 
     spv_operand_desc entry = nullptr;
-    if (spvOperandTableNameLookup(env, operandTable, type, begin, end - begin,
-                                  &entry)) {
-      return SPV_ERROR_INVALID_TEXT;
+    if (auto error = spvOperandTableNameLookup(env, operandTable, type, begin,
+                                               end - begin, &entry)) {
+      return error;
     }
     value |= entry->value;
 

source/operand.cpp

@@ -72,12 +72,16 @@ spv_result_t spvOperandTableNameLookup(spv_target_env env,
       // Note that the second rule assumes the extension enabling this operand
       // is indeed requested in the SPIR-V code; checking that should be
       // validator's work.
-      if (((version >= entry.minVersion && version <= entry.lastVersion) ||
-           entry.numExtensions > 0u || entry.numCapabilities > 0u) &&
-          nameLength == strlen(entry.name) &&
+      if (nameLength == strlen(entry.name) &&
           !strncmp(entry.name, name, nameLength)) {
+        if ((version >= entry.minVersion && version <= entry.lastVersion) ||
+            entry.numExtensions > 0u || entry.numCapabilities > 0u) {
           *pEntry = &entry;
           return SPV_SUCCESS;
+        } else {
+          // if there is no extension/capability then the version is wrong
+          return SPV_ERROR_WRONG_VERSION;
+        }
       }
     }
   }

source/text.cpp

@@ -403,9 +403,10 @@ spv_result_t spvTextEncodeOperand(const spvtools::AssemblyGrammar& grammar,
     case SPV_OPERAND_TYPE_DEBUG_INFO_FLAGS:
     case SPV_OPERAND_TYPE_CLDEBUG100_DEBUG_INFO_FLAGS: {
       uint32_t value;
-      if (grammar.parseMaskOperand(type, textValue, &value)) {
-        return context->diagnostic() << "Invalid " << spvOperandTypeStr(type)
-                                     << " operand '" << textValue << "'.";
+      if (auto error = grammar.parseMaskOperand(type, textValue, &value)) {
+        return context->diagnostic(error)
+               << "Invalid " << spvOperandTypeStr(type) << " operand '"
+               << textValue << "'.";
       }
       if (auto error = context->binaryEncodeU32(value, pInst)) return error;
       // Prepare to parse the operands for this logical operand.
@@ -769,8 +770,8 @@ spv_result_t spvTextToBinaryInternal(const spvtools::AssemblyGrammar& grammar,
     instructions.push_back({});
     spv_instruction_t& inst = instructions.back();
 
-    if (spvTextEncodeOpcode(grammar, &context, &inst)) {
-      return SPV_ERROR_INVALID_TEXT;
+    if (auto error = spvTextEncodeOpcode(grammar, &context, &inst)) {
+      return error;
     }
 
     if (context.advance()) break;

test/val/val_fixtures.h

@@ -40,8 +40,10 @@ class ValidateBase : public ::testing::Test,
 
   // Assembles the given SPIR-V text, checks that it fails to assemble,
   // and returns resulting diagnostic.  No internal state is updated.
+  // Setting the desired_result to SPV_SUCCESS is used to allow all results
   std::string CompileFailure(std::string code,
-                             spv_target_env env = SPV_ENV_UNIVERSAL_1_0);
+                             spv_target_env env = SPV_ENV_UNIVERSAL_1_0,
+                             spv_result_t desired_result = SPV_SUCCESS);
 
   // Checks that 'code' is valid SPIR-V text representation and stores the
   // binary version for further method calls.
@@ -108,11 +110,17 @@ void ValidateBase<T>::TearDown() {
 
 template <typename T>
 std::string ValidateBase<T>::CompileFailure(std::string code,
-                                            spv_target_env env) {
+                                            spv_target_env env,
+                                            spv_result_t desired_result) {
   spv_diagnostic diagnostic = nullptr;
-  EXPECT_NE(SPV_SUCCESS,
-            spvTextToBinary(ScopedContext(env).context, code.c_str(),
-                            code.size(), &binary_, &diagnostic));
+  spv_result_t actual_result =
+      spvTextToBinary(ScopedContext(env).context, code.c_str(), code.size(),
+                      &binary_, &diagnostic);
+  EXPECT_NE(SPV_SUCCESS, actual_result);
+  // optional check for exact result
+  if (desired_result != SPV_SUCCESS) {
+    EXPECT_EQ(actual_result, desired_result);
+  }
   std::string result(diagnostic->error);
   spvDiagnosticDestroy(diagnostic);
   return result;

test/val/val_image_test.cpp

@@ -5540,7 +5540,8 @@ TEST_F(ValidateImage, SignExtendV13Bad) {
 )";
 
   EXPECT_THAT(CompileFailure(GenerateShaderCode(body, "", "Fragment", "",
-                                                SPV_ENV_UNIVERSAL_1_3)),
+                                                SPV_ENV_UNIVERSAL_1_3),
+                             SPV_ENV_UNIVERSAL_1_3, SPV_ERROR_WRONG_VERSION),
               HasSubstr("Invalid image operand 'SignExtend'"));
 }
 
@@ -5551,7 +5552,8 @@ TEST_F(ValidateImage, ZeroExtendV13Bad) {
 )";
 
   EXPECT_THAT(CompileFailure(GenerateShaderCode(body, "", "Fragment", "",
-                                                SPV_ENV_UNIVERSAL_1_3)),
+                                                SPV_ENV_UNIVERSAL_1_3),
+                             SPV_ENV_UNIVERSAL_1_3, SPV_ERROR_WRONG_VERSION),
               HasSubstr("Invalid image operand 'ZeroExtend'"));
 }