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 <bclayton@google.com>
Kokoro: Kokoro <noreply+kokoro@google.com>
Reviewed-by: Dan Sinclair <dsinclair@chromium.org>
diff --git a/src/tint/resolver/attribute_validation_test.cc b/src/tint/resolver/attribute_validation_test.cc
index 86d6825..762eea9 100644
--- a/src/tint/resolver/attribute_validation_test.cc
+++ b/src/tint/resolver/attribute_validation_test.cc
@@ -1606,12 +1606,22 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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<LocationAttributeType> {
+ 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 4fdbcee..a12d3c1 100644
--- a/src/tint/resolver/override_test.cc
+++ b/src/tint/resolver/override_test.cc
@@ -91,7 +91,7 @@
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 @@
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 3252829..cb24955 100644
--- a/src/tint/resolver/resolver.cc
+++ b/src/tint/resolver/resolver.cc
@@ -461,18 +461,18 @@
return nullptr;
}
if (!materialized->Type()->IsAnyOf<sem::I32, sem::U32>()) {
- 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<AInt>();
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<decltype(OverrideId::value)>::max()) {
- AddError("override IDs must be between 0 and " +
+ AddError("@id value must be between 0 and " +
std::to_string(std::numeric_limits<decltype(OverrideId::value)>::max()),
id_attr->source);
return nullptr;
@@ -638,14 +638,14 @@
return nullptr;
}
if (!materialized->Type()->IsAnyOf<sem::I32, sem::U32>()) {
- 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<AInt>();
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 @@
return nullptr;
}
if (!materialized->Type()->IsAnyOf<sem::I32, sem::U32>()) {
- 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<AInt>();
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 @@
std::optional<uint32_t> location;
if (auto* attr = ast::GetAttribute<ast::LocationAttribute>(var->attributes)) {
- auto* materialized = Materialize(Expression(attr->expr));
- if (!materialized) {
+ auto value = LocationAttribute(attr);
+ if (!value) {
return nullptr;
}
- if (!materialized->Type()->IsAnyOf<sem::I32, sem::U32>()) {
- AddError("'location' must be an i32 or u32 value", attr->source);
- return nullptr;
- }
-
- auto const_value = materialized->ConstantValue();
- auto value = const_value->As<AInt>();
- if (value < 0) {
- AddError("'location' value must be non-negative", attr->source);
- return nullptr;
- }
- location = u32(value);
+ location = value.Get();
}
sem = builder_->create<sem::GlobalVariable>(
@@ -748,48 +737,36 @@
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<ast::BindingAttribute>(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<uint32_t>();
+ binding_point.binding = materialized->ConstantValue()->As<uint32_t>();
}
{
+ ExprEvalStageConstraint constraint{sem::EvaluationStage::kConstant, "@group value"};
+ TINT_SCOPED_ASSIGNMENT(expr_eval_stage_constraint_, constraint);
+
auto* attr = ast::GetAttribute<ast::GroupAttribute>(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<uint32_t>();
+ binding_point.group = materialized->ConstantValue()->As<uint32_t>();
}
}
std::optional<uint32_t> location;
- if (auto* l = ast::GetAttribute<ast::LocationAttribute>(param->attributes)) {
- auto* materialize = Materialize(Expression(l->expr));
- if (!materialize) {
+ if (auto* attr = ast::GetAttribute<ast::LocationAttribute>(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<uint32_t>();
+ location = value.Get();
}
auto* sem = builder_->create<sem::Parameter>(
@@ -799,6 +776,30 @@
return sem;
}
+utils::Result<uint32_t> 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<sem::I32, sem::U32>()) {
+ AddError("@location must be an i32 or u32 value", attr->source);
+ return utils::Failure;
+ }
+
+ auto const_value = materialized->ConstantValue();
+ auto value = const_value->As<AInt>();
+ if (value < 0) {
+ AddError("@location value must be non-negative", attr->source);
+ return utils::Failure;
+ }
+
+ return static_cast<uint32_t>(value);
+}
+
ast::Access Resolver::DefaultAccessForAddressSpace(ast::AddressSpace address_space) {
// https://gpuweb.github.io/gpuweb/wgsl/#storage-class
switch (address_space) {
@@ -984,18 +985,12 @@
for (auto* attr : decl->return_type_attributes) {
Mark(attr);
- if (auto* a = attr->As<ast::LocationAttribute>()) {
- auto* materialize = Materialize(Expression(a->expr));
- if (!materialize) {
+ if (auto* loc_attr = attr->As<ast::LocationAttribute>()) {
+ 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<uint32_t>();
+ return_location = value.Get();
}
}
if (!validator_.NoDuplicateAttributes(decl->attributes)) {
@@ -2969,22 +2964,12 @@
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<uint32_t>();
+ location = value.Get();
return true;
},
[&](Default) {
diff --git a/src/tint/resolver/resolver.h b/src/tint/resolver/resolver.h
index b657b46..89aab13 100644
--- a/src/tint/resolver/resolver.h
+++ b/src/tint/resolver/resolver.h
@@ -349,6 +349,10 @@
/// @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<uint32_t> 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 7f1cecf..0d6bfe6 100644
--- a/src/tint/resolver/validator.cc
+++ b/src/tint/resolver/validator.cc
@@ -797,7 +797,7 @@
if (attr->Is<ast::IdAttribute>()) {
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<ast::IdAttribute>(it->second->Declaration()->attributes)
