resolver: Ensure that decorations aren't duplicated

Fixed: tint:525 Change-Id: I993b60f82ac5d5e07e1c76980604e6aaf1b94fb3 Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/53806 Kokoro: Kokoro <noreply+kokoro@google.com> Reviewed-by: James Price <jrprice@google.com>
2025-12-17 00:47:13 +00:00 · 2021-06-09 18:53:57 +00:00
parent c58738a49e
commit 241c16d626
41 changed files with 320 additions and 39 deletions
--- a/src/resolver/decoration_validation_test.cc
+++ b/src/resolver/decoration_validation_test.cc
@@ -159,6 +159,20 @@ INSTANTIATE_TEST_SUITE_P(
                    TestParams{DecorationKind::kWorkgroup, false},
                    TestParams{DecorationKind::kBindingAndGroup, false}));

+TEST_F(FunctionReturnTypeDecorationTest, DuplicateDecoration) {
+  Func("main", ast::VariableList{}, ty.f32(), ast::StatementList{Return(1.f)},
+       ast::DecorationList{Stage(ast::PipelineStage::kCompute)},
+       ast::DecorationList{
+           Location(Source{{12, 34}}, 2),
+           Location(Source{{56, 78}}, 3),
+       });
+
+  EXPECT_FALSE(r()->Resolve());
+  EXPECT_EQ(r()->error(),
+            R"(56:78 error: duplicate location decoration
+12:34 note: first decoration declared here)");
+}
+
 using ArrayDecorationTest = TestWithParams;
 TEST_P(ArrayDecorationTest, IsValid) {
  auto& params = GetParam();
@@ -232,6 +246,24 @@ INSTANTIATE_TEST_SUITE_P(
                    TestParams{DecorationKind::kWorkgroup, false},
                    TestParams{DecorationKind::kBindingAndGroup, false}));

+TEST_F(StructDecorationTest, DuplicateDecoration) {
+  Structure("mystruct",
+            {
+                Member("a", ty.i32()),
+            },
+            {
+                create<ast::StructBlockDecoration>(Source{{12, 34}}),
+                create<ast::StructBlockDecoration>(Source{{56, 78}}),
+            });
+
+  WrapInFunction();
+
+  EXPECT_FALSE(r()->Resolve());
+  EXPECT_EQ(r()->error(),
+            R"(56:78 error: duplicate block decoration
+12:34 note: first decoration declared here)");
+}
+
 using StructMemberDecorationTest = TestWithParams;
 TEST_P(StructMemberDecorationTest, IsValid) {
  auto& params = GetParam();
@@ -268,6 +300,25 @@ INSTANTIATE_TEST_SUITE_P(
                    TestParams{DecorationKind::kWorkgroup, false},
                    TestParams{DecorationKind::kBindingAndGroup, false}));

+TEST_F(StructMemberDecorationTest, DuplicateDecoration) {
+  Structure("mystruct", {
+                            Member("a", ty.i32(),
+                                   {
+                                       create<ast::StructMemberAlignDecoration>(
+                                           Source{{12, 34}}, 4u),
+                                       create<ast::StructMemberAlignDecoration>(
+                                           Source{{56, 78}}, 8u),
+                                   }),
+                        });
+
+  WrapInFunction();
+
+  EXPECT_FALSE(r()->Resolve());
+  EXPECT_EQ(r()->error(),
+            R"(56:78 error: duplicate align decoration
+12:34 note: first decoration declared here)");
+}
+
 using VariableDecorationTest = TestWithParams;
 TEST_P(VariableDecorationTest, IsValid) {
  auto& params = GetParam();
@@ -310,6 +361,22 @@ INSTANTIATE_TEST_SUITE_P(
                    TestParams{DecorationKind::kWorkgroup, false},
                    TestParams{DecorationKind::kBindingAndGroup, true}));

+TEST_F(VariableDecorationTest, DuplicateDecoration) {
+  Global("a", ty.sampler(ast::SamplerKind::kSampler),
+         ast::DecorationList{
+             create<ast::BindingDecoration>(Source{{12, 34}}, 2),
+             create<ast::GroupDecoration>(2),
+             create<ast::BindingDecoration>(Source{{56, 78}}, 3),
+         });
+
+  WrapInFunction();
+
+  EXPECT_FALSE(r()->Resolve());
+  EXPECT_EQ(r()->error(),
+            R"(56:78 error: duplicate binding decoration
+12:34 note: first decoration declared here)");
+}
+
 using ConstantDecorationTest = TestWithParams;
 TEST_P(ConstantDecorationTest, IsValid) {
  auto& params = GetParam();
@@ -344,6 +411,21 @@ INSTANTIATE_TEST_SUITE_P(
                    TestParams{DecorationKind::kWorkgroup, false},
                    TestParams{DecorationKind::kBindingAndGroup, false}));

