From cf011f990163ebb3c61252dea149cd9e1799bdf2 Mon Sep 17 00:00:00 2001 From: alan-baker <33432579+alan-baker@users.noreply.github.com> Date: Thu, 24 Jan 2019 17:24:31 -0500 Subject: [PATCH] More layout check fixes (#2315) * check array strides for multidimensional arrays * check layouts of structs in arrays for multiple indices * new tests --- source/val/validate_decorations.cpp | 60 ++++++++++++++++------ test/val/val_decoration_test.cpp | 80 ++++++++++++++++++++++++++++- 2 files changed, 124 insertions(+), 16 deletions(-) diff --git a/source/val/validate_decorations.cpp b/source/val/validate_decorations.cpp index 6dd775d3c..164a5ddc9 100644 --- a/source/val/validate_decorations.cpp +++ b/source/val/validate_decorations.cpp @@ -509,23 +509,53 @@ spv_result_t checkLayout(uint32_t struct_id, const char* storage_class_str, << " not satisfying alignment to " << alignment; } } - // Check arrays and runtime arrays. - if (SpvOpTypeArray == opcode || SpvOpTypeRuntimeArray == opcode) { - const auto typeId = inst->word(2); - const auto arrayInst = vstate.FindDef(typeId); - if (SpvOpTypeStruct == arrayInst->opcode() && - SPV_SUCCESS != (recursive_status = checkLayout( - typeId, storage_class_str, decoration_str, - blockRules, offset, constraints, vstate))) - return recursive_status; + + // Check arrays and runtime arrays recursively. + auto array_inst = inst; + auto array_alignment = alignment; + while (array_inst->opcode() == SpvOpTypeArray || + array_inst->opcode() == SpvOpTypeRuntimeArray) { + const auto typeId = array_inst->word(2); + const auto element_inst = vstate.FindDef(typeId); // Check array stride. - for (auto& decoration : vstate.id_decorations(id)) { - if (SpvDecorationArrayStride == decoration.dec_type() && - !IsAlignedTo(decoration.params()[0], alignment)) - return fail(memberIdx) - << "is an array with stride " << decoration.params()[0] - << " not satisfying alignment to " << alignment; + auto array_stride = 0; + for (auto& decoration : vstate.id_decorations(array_inst->id())) { + if (SpvDecorationArrayStride == decoration.dec_type()) { + array_stride = decoration.params()[0]; + if (!IsAlignedTo(array_stride, array_alignment)) + return fail(memberIdx) + << "contains an array with stride " << decoration.params()[0] + << " not satisfying alignment to " << alignment; + } } + + bool is_int32 = false; + bool is_const = false; + uint32_t num_elements = 0; + if (array_inst->opcode() == SpvOpTypeArray) { + std::tie(is_int32, is_const, num_elements) = + vstate.EvalInt32IfConst(array_inst->word(3)); + } + num_elements = std::max(1u, num_elements); + // Check each element recursively if it is a struct. There is a + // limitation to this check if the array size is a spec constant or is a + // runtime array then we will only check a single element. This means + // some improper straddles might be missed. + for (uint32_t i = 0; i < num_elements; ++i) { + uint32_t next_offset = i * array_stride + offset; + if (SpvOpTypeStruct == element_inst->opcode() && + SPV_SUCCESS != (recursive_status = checkLayout( + typeId, storage_class_str, decoration_str, + blockRules, next_offset, constraints, vstate))) + return recursive_status; + } + + // Proceed to the element in case it is an array. + array_inst = element_inst; + array_alignment = scalar_block_layout + ? getScalarAlignment(array_inst->id(), vstate) + : getBaseAlignment(array_inst->id(), blockRules, + constraint, constraints, vstate); } nextValidOffset = offset + size; if (!scalar_block_layout && blockRules && diff --git a/test/val/val_decoration_test.cpp b/test/val/val_decoration_test.cpp index 4c7a80e41..1506a29a6 100644 --- a/test/val/val_decoration_test.cpp +++ b/test/val/val_decoration_test.cpp @@ -3418,7 +3418,8 @@ TEST_F(ValidateDecorations, BlockUniformBufferLayoutIncorrectArrayStrideBad) { getDiagnosticString(), HasSubstr( "Structure id 6 decorated as Block for variable in Uniform storage " - "class must follow standard uniform buffer layout rules: member 4 is " + "class must follow standard uniform buffer layout rules: member 4 " + "contains " "an array with stride 49 not satisfying alignment to 16")); } @@ -5413,6 +5414,83 @@ OpFunctionEnd "not aligned to 8")); } +TEST_F(ValidateDecorations, MultiDimensionalArray) { + const std::string spirv = R"( +OpCapability Shader +OpMemoryModel Logical GLSL450 +OpEntryPoint GLCompute %main "main" +OpExecutionMode %main LocalSize 1 1 1 +OpDecorate %struct Block +OpMemberDecorate %struct 0 Offset 0 +OpDecorate %array_4 ArrayStride 4 +OpDecorate %array_3 ArrayStride 48 +OpDecorate %var DescriptorSet 0 +OpDecorate %var Binding 0 +%void = OpTypeVoid +%int = OpTypeInt 32 0 +%int_3 = OpConstant %int 3 +%int_4 = OpConstant %int 4 +%array_4 = OpTypeArray %int %int_4 +%array_3 = OpTypeArray %array_4 %int_3 +%struct = OpTypeStruct %array_3 +%ptr_struct = OpTypePointer Uniform %struct +%var = OpVariable %ptr_struct Uniform +%void_fn = OpTypeFunction %void +%main = OpFunction %void None %void_fn +%entry = OpLabel +OpReturn +OpFunctionEnd +)"; + + CompileSuccessfully(spirv, SPV_ENV_VULKAN_1_0); + EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions(SPV_ENV_VULKAN_1_0)); + EXPECT_THAT(getDiagnosticString(), + HasSubstr("Structure id 2 decorated as Block for variable in " + "Uniform storage class must follow standard uniform " + "buffer layout rules: member 0 contains an array with " + "stride 4 not satisfying alignment to 16")); +} + +TEST_F(ValidateDecorations, ImproperStraddleInArray) { + const std::string spirv = R"( +OpCapability Shader +OpMemoryModel Logical GLSL450 +OpEntryPoint GLCompute %main "main" +OpExecutionMode %main LocalSize 1 1 1 +OpDecorate %struct Block +OpMemberDecorate %struct 0 Offset 0 +OpDecorate %array ArrayStride 24 +OpMemberDecorate %inner 0 Offset 0 +OpMemberDecorate %inner 1 Offset 4 +OpMemberDecorate %inner 2 Offset 12 +OpMemberDecorate %inner 3 Offset 16 +OpDecorate %var DescriptorSet 0 +OpDecorate %var Binding 0 +%void = OpTypeVoid +%int = OpTypeInt 32 0 +%int_2 = OpConstant %int 2 +%int2 = OpTypeVector %int 2 +%inner = OpTypeStruct %int %int2 %int %int +%array = OpTypeArray %inner %int_2 +%struct = OpTypeStruct %array +%ptr_struct = OpTypePointer StorageBuffer %struct +%var = OpVariable %ptr_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("Structure id 4 decorated as Block for variable in " + "StorageBuffer storage class must follow relaxed " + "storage buffer layout rules: member 1 is an " + "improperly straddling vector at offset 28")); +} + } // namespace } // namespace val } // namespace spvtools