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

[Matt Turner]

On Tue, Jul 30, 2013 at 12:13 PM, Carl Worth <cworth at cworth.org> 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?
>
> 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.
>         */

I think that this patch /should/ go in, since it was a fix for a new extension.

9.2 will be the first version of Mesa to ship with support for
ARB_shading_language_420pack, so the branch should contain bug fixes
for the extension.