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

[Nicolai Hähnle]

On 18.10.2016 14:15, Iago Toral Quiroga wrote:
> 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.

Makes sense to me.

Reviewed-by: Nicolai Hähnle <nicolai.haehnle at amd.com>

> ---
>  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();
>        }
>
>