Validate that OpDecorateId IDs are well-ordered

source/val/validate_annotation.cpp

@@ -333,13 +333,35 @@ spv_result_t ValidateDecorate(ValidationState_t& _, const Instruction* inst) {
 }
 
 spv_result_t ValidateDecorateId(ValidationState_t& _, const Instruction* inst) {
+  const auto target_id = inst->GetOperandAs<uint32_t>(0);
+  const auto target = _.FindDef(target_id);
+  if (target && spv::Op::OpDecorationGroup == target->opcode()) {
+    return _.diag(SPV_ERROR_INVALID_ID, inst)
+           << "OpMemberDecorate Target <id> " << _.getIdName(target_id)
+           << " must not be an OpDecorationGroup instruction.";
+  }
+
   const auto decoration = inst->GetOperandAs<spv::Decoration>(1);
   if (!DecorationTakesIdParameters(decoration)) {
     return _.diag(SPV_ERROR_INVALID_ID, inst)
            << "Decorations that don't take ID parameters may not be used with "
               "OpDecorateId";
   }
 
+  for (uint32_t i = 2; i < inst->operands().size(); ++i) {
+    const auto param_id = inst->GetOperandAs<uint32_t>(i);
+    const auto param = _.FindDef(param_id);
+
+    // Both target and param are elements of ordered_instructions we can
+    // determine their relative positions in the SPIR-V module by comparing
+    // pointers.
+    if (target <= param) {
+      return _.diag(SPV_ERROR_INVALID_ID, inst)
+             << "Parameter <ID> " << _.getIdName(param_id)
+             << " must appear earlier in the binary than the target";
+    }
+  }
+
   // No member decorations take id parameters, so we don't bother checking if
   // we are using a member only decoration here.
 
@@ -388,8 +410,7 @@ spv_result_t ValidateDecorationGroup(ValidationState_t& _,
     if (use->opcode() != spv::Op::OpDecorate &&
         use->opcode() != spv::Op::OpGroupDecorate &&
         use->opcode() != spv::Op::OpGroupMemberDecorate &&
-        use->opcode() != spv::Op::OpName &&
-        use->opcode() != spv::Op::OpDecorateId && !use->IsNonSemantic()) {
+        use->opcode() != spv::Op::OpName && !use->IsNonSemantic()) {
       return _.diag(SPV_ERROR_INVALID_ID, inst)
              << "Result id of OpDecorationGroup can only "
              << "be targeted by OpName, OpGroupDecorate, "

test/val/val_decoration_test.cpp

@@ -5445,6 +5445,97 @@ OpFunctionEnd
   EXPECT_EQ(SPV_SUCCESS, ValidateInstructions());
 }
 
+// OpDecorateId
+
+TEST_F(ValidateDecorations, DecorateIdGood) {
+  const std::string spirv = R"(
+OpCapability Shader
+OpMemoryModel Logical Simple
+OpEntryPoint GLCompute %main "main"
+OpExecutionMode %main LocalSize 1 1 1
+OpName %subgroupscope "subgroupscope"
+OpName %int0 "int0"
+OpName %fn "fn"
+OpDecorateId %int0 UniformId %subgroupscope
+%void = OpTypeVoid
+%float = OpTypeFloat 32
+%int = OpTypeInt 32 1
+%subgroupscope = OpConstant %int 3
+%int0 = OpConstantNull %int
+%fn = OpTypeFunction %void
+%main = OpFunction %void None %fn
+%entry = OpLabel
+OpReturn
+OpFunctionEnd
+)";
+
+  CompileSuccessfully(spirv, SPV_ENV_UNIVERSAL_1_4);
+  EXPECT_EQ(SPV_SUCCESS, ValidateInstructions(SPV_ENV_UNIVERSAL_1_4));
+  EXPECT_THAT(getDiagnosticString(), Eq(""));
+}
+
+TEST_F(ValidateDecorations, DecorateIdGroupBad) {
+  const std::string spirv = R"(
+OpCapability Shader
+OpMemoryModel Logical Simple
+OpEntryPoint GLCompute %main "main"
+OpExecutionMode %main LocalSize 1 1 1
+OpName %subgroupscope "subgroupscope"
+OpName %int0 "int0"
+OpName %fn "fn"
+OpName %group "group"
+OpDecorateId %group UniformId %subgroupscope
+%group = OpDecorationGroup
+OpGroupDecorate %group %int0
+%void = OpTypeVoid
+%float = OpTypeFloat 32
+%int = OpTypeInt 32 1
+%subgroupscope = OpConstant %int 3
+%int0 = OpConstantNull %int
+%fn = OpTypeFunction %void
+%main = OpFunction %void None %fn
+%entry = OpLabel
+OpReturn
+OpFunctionEnd
+)";
+
+  CompileSuccessfully(spirv, SPV_ENV_UNIVERSAL_1_4);
+  EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions(SPV_ENV_UNIVERSAL_1_4));
+  EXPECT_THAT(getDiagnosticString(),
+              HasSubstr("must not be an OpDecorationGroup instruction.\n"
+                        "  OpDecorateId %group UniformId %subgroupscope"));
+}
+
+TEST_F(ValidateDecorations, DecorateIdOutOfOrderBad) {
+  const std::string spirv = R"(
+OpCapability Shader
+OpMemoryModel Logical Simple
+OpEntryPoint GLCompute %main "main"
+OpExecutionMode %main LocalSize 1 1 1
+OpName %subgroupscope "subgroupscope"
+OpName %int0 "int0"
+OpName %fn "fn"
+OpDecorateId %int0 UniformId %subgroupscope
+%void = OpTypeVoid
+%float = OpTypeFloat 32
+%int = OpTypeInt 32 1
+%int0 = OpConstantNull %int
+%subgroupscope = OpConstant %int 3
+%fn = OpTypeFunction %void
+%main = OpFunction %void None %fn
+%entry = OpLabel
+OpReturn
+OpFunctionEnd
+)";
+
+  CompileSuccessfully(spirv, SPV_ENV_UNIVERSAL_1_4);
+  EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions(SPV_ENV_UNIVERSAL_1_4));
+  EXPECT_THAT(getDiagnosticString(),
+              HasSubstr("[%subgroupscope]' must appear earlier in the"
+                        " binary than the target\n"
+                        "  OpDecorateId %int0 UniformId %subgroupscope"));
+}
+
 // Uniform and UniformId decorations
 
 TEST_F(ValidateDecorations, UniformDecorationGood) {
@@ -5486,17 +5577,19 @@ OpName %subgroupscope "subgroupscope"
 OpName %call "call"
 OpName %myfunc "myfunc"
 OpName %int0 "int0"
+OpName %int1 "int1"
 OpName %float0 "float0"
 OpName %fn "fn"
 )") + inst +
          R"(
 %void = OpTypeVoid
 %float = OpTypeFloat 32
 %int = OpTypeInt 32 1
