Validate layouts for PhysicalStorageBuffer pointers (#5291)

* Validate layouts for PhysicalStorageBuffer pointers

Fixes #5282

* These pointers may not orginate from a variable so standard layout
  validation misses them
* Now checks every instructions that results in a physical storage
  buffer pointer
  * May not start from a Block-decorated struct so that part is fudged
    with a valid layout

* formatting

source/val/validate_decorations.cpp

@@ -452,7 +452,16 @@ spv_result_t checkLayout(uint32_t struct_id, const char* storage_class_str,
     return ds;
   };
 
-  const auto& members = getStructMembers(struct_id, vstate);
+  // If we are checking physical storage buffer pointers, we may not actually
+  // have a struct here. Instead, pretend we have a struct with a single member
+  // at offset 0.
+  const auto& struct_type = vstate.FindDef(struct_id);
+  std::vector<uint32_t> members;
+  if (struct_type->opcode() == spv::Op::OpTypeStruct) {
+    members = getStructMembers(struct_id, vstate);
+  } else {
+    members.push_back(struct_id);
+  }
 
   // To check for member overlaps, we want to traverse the members in
   // offset order.
@@ -461,31 +470,38 @@ spv_result_t checkLayout(uint32_t struct_id, const char* storage_class_str,
     uint32_t offset;
   };
   std::vector<MemberOffsetPair> member_offsets;
-  member_offsets.reserve(members.size());
+
-  for (uint32_t memberIdx = 0, numMembers = uint32_t(members.size());
+  // With physical storage buffers, we might be checking layouts that do not
-       memberIdx < numMembers; memberIdx++) {
+  // originate from a structure.
-    uint32_t offset = 0xffffffff;
+  if (struct_type->opcode() == spv::Op::OpTypeStruct) {
-    auto member_decorations =
+    member_offsets.reserve(members.size());
-        vstate.id_member_decorations(struct_id, memberIdx);
+    for (uint32_t memberIdx = 0, numMembers = uint32_t(members.size());
-    for (auto decoration = member_decorations.begin;
+         memberIdx < numMembers; memberIdx++) {
-         decoration != member_decorations.end; ++decoration) {
+      uint32_t offset = 0xffffffff;
-      assert(decoration->struct_member_index() == (int)memberIdx);
+      auto member_decorations =
-      switch (decoration->dec_type()) {
+          vstate.id_member_decorations(struct_id, memberIdx);
-        case spv::Decoration::Offset:
+      for (auto decoration = member_decorations.begin;
-          offset = decoration->params()[0];
+           decoration != member_decorations.end; ++decoration) {
-          break;
+        assert(decoration->struct_member_index() == (int)memberIdx);
-        default:
+        switch (decoration->dec_type()) {
-          break;
+          case spv::Decoration::Offset:
+            offset = decoration->params()[0];
+            break;
+          default:
+            break;
+        }
       }
+      member_offsets.push_back(
+          MemberOffsetPair{memberIdx, incoming_offset + offset});
     }
-    member_offsets.push_back(
+    std::stable_sort(
-        MemberOffsetPair{memberIdx, incoming_offset + offset});
+        member_offsets.begin(), member_offsets.end(),
+        [](const MemberOffsetPair& lhs, const MemberOffsetPair& rhs) {
+          return lhs.offset < rhs.offset;
+        });
+  } else {
+    member_offsets.push_back({0, 0});
   }
-  std::stable_sort(
-      member_offsets.begin(), member_offsets.end(),
-      [](const MemberOffsetPair& lhs, const MemberOffsetPair& rhs) {
-        return lhs.offset < rhs.offset;
-      });
 
   // Now scan from lowest offset to highest offset.
   uint32_t nextValidOffset = 0;
@@ -1023,6 +1039,8 @@ spv_result_t CheckDecorationsOfBuffers(ValidationState_t& vstate) {
   std::unordered_set<uint32_t> uses_push_constant;
   for (const auto& inst : vstate.ordered_instructions()) {
     const auto& words = inst.words();
+    auto type_id = inst.type_id();
+    const Instruction* type_inst = vstate.FindDef(type_id);
     if (spv::Op::OpVariable == inst.opcode()) {
       const auto var_id = inst.id();
       // For storage class / decoration combinations, see Vulkan 14.5.4 "Offset
@@ -1276,6 +1294,23 @@ spv_result_t CheckDecorationsOfBuffers(ValidationState_t& vstate) {
           }
         }
       }
+    } else if (type_inst && type_inst->opcode() == spv::Op::OpTypePointer &&
+               type_inst->GetOperandAs<spv::StorageClass>(1u) ==
+                   spv::StorageClass::PhysicalStorageBuffer) {
+      const bool scalar_block_layout = vstate.options()->scalar_block_layout;
+      MemberConstraints constraints;
+      const bool buffer = true;
+      const auto data_type_id = type_inst->GetOperandAs<uint32_t>(2u);
+      const auto* data_type_inst = vstate.FindDef(data_type_id);
+      if (data_type_inst->opcode() == spv::Op::OpTypeStruct) {
+        ComputeMemberConstraintsForStruct(&constraints, data_type_id,
+                                          LayoutConstraints(), vstate);
+      }
+      if (auto res = checkLayout(data_type_id, "PhysicalStorageBuffer", "Block",
+                                 !buffer, scalar_block_layout, 0, constraints,
+                                 vstate)) {
+        return res;
+      }
     }
   }
   return SPV_SUCCESS;

test/opt/inst_buff_addr_check_test.cpp

@@ -928,6 +928,7 @@ OpFunctionEnd
 
   SetTargetEnv(SPV_ENV_VULKAN_1_2);
   SetAssembleOptions(SPV_TEXT_TO_BINARY_OPTION_PRESERVE_NUMERIC_IDS);
+  ValidatorOptions()->scalar_block_layout = true;
   SinglePassRunAndMatch<InstBuffAddrCheckPass>(text, true, 7, 23);
 }
 

test/val/val_decoration_test.cpp

@@ -9252,6 +9252,124 @@ OpFunctionEnd
   EXPECT_EQ(SPV_SUCCESS, ValidateInstructions(SPV_ENV_VULKAN_1_0));
 }
 
+TEST_F(ValidateDecorations, PhysicalStorageBufferWithOffset) {
+  const std::string spirv = R"(
+OpCapability Shader
+OpCapability Int64
+OpCapability PhysicalStorageBufferAddresses
+OpMemoryModel PhysicalStorageBuffer64 GLSL450
+OpEntryPoint GLCompute %main "main" %pc
+OpExecutionMode %main LocalSize 1 1 1
+OpDecorate %pc_block Block
+OpMemberDecorate %pc_block 0 Offset 0
+OpMemberDecorate %pssbo_struct 0 Offset 0
+%void = OpTypeVoid
+%long = OpTypeInt 64 0
+%float = OpTypeFloat 32
+%int = OpTypeInt 32 0
+%int_0 = OpConstant %int 0
+%pc_block = OpTypeStruct %long
+%pc_block_ptr = OpTypePointer PushConstant %pc_block
+%pc_long_ptr = OpTypePointer PushConstant %long
+%pc = OpVariable %pc_block_ptr PushConstant
+%pssbo_struct = OpTypeStruct %float
+%pssbo_ptr = OpTypePointer PhysicalStorageBuffer %pssbo_struct
+%void_fn = OpTypeFunction %void
+%main = OpFunction %void None %void_fn
+%entry = OpLabel
+%pc_gep = OpAccessChain %pc_long_ptr %pc %int_0
+%addr = OpLoad %long %pc_gep
+%ptr = OpConvertUToPtr %pssbo_ptr %addr
+OpReturn
+OpFunctionEnd
+)";
+
+  CompileSuccessfully(spirv, SPV_ENV_VULKAN_1_3);
+  EXPECT_EQ(SPV_SUCCESS, ValidateInstructions(SPV_ENV_VULKAN_1_3));
+}
+
+TEST_F(ValidateDecorations, PhysicalStorageBufferMissingOffset) {
+  const std::string spirv = R"(
+OpCapability Shader
+OpCapability Int64
+OpCapability PhysicalStorageBufferAddresses
+OpMemoryModel PhysicalStorageBuffer64 GLSL450
+OpEntryPoint GLCompute %main "main" %pc
+OpExecutionMode %main LocalSize 1 1 1
+OpDecorate %pc_block Block
+OpMemberDecorate %pc_block 0 Offset 0
+%void = OpTypeVoid
+%long = OpTypeInt 64 0
+%float = OpTypeFloat 32
+%int = OpTypeInt 32 0
+%int_0 = OpConstant %int 0
+%pc_block = OpTypeStruct %long
+%pc_block_ptr = OpTypePointer PushConstant %pc_block
+%pc_long_ptr = OpTypePointer PushConstant %long
+%pc = OpVariable %pc_block_ptr PushConstant
+%pssbo_struct = OpTypeStruct %float
+%pssbo_ptr = OpTypePointer PhysicalStorageBuffer %pssbo_struct
+%void_fn = OpTypeFunction %void
+%main = OpFunction %void None %void_fn
+%entry = OpLabel
+%pc_gep = OpAccessChain %pc_long_ptr %pc %int_0
+%addr = OpLoad %long %pc_gep
+%ptr = OpConvertUToPtr %pssbo_ptr %addr
+OpReturn
+OpFunctionEnd
+)";
+
+  CompileSuccessfully(spirv, SPV_ENV_VULKAN_1_3);
+  EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions(SPV_ENV_VULKAN_1_3));
+  EXPECT_THAT(
+      getDiagnosticString(),
+      HasSubstr("decorated as Block for variable in PhysicalStorageBuffer "
+                "storage class must follow relaxed storage buffer layout "
+                "rules: member 0 is missing an Offset decoration"));
+}
+
+TEST_F(ValidateDecorations, PhysicalStorageBufferMissingArrayStride) {
+  const std::string spirv = R"(
+OpCapability Shader
+OpCapability Int64
+OpCapability PhysicalStorageBufferAddresses
+OpMemoryModel PhysicalStorageBuffer64 GLSL450
+OpEntryPoint GLCompute %main "main" %pc
+OpExecutionMode %main LocalSize 1 1 1
+OpDecorate %pc_block Block
+OpMemberDecorate %pc_block 0 Offset 0
+%void = OpTypeVoid
+%long = OpTypeInt 64 0
+%float = OpTypeFloat 32
+%int = OpTypeInt 32 0
+%int_0 = OpConstant %int 0
+%int_4 = OpConstant %int 4
+%pc_block = OpTypeStruct %long
+%pc_block_ptr = OpTypePointer PushConstant %pc_block
+%pc_long_ptr = OpTypePointer PushConstant %long
+%pc = OpVariable %pc_block_ptr PushConstant
+%pssbo_array = OpTypeArray %float %int_4
+%pssbo_ptr = OpTypePointer PhysicalStorageBuffer %pssbo_array
+%void_fn = OpTypeFunction %void
+%main = OpFunction %void None %void_fn
+%entry = OpLabel
+%pc_gep = OpAccessChain %pc_long_ptr %pc %int_0
+%addr = OpLoad %long %pc_gep
+%ptr = OpConvertUToPtr %pssbo_ptr %addr
+OpReturn
+OpFunctionEnd
+)";
+
+  CompileSuccessfully(spirv, SPV_ENV_VULKAN_1_3);
+  EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions(SPV_ENV_VULKAN_1_3));
+  EXPECT_THAT(
+      getDiagnosticString(),
+      HasSubstr(
+          "decorated as Block for variable in PhysicalStorageBuffer storage "
+          "class must follow relaxed storage buffer layout rules: member 0 "
+          "contains an array with stride 0, but with an element size of 4"));
+}
+
 }  // namespace
 }  // namespace val
 }  // namespace spvtools