Google Git
Sign in
dawn/tint/refs/heads/chromium/4591^!/.
commit
72494048277a6191c8bed6004a353b767197b28f
[log]
author
Sarah <sarahmashay@google.com>
Wed Jul 28 22:43:36 2021 +0000
committer
Tint LUCI CQ <tint-scoped@luci-project-accounts.iam.gserviceaccount.com>
Wed Jul 28 22:43:36 2021 +0000
tree
e4d48209c8c00be63f98f162d1a9916689d7ad59
parent
9ba6500c3f6e17a2375f98b9451beba4a45480a8 [diff]
validation: function scope variable store type must be constructible

- function scope variable store type must be constructible
- add IsConstructible() to sem::atomic

Bug: tint:1069
Change-Id: Ib0616b486ecf278dbdd99640dc4ede7f3007feb8
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/60120
Kokoro: Kokoro <noreply+kokoro@google.com>
Reviewed-by: Ben Clayton <bclayton@google.com>
Commit-Queue: Sarah Mashayekhi <sarahmashay@google.com>
Auto-Submit: Sarah Mashayekhi <sarahmashay@google.com>

diff --git a/src/ast/module_clone_test.cc b/src/ast/module_clone_test.cc
index 2a78bb5..8d3c1a1 100644
--- a/src/ast/module_clone_test.cc
+++ b/src/ast/module_clone_test.cc

