Enforce best practices for guarding enums

tools/buildHeaders/jsonToSpirv.cpp

@@ -36,8 +36,98 @@
 
 #include "jsonToSpirv.h"
 
+namespace {
+// Returns true if the given string is a valid SPIR-V version.
+bool validSpirvVersionString(const std::string s) {
+  return
+  s == "1.0" ||
+  s == "1.1" ||
+  s == "1.2" ||
+  s == "1.3" ||
+  s == "1.4" ||
+  s == "1.5" ||
+  s == "1.6";
+}
+
+// Returns true if the given string is a valid version
+// specifier in the grammar file.
+bool validSpirvVersionStringSpecifier(const std::string s) {
+  return s.empty() || s == "None" || validSpirvVersionString(s);
+}
+
+bool inSomeSpirvVersionCore(const std::string s) {
+  return s.empty() || validSpirvVersionString(s);
+}
+}  // anonymous namespace
+
 namespace spv {
 
+const std::unordered_set<std::string>& PoorlyGuardedEnums() {
+  // Please avoid expanding this list.
+  // Non-capability enums in an extension should be guarded by a capability.
+  // The capability itself should be the only thing enabled by the extension.
+  static std::unordered_set<std::string> table = {
+    "enum Decoration WeightTextureQCOM",
+    "enum Decoration BlockMatchTextureQCOM",
+    "enum Decoration ExplicitInterpAMD",
+    "enum Decoration HlslCounterBufferGOOGLE",
+    "enum Decoration HlslSemanticGOOGLE",
+    "enum Decoration UserTypeGOOGLE",
+    "enum BuiltIn BaryCoordNoPerspAMD",
+    "enum BuiltIn BaryCoordNoPerspCentroidAMD",
+    "enum BuiltIn BaryCoordNoPerspSampleAMD",
+    "enum BuiltIn BaryCoordSmoothAMD",
+    "enum BuiltIn BaryCoordSmoothCentroidAMD",
+    "enum BuiltIn BaryCoordSmoothSampleAMD",
+    "enum BuiltIn BaryCoordPullModelAMD",
+    "enum CooperativeMatrixOperands MatrixASignedComponentsKHR",
+    "enum CooperativeMatrixOperands MatrixBSignedComponentsKHR",
+    "enum CooperativeMatrixOperands MatrixCSignedComponentsKHR",
+    "enum CooperativeMatrixOperands MatrixResultSignedComponentsKHR",
+    "enum CooperativeMatrixOperands SaturatingAccumulationKHR",
+    "enum CooperativeMatrixLayout RowMajorKHR",
+    "enum CooperativeMatrixLayout ColumnMajorKHR",
+    "enum CooperativeMatrixUse MatrixAKHR",
+    "enum CooperativeMatrixUse MatrixBKHR",
+    "enum CooperativeMatrixUse MatrixAccumulatorKHR",
+  };
+  return table;
+}
+
+bool EnumValue::IsValid(OperandClass oc, const std::string& context) const
+{
+  bool result = true;
+  if (!validSpirvVersionStringSpecifier(firstVersion)) {
+    std::cerr << "Error: " << context << " " << name << " \"version\" is invalid: " << firstVersion << std::endl;
+    result = false;
+  }
+  if (!validSpirvVersionStringSpecifier(lastVersion)) {
+    std::cerr << "Error: " << context << " " << name << " \"lastVersion\" is invalid: " << lastVersion << std::endl;
+    result = false;
+  }
+
+  // It must be visible.
+  const bool visible = (firstVersion != "None") || !extensions.empty() || !capabilities.empty();
+  if (!visible) {
+    std::cerr << "Error: " << context << " " << name << " is not visible: "
+              << "its version is set to \"None\", and it is not enabled by a capability or extension" << std::endl;
+    result = false;
+  }
+
+  if (result) {
+    if (oc != OperandCapability && !inSomeSpirvVersionCore(firstVersion) && capabilities.empty()) {
+      // The visibility check required extensions to be non-empty.
+      assert(!extensions.empty());
+      if (PoorlyGuardedEnums().count(context + " " + name) == 0) {
+        std::cerr << "Error: Non-capability " << context << " " << name << " should be guarded by a capability instead of an extension." << std::endl;
+        result = false;
+      }
+    }
+  }
+
+  return result;
+}
+
 // The set of objects that hold all the instruction/operand
 // parameterization information.
 InstructionValues InstructionDesc;
@@ -284,6 +374,8 @@ void jsonToSpirv(const std::string& jsonPath, bool buildingHeaders)
         return;
     initialized = true;
 
+    size_t errorCount = 0;
+
     // Read the JSON grammar file.
     bool fileReadOk = false;
     std::string content;
@@ -398,12 +490,15 @@ void jsonToSpirv(const std::string& jsonPath, bool buildingHeaders)
                                 std::move(caps), std::move(version), std::move(lastVersion), std::move(exts),
                                 std::move(operands))),
              printingClass, defTypeId, defResultId);