+TEST_F(ConstantDecorationTest, DuplicateDecoration) {
+  GlobalConst("a", ty.f32(), Expr(1.23f),
+              ast::DecorationList{
+                  create<ast::OverrideDecoration>(Source{{12, 34}}),
+                  create<ast::OverrideDecoration>(Source{{56, 78}}, 1),
+              });
+
+  WrapInFunction();
+
+  EXPECT_FALSE(r()->Resolve());
+  EXPECT_EQ(r()->error(),
+            R"(56:78 error: duplicate override decoration
+12:34 note: first decoration declared here)");
+}
+
 using FunctionDecorationTest = TestWithParams;
 TEST_P(FunctionDecorationTest, IsValid) {
  auto& params = GetParam();
@@ -485,18 +567,19 @@ INSTANTIATE_TEST_SUITE_P(
        ParamsFor<mat3x3<f32>>((default_mat3x3.align - 1) * 7, false),
        ParamsFor<mat4x4<f32>>((default_mat4x4.align - 1) * 7, false)));

-TEST_F(ArrayStrideTest, MultipleDecorations) {
+TEST_F(ArrayStrideTest, DuplicateDecoration) {
  auto* arr = ty.array(Source{{12, 34}}, ty.i32(), 4,
                       {
-                           create<ast::StrideDecoration>(4),
-                           create<ast::StrideDecoration>(4),
+                           create<ast::StrideDecoration>(Source{{12, 34}}, 4),
+                           create<ast::StrideDecoration>(Source{{56, 78}}, 4),
                       });

  Global("myarray", arr, ast::StorageClass::kInput);

  EXPECT_FALSE(r()->Resolve());
  EXPECT_EQ(r()->error(),
-            "12:34 error: array must have at most one [[stride]] decoration");
+            R"(56:78 error: duplicate stride decoration
+12:34 note: first decoration declared here)");
 }

 }  // namespace
@@ -700,15 +783,18 @@ TEST_F(WorkgroupDecoration, NotAComputeShader) {
            "compute stages");
 }

-TEST_F(WorkgroupDecoration, MultipleAttributes) {
+TEST_F(WorkgroupDecoration, DuplicateDecoration) {
  Func(Source{{12, 34}}, "main", {}, ty.void_(), {},
-       {Stage(ast::PipelineStage::kCompute), WorkgroupSize(1),
-        WorkgroupSize(2)});
+       {
+           Stage(ast::PipelineStage::kCompute),
+           WorkgroupSize(Source{{12, 34}}, 1, nullptr, nullptr),
+           WorkgroupSize(Source{{56, 78}}, 2, nullptr, nullptr),
+       });

  EXPECT_FALSE(r()->Resolve());
  EXPECT_EQ(r()->error(),
-            "12:34 error: only one workgroup_size attribute permitted per "
-            "entry point");
+            R"(56:78 error: duplicate workgroup_size decoration
+12:34 note: first decoration declared here)");
 }

 }  // namespace
--- a/src/resolver/function_validation_test.cc
+++ b/src/resolver/function_validation_test.cc
@@ -267,14 +267,14 @@ TEST_F(ResolverFunctionValidationTest, PipelineStage_MustBeUnique_Fail) {
           Return(),
       },
       ast::DecorationList{
-           Stage(ast::PipelineStage::kVertex),
-           Stage(ast::PipelineStage::kFragment),
+           Stage(Source{{12, 34}}, ast::PipelineStage::kVertex),
+           Stage(Source{{56, 78}}, ast::PipelineStage::kFragment),
       });

  EXPECT_FALSE(r()->Resolve());
  EXPECT_EQ(r()->error(),
-            "12:34 error v-0020: only one stage decoration permitted per entry "
-            "point");
+            R"(56:78 error: duplicate stage decoration
+12:34 note: first decoration declared here)");
 }

 TEST_F(ResolverFunctionValidationTest, NoPipelineEntryPoints) {
--- a/src/resolver/resolver.cc
+++ b/src/resolver/resolver.cc
@@ -573,6 +573,10 @@ bool Resolver::GlobalVariable(ast::Variable* var) {
    }
  }

+  if (!ValidateNoDuplicateDecorations(var->decorations())) {
+    return false;
+  }
+
  if (auto bp = var->binding_point()) {
    info->binding_point = {bp.group->value(), bp.binding->value()};
  }
@@ -608,6 +612,10 @@ bool Resolver::ValidateGlobalVariable(const VariableInfo* info) {
    return false;
  }

