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

Timothy Arceri tarceri at itsqueeze.com
Wed Nov 8 06:58:27 UTC 2017


On 08/11/17 16:53, Kenneth Graunke wrote:
> On Tuesday, November 7, 2017 5:41:59 PM PST 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)
>> +{
> 
> I'm not sure why you need to ralloc_free things explicitly here.
> 
> Most of these fields appear to be ralloc'd using "data" as the context.
> So, I'm not sure why you need to ralloc_free them all explicitly here.
> When you ralloc_free data (in the caller), it will free them too.
> 
> Some of them weren't, but isn't that what you fixed in patch 2?

Yeah I did this first then noticed it was crashing so I fixed the 
rallocs in patch 2. I noticed after sending I probably could drop the 
rest of these rallocs and just leave the 
_mesa_uniform_detach_all_driver_storage() calls. I'll fix this up locally.


> 
> Otherwise, this looks good to me...hopefully Neil can take a look too...
> 
>> +   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;
>>   }
>>   
>> -
> 
> Bonus whitespace change
> 
>>   /**
>>    * 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);
>>   }
>>   
>>   
>> @@ -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