[Mesa-dev] [PATCH V3 1/2] glsl: don't attempt to link empty program

[Kenneth Graunke]

On Tuesday, January 26, 2016 11:46:19 AM PST Timothy Arceri wrote:
> Previously an empty program would go through the entire
> link_shaders() function and we would have to be careful
> not to cause a segfault.
> 
> In core profile also now set link_status to false by
> generating an error, it was previously set to true.
> 
> From Section 7.3 (PROGRAM OBJECTS) of the OpenGL 4.5 spec:
> 
>    "Linking can fail for a variety of reasons as specified in the
>    OpenGL Shading Language Specification, as well as any of the
>    following reasons:
> 
>     - No shader objects are attached to program."
> 
> V2: Only generate an error in core profile and add spec quote (Ian)
> 
> V3: generate error in ES too, remove previous check which was only
> applying the rule to GL 4.5/ES 3.1 and above. My understand is that
> this spec change is clarifying previously undefined behaviour and
> therefore should be applied retrospectively. The ES CTS tests for
> this are in ES 2 I suspect it was passing because it would have
> generated an error for not having both a vertex and fragment shader.
> 
> Cc: Ian Romanick <idr at freedesktop.org>
> Cc: Tapani Pälli <tapani.palli at intel.com>
> Cc: Ilia Mirkin <imirkin at alum.mit.edu>
> ---
>  src/glsl/linker.cpp | 43 ++++++++++++++++++++++---------------------
>  1 file changed, 22 insertions(+), 21 deletions(-)
> 
> diff --git a/src/glsl/linker.cpp b/src/glsl/linker.cpp
> index 6657777..79039a0 100644
> --- a/src/glsl/linker.cpp
> +++ b/src/glsl/linker.cpp
> @@ -4105,14 +4105,34 @@ disable_varying_optimizations_for_sso(struct gl_shader_program *prog)
>  void
>  link_shaders(struct gl_context *ctx, struct gl_shader_program *prog)
>  {
> +   prog->Validated = false;
> +   prog->_Used = false;
> +
> +   /* Section 7.3 (Program Objects) of the OpenGL 4.5 Core Profile spec says:
> +    *
> +    *     "Linking can fail for a variety of reasons as specified in the
> +    *     OpenGL Shading Language Specification, as well as any of the
> +    *     following reasons:
> +    *
> +    *     - No shader objects are attached to program."
> +    *
> +    * The Compatibility Profile specification does not list the error.  In
> +    * Compatibility Profile missing shader stages are replaced by
> +    * fixed-function.  This applies to the case where all stages are
> +    * missing.
> +    */
> +   if (prog->NumShaders == 0) {
> +      if (ctx->API != API_OPENGL_COMPAT)
> +         linker_error(prog, "no shaders attached to the program\n");
> +      return;
> +   }
> +
>     tfeedback_decl *tfeedback_decls = NULL;
>     unsigned num_tfeedback_decls = prog->TransformFeedback.NumVarying;
>  
>     void *mem_ctx = ralloc_context(NULL); // temporary linker context
>  
>     prog->LinkStatus = true; /* All error paths will set this to false */
> -   prog->Validated = false;
> -   prog->_Used = false;
>  
>     prog->ARB_fragment_coord_conventions_enable = false;
>  
> @@ -4162,25 +4182,6 @@ link_shaders(struct gl_context *ctx, struct gl_shader_program *prog)
>     prog->Version = max_version;
>     prog->IsES = is_es_prog;
>  
> -   /* From OpenGL 4.5 Core specification (7.3 Program Objects):
> -    *     "Linking can fail for a variety of reasons as specified in the OpenGL
> -    *     Shading Language Specification, as well as any of the following
> -    *     reasons:
> -    *
> -    *     * No shader objects are attached to program.
> -    *
> -    *     ..."
> -    *
> -    *     Same rule applies for OpenGL ES >= 3.1.
> -    */
> -
> -   if (prog->NumShaders == 0 &&
> -       ((ctx->API == API_OPENGL_CORE && ctx->Version >= 45) ||
> -        (ctx->API == API_OPENGLES2 && ctx->Version >= 31))) {
> -      linker_error(prog, "No shader objects are attached to program.\n");
> -      goto done;
> -   }
> -
>     /* Some shaders have to be linked with some other shaders present.
>      */
>     if (num_shaders[MESA_SHADER_GEOMETRY] > 0 &&

This patch seems good to me.  It's nice to short-circuit the NumShaders
== 0 case so we don't have to think about it later.  Taking the new text
as a clarification makes a ton of sense.

Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160209/86d67022/attachment.sig>