From ceab140a8b429f502e118c8e62df60d44a917534 Mon Sep 17 00:00:00 2001 From: James Price Date: Tue, 5 Oct 2021 15:02:17 +0000 Subject: [PATCH] validation: Reject struct builtins on wrong stages Change-Id: I047f28c5be6a7d878b9f7ad37b6accc842b4fe98 Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/65840 Auto-Submit: James Price Commit-Queue: Ben Clayton Kokoro: Kokoro Reviewed-by: Ben Clayton --- src/resolver/builtins_validation_test.cc | 22 +++++++++++++- src/resolver/resolver.cc | 29 +++++++------------ src/resolver/resolver.h | 3 +- .../struct_pipeline_stage_use_test.cc | 7 ++--- 4 files changed, 36 insertions(+), 25 deletions(-) diff --git a/src/resolver/builtins_validation_test.cc b/src/resolver/builtins_validation_test.cc index e29b6f224e..733093ab14 100644 --- a/src/resolver/builtins_validation_test.cc +++ b/src/resolver/builtins_validation_test.cc @@ -217,7 +217,7 @@ TEST_F(ResolverBuiltinsValidationTest, FragDepthIsInput_Fail) { "fragment pipeline stage"); } -TEST_F(ResolverBuiltinsValidationTest, FragDepthIsInputStruct_Ignored) { +TEST_F(ResolverBuiltinsValidationTest, FragDepthIsInputStruct_Fail) { // struct MyInputs { // [[builtin(frag_depth)]] ff: f32; // }; @@ -231,8 +231,28 @@ TEST_F(ResolverBuiltinsValidationTest, FragDepthIsInputStruct_Ignored) { Func("fragShader", {Param("arg", ty.Of(s))}, ty.f32(), {Return(1.0f)}, {Stage(ast::PipelineStage::kFragment)}, {Location(0)}); + EXPECT_FALSE(r()->Resolve()); + EXPECT_EQ(r()->error(), + "12:34 error: builtin(frag_depth) cannot be used in input of " + "fragment pipeline stage\n" + "note: while analysing entry point 'fragShader'"); +} + +TEST_F(ResolverBuiltinsValidationTest, StructBuiltinInsideEntryPoint_Ignored) { + // struct S { + // [[builtin(vertex_index)]] idx: u32; + // }; + // [[stage(fragment)]] + // fn fragShader() { var s : S; } + + Structure("S", + {Member("idx", ty.u32(), {Builtin(ast::Builtin::kVertexIndex)})}); + + Func("fragShader", {}, ty.void_(), {Decl(Var("s", ty.type_name("S")))}, + {Stage(ast::PipelineStage::kFragment)}); EXPECT_TRUE(r()->Resolve()); } + } // namespace StageTest TEST_F(ResolverBuiltinsValidationTest, PositionNotF32_Struct_Fail) { diff --git a/src/resolver/resolver.cc b/src/resolver/resolver.cc index 9617d5dcd4..dd9904ca70 100644 --- a/src/resolver/resolver.cc +++ b/src/resolver/resolver.cc @@ -1277,8 +1277,7 @@ bool Resolver::ValidateFunctionParameter(const ast::Function* func, bool Resolver::ValidateBuiltinDecoration(const ast::BuiltinDecoration* deco, const sem::Type* storage_type, - const bool is_input, - const bool is_struct_member) { + const bool is_input) { auto* type = storage_type->UnwrapRef(); const auto stage = current_function_ ? current_function_->declaration->pipeline_stage() @@ -1386,16 +1385,12 @@ bool Resolver::ValidateBuiltinDecoration(const ast::BuiltinDecoration* deco, break; } - // ignore builtin attribute on struct members to facillate data movement - // between stages - if (!is_struct_member) { - if (is_stage_mismatch) { - AddError(deco_to_str(deco) + " cannot be used in " + - (is_input ? "input of " : "output of ") + stage_name.str() + - " pipeline stage", - deco->source()); - return false; - } + if (is_stage_mismatch) { + AddError(deco_to_str(deco) + " cannot be used in " + + (is_input ? "input of " : "output of ") + stage_name.str() + + " pipeline stage", + deco->source()); + return false; } return true; @@ -1556,10 +1551,9 @@ bool Resolver::ValidateEntryPoint(const ast::Function* func, return false; } - if (!ValidateBuiltinDecoration( - builtin, ty, - /* is_input */ param_or_ret == ParamOrRetType::kParameter, - /* is_struct_member */ is_struct_member)) { + if (!ValidateBuiltinDecoration(builtin, ty, + /* is_input */ param_or_ret == + ParamOrRetType::kParameter)) { return false; } builtins.emplace(builtin->value()); @@ -4098,8 +4092,7 @@ bool Resolver::ValidateStructure(const sem::Struct* str) { } } else if (auto* builtin = deco->As()) { if (!ValidateBuiltinDecoration(builtin, member->Type(), - /* is_input */ false, - /* is_struct_member */ true)) { + /* is_input */ false)) { return false; } if (builtin->value() == ast::Builtin::kPosition) { diff --git a/src/resolver/resolver.h b/src/resolver/resolver.h index 18965be5df..3b05f2c619 100644 --- a/src/resolver/resolver.h +++ b/src/resolver/resolver.h @@ -283,8 +283,7 @@ class Resolver { bool ValidateAssignment(const ast::AssignmentStatement* a); bool ValidateBuiltinDecoration(const ast::BuiltinDecoration* deco, const sem::Type* storage_type, - const bool is_input, - const bool is_struct_member); + const bool is_input); bool ValidateCall(ast::CallExpression* call); bool ValidateCallStatement(ast::CallStatement* stmt); bool ValidateEntryPoint(const ast::Function* func, const FunctionInfo* info); diff --git a/src/resolver/struct_pipeline_stage_use_test.cc b/src/resolver/struct_pipeline_stage_use_test.cc index 7c0c0223c2..9cd48714e4 100644 --- a/src/resolver/struct_pipeline_stage_use_test.cc +++ b/src/resolver/struct_pipeline_stage_use_test.cc @@ -140,8 +140,8 @@ TEST_F(ResolverPipelineStageUseTest, StructUsedMultipleStages) { auto* s = Structure( "S", {Member("a", ty.vec4(), {Builtin(ast::Builtin::kPosition)})}); - Func("vert_main", {Param("param", ty.Of(s))}, ty.Of(s), - {Return(Construct(ty.Of(s)))}, {Stage(ast::PipelineStage::kVertex)}); + Func("vert_main", {}, ty.Of(s), {Return(Construct(ty.Of(s)))}, + {Stage(ast::PipelineStage::kVertex)}); Func("frag_main", {Param("param", ty.Of(s))}, ty.void_(), {}, {Stage(ast::PipelineStage::kFragment)}); @@ -151,8 +151,7 @@ TEST_F(ResolverPipelineStageUseTest, StructUsedMultipleStages) { auto* sem = TypeOf(s)->As(); ASSERT_NE(sem, nullptr); EXPECT_THAT(sem->PipelineStageUses(), - UnorderedElementsAre(sem::PipelineStageUsage::kVertexInput, - sem::PipelineStageUsage::kVertexOutput, + UnorderedElementsAre(sem::PipelineStageUsage::kVertexOutput, sem::PipelineStageUsage::kFragmentInput)); }