[Mesa-dev] [Mesa-stable] Should we add new error cases to stable releases?

[Ian Romanick]

On 07/30/2013 12:13 PM, Carl Worth wrote:
> Hi Ken,
>
> In the last stable-release cycle you argued for not cherry picking
> commit fcaa48d9cc8937e0ceb59dfd22ef5b6e6fd1a273 on the grounds that
> we shouldn't change mesa to be strictly more strict in a stable release.
>
> That is, adding a new error case to the compiler cannot actually cause a
> valid program to start working, (which would be a nice bug fix), but
> could potentially cause an invalid program to stop working (which could
> be regarded as a regression).
>
> The same reasoning seems to disqualify the below patch. Do you agree?

Maybe.  If both NVIDIA and AMD correctly generate the error, the 
probability of any application using it is epsilon.  We should collect 
that information and add it to the commit message.

I also think this patch may have just been a candidate for 9.2. :/

> Anyone else have input on this issue? If we agree on the rationale, I'd
> like to codify it with some language in our documentation for
> stable-branch criteria. Something like:
>
> 	Note: Not all bug fixes are automatically suitable for the
> 	stable branch.
>
> 	For example, a patch that makes the implementation more strict,
> 	(such as detecting some invalid GLSL code that was previously
> 	silently accepted), can be rejected by the stable-branch
> 	maintainer. The rationale is that a patch like this can cause a
> 	program which was previously working as desired to now stop
> 	working due to the new error case. From the point of view of the
> 	user of such an application, that's a regression that we do not
> 	want to allow during a stable release.
>
> -Carl
>
> commit 17856726c94000bf16156f7f9acea77a271a6005
> Author: Kenneth Graunke <kenneth at whitecape.org>
> Date:   Fri Jul 26 21:18:56 2013 -0700
>
>      glsl: Disallow auxiliary storage qualifiers on FS outputs.
>
>      This has always been an error; we just forgot to check for it.
>
>      Fixes Piglit's no-aux-qual-on-fs-output.frag.
>
>      Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=67333
>      Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
>      Reviewed-by: Matt Turner <mattst88 at gmail.com>
>      Cc: mesa-stable at lists.freedesktop.org
>
> diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp
> index 2569cde..598da92 100644
> --- a/src/glsl/ast_to_hir.cpp
> +++ b/src/glsl/ast_to_hir.cpp
> @@ -2927,6 +2927,20 @@ ast_declarator_list::hir(exec_list *instructions,
>                             "'centroid in' cannot be used in a vertex shader");
>         }
>
> +      /* Section 4.3.6 of the GLSL 1.30 specification states:
> +       * "It is an error to use centroid out in a fragment shader."
> +       *
> +       * The GL_ARB_shading_language_420pack extension specification states:
> +       * "It is an error to use auxiliary storage qualifiers or interpolation
> +       *  qualifiers on an output in a fragment shader."
> +       */
> +      if (state->target == fragment_shader &&
> +          this->type->qualifier.flags.q.out &&
> +          this->type->qualifier.has_auxiliary_storage()) {
> +         _mesa_glsl_error(&loc, state,
> +                          "auxiliary storage qualifiers cannot be used on "
> +                          "fragment shader outputs");
> +      }
>
>         /* Precision qualifiers exists only in GLSL versions 1.00 and >= 1.30.
>          */
>
>
>
> _______________________________________________
> mesa-stable mailing list
> mesa-stable at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-stable
>