@@ -27,12 +27,18 @@
   // Shader that exercises the bulk of the AST nodes and types.
   // See also fuzzers/tint_ast_clone_fuzzer.cc for further coverage of cloning.
   Source::File file("test.wgsl", R"([[block]]
-struct S {
+struct S0 {
   [[size(4)]]
   m0 : u32;
   m1 : array<u32>;
 };
 
+[[block]] struct S1 {
+  [[size(4)]]
+  m0 : u32;
+  m1 : array<u32, 6>;
+};
+
 let c0 : i32 = 10;
 let c1 : bool = true;
 
@@ -48,9 +54,9 @@
 [[group(4), binding(0)]] var g6 : texture_storage_2d<rg32float, write>;
 
 var<private> g7 : vec3<f32>;
-[[group(0), binding(1)]] var<storage, write> g8 : S;
-[[group(1), binding(1)]] var<storage, read> g9 : S;
-[[group(2), binding(1)]] var<storage, read_write> g10 : S;
+[[group(0), binding(1)]] var<storage, write> g8 : S0;
+[[group(1), binding(1)]] var<storage, read> g9 : S0;
+[[group(2), binding(1)]] var<storage, read_write> g10 : S0;
 
 fn f0(p0 : bool) -> f32 {
   if (p0) {
@@ -64,7 +70,7 @@
   var l1 : f32 = 8.0;
   var l2 : u32 = bitcast<u32>(4);
   var l3 : vec2<u32> = vec2<u32>(u32(l0), u32(l1));
-  var l4 : S;
+  var l4 : S1;
   var l5 : u32 = l4.m1[5];
   let l6 : ptr<private, u32> = &g0;
   loop {

diff --git a/src/resolver/atomics_validation_test.cc b/src/resolver/atomics_validation_test.cc
index b480eae..73181dc 100644
--- a/src/resolver/atomics_validation_test.cc
+++ b/src/resolver/atomics_validation_test.cc

@@ -34,7 +34,8 @@
   EXPECT_EQ(r()->error(), "12:34 error: atomic only supports i32 or u32 types");
 }
 
-TEST_F(ResolverAtomicValidationTest, GlobalOfInvalidStorageClass) {
+// TODO(crbug.com/tint/909): add validation and enable this test
+TEST_F(ResolverAtomicValidationTest, DISABLED_GlobalOfInvalidStorageClass) {
   Global("a", ty.atomic(Source{{12, 34}}, ty.i32()),
          ast::StorageClass::kPrivate);
 
@@ -60,7 +61,8 @@
   WrapInFunction(Var("a", ty.atomic(Source{{12, 34}}, ty.i32())));
 
   EXPECT_FALSE(r()->Resolve());
-  EXPECT_EQ(r()->error(), "12:34 error: atomic var requires workgroup storage");
+  EXPECT_EQ(r()->error(),
+            "12:34 error: function variable must have a constructible type");
 }
 
 TEST_F(ResolverAtomicValidationTest, NoAtomicExpr) {

diff --git a/src/resolver/resolver.cc b/src/resolver/resolver.cc
index 58aaf57..6c8341c 100644
--- a/src/resolver/resolver.cc
+++ b/src/resolver/resolver.cc

@@ -1122,19 +1122,6 @@
     AddError("invalid use of input/output storage class", var->source());
     return false;
   }
-
-  // https://gpuweb.github.io/gpuweb/wgsl/#atomic-types
-  // Atomic types may only be instantiated by variables in the workgroup storage
-  // class or by storage buffer variables with a read_write access mode.
-  if (info->type->UnwrapRef()->Is<sem::Atomic>() &&
-      info->storage_class != ast::StorageClass::kWorkgroup) {
-    // Storage buffers require a structure, so just check for workgroup
-    // storage here.
-    AddError("atomic var requires workgroup storage",
-             info->declaration->type()->source());
-    return false;
-  }
-
   return true;
 }
 
@@ -3370,10 +3357,15 @@
     return false;
   }
 
-  if (!var->is_const()) {
-    if (info->storage_class != ast::StorageClass::kFunction &&
-        IsValidationEnabled(var->decorations(),
-                            ast::DisabledValidation::kIgnoreStorageClass)) {
+  if (!var->is_const() &&
+      IsValidationEnabled(var->decorations(),
+                          ast::DisabledValidation::kIgnoreStorageClass)) {
+    if (!info->type->UnwrapRef()->IsConstructible()) {
+      AddError("function variable must have a constructible type",
+               var->type()->source());
+      return false;
+    }
+    if (info->storage_class != ast::StorageClass::kFunction) {
       if (info->storage_class != ast::StorageClass::kNone) {
         AddError("function variable has a non-function storage class",
                  stmt->source());

diff --git a/src/resolver/validation_test.cc b/src/resolver/validation_test.cc
index c34fd7d..eb00dcd 100644
--- a/src/resolver/validation_test.cc
+++ b/src/resolver/validation_test.cc

@@ -352,8 +352,8 @@
             "error: function variable has a non-function storage class");
 }
 
-TEST_F(ResolverValidationTest, StorageClass_FunctionVariableHandleClass) {
-  auto* var = Var("s", ty.sampler(ast::SamplerKind::kSampler));
+TEST_F(ResolverValidationTest, StorageClass_FunctionVariableI32) {
+  auto* var = Var("s", ty.i32(), ast::StorageClass::kPrivate);
 
   auto* stmt = Decl(var);
   Func("func", ast::VariableList{}, ty.void_(), ast::StatementList{stmt},

diff --git a/src/resolver/var_let_validation_test.cc b/src/resolver/var_let_validation_test.cc
index 7ac9324..43488d1 100644
--- a/src/resolver/var_let_validation_test.cc
+++ b/src/resolver/var_let_validation_test.cc

@@ -298,6 +298,35 @@
             "'ptr<storage, i32, read>'");
 }
 
+TEST_F(ResolverVarLetValidationTest, NonConstructibleType_Atomic) {
+  auto* v = Var("v", ty.atomic(Source{{12, 34}}, ty.i32()));
+  WrapInFunction(v);
+
+  EXPECT_FALSE(r()->Resolve());
+  EXPECT_EQ(r()->error(),
+            "12:34 error: function variable must have a constructible type");
+}
+
+TEST_F(ResolverVarLetValidationTest, NonConstructibleType_RuntimeArray) {
+  auto* s = Structure("S", {Member("m", ty.array(ty.i32()))}, {StructBlock()});
+  auto* v = Var("v", ty.Of(s));
+  WrapInFunction(v);
+
+  EXPECT_FALSE(r()->Resolve());
+  EXPECT_EQ(r()->error(),
+            "error: function variable must have a constructible type");
+}
+
+TEST_F(ResolverVarLetValidationTest, NonConstructibleType_Struct_WithAtomic) {
+  auto* s = Structure("S", {Member("m", ty.atomic(ty.i32()))});
+  auto* v = Var("v", ty.Of(s));
+  WrapInFunction(v);
+
+  EXPECT_FALSE(r()->Resolve());
+  EXPECT_EQ(r()->error(),
+            "error: function variable must have a constructible type");
+}
+
 }  // namespace
 }  // namespace resolver
 }  // namespace tint

diff --git a/src/sem/atomic_type.cc b/src/sem/atomic_type.cc
index 18fc5d2..59a7ddf 100644
--- a/src/sem/atomic_type.cc
+++ b/src/sem/atomic_type.cc

@@ -46,6 +46,10 @@
   return subtype_->Align();
 }
 
+bool Atomic::IsConstructible() const {
+  return false;
+}
+
 Atomic::Atomic(Atomic&&) = default;
 
 Atomic::~Atomic() = default;

diff --git a/src/sem/atomic_type.h b/src/sem/atomic_type.h
index c284d1d..0fa74c6 100644
--- a/src/sem/atomic_type.h
+++ b/src/sem/atomic_type.h

@@ -50,6 +50,10 @@
   /// @returns the alignment in bytes of the type.
   uint32_t Align() const override;
 
+  /// @returns true if constructible as per
+  /// https://gpuweb.github.io/gpuweb/wgsl/#constructible-typesd
+  bool IsConstructible() const override;
+
  private:
   sem::Type const* const subtype_;
 };