[Mesa-dev] [PATCH 3/3] mesa: rework how we free gl_shader_program_data
Kenneth Graunke
kenneth at whitecape.org
Wed Nov 8 05:53:13 UTC 2017
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?
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++) {
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20171107/e11dd7b1/attachment-0001.sig>
More information about the mesa-dev
mailing list