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

[Samuel Iglesias Gonsálvez]

On 22/10/15 08:29, Timothy Arceri wrote:
> 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>
> 

OK, I will do them.

Thanks for your review!

Sam

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