+  if (!ValidateNoDuplicateDecorations(info->declaration->decorations())) {
+    return false;
+  }
+
  for (auto* deco : info->declaration->decorations()) {
    if (info->declaration->is_const()) {
      if (auto* override_deco = deco->As<ast::OverrideDecoration>()) {
@@ -872,18 +880,6 @@ bool Resolver::ValidateFunction(const ast::Function* func,
      return false;
    }
  }
-  if (stage_deco_count > 1) {
-    diagnostics_.add_error(
-        "v-0020", "only one stage decoration permitted per entry point",
-        func->source());
-    return false;
-  }
-  if (workgroup_deco_count > 1) {
-    diagnostics_.add_error(
-        "only one workgroup_size attribute permitted per entry point",
-        func->source());
-    return false;
-  }

  for (auto* param : func->params()) {
    if (!ValidateParameter(variable_to_info_.at(param))) {
@@ -1197,6 +1193,9 @@ bool Resolver::Function(ast::Function* func) {
    for (auto* deco : param->decorations()) {
      Mark(deco);
    }
+    if (!ValidateNoDuplicateDecorations(param->decorations())) {
+      return false;
+    }

    variable_stack_.set(param->symbol(), param_info);
    info->parameters.emplace_back(param_info);
@@ -1282,9 +1281,16 @@ bool Resolver::Function(ast::Function* func) {
  for (auto* deco : func->decorations()) {
    Mark(deco);
  }
+  if (!ValidateNoDuplicateDecorations(func->decorations())) {
+    return false;
+  }
+
  for (auto* deco : func->return_type_decorations()) {
    Mark(deco);
  }
+  if (!ValidateNoDuplicateDecorations(func->return_type_decorations())) {
+    return false;
+  }

  // Set work-group size defaults.
  for (int i = 0; i < 3; i++) {
@@ -2777,16 +2783,15 @@ sem::Array* Resolver::Array(const ast::Array* arr) {
    return nullptr;
  }

+  if (!ValidateNoDuplicateDecorations(arr->decorations())) {
+    return nullptr;
+  }
+
  // Look for explicit stride via [[stride(n)]] decoration
  uint32_t explicit_stride = 0;
  for (auto* deco : arr->decorations()) {
    Mark(deco);
    if (auto* sd = deco->As<ast::StrideDecoration>()) {
-      if (explicit_stride) {
-        diagnostics_.add_error(
-            "array must have at most one [[stride]] decoration", source);
-        return nullptr;
-      }
      explicit_stride = sd->stride();
      if (!ValidateArrayStrideDecoration(sd, el_size, el_align, source)) {
        return nullptr;
@@ -2916,6 +2921,9 @@ bool Resolver::ValidateStructure(const sem::Struct* str) {
 }

 sem::Struct* Resolver::Structure(const ast::Struct* str) {
+  if (!ValidateNoDuplicateDecorations(str->decorations())) {
+    return nullptr;
+  }
  for (auto* deco : str->decorations()) {
    Mark(deco);
  }
@@ -2961,6 +2969,10 @@ sem::Struct* Resolver::Structure(const ast::Struct* str) {
      return nullptr;
    }

+    if (!ValidateNoDuplicateDecorations(member->decorations())) {
+      return nullptr;
+    }
+
    bool has_offset_deco = false;
    bool has_align_deco = false;
    bool has_size_deco = false;
@@ -3201,6 +3213,22 @@ bool Resolver::ValidateAssignment(const ast::AssignmentStatement* a) {
  return true;
 }

+bool Resolver::ValidateNoDuplicateDecorations(
+    const ast::DecorationList& decorations) {
+  std::unordered_map<const TypeInfo*, Source> seen;
+  for (auto* d : decorations) {
+    auto res = seen.emplace(&d->TypeInfo(), d->source());
+    if (!res.second) {
+      diagnostics_.add_error("duplicate " + d->name() + " decoration",
+                             d->source());
+      diagnostics_.add_note("first decoration declared here",
+                            res.first->second);
+      return false;
+    }
+  }
+  return true;
+}
+
 bool Resolver::ApplyStorageClassUsageToType(ast::StorageClass sc,
                                            sem::Type* ty,
                                            const Source& usage) {
--- a/src/resolver/resolver.h
+++ b/src/resolver/resolver.h
@@ -271,6 +271,7 @@ class Resolver {
  bool ValidateVectorConstructor(const ast::TypeConstructorExpression* ctor,
                                 const sem::Vector* vec_type);
  bool ValidateTypeDecl(const ast::TypeDecl* named_type) const;
+  bool ValidateNoDuplicateDecorations(const ast::DecorationList& decorations);

  /// @returns the sem::Type for the ast::Type `ty`, building it if it
  /// hasn't been constructed already. If an error is raised, nullptr is