[Mesa-dev] [PATCH 1/3 v2] glsl: prevent qualifiers modification of predeclared variables

[Timothy Arceri]

Nice! Series is:

Reviewed-by: Timothy Arceri <tarceri at itsqueeze.com>

On 10/10/18 9:07 am, Ian Romanick wrote:
> From: Ian Romanick <ian.d.romanick at intel.com>
> 
> Section 3.7 (Identifiers) of the GLSL spec says:
> 
>      However, as noted in the specification, there are some cases where
>      previously declared variables can be redeclared to change or add
>      some property, and predeclared "gl_" names are allowed to be
>      redeclared in a shader only for these specific purposes.  More
>      generally, it is an error to redeclare a variable, including those
>      starting "gl_".
> 
> This patch should fix piglit tests:
> clip-distance-redeclare-without-inout.frag
> clip-distance-redeclare-without-inout.vert
> 
> However, this causes a regression in
> clip-distance-out-values.shader_test.  A fix for that test has been sent
> to the piglit list for review:
> 
>      https://patchwork.freedesktop.org/patch/255201/
> 
> As far as I understood following mailing thread:
> https://lists.freedesktop.org/archives/piglit/2013-October/007935.html
> looks like we have accepted to remove an ability to change qualifiers
> but have not done it yet. Unless I missed something)
> 
> v2 (idr): Move 'earlier->data.mode != var->data.mode' test much earlier
> in the function.  Add special handling for gl_LastFragData.
> 
> Signed-off-by: Andrii Simiklit <andrii.simiklit at globallogic.com>
> Signed-off-by: Ian Romanick <ian.d.romanick at intel.com>
> ---
>   src/compiler/glsl/ast_to_hir.cpp | 51 +++++++++++++++++++++-------------------
>   1 file changed, 27 insertions(+), 24 deletions(-)
> 
> diff --git a/src/compiler/glsl/ast_to_hir.cpp b/src/compiler/glsl/ast_to_hir.cpp
> index 1082d6c91cf..2e4c9ef6776 100644
> --- a/src/compiler/glsl/ast_to_hir.cpp
> +++ b/src/compiler/glsl/ast_to_hir.cpp
> @@ -4238,6 +4238,29 @@ get_variable_being_redeclared(ir_variable **var_ptr, YYLTYPE loc,
>   
>      *is_redeclaration = true;
>   
> +   if (earlier->data.how_declared == ir_var_declared_implicitly) {
> +      /* Verify that the redeclaration of a built-in does not change the
> +       * storage qualifier.  There are a couple special cases.
> +       *
> +       * 1. Some built-in variables that are defined as 'in' in the
> +       *    specification are implemented as system values.  Allow
> +       *    ir_var_system_value -> ir_var_shader_in.
> +       *
> +       * 2. gl_LastFragData is implemented as a ir_var_shader_out, but the
> +       *    specification requires that redeclarations omit any qualifier.
> +       *    Allow ir_var_shader_out -> ir_var_auto for this one variable.
> +       */
> +      if (earlier->data.mode != var->data.mode &&
> +          !(earlier->data.mode == ir_var_system_value &&
> +            var->data.mode == ir_var_shader_in) &&
> +          !(strcmp(var->name, "gl_LastFragData") == 0 &&
> +            var->data.mode == ir_var_auto)) {
> +         _mesa_glsl_error(&loc, state,
> +                          "redeclaration cannot change qualification of `%s'",
> +                          var->name);
> +      }
> +   }
> +
>      /* From page 24 (page 30 of the PDF) of the GLSL 1.50 spec,
>       *
>       * "It is legal to declare an array without a size and then
> @@ -4246,11 +4269,6 @@ get_variable_being_redeclared(ir_variable **var_ptr, YYLTYPE loc,
>       */
>      if (earlier->type->is_unsized_array() && var->type->is_array()
>          && (var->type->fields.array == earlier->type->fields.array)) {
> -      /* FINISHME: This doesn't match the qualifiers on the two
> -       * FINISHME: declarations.  It's not 100% clear whether this is
> -       * FINISHME: required or not.
> -       */
> -
>         const int size = var->type->array_size();
>         check_builtin_array_max_size(var->name, size, loc, state);
>         if ((size > 0) && (size <= earlier->data.max_array_access)) {
> @@ -4342,28 +4360,13 @@ get_variable_being_redeclared(ir_variable **var_ptr, YYLTYPE loc,
>         earlier->data.precision = var->data.precision;
>         earlier->data.memory_coherent = var->data.memory_coherent;
>   
> -   } else if (earlier->data.how_declared == ir_var_declared_implicitly &&
> -              state->allow_builtin_variable_redeclaration) {
> +   } else if ((earlier->data.how_declared == ir_var_declared_implicitly &&
> +               state->allow_builtin_variable_redeclaration) ||
> +              allow_all_redeclarations) {
>         /* Allow verbatim redeclarations of built-in variables. Not explicitly
>          * valid, but some applications do it.
>          */
> -      if (earlier->data.mode != var->data.mode &&
> -          !(earlier->data.mode == ir_var_system_value &&
> -            var->data.mode == ir_var_shader_in)) {
> -         _mesa_glsl_error(&loc, state,
> -                          "redeclaration of `%s' with incorrect qualifiers",
> -                          var->name);
> -      } else if (earlier->type != var->type) {
> -         _mesa_glsl_error(&loc, state,
> -                          "redeclaration of `%s' has incorrect type",
> -                          var->name);
> -      }
> -   } else if (allow_all_redeclarations) {
> -      if (earlier->data.mode != var->data.mode) {
> -         _mesa_glsl_error(&loc, state,
> -                          "redeclaration of `%s' with incorrect qualifiers",
> -                          var->name);
> -      } else if (earlier->type != var->type) {
> +      if (earlier->type != var->type) {
>            _mesa_glsl_error(&loc, state,
>                             "redeclaration of `%s' has incorrect type",
>                             var->name);
>