[Mesa-dev] [PATCH] mesa/shaderapi: Do a dry run of linking an in-use program

[Timothy Arceri]

I think I'd rather see this handled by releasing the uniforms only after 
the second link is successful and using a temp/fallback pointer to hold 
it until then. We need to do a similar type of thing with shader source 
and the shader cache e.g [1].

[1] 
https://cgit.freedesktop.org/mesa/mesa/commit/?id=e5bb4a0b0f4743fa0a0567991dca751ef49a7200 


On 03/11/17 05:35, Neil Roberts wrote:
> If an in-use program is unsuccessfully linked, the GL spec requires
> that the executable and the uniform state of the program should remain
> until a new program is bound. Previously this sort of worked in Mesa
> except that it would free the uniform state before attempting to link.
> At least on i965 this would usually continue to work because it
> accesses the uniform values via a dangling pointer. However it was
> causing sporadic failures in one of the CTS tests.
> 
> To fix it, when an in-use program is about to be linked, it will now
> make a copy of the program and first try to link that instead. If it
> works it will let the link continue, otherwise it will copy the status
> to the new program and abandon the link. That way the link status and
> info log is preserved without freeing the uniform state.
> 
> This isn’t very efficient but it would probably quite a big overhaul
> to fix it properly and it doesn’t seem worth it because I can’t
> imagine that any performance-sensitive application would be hitting
> this very strange corner case of the API.
> 
> Fixes: KHR-GL46.sepshaderobjs.StateInteraction
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102177
> ---
>   src/mesa/main/shaderapi.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 57 insertions(+), 1 deletion(-)
> 
> diff --git a/src/mesa/main/shaderapi.c b/src/mesa/main/shaderapi.c
> index 7282435..3f7fe02 100644
> --- a/src/mesa/main/shaderapi.c
> +++ b/src/mesa/main/shaderapi.c
> @@ -1134,6 +1134,58 @@ _mesa_compile_shader(struct gl_context *ctx, struct gl_shader *sh)
>      }
>   }
>   
> +static bool
> +try_link_in_use_program(struct gl_context *ctx,
> +                        struct gl_shader_program *shProg)
> +{
> +   struct gl_shader_program *shProgCopy = _mesa_new_shader_program(0);
> +   bool ret;
> +   int i;
> +
> +   /* If the program is in use then try linking a copy of the program first.
> +    * If it won’t work then we’ll copy the link status to the old program and
> +    * abandon the linking. Otherwise we let it relink as normal. This is done
> +    * because relinking destroys the uniform state of the program but the GL
> +    * spec actually requires that the state be preserved when the program is
> +    * in use and the link fails. This isn’t terribly efficent but in practice
> +    * it is probably not worth optimising because it’s a rather strange corner
> +    * case of the spec.
> +    *
> +    * GLSL 4.6 spec section 7.3:
> +    *
> +    * “If a program object that is active for any shader stage is re-linked
> +    *  unsuccessfully, the link status will be set to FALSE, but any existing
> +    *  executables and associated state will remain part of the current
> +    *  rendering state until a subsequent call to UseProgram,
> +    *  UseProgramStages, or BindProgramPipeline removes them from use.”
> +    */
> +
> +   for (i = 0; i < shProg->NumShaders; i++)
> +      attach_shader(ctx, shProgCopy, shProg->Shaders[i]);
> +
> +   shProgCopy->BinaryRetreivableHint = shProg->BinaryRetreivableHint;
> +   shProgCopy->SeparateShader = shProg->SeparateShader;
> +
> +   _mesa_glsl_link_shader(ctx, shProgCopy);
> +
> +   if (shProgCopy->data->LinkStatus) {
> +      ret = true;
> +   } else {
> +      shProg->data->LinkStatus = shProgCopy->data->LinkStatus;
> +      shProg->data->Validated = shProgCopy->data->Validated;
> +      shProg->data->Version = shProgCopy->data->Version;
> +      shProg->IsES = shProgCopy->IsES;
> +      ralloc_free(shProg->data->InfoLog);
> +      shProg->data->InfoLog =
> +         ralloc_strdup(shProg->data, shProgCopy->data->InfoLog);
> +
> +      ret = false;
> +   }
> +
> +   _mesa_reference_shader_program(ctx, &shProgCopy, NULL);
> +
> +   return ret;
> +}
>   
>   /**
>    * Link a program's shaders.
> @@ -1159,12 +1211,16 @@ link_program(struct gl_context *ctx, struct gl_shader_program *shProg,
>      }
>   
>      unsigned programs_in_use = 0;
> -   if (ctx->_Shader)
> +   if (ctx->_Shader) {
>         for (unsigned stage = 0; stage < MESA_SHADER_STAGES; stage++) {
>            if (ctx->_Shader->CurrentProgram[stage] &&
>                ctx->_Shader->CurrentProgram[stage]->Id == shProg->Name) {
>               programs_in_use |= 1 << stage;
>            }
> +      }
> +
> +      if (programs_in_use && !try_link_in_use_program(ctx, shProg))
> +         return;
>      }
>   
>      FLUSH_VERTICES(ctx, 0);
>