From b5c213b9951681beca81de2b7fda3c448df850f0 Mon Sep 17 00:00:00 2001 From: Ben Clayton Date: Wed, 2 Nov 2022 07:49:23 +0000 Subject: [PATCH] tint/resolver: Fix failures with no error Invalid values to @binding(), @group() and @location() would fail resolving without an error diagnostic. This later triggers and ICE. Refactor duplicate @location resolving in 4 places to a single method. Canonicalize the diagnostic messages for attributes. Remove a bunch of TODOs Bug: chromium:1380212 Bug: tint:1633 Change-Id: Id2cc6ba4b807f12f350a2a31ef87fa0f185b64c3 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/108144 Commit-Queue: Ben Clayton Kokoro: Kokoro Reviewed-by: Dan Sinclair --- .../resolver/attribute_validation_test.cc | 139 ++++++++++++++++-- src/tint/resolver/override_test.cc | 4 +- src/tint/resolver/resolver.cc | 131 ++++++++--------- src/tint/resolver/resolver.h | 4 + src/tint/resolver/validator.cc | 2 +- 5 files changed, 195 insertions(+), 85 deletions(-) diff --git a/src/tint/resolver/attribute_validation_test.cc b/src/tint/resolver/attribute_validation_test.cc index 86d6825cb1..762eea93d8 100644 --- a/src/tint/resolver/attribute_validation_test.cc +++ b/src/tint/resolver/attribute_validation_test.cc @@ -1606,12 +1606,22 @@ TEST_F(GroupAndBindingTest, Const_AInt) { EXPECT_TRUE(r()->Resolve()) << r()->error(); } +TEST_F(GroupAndBindingTest, Binding_NonConstant) { + GlobalVar("val", ty.sampled_texture(ast::TextureDimension::k2d, ty.f32()), + Binding(Construct(ty.u32(), Call(Source{{12, 34}}, "dpdx", 1_a))), Group(1_i)); + + EXPECT_FALSE(r()->Resolve()); + EXPECT_EQ( + r()->error(), + R"(12:34 error: @binding requires a const-expression, but expression is a runtime-expression)"); +} + TEST_F(GroupAndBindingTest, Binding_Negative) { GlobalVar("val", ty.sampled_texture(ast::TextureDimension::k2d, ty.f32()), Binding(Source{{12, 34}}, -2_i), Group(1_i)); EXPECT_FALSE(r()->Resolve()); - EXPECT_EQ(r()->error(), R"(12:34 error: 'binding' value must be non-negative)"); + EXPECT_EQ(r()->error(), R"(12:34 error: @binding value must be non-negative)"); } TEST_F(GroupAndBindingTest, Binding_F32) { @@ -1619,7 +1629,7 @@ TEST_F(GroupAndBindingTest, Binding_F32) { Binding(Source{{12, 34}}, 2.0_f), Group(1_u)); EXPECT_FALSE(r()->Resolve()); - EXPECT_EQ(r()->error(), R"(12:34 error: 'binding' must be an i32 or u32 value)"); + EXPECT_EQ(r()->error(), R"(12:34 error: @binding must be an i32 or u32 value)"); } TEST_F(GroupAndBindingTest, Binding_AFloat) { @@ -1627,7 +1637,17 @@ TEST_F(GroupAndBindingTest, Binding_AFloat) { Binding(Source{{12, 34}}, 2.0_a), Group(1_u)); EXPECT_FALSE(r()->Resolve()); - EXPECT_EQ(r()->error(), R"(12:34 error: 'binding' must be an i32 or u32 value)"); + EXPECT_EQ(r()->error(), R"(12:34 error: @binding must be an i32 or u32 value)"); +} + +TEST_F(GroupAndBindingTest, Group_NonConstant) { + GlobalVar("val", ty.sampled_texture(ast::TextureDimension::k2d, ty.f32()), Binding(2_u), + Group(Construct(ty.u32(), Call(Source{{12, 34}}, "dpdx", 1_a)))); + + EXPECT_FALSE(r()->Resolve()); + EXPECT_EQ( + r()->error(), + R"(12:34 error: @group requires a const-expression, but expression is a runtime-expression)"); } TEST_F(GroupAndBindingTest, Group_Negative) { @@ -1635,7 +1655,7 @@ TEST_F(GroupAndBindingTest, Group_Negative) { Group(Source{{12, 34}}, -1_i)); EXPECT_FALSE(r()->Resolve()); - EXPECT_EQ(r()->error(), R"(12:34 error: 'group' value must be non-negative)"); + EXPECT_EQ(r()->error(), R"(12:34 error: @group value must be non-negative)"); } TEST_F(GroupAndBindingTest, Group_F32) { @@ -1643,7 +1663,7 @@ TEST_F(GroupAndBindingTest, Group_F32) { Group(Source{{12, 34}}, 1.0_f)); EXPECT_FALSE(r()->Resolve()); - EXPECT_EQ(r()->error(), R"(12:34 error: 'group' must be an i32 or u32 value)"); + EXPECT_EQ(r()->error(), R"(12:34 error: @group must be an i32 or u32 value)"); } TEST_F(GroupAndBindingTest, Group_AFloat) { @@ -1651,7 +1671,7 @@ TEST_F(GroupAndBindingTest, Group_AFloat) { Group(Source{{12, 34}}, 1.0_a)); EXPECT_FALSE(r()->Resolve()); - EXPECT_EQ(r()->error(), R"(12:34 error: 'group' must be an i32 or u32 value)"); + EXPECT_EQ(r()->error(), R"(12:34 error: @group must be an i32 or u32 value)"); } using IdTest = ResolverTest; @@ -1671,24 +1691,125 @@ TEST_F(IdTest, Const_AInt) { EXPECT_TRUE(r()->Resolve()) << r()->error(); } +TEST_F(IdTest, NonConstant) { + Override("val", ty.f32(), + utils::Vector{Id(Construct(ty.u32(), Call(Source{{12, 34}}, "dpdx", 1_a)))}); + EXPECT_FALSE(r()->Resolve()); + EXPECT_EQ( + r()->error(), + R"(12:34 error: @id requires a const-expression, but expression is a runtime-expression)"); +} + TEST_F(IdTest, Negative) { Override("val", ty.f32(), utils::Vector{Id(Source{{12, 34}}, -1_i)}); EXPECT_FALSE(r()->Resolve()); - EXPECT_EQ(r()->error(), R"(12:34 error: 'id' value must be non-negative)"); + EXPECT_EQ(r()->error(), R"(12:34 error: @id value must be non-negative)"); } TEST_F(IdTest, F32) { Override("val", ty.f32(), utils::Vector{Id(Source{{12, 34}}, 1_f)}); EXPECT_FALSE(r()->Resolve()); - EXPECT_EQ(r()->error(), R"(12:34 error: 'id' must be an i32 or u32 value)"); + EXPECT_EQ(r()->error(), R"(12:34 error: @id must be an i32 or u32 value)"); } TEST_F(IdTest, AFloat) { Override("val", ty.f32(), utils::Vector{Id(Source{{12, 34}}, 1.0_a)}); EXPECT_FALSE(r()->Resolve()); - EXPECT_EQ(r()->error(), R"(12:34 error: 'id' must be an i32 or u32 value)"); + EXPECT_EQ(r()->error(), R"(12:34 error: @id must be an i32 or u32 value)"); } +enum class LocationAttributeType { + kEntryPointParameter, + kEntryPointReturnType, + kStructureMember, +}; + +struct LocationTest : ResolverTestWithParam { + void Build(const ast::Expression* location_value) { + switch (GetParam()) { + case LocationAttributeType::kEntryPointParameter: + Func("main", + utils::Vector{Param(Source{{12, 34}}, "a", ty.i32(), + utils::Vector{ + Location(Source{{12, 34}}, location_value), + Flat(), + })}, + ty.void_(), utils::Empty, + utils::Vector{ + Stage(ast::PipelineStage::kFragment), + }); + return; + case LocationAttributeType::kEntryPointReturnType: + Func("main", utils::Empty, ty.f32(), + utils::Vector{ + Return(1_a), + }, + utils::Vector{ + Stage(ast::PipelineStage::kFragment), + }, + utils::Vector{ + Location(Source{{12, 34}}, location_value), + }); + return; + case LocationAttributeType::kStructureMember: + Structure("S", utils::Vector{ + Member("m", ty.f32(), + utils::Vector{ + Location(Source{{12, 34}}, location_value), + }), + }); + return; + } + } +}; + +TEST_P(LocationTest, Const_I32) { + Build(Expr(0_i)); + EXPECT_TRUE(r()->Resolve()) << r()->error(); +} + +TEST_P(LocationTest, Const_U32) { + Build(Expr(0_u)); + EXPECT_TRUE(r()->Resolve()) << r()->error(); +} + +TEST_P(LocationTest, Const_AInt) { + Build(Expr(0_a)); + EXPECT_TRUE(r()->Resolve()) << r()->error(); +} + +TEST_P(LocationTest, NonConstant) { + Build(Construct(ty.u32(), Call(Source{{12, 34}}, "dpdx", 1_a))); + EXPECT_FALSE(r()->Resolve()); + EXPECT_EQ( + r()->error(), + R"(12:34 error: @location value requires a const-expression, but expression is a runtime-expression)"); +} + +TEST_P(LocationTest, Negative) { + Build(Expr(-1_a)); + EXPECT_FALSE(r()->Resolve()); + EXPECT_EQ(r()->error(), R"(12:34 error: @location value must be non-negative)"); +} + +TEST_P(LocationTest, F32) { + Build(Expr(1_f)); + EXPECT_FALSE(r()->Resolve()); + EXPECT_EQ(r()->error(), R"(12:34 error: @location must be an i32 or u32 value)"); +} + +TEST_P(LocationTest, AFloat) { + Build(Expr(1.0_a)); + EXPECT_FALSE(r()->Resolve()); + EXPECT_EQ(r()->error(), R"(12:34 error: @location must be an i32 or u32 value)"); +} + +INSTANTIATE_TEST_SUITE_P(LocationTest, + LocationTest, + testing::Values(LocationAttributeType::kEntryPointParameter, + LocationAttributeType::kEntryPointReturnType, + LocationAttributeType::kStructureMember)); + } // namespace } // namespace InterpolateTests diff --git a/src/tint/resolver/override_test.cc b/src/tint/resolver/override_test.cc index 4fdbceec9a..a12d3c148b 100644 --- a/src/tint/resolver/override_test.cc +++ b/src/tint/resolver/override_test.cc @@ -91,7 +91,7 @@ TEST_F(ResolverOverrideTest, DuplicateIds) { EXPECT_FALSE(r()->Resolve()); - EXPECT_EQ(r()->error(), R"(56:78 error: override IDs must be unique + EXPECT_EQ(r()->error(), R"(56:78 error: @id values must be unique 12:34 note: a override with an ID of 7 was previously declared here:)"); } @@ -100,7 +100,7 @@ TEST_F(ResolverOverrideTest, IdTooLarge) { EXPECT_FALSE(r()->Resolve()); - EXPECT_EQ(r()->error(), "12:34 error: override IDs must be between 0 and 65535"); + EXPECT_EQ(r()->error(), "12:34 error: @id value must be between 0 and 65535"); } TEST_F(ResolverOverrideTest, F16_TemporallyBan) { diff --git a/src/tint/resolver/resolver.cc b/src/tint/resolver/resolver.cc index 3252829599..cb249559c0 100644 --- a/src/tint/resolver/resolver.cc +++ b/src/tint/resolver/resolver.cc @@ -461,18 +461,18 @@ sem::Variable* Resolver::Override(const ast::Override* v) { return nullptr; } if (!materialized->Type()->IsAnyOf()) { - AddError("'id' must be an i32 or u32 value", id_attr->source); + AddError("@id must be an i32 or u32 value", id_attr->source); return nullptr; } auto const_value = materialized->ConstantValue(); auto value = const_value->As(); if (value < 0) { - AddError("'id' value must be non-negative", id_attr->source); + AddError("@id value must be non-negative", id_attr->source); return nullptr; } if (value > std::numeric_limits::max()) { - AddError("override IDs must be between 0 and " + + AddError("@id value must be between 0 and " + std::to_string(std::numeric_limits::max()), id_attr->source); return nullptr; @@ -638,14 +638,14 @@ sem::Variable* Resolver::Var(const ast::Var* var, bool is_global) { return nullptr; } if (!materialized->Type()->IsAnyOf()) { - AddError("'binding' must be an i32 or u32 value", attr->source); + AddError("@binding must be an i32 or u32 value", attr->source); return nullptr; } auto const_value = materialized->ConstantValue(); auto value = const_value->As(); if (value < 0) { - AddError("'binding' value must be non-negative", attr->source); + AddError("@binding value must be non-negative", attr->source); return nullptr; } binding = u32(value); @@ -662,14 +662,14 @@ sem::Variable* Resolver::Var(const ast::Var* var, bool is_global) { return nullptr; } if (!materialized->Type()->IsAnyOf()) { - AddError("'group' must be an i32 or u32 value", attr->source); + AddError("@group must be an i32 or u32 value", attr->source); return nullptr; } auto const_value = materialized->ConstantValue(); auto value = const_value->As(); if (value < 0) { - AddError("'group' value must be non-negative", attr->source); + AddError("@group value must be non-negative", attr->source); return nullptr; } group = u32(value); @@ -679,22 +679,11 @@ sem::Variable* Resolver::Var(const ast::Var* var, bool is_global) { std::optional location; if (auto* attr = ast::GetAttribute(var->attributes)) { - auto* materialized = Materialize(Expression(attr->expr)); - if (!materialized) { + auto value = LocationAttribute(attr); + if (!value) { return nullptr; } - if (!materialized->Type()->IsAnyOf()) { - AddError("'location' must be an i32 or u32 value", attr->source); - return nullptr; - } - - auto const_value = materialized->ConstantValue(); - auto value = const_value->As(); - if (value < 0) { - AddError("'location' value must be non-negative", attr->source); - return nullptr; - } - location = u32(value); + location = value.Get(); } sem = builder_->create( @@ -748,48 +737,36 @@ sem::Parameter* Resolver::Parameter(const ast::Parameter* param, uint32_t index) sem::BindingPoint binding_point; if (param->HasBindingPoint()) { { + ExprEvalStageConstraint constraint{sem::EvaluationStage::kConstant, "@binding value"}; + TINT_SCOPED_ASSIGNMENT(expr_eval_stage_constraint_, constraint); + auto* attr = ast::GetAttribute(param->attributes); - auto* materialize = Materialize(Expression(attr->expr)); - if (!materialize) { + auto* materialized = Materialize(Expression(attr->expr)); + if (!materialized) { return nullptr; } - auto* c = materialize->ConstantValue(); - if (!c) { - // TODO(crbug.com/tint/1633): Add error message about invalid materialization when - // binding can be an expression. - return nullptr; - } - binding_point.binding = c->As(); + binding_point.binding = materialized->ConstantValue()->As(); } { + ExprEvalStageConstraint constraint{sem::EvaluationStage::kConstant, "@group value"}; + TINT_SCOPED_ASSIGNMENT(expr_eval_stage_constraint_, constraint); + auto* attr = ast::GetAttribute(param->attributes); - auto* materialize = Materialize(Expression(attr->expr)); - if (!materialize) { + auto* materialized = Materialize(Expression(attr->expr)); + if (!materialized) { return nullptr; } - auto* c = materialize->ConstantValue(); - if (!c) { - // TODO(crbug.com/tint/1633): Add error message about invalid materialization when - // binding can be an expression. - return nullptr; - } - binding_point.group = c->As(); + binding_point.group = materialized->ConstantValue()->As(); } } std::optional location; - if (auto* l = ast::GetAttribute(param->attributes)) { - auto* materialize = Materialize(Expression(l->expr)); - if (!materialize) { + if (auto* attr = ast::GetAttribute(param->attributes)) { + auto value = LocationAttribute(attr); + if (!value) { return nullptr; } - auto* c = materialize->ConstantValue(); - if (!c) { - // TODO(crbug.com/tint/1633): Add error message about invalid materialization when - // location can be an expression. - return nullptr; - } - location = c->As(); + location = value.Get(); } auto* sem = builder_->create( @@ -799,6 +776,30 @@ sem::Parameter* Resolver::Parameter(const ast::Parameter* param, uint32_t index) return sem; } +utils::Result Resolver::LocationAttribute(const ast::LocationAttribute* attr) { + ExprEvalStageConstraint constraint{sem::EvaluationStage::kConstant, "@location value"}; + TINT_SCOPED_ASSIGNMENT(expr_eval_stage_constraint_, constraint); + + auto* materialized = Materialize(Expression(attr->expr)); + if (!materialized) { + return utils::Failure; + } + + if (!materialized->Type()->IsAnyOf()) { + AddError("@location must be an i32 or u32 value", attr->source); + return utils::Failure; + } + + auto const_value = materialized->ConstantValue(); + auto value = const_value->As(); + if (value < 0) { + AddError("@location value must be non-negative", attr->source); + return utils::Failure; + } + + return static_cast(value); +} + ast::Access Resolver::DefaultAccessForAddressSpace(ast::AddressSpace address_space) { // https://gpuweb.github.io/gpuweb/wgsl/#storage-class switch (address_space) { @@ -984,18 +985,12 @@ sem::Function* Resolver::Function(const ast::Function* decl) { for (auto* attr : decl->return_type_attributes) { Mark(attr); - if (auto* a = attr->As()) { - auto* materialize = Materialize(Expression(a->expr)); - if (!materialize) { + if (auto* loc_attr = attr->As()) { + auto value = LocationAttribute(loc_attr); + if (!value) { return nullptr; } - auto* c = materialize->ConstantValue(); - if (!c) { - // TODO(crbug.com/tint/1633): Add error message about invalid materialization when - // location can be an expression. - return nullptr; - } - return_location = c->As(); + return_location = value.Get(); } } if (!validator_.NoDuplicateAttributes(decl->attributes)) { @@ -2969,22 +2964,12 @@ sem::Struct* Resolver::Structure(const ast::Struct* str) { has_size_attr = true; return true; }, - [&](const ast::LocationAttribute* l) { - ExprEvalStageConstraint constraint{sem::EvaluationStage::kConstant, - "@location"}; - TINT_SCOPED_ASSIGNMENT(expr_eval_stage_constraint_, constraint); - - auto* materialize = Materialize(Expression(l->expr)); - if (!materialize) { + [&](const ast::LocationAttribute* loc_attr) { + auto value = LocationAttribute(loc_attr); + if (!value) { return false; } - auto* c = materialize->ConstantValue(); - if (!c) { - // TODO(crbug.com/tint/1633): Add error message about invalid - // materialization when location can be an expression. - return false; - } - location = c->As(); + location = value.Get(); return true; }, [&](Default) { diff --git a/src/tint/resolver/resolver.h b/src/tint/resolver/resolver.h index b657b4692c..89aab13709 100644 --- a/src/tint/resolver/resolver.h +++ b/src/tint/resolver/resolver.h @@ -349,6 +349,10 @@ class Resolver { /// @param index the index of the parameter sem::Parameter* Parameter(const ast::Parameter* param, uint32_t index); + /// @returns the location value for a `@location` attribute, validating the value's range and + /// type. + utils::Result LocationAttribute(const ast::LocationAttribute* attr); + /// Records the address space usage for the given type, and any transient /// dependencies of the type. Validates that the type can be used for the /// given address space, erroring if it cannot. diff --git a/src/tint/resolver/validator.cc b/src/tint/resolver/validator.cc index 7f1cecfb38..0d6bfe65bd 100644 --- a/src/tint/resolver/validator.cc +++ b/src/tint/resolver/validator.cc @@ -797,7 +797,7 @@ bool Validator::Override( if (attr->Is()) { auto id = v->OverrideId(); if (auto it = override_ids.find(id); it != override_ids.end() && it->second != v) { - AddError("override IDs must be unique", attr->source); + AddError("@id values must be unique", attr->source); AddNote("a override with an ID of " + std::to_string(id.value) + " was previously declared here:", ast::GetAttribute(it->second->Declaration()->attributes)