resolver: Validate storage buffers have the [[block]] decoration

Fixed: tint:94
Change-Id: I1d3e512c030ec16031b8c8fcfbde0cd1db5d1ea4
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/48380
Reviewed-by: Ryan Harrison <rharrison@chromium.org>
Reviewed-by: James Price <jrprice@google.com>
Commit-Queue: Ben Clayton <bclayton@google.com>

src/resolver/host_shareable_validation_test.cc

@@ -15,6 +15,7 @@
 #include "src/resolver/resolver.h"
 
 #include "gmock/gmock.h"
+#include "src/ast/struct_block_decoration.h"
 #include "src/resolver/resolver_test_helper.h"
 #include "src/sem/struct.h"
 #include "src/type/access_control_type.h"
@@ -26,7 +27,8 @@ namespace {
 using ResolverHostShareableValidationTest = ResolverTest;
 
 TEST_F(ResolverHostShareableValidationTest, BoolMember) {
-  auto* s = Structure("S", {Member(Source{{12, 34}}, "x", ty.bool_())});
+  auto* s = Structure("S", {Member(Source{{12, 34}}, "x", ty.bool_())},
+                      {create<ast::StructBlockDecoration>()});
   auto* a = ty.access(ast::AccessControl::kReadOnly, s);
   Global(Source{{56, 78}}, "g", a, ast::StorageClass::kStorage);
 
@@ -40,7 +42,8 @@ TEST_F(ResolverHostShareableValidationTest, BoolMember) {
 }
 
 TEST_F(ResolverHostShareableValidationTest, BoolVectorMember) {
-  auto* s = Structure("S", {Member(Source{{12, 34}}, "x", ty.vec3<bool>())});
+  auto* s = Structure("S", {Member(Source{{12, 34}}, "x", ty.vec3<bool>())},
+                      {create<ast::StructBlockDecoration>()});
   auto* a = ty.access(ast::AccessControl::kReadOnly, s);
   Global(Source{{56, 78}}, "g", a, ast::StorageClass::kStorage);
 
@@ -55,7 +58,8 @@ TEST_F(ResolverHostShareableValidationTest, BoolVectorMember) {
 
 TEST_F(ResolverHostShareableValidationTest, Aliases) {
   auto* a1 = ty.alias("a1", ty.bool_());
-  auto* s = Structure("S", {Member(Source{{12, 34}}, "x", a1)});
+  auto* s = Structure("S", {Member(Source{{12, 34}}, "x", a1)},
+                      {create<ast::StructBlockDecoration>()});
   auto* ac = ty.access(ast::AccessControl::kReadOnly, s);
   auto* a2 = ty.alias("a2", ac);
   Global(Source{{56, 78}}, "g", a2, ast::StorageClass::kStorage);
@@ -74,7 +78,8 @@ TEST_F(ResolverHostShareableValidationTest, NestedStructures) {
   auto* i2 = Structure("I2", {Member(Source{{3, 4}}, "y", i1)});
   auto* i3 = Structure("I3", {Member(Source{{5, 6}}, "z", i2)});
 
-  auto* s = Structure("S", {Member(Source{{7, 8}}, "m", i3)});
+  auto* s = Structure("S", {Member(Source{{7, 8}}, "m", i3)},
+                      {create<ast::StructBlockDecoration>()});
   auto* a = ty.access(ast::AccessControl::kReadOnly, s);
   Global(Source{{9, 10}}, "g", a, ast::StorageClass::kStorage);
 
@@ -109,7 +114,8 @@ TEST_F(ResolverHostShareableValidationTest, NoError) {
                           Member(Source{{6, 1}}, "z3", ty.alias("a2", i2)),
                       });
 
-  auto* s = Structure("S", {Member(Source{{7, 8}}, "m", i3)});
+  auto* s = Structure("S", {Member(Source{{7, 8}}, "m", i3)},
+                      {create<ast::StructBlockDecoration>()});
   auto* a = ty.access(ast::AccessControl::kReadOnly, s);
   Global(Source{{9, 10}}, "g", a, ast::StorageClass::kStorage);
 

src/resolver/resolver.cc

@@ -284,27 +284,6 @@ bool Resolver::GlobalVariable(ast::Variable* var) {
     return false;
   }
 
-  if (info->storage_class == ast::StorageClass::kStorage) {
-    // https://gpuweb.github.io/gpuweb/wgsl/#variable-declaration
-    // Variables in the storage storage class and variables with a storage
-    // texture type must have an access attribute applied to the store type.
-
-    // https://gpuweb.github.io/gpuweb/wgsl/#module-scope-variables
-    // A variable in the storage storage class is a storage buffer variable. Its
-    // store type must be a host-shareable structure type with block attribute,
-    // satisfying the storage class constraints.
-
-    auto* access = info->type->As<type::AccessControl>();
-    auto* str = access ? access->type()->As<type::Struct>() : nullptr;
-    if (!str) {
-      diagnostics_.add_error(
-          "variables declared in the <storage> storage class must be of an "
-          "[[access]] qualified structure type",
-          var->source());
-      return false;
-    }
-  }
-
   for (auto* deco : var->decorations()) {
     Mark(deco);
     if (!(deco->Is<ast::BindingDecoration>() ||
@@ -325,6 +304,10 @@ bool Resolver::GlobalVariable(ast::Variable* var) {
     }
   }
 
+  if (!ValidateGlobalVariable(info)) {
+    return false;
+  }
+
   if (!ApplyStorageClassUsageToType(var->declared_storage_class(), info->type,
                                     var->source())) {
     diagnostics_.add_note("while instantiating variable " +
@@ -336,6 +319,43 @@ bool Resolver::GlobalVariable(ast::Variable* var) {
   return true;
 }
 
+bool Resolver::ValidateGlobalVariable(const VariableInfo* info) {
+  if (info->storage_class == ast::StorageClass::kStorage) {
+    // https://gpuweb.github.io/gpuweb/wgsl/#variable-declaration
+    // Variables in the storage storage class and variables with a storage
+    // texture type must have an access attribute applied to the store type.
+
+    // https://gpuweb.github.io/gpuweb/wgsl/#module-scope-variables
+    // A variable in the storage storage class is a storage buffer variable. Its
+    // store type must be a host-shareable structure type with block attribute,
+    // satisfying the storage class constraints.
+
+    auto* access = info->type->As<type::AccessControl>();
+    auto* str = access ? access->type()->As<type::Struct>() : nullptr;
+    if (!str) {
+      diagnostics_.add_error(
+          "variables declared in the <storage> storage class must be of an "
+          "[[access]] qualified structure type",
+          info->declaration->source());
+      return false;
+    }
+
+    if (!str->IsBlockDecorated()) {
+      diagnostics_.add_error(
+          "structure used as a storage buffer must be declared with the "
+          "[[block]] decoration",
+          str->impl()->source());
+      if (info->declaration->source().range.begin.line) {
+        diagnostics_.add_note("structure used as storage buffer here",
+                              info->declaration->source());
+      }
+      return false;
+    }
+  }
+
+  return true;
+}
+
 bool Resolver::ValidateVariable(const ast::Variable* var) {
   auto* type = variable_to_info_[var]->type;
   if (auto* r = type->UnwrapAll()->As<type::Array>()) {

src/resolver/resolver.h

@@ -240,6 +240,7 @@ class Resolver {
   bool ValidateBinary(ast::BinaryExpression* expr);
   bool ValidateEntryPoint(const ast::Function* func);
   bool ValidateFunction(const ast::Function* func);
+  bool ValidateGlobalVariable(const VariableInfo* var);
   bool ValidateMatrixConstructor(const type::Matrix* matrix_type,
                                  const ast::ExpressionList& values);
   bool ValidateParameter(const ast::Variable* param);

src/resolver/resolver_test.cc

@@ -28,6 +28,7 @@
 #include "src/ast/loop_statement.h"
 #include "src/ast/return_statement.h"
 #include "src/ast/stage_decoration.h"
+#include "src/ast/struct_block_decoration.h"
 #include "src/ast/switch_statement.h"
 #include "src/ast/unary_op_expression.h"
 #include "src/ast/variable_decl_statement.h"
@@ -764,7 +765,8 @@ TEST_F(ResolverTest, Function_Parameters) {
 }
 
 TEST_F(ResolverTest, Function_RegisterInputOutputVariables) {
-  auto* s = Structure("S", {Member("m", ty.u32())});
+  auto* s = Structure("S", {Member("m", ty.u32())},
+                      {create<ast::StructBlockDecoration>()});
   auto* a = ty.access(ast::AccessControl::kReadOnly, s);
 
   auto* in_var = Global("in_var", ty.f32(), ast::StorageClass::kInput);
@@ -799,7 +801,8 @@ TEST_F(ResolverTest, Function_RegisterInputOutputVariables) {
 }
 
 TEST_F(ResolverTest, Function_RegisterInputOutputVariables_SubFunction) {
-  auto* s = Structure("S", {Member("m", ty.u32())});
+  auto* s = Structure("S", {Member("m", ty.u32())},
+                      {create<ast::StructBlockDecoration>()});
   auto* a = ty.access(ast::AccessControl::kReadOnly, s);
 
   auto* in_var = Global("in_var", ty.f32(), ast::StorageClass::kInput);

src/resolver/storage_class_validation_test.cc

@@ -15,6 +15,7 @@
 #include "src/resolver/resolver.h"
 
 #include "gmock/gmock.h"
+#include "src/ast/struct_block_decoration.h"
 #include "src/resolver/resolver_test_helper.h"
 #include "src/sem/struct.h"
 #include "src/type/access_control_type.h"
@@ -34,7 +35,7 @@ TEST_F(ResolverStorageClassValidationTest, GlobalVariableNoStorageClass_Fail) {
             "12:34 error v-0022: global variables must have a storage class");
 }
 
-TEST_F(ResolverStorageClassValidationTest, Bool) {
+TEST_F(ResolverStorageClassValidationTest, StorageBufferBool) {
   // var<storage> g : bool;
   Global(Source{{56, 78}}, "g", ty.bool_(), ast::StorageClass::kStorage);
 
@@ -45,7 +46,7 @@ TEST_F(ResolverStorageClassValidationTest, Bool) {
       R"(56:78 error: variables declared in the <storage> storage class must be of an [[access]] qualified structure type)");
 }
 
-TEST_F(ResolverStorageClassValidationTest, Pointer) {
+TEST_F(ResolverStorageClassValidationTest, StorageBufferPointer) {
   // var<storage> g : ptr<i32, input>;
   Global(Source{{56, 78}}, "g", ty.pointer<i32>(ast::StorageClass::kInput),
          ast::StorageClass::kStorage);
@@ -57,7 +58,7 @@ TEST_F(ResolverStorageClassValidationTest, Pointer) {
       R"(56:78 error: variables declared in the <storage> storage class must be of an [[access]] qualified structure type)");
 }
 
-TEST_F(ResolverStorageClassValidationTest, Array) {
+TEST_F(ResolverStorageClassValidationTest, StorageBufferArray) {
   // var<storage> g : [[access(read)]] array<S, 3>;
   auto* s = Structure("S", {Member("a", ty.f32())});
   auto* a = ty.array(s, 3);
@@ -71,7 +72,7 @@ TEST_F(ResolverStorageClassValidationTest, Array) {
       R"(56:78 error: variables declared in the <storage> storage class must be of an [[access]] qualified structure type)");
 }
 
-TEST_F(ResolverStorageClassValidationTest, BoolAlias) {
+TEST_F(ResolverStorageClassValidationTest, StorageBufferBoolAlias) {
   // type a = bool;
   // var<storage> g : [[access(read)]] a;
   auto* a = ty.alias("a", ty.bool_());
@@ -84,7 +85,7 @@ TEST_F(ResolverStorageClassValidationTest, BoolAlias) {
       R"(56:78 error: variables declared in the <storage> storage class must be of an [[access]] qualified structure type)");
 }
 
-TEST_F(ResolverStorageClassValidationTest, NoAccessControl) {
+TEST_F(ResolverStorageClassValidationTest, StorageBufferNoAccessControl) {
   // var<storage> g : S;
   auto* s = Structure("S", {Member(Source{{12, 34}}, "x", ty.i32())});
   Global(Source{{56, 78}}, "g", s, ast::StorageClass::kStorage);
@@ -96,20 +97,39 @@ TEST_F(ResolverStorageClassValidationTest, NoAccessControl) {
       R"(56:78 error: variables declared in the <storage> storage class must be of an [[access]] qualified structure type)");
 }
 
-TEST_F(ResolverStorageClassValidationTest, NoError_Basic) {
+TEST_F(ResolverStorageClassValidationTest, StorageBufferNoBlockDecoration) {
+  // struct S { x : i32 };
   // var<storage> g : [[access(read)]] S;
-  auto* s = Structure("S", {Member(Source{{12, 34}}, "x", ty.i32())});
+  auto* s = Structure(Source{{12, 34}}, "S", {Member("x", ty.i32())});
+  auto* a = ty.access(ast::AccessControl::kReadOnly, s);
+  Global(Source{{56, 78}}, "g", a, ast::StorageClass::kStorage);
+
+  ASSERT_FALSE(r()->Resolve());
+
+  EXPECT_EQ(
+      r()->error(),
+      R"(12:34 error: structure used as a storage buffer must be declared with the [[block]] decoration
+56:78 note: structure used as storage buffer here)");
+}
+
+TEST_F(ResolverStorageClassValidationTest, StorageBufferNoError_Basic) {
+  // [[block]] struct S { x : i32 };
+  // var<storage> g : [[access(read)]] S;
+  auto* s = Structure("S", {Member(Source{{12, 34}}, "x", ty.i32())},
+                      {create<ast::StructBlockDecoration>()});
   auto* a = ty.access(ast::AccessControl::kReadOnly, s);
   Global(Source{{56, 78}}, "g", a, ast::StorageClass::kStorage);
 
   ASSERT_TRUE(r()->Resolve());
 }
 
-TEST_F(ResolverStorageClassValidationTest, NoError_Aliases) {
+TEST_F(ResolverStorageClassValidationTest, StorageBufferNoError_Aliases) {
+  // [[block]] struct S { x : i32 };
   // type a1 = S;
   // type a2 = [[access(read)]] a1;
   // var<storage> g : a2;
-  auto* s = Structure("S", {Member(Source{{12, 34}}, "x", ty.i32())});
+  auto* s = Structure("S", {Member(Source{{12, 34}}, "x", ty.i32())},
+                      {create<ast::StructBlockDecoration>()});
   auto* a1 = ty.alias("a1", s);
   auto* ac = ty.access(ast::AccessControl::kReadOnly, a1);
   auto* a2 = ty.alias("a2", ac);

src/resolver/struct_storage_class_use_test.cc

@@ -15,6 +15,7 @@
 #include "src/resolver/resolver.h"
 
 #include "gmock/gmock.h"
+#include "src/ast/struct_block_decoration.h"
 #include "src/resolver/resolver_test_helper.h"
 #include "src/sem/struct.h"
 
@@ -167,7 +168,8 @@ TEST_F(ResolverStorageClassUseTest, StructReachableViaLocalArray) {
 }
 
 TEST_F(ResolverStorageClassUseTest, StructMultipleStorageClassUses) {
-  auto* s = Structure("S", {Member("a", ty.f32())});
+  auto* s = Structure("S", {Member("a", ty.f32())},
+                      {create<ast::StructBlockDecoration>()});
   auto* ac = ty.access(ast::AccessControl::kReadOnly, s);
   Global("x", s, ast::StorageClass::kUniform);
   Global("y", ac, ast::StorageClass::kStorage);