[Mesa-dev] [PACH v2] glsl: SSBO unsized array declarations, if present, must be declared last

[Timothy Arceri]

On Fri, 2016-10-14 at 14:30 +0200, Iago Toral Quiroga wrote:
> From the ARB_shader_storage_buffer_object spec:
> 
> "In a shader storage block, the last member may be declared without
> an explicit
>  size.  In this case, the effective array size is inferred at run-
> time from
>  the size of the data store backing the interface block.  Such
> unsized
>  arrays may be indexed with general integer expressions, but may not
> be
>  passed as an argument to a function or indexed with a negative
> constant
>  expression."
> 
> dEQP tests that SSBOs that declare field members after an unsized
> array
> declaration fail to compile.
> 
> Fixes the remaining subcase of the following dEQP tests:
> dEQP-
> GLES31.functional.debug.negative_coverage.callbacks.shader.compile_co
> mpute_shader
> dEQP-
> GLES31.functional.debug.negative_coverage.get_error.shader.compile_co
> mpute_shader
> dEQP-
> GLES31.functional.debug.negative_coverage.log.shader.compile_compute_
> shader
> 
> v2: only update has_unsized_array while we have not found a previous
> unsized
>     array declaration.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98132
> ---
>  src/compiler/glsl/ast_to_hir.cpp | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/src/compiler/glsl/ast_to_hir.cpp
> b/src/compiler/glsl/ast_to_hir.cpp
> index c3c8cef..462838a 100644
> --- a/src/compiler/glsl/ast_to_hir.cpp
> +++ b/src/compiler/glsl/ast_to_hir.cpp
> @@ -6645,6 +6645,7 @@
> ast_process_struct_or_iface_block_members(exec_list *instructions,
>  
>     bool first_member = true;
>     bool first_member_has_explicit_location = false;
> +   bool has_unsized_array = false;
>  
>     unsigned i = 0;
>     foreach_list_typed (ast_declarator_list, decl_list, link,
> declarations) {
> @@ -6840,6 +6841,13 @@
> ast_process_struct_or_iface_block_members(exec_list *instructions,
>  
>           const struct glsl_type *field_type =
>              process_array_type(&loc, decl_type, decl-
> >array_specifier, state);
> +
> +         if (has_unsized_array && var_mode == ir_var_shader_storage)
> +            _mesa_glsl_error(&loc, state, "SSBO member declared "
> +                             "after unsized array.");
> +         else if (!has_unsized_array)
> +            has_unsized_array = field_type->is_unsized_array();
> +

I suspect this fixes named ifc blocks? There is code to detect these
for unnamed ifc blocks and an is_unsized_array_last_element() function.

I think you want to merge to code from the two of these checks (I think
here is the correct spot) and probably remove
the is_unsized_array_last_element() helper.

>           validate_array_dimensions(field_type, state, &loc);
>           fields[i].type = field_type;
>           fields[i].name = decl->identifier;