-%int0 = OpConstantNull %int
+%int1 = OpConstant %int 1
 %int_99 = OpConstant %int 99
 %subgroupscope = OpConstant %int 3
 %float0 = OpConstantNull %float
+%int0 = OpConstantNull %int
 %fn = OpTypeFunction %void
 %myfunc = OpFunction %void None %fn
 %myfuncentry = OpLabel
@@ -5613,7 +5706,7 @@ TEST_F(ValidateDecorations,
 
 TEST_F(ValidateDecorations, UniformDecorationWithScopeIdV14VulkanEnv) {
   const std::string spirv =
-      ShaderWithUniformLikeDecoration("OpDecorateId %int0 UniformId %int0");
+      ShaderWithUniformLikeDecoration("OpDecorateId %int0 UniformId %int1");
 
   CompileSuccessfully(spirv, SPV_ENV_VULKAN_1_1_SPIRV_1_4);
   EXPECT_EQ(SPV_ERROR_INVALID_DATA,
@@ -10519,8 +10612,8 @@ const std::string kNodeShaderPostlude = R"(
 %node1 = OpConstantStringAMDX "node1"
 %node2 = OpConstantStringAMDX "node2"
 %S = OpTypeStruct
-%_payloadarr_S = OpTypeNodePayloadArrayAMDX %S
 %_payloadarr_S_0 = OpTypeNodePayloadArrayAMDX %S
+%_payloadarr_S = OpTypeNodePayloadArrayAMDX %S
 %bool = OpTypeBool
 %true = OpConstantTrue %bool
 %void = OpTypeVoid

test/val/val_modes_test.cpp

@@ -2543,8 +2543,8 @@ const std::string kNodeShaderPostlude = R"(
 %node1 = OpConstantStringAMDX "node1"
 %node2 = OpConstantStringAMDX "node2"
 %S = OpTypeStruct
-%_payloadarr_S = OpTypeNodePayloadArrayAMDX %S
 %_payloadarr_S_0 = OpTypeNodePayloadArrayAMDX %S
+%_payloadarr_S = OpTypeNodePayloadArrayAMDX %S
 %bool = OpTypeBool
 %true = OpConstantTrue %bool
 %void = OpTypeVoid