[Mesa-dev] [PATCH] glsl: Indirect array indexing on non-last SSBO member must fail compilation

[Iago Toral Quiroga]

After the changes in comit 5b2675093e863a52, we moved this check to the
linker, but the spec expects this to be checked at compile-time. There are
dEQP tests that expect an error at compile time and the spec seems to confirm
that expectation:

"Except for the last declared member of a shader storage block (section 4.3.9
 “Interface Blocks”), the size of an array must be declared (explicitly sized)
 before it is indexed with anything other than an integral constant expression.
 The size of any array must be declared before passing it as an argument to a
 function. Violation of any of these rules result in compile-time errors. It
 is legal to declare an array without a size (unsized) and then later
 redeclare the same name as an array of the same type and specify a size, or
 index it only with integral constant expressions (implicitly sized)."

Commit 5b2675093e863a52 tries to take care of the case where we have implicitly
sized arrays in SSBOs and it does so by checking the max_array_access field
in ir_variable during linking. In this patch we change the approach: we look
for indirect access on SSBO arrays, and when we find one, we emit a
compile-time error if the accessed member is not the last in the SSBO
definition.

There is a corner case that the specs do not address directly though and that
dEQP checks for: the case of an unsized array in an SSBO definition that is
not defined last but is never used in the shader code either. The following
dEQP tests expect a compile-time error in this scenario:

dEQP-GLES31.functional.debug.negative_coverage.callbacks.shader.compile_compute_shader
dEQP-GLES31.functional.debug.negative_coverage.get_error.shader.compile_compute_shader
dEQP-GLES31.functional.debug.negative_coverage.log.shader.compile_compute_shader

However, since the unsized array is never used it is never indexed with a
non-constant expression, so by the spec quotation above, it should be valid and
the tests are probably incorrect.
---
 src/compiler/glsl/ast_array_index.cpp     | 14 ++++++++++++++
 src/compiler/glsl/link_uniform_blocks.cpp |  8 +-------
 2 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/src/compiler/glsl/ast_array_index.cpp b/src/compiler/glsl/ast_array_index.cpp
index e29dafb..dfa44b7 100644
--- a/src/compiler/glsl/ast_array_index.cpp
+++ b/src/compiler/glsl/ast_array_index.cpp
@@ -233,6 +233,20 @@ _mesa_ast_array_index_to_hir(void *mem_ctx,
          else if (array->variable_referenced()->data.mode !=
                   ir_var_shader_storage) {
             _mesa_glsl_error(&loc, state, "unsized array index must be constant");
+         } else {
+            /* Unsized array non-constant indexing on SSBO is allowed only for
+             * the last member of the SSBO definition.
+             */
+            ir_variable *var = array->variable_referenced();
+            const glsl_type *iface_type = var->get_interface_type();
+            int field_index = iface_type->field_index(var->name);
+            /* Field index can be < 0 for instance arrays */
+            if (field_index >= 0 &&
+                field_index != (int) iface_type->length - 1) {
+               _mesa_glsl_error(&loc, state, "Indirect access on unsized "
+                                "array is limited to the last member of "
+                                "SSBO.");
+            }
          }
       } else if (array->type->without_array()->is_interface()
                  && ((array->variable_referenced()->data.mode == ir_var_uniform
diff --git a/src/compiler/glsl/link_uniform_blocks.cpp b/src/compiler/glsl/link_uniform_blocks.cpp
index 5b0dff6..bb423c5 100644
--- a/src/compiler/glsl/link_uniform_blocks.cpp
+++ b/src/compiler/glsl/link_uniform_blocks.cpp
@@ -150,13 +150,7 @@ private:
        */
       const glsl_type *type_for_size = type;
       if (type->is_unsized_array()) {
-         if (!last_field) {
-            linker_error(prog, "unsized array `%s' definition: "
-                         "only last member of a shader storage block "
-                         "can be defined as unsized array",
-                         name);
-         }
-
+         assert(last_field);
          type_for_size = type->without_array();
       }
 
-- 
2.7.4