Extra restrictions for accesses of block arrays (KhronosGroup#6226)

* Extra restrictions for accesses of block arrays

* If a PtrAccessChain is rooted on a block, element (if constant) must
  be zero
* UntypedAccessChains check that block arrays must not be reinterpreted
* Basic element operand checks for ptr access chains

* add tests

* formatting

* formatting

Author: alan-baker

source/val/validate_memory.cpp

@@ -1555,6 +1555,60 @@ spv_result_t ValidateAccessChain(ValidationState_t& _,
       return _.diag(SPV_ERROR_INVALID_ID, inst)
              << "Base type must be a non-pointer type";
     }
+
+    const auto ContainsBlock = [&_](const Instruction* type_inst) {
+      if (type_inst->opcode() == spv::Op::OpTypeStruct) {
+        if (_.HasDecoration(type_inst->id(), spv::Decoration::Block) ||
+            _.HasDecoration(type_inst->id(), spv::Decoration::BufferBlock)) {
+          return true;
+        }
+      }
+      return false;
+    };
+
+    // Block (and BufferBlock) arrays cannot be reinterpreted via untyped access
+    // chains.
+    const bool base_type_block_array =
+        base_type->opcode() == spv::Op::OpTypeArray &&
+        _.ContainsType(base_type->id(), ContainsBlock,
+                       /* traverse_all_types = */ false);
+
+    const auto base_index = untyped_pointer ? 3 : 2;
+    const auto base_id = inst->GetOperandAs<uint32_t>(base_index);
+    auto base = _.FindDef(base_id);
+    // Strictly speaking this misses trivial access chains and function
+    // parameter chasing, but that would be a significant complication in the
+    // traversal.
+    while (base->opcode() == spv::Op::OpCopyObject) {
+      base = _.FindDef(base->GetOperandAs<uint32_t>(2));
+    }
+    const Instruction* base_data_type = nullptr;
+    if (base->opcode() == spv::Op::OpVariable) {
+      const auto ptr_type = _.FindDef(base->type_id());
+      base_data_type = _.FindDef(ptr_type->GetOperandAs<uint32_t>(2));
+    } else if (base->opcode() == spv::Op::OpUntypedVariableKHR) {
+      if (base->operands().size() > 3) {
+        base_data_type = _.FindDef(base->GetOperandAs<uint32_t>(3));
+      }
+    }
+
+    if (base_data_type) {
+      const bool base_block_array =
+          base_data_type->opcode() == spv::Op::OpTypeArray &&
+          _.ContainsType(base_data_type->id(), ContainsBlock,
+                         /* traverse_all_types = */ false);
+
+      if (base_type_block_array != base_block_array) {
+        return _.diag(SPV_ERROR_INVALID_ID, inst)
+               << "Both Base Type and Base must be Block or BufferBlock arrays "
+                  "or neither can be";
+      } else if (base_type_block_array && base_block_array &&
+                 base_type->id() != base_data_type->id()) {
+        return _.diag(SPV_ERROR_INVALID_ID, inst)
+               << "If Base or Base Type is a Block or BufferBlock array, the "
+                  "other must also be the same array";
+      }
+    }
   }
 
   // Base must be a pointer, pointing to the base of a composite object.
