From ab3a9218b7510b240700eb3aa387aadc36f98f0c Mon Sep 17 00:00:00 2001 From: James Price Date: Thu, 6 May 2021 12:36:52 +0000 Subject: [PATCH] validator: Disallow [[override]] on non-const vars Only allow them on constants, where no other decoration is valid. Change-Id: I83f19667adb1dd4ebbba86827324a45a8f1a80a4 Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/50041 Auto-Submit: James Price Reviewed-by: Ben Clayton Commit-Queue: James Price --- src/resolver/decoration_validation_test.cc | 53 ++++++++++++++++++---- src/resolver/resolver.cc | 15 ++++-- 2 files changed, 54 insertions(+), 14 deletions(-) diff --git a/src/resolver/decoration_validation_test.cc b/src/resolver/decoration_validation_test.cc index 1ff15c53a9..be3926e85a 100644 --- a/src/resolver/decoration_validation_test.cc +++ b/src/resolver/decoration_validation_test.cc @@ -34,9 +34,9 @@ enum class DecorationKind { kAlign, kBinding, kBuiltin, - kConstantId, kGroup, kLocation, + kOverride, kOffset, kSize, kStage, @@ -63,12 +63,12 @@ static ast::Decoration* createDecoration(const Source& source, return builder.create(source, 1); case DecorationKind::kBuiltin: return builder.Builtin(source, ast::Builtin::kPosition); - case DecorationKind::kConstantId: - return builder.create(source, 0u); case DecorationKind::kGroup: return builder.create(source, 1u); case DecorationKind::kLocation: return builder.Location(source, 1); + case DecorationKind::kOverride: + return builder.create(source, 0u); case DecorationKind::kOffset: return builder.create(source, 4u); case DecorationKind::kSize: @@ -108,9 +108,9 @@ INSTANTIATE_TEST_SUITE_P( TestParams{DecorationKind::kAlign, false}, TestParams{DecorationKind::kBinding, false}, TestParams{DecorationKind::kBuiltin, true}, - TestParams{DecorationKind::kConstantId, false}, TestParams{DecorationKind::kGroup, false}, TestParams{DecorationKind::kLocation, true}, + TestParams{DecorationKind::kOverride, false}, TestParams{DecorationKind::kOffset, false}, TestParams{DecorationKind::kSize, false}, TestParams{DecorationKind::kStage, false}, @@ -150,9 +150,9 @@ INSTANTIATE_TEST_SUITE_P( TestParams{DecorationKind::kAlign, false}, TestParams{DecorationKind::kBinding, false}, TestParams{DecorationKind::kBuiltin, false}, - TestParams{DecorationKind::kConstantId, false}, TestParams{DecorationKind::kGroup, false}, TestParams{DecorationKind::kLocation, false}, + TestParams{DecorationKind::kOverride, false}, TestParams{DecorationKind::kOffset, false}, TestParams{DecorationKind::kSize, false}, TestParams{DecorationKind::kStage, false}, @@ -184,9 +184,9 @@ INSTANTIATE_TEST_SUITE_P( TestParams{DecorationKind::kAlign, false}, TestParams{DecorationKind::kBinding, false}, TestParams{DecorationKind::kBuiltin, false}, - TestParams{DecorationKind::kConstantId, false}, TestParams{DecorationKind::kGroup, false}, TestParams{DecorationKind::kLocation, false}, + TestParams{DecorationKind::kOverride, false}, TestParams{DecorationKind::kOffset, false}, TestParams{DecorationKind::kSize, false}, TestParams{DecorationKind::kStage, false}, @@ -222,9 +222,9 @@ INSTANTIATE_TEST_SUITE_P( TestParams{DecorationKind::kAlign, true}, TestParams{DecorationKind::kBinding, false}, TestParams{DecorationKind::kBuiltin, true}, - TestParams{DecorationKind::kConstantId, false}, TestParams{DecorationKind::kGroup, false}, TestParams{DecorationKind::kLocation, true}, + TestParams{DecorationKind::kOverride, false}, TestParams{DecorationKind::kOffset, true}, TestParams{DecorationKind::kSize, true}, TestParams{DecorationKind::kStage, false}, @@ -257,9 +257,44 @@ INSTANTIATE_TEST_SUITE_P( TestParams{DecorationKind::kAlign, false}, TestParams{DecorationKind::kBinding, true}, TestParams{DecorationKind::kBuiltin, true}, - TestParams{DecorationKind::kConstantId, true}, TestParams{DecorationKind::kGroup, true}, TestParams{DecorationKind::kLocation, true}, + TestParams{DecorationKind::kOverride, false}, + TestParams{DecorationKind::kOffset, false}, + TestParams{DecorationKind::kSize, false}, + TestParams{DecorationKind::kStage, false}, + TestParams{DecorationKind::kStride, false}, + TestParams{DecorationKind::kStructBlock, false}, + TestParams{DecorationKind::kWorkgroup, false})); + +using ConstantDecorationTest = TestWithParams; +TEST_P(ConstantDecorationTest, IsValid) { + auto& params = GetParam(); + + GlobalConst("a", ty.f32(), nullptr, + ast::DecorationList{ + createDecoration(Source{{12, 34}}, *this, params.kind)}); + + WrapInFunction(); + + if (params.should_pass) { + EXPECT_TRUE(r()->Resolve()) << r()->error(); + } else { + EXPECT_FALSE(r()->Resolve()) << r()->error(); + EXPECT_EQ(r()->error(), + "12:34 error: decoration is not valid for constants"); + } +} +INSTANTIATE_TEST_SUITE_P( + ResolverDecorationValidationTest, + ConstantDecorationTest, + testing::Values(TestParams{DecorationKind::kAccess, false}, + TestParams{DecorationKind::kAlign, false}, + TestParams{DecorationKind::kBinding, false}, + TestParams{DecorationKind::kBuiltin, false}, + TestParams{DecorationKind::kGroup, false}, + TestParams{DecorationKind::kLocation, false}, + TestParams{DecorationKind::kOverride, true}, TestParams{DecorationKind::kOffset, false}, TestParams{DecorationKind::kSize, false}, TestParams{DecorationKind::kStage, false}, @@ -291,9 +326,9 @@ INSTANTIATE_TEST_SUITE_P( TestParams{DecorationKind::kAlign, false}, TestParams{DecorationKind::kBinding, false}, TestParams{DecorationKind::kBuiltin, false}, - TestParams{DecorationKind::kConstantId, false}, TestParams{DecorationKind::kGroup, false}, TestParams{DecorationKind::kLocation, false}, + TestParams{DecorationKind::kOverride, false}, TestParams{DecorationKind::kOffset, false}, TestParams{DecorationKind::kSize, false}, // Skip kStage as we always apply it in this test diff --git a/src/resolver/resolver.cc b/src/resolver/resolver.cc index 603f3dda05..8d3f35f3ca 100644 --- a/src/resolver/resolver.cc +++ b/src/resolver/resolver.cc @@ -501,11 +501,16 @@ bool Resolver::GlobalVariable(ast::Variable* var) { for (auto* deco : var->decorations()) { Mark(deco); - if (!(deco->Is() || - deco->Is() || - deco->Is() || - deco->Is() || - deco->Is())) { + if (var->is_const()) { + if (!deco->Is()) { + diagnostics_.add_error("decoration is not valid for constants", + deco->source()); + return false; + } + } else if (!(deco->Is() || + deco->Is() || + deco->Is() || + deco->Is())) { diagnostics_.add_error("decoration is not valid for variables", deco->source()); return false;