[Mesa-dev] [PATCH] mesa/glsl: delete previously linked shaders earlier when linking

[Tapani Pälli]

Nice work!

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

On 11/02/2016 03:18 AM, Timothy Arceri wrote:
> This moves the delete linked shaders call to
> _mesa_clear_shader_program_data() which makes sure we delete them
> before returning due to any validation problems.
>
> It also reduces some code duplication.
>
> From the OpenGL 4.5 Core spec:
>
>    "If LinkProgram failed, any information about a previous link of
>    that program object is lost. Thus, a failed link does not restore
>    the old state of program.
>
>    ...
>
>    If one of these commands is called with a program for which
>    LinkProgram failed, no error is generated unless otherwise noted.
>    Implementations may return information on variables and interface
>    blocks that would have been active had the program been linked
>    successfully. In cases where the link failed because the program
>    required too many resources, these commands may help applications
>    determine why limits were exceeded."
>
> Therefore it's expected that we shouldn't be able to query the
> program that failed to link and retrieve inforemation about a
> previously successful link.
>
> Before this change the linker was doing validation before freeing
> the previously linked shaders and therefore could exit on failure
> before they were freed.
>
> This change also fixes an issue in compat profile where a program
> with no shaders attached is expect to fall back to fixed function
> but was instead trying to relink IR from a previous link.
>
> Cc: Matt Turner <mattst88 at gmail.com>
> Cc: Tapani Pälli <tapani.palli at intel.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97715
> Cc: "12.0 13.0" <mesa-stable at lists.freedesktop.org>
> ---
>  src/compiler/glsl/linker.cpp    |  8 --------
>  src/mesa/main/shaderobj.c       | 23 ++++++++++-------------
>  src/mesa/main/shaderobj.h       |  3 ++-
>  src/mesa/program/ir_to_mesa.cpp |  2 +-
>  4 files changed, 13 insertions(+), 23 deletions(-)
>
> diff --git a/src/compiler/glsl/linker.cpp b/src/compiler/glsl/linker.cpp
> index 215fcc4..686f233 100644
> --- a/src/compiler/glsl/linker.cpp
> +++ b/src/compiler/glsl/linker.cpp
> @@ -4795,14 +4795,6 @@ link_shaders(struct gl_context *ctx, struct gl_shader_program *prog)
>                     "type of shader\n");
>     }
>
> -   for (unsigned int i = 0; i < MESA_SHADER_STAGES; i++) {
> -      if (prog->_LinkedShaders[i] != NULL) {
> -         _mesa_delete_linked_shader(ctx, prog->_LinkedShaders[i]);
> -      }
> -
> -      prog->_LinkedShaders[i] = NULL;
> -   }
> -
>     /* Link all shaders for a particular stage and validate the result.
>      */
>     for (int stage = 0; stage < MESA_SHADER_STAGES; stage++) {
> diff --git a/src/mesa/main/shaderobj.c b/src/mesa/main/shaderobj.c
> index 136ac7b..8fd574e 100644
> --- a/src/mesa/main/shaderobj.c
> +++ b/src/mesa/main/shaderobj.c
> @@ -291,12 +291,18 @@ _mesa_new_shader_program(GLuint name)
>   * Clear (free) the shader program state that gets produced by linking.
>   */
>  void
> -_mesa_clear_shader_program_data(struct gl_shader_program *shProg)
> +_mesa_clear_shader_program_data(struct gl_context *ctx,
> +                                struct gl_shader_program *shProg)
>  {
> -   unsigned i;
> +   for (gl_shader_stage sh = 0; sh < MESA_SHADER_STAGES; sh++) {
> +      if (shProg->_LinkedShaders[sh] != NULL) {
> +         _mesa_delete_linked_shader(ctx, shProg->_LinkedShaders[sh]);
> +         shProg->_LinkedShaders[sh] = NULL;
> +      }
> +   }
>
>     if (shProg->UniformStorage) {
> -      for (i = 0; i < shProg->NumUniformStorage; ++i)
> +      for (unsigned i = 0; i < shProg->NumUniformStorage; ++i)
>           _mesa_uniform_detach_all_driver_storage(&shProg->UniformStorage[i]);
>        ralloc_free(shProg->UniformStorage);
>        shProg->NumUniformStorage = 0;
> @@ -347,11 +353,10 @@ _mesa_free_shader_program_data(struct gl_context *ctx,
>                                 struct gl_shader_program *shProg)
>  {
>     GLuint i;
> -   gl_shader_stage sh;
>
>     assert(shProg->Type == GL_SHADER_PROGRAM_MESA);
>
> -   _mesa_clear_shader_program_data(shProg);
> +   _mesa_clear_shader_program_data(ctx, shProg);
>
>     if (shProg->AttributeBindings) {
>        string_to_uint_map_dtor(shProg->AttributeBindings);
> @@ -385,14 +390,6 @@ _mesa_free_shader_program_data(struct gl_context *ctx,
>     shProg->TransformFeedback.VaryingNames = NULL;
>     shProg->TransformFeedback.NumVarying = 0;
>
> -
> -   for (sh = 0; sh < MESA_SHADER_STAGES; sh++) {
> -      if (shProg->_LinkedShaders[sh] != NULL) {
> -	 _mesa_delete_linked_shader(ctx, shProg->_LinkedShaders[sh]);
> -	 shProg->_LinkedShaders[sh] = NULL;
> -      }
> -   }
> -
>     free(shProg->Label);
>     shProg->Label = NULL;
>  }
> diff --git a/src/mesa/main/shaderobj.h b/src/mesa/main/shaderobj.h
> index 814a7f1..1249732 100644
> --- a/src/mesa/main/shaderobj.h
> +++ b/src/mesa/main/shaderobj.h
> @@ -99,7 +99,8 @@ extern struct gl_shader_program *
>  _mesa_new_shader_program(GLuint name);
>
>  extern void
> -_mesa_clear_shader_program_data(struct gl_shader_program *shProg);
> +_mesa_clear_shader_program_data(struct gl_context *ctx,
> +                                struct gl_shader_program *shProg);
>
>  extern void
>  _mesa_free_shader_program_data(struct gl_context *ctx,
> diff --git a/src/mesa/program/ir_to_mesa.cpp b/src/mesa/program/ir_to_mesa.cpp
> index 5776d15..f4c2ad6 100644
> --- a/src/mesa/program/ir_to_mesa.cpp
> +++ b/src/mesa/program/ir_to_mesa.cpp
> @@ -3049,7 +3049,7 @@ _mesa_glsl_link_shader(struct gl_context *ctx, struct gl_shader_program *prog)
>  {
>     unsigned int i;
>
> -   _mesa_clear_shader_program_data(prog);
> +   _mesa_clear_shader_program_data(ctx, prog);
>
>     prog->LinkStatus = GL_TRUE;
>
>