Make it an error for a stage input to take more than one slot. (#161)

Earlier, no PerVertexData struct would be generated. The shader is
useless without reflection information.

Fixes flutter/flutter#102521.

Author: chinmaygarde

impeller/compiler/compiler_unittests.cc

@@ -24,6 +24,14 @@ TEST_P(CompilerTest, CanCompile) {
   ASSERT_TRUE(CanCompileAndReflect("sample.vert"));
 }
 
+TEST_P(CompilerTest, MustFailDueToMultipleLocationPerStructMember) {
+  if (GetParam() == TargetPlatform::kFlutterSPIRV) {
+    // This is a failure of reflection which this target doesn't perform.
+    GTEST_SKIP();
+  }
+  ASSERT_FALSE(CanCompileAndReflect("struct_def_bug.vert"));
+}
+
 #define INSTANTIATE_TARGET_PLATFORM_TEST_SUITE_P(suite_name)              \
   INSTANTIATE_TEST_SUITE_P(                                               \
       suite_name, CompilerTest,                                           \

impeller/compiler/reflector.cc

@@ -232,11 +232,16 @@ std::optional<nlohmann::json> Reflector::GenerateTemplateArguments() const {
     auto& struct_definitions = root["struct_definitions"] =
         nlohmann::json::array_t{};
     if (entrypoints.front().execution_model ==
-        spv::ExecutionModel::ExecutionModelVertex) {
+            spv::ExecutionModel::ExecutionModelVertex &&
+        !shader_resources.stage_inputs.empty()) {
       if (auto struc =
               ReflectPerVertexStructDefinition(shader_resources.stage_inputs);
           struc.has_value()) {
         struct_definitions.emplace_back(EmitStructDefinition(struc.value()));
+      } else {
+        // If there are stage inputs, it is an error to not generate a per
+        // vertex data struct for a vertex like shader stage.
+        return std::nullopt;
       }
     }
 
@@ -662,7 +667,10 @@ Reflector::ReflectPerVertexStructDefinition(
 
   for (size_t i = 0; i < locations.size(); i++) {
     if (locations.count(i) != 1) {
-      // Locations are not contiguous. Bail.
+      // Locations are not contiguous. This usually happens when a single stage
+      // input takes multiple input slots. No reflection information can be
+      // generated for such cases anyway. So bail! It is up to the shader author
+      // to make sure one stage input maps to a single input slot.
       return std::nullopt;
     }
   }
@@ -677,6 +685,7 @@ Reflector::ReflectPerVertexStructDefinition(
       }
     }
     // This really cannot happen with all the validation above.
+    FML_UNREACHABLE();
     return nullptr;
   };
 

impeller/fixtures/BUILD.gn

@@ -27,6 +27,7 @@ test_fixtures("file_fixtures") {
     "embarcadero.jpg",
     "kalimba.jpg",
     "sample.vert",
+    "struct_def_bug.vert",
     "types.h",
     "test_texture.frag",
     "//flutter/third_party/txt/third_party/fonts/Roboto-Regular.ttf",

impeller/fixtures/struct_def_bug.vert

@@ -0,0 +1,34 @@
+// Copyright 2013 The Flutter Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+uniform FrameInfo {
+  mat4 mvp;
+  vec2 atlas_size;
+  vec4 text_color;
+} frame_info;
+
+in vec2 unit_vertex;
+in mat4 glyph_position; // <--- Causes multiple slots to be used and is a failure.
+in vec2 glyph_size;
+in vec2 atlas_position;
+in vec2 atlas_glyph_size;
+
+out vec2 v_unit_vertex;
+out vec2 v_atlas_position;
+out vec2 v_atlas_glyph_size;
+out vec2 v_atlas_size;
+out vec4 v_text_color;
+
+void main() {
+  gl_Position = frame_info.mvp
+              * glyph_position
+              * vec4(unit_vertex.x * glyph_size.x,
+                     unit_vertex.y * glyph_size.y, 0.0, 1.0);
+
+  v_unit_vertex = unit_vertex;
+  v_atlas_position = atlas_position;
+  v_atlas_glyph_size = atlas_glyph_size;
+  v_atlas_size = frame_info.atlas_size;
+  v_text_color = frame_info.text_color;
+}