[Mesa-stable] [Mesa-dev] [PATCH 3/3] mesa: rework how we free gl_shader_program_data

Tapani Pälli tapani.palli at intel.com
Wed Nov 8 07:14:35 UTC 2017



On 11/08/2017 08:59 AM, Timothy Arceri wrote:
> 
> 
> On 08/11/17 16:58, Tapani Pälli wrote:
>> One question below ...
>>
>> On 11/08/2017 03:41 AM, Timothy Arceri wrote:
>>> When I introduced gl_shader_program_data one of the intentions was to
>>> fix a bug where a failed linking attempt freed data required by a
>>> currently active program. However I seem to have failed to finish
>>> hooking up the final steps required to have the data hang around.
>>>
>>> Here we create a fresh instance of gl_shader_program_data every
>>> time we link. gl_program has a reference to gl_shader_program_data
>>> so it will be freed once the program is no longer active.
>>>
>>> Cc: Neil Roberts <nroberts at igalia.com>
>>> Cc: "17.2 17.3" <mesa-stable at lists.freedesktop.org>
>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102177
>>> ---
>>>   src/mesa/main/shaderobj.c       | 71 
>>> +++++++++++++++++------------------------
>>>   src/mesa/main/shaderobj.h       |  3 ++
>>>   src/mesa/program/ir_to_mesa.cpp |  2 ++
>>>   3 files changed, 35 insertions(+), 41 deletions(-)
>>>
>>> diff --git a/src/mesa/main/shaderobj.c b/src/mesa/main/shaderobj.c
>>> index e2103bcde49..5501a0157db 100644
>>> --- a/src/mesa/main/shaderobj.c
>>> +++ b/src/mesa/main/shaderobj.c
>>> @@ -192,7 +192,30 @@ _mesa_lookup_shader_err(struct gl_context *ctx, 
>>> GLuint name, const char *caller)
>>> /**********************************************************************/
>>>   /*** Shader Program object functions ***/
>>> /**********************************************************************/
>>> +static void
>>> +free_shader_program_data(struct gl_shader_program_data *data)
>>> +{
>>> +   if (data->UniformStorage) {
>>> +      for (unsigned i = 0; i < data->NumUniformStorage; ++i)
>>> + _mesa_uniform_detach_all_driver_storage(&data->UniformStorage[i]);
>>> +      ralloc_free(data->UniformStorage);
>>> +   }
>>> +
>>> +   assert(data->InfoLog != NULL);
>>> +   ralloc_free(data->InfoLog);
>>> +
>>> +   ralloc_free(data->UniformBlocks);
>>> +
>>> +   ralloc_free(data->ShaderStorageBlocks);
>>> +   if (data->AtomicBuffers) {
>>> +      ralloc_free(data->AtomicBuffers);
>>> +   }
>>> +
>>> +   if (data->ProgramResourceList) {
>>> +      ralloc_free(data->ProgramResourceList);
>>> +   }
>>> +}
>>>   void
>>>   _mesa_reference_shader_program_data(struct gl_context *ctx,
>>> @@ -209,6 +232,7 @@ _mesa_reference_shader_program_data(struct 
>>> gl_context *ctx,
>>>         if (p_atomic_dec_zero(&oldData->RefCount)) {
>>>            assert(ctx);
>>> +         free_shader_program_data(oldData);
>>>            ralloc_free(oldData);
>>>         }
>>> @@ -259,14 +283,16 @@ _mesa_reference_shader_program_(struct 
>>> gl_context *ctx,
>>>      }
>>>   }
>>> -static struct gl_shader_program_data *
>>> -create_shader_program_data()
>>> +struct gl_shader_program_data *
>>> +_mesa_create_shader_program_data()
>>>   {
>>>      struct gl_shader_program_data *data;
>>>      data = rzalloc(NULL, struct gl_shader_program_data);
>>>      if (data)
>>>         data->RefCount = 1;
>>> +   data->InfoLog = ralloc_strdup(data, "");
>>> +
>>>      return data;
>>>   }
>>> @@ -286,8 +312,6 @@ init_shader_program(struct gl_shader_program *prog)
>>>      prog->TransformFeedback.BufferMode = GL_INTERLEAVED_ATTRIBS;
>>>      exec_list_make_empty(&prog->EmptyUniformLocations);
>>> -
>>> -   prog->data->InfoLog = ralloc_strdup(prog->data, "");
>>>   }
>>>   /**
>>> @@ -300,7 +324,7 @@ _mesa_new_shader_program(GLuint name)
>>>      shProg = rzalloc(NULL, struct gl_shader_program);
>>>      if (shProg) {
>>>         shProg->Name = name;
>>> -      shProg->data = create_shader_program_data();
>>> +      shProg->data = _mesa_create_shader_program_data();
>>>         if (!shProg->data) {
>>>            ralloc_free(shProg);
>>>            return NULL;
>>> @@ -310,7 +334,6 @@ _mesa_new_shader_program(GLuint name)
>>>      return shProg;
>>>   }
>>> -
>>>   /**
>>>    * Clear (free) the shader program state that gets produced by 
>>> linking.
>>>    */
>>> @@ -325,17 +348,6 @@ _mesa_clear_shader_program_data(struct 
>>> gl_context *ctx,
>>>         }
>>>      }
>>> -   shProg->data->linked_stages = 0;
>>> -
>>> -   if (shProg->data->UniformStorage) {
>>> -      for (unsigned i = 0; i < shProg->data->NumUniformStorage; ++i)
>>> -         _mesa_uniform_detach_all_driver_storage(&shProg->data->
>>> -                                                    UniformStorage[i]);
>>> -      ralloc_free(shProg->data->UniformStorage);
>>> -      shProg->data->NumUniformStorage = 0;
>>> -      shProg->data->UniformStorage = NULL;
>>> -   }
>>> -
>>>      if (shProg->UniformRemapTable) {
>>>         ralloc_free(shProg->UniformRemapTable);
>>>         shProg->NumUniformRemapTable = 0;
>>> @@ -347,29 +359,7 @@ _mesa_clear_shader_program_data(struct 
>>> gl_context *ctx,
>>>         shProg->UniformHash = NULL;
>>>      }
>>> -   assert(shProg->data->InfoLog != NULL);
>>> -   ralloc_free(shProg->data->InfoLog);
>>> -   shProg->data->InfoLog = ralloc_strdup(shProg->data, "");
>>> -
>>> -   ralloc_free(shProg->data->UniformBlocks);
>>> -   shProg->data->UniformBlocks = NULL;
>>> -   shProg->data->NumUniformBlocks = 0;
>>> -
>>> -   ralloc_free(shProg->data->ShaderStorageBlocks);
>>> -   shProg->data->ShaderStorageBlocks = NULL;
>>> -   shProg->data->NumShaderStorageBlocks = 0;
>>> -
>>> -   if (shProg->data->AtomicBuffers) {
>>> -      ralloc_free(shProg->data->AtomicBuffers);
>>> -      shProg->data->AtomicBuffers = NULL;
>>> -      shProg->data->NumAtomicBuffers = 0;
>>> -   }
>>> -
>>> -   if (shProg->data->ProgramResourceList) {
>>> -      ralloc_free(shProg->data->ProgramResourceList);
>>> -      shProg->data->ProgramResourceList = NULL;
>>> -      shProg->data->NumProgramResourceList = 0;
>>> -   }
>>> +   _mesa_reference_shader_program_data(ctx, &shProg->data, NULL);
>>
>>
>> Previously we set Num as 0 and pointers to NULL. Did you sanity check 
>> if something does not go and check "if (ProgramResourceList) ..." and 
>> assume list is there if not NULL?
> 
> We create gl_shader_program_data with rzalloc so it should all work the 
> same.


Right, makes sense.

Series is
Reviewed-by: Tapani Pälli <tapani.palli at intel.com>

> 
>>
>>
>>>   }
>>> @@ -432,7 +422,6 @@ _mesa_delete_shader_program(struct gl_context *ctx,
>>>                               struct gl_shader_program *shProg)
>>>   {
>>>      _mesa_free_shader_program_data(ctx, shProg);
>>> -   _mesa_reference_shader_program_data(ctx, &shProg->data, NULL);
>>>      ralloc_free(shProg);
>>>   }
>>> diff --git a/src/mesa/main/shaderobj.h b/src/mesa/main/shaderobj.h
>>> index 97b8ce7ac23..084848e220c 100644
>>> --- a/src/mesa/main/shaderobj.h
>>> +++ b/src/mesa/main/shaderobj.h
>>> @@ -100,6 +100,9 @@ _mesa_lookup_shader_program_err(struct gl_context 
>>> *ctx, GLuint name,
>>>   extern struct gl_shader_program *
>>>   _mesa_new_shader_program(GLuint name);
>>> +extern struct gl_shader_program_data *
>>> +_mesa_create_shader_program_data(void);
>>> +
>>>   extern void
>>>   _mesa_clear_shader_program_data(struct gl_context *ctx,
>>>                                   struct gl_shader_program *shProg);
>>> diff --git a/src/mesa/program/ir_to_mesa.cpp 
>>> b/src/mesa/program/ir_to_mesa.cpp
>>> index aa330638836..327fd61d422 100644
>>> --- a/src/mesa/program/ir_to_mesa.cpp
>>> +++ b/src/mesa/program/ir_to_mesa.cpp
>>> @@ -3067,6 +3067,8 @@ _mesa_glsl_link_shader(struct gl_context *ctx, 
>>> struct gl_shader_program *prog)
>>>      _mesa_clear_shader_program_data(ctx, prog);
>>> +   prog->data = _mesa_create_shader_program_data();
>>> +
>>>      prog->data->LinkStatus = linking_success;
>>>      for (i = 0; i < prog->NumShaders; i++) {
>>>


More information about the mesa-stable mailing list