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

Timothy Arceri Tue, 07 Nov 2017 22:58:59 -0800

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 <nrobe...@igalia.com>
Cc: "17.2 17.3" <mesa-sta...@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 therallocs in patch 2. I noticed after sending I probably could drop therest 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++) {

_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

Reply via email to