@@ -1845,14 +1899,34 @@ spv_result_t ValidatePtrAccessChain(ValidationState_t& _,
 
   const bool untyped_pointer = spvOpcodeGeneratesUntypedPointer(inst->opcode());
 
-  const auto base_id = inst->GetOperandAs<uint32_t>(2);
-  const auto base = _.FindDef(base_id);
-  const auto base_type = untyped_pointer
-                             ? _.FindDef(inst->GetOperandAs<uint32_t>(2))
-                             : _.FindDef(base->type_id());
+  const auto base_idx = untyped_pointer ? 3 : 2;
+  const auto base = _.FindDef(inst->GetOperandAs<uint32_t>(base_idx));
+  const auto base_type = _.FindDef(base->type_id());
   const auto base_type_storage_class =
       base_type->GetOperandAs<spv::StorageClass>(1);
 
+  const auto element_idx = untyped_pointer ? 4 : 3;
+  const auto element = _.FindDef(inst->GetOperandAs<uint32_t>(element_idx));
+  const auto element_type = _.FindDef(element->type_id());
+  if (!element_type || element_type->opcode() != spv::Op::OpTypeInt) {
+    return _.diag(SPV_ERROR_INVALID_DATA, inst) << "Element must be an integer";
+  }
+  uint64_t element_val = 0;
+  if (_.EvalConstantValUint64(element->id(), &element_val)) {
+    if (element_val != 0) {
+      const auto interp_type =
+          untyped_pointer ? _.FindDef(inst->GetOperandAs<uint32_t>(2))
+                          : _.FindDef(base_type->GetOperandAs<uint32_t>(2));
+      if (interp_type->opcode() == spv::Op::OpTypeStruct &&
+          (_.HasDecoration(interp_type->id(), spv::Decoration::Block) ||
+           _.HasDecoration(interp_type->id(), spv::Decoration::BufferBlock))) {
+        return _.diag(SPV_ERROR_INVALID_DATA, inst)
+               << "Element must be 0 if the interpretation type is a Block- or "
+                  "BufferBlock-decorated structure";
+      }
+    }
+  }
+
   if (_.HasCapability(spv::Capability::Shader) &&
       (base_type_storage_class == spv::StorageClass::Uniform ||
        base_type_storage_class == spv::StorageClass::StorageBuffer ||

test/opt/eliminate_dead_member_test.cpp

@@ -958,7 +958,7 @@ TEST_F(EliminateDeadMemberTest, RemoveMemberPtrAccessChain) {
 ; CHECK: OpMemberDecorate %type__Globals 1 Offset 16
 ; CHECK: %type__Globals = OpTypeStruct %float %float
 ; CHECK: [[ac:%\w+]] = OpAccessChain %_ptr_Uniform_type__Globals %_Globals %uint_0
-; CHECK: OpPtrAccessChain %_ptr_Uniform_float [[ac]] %uint_1 %uint_0
+; CHECK: OpPtrAccessChain %_ptr_Uniform_float [[ac]] %uint_0 %uint_0
 ; CHECK: OpPtrAccessChain %_ptr_Uniform_float [[ac]] %uint_0 %uint_1
                OpCapability Shader
                OpCapability VariablePointersStorageBuffer
@@ -995,7 +995,7 @@ TEST_F(EliminateDeadMemberTest, RemoveMemberPtrAccessChain) {
        %main = OpFunction %void None %14
          %16 = OpLabel
          %17 = OpAccessChain %_ptr_Uniform_type__Globals %_Globals %uint_0
-         %18 = OpPtrAccessChain %_ptr_Uniform_float %17 %uint_1 %uint_0
+         %18 = OpPtrAccessChain %_ptr_Uniform_float %17 %uint_0 %uint_0
          %19 = OpPtrAccessChain %_ptr_Uniform_float %17 %uint_0 %uint_2
                OpReturn
                OpFunctionEnd

test/val/val_decoration_test.cpp

@@ -10304,6 +10304,7 @@ OpMemberDecorate %struct 0 Offset 0
 OpMemberDecorate %struct 1 Offset 4
 )" + set + R"(OpMemberDecorate %test_type 0 Offset 0
 OpMemberDecorate %test_type 1 Offset 1
+OpDecorate %ptr ArrayStride 16
 %void = OpTypeVoid
 %int = OpTypeInt 32 0
 %int_0 = OpConstant %int 0
@@ -10312,7 +10313,8 @@ OpMemberDecorate %test_type 1 Offset 1
 %test_val = OpConstantNull %test_type
 %ptr = OpTypeUntypedPointerKHR )" +
                             sc + R"(
-%var = OpUntypedVariableKHR %ptr )" + sc + R"( %struct
+%var = OpUntypedVariableKHR %ptr )" +
+                            sc + R"( %struct
 %void_fn = OpTypeFunction %void
 %main = OpFunction %void None %void_fn
 %entry = OpLabel
@@ -10355,6 +10357,7 @@ OpDecorate %struct Block
 OpMemberDecorate %struct 0 Offset 0
 OpMemberDecorate %struct 1 Offset 4
 )" + set + R"(OpDecorate %test_type ArrayStride 4
+OpDecorate %ptr ArrayStride 16
 %void = OpTypeVoid
 %int = OpTypeInt 32 0
 %int_0 = OpConstant %int 0
@@ -10365,7 +10368,8 @@ OpMemberDecorate %struct 1 Offset 4
 %struct = OpTypeStruct %int %int
 %ptr = OpTypeUntypedPointerKHR )" +
                             sc + R"(
-%var = OpUntypedVariableKHR %ptr )" + sc + R"( %struct
+%var = OpUntypedVariableKHR %ptr )" +
+                            sc + R"( %struct
 %void_fn = OpTypeFunction %void
 %main = OpFunction %void None %void_fn
 %entry = OpLabel