Validate array stride does not cause overlap (#3028)

Fixes #3027

* Disallow array stride 0
* Check array stride against element size
* Fix up tests
* Add new tests

source/val/validate_decorations.cpp

@@ -522,10 +522,13 @@ spv_result_t checkLayout(uint32_t struct_id, const char* storage_class_str,
       const auto typeId = array_inst->word(2);
       const auto element_inst = vstate.FindDef(typeId);
       // Check array stride.
-      auto array_stride = 0;
+      uint32_t array_stride = 0;
       for (auto& decoration : vstate.id_decorations(array_inst->id())) {
         if (SpvDecorationArrayStride == decoration.dec_type()) {
           array_stride = decoration.params()[0];
+          if (array_stride == 0) {
+            return fail(memberIdx) << "contains an array with stride 0";
+          }
           if (!IsAlignedTo(array_stride, array_alignment))
             return fail(memberIdx)
                    << "contains an array with stride " << decoration.params()[0]
@@ -563,6 +566,14 @@ spv_result_t checkLayout(uint32_t struct_id, const char* storage_class_str,
                             ? getScalarAlignment(array_inst->id(), vstate)
                             : getBaseAlignment(array_inst->id(), blockRules,
                                                constraint, constraints, vstate);
+
+      const auto element_size =
+          getSize(element_inst->id(), constraint, constraints, vstate);
+      if (element_size > array_stride) {
+        return fail(memberIdx)
+               << "contains an array with stride " << array_stride
+               << ", but with an element size of " << element_size;
+      }
     }
     nextValidOffset = offset + size;
     if (!scalar_block_layout && blockRules &&

test/opt/eliminate_dead_member_test.cpp

@@ -561,6 +561,7 @@ TEST_F(EliminateDeadMemberTest, RemoveMembersUpdateArrayLength) {
                OpName %main "main"
                OpDecorate %_Globals DescriptorSet 0
                OpDecorate %_Globals Binding 0
+               OpDecorate %_runtimearr_float ArrayStride 16
                OpMemberDecorate %type__Globals 0 Offset 0
                OpMemberDecorate %type__Globals 1 Offset 4
                OpMemberDecorate %type__Globals 2 Offset 16

test/opt/graphics_robust_access_test.cpp

@@ -898,7 +898,7 @@ TEST_F(GraphicsRobustAccessTest, ACStructNegativeFail) {
 TEST_F(GraphicsRobustAccessTest, ACRTArrayLeastInboundClamped) {
   for (auto* ac : AccessChains()) {
     std::ostringstream shaders;
-    shaders << ShaderPreambleAC() << "OpMemberDecorate %ssbo_s 0 ArrayStride 4 "
+    shaders << ShaderPreambleAC() << "OpDecorate %rtarr ArrayStride 4 "
             << DecoSSBO() << TypesVoid() << TypesInt() << TypesFloat() << R"(
        %rtarr = OpTypeRuntimeArray %float
        %ssbo_s = OpTypeStruct %uint %uint %rtarr
@@ -924,9 +924,8 @@ TEST_F(GraphicsRobustAccessTest, ACRTArrayGeneralShortIndexClamped) {
   for (auto* ac : AccessChains()) {
     std::ostringstream shaders;
     shaders << "OpCapability Int16\n"
-            << ShaderPreambleAC({"i"})
+            << ShaderPreambleAC({"i"}) << "OpDecorate %rtarr ArrayStride 4 "
-            << "OpMemberDecorate %ssbo_s 0 ArrayStride 4 " << DecoSSBO()
+            << DecoSSBO() << TypesVoid() << TypesShort() << TypesFloat() << R"(
-            << TypesVoid() << TypesShort() << TypesFloat() << R"(
        %rtarr = OpTypeRuntimeArray %float
        %ssbo_s = OpTypeStruct %short %short %rtarr
        %var_ty = OpTypePointer Uniform %ssbo_s
@@ -954,9 +953,8 @@ TEST_F(GraphicsRobustAccessTest, ACRTArrayGeneralUShortIndexClamped) {
   for (auto* ac : AccessChains()) {
     std::ostringstream shaders;
     shaders << "OpCapability Int16\n"
-            << ShaderPreambleAC({"i"})
+            << ShaderPreambleAC({"i"}) << "OpDecorate %rtarr ArrayStride 4 "
-            << "OpMemberDecorate %ssbo_s 0 ArrayStride 4 " << DecoSSBO()
+            << DecoSSBO() << TypesVoid() << TypesShort() << TypesFloat() << R"(
-            << TypesVoid() << TypesShort() << TypesFloat() << R"(
        %rtarr = OpTypeRuntimeArray %float
        %ssbo_s = OpTypeStruct %short %short %rtarr
        %var_ty = OpTypePointer Uniform %ssbo_s
@@ -983,9 +981,8 @@ TEST_F(GraphicsRobustAccessTest, ACRTArrayGeneralUShortIndexClamped) {
 TEST_F(GraphicsRobustAccessTest, ACRTArrayGeneralIntIndexClamped) {
   for (auto* ac : AccessChains()) {
     std::ostringstream shaders;
-    shaders << ShaderPreambleAC({"i"})
+    shaders << ShaderPreambleAC({"i"}) << "OpDecorate %rtarr ArrayStride 4 "
-            << "OpMemberDecorate %ssbo_s 0 ArrayStride 4 " << DecoSSBO()
+            << DecoSSBO() << TypesVoid() << TypesInt() << TypesFloat() << R"(
-            << TypesVoid() << TypesInt() << TypesFloat() << R"(
        %rtarr = OpTypeRuntimeArray %float
        %ssbo_s = OpTypeStruct %int %int %rtarr
        %var_ty = OpTypePointer Uniform %ssbo_s
@@ -1000,8 +997,9 @@ TEST_F(GraphicsRobustAccessTest, ACRTArrayGeneralIntIndexClamped) {
        ; CHECK: %[[arrlen:\w+]] = OpArrayLength %uint %var 2
        ; CHECK: %[[max:\w+]] = OpISub %int %[[arrlen]] %int_1
        ; CHECK: %[[clamp:\w+]] = OpExtInst %int %[[GLSLSTD450]] UClamp %i %int_0 %[[max]]
-       )" << MainPrefix()
+       )"
-            << ACCheck(ac, "%int_2 %i", "%int_2 %[[clamp]]") << MainSuffix();
+            << MainPrefix() << ACCheck(ac, "%int_2 %i", "%int_2 %[[clamp]]")
+            << MainSuffix();
     SinglePassRunAndMatch<GraphicsRobustAccessPass>(shaders.str(), true);
   }
 }
@@ -1009,9 +1007,8 @@ TEST_F(GraphicsRobustAccessTest, ACRTArrayGeneralIntIndexClamped) {
 TEST_F(GraphicsRobustAccessTest, ACRTArrayGeneralUIntIndexClamped) {
   for (auto* ac : AccessChains()) {
     std::ostringstream shaders;
-    shaders << ShaderPreambleAC({"i"})
+    shaders << ShaderPreambleAC({"i"}) << "OpDecorate %rtarr ArrayStride 4 "
-            << "OpMemberDecorate %ssbo_s 0 ArrayStride 4 " << DecoSSBO()
+            << DecoSSBO() << TypesVoid() << TypesInt() << TypesFloat() << R"(
-            << TypesVoid() << TypesInt() << TypesFloat() << R"(
        %rtarr = OpTypeRuntimeArray %float
        %ssbo_s = OpTypeStruct %int %int %rtarr
        %var_ty = OpTypePointer Uniform %ssbo_s
@@ -1026,8 +1023,9 @@ TEST_F(GraphicsRobustAccessTest, ACRTArrayGeneralUIntIndexClamped) {
        ; CHECK: %[[arrlen:\w+]] = OpArrayLength %uint %var 2
        ; CHECK: %[[max:\w+]] = OpISub %uint %[[arrlen]] %uint_1
        ; CHECK: %[[clamp:\w+]] = OpExtInst %uint %[[GLSLSTD450]] UClamp %i %uint_0 %[[max]]
-       )" << MainPrefix()
+       )"
-            << ACCheck(ac, "%int_2 %i", "%int_2 %[[clamp]]") << MainSuffix();
+            << MainPrefix() << ACCheck(ac, "%int_2 %i", "%int_2 %[[clamp]]")
+            << MainSuffix();
     SinglePassRunAndMatch<GraphicsRobustAccessPass>(shaders.str(), true);
   }
 }
@@ -1036,8 +1034,8 @@ TEST_F(GraphicsRobustAccessTest, ACRTArrayGeneralLongIndexClamped) {
   for (auto* ac : AccessChains()) {
     std::ostringstream shaders;
     shaders << "OpCapability Int64" << ShaderPreambleAC({"i"})
-            << "OpMemberDecorate %ssbo_s 0 ArrayStride 4 " << DecoSSBO()
+            << "OpDecorate %rtarr ArrayStride 4 " << DecoSSBO() << TypesVoid()
-            << TypesVoid() << TypesInt() << TypesLong() << TypesFloat() << R"(
+            << TypesInt() << TypesLong() << TypesFloat() << R"(
        %rtarr = OpTypeRuntimeArray %float
        %ssbo_s = OpTypeStruct %int %int %rtarr
        %var_ty = OpTypePointer Uniform %ssbo_s
@@ -1053,9 +1051,8 @@ TEST_F(GraphicsRobustAccessTest, ACRTArrayGeneralLongIndexClamped) {
        ; CHECK: %[[arrlen_ext:\w+]] = OpUConvert %ulong %[[arrlen]]
        ; CHECK: %[[max:\w+]] = OpISub %long %[[arrlen_ext]] %long_1
        ; CHECK: %[[clamp:\w+]] = OpExtInst %long %[[GLSLSTD450]] UClamp %i %long_0 %[[max]]
-       )"
+       )" << MainPrefix()
-            << MainPrefix() << ACCheck(ac, "%int_2 %i", "%int_2 %[[clamp]]")
+            << ACCheck(ac, "%int_2 %i", "%int_2 %[[clamp]]") << MainSuffix();
-            << MainSuffix();
     SinglePassRunAndMatch<GraphicsRobustAccessPass>(shaders.str(), true);
   }
 }
@@ -1064,8 +1061,8 @@ TEST_F(GraphicsRobustAccessTest, ACRTArrayGeneralULongIndexClamped) {
   for (auto* ac : AccessChains()) {
     std::ostringstream shaders;
     shaders << "OpCapability Int64" << ShaderPreambleAC({"i"})
-            << "OpMemberDecorate %ssbo_s 0 ArrayStride 4 " << DecoSSBO()
+            << "OpDecorate %rtarr ArrayStride 4 " << DecoSSBO() << TypesVoid()
-            << TypesVoid() << TypesInt() << TypesLong() << TypesFloat() << R"(
+            << TypesInt() << TypesLong() << TypesFloat() << R"(
        %rtarr = OpTypeRuntimeArray %float
        %ssbo_s = OpTypeStruct %int %int %rtarr
        %var_ty = OpTypePointer Uniform %ssbo_s
@@ -1081,9 +1078,8 @@ TEST_F(GraphicsRobustAccessTest, ACRTArrayGeneralULongIndexClamped) {
        ; CHECK: %[[arrlen_ext:\w+]] = OpUConvert %ulong %[[arrlen]]
        ; CHECK: %[[max:\w+]] = OpISub %ulong %[[arrlen_ext]] %ulong_1
        ; CHECK: %[[clamp:\w+]] = OpExtInst %ulong %[[GLSLSTD450]] UClamp %i %ulong_0 %[[max]]
-       )"
+       )" << MainPrefix()
-            << MainPrefix() << ACCheck(ac, "%int_2 %i", "%int_2 %[[clamp]]")
+            << ACCheck(ac, "%int_2 %i", "%int_2 %[[clamp]]") << MainSuffix();
-            << MainSuffix();
     SinglePassRunAndMatch<GraphicsRobustAccessPass>(shaders.str(), true);
   }
 }
@@ -1095,7 +1091,7 @@ TEST_F(GraphicsRobustAccessTest, ACRTArrayStructVectorElem) {
   for (auto* ac : AccessChains()) {
     std::ostringstream shaders;
     shaders << ShaderPreambleAC({"i", "j"})
-            << "OpMemberDecorate %ssbo_s 0 ArrayStride 32\n"
+            << "OpDecorate %rtarr ArrayStride 32\n"
             << DecoSSBO() << "OpMemberDecorate %rtelem 0 Offset 0\n"
             << "OpMemberDecorate %rtelem 1 Offset 16\n"
             << TypesVoid() << TypesInt() << TypesFloat() << R"(
@@ -1131,7 +1127,7 @@ TEST_F(GraphicsRobustAccessTest, ACArrayRTArrayStructVectorElem) {
   for (auto* ac : AccessChains()) {
     std::ostringstream shaders;
     shaders << ShaderPreambleAC({"i", "ssbo_s"})
-            << "OpMemberDecorate %ssbo_s 0 ArrayStride 32\n"
+            << "OpDecorate %rtarr ArrayStride 32\n"
             << DecoSSBO() << "OpMemberDecorate %rtelem 0 Offset 0\n"
             << "OpMemberDecorate %rtelem 1 Offset 16\n"
             << TypesVoid() << TypesInt() << TypesFloat() << R"(
@@ -1175,7 +1171,7 @@ TEST_F(GraphicsRobustAccessTest, ACSplitACArrayRTArrayStructVectorElem) {
     std::ostringstream shaders;
     shaders << ShaderPreambleAC({"i", "j", "k", "ssbo_s", "ssbo_pty",
                                  "rtarr_pty", "ac_ssbo", "ac_rtarr"})
-            << "OpMemberDecorate %ssbo_s 0 ArrayStride 32\n"
+            << "OpDecorate %rtarr ArrayStride 32\n"
             << DecoSSBO() << "OpMemberDecorate %rtelem 0 Offset 0\n"
             << "OpMemberDecorate %rtelem 1 Offset 16\n"
             << TypesVoid() << TypesInt() << TypesFloat() << R"(
@@ -1238,7 +1234,7 @@ TEST_F(GraphicsRobustAccessTest,
     shaders << ShaderPreambleAC({"i", "j", "k", "bb1", "bb2", "ssbo_s",
                                  "ssbo_pty", "rtarr_pty", "ac_ssbo",
                                  "ac_rtarr"})
-            << "OpMemberDecorate %ssbo_s 0 ArrayStride 32\n"
+            << "OpDecorate %rtarr ArrayStride 32\n"
             << DecoSSBO() << "OpMemberDecorate %rtelem 0 Offset 0\n"
             << "OpMemberDecorate %rtelem 1 Offset 16\n"
             << TypesVoid() << TypesInt() << TypesFloat() << R"(

test/val/val_decoration_test.cpp

@@ -6929,6 +6929,72 @@ OpFunctionEnd
                 "identified with a Block or BufferBlock decoration"));
 }
 
+TEST_F(ValidateDecorations, VulkanArrayStrideZero) {
+  const std::string spirv = R"(
+OpCapability Shader
+OpMemoryModel Logical GLSL450
+OpEntryPoint GLCompute %main "main"
+OpExecutionMode %main LocalSize 1 1 1
+OpDecorate %var DescriptorSet 0
+OpDecorate %var Binding 0
+OpDecorate %struct Block
+OpMemberDecorate %struct 0 Offset 0
+OpDecorate %array ArrayStride 0
+%void = OpTypeVoid
+%int = OpTypeInt 32 0
+%int_4 = OpConstant %int 4
+%array = OpTypeArray %int %int_4
+%struct = OpTypeStruct %array
+%ptr_ssbo_struct = OpTypePointer StorageBuffer %struct
+%var = OpVariable %ptr_ssbo_struct StorageBuffer
+%void_fn = OpTypeFunction %void
+%main = OpFunction %void None %void_fn
+%entry = OpLabel
+OpReturn
+OpFunctionEnd
+)";
+
+  CompileSuccessfully(spirv, SPV_ENV_VULKAN_1_1);
+  EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions(SPV_ENV_VULKAN_1_1));
+  EXPECT_THAT(getDiagnosticString(),
+              HasSubstr("contains an array with stride 0"));
+}
+
+TEST_F(ValidateDecorations, VulkanArrayStrideTooSmall) {
+  const std::string spirv = R"(
+OpCapability Shader
+OpMemoryModel Logical GLSL450
+OpEntryPoint GLCompute %main "main"
+OpExecutionMode %main LocalSize 1 1 1
+OpDecorate %var DescriptorSet 0
+OpDecorate %var Binding 0
+OpDecorate %struct Block
+OpMemberDecorate %struct 0 Offset 0
+OpDecorate %inner ArrayStride 4
+OpDecorate %outer ArrayStride 4
+%void = OpTypeVoid
+%int = OpTypeInt 32 0
+%int_4 = OpConstant %int 4
+%inner = OpTypeArray %int %int_4
+%outer = OpTypeArray %inner %int_4
+%struct = OpTypeStruct %outer
+%ptr_ssbo_struct = OpTypePointer StorageBuffer %struct
+%var = OpVariable %ptr_ssbo_struct StorageBuffer
+%void_fn = OpTypeFunction %void
+%main = OpFunction %void None %void_fn
+%entry = OpLabel
+OpReturn
+OpFunctionEnd
+)";
+
+  CompileSuccessfully(spirv, SPV_ENV_VULKAN_1_1);
+  EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions(SPV_ENV_VULKAN_1_1));
+  EXPECT_THAT(
+      getDiagnosticString(),
+      HasSubstr(
+          "contains an array with stride 4, but with an element size of 16"));
+}
+
 }  // namespace
 }  // namespace val
 }  // namespace spvtools

test/val/val_memory_test.cpp

@@ -2996,6 +2996,7 @@ OpExtension "SPV_EXT_descriptor_indexing"
 OpMemoryModel Logical GLSL450
 OpEntryPoint Fragment %func "func"
 OpExecutionMode %func OriginUpperLeft
+OpDecorate %inner_array_t ArrayStride 4
 OpDecorate %array_t ArrayStride 4
 OpMemberDecorate %struct_t 0 Offset 0
 OpDecorate %struct_t Block
@@ -3025,6 +3026,7 @@ OpExtension "SPV_EXT_descriptor_indexing"
 OpMemoryModel Logical GLSL450
 OpEntryPoint Fragment %func "func"
 OpExecutionMode %func OriginUpperLeft
+OpDecorate %inner_array_t ArrayStride 4
 OpDecorate %array_t ArrayStride 4
 OpMemberDecorate %struct_t 0 Offset 0
 OpDecorate %struct_t Block