+        if (!InstructionDesc.back().IsValid(OperandOpcode, "instruction")) {
+          errorCount++;
+        }
     }
 
     // Specific additional context-dependent operands
 
     // Populate dest with EnumValue objects constructed from source.
-    const auto populateEnumValues = [&getCaps,&getExts](EnumValues* dest, const Json::Value& source, bool bitEnum) {
+    const auto populateEnumValues = [&getCaps,&getExts,&errorCount](EnumValues* dest, const Json::Value& source, bool bitEnum) {
         // A lambda for determining the numeric value to be used for a given
         // enumerant in JSON form, and whether that value is a 0 in a bitfield.
         auto getValue = [&bitEnum](const Json::Value& enumerant) {
@@ -462,12 +557,18 @@ void jsonToSpirv(const std::string& jsonPath, bool buildingHeaders)
         }
     };
 
-    const auto establishOperandClass = [&populateEnumValues](
+    const auto establishOperandClass = [&populateEnumValues,&errorCount](
             const std::string& enumName, spv::OperandClass operandClass,
             spv::EnumValues* enumValues, const Json::Value& operandEnum, const std::string& category) {
         assert(category == "BitEnum" || category == "ValueEnum");
         bool bitEnum = (category == "BitEnum");
         populateEnumValues(enumValues, operandEnum, bitEnum);
+        const std::string errContext = "enum " + enumName;
+        for (const auto& e: *enumValues) {
+          if (!e.IsValid(operandClass, errContext)) {
+            errorCount++;
+          }
+        }
         OperandClassParams[operandClass].set(enumName, enumValues, bitEnum);
     };
 
@@ -563,6 +664,10 @@ void jsonToSpirv(const std::string& jsonPath, bool buildingHeaders)
             establishOperandClass(enumName, OperandCooperativeMatrixUse, &CooperativeMatrixUseParams, operandEnum, category);
         }
     }
+
+    if (errorCount > 0) {
+      std::exit(1);
+    }
 }
 
 };  // end namespace spv

tools/buildHeaders/jsonToSpirv.h

@@ -28,6 +28,7 @@
 
 #include <algorithm>
 #include <string>
+#include <iostream>
 #include <vector>
 #include <assert.h>
 
@@ -187,6 +188,7 @@ class EnumValuesContainer {
 
     iterator begin() { return values.begin(); }
     iterator end() { return values.end(); }
+    EValue& back() { return values.back(); }
 
 private:
     ContainerType values;
@@ -219,6 +221,10 @@ class EnumValue {
     Extensions extensions;
     OperandParameters operands;
     const char* desc;
+
+    // Returns true if this enum is valid, in isolation.
+    // Otherwise emits a diagnostic to std::cerr and returns false.
+    bool IsValid(OperandClass oc, const std::string& context) const;
 };
 
 using EnumValues = EnumValuesContainer<EnumValue>;