[Mesa-dev] [PATCH] shaderapi: Fix AttachShader error

[Chad Versace]

On 02/11/2013 12:03 AM, Tapani Pälli wrote:
> From: bma <Bo.Ma at windriver.com>
> 
> Detect a duplicate Shader type as and error instead of silently allowing
> it, restrict to ES2 API.
> 
> v2: Tapani Pälli <tapani.palli at intel.com>
>     - make the check run time instead of compile time
> 
> Signed-off-by: bma <Bo.Ma at windriver.com>
> Signed-off-by: Tapani Pälli <tapani.palli at intel.com>
> ---
>  src/mesa/main/shaderapi.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/src/mesa/main/shaderapi.c b/src/mesa/main/shaderapi.c
> index 2590abe..39f557a 100644
> --- a/src/mesa/main/shaderapi.c
> +++ b/src/mesa/main/shaderapi.c
> @@ -207,6 +207,9 @@ attach_shader(struct gl_context *ctx, GLuint program, GLuint shader)
>     struct gl_shader *sh;
>     GLuint i, n;
>  
> +   const bool same_type_disallowed =
> +	_mesa_is_gles(ctx) || _mesa_is_gles3(ctx);
> +

As Matt sais, the test above is redundant. It should be changed to either
  same_type_disallowed = _mesa_is_gles(ctx);
or
  same_type_disallowed = (ctx->API == API_OPENGL_ES2);
I prefer the first way for sake of encapsultion.


>     shProg = _mesa_lookup_shader_program_err(ctx, program, "glAttachShader");
>     if (!shProg)
>        return;
> @@ -218,12 +221,20 @@ attach_shader(struct gl_context *ctx, GLuint program, GLuint shader)
>  
>     n = shProg->NumShaders;
>     for (i = 0; i < n; i++) {
> -      if (shProg->Shaders[i] == sh) {
> +      if (shProg->Shaders[i] == sh ||
> +         same_type_disallowed && shProg->Shaders[i]->Type == sh->Type) {
> +
>           /* The shader is already attched to this program.  The
>            * GL_ARB_shader_objects spec says:
>            *
>            *     "The error INVALID_OPERATION is generated by AttachObjectARB
>            *     if <obj> is already attached to <containerObj>."
> +          *
> +          * Shader with the same type is already attached to this program,
> +          * OpenGL ES 2.0 and 3.0 spec says:
> +          *
> +          *     "Multiple shader objects of the same type may not be attached
> +          *     to a single program object."
>            */
>           _mesa_error(ctx, GL_INVALID_OPERATION, "glAttachShader");
>           return;

The comment block confuses me. It says "Bad thing X happened. Bad thing
Y happened". But, in fact, likely only one of X or Y happened. To clarify
things, I'd leave the original if-block unchanged and append the new check
in an else-if-block, like so:

  } else if (same_type_disallowed && shProg->Shaders[i]->Type == sh->type) {
    /* Shader with same type blah blah. */
    _mesa_error(ctx, GL_INVALID_OPERATION, "glAttachShader");
    return;
  }

Otherwise, this patch looks good.