validation: Fix array storage class validation

We weren't recursing into array element types, which meant that we
weren't validating nested arrays or buffers whose top-level store-type
was an array.

Change-Id: Ib897b36e0b5c3de3dc67c4f60805411c014cd914
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/77561
Reviewed-by: Ben Clayton <bclayton@google.com>
Kokoro: Kokoro <noreply+kokoro@google.com>

src/resolver/resolver.h

@@ -108,8 +108,8 @@ class Resolver {
   /// Describes the context in which a variable is declared
   enum class VariableKind { kParameter, kLocal, kGlobal };
 
-  std::set<std::pair<const sem::Struct*, ast::StorageClass>>
+  std::set<std::pair<const sem::Type*, ast::StorageClass>>
-      valid_struct_storage_layouts_;
+      valid_type_storage_layouts_;
 
   /// Structure holding semantic information about a block (i.e. scope), such as
   /// parent block and variables declared in the block.
@@ -292,9 +292,9 @@ class Resolver {
                                       const sem::Array* arr_type);
   bool ValidateTextureIntrinsicFunction(const sem::Call* call);
   bool ValidateNoDuplicateDecorations(const ast::DecorationList& decorations);
-  // sem::Struct is assumed to have at least one member
+  bool ValidateStorageClassLayout(const sem::Type* type,
-  bool ValidateStorageClassLayout(const sem::Struct* type,
+                                  ast::StorageClass sc,
-                                  ast::StorageClass sc);
+                                  Source source);
   bool ValidateStorageClassLayout(const sem::Variable* var);
 
   /// @returns true if the decoration list contains a

src/resolver/resolver_validation.cc

@@ -229,8 +229,9 @@ bool Resolver::ValidateVariableConstructorOrCast(
   return true;
 }
 
-bool Resolver::ValidateStorageClassLayout(const sem::Struct* str,
+bool Resolver::ValidateStorageClassLayout(const sem::Type* store_ty,
-                                          ast::StorageClass sc) {
+                                          ast::StorageClass sc,
+                                          Source source) {
   // https://gpuweb.github.io/gpuweb/wgsl/#storage-class-layout-constraints
 
   auto is_uniform_struct_or_array = [sc](const sem::Type* ty) {
@@ -255,110 +256,125 @@ bool Resolver::ValidateStorageClassLayout(const sem::Struct* str,
     return builder_->Symbols().NameFor(sm->Declaration()->symbol);
   };
 
+  // Cache result of type + storage class pair.
+  if (!valid_type_storage_layouts_.emplace(store_ty, sc).second) {
+    return true;
+  }
+
   if (!ast::IsHostShareable(sc)) {
     return true;
   }
 
-  for (size_t i = 0; i < str->Members().size(); ++i) {
+  if (auto* str = store_ty->As<sem::Struct>()) {
-    auto* const m = str->Members()[i];
+    for (size_t i = 0; i < str->Members().size(); ++i) {
-    uint32_t required_align = required_alignment_of(m->Type());
+      auto* const m = str->Members()[i];
+      uint32_t required_align = required_alignment_of(m->Type());
 
-    // Validate that member is at a valid byte offset
+      // Recurse into the member type.
-    if (m->Offset() % required_align != 0) {
+      if (!ValidateStorageClassLayout(m->Type(), sc,
-      AddError("the offset of a struct member of type '" +
+                                      m->Declaration()->type->source)) {
-                   m->Type()->UnwrapRef()->FriendlyName(builder_->Symbols()) +
+        AddNote("see layout of struct:\n" + str->Layout(builder_->Symbols()),
-                   "' in storage class '" + ast::ToString(sc) +
+                str->Declaration()->source);
-                   "' must be a multiple of " + std::to_string(required_align) +
+        return false;
-                   " bytes, but '" + member_name_of(m) +
-                   "' is currently at offset " + std::to_string(m->Offset()) +
-                   ". Consider setting @align(" +
-                   std::to_string(required_align) + ") on this member",
-               m->Declaration()->source);
-
-      AddNote("see layout of struct:\n" + str->Layout(builder_->Symbols()),
-              str->Declaration()->source);
-
-      if (auto* member_str = m->Type()->As<sem::Struct>()) {
-        AddNote("and layout of struct member:\n" +
-                    member_str->Layout(builder_->Symbols()),
-                member_str->Declaration()->source);
       }
 
-      return false;
+      // Validate that member is at a valid byte offset
-    }
+      if (m->Offset() % required_align != 0) {
-
+        AddError("the offset of a struct member of type '" +
-    // For uniform buffers, validate that the number of bytes between the
+                     m->Type()->UnwrapRef()->FriendlyName(builder_->Symbols()) +
-    // previous member of type struct and the current is a multiple of 16 bytes.
+                     "' in storage class '" + ast::ToString(sc) +
-    auto* const prev_member = (i == 0) ? nullptr : str->Members()[i - 1];
+                     "' must be a multiple of " +
-    if (prev_member && is_uniform_struct(prev_member->Type())) {
+                     std::to_string(required_align) + " bytes, but '" +
-      const uint32_t prev_to_curr_offset = m->Offset() - prev_member->Offset();
+                     member_name_of(m) + "' is currently at offset " +
-      if (prev_to_curr_offset % 16 != 0) {
+                     std::to_string(m->Offset()) +
-        AddError(
+                     ". Consider setting @align(" +
-            "uniform storage requires that the number of bytes between the "
+                     std::to_string(required_align) + ") on this member",
-            "start of the previous member of type struct and the current "
+                 m->Declaration()->source);
-            "member be a multiple of 16 bytes, but there are currently " +
-                std::to_string(prev_to_curr_offset) + " bytes between '" +
-                member_name_of(prev_member) + "' and '" + member_name_of(m) +
-                "'. Consider setting @align(16) on this member",
-            m->Declaration()->source);
 
         AddNote("see layout of struct:\n" + str->Layout(builder_->Symbols()),
                 str->Declaration()->source);
 
-        auto* prev_member_str = prev_member->Type()->As<sem::Struct>();
+        if (auto* member_str = m->Type()->As<sem::Struct>()) {
-        AddNote("and layout of previous member struct:\n" +
+          AddNote("and layout of struct member:\n" +
-                    prev_member_str->Layout(builder_->Symbols()),
+                      member_str->Layout(builder_->Symbols()),
-                prev_member_str->Declaration()->source);
+                  member_str->Declaration()->source);
+        }
+
         return false;
       }
-    }
 
-    // For uniform buffer array members, validate that array elements are
+      // For uniform buffers, validate that the number of bytes between the
-    // aligned to 16 bytes
+      // previous member of type struct and the current is a multiple of 16
-    if (auto* arr = m->Type()->As<sem::Array>()) {
+      // bytes.
-      if (sc == ast::StorageClass::kUniform) {
+      auto* const prev_member = (i == 0) ? nullptr : str->Members()[i - 1];
-        // We already validated that this array member is itself aligned to 16
+      if (prev_member && is_uniform_struct(prev_member->Type())) {
-        // bytes above, so we only need to validate that stride is a multiple of
+        const uint32_t prev_to_curr_offset =
-        // 16 bytes.
+            m->Offset() - prev_member->Offset();
-        if (arr->Stride() % 16 != 0) {
+        if (prev_to_curr_offset % 16 != 0) {
-          // Since WGSL has no stride attribute, try to provide a useful hint
-          // for how the shader author can resolve the issue.
-          std::string hint;
-          if (arr->ElemType()->is_scalar()) {
-            hint =
-                "Consider using a vector or struct as the element type "
-                "instead.";
-          } else if (auto* vec = arr->ElemType()->As<sem::Vector>();
-                     vec && vec->type()->Size() == 4) {
-            hint = "Consider using a vec4 instead.";
-          } else if (arr->ElemType()->Is<sem::Struct>()) {
-            hint =
-                "Consider using the @size attribute on the last struct member.";
-          } else {
-            hint =
-                "Consider wrapping the element type in a struct and using the "
-                "@size attribute.";
-          }
           AddError(
-              "uniform storage requires that array elements be aligned to 16 "
+              "uniform storage requires that the number of bytes between the "
-              "bytes, but array element alignment of '" +
+              "start of the previous member of type struct and the current "
-                  member_name_of(m) + "' is currently " +
+              "member be a multiple of 16 bytes, but there are currently " +
-                  std::to_string(arr->Stride()) + ". " + hint,
+                  std::to_string(prev_to_curr_offset) + " bytes between '" +
-              m->Declaration()->type->source);
+                  member_name_of(prev_member) + "' and '" + member_name_of(m) +
+                  "'. Consider setting @align(16) on this member",
+              m->Declaration()->source);
+
           AddNote("see layout of struct:\n" + str->Layout(builder_->Symbols()),
                   str->Declaration()->source);
+
+          auto* prev_member_str = prev_member->Type()->As<sem::Struct>();
+          AddNote("and layout of previous member struct:\n" +
+                      prev_member_str->Layout(builder_->Symbols()),
+                  prev_member_str->Declaration()->source);
           return false;
         }
       }
     }
+  }
 
-    // If member is struct, recurse
+  // For uniform buffer array members, validate that array elements are
-    if (auto* str_member = m->Type()->As<sem::Struct>()) {
+  // aligned to 16 bytes
-      // Cache result of struct + storage class pair
+  if (auto* arr = store_ty->As<sem::Array>()) {
-      if (valid_struct_storage_layouts_.emplace(str_member, sc).second) {
+    // Recurse into the element type.
-        if (!ValidateStorageClassLayout(str_member, sc)) {
+    // TODO(crbug.com/tint/1388): Ideally we'd pass the source for nested
-          return false;
+    // element type here, but we can't easily get that from the semantic node.
+    // We should consider recursing through the AST type nodes instead.
+    if (!ValidateStorageClassLayout(arr->ElemType(), sc, source)) {
+      return false;
+    }
+
+    if (sc == ast::StorageClass::kUniform) {
+      // We already validated that this array member is itself aligned to 16
+      // bytes above, so we only need to validate that stride is a multiple
+      // of 16 bytes.
+      if (arr->Stride() % 16 != 0) {
+        // Since WGSL has no stride attribute, try to provide a useful hint
+        // for how the shader author can resolve the issue.
+        std::string hint;
+        if (arr->ElemType()->is_scalar()) {
+          hint =
+              "Consider using a vector or struct as the element type "
+              "instead.";
+        } else if (auto* vec = arr->ElemType()->As<sem::Vector>();
+                   vec && vec->type()->Size() == 4) {
+          hint = "Consider using a vec4 instead.";
+        } else if (arr->ElemType()->Is<sem::Struct>()) {
+          hint =
+              "Consider using the @size attribute on the last struct "
+              "member.";
+        } else {
+          hint =
+              "Consider wrapping the element type in a struct and using "
+              "the "
+              "@size attribute.";
         }
+        AddError(
+            "uniform storage requires that array elements be aligned to 16 "
+            "bytes, but array element alignment is currently " +
+                std::to_string(arr->Stride()) + ". " + hint,
+            source);
+        return false;
       }
     }
   }
@@ -368,10 +384,20 @@ bool Resolver::ValidateStorageClassLayout(const sem::Struct* str,
 
 bool Resolver::ValidateStorageClassLayout(const sem::Variable* var) {
   if (auto* str = var->Type()->UnwrapRef()->As<sem::Struct>()) {
-    if (!ValidateStorageClassLayout(str, var->StorageClass())) {
+    if (!ValidateStorageClassLayout(str, var->StorageClass(),
+                                    str->Declaration()->source)) {
       AddNote("see declaration of variable", var->Declaration()->source);
       return false;
     }
+  } else {
+    Source source = var->Declaration()->source;
+    if (var->Declaration()->type) {
+      source = var->Declaration()->type->source;
+    }
+    if (!ValidateStorageClassLayout(var->Type()->UnwrapRef(),
+                                    var->StorageClass(), source)) {
+      return false;
+    }
   }
 
   return true;

src/resolver/storage_class_layout_validation_test.cc

@@ -405,7 +405,7 @@ TEST_F(ResolverStorageClassLayoutValidationTest,
   ASSERT_FALSE(r()->Resolve());
   EXPECT_EQ(
       r()->error(),
-      R"(34:56 error: uniform storage requires that array elements be aligned to 16 bytes, but array element alignment of 'inner' is currently 4. Consider using a vector or struct as the element type instead.
+      R"(34:56 error: uniform storage requires that array elements be aligned to 16 bytes, but array element alignment is currently 4. Consider using a vector or struct as the element type instead.
 12:34 note: see layout of struct:
 /*            align(4) size(44) */ struct Outer {
 /* offset( 0) align(4) size(40) */   inner : array<f32, 10>;
@@ -442,7 +442,7 @@ TEST_F(ResolverStorageClassLayoutValidationTest,
   ASSERT_FALSE(r()->Resolve());
   EXPECT_EQ(
       r()->error(),
-      R"(34:56 error: uniform storage requires that array elements be aligned to 16 bytes, but array element alignment of 'inner' is currently 8. Consider using a vec4 instead.
+      R"(34:56 error: uniform storage requires that array elements be aligned to 16 bytes, but array element alignment is currently 8. Consider using a vec4 instead.
 12:34 note: see layout of struct:
 /*            align(8) size(88) */ struct Outer {
 /* offset( 0) align(8) size(80) */   inner : array<vec2<f32>, 10>;
@@ -488,7 +488,7 @@ TEST_F(ResolverStorageClassLayoutValidationTest,
   ASSERT_FALSE(r()->Resolve());
   EXPECT_EQ(
       r()->error(),
-      R"(34:56 error: uniform storage requires that array elements be aligned to 16 bytes, but array element alignment of 'inner' is currently 8. Consider using the @size attribute on the last struct member.
+      R"(34:56 error: uniform storage requires that array elements be aligned to 16 bytes, but array element alignment is currently 8. Consider using the @size attribute on the last struct member.
 12:34 note: see layout of struct:
 /*            align(4) size(84) */ struct Outer {
 /* offset( 0) align(4) size(80) */   inner : array<ArrayElem, 10>;
@@ -497,6 +497,49 @@ TEST_F(ResolverStorageClassLayoutValidationTest,
 78:90 note: see declaration of variable)");
 }
 
+TEST_F(ResolverStorageClassLayoutValidationTest,
+       UniformBuffer_InvalidArrayStride_TopLevelArray) {
+  // @group(0) @binding(0)
+  // var<uniform> a : array<f32, 4>;
+  Global(Source{{78, 90}}, "a", ty.array(Source{{34, 56}}, ty.f32(), 4),
+         ast::StorageClass::kUniform, GroupAndBinding(0, 0));
+
+  ASSERT_FALSE(r()->Resolve());
+  EXPECT_EQ(
+      r()->error(),
+      R"(34:56 error: uniform storage requires that array elements be aligned to 16 bytes, but array element alignment is currently 4. Consider using a vector or struct as the element type instead.)");
+}
+
+TEST_F(ResolverStorageClassLayoutValidationTest,
+       UniformBuffer_InvalidArrayStride_NestedArray) {
+  // struct Outer {
+  //   inner : array<array<f32, 4>, 4>
+  // };
+  //
+  // @group(0) @binding(0)
+  // var<uniform> a : array<Outer, 4>;
+
+  Structure(
+      Source{{12, 34}}, "Outer",
+      {
+          Member("inner", ty.array(Source{{34, 56}}, ty.array(ty.f32(), 4), 4)),
+      },
+      {StructBlock()});
+
+  Global(Source{{78, 90}}, "a", ty.type_name("Outer"),
+         ast::StorageClass::kUniform, GroupAndBinding(0, 0));
+
+  ASSERT_FALSE(r()->Resolve());
+  EXPECT_EQ(
+      r()->error(),
+      R"(34:56 error: uniform storage requires that array elements be aligned to 16 bytes, but array element alignment is currently 4. Consider using a vector or struct as the element type instead.
+12:34 note: see layout of struct:
+/*            align(4) size(64) */ struct Outer {
+/* offset( 0) align(4) size(64) */   inner : array<array<f32, 4>, 4>;
+/*                              */ };
+78:90 note: see declaration of variable)");
+}
+
 TEST_F(ResolverStorageClassLayoutValidationTest,
        UniformBuffer_InvalidArrayStride_SuggestedFix) {
   // type Inner = @stride(16) array<f32, 10>;

src/resolver/storage_class_validation_test.cc

@@ -302,8 +302,11 @@ TEST_F(ResolverStorageClassValidationTest, UniformBufferVector) {
 }
 
 TEST_F(ResolverStorageClassValidationTest, UniformBufferArray) {
+  // struct S {
+  //   @size(16) f : f32;
+  // }
   // var<uniform> g : array<S, 3>;
-  auto* s = Structure("S", {Member("a", ty.f32())});
+  auto* s = Structure("S", {Member("a", ty.f32(), {MemberSize(16)})});
   auto* a = ty.array(ty.Of(s), 3);
   Global(Source{{56, 78}}, "g", a, ast::StorageClass::kUniform,
          ast::DecorationList{