More explicit layout validation (#5958) * Strengthen the checks that Block/BufferBlock cannot be nested in another Block/BufferBlock * previously missed arrays * Add check that arrays containing a Block/BufferBlock must not be decorated with array stride
diff --git a/source/val/validate_type.cpp b/source/val/validate_type.cpp index 432f12f..4cd7684 100644 --- a/source/val/validate_type.cpp +++ b/source/val/validate_type.cpp
@@ -215,6 +215,18 @@ << " is a void type."; } + if (_.HasCapability(spv::Capability::Shader)) { + if (element_type->opcode() == spv::Op::OpTypeStruct && + (_.HasDecoration(element_type->id(), spv::Decoration::Block) || + _.HasDecoration(element_type->id(), spv::Decoration::BufferBlock))) { + if (_.HasDecoration(inst->id(), spv::Decoration::ArrayStride)) { + return _.diag(SPV_ERROR_INVALID_ID, inst) + << "Array containing a Block or BufferBlock must not be " + "decorated with ArrayStride"; + } + } + } + if (spvIsVulkanEnv(_.context()->target_env) && element_type->opcode() == spv::Op::OpTypeRuntimeArray) { return _.diag(SPV_ERROR_INVALID_ID, inst) @@ -273,6 +285,18 @@ << " is a void type."; } + if (_.HasCapability(spv::Capability::Shader)) { + if (element_type->opcode() == spv::Op::OpTypeStruct && + (_.HasDecoration(element_type->id(), spv::Decoration::Block) || + _.HasDecoration(element_type->id(), spv::Decoration::BufferBlock))) { + if (_.HasDecoration(inst->id(), spv::Decoration::ArrayStride)) { + return _.diag(SPV_ERROR_INVALID_ID, inst) + << "Array containing a Block or BufferBlock must not be " + "decorated with ArrayStride"; + } + } + } + if (spvIsVulkanEnv(_.context()->target_env) && element_type->opcode() == spv::Op::OpTypeRuntimeArray) { return _.diag(SPV_ERROR_INVALID_ID, inst) @@ -343,13 +367,20 @@ // Struct members start at word 2 of OpTypeStruct instruction. for (size_t word_i = 2; word_i < inst->words().size(); ++word_i) { auto member = inst->word(word_i); - auto memberTypeInstr = _.FindDef(member); - if (memberTypeInstr && spv::Op::OpTypeStruct == memberTypeInstr->opcode()) { - if (_.HasDecoration(memberTypeInstr->id(), spv::Decoration::Block) || - _.HasDecoration(memberTypeInstr->id(), - spv::Decoration::BufferBlock) || - _.GetHasNestedBlockOrBufferBlockStruct(memberTypeInstr->id())) - has_nested_blockOrBufferBlock_struct = true; + if (_.ContainsType( + member, + [&_](const Instruction* type_inst) { + if (type_inst->opcode() == spv::Op::OpTypeStruct && + (_.HasDecoration(type_inst->id(), spv::Decoration::Block) || + _.HasDecoration(type_inst->id(), + spv::Decoration::BufferBlock))) { + return true; + } + return false; + }, + /* traverse_all_types = */ false)) { + has_nested_blockOrBufferBlock_struct = true; + break; } }
diff --git a/test/val/val_decoration_test.cpp b/test/val/val_decoration_test.cpp index 2154e2c..04533b2 100644 --- a/test/val/val_decoration_test.cpp +++ b/test/val/val_decoration_test.cpp
@@ -2076,6 +2076,68 @@ "another Block or BufferBlock.")); } +TEST_F(ValidateDecorations, BlockCannotAppearWithinBlockArray) { + const std::string spirv = R"( +OpCapability Shader +OpMemoryModel Logical GLSL450 +OpEntryPoint Vertex %main "main" +OpMemberDecorate %outer 0 Offset 0 +OpMemberDecorate %outer 1 Offset 4 +OpMemberDecorate %outer 2 Offset 20 +OpDecorate %outer Block +OpMemberDecorate %inner 0 Offset 0 +OpDecorate %inner Block +%void = OpTypeVoid +%void_fn = OpTypeFunction %void +%int = OpTypeInt 32 0 +%int_4 = OpConstant %int 4 +%inner = OpTypeStruct %int +%array = OpTypeArray %inner %int_4 +%outer = OpTypeStruct %int %array %int +%main = OpFunction %void None %void_fn +%entry = OpLabel +OpReturn +OpFunctionEnd +)"; + + CompileSuccessfully(spirv); + EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions()); + EXPECT_THAT(getDiagnosticString(), + HasSubstr("rules: A Block or BufferBlock cannot be nested within " + "another Block or BufferBlock.")); +} + +TEST_F(ValidateDecorations, BlockCannotAppearWithinBlockMultiArray) { + const std::string spirv = R"( +OpCapability Shader +OpMemoryModel Logical GLSL450 +OpEntryPoint Vertex %main "main" +OpMemberDecorate %outer 0 Offset 0 +OpMemberDecorate %outer 1 Offset 4 +OpDecorate %outer Block +OpMemberDecorate %inner 0 Offset 0 +OpDecorate %inner Block +%void = OpTypeVoid +%void_fn = OpTypeFunction %void +%int = OpTypeInt 32 0 +%int_4 = OpConstant %int 4 +%inner = OpTypeStruct %int +%array1 = OpTypeArray %inner %int_4 +%array2 = OpTypeArray %array1 %int_4 +%outer = OpTypeStruct %int %array2 +%main = OpFunction %void None %void_fn +%entry = OpLabel +OpReturn +OpFunctionEnd +)"; + + CompileSuccessfully(spirv); + EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions()); + EXPECT_THAT(getDiagnosticString(), + HasSubstr("rules: A Block or BufferBlock cannot be nested within " + "another Block or BufferBlock.")); +} + TEST_F(ValidateDecorations, BlockLayoutForbidsTightScalarVec3PackingBad) { // See https://github.com/KhronosGroup/SPIRV-Tools/issues/1666 std::string spirv = R"( @@ -10593,6 +10655,47 @@ "Decorations taking ID parameters may not be used with OpDecorate")); } +TEST_F(ValidateDecorations, BlockArrayWithStride) { + const std::string spirv = R"( +OpCapability Shader +OpCapability Linkage +OpMemoryModel Logical GLSL450 +OpDecorate %struct Block +OpMemberDecorate %struct 0 Offset 0 +OpDecorate %array ArrayStride 4 +%int = OpTypeInt 32 0 +%int_4 = OpConstant %int 4 +%struct = OpTypeStruct %int +%array = OpTypeArray %struct %int_4 +)"; + + CompileSuccessfully(spirv); + EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions()); + EXPECT_THAT(getDiagnosticString(), + HasSubstr("Array containing a Block or BufferBlock must not be " + "decorated with ArrayStride")); +} + +TEST_F(ValidateDecorations, BufferBlockRuntimeArrayWithStride) { + const std::string spirv = R"( +OpCapability Shader +OpCapability Linkage +OpMemoryModel Logical GLSL450 +OpDecorate %struct BufferBlock +OpMemberDecorate %struct 0 Offset 0 +OpDecorate %array ArrayStride 4 +%int = OpTypeInt 32 0 +%struct = OpTypeStruct %int +%array = OpTypeRuntimeArray %struct +)"; + + CompileSuccessfully(spirv); + EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions()); + EXPECT_THAT(getDiagnosticString(), + HasSubstr("Array containing a Block or BufferBlock must not be " + "decorated with ArrayStride")); +} + } // namespace } // namespace val } // namespace spvtools
diff --git a/test/val/val_memory_test.cpp b/test/val/val_memory_test.cpp index c5a77e7..06eaff0 100644 --- a/test/val/val_memory_test.cpp +++ b/test/val/val_memory_test.cpp
@@ -3114,7 +3114,6 @@ OpMemoryModel Logical GLSL450 OpEntryPoint Fragment %func "func" OpExecutionMode %func OriginUpperLeft -OpDecorate %array_t ArrayStride 4 OpMemberDecorate %struct_t 0 Offset 0 OpDecorate %struct_t Block %uint_t = OpTypeInt 32 0 @@ -3360,7 +3359,6 @@ 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 %uint_t = OpTypeInt 32 0 @@ -3390,7 +3388,6 @@ 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 %uint_t = OpTypeInt 32 0