diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index d23767aa88..faec6b1314 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -467,6 +467,7 @@ if(${TINT_BUILD_TESTS}) inspector/inspector_test.cc intrinsic_table_test.cc program_test.cc + resolver/decoration_validation_test.cc resolver/intrinsic_test.cc resolver/is_storeable_test.cc resolver/resolver_test_helper.cc @@ -474,6 +475,7 @@ if(${TINT_BUILD_TESTS}) resolver/resolver_test.cc resolver/struct_layout_test.cc resolver/struct_storage_class_use_test.cc + resolver/type_validation_test.cc resolver/validation_test.cc scope_stack_test.cc semantic/sem_intrinsic_test.cc diff --git a/src/diagnostic/diagnostic.h b/src/diagnostic/diagnostic.h index 79d978ffe5..4b0c824913 100644 --- a/src/diagnostic/diagnostic.h +++ b/src/diagnostic/diagnostic.h @@ -110,21 +110,35 @@ class List { /// adds the error message without a source to the end of this list. /// @param err_msg the error message - void add_error(const std::string& err_msg) { + void add_error(std::string err_msg) { diag::Diagnostic error{}; error.severity = diag::Severity::Error; - error.message = err_msg; + error.message = std::move(err_msg); add(std::move(error)); } /// adds the error message with the given Source to the end of this list. /// @param err_msg the error message /// @param source the source of the error diagnostic - void add_error(const std::string& err_msg, const Source& source) { + void add_error(std::string err_msg, const Source& source) { diag::Diagnostic error{}; error.severity = diag::Severity::Error; error.source = source; - error.message = err_msg; + error.message = std::move(err_msg); + add(std::move(error)); + } + + /// adds the error message with the given code and Source to the end of this + /// list. + /// @param code the error code + /// @param err_msg the error message + /// @param source the source of the error diagnostic + void add_error(const char* code, std::string err_msg, const Source& source) { + diag::Diagnostic error{}; + error.code = code; + error.severity = diag::Severity::Error; + error.source = source; + error.message = std::move(err_msg); add(std::move(error)); } diff --git a/src/inspector/inspector_test.cc b/src/inspector/inspector_test.cc index c1a3a42377..436381522e 100644 --- a/src/inspector/inspector_test.cc +++ b/src/inspector/inspector_test.cc @@ -222,6 +222,17 @@ class InspectorHelper : public ProgramBuilder { return struct_type; } + /// Returns true if the struct with `member_types` requires a block decoration + /// @param member_types a vector of member types + /// @returns true if block decoration is required + bool StructRequiresBlockDecoration( + std::vector member_types) const { + // Structure needs a [[block]] attribute if the last member is a + // dynamically-sized array. + return member_types.back()->Is( + [](auto&& a) { return a->IsRuntimeArray(); }); + } + /// Generates types appropriate for using in a storage buffer /// @param name name for the type /// @param member_types a vector of member types @@ -231,7 +242,8 @@ class InspectorHelper : public ProgramBuilder { std::tuple MakeStorageBufferTypes( const std::string& name, std::vector member_types) { - auto* struct_type = MakeStructType(name, member_types, false); + bool is_block = StructRequiresBlockDecoration(member_types); + auto* struct_type = MakeStructType(name, member_types, is_block); auto* access_type = create( ast::AccessControl::kReadWrite, struct_type); return {struct_type, std::move(access_type)}; @@ -246,7 +258,8 @@ class InspectorHelper : public ProgramBuilder { std::tuple MakeReadOnlyStorageBufferTypes(const std::string& name, std::vector member_types) { - auto* struct_type = MakeStructType(name, member_types, false); + bool is_block = StructRequiresBlockDecoration(member_types); + auto* struct_type = MakeStructType(name, member_types, is_block); auto* access_type = create(ast::AccessControl::kReadOnly, struct_type); return {struct_type, std::move(access_type)}; diff --git a/src/resolver/decoration_validation_test.cc b/src/resolver/decoration_validation_test.cc new file mode 100644 index 0000000000..b1a45c18e8 --- /dev/null +++ b/src/resolver/decoration_validation_test.cc @@ -0,0 +1,205 @@ +// Copyright 2021 The Tint Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "src/ast/access_decoration.h" +#include "src/ast/constant_id_decoration.h" +#include "src/ast/stage_decoration.h" +#include "src/ast/struct_block_decoration.h" +#include "src/ast/workgroup_decoration.h" +#include "src/resolver/resolver.h" +#include "src/resolver/resolver_test_helper.h" + +#include "gmock/gmock.h" + +namespace tint { +namespace { + +enum class DecorationKind { + kAccess, + kAlign, + kBinding, + kBuiltin, + kConstantId, + kGroup, + kLocation, + kOffset, + kSize, + kStage, + kStride, + kStructBlock, + kWorkgroup, +}; +struct TestParams { + DecorationKind kind; + bool should_pass; +}; +class TestWithParams : public resolver::TestHelper, + public testing::TestWithParam {}; + +ast::Decoration* createDecoration(const Source& source, + ProgramBuilder& builder, + DecorationKind kind) { + switch (kind) { + case DecorationKind::kAccess: + return builder.create( + source, ast::AccessControl::kReadOnly); + case DecorationKind::kAlign: + return builder.create(source, 4u); + case DecorationKind::kBinding: + return builder.create(source, 1); + case DecorationKind::kBuiltin: + return builder.create(source, + ast::Builtin::kPosition); + case DecorationKind::kConstantId: + return builder.create(source, 0u); + case DecorationKind::kGroup: + return builder.create(source, 1u); + case DecorationKind::kLocation: + return builder.create(source, 1); + case DecorationKind::kOffset: + return builder.create(source, 4u); + case DecorationKind::kSize: + return builder.create(source, 4u); + case DecorationKind::kStage: + return builder.create(source, + ast::PipelineStage::kCompute); + case DecorationKind::kStride: + return builder.create(source, 4u); + case DecorationKind::kStructBlock: + return builder.create(source); + case DecorationKind::kWorkgroup: + return builder.create(source, 1u, 1u, 1u); + } + return nullptr; +} + +using ArrayDecorationTest = TestWithParams; + +TEST_P(ArrayDecorationTest, IsValid) { + auto params = GetParam(); + + ast::StructMemberList members{Member( + "a", create(ty.f32(), 0, + ast::DecorationList{createDecoration( + Source{{12, 34}}, *this, params.kind)}))}; + auto* s = create( + members, ast::DecorationList{create()}); + auto* s_ty = ty.struct_("mystruct", s); + AST().AddConstructedType(s_ty); + + WrapInFunction(); + + if (params.should_pass) { + EXPECT_TRUE(r()->Resolve()) << r()->error(); + } else { + EXPECT_FALSE(r()->Resolve()) << r()->error(); + EXPECT_EQ(r()->error(), + "12:34 error: decoration is not valid for array types"); + } +} +INSTANTIATE_TEST_SUITE_P( + ResolverDecorationValidationTest, + ArrayDecorationTest, + testing::Values(TestParams{DecorationKind::kAccess, false}, + TestParams{DecorationKind::kAlign, false}, + TestParams{DecorationKind::kBinding, false}, + TestParams{DecorationKind::kBuiltin, false}, + TestParams{DecorationKind::kConstantId, false}, + TestParams{DecorationKind::kGroup, false}, + TestParams{DecorationKind::kLocation, false}, + TestParams{DecorationKind::kOffset, false}, + TestParams{DecorationKind::kSize, false}, + TestParams{DecorationKind::kStage, false}, + TestParams{DecorationKind::kStride, true}, + TestParams{DecorationKind::kStructBlock, false}, + TestParams{DecorationKind::kWorkgroup, false})); + +using StructDecorationTest = TestWithParams; +TEST_P(StructDecorationTest, IsValid) { + auto params = GetParam(); + + auto* s = create(ast::StructMemberList{}, + ast::DecorationList{createDecoration( + Source{{12, 34}}, *this, params.kind)}); + auto* s_ty = ty.struct_("mystruct", s); + AST().AddConstructedType(s_ty); + + WrapInFunction(); + + if (params.should_pass) { + EXPECT_TRUE(r()->Resolve()) << r()->error(); + } else { + EXPECT_FALSE(r()->Resolve()) << r()->error(); + EXPECT_EQ(r()->error(), + "12:34 error: decoration is not valid for struct declarations"); + } +} +INSTANTIATE_TEST_SUITE_P( + ResolverDecorationValidationTest, + StructDecorationTest, + testing::Values(TestParams{DecorationKind::kAccess, false}, + TestParams{DecorationKind::kAlign, false}, + TestParams{DecorationKind::kBinding, false}, + TestParams{DecorationKind::kBuiltin, false}, + TestParams{DecorationKind::kConstantId, false}, + TestParams{DecorationKind::kGroup, false}, + TestParams{DecorationKind::kLocation, false}, + TestParams{DecorationKind::kOffset, false}, + TestParams{DecorationKind::kSize, false}, + TestParams{DecorationKind::kStage, false}, + TestParams{DecorationKind::kStride, false}, + TestParams{DecorationKind::kStructBlock, true}, + TestParams{DecorationKind::kWorkgroup, false})); + +using StructMemberDecorationTest = TestWithParams; +TEST_P(StructMemberDecorationTest, IsValid) { + auto params = GetParam(); + + ast::StructMemberList members{ + Member("a", ty.i32(), + ast::DecorationList{ + createDecoration(Source{{12, 34}}, *this, params.kind)})}; + auto* s = create(members, ast::DecorationList{}); + auto* s_ty = ty.struct_("mystruct", s); + AST().AddConstructedType(s_ty); + + WrapInFunction(); + + if (params.should_pass) { + EXPECT_TRUE(r()->Resolve()) << r()->error(); + } else { + EXPECT_FALSE(r()->Resolve()) << r()->error(); + EXPECT_EQ(r()->error(), + "12:34 error: decoration is not valid for structure members"); + } +} +INSTANTIATE_TEST_SUITE_P( + ResolverDecorationValidationTest, + StructMemberDecorationTest, + testing::Values(TestParams{DecorationKind::kAccess, false}, + TestParams{DecorationKind::kAlign, true}, + TestParams{DecorationKind::kBinding, false}, + TestParams{DecorationKind::kBuiltin, true}, + TestParams{DecorationKind::kConstantId, false}, + TestParams{DecorationKind::kGroup, false}, + TestParams{DecorationKind::kLocation, true}, + TestParams{DecorationKind::kOffset, true}, + TestParams{DecorationKind::kSize, true}, + TestParams{DecorationKind::kStage, false}, + TestParams{DecorationKind::kStride, false}, + TestParams{DecorationKind::kStructBlock, false}, + TestParams{DecorationKind::kWorkgroup, false})); + +} // namespace +} // namespace tint diff --git a/src/resolver/resolver.cc b/src/resolver/resolver.cc index 5d6b756b61..90a4afc0bd 100644 --- a/src/resolver/resolver.cc +++ b/src/resolver/resolver.cc @@ -27,6 +27,7 @@ #include "src/ast/if_statement.h" #include "src/ast/loop_statement.h" #include "src/ast/return_statement.h" +#include "src/ast/struct_block_decoration.h" #include "src/ast/switch_statement.h" #include "src/ast/unary_op_expression.h" #include "src/ast/variable_decl_statement.h" @@ -1391,6 +1392,61 @@ const semantic::Array* Resolver::Array(type::Array* arr) { return create_semantic(utils::RoundUp(el_align, el_size)); } +bool Resolver::ValidateStructure(const type::Struct* st) { + for (auto* member : st->impl()->members()) { + if (auto* r = member->type()->UnwrapAll()->As()) { + if (r->IsRuntimeArray()) { + if (member != st->impl()->members().back()) { + diagnostics_.add_error( + "v-0015", + "runtime arrays may only appear as the last member of a struct", + member->source()); + return false; + } + if (!st->IsBlockDecorated()) { + diagnostics_.add_error("v-0015", + "a struct containing a runtime-sized array " + "requires the [[block]] attribute: '" + + builder_->Symbols().NameFor(st->symbol()) + + "'", + member->source()); + return false; + } + + for (auto* deco : r->decorations()) { + if (!deco->Is()) { + diagnostics_.add_error("decoration is not valid for array types", + deco->source()); + return false; + } + } + } + } + + for (auto* deco : member->decorations()) { + if (!(deco->Is() || + deco->Is() || + deco->Is() || + deco->Is() || + deco->Is())) { + diagnostics_.add_error("decoration is not valid for structure members", + deco->source()); + return false; + } + } + } + + for (auto* deco : st->impl()->decorations()) { + if (!(deco->Is())) { + diagnostics_.add_error("decoration is not valid for struct declarations", + deco->source()); + return false; + } + } + + return true; +} + Resolver::StructInfo* Resolver::Structure(type::Struct* str) { auto info_it = struct_info_.find(str); if (info_it != struct_info_.end()) { @@ -1398,6 +1454,10 @@ Resolver::StructInfo* Resolver::Structure(type::Struct* str) { return info_it->second; } + if (!ValidateStructure(str)) { + return nullptr; + } + semantic::StructMemberList sem_members; sem_members.reserve(str->impl()->members().size()); diff --git a/src/resolver/resolver.h b/src/resolver/resolver.h index 63e5850827..97e915aa43 100644 --- a/src/resolver/resolver.h +++ b/src/resolver/resolver.h @@ -216,6 +216,10 @@ class Resolver { /// returned. const semantic::Array* Array(type::Array*); + /// @returns returns true if input struct is valid + /// @param st the struct to validate + bool ValidateStructure(const type::Struct* st); + /// @returns the StructInfo for the structure `str`, building it if it hasn't /// been constructed already. If an error is raised, nullptr is returned. StructInfo* Structure(type::Struct* str); diff --git a/src/resolver/struct_layout_test.cc b/src/resolver/struct_layout_test.cc index f77e5e3127..6b29a88d6b 100644 --- a/src/resolver/struct_layout_test.cc +++ b/src/resolver/struct_layout_test.cc @@ -15,6 +15,7 @@ #include "src/resolver/resolver.h" #include "gmock/gmock.h" +#include "src/ast/struct_block_decoration.h" #include "src/resolver/resolver_test_helper.h" #include "src/semantic/struct.h" @@ -125,9 +126,12 @@ TEST_F(ResolverStructLayoutTest, ExplicitStrideArrayStaticSize) { } TEST_F(ResolverStructLayoutTest, ImplicitStrideArrayRuntimeSized) { - auto* s = Structure("S", { - Member("c", ty.array()), - }); + auto* s = + Structure("S", + { + Member("c", ty.array()), + }, + ast::DecorationList{create()}); ASSERT_TRUE(r()->Resolve()) << r()->error(); @@ -143,9 +147,12 @@ TEST_F(ResolverStructLayoutTest, ImplicitStrideArrayRuntimeSized) { } TEST_F(ResolverStructLayoutTest, ExplicitStrideArrayRuntimeSized) { - auto* s = Structure("S", { - Member("c", ty.array(/*stride*/ 32)), - }); + auto* s = + Structure("S", + { + Member("c", ty.array(/*stride*/ 32)), + }, + ast::DecorationList{create()}); ASSERT_TRUE(r()->Resolve()) << r()->error(); diff --git a/src/resolver/type_validation_test.cc b/src/resolver/type_validation_test.cc new file mode 100644 index 0000000000..23caf52496 --- /dev/null +++ b/src/resolver/type_validation_test.cc @@ -0,0 +1,152 @@ +// Copyright 2021 The Tint Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "src/ast/stage_decoration.h" +#include "src/ast/struct_block_decoration.h" +#include "src/resolver/resolver.h" +#include "src/resolver/resolver_test_helper.h" + +#include "gmock/gmock.h" + +namespace tint { +namespace { + +class ResolverTypeValidationTest : public resolver::TestHelper, + public testing::Test {}; + +TEST_F(ResolverTypeValidationTest, RuntimeArrayIsLast_Pass) { + // [[Block]] + // struct Foo { + // vf: f32; + // rt: array; + // }; + + ast::DecorationList decos; + decos.push_back(create()); + auto* st = + create(ast::StructMemberList{Member("vf", ty.f32()), + Member("rt", ty.array())}, + decos); + + auto* struct_type = ty.struct_("Foo", st); + AST().AddConstructedType(struct_type); + + WrapInFunction(); + + EXPECT_TRUE(r()->Resolve()) << r()->error(); +} + +TEST_F(ResolverTypeValidationTest, RuntimeArrayIsLastNoBlock_Fail) { + // struct Foo { + // vf: f32; + // rt: array; + // }; + + ast::DecorationList decos; + auto* st = create( + ast::StructMemberList{Member("vf", ty.f32()), + Member(Source{{12, 34}}, "rt", ty.array())}, + decos); + + auto* struct_type = ty.struct_("Foo", st); + AST().AddConstructedType(struct_type); + + WrapInFunction(); + + EXPECT_FALSE(r()->Resolve()) << r()->error(); + EXPECT_EQ(r()->error(), + "12:34 error v-0015: a struct containing a runtime-sized array " + "requires the [[block]] attribute: 'Foo'"); +} + +TEST_F(ResolverTypeValidationTest, RuntimeArrayIsNotLast_Fail) { + // [[Block]] + // struct Foo { + // rt: array; + // vf: f32; + // }; + + ast::DecorationList decos; + decos.push_back(create()); + + auto* rt = Member(Source{{12, 34}}, "rt", ty.array()); + auto* st = create( + ast::StructMemberList{rt, Member("vf", ty.f32())}, decos); + + auto* struct_type = ty.struct_("Foo", st); + + AST().AddConstructedType(struct_type); + + WrapInFunction(); + + EXPECT_FALSE(r()->Resolve()) << r()->error(); + EXPECT_EQ(r()->error(), + "12:34 error v-0015: runtime arrays may only appear as the last " + "member of a struct"); +} + +TEST_F(ResolverTypeValidationTest, AliasRuntimeArrayIsNotLast_Fail) { + // [[Block]] + // type RTArr = array; + // struct s { + // b: RTArr; + // a: u32; + //} + + auto* alias = ty.alias("RTArr", ty.array()); + + ast::DecorationList decos; + decos.push_back(create()); + auto* st = create( + ast::StructMemberList{Member(Source{{12, 34}}, "b", alias), + Member("a", ty.u32())}, + decos); + + auto* struct_type = ty.struct_("s", st); + AST().AddConstructedType(struct_type); + + WrapInFunction(); + + EXPECT_FALSE(r()->Resolve()) << r()->error(); + EXPECT_EQ( + r()->error(), + "12:34 error v-0015: runtime arrays may only appear as the last member " + "of a struct"); +} + +TEST_F(ResolverTypeValidationTest, AliasRuntimeArrayIsLast_Pass) { + // [[Block]] + // type RTArr = array; + // struct s { + // a: u32; + // b: RTArr; + //} + + auto* alias = ty.alias("RTArr", ty.array()); + + ast::DecorationList decos; + decos.push_back(create()); + auto* st = create( + ast::StructMemberList{Member("a", ty.u32()), Member("b", alias)}, decos); + + auto* struct_type = ty.struct_("s", st); + AST().AddConstructedType(struct_type); + + WrapInFunction(); + + EXPECT_TRUE(r()->Resolve()) << r()->error(); +} + +} // namespace +} // namespace tint diff --git a/src/transform/bound_array_accessors_test.cc b/src/transform/bound_array_accessors_test.cc index 7eedc187f7..344a75e9ea 100644 --- a/src/transform/bound_array_accessors_test.cc +++ b/src/transform/bound_array_accessors_test.cc @@ -535,6 +535,7 @@ TEST_F(BoundArrayAccessorsTest, DISABLED_Matrix_Row_Constant_Id_Clamps) { TEST_F(BoundArrayAccessorsTest, RuntimeArray_Clamps) { auto* src = R"( +[[block]] struct S { a : f32; b : array; @@ -547,6 +548,7 @@ fn f() -> void { )"; auto* expect = R"( +[[block]] struct S { a : f32; b : array; diff --git a/src/validator/validator_decoration_test.cc b/src/validator/validator_decoration_test.cc index a9b8de7615..52343f83f2 100644 --- a/src/validator/validator_decoration_test.cc +++ b/src/validator/validator_decoration_test.cc @@ -86,44 +86,6 @@ ast::Decoration* createDecoration(ProgramBuilder& builder, return nullptr; } -using ArrayDecorationTest = ValidatorDecorationsTestWithParams; -TEST_P(ArrayDecorationTest, Decoration_IsValid) { - auto params = GetParam(); - - ast::StructMemberList members{Member( - "a", create( - ty.f32(), 0, - ast::DecorationList{createDecoration(*this, params.kind)}))}; - auto* s = create( - members, ast::DecorationList{create()}); - auto* s_ty = ty.struct_("mystruct", s); - - ValidatorImpl& v = Build(); - - if (params.should_pass) { - EXPECT_TRUE(v.ValidateConstructedType(s_ty)); - } else { - EXPECT_FALSE(v.ValidateConstructedType(s_ty)); - EXPECT_EQ(v.error(), "decoration is not valid for array types"); - } -} -INSTANTIATE_TEST_SUITE_P( - ValidatorTest, - ArrayDecorationTest, - testing::Values(DecorationTestParams{DecorationKind::kAccess, false}, - DecorationTestParams{DecorationKind::kAlign, false}, - DecorationTestParams{DecorationKind::kBinding, false}, - DecorationTestParams{DecorationKind::kBuiltin, false}, - DecorationTestParams{DecorationKind::kConstantId, false}, - DecorationTestParams{DecorationKind::kGroup, false}, - DecorationTestParams{DecorationKind::kLocation, false}, - DecorationTestParams{DecorationKind::kOffset, false}, - DecorationTestParams{DecorationKind::kSize, false}, - DecorationTestParams{DecorationKind::kStage, false}, - DecorationTestParams{DecorationKind::kStride, true}, - DecorationTestParams{DecorationKind::kStructBlock, false}, - DecorationTestParams{DecorationKind::kWorkgroup, false})); - using FunctionDecorationTest = ValidatorDecorationsTestWithParams; TEST_P(FunctionDecorationTest, Decoration_IsValid) { auto params = GetParam(); @@ -195,77 +157,6 @@ INSTANTIATE_TEST_SUITE_P( DecorationTestParams{DecorationKind::kStructBlock, false}, DecorationTestParams{DecorationKind::kWorkgroup, false})); -using StructDecorationTest = ValidatorDecorationsTestWithParams; -TEST_P(StructDecorationTest, Decoration_IsValid) { - auto params = GetParam(); - - auto* s = create( - ast::StructMemberList{}, - ast::DecorationList{createDecoration(*this, params.kind)}); - auto* s_ty = ty.struct_("mystruct", s); - - ValidatorImpl& v = Build(); - - if (params.should_pass) { - EXPECT_TRUE(v.ValidateConstructedType(s_ty)); - } else { - EXPECT_FALSE(v.ValidateConstructedType(s_ty)); - EXPECT_EQ(v.error(), "decoration is not valid for struct declarations"); - } -} -INSTANTIATE_TEST_SUITE_P( - ValidatorTest, - StructDecorationTest, - testing::Values(DecorationTestParams{DecorationKind::kAccess, false}, - DecorationTestParams{DecorationKind::kAlign, false}, - DecorationTestParams{DecorationKind::kBinding, false}, - DecorationTestParams{DecorationKind::kBuiltin, false}, - DecorationTestParams{DecorationKind::kConstantId, false}, - DecorationTestParams{DecorationKind::kGroup, false}, - DecorationTestParams{DecorationKind::kLocation, false}, - DecorationTestParams{DecorationKind::kOffset, false}, - DecorationTestParams{DecorationKind::kSize, false}, - DecorationTestParams{DecorationKind::kStage, false}, - DecorationTestParams{DecorationKind::kStride, false}, - DecorationTestParams{DecorationKind::kStructBlock, true}, - DecorationTestParams{DecorationKind::kWorkgroup, false})); - -using StructMemberDecorations = ValidatorDecorationsTestWithParams; -TEST_P(StructMemberDecorations, Decoration_IsValid) { - auto params = GetParam(); - - ast::StructMemberList members{ - Member("a", ty.i32(), - ast::DecorationList{createDecoration(*this, params.kind)})}; - auto* s = create(members, ast::DecorationList{}); - auto* s_ty = ty.struct_("mystruct", s); - - ValidatorImpl& v = Build(); - - if (params.should_pass) { - EXPECT_TRUE(v.ValidateConstructedType(s_ty)); - } else { - EXPECT_FALSE(v.ValidateConstructedType(s_ty)); - EXPECT_EQ(v.error(), "decoration is not valid for structure members"); - } -} -INSTANTIATE_TEST_SUITE_P( - ValidatorTest, - StructMemberDecorations, - testing::Values(DecorationTestParams{DecorationKind::kAccess, false}, - DecorationTestParams{DecorationKind::kAlign, true}, - DecorationTestParams{DecorationKind::kBinding, false}, - DecorationTestParams{DecorationKind::kBuiltin, true}, - DecorationTestParams{DecorationKind::kConstantId, false}, - DecorationTestParams{DecorationKind::kGroup, false}, - DecorationTestParams{DecorationKind::kLocation, true}, - DecorationTestParams{DecorationKind::kOffset, true}, - DecorationTestParams{DecorationKind::kSize, true}, - DecorationTestParams{DecorationKind::kStage, false}, - DecorationTestParams{DecorationKind::kStride, false}, - DecorationTestParams{DecorationKind::kStructBlock, false}, - DecorationTestParams{DecorationKind::kWorkgroup, false})); - using VariableDecorationTest = ValidatorDecorationsTestWithParams; TEST_P(VariableDecorationTest, Decoration_IsValid) { auto params = GetParam(); diff --git a/src/validator/validator_impl.cc b/src/validator/validator_impl.cc index add2fc1c41..d57a3c4e46 100644 --- a/src/validator/validator_impl.cc +++ b/src/validator/validator_impl.cc @@ -72,9 +72,8 @@ bool ValidatorImpl::Validate() { // Validate global declarations in the order they appear in the module. for (auto* decl : program_->AST().GlobalDeclarations()) { if (auto* ty = decl->As()) { - if (!ValidateConstructedType(ty)) { - return false; - } + // Validated by Resolver (Struct types only) + return true; } else if (auto* func = decl->As()) { current_function_ = func; if (!ValidateFunction(func)) { @@ -97,60 +96,6 @@ bool ValidatorImpl::Validate() { return true; } -bool ValidatorImpl::ValidateConstructedType(const type::Type* type) { - if (auto* st = type->As()) { - for (auto* member : st->impl()->members()) { - if (auto* r = member->type()->UnwrapAll()->As()) { - if (r->IsRuntimeArray()) { - if (member != st->impl()->members().back()) { - add_error(member->source(), "v-0015", - "runtime arrays may only appear as the last member of " - "a struct"); - return false; - } - if (!st->IsBlockDecorated()) { - add_error(member->source(), "v-0015", - "a struct containing a runtime-sized array " - "requires the [[block]] attribute: '" + - program_->Symbols().NameFor(st->symbol()) + "'"); - return false; - } - - for (auto* deco : r->decorations()) { - if (!deco->Is()) { - add_error(deco->source(), - "decoration is not valid for array types"); - return false; - } - } - } - } - - for (auto* deco : member->decorations()) { - if (!(deco->Is() || - deco->Is() || - deco->Is() || - deco->Is() || - deco->Is())) { - add_error(deco->source(), - "decoration is not valid for structure members"); - return false; - } - } - } - - for (auto* deco : st->impl()->decorations()) { - if (!(deco->Is())) { - add_error(deco->source(), - "decoration is not valid for struct declarations"); - return false; - } - } - } - - return true; -} - bool ValidatorImpl::ValidateGlobalVariable(const ast::Variable* var) { auto* sem = program_->Sem().Get(var); if (!sem) { diff --git a/src/validator/validator_impl.h b/src/validator/validator_impl.h index e5f02c3428..1d2338ed88 100644 --- a/src/validator/validator_impl.h +++ b/src/validator/validator_impl.h @@ -131,12 +131,6 @@ class ValidatorImpl { /// @param funcs the functions to check /// @returns true if the valdiation was successful bool ValidateEntryPoint(const ast::FunctionList& funcs); - - /// Validates a constructed type - /// @param type the type to check - /// @returns true if the valdiation was successful - bool ValidateConstructedType(const type::Type* type); - /// Returns true if the given type is storable. This uses and /// updates `storable_` and `not_storable_`. /// @param type the given type diff --git a/src/validator/validator_type_test.cc b/src/validator/validator_type_test.cc index da372c69d3..dd991ae924 100644 --- a/src/validator/validator_type_test.cc +++ b/src/validator/validator_type_test.cc @@ -21,129 +21,6 @@ namespace { class ValidatorTypeTest : public ValidatorTestHelper, public testing::Test {}; -TEST_F(ValidatorTypeTest, RuntimeArrayIsLast_Pass) { - // [[Block]] - // struct Foo { - // vf: f32; - // rt: array; - // }; - - ast::DecorationList decos; - decos.push_back(create()); - auto* st = - create(ast::StructMemberList{Member("vf", ty.f32()), - Member("rt", ty.array())}, - decos); - - auto* struct_type = ty.struct_("Foo", st); - - AST().AddConstructedType(struct_type); - - ValidatorImpl& v = Build(); - - EXPECT_TRUE(v.ValidateConstructedType(struct_type)); -} - -TEST_F(ValidatorTypeTest, RuntimeArrayIsLastNoBlock_Fail) { - // struct Foo { - // vf: f32; - // rt: array; - // }; - - ast::DecorationList decos; - auto* st = - create(ast::StructMemberList{Member("vf", ty.f32()), - Member("rt", ty.array())}, - decos); - - auto* struct_type = ty.struct_("Foo", st); - AST().AddConstructedType(struct_type); - - ValidatorImpl& v = Build(); - - EXPECT_FALSE(v.ValidateConstructedType(struct_type)); - EXPECT_EQ(v.error(), - "v-0015: a struct containing a runtime-sized array requires the " - "[[block]] attribute: 'Foo'"); -} - -TEST_F(ValidatorTypeTest, RuntimeArrayIsNotLast_Fail) { - // [[Block]] - // struct Foo { - // rt: array; - // vf: f32; - // }; - - ast::DecorationList decos; - decos.push_back(create()); - - SetSource(Source::Location{12, 34}); - auto* rt = Member("rt", ty.array()); - SetSource(Source{}); - auto* st = create( - ast::StructMemberList{rt, Member("vf", ty.f32())}, decos); - - auto* struct_type = ty.struct_("Foo", st); - - AST().AddConstructedType(struct_type); - - ValidatorImpl& v = Build(); - - EXPECT_FALSE(v.ValidateConstructedType(struct_type)); - EXPECT_EQ(v.error(), - "12:34 v-0015: runtime arrays may only appear as the last member " - "of a struct"); -} - -TEST_F(ValidatorTypeTest, AliasRuntimeArrayIsNotLast_Fail) { - // [[Block]] - // type RTArr = array; - // struct s { - // b: RTArr; - // a: u32; - //} - - auto* alias = ty.alias("RTArr", ty.array()); - - ast::DecorationList decos; - decos.push_back(create()); - auto* st = create( - ast::StructMemberList{Member("b", alias), Member("a", ty.u32())}, decos); - - auto* struct_type = ty.struct_("s", st); - AST().AddConstructedType(struct_type); - - ValidatorImpl& v = Build(); - - EXPECT_FALSE(v.ValidateConstructedType(struct_type)); - EXPECT_EQ(v.error(), - "v-0015: runtime arrays may only appear as the last member " - "of a struct"); -} - -TEST_F(ValidatorTypeTest, AliasRuntimeArrayIsLast_Pass) { - // [[Block]] - // type RTArr = array; - // struct s { - // a: u32; - // b: RTArr; - //} - - auto* alias = ty.alias("RTArr", ty.array()); - - ast::DecorationList decos; - decos.push_back(create()); - auto* st = create( - ast::StructMemberList{Member("a", ty.u32()), Member("b", alias)}, decos); - - auto* struct_type = ty.struct_("s", st); - AST().AddConstructedType(struct_type); - - ValidatorImpl& v = Build(); - - EXPECT_TRUE(v.ValidateConstructedType(struct_type)); -} - TEST_F(ValidatorTypeTest, RuntimeArrayInFunction_Fail) { /// [[stage(vertex)]] // fn func -> void { var a : array; } diff --git a/src/writer/msl/generator_impl_type_test.cc b/src/writer/msl/generator_impl_type_test.cc index d13ad94383..2540a8f37b 100644 --- a/src/writer/msl/generator_impl_type_test.cc +++ b/src/writer/msl/generator_impl_type_test.cc @@ -410,14 +410,17 @@ TEST_F(MslGeneratorImplTest, EmitType_Struct_Layout_ArrayDefaultStride) { // array_z: size(4), align(4) auto* array_z = ty.array(); - auto* s = Structure("S", { - Member("a", ty.i32()), - Member("b", array_x), - Member("c", ty.f32()), - Member("d", array_y), - Member("e", ty.f32()), - Member("f", array_z), - }); + auto* s = + Structure("S", + { + Member("a", ty.i32()), + Member("b", array_x), + Member("c", ty.f32()), + Member("d", array_y), + Member("e", ty.f32()), + Member("f", array_z), + }, + ast::DecorationList{create()}); Global("G", s, ast::StorageClass::kStorage); diff --git a/src/writer/spirv/builder_type_test.cc b/src/writer/spirv/builder_type_test.cc index 5e990a1a4c..6ebb64d588 100644 --- a/src/writer/spirv/builder_type_test.cc +++ b/src/writer/spirv/builder_type_test.cc @@ -438,11 +438,14 @@ TEST_F(BuilderTest_Type, GenerateStruct_DecoratedMembers_LayoutArraysOfMatrix) { auto* arr_arr_mat2x3 = ty.array(ty.mat2x3(), 1); // Doubly nested array auto* rtarr_mat4x4 = ty.array(ty.mat4x4(), 0); // Runtime array - auto* s = Structure("S", { - Member("a", arr_mat2x2), - Member("b", arr_arr_mat2x3), - Member("c", rtarr_mat4x4), - }); + auto* s = + Structure("S", + { + Member("a", arr_mat2x2), + Member("b", arr_arr_mat2x3), + Member("c", rtarr_mat4x4), + }, + ast::DecorationList{create()}); spirv::Builder& b = Build(); @@ -469,7 +472,8 @@ OpMemberName %1 0 "a" OpMemberName %1 1 "b" OpMemberName %1 2 "c" )"); - EXPECT_EQ(DumpInstructions(b.annots()), R"(OpMemberDecorate %1 0 Offset 0 + EXPECT_EQ(DumpInstructions(b.annots()), R"(OpDecorate %1 Block +OpMemberDecorate %1 0 Offset 0 OpMemberDecorate %1 0 ColMajor OpMemberDecorate %1 0 MatrixStride 8 OpDecorate %2 ArrayStride 16 diff --git a/test/BUILD.gn b/test/BUILD.gn index 228541551d..d5952090ea 100644 --- a/test/BUILD.gn +++ b/test/BUILD.gn @@ -169,13 +169,15 @@ source_set("tint_unittests_core_src") { "../src/intrinsic_table_test.cc", "../src/program_builder_test.cc", "../src/program_test.cc", + "../src/resolver/decoration_validation_test.cc", "../src/resolver/intrinsic_test.cc", "../src/resolver/is_storeable_test.cc", + "../src/resolver/resolver_test.cc", "../src/resolver/resolver_test_helper.cc", "../src/resolver/resolver_test_helper.h", - "../src/resolver/resolver_test.cc", "../src/resolver/struct_layout_test.cc", "../src/resolver/struct_storage_class_use_test.cc", + "../src/resolver/type_validation_test.cc", "../src/resolver/validation_test.cc", "../src/scope_stack_test.cc", "../src/semantic/sem_intrinsic_test.cc",