[Mesa-dev] [PATCH] glsl: fix shader storage block member rules when adding program resources

[Timothy Arceri]

On Wed, 2015-10-21 at 12:18 +0200, Samuel Iglesias Gonsalvez wrote:
> Commit f24e5e did not take into account arrays of named shader
> storage blocks.
> 
> Fixes 20 dEQP-GLES31.functional.ssbo.* tests:
> 
> dEQP
> -GLES31.functional.ssbo.layout.single_struct_array.per_block_buffer.s
> hared_instance_array
> dEQP
> -GLES31.functional.ssbo.layout.single_struct_array.per_block_buffer.p
> acked_instance_array
> dEQP
> -GLES31.functional.ssbo.layout.single_struct_array.per_block_buffer.s
> td140_instance_array
> dEQP
> -GLES31.functional.ssbo.layout.single_struct_array.per_block_buffer.s
> td430_instance_array
> dEQP
> -GLES31.functional.ssbo.layout.single_struct_array.single_buffer.shar
> ed_instance_array
> dEQP
> -GLES31.functional.ssbo.layout.single_struct_array.single_buffer.pack
> ed_instance_array
> dEQP
> -GLES31.functional.ssbo.layout.single_struct_array.single_buffer.std1
> 40_instance_array
> dEQP
> -GLES31.functional.ssbo.layout.single_struct_array.single_buffer.std4
> 30_instance_array
> dEQP
> -GLES31.functional.ssbo.layout.single_nested_struct_array.per_block_b
> uffer.shared_instance_array
> dEQP
> -GLES31.functional.ssbo.layout.single_nested_struct_array.per_block_b
> uffer.packed_instance_array
> dEQP
> -GLES31.functional.ssbo.layout.single_nested_struct_array.per_block_b
> uffer.std140_instance_array
> dEQP
> -GLES31.functional.ssbo.layout.single_nested_struct_array.per_block_b
> uffer.std430_instance_array
> dEQP
> -GLES31.functional.ssbo.layout.single_nested_struct_array.single_buff
> er.shared_instance_array
> dEQP
> -GLES31.functional.ssbo.layout.single_nested_struct_array.single_buff
> er.packed_instance_array
> dEQP
> -GLES31.functional.ssbo.layout.single_nested_struct_array.single_buff
> er.std140_instance_array
> dEQP
> -GLES31.functional.ssbo.layout.single_nested_struct_array.single_buff
> er.std430_instance_array
> dEQP-GLES31.functional.ssbo.layout.random.all_per_block_buffers.2
> dEQP-GLES31.functional.ssbo.layout.random.all_per_block_buffers.29
> dEQP-GLES31.functional.ssbo.layout.random.all_per_block_buffers.33
> dEQP-GLES31.functional.ssbo.layout.random.all_shared_buffer.3
> 
> Signed-off-by: Samuel Iglesias Gonsalvez <siglesias at igalia.com>
> ---
>  src/glsl/linker.cpp | 34 +++++++++++++++++++++++++++++-----
>  1 file changed, 29 insertions(+), 5 deletions(-)
> 
> diff --git a/src/glsl/linker.cpp b/src/glsl/linker.cpp
> index 07ea0e0..6593e58 100644
> --- a/src/glsl/linker.cpp
> +++ b/src/glsl/linker.cpp
> @@ -3138,6 +3138,9 @@ should_add_buffer_variable(struct
> gl_shader_program *shProg,
>  {
>     bool found_interface = false;
>     const char *block_name = NULL;
> +   unsigned block_name_len = 0;
> +   const char *first_dot = strchr(name, '.');

Maybe call this block_name_dot rather than reusing first_dot

> +   const char *first_square_bracket = strchr(name, '[');
>  
>     /* These rules only apply to buffer variables. So we return
>      * true for the rest of types.
> @@ -3147,7 +3150,27 @@ should_add_buffer_variable(struct
> gl_shader_program *shProg,
>  
>     for (unsigned i = 0; i < shProg->NumBufferInterfaceBlocks; i++) {
>        block_name = shProg->BufferInterfaceBlocks[i].Name;
> -      if (strncmp(block_name, name, strlen(block_name)) == 0) {
> +      block_name_len = strlen(block_name);
> +
> +      const char *first_block_square_bracket = strchr(block_name,
> '[');
> +      if (first_block_square_bracket) {
> +         /* The block is part of an array of named interfaces,
> +          * for the name comparison we ignore the "[x]" part.
> +          */
> +         block_name_len -= strlen(first_block_square_bracket);
> +      }
> +
> +      if (first_dot) {
> +         /* Check if the variable name starts with the interface
> +          * name. The interface name (if present) should have the
> +          * length than the interface block name we are comparing
> to.
> +          */
> +         unsigned len = strlen(name) - strlen(first_dot);
> +         if (len != block_name_len)
> +            continue;
> +      }
> +
> +      if (strncmp(block_name, name, block_name_len) == 0) {
>           found_interface = true;
>           break;
>        }
> @@ -3156,8 +3179,11 @@ should_add_buffer_variable(struct
> gl_shader_program *shProg,
>     /* We remove the interface name from the buffer variable name,
>      * including the dot that follows it.
>      */
> -   if (found_interface)
> -      name = name + strlen(block_name) + 1;
> +   if (found_interface) {
> +      name = name + block_name_len + 1;
> +      first_dot = strchr(name, '.');
> +      first_square_bracket = strchr(name, '[');
> +   }
>  
>     /* From: ARB_program_interface_query extension:
>      *
> @@ -3166,8 +3192,6 @@ should_add_buffer_variable(struct
> gl_shader_program *shProg,
>      *   of its type.  For arrays of aggregate types, the enumeration
> rules are
>      *   applied recursively for the single enumerated array element.
>      */
> -   const char *first_dot = strchr(name, '.');

Maybe leave this here and name it struct_dot

> -   const char *first_square_bracket = strchr(name, '[');

I don't think you need to move the above variable right?

With those couple of small nits fixed

Reviewed-by: Timothy Arceri <timothy.arceri at collabora.com>

>  
>     /* The buffer variable is on top level and it is not an array */
>     if (!first_square_bracket) {