[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 05:58:01 UTC 2017
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?
> }
>
>
> @@ -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-dev
mailing list