[Mesa-dev] [PATCH] shaderapi: don't generate not linked error on GetProgramStage in general

[Tapani Pälli]

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


On 08/23/2016 06:40 PM, Alejandro Piñeiro wrote:
> Both ARB_shader_subroutine and the GL core spec doesn't list any
> error when the program is not linked.
>
> We left a error generation for the uniform location, in order to be
> consistent with other methods from the spec that generate them.
> ---
>
> On 23/08/16 10:14, Tapani Pälli wrote:
>
>> IMO it looks like GetProgramStageiv is currently not correct from this
>> perspective as it does not follow these rules, it should not require
>> linking either as GL_ARB_shader_subroutine spec does not specify such
>> for the call.
> This patch tries to put some coherence between both specs. Having said
> so I found this to be tricky, as GetProgramStageiv also allow tos ask
> for uniform locations. The rest of the methods at the subroutine spec
> generates an error if asking for locations when the program is not linked.
>
> So I went for the compromise of keep this method not generate errors
> when the program is not linked in general, as the spec for the method
> points, but still generates the error for uniform location, trying to be
> coherent with other methods.
>
> Or in other words: I tried to be as coherent as possible, but failing to
> be be 100% coherent and follow the spec 100% at the same time.

IMO this patch follows the spec 100% It is true that spec is a bit weird 
regarding queries of unlinked (or failed to link) shaders, would be 
interesting to see actual use-case for this.

>
>   src/mesa/main/shaderapi.c | 19 ++++++++++++++++++-
>   1 file changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/src/mesa/main/shaderapi.c b/src/mesa/main/shaderapi.c
> index 4f29cd9..6806c02 100644
> --- a/src/mesa/main/shaderapi.c
> +++ b/src/mesa/main/shaderapi.c
> @@ -2745,8 +2745,25 @@ _mesa_GetProgramStageiv(GLuint program, GLenum shadertype,
>   
>      stage = _mesa_shader_enum_to_shader_stage(shadertype);
>      sh = shProg->_LinkedShaders[stage];
> +
> +   /* ARB_shader_subroutine doesn't ask the program to be linked, or list any
> +    * INVALID_OPERATION in the case of not be linked.
> +    *
> +    * And for some pnames, like GL_ACTIVE_SUBROUTINE_UNIFORMS, you can ask the
> +    * same info using other specs (ARB_program_interface_query), without the
> +    * need of the program to be linked, being the value for that case 0.
> +    *
> +    * But at the same time, some other methods require the program to be
> +    * linked for pname related to locations, so it would be inconsistent to
> +    * not do the same here. So we are:
> +    *   * Return GL_INVALID_OPERATION if not linked only for locations.
> +    *   * Setting a default value of 0, to be returned if not linked.
> +    */
>      if (!sh) {
> -      _mesa_error(ctx, GL_INVALID_OPERATION, "%s", api_name);
> +      values[0] = 0;
> +      if (pname == GL_ACTIVE_SUBROUTINE_UNIFORM_LOCATIONS) {
> +         _mesa_error(ctx, GL_INVALID_OPERATION, "%s", api_name);
> +      }
>         return;
>      }
>