From ca2c1ed3706702f116c7c4723222fdd2f251b88e Mon Sep 17 00:00:00 2001 From: Ben Clayton Date: Tue, 5 Jan 2021 18:20:40 +0000 Subject: [PATCH] Validate that runtime arrays aren't used as parameters Remove the logic around `arrayLength()` being passed anything but a member accessor. Runtime arrays types cannot be used for variables nor parameters. Add validator logic and test for runtime array parameters. Adjust the validator error message so it doesn't include the field / variable name. This read weird, and the same information is already provided by the source. Bug: tint:266 Bug: tint:252 Change-Id: Iecedb0524e10a67b4f8ad15635d67fe61e9d69d9 Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/36420 Auto-Submit: Ben Clayton Commit-Queue: dan sinclair Reviewed-by: dan sinclair --- src/validator/validator_impl.cc | 27 ++++++++---- src/validator/validator_impl.h | 4 ++ src/validator/validator_type_test.cc | 49 ++++++++++++++++++---- src/writer/spirv/builder.cc | 14 +++---- src/writer/spirv/builder_intrinsic_test.cc | 36 +--------------- 5 files changed, 74 insertions(+), 56 deletions(-) diff --git a/src/validator/validator_impl.cc b/src/validator/validator_impl.cc index b6fe9032b3..a2a6637c3b 100644 --- a/src/validator/validator_impl.cc +++ b/src/validator/validator_impl.cc @@ -91,9 +91,8 @@ bool ValidatorImpl::ValidateConstructedTypes( 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: '" + - member->name() + "'"); + "runtime arrays may only appear as the last member of " + "a struct"); return false; } if (!st->IsBlockDecorated()) { @@ -199,6 +198,9 @@ bool ValidatorImpl::ValidateFunction(const ast::Function* func) { for (auto* param : func->params()) { variable_stack_.set(param->name(), param); + if (!ValidateParameter(param)) { + return false; + } } if (!ValidateStatements(func->body())) { return false; @@ -216,6 +218,18 @@ bool ValidatorImpl::ValidateFunction(const ast::Function* func) { return true; } +bool ValidatorImpl::ValidateParameter(const ast::Variable* param) { + if (auto* r = param->type()->UnwrapAll()->As()) { + if (r->IsRuntimeArray()) { + add_error( + param->source(), "v-0015", + "runtime arrays may only appear as the last member of a struct"); + return false; + } + } + return true; +} + bool ValidatorImpl::ValidateReturnStatement(const ast::ReturnStatement* ret) { // TODO(sarahM0): update this when this issue resolves: // https://github.com/gpuweb/gpuweb/issues/996 @@ -266,10 +280,9 @@ bool ValidatorImpl::ValidateDeclStatement( if (auto* arr = decl->variable()->type()->UnwrapAll()->As()) { if (arr->IsRuntimeArray()) { - add_error(decl->source(), "v-0015", - "runtime arrays may only appear as the last " - "member of a struct: '" + - decl->variable()->name() + "'"); + add_error( + decl->source(), "v-0015", + "runtime arrays may only appear as the last member of a struct"); return false; } } diff --git a/src/validator/validator_impl.h b/src/validator/validator_impl.h index 82ba625be0..53a8edf79a 100644 --- a/src/validator/validator_impl.h +++ b/src/validator/validator_impl.h @@ -83,6 +83,10 @@ class ValidatorImpl { /// @param func the function to check /// @returns true if the validation was successful bool ValidateFunction(const ast::Function* func); + /// Validates a function parameter + /// @param param the function parameter to check + /// @returns true if the validation was successful + bool ValidateParameter(const ast::Variable* param); /// Validates a block of statements /// @param block the statements to check /// @returns true if the validation was successful diff --git a/src/validator/validator_type_test.cc b/src/validator/validator_type_test.cc index fec698f092..2a3721c2ef 100644 --- a/src/validator/validator_type_test.cc +++ b/src/validator/validator_type_test.cc @@ -84,18 +84,20 @@ TEST_F(ValidatorTypeTest, RuntimeArrayIsNotLast_Fail) { ast::StructDecorationList decos; decos.push_back(create()); - auto* st = - create(ast::StructMemberList{Member("rt", ty.array()), - Member("vf", ty.f32)}, - decos); + + 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); mod->AddConstructedType(struct_type); EXPECT_FALSE(v()->ValidateConstructedTypes(mod->constructed_types())); EXPECT_EQ(v()->error(), - "v-0015: runtime arrays may only appear as the last member " - "of a struct: 'rt'"); + "12:34 v-0015: runtime arrays may only appear as the last member " + "of a struct"); } TEST_F(ValidatorTypeTest, AliasRuntimeArrayIsNotLast_Fail) { @@ -118,7 +120,7 @@ TEST_F(ValidatorTypeTest, AliasRuntimeArrayIsNotLast_Fail) { EXPECT_FALSE(v()->ValidateConstructedTypes(mod->constructed_types())); EXPECT_EQ(v()->error(), "v-0015: runtime arrays may only appear as the last member " - "of a struct: 'b'"); + "of a struct"); } TEST_F(ValidatorTypeTest, AliasRuntimeArrayIsLast_Pass) { @@ -161,8 +163,39 @@ TEST_F(ValidatorTypeTest, RuntimeArrayInFunction_Fail) { EXPECT_FALSE(v()->Validate(mod)); EXPECT_EQ(v()->error(), "12:34 v-0015: runtime arrays may only appear as the last member " - "of a struct: 'a'"); + "of a struct"); } +TEST_F(ValidatorTypeTest, RuntimeArrayAsParameter_Fail) { + // fn func(a : array) {} + // [[stage(vertex)]] fn main() {} + + auto* param = + Var(Source{Source::Location{12, 34}}, "a", ast::StorageClass::kNone, + ty.array(), nullptr, ast::VariableDecorationList{}); + + auto* func = Func("func", ast::VariableList{param}, ty.void_, + ast::StatementList{ + create(), + }, + ast::FunctionDecorationList{}); + mod->AddFunction(func); + + auto* main = + Func("main", ast::VariableList{}, ty.void_, + ast::StatementList{ + create(), + }, + ast::FunctionDecorationList{ + create(ast::PipelineStage::kVertex), + }); + mod->AddFunction(main); + + EXPECT_TRUE(td()->Determine()) << td()->error(); + EXPECT_FALSE(v()->Validate(mod)); + EXPECT_EQ(v()->error(), + "12:34 v-0015: runtime arrays may only appear as the last member " + "of a struct"); +} } // namespace } // namespace tint diff --git a/src/writer/spirv/builder.cc b/src/writer/spirv/builder.cc index 917cbdc8a8..ca93364838 100644 --- a/src/writer/spirv/builder.cc +++ b/src/writer/spirv/builder.cc @@ -1871,15 +1871,15 @@ uint32_t Builder::GenerateIntrinsic(ast::IdentifierExpression* ident, if (call->params().empty()) { error_ = "missing param for runtime array length"; return 0; - } else if (!call->params()[0]->Is()) { - if (call->params()[0]->result_type()->Is()) { - error_ = "pointer accessors not supported yet"; - } else { - error_ = "invalid accessor for runtime array length"; - } + } + auto* arg = call->params()[0]; + + auto* accessor = arg->As(); + if (accessor == nullptr) { + error_ = "invalid expression for array length"; return 0; } - auto* accessor = call->params()[0]->As(); + auto struct_id = GenerateExpression(accessor->structure()); if (struct_id == 0) { return 0; diff --git a/src/writer/spirv/builder_intrinsic_test.cc b/src/writer/spirv/builder_intrinsic_test.cc index d927f74d50..c2dde98401 100644 --- a/src/writer/spirv/builder_intrinsic_test.cc +++ b/src/writer/spirv/builder_intrinsic_test.cc @@ -1285,8 +1285,7 @@ TEST_F(IntrinsicBuilderTest, Call_ArrayLength) { ast::StructDecorationList{}); auto* s_type = ty.struct_("my_struct", s); auto* var = Var("b", ast::StorageClass::kPrivate, s_type); - auto* expr = Call("arrayLength", create( - Expr("b"), Expr("a"))); + auto* expr = Call("arrayLength", MemberAccessor("b", "a")); ASSERT_TRUE(td.DetermineResultType(expr)) << td.error(); @@ -1321,8 +1320,7 @@ TEST_F(IntrinsicBuilderTest, Call_ArrayLength_OtherMembersInStruct) { auto* s_type = ty.struct_("my_struct", s); auto* var = Var("b", ast::StorageClass::kPrivate, s_type); - auto* expr = Call("arrayLength", create( - Expr("b"), Expr("a"))); + auto* expr = Call("arrayLength", MemberAccessor("b", "a")); ASSERT_TRUE(td.DetermineResultType(expr)) << td.error(); @@ -1350,36 +1348,6 @@ TEST_F(IntrinsicBuilderTest, Call_ArrayLength_OtherMembersInStruct) { )"); } -// TODO(dsinclair): https://bugs.chromium.org/p/tint/issues/detail?id=266 -TEST_F(IntrinsicBuilderTest, DISABLED_Call_ArrayLength_Ptr) { - ast::type::Pointer ptr(ty.array(), ast::StorageClass::kStorageBuffer); - - auto* s = create( - ast::StructMemberList{Member("z", ty.f32), Member("a", ty.array())}, - ast::StructDecorationList{}); - auto* s_type = ty.struct_("my_struct", s); - auto* var = Var("b", ast::StorageClass::kPrivate, s_type); - - Var("ptr_var", ast::StorageClass::kPrivate, &ptr, - create(Expr("b"), Expr("a")), {}); - - auto* expr = Call("arrayLength", "ptr_var"); - ASSERT_TRUE(td.DetermineResultType(expr)) << td.error(); - - auto* func = Func("a_func", ast::VariableList{}, ty.void_, - ast::StatementList{}, ast::FunctionDecorationList{}); - - ASSERT_TRUE(b.GenerateFunction(func)) << b.error(); - ASSERT_TRUE(b.GenerateGlobalVariable(var)) << b.error(); - EXPECT_EQ(b.GenerateExpression(expr), 11u) << b.error(); - - EXPECT_EQ(DumpInstructions(b.types()), R"( ... )"); - - EXPECT_EQ(DumpInstructions(b.functions()[0].instructions()), - R"(%11 = OpArrayLength %12 %5 1 -)"); -} - } // namespace } // namespace spirv } // namespace writer