From 9c71c572e5a1fcd5de8a926571b4122aa10a0c6c Mon Sep 17 00:00:00 2001 From: Ehsan Nasiri Date: Mon, 16 Jan 2017 12:54:15 -0500 Subject: [PATCH] Check BuiltIn Decoration rules. When applied to a structure-type member, all members of that structure type must also be decorated with BuiltIn. (No allowed mixing of built-in variables and non-built-in variables within a single structure.) When applied to a structure-type member, that structure type cannot be contained as a member of another structure type. There is at most one object per Storage Class that can contain a structure type containing members decorated with BuiltIn, consumed per entry-point. --- source/val/decoration.h | 1 + source/val/validation_state.h | 35 ++++++- source/validate.cpp | 8 +- source/validate_decorations.cpp | 61 +++++++++++- source/validate_id.cpp | 36 ++++++- test/val/val_decoration_test.cpp | 159 +++++++++++++++++++++++++++++++ test/val/val_id_test.cpp | 51 ++++++++++ 7 files changed, 347 insertions(+), 4 deletions(-) diff --git a/source/val/decoration.h b/source/val/decoration.h index 01974af92..a6d7ff955 100644 --- a/source/val/decoration.h +++ b/source/val/decoration.h @@ -61,6 +61,7 @@ class Decoration { void set_struct_member_index(uint32_t index) { struct_member_index_ = index; } int struct_member_index() { return struct_member_index_; } + int struct_member_index() const { return struct_member_index_; } SpvDecoration dec_type() { return dec_type_; } SpvDecoration dec_type() const { return dec_type_; } std::vector& params() { return params_; } diff --git a/source/val/validation_state.h b/source/val/validation_state.h index 426be45e3..3900487da 100644 --- a/source/val/validation_state.h +++ b/source/val/validation_state.h @@ -126,10 +126,28 @@ class ValidationState_t { /// instruction bool in_block() const; + /// Registers the given as an Entry Point. + void RegisterEntryPointId(const uint32_t id) { + entry_points_.push_back(id); + entry_point_interfaces_.insert(std::make_pair(id, std::vector())); + } + /// Returns a list of entry point function ids - std::vector& entry_points() { return entry_points_; } const std::vector& entry_points() const { return entry_points_; } + /// Adds a new interface id to the interfaces of the given entry point. + void RegisterInterfaceForEntryPoint(uint32_t entry_point, + uint32_t interface) { + entry_point_interfaces_[entry_point].push_back(interface); + } + + /// Returns the interfaces of a given entry point. If the given id is not a + /// valid Entry Point id, std::out_of_range exception is thrown. + const std::vector& entry_point_interfaces( + uint32_t entry_point) const { + return entry_point_interfaces_.at(entry_point); + } + /// Inserts an to the set of functions that are target of OpFunctionCall. void AddFunctionCallTarget(const uint32_t id) { function_call_targets_.insert(id); @@ -263,6 +281,15 @@ class ValidationState_t { return struct_nesting_depth_[id]; } + /// Records that the structure type has a member decorated with a built-in. + void RegisterStructTypeWithBuiltInMember(uint32_t id) { + builtin_structs_.insert(id); + } + + /// Returns true if the struct type with the given Id has a BuiltIn member. + bool IsStructTypeWithBuiltInMember(uint32_t id) const { + return (builtin_structs_.find(id) != builtin_structs_.end()); + } private: ValidationState_t(const ValidationState_t&); @@ -303,6 +330,9 @@ class ValidationState_t { /// IDs that are entry points, ie, arguments to OpEntryPoint. std::vector entry_points_; + /// Maps an entry point id to its interfaces. + std::unordered_map> entry_point_interfaces_; + /// Functions IDs that are target of OpFunctionCall. std::unordered_set function_call_targets_; @@ -315,6 +345,9 @@ class ValidationState_t { /// Set of Local Variable IDs ('Function' Storage Class) std::unordered_set local_vars_; + /// Set of struct types that have members with a BuiltIn decoration. + std::unordered_set builtin_structs_; + /// Structure Nesting Depth std::unordered_map struct_nesting_depth_; diff --git a/source/validate.cpp b/source/validate.cpp index a0830fe33..7757f973b 100644 --- a/source/validate.cpp +++ b/source/validate.cpp @@ -121,7 +121,13 @@ spv_result_t ProcessInstruction(void* user_data, ValidationState_t& _ = *(reinterpret_cast(user_data)); _.increment_instruction_count(); if (static_cast(inst->opcode) == SpvOpEntryPoint) { - _.entry_points().push_back(inst->words[2]); + const auto entry_point = inst->words[2]; + _.RegisterEntryPointId(entry_point); + // Operand 3 and later are the of interfaces for the entry point. + for (int i = 3; i < inst->num_operands; ++i) { + _.RegisterInterfaceForEntryPoint(entry_point, + inst->words[inst->operands[i].offset]); + } } if (static_cast(inst->opcode) == SpvOpFunctionCall) { _.AddFunctionCallTarget(inst->words[3]); diff --git a/source/validate_decorations.cpp b/source/validate_decorations.cpp index 39e9d66ea..d7e56e6b1 100644 --- a/source/validate_decorations.cpp +++ b/source/validate_decorations.cpp @@ -14,15 +14,32 @@ #include "validate.h" +#include #include #include "diagnostic.h" #include "opcode.h" #include "val/validation_state.h" +using libspirv::Decoration; using libspirv::DiagnosticStream; using libspirv::ValidationState_t; +namespace { + +// Returns whether the given structure type has any members with BuiltIn +// decoration. +bool isBuiltInStruct(uint32_t struct_id, ValidationState_t& vstate) { + const auto& decorations = vstate.id_decorations(struct_id); + return std::any_of( + decorations.begin(), decorations.end(), [](const Decoration& d) { + return SpvDecorationBuiltIn == d.dec_type() && + Decoration::kInvalidMember != d.struct_member_index(); + }); +} + +} // anonymous namespace + namespace libspirv { // Validates that decorations have been applied properly. @@ -48,7 +65,49 @@ spv_result_t ValidateDecorations(ValidationState_t& vstate) { } } - // TODO: Add more decoration validation code here. + for (uint32_t entry_point : vstate.entry_points()) { + const auto& interfaces = vstate.entry_point_interfaces(entry_point); + int num_builtin_inputs = 0; + int num_builtin_outputs = 0; + for (auto interface : interfaces) { + Instruction* var_instr = vstate.FindDef(interface); + if (SpvOpVariable != var_instr->opcode()) { + return vstate.diag(SPV_ERROR_INVALID_ID) + << "Interfaces passed to OpEntryPoint must be of type " + "OpTypeVariable. Found Op" + << spvOpcodeString(static_cast(var_instr->opcode())) + << "."; + } + const uint32_t ptr_id = var_instr->word(1); + Instruction* ptr_instr = vstate.FindDef(ptr_id); + // It is guaranteed (by validator ID checks) that ptr_instr is + // OpTypePointer. Word 3 of this instruction is the type being pointed to. + const uint32_t type_id = ptr_instr->word(3); + Instruction* type_instr = vstate.FindDef(type_id); + const auto storage_class = + static_cast(var_instr->word(3)); + if (storage_class != SpvStorageClassInput && + storage_class != SpvStorageClassOutput) { + return vstate.diag(SPV_ERROR_INVALID_ID) + << "OpEntryPoint interfaces must be OpVariables with " + "Storage Class of Input(1) or Output(3). Found Storage Class " + << storage_class << " for Entry Point id " << entry_point << "."; + } + if (type_instr && SpvOpTypeStruct == type_instr->opcode() && + isBuiltInStruct(type_id, vstate)) { + if (storage_class == SpvStorageClassInput) ++num_builtin_inputs; + if (storage_class == SpvStorageClassOutput) ++num_builtin_outputs; + if (num_builtin_inputs > 1 || num_builtin_outputs > 1) break; + } + } + if (num_builtin_inputs > 1 || num_builtin_outputs > 1) { + return vstate.diag(SPV_ERROR_INVALID_BINARY) + << "There must be at most one object per Storage Class that can " + "contain a structure type containing members decorated with " + "BuiltIn, consumed per entry-point. Entry Point id " + << entry_point << " does not meet this requirement."; + } + } return SPV_SUCCESS; } diff --git a/source/validate_id.cpp b/source/validate_id.cpp index 503fea6d1..5b7057434 100644 --- a/source/validate_id.cpp +++ b/source/validate_id.cpp @@ -31,6 +31,7 @@ #include "val/validation_state.h" using libspirv::ValidationState_t; +using libspirv::Decoration; using std::function; using std::ignore; using std::make_pair; @@ -386,6 +387,8 @@ bool idUsage::isValid(const spv_instruction_t* inst, template <> bool idUsage::isValid(const spv_instruction_t* inst, const spv_opcode_desc) { + ValidationState_t& vstate = const_cast(module_); + const uint32_t struct_id = inst->words[1]; for (size_t memberTypeIndex = 2; memberTypeIndex < inst->words.size(); ++memberTypeIndex) { auto memberTypeId = inst->words[memberTypeIndex]; @@ -396,7 +399,17 @@ bool idUsage::isValid(const spv_instruction_t* inst, << "' is not a type."; return false; } - if (memberType && module_.IsForwardPointer(memberTypeId)) { + if (SpvOpTypeStruct == memberType->opcode() && + module_.IsStructTypeWithBuiltInMember(memberTypeId)) { + DIAG(memberTypeIndex) + << "Structure " << memberTypeId + << " contains members with BuiltIn decoration. Therefore this " + "structure may not be contained as a member of another structure " + "type. Structure " + << struct_id << " contains structure " << memberTypeId << "."; + return false; + } + if (module_.IsForwardPointer(memberTypeId)) { if (memberType->opcode() != SpvOpTypePointer) { DIAG(memberTypeIndex) << "Found a forward reference to a non-pointer " "type in OpTypeStruct instruction."; @@ -418,6 +431,27 @@ bool idUsage::isValid(const spv_instruction_t* inst, } } } + std::unordered_set built_in_members; + for (auto decoration : vstate.id_decorations(struct_id)) { + if (decoration.dec_type() == SpvDecorationBuiltIn && + decoration.struct_member_index() != Decoration::kInvalidMember) { + built_in_members.insert(decoration.struct_member_index()); + } + } + int num_struct_members = static_cast(inst->words.size() - 2); + int num_builtin_members = static_cast(built_in_members.size()); + if (num_builtin_members > 0 && num_builtin_members != num_struct_members) { + DIAG(0) + << "When BuiltIn decoration is applied to a structure-type member, " + "all members of that structure type must also be decorated with " + "BuiltIn (No allowed mixing of built-in variables and " + "non-built-in variables within a single structure). Structure id " + << struct_id << " does not meet this requirement."; + return false; + } + if (num_builtin_members > 0) { + vstate.RegisterStructTypeWithBuiltInMember(struct_id); + } return true; } diff --git a/test/val/val_decoration_test.cpp b/test/val/val_decoration_test.cpp index 7fc886ac5..831595974 100644 --- a/test/val/val_decoration_test.cpp +++ b/test/val/val_decoration_test.cpp @@ -178,5 +178,164 @@ TEST_F(ValidateDecorations, LinkageExportUsedForInitializedVariableGood) { EXPECT_EQ(SPV_SUCCESS, ValidateAndRetrieveValidationState()); } +TEST_F(ValidateDecorations, StructAllMembersHaveBuiltInDecorationsGood) { + string spirv = R"( + OpCapability Shader + OpCapability Linkage + OpMemoryModel Logical GLSL450 + OpMemberDecorate %_struct_1 0 BuiltIn Position + OpMemberDecorate %_struct_1 1 BuiltIn Position + OpMemberDecorate %_struct_1 2 BuiltIn Position + OpMemberDecorate %_struct_1 3 BuiltIn Position + %float = OpTypeFloat 32 +%_runtimearr = OpTypeRuntimeArray %float + %_struct_1 = OpTypeStruct %float %float %float %_runtimearr + )"; + CompileSuccessfully(spirv); + EXPECT_EQ(SPV_SUCCESS, ValidateAndRetrieveValidationState()); +} + +TEST_F(ValidateDecorations, MixedBuiltInDecorationsBad) { + string spirv = R"( + OpCapability Shader + OpCapability Linkage + OpMemoryModel Logical GLSL450 + OpMemberDecorate %_struct_1 0 BuiltIn Position + OpMemberDecorate %_struct_1 1 BuiltIn Position + %float = OpTypeFloat 32 +%_runtimearr = OpTypeRuntimeArray %float + %_struct_1 = OpTypeStruct %float %float %float %_runtimearr + )"; + CompileSuccessfully(spirv); + EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateAndRetrieveValidationState()); + EXPECT_THAT( + getDiagnosticString(), + HasSubstr("When BuiltIn decoration is applied to a structure-type " + "member, all members of that structure type must also be " + "decorated with BuiltIn (No allowed mixing of built-in " + "variables and non-built-in variables within a single " + "structure). Structure id 1 does not meet this requirement.")); +} + +TEST_F(ValidateDecorations, StructContainsBuiltInStructBad) { + string spirv = R"( + OpCapability Shader + OpCapability Linkage + OpMemoryModel Logical GLSL450 + OpMemberDecorate %_struct_1 0 BuiltIn Position + OpMemberDecorate %_struct_1 1 BuiltIn Position + OpMemberDecorate %_struct_1 2 BuiltIn Position + OpMemberDecorate %_struct_1 3 BuiltIn Position + %float = OpTypeFloat 32 +%_runtimearr = OpTypeRuntimeArray %float + %_struct_1 = OpTypeStruct %float %float %float %_runtimearr + %_struct_2 = OpTypeStruct %_struct_1 + )"; + CompileSuccessfully(spirv); + EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateAndRetrieveValidationState()); + EXPECT_THAT(getDiagnosticString(), + HasSubstr("Structure 1 contains members with BuiltIn " + "decoration. Therefore this structure may not be " + "contained as a member of another structure type. " + "Structure 4 contains structure 1.")); +} + +TEST_F(ValidateDecorations, StructContainsNonBuiltInStructGood) { + string spirv = R"( + OpCapability Shader + OpCapability Linkage + OpMemoryModel Logical GLSL450 + %float = OpTypeFloat 32 + %_struct_1 = OpTypeStruct %float + %_struct_2 = OpTypeStruct %_struct_1 + )"; + CompileSuccessfully(spirv); + EXPECT_EQ(SPV_SUCCESS, ValidateAndRetrieveValidationState()); +} + +TEST_F(ValidateDecorations, MultipleBuiltInObjectsConsumedByOpEntryPointBad) { + string spirv = R"( + OpCapability Shader + OpCapability Geometry + OpMemoryModel Logical GLSL450 + OpEntryPoint Geometry %main "main" %in_1 %in_2 + OpMemberDecorate %struct_1 0 BuiltIn InvocationId + OpMemberDecorate %struct_2 0 BuiltIn Position + %int = OpTypeInt 32 1 + %void = OpTypeVoid + %func = OpTypeFunction %void + %float = OpTypeFloat 32 + %struct_1 = OpTypeStruct %int + %struct_2 = OpTypeStruct %float +%ptr_builtin_1 = OpTypePointer Input %struct_1 +%ptr_builtin_2 = OpTypePointer Input %struct_2 +%in_1 = OpVariable %ptr_builtin_1 Input +%in_2 = OpVariable %ptr_builtin_2 Input + %main = OpFunction %void None %func + %5 = OpLabel + OpReturn + OpFunctionEnd + )"; + CompileSuccessfully(spirv); + EXPECT_EQ(SPV_ERROR_INVALID_BINARY, ValidateAndRetrieveValidationState()); + EXPECT_THAT(getDiagnosticString(), + HasSubstr("There must be at most one object per Storage Class " + "that can contain a structure type containing members " + "decorated with BuiltIn, consumed per entry-point.")); +} + +TEST_F(ValidateDecorations, + OneBuiltInObjectPerStorageClassConsumedByOpEntryPointGood) { + string spirv = R"( + OpCapability Shader + OpCapability Geometry + OpMemoryModel Logical GLSL450 + OpEntryPoint Geometry %main "main" %in_1 %out_1 + OpMemberDecorate %struct_1 0 BuiltIn InvocationId + OpMemberDecorate %struct_2 0 BuiltIn Position + %int = OpTypeInt 32 1 + %void = OpTypeVoid + %func = OpTypeFunction %void + %float = OpTypeFloat 32 + %struct_1 = OpTypeStruct %int + %struct_2 = OpTypeStruct %float +%ptr_builtin_1 = OpTypePointer Input %struct_1 +%ptr_builtin_2 = OpTypePointer Output %struct_2 +%in_1 = OpVariable %ptr_builtin_1 Input +%out_1 = OpVariable %ptr_builtin_2 Output + %main = OpFunction %void None %func + %5 = OpLabel + OpReturn + OpFunctionEnd + )"; + CompileSuccessfully(spirv); + EXPECT_EQ(SPV_SUCCESS, ValidateAndRetrieveValidationState()); +} + +TEST_F(ValidateDecorations, NoBuiltInObjectsConsumedByOpEntryPointGood) { + string spirv = R"( + OpCapability Shader + OpCapability Geometry + OpMemoryModel Logical GLSL450 + OpEntryPoint Geometry %main "main" %in_1 %out_1 + %int = OpTypeInt 32 1 + %void = OpTypeVoid + %func = OpTypeFunction %void + %float = OpTypeFloat 32 + %struct_1 = OpTypeStruct %int + %struct_2 = OpTypeStruct %float +%ptr_builtin_1 = OpTypePointer Input %struct_1 +%ptr_builtin_2 = OpTypePointer Output %struct_2 +%in_1 = OpVariable %ptr_builtin_1 Input +%out_1 = OpVariable %ptr_builtin_2 Output + %main = OpFunction %void None %func + %5 = OpLabel + OpReturn + OpFunctionEnd + )"; + CompileSuccessfully(spirv); + EXPECT_EQ(SPV_SUCCESS, ValidateAndRetrieveValidationState()); +} + } // anonymous namespace diff --git a/test/val/val_id_test.cpp b/test/val/val_id_test.cpp index 35df274b8..8855394e1 100644 --- a/test/val/val_id_test.cpp +++ b/test/val/val_id_test.cpp @@ -326,6 +326,57 @@ TEST_F(ValidateIdWithMessage, OpEntryPointReturnTypeBad) { EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions()); } +TEST_F(ValidateIdWithMessage, OpEntryPointInterfaceIsNotVariableTypeBad) { + string spirv = R"( + OpCapability Shader + OpCapability Geometry + OpMemoryModel Logical GLSL450 + OpEntryPoint Geometry %main "main" %ptr_builtin_1 + OpMemberDecorate %struct_1 0 BuiltIn InvocationId + %int = OpTypeInt 32 1 + %void = OpTypeVoid + %func = OpTypeFunction %void + %struct_1 = OpTypeStruct %int +%ptr_builtin_1 = OpTypePointer Input %struct_1 + %main = OpFunction %void None %func + %5 = OpLabel + OpReturn + OpFunctionEnd + )"; + CompileSuccessfully(spirv); + EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions()); + EXPECT_THAT(getDiagnosticString(), + HasSubstr("Interfaces passed to OpEntryPoint must be of type " + "OpTypeVariable. Found OpTypePointer.")); +} + + +TEST_F(ValidateIdWithMessage, OpEntryPointInterfaceStorageClassBad) { + string spirv = R"( + OpCapability Shader + OpCapability Geometry + OpMemoryModel Logical GLSL450 + OpEntryPoint Geometry %main "main" %in_1 + OpMemberDecorate %struct_1 0 BuiltIn InvocationId + %int = OpTypeInt 32 1 + %void = OpTypeVoid + %func = OpTypeFunction %void + %struct_1 = OpTypeStruct %int +%ptr_builtin_1 = OpTypePointer Uniform %struct_1 + %in_1 = OpVariable %ptr_builtin_1 Uniform + %main = OpFunction %void None %func + %5 = OpLabel + OpReturn + OpFunctionEnd + )"; + CompileSuccessfully(spirv); + EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions()); + EXPECT_THAT(getDiagnosticString(), + HasSubstr("OpEntryPoint interfaces must be OpVariables with " + "Storage Class of Input(1) or Output(3). Found Storage " + "Class 2 for Entry Point id 1.")); +} + TEST_F(ValidateIdWithMessage, OpExecutionModeGood) { string spirv = kGLSL450MemoryModel + R"( OpEntryPoint GLCompute %3 ""