[Mesa-dev] [PATCH v5 29/70] glsl: a shader storage buffer must be smaller than the maximum size allowed

[Samuel Iglesias Gonsálvez]

On 18/09/15 20:59, Kristian Høgsberg wrote:
> On Thu, Sep 10, 2015 at 03:35:45PM +0200, Iago Toral Quiroga wrote:
>> From: Samuel Iglesias Gonsalvez <siglesias at igalia.com>
>>
>> Otherwise, generate a link time error as per the
>> ARB_shader_storage_buffer_object spec.
>>
>> v2:
>> - Fix error message (Jordan)
>>
>> Signed-off-by: Samuel Iglesias Gonsalvez <siglesias at igalia.com>
>> Reviewed-by: Jordan Justen <jordan.l.justen at intel.com>
>> ---
>>  src/glsl/glsl_types.cpp          |  9 +++++++--
>>  src/glsl/link_uniform_blocks.cpp | 19 +++++++++++++++++++
>>  src/glsl/linker.cpp              |  2 +-
>>  src/glsl/linker.h                |  1 +
>>  4 files changed, 28 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/glsl/glsl_types.cpp b/src/glsl/glsl_types.cpp
>> index d97991a..f8227df 100644
>> --- a/src/glsl/glsl_types.cpp
>> +++ b/src/glsl/glsl_types.cpp
>> @@ -1325,7 +1325,7 @@ glsl_type::std140_size(bool row_major) const
>>      *     rounded up to the next multiple of the base alignment of the
>>      *     structure.
>>      */
>> -   if (this->is_record()) {
>> +   if (this->is_record() || this->is_interface()) {
>>        unsigned size = 0;
>>        unsigned max_align = 0;
>>  
>> @@ -1341,7 +1341,12 @@ glsl_type::std140_size(bool row_major) const
>>  
>>  	 const struct glsl_type *field_type = this->fields.structure[i].type;
>>  	 unsigned align = field_type->std140_base_alignment(field_row_major);
>> -	 size = glsl_align(size, align);
>> +
>> +         /* Ignore unsized arrays when calculating size */
>> +         if (field_type->is_unsized_array())
>> +            continue;
>> +
>> +         size = glsl_align(size, align);
>>  	 size += field_type->std140_size(field_row_major);
>>  
>>           max_align = MAX2(align, max_align);
> 
> These two chunks look like they should be their own patch ("Add
> unsized array support to glsl_type::std140_size" or such).
> 

Agreed. I will do that change.

Thanks for reviewing!

Sam

>> diff --git a/src/glsl/link_uniform_blocks.cpp b/src/glsl/link_uniform_blocks.cpp
>> index 8f65f4a..7ceffee 100644
>> --- a/src/glsl/link_uniform_blocks.cpp
>> +++ b/src/glsl/link_uniform_blocks.cpp
>> @@ -187,6 +187,7 @@ struct block {
>>  
>>  unsigned
>>  link_uniform_blocks(void *mem_ctx,
>> +                    struct gl_context *ctx,
>>                      struct gl_shader_program *prog,
>>                      struct gl_shader **shader_list,
>>                      unsigned num_shaders,
>> @@ -308,6 +309,15 @@ link_uniform_blocks(void *mem_ctx,
>>  
>>              blocks[i].UniformBufferSize = parcel.buffer_size;
>>  
>> +            /* Check SSBO size is lower than maximum supported size for SSBO */
>> +            if (b->is_shader_storage &&
>> +                parcel.buffer_size > ctx->Const.MaxShaderStorageBlockSize) {
>> +               linker_error(prog, "shader storage block `%s' has size %d, "
>> +                            "which is larger than than the maximum allowed (%d)",
>> +                            block_type->name,
>> +                            parcel.buffer_size,
>> +                            ctx->Const.MaxShaderStorageBlockSize);
>> +            }
>>              blocks[i].NumUniforms =
>>                 (unsigned)(ptrdiff_t)(&variables[parcel.index] - blocks[i].Uniforms);
>>  
>> @@ -328,6 +338,15 @@ link_uniform_blocks(void *mem_ctx,
>>  
>>           blocks[i].UniformBufferSize = parcel.buffer_size;
>>  
>> +         /* Check SSBO size is lower than maximum supported size for SSBO */
>> +         if (b->is_shader_storage &&
>> +             parcel.buffer_size > ctx->Const.MaxShaderStorageBlockSize) {
>> +            linker_error(prog, "shader storage block `%s' has size %d, "
>> +                         "which is larger than than the maximum allowed (%d)",
>> +                         block_type->name,
>> +                         parcel.buffer_size,
>> +                         ctx->Const.MaxShaderStorageBlockSize);
>> +         }
>>           blocks[i].NumUniforms =
>>              (unsigned)(ptrdiff_t)(&variables[parcel.index] - blocks[i].Uniforms);
>>  
>> diff --git a/src/glsl/linker.cpp b/src/glsl/linker.cpp
>> index e078f86..cf9f1f6 100644
>> --- a/src/glsl/linker.cpp
>> +++ b/src/glsl/linker.cpp
>> @@ -2023,7 +2023,7 @@ link_intrastage_shaders(void *mem_ctx,
>>  
>>     /* Link up uniform blocks defined within this stage. */
>>     const unsigned num_uniform_blocks =
>> -      link_uniform_blocks(mem_ctx, prog, shader_list, num_shaders,
>> +      link_uniform_blocks(mem_ctx, ctx, prog, shader_list, num_shaders,
>>                            &uniform_blocks);
>>     if (!prog->LinkStatus)
>>        return NULL;
>> diff --git a/src/glsl/linker.h b/src/glsl/linker.h
>> index fb8f81e..953b49f 100644
>> --- a/src/glsl/linker.h
>> +++ b/src/glsl/linker.h
>> @@ -56,6 +56,7 @@ link_uniform_blocks_are_compatible(const gl_uniform_block *a,
>>  
>>  extern unsigned
>>  link_uniform_blocks(void *mem_ctx,
>> +                    struct gl_context *ctx,
>>                      struct gl_shader_program *prog,
>>                      struct gl_shader **shader_list,
>>                      unsigned num_shaders,
>> -- 
>> 1.9.1
>>
>