Avoid operand type range checks (#3379)

* Avoid operand type range checks

Deprecates the SPV_OPERAND_TYPE_FIRST_* and SPV_OPERAND_TYPE_LAST_*
macros.

The "variable" and "optional" operand types are only for internal use.
Export spvOperandIsConcrete instead, as that should cover intended
external uses.

Test that each operand type is classified either as one of:
- a sentinel value
- a concrete operand type
- an optional operand type (which includes variable-expansion types)

Test that each concrete and optional non-variable operand type
has a name for use internally when generating messages.

Co-authored-by: Steven Perron <stevenperron@google.com>

include/spirv-tools/libspirv.h

@@ -48,8 +48,8 @@ extern "C" {
 
 #define SPV_BIT(shift) (1 << (shift))
 
-#define SPV_FORCE_16_BIT_ENUM(name) _##name = 0x7fff
-#define SPV_FORCE_32_BIT_ENUM(name) _##name = 0x7fffffff
+#define SPV_FORCE_16_BIT_ENUM(name) SPV_FORCE_16BIT_##name = 0x7fff
+#define SPV_FORCE_32_BIT_ENUM(name) SPV_FORCE_32BIT_##name = 0x7fffffff
 
 // Enumerations
 
@@ -189,8 +189,17 @@ typedef enum spv_operand_type_t {
 //    Variable : expands to 0, 1 or many operands or pairs of operands.
 //               This is similar to * in regular expressions.
 
+// NOTE: These FIRST_* and LAST_* enum values are DEPRECATED.
+// The concept of "optional" and "variable" operand types are only intended
+// for use as an implementation detail of parsing SPIR-V, either in text or
+// binary form.  Instead of using enum ranges, use characteristic function
+// spvOperandIsConcrete.
+// The use of enum value ranges in a public API makes it difficult to insert
+// new values into a range without also breaking binary compatibility.
+//
 // Macros for defining bounds on optional and variable operand types.
 // Any variable operand type is also optional.
+// TODO(dneto): Remove SPV_OPERAND_TYPE_FIRST_* and SPV_OPERAND_TYPE_LAST_*
 #define FIRST_OPTIONAL(ENUM) ENUM, SPV_OPERAND_TYPE_FIRST_OPTIONAL_TYPE = ENUM
 #define FIRST_VARIABLE(ENUM) ENUM, SPV_OPERAND_TYPE_FIRST_VARIABLE_TYPE = ENUM
 #define LAST_VARIABLE(ENUM)                         \
@@ -257,6 +266,12 @@ typedef enum spv_operand_type_t {
   SPV_FORCE_32_BIT_ENUM(spv_operand_type_t)
 } spv_operand_type_t;
 
+// Returns true if the given type is concrete.
+bool spvOperandIsConcrete(spv_operand_type_t type);
+
+// Returns true if the given type is concrete and also a mask.
+bool spvOperandIsConcreteMask(spv_operand_type_t type);
+
 typedef enum spv_ext_inst_type_t {
   SPV_EXT_INST_TYPE_NONE = 0,
   SPV_EXT_INST_TYPE_GLSL_STD_450,

source/operand.cpp

@@ -265,7 +265,6 @@ const char* spvOperandTypeStr(spv_operand_type_t type) {
     case SPV_OPERAND_TYPE_NONE:
       return "NONE";
     default:
-      assert(0 && "Unhandled operand type!");
       break;
   }
   return "unknown";
@@ -371,13 +370,35 @@ bool spvOperandIsConcreteMask(spv_operand_type_t type) {
 }
 
 bool spvOperandIsOptional(spv_operand_type_t type) {
-  return SPV_OPERAND_TYPE_FIRST_OPTIONAL_TYPE <= type &&
-         type <= SPV_OPERAND_TYPE_LAST_OPTIONAL_TYPE;
+  switch (type) {
+    case SPV_OPERAND_TYPE_OPTIONAL_ID:
+    case SPV_OPERAND_TYPE_OPTIONAL_IMAGE:
+    case SPV_OPERAND_TYPE_OPTIONAL_MEMORY_ACCESS:
+    case SPV_OPERAND_TYPE_OPTIONAL_LITERAL_INTEGER:
+    case SPV_OPERAND_TYPE_OPTIONAL_LITERAL_NUMBER:
+    case SPV_OPERAND_TYPE_OPTIONAL_TYPED_LITERAL_INTEGER:
+    case SPV_OPERAND_TYPE_OPTIONAL_LITERAL_STRING:
+    case SPV_OPERAND_TYPE_OPTIONAL_ACCESS_QUALIFIER:
+    case SPV_OPERAND_TYPE_OPTIONAL_CIV:
+      return true;
+    default:
+      break;
+  }
+  // Any variable operand is also optional.
+  return spvOperandIsVariable(type);
 }
 
 bool spvOperandIsVariable(spv_operand_type_t type) {
-  return SPV_OPERAND_TYPE_FIRST_VARIABLE_TYPE <= type &&
-         type <= SPV_OPERAND_TYPE_LAST_VARIABLE_TYPE;
+  switch (type) {
+    case SPV_OPERAND_TYPE_VARIABLE_ID:
+    case SPV_OPERAND_TYPE_VARIABLE_LITERAL_INTEGER:
+    case SPV_OPERAND_TYPE_VARIABLE_LITERAL_INTEGER_ID:
+    case SPV_OPERAND_TYPE_VARIABLE_ID_LITERAL_INTEGER:
+      return true;
+    default:
+      break;
+  }
+  return false;
 }
 
 bool spvExpandOperandSequenceOnce(spv_operand_type_t type,

test/operand_test.cpp

@@ -42,9 +42,16 @@ TEST(OperandString, AllAreDefinedExceptVariable) {
   // None has no string, so don't test it.
   EXPECT_EQ(0u, SPV_OPERAND_TYPE_NONE);
   // Start testing at enum with value 1, skipping None.
-  for (int i = 1; i < int(SPV_OPERAND_TYPE_FIRST_VARIABLE_TYPE); i++) {
-    EXPECT_NE(nullptr, spvOperandTypeStr(static_cast<spv_operand_type_t>(i)))
-        << " Operand type " << i;
+  for (int i = 1; i < int(SPV_OPERAND_TYPE_NUM_OPERAND_TYPES); i++) {
+    const auto type = static_cast<spv_operand_type_t>(i);
+    if (spvOperandIsVariable(type)) {
+      EXPECT_STREQ("unknown", spvOperandTypeStr(type))
+          << " variable type " << i << " has a name '"
+          << spvOperandTypeStr(type) << "'when it should not";
+    } else {
+      EXPECT_STRNE("unknown", spvOperandTypeStr(type))
+          << " operand type " << i << " has no name when it should";
+    }
   }
 }
 
@@ -71,5 +78,46 @@ TEST(OperandIsConcreteMask, Sample) {
       spvOperandIsConcreteMask(SPV_OPERAND_TYPE_OPTIONAL_MEMORY_ACCESS));
 }
 
+TEST(OperandType, NoneTypeClassification) {
+  EXPECT_FALSE(spvOperandIsConcrete(SPV_OPERAND_TYPE_NONE));
+  EXPECT_FALSE(spvOperandIsOptional(SPV_OPERAND_TYPE_NONE));
+  EXPECT_FALSE(spvOperandIsVariable(SPV_OPERAND_TYPE_NONE));
+}
+
+TEST(OperandType, EndSentinelTypeClassification) {
+  EXPECT_FALSE(spvOperandIsConcrete(SPV_OPERAND_TYPE_NUM_OPERAND_TYPES));
+  EXPECT_FALSE(spvOperandIsOptional(SPV_OPERAND_TYPE_NUM_OPERAND_TYPES));
+  EXPECT_FALSE(spvOperandIsVariable(SPV_OPERAND_TYPE_NUM_OPERAND_TYPES));
+}
+
+TEST(OperandType, WidthForcingTypeClassification) {
+  EXPECT_FALSE(spvOperandIsConcrete(SPV_FORCE_32BIT_spv_operand_type_t));
+  EXPECT_FALSE(spvOperandIsOptional(SPV_FORCE_32BIT_spv_operand_type_t));
+  EXPECT_FALSE(spvOperandIsVariable(SPV_FORCE_32BIT_spv_operand_type_t));
+}
+
+TEST(OperandType, EachTypeIsEitherConcreteOrOptionalNotBoth) {
+  EXPECT_EQ(0u, SPV_OPERAND_TYPE_NONE);
+  // Start testing at enum with value 1, skipping None.
+  for (int i = 1; i < int(SPV_OPERAND_TYPE_NUM_OPERAND_TYPES); i++) {
+    const auto type = static_cast<spv_operand_type_t>(i);
+    EXPECT_NE(spvOperandIsConcrete(type), spvOperandIsOptional(type))
+        << " operand type " << int(type) << " concrete? "
+        << int(spvOperandIsConcrete(type)) << " optional? "
+        << int(spvOperandIsOptional(type));
+  }
+}
+
+TEST(OperandType, EachVariableTypeIsOptional) {
+  EXPECT_EQ(0u, SPV_OPERAND_TYPE_NONE);
+  // Start testing at enum with value 1, skipping None.
+  for (int i = 1; i < int(SPV_OPERAND_TYPE_NUM_OPERAND_TYPES); i++) {
+    const auto type = static_cast<spv_operand_type_t>(i);
+    if (spvOperandIsVariable(type)) {
+      EXPECT_TRUE(spvOperandIsOptional(type)) << " variable type " << int(type);
+    }
+  }
+}
+
 }  // namespace
 }  // namespace spvtools