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 <bclayton@google.com>
Commit-Queue: dan sinclair <dsinclair@chromium.org>
Reviewed-by: dan sinclair <dsinclair@chromium.org>
diff --git a/src/validator/validator_impl.cc b/src/validator/validator_impl.cc
index b6fe903..a2a6637 100644
--- a/src/validator/validator_impl.cc
+++ b/src/validator/validator_impl.cc
@@ -91,9 +91,8 @@
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 @@
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 @@
return true;
}
+bool ValidatorImpl::ValidateParameter(const ast::Variable* param) {
+ if (auto* r = param->type()->UnwrapAll()->As<ast::type::Array>()) {
+ 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 @@
if (auto* arr =
decl->variable()->type()->UnwrapAll()->As<ast::type::Array>()) {
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 82ba625..53a8edf 100644
--- a/src/validator/validator_impl.h
+++ b/src/validator/validator_impl.h
@@ -83,6 +83,10 @@
/// @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 fec698f..2a3721c 100644
--- a/src/validator/validator_type_test.cc
+++ b/src/validator/validator_type_test.cc
@@ -84,18 +84,20 @@
ast::StructDecorationList decos;
decos.push_back(create<ast::StructBlockDecoration>());
- auto* st =
- create<ast::Struct>(ast::StructMemberList{Member("rt", ty.array<f32>()),
- Member("vf", ty.f32)},
- decos);
+
+ SetSource(Source::Location{12, 34});
+ auto* rt = Member("rt", ty.array<f32>());
+ SetSource(Source{});
+ auto* st = create<ast::Struct>(
+ 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 @@
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 @@
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<u32>) {}
+ // [[stage(vertex)]] fn main() {}
+
+ auto* param =
+ Var(Source{Source::Location{12, 34}}, "a", ast::StorageClass::kNone,
+ ty.array<i32>(), nullptr, ast::VariableDecorationList{});
+
+ auto* func = Func("func", ast::VariableList{param}, ty.void_,
+ ast::StatementList{
+ create<ast::ReturnStatement>(),
+ },
+ ast::FunctionDecorationList{});
+ mod->AddFunction(func);
+
+ auto* main =
+ Func("main", ast::VariableList{}, ty.void_,
+ ast::StatementList{
+ create<ast::ReturnStatement>(),
+ },
+ ast::FunctionDecorationList{
+ create<ast::StageDecoration>(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 917cbdc..ca93364 100644
--- a/src/writer/spirv/builder.cc
+++ b/src/writer/spirv/builder.cc
@@ -1871,15 +1871,15 @@
if (call->params().empty()) {
error_ = "missing param for runtime array length";
return 0;
- } else if (!call->params()[0]->Is<ast::MemberAccessorExpression>()) {
- if (call->params()[0]->result_type()->Is<ast::type::Pointer>()) {
- error_ = "pointer accessors not supported yet";
- } else {
- error_ = "invalid accessor for runtime array length";
- }
+ }
+ auto* arg = call->params()[0];
+
+ auto* accessor = arg->As<ast::MemberAccessorExpression>();
+ if (accessor == nullptr) {
+ error_ = "invalid expression for array length";
return 0;
}
- auto* accessor = call->params()[0]->As<ast::MemberAccessorExpression>();
+
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 d927f74..c2dde98 100644
--- a/src/writer/spirv/builder_intrinsic_test.cc
+++ b/src/writer/spirv/builder_intrinsic_test.cc
@@ -1285,8 +1285,7 @@
ast::StructDecorationList{});
auto* s_type = ty.struct_("my_struct", s);
auto* var = Var("b", ast::StorageClass::kPrivate, s_type);
- auto* expr = Call("arrayLength", create<ast::MemberAccessorExpression>(
- Expr("b"), Expr("a")));
+ auto* expr = Call("arrayLength", MemberAccessor("b", "a"));
ASSERT_TRUE(td.DetermineResultType(expr)) << td.error();
@@ -1321,8 +1320,7 @@
auto* s_type = ty.struct_("my_struct", s);
auto* var = Var("b", ast::StorageClass::kPrivate, s_type);
- auto* expr = Call("arrayLength", create<ast::MemberAccessorExpression>(
- Expr("b"), Expr("a")));
+ auto* expr = Call("arrayLength", MemberAccessor("b", "a"));
ASSERT_TRUE(td.DetermineResultType(expr)) << td.error();
@@ -1350,36 +1348,6 @@
)");
}
-// 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<f32>(), ast::StorageClass::kStorageBuffer);
-
- auto* s = create<ast::Struct>(
- ast::StructMemberList{Member("z", ty.f32), Member("a", ty.array<f32>())},
- 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<ast::MemberAccessorExpression>(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
