glsl: prevent qualifiers modification of predeclared variables

Submitted by andrey simiklit on Oct. 4, 2018, 2:08 p.m.

Details

Message ID 20181004140826.7311-1-asimiklit.work@gmail.com
State New
Headers show
Series "glsl: prevent qualifiers modification of predeclared variables" ( rev: 1 ) in Mesa

Not browsing as part of any series.

Commit Message

andrey simiklit Oct. 4, 2018, 2:08 p.m.
From: Andrii Simiklit <andrii.simiklit@globallogic.com>

GLSL 3.7 (Identifiers):
"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'
and leads to regression in clip-distance-out-values.shader_test
but probably a fix should be made in the test.

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)

Fixes: 8e6cb9fe51a2 "glsl: Refactor AST-to-HIR code handling variable
					 redeclarations"
Signed-off-by: Andrii Simiklit <andrii.simiklit@globallogic.com>
---
 src/compiler/glsl/ast_to_hir.cpp | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/compiler/glsl/ast_to_hir.cpp b/src/compiler/glsl/ast_to_hir.cpp
index 93e7c8ec33..e26ae6b92a 100644
--- a/src/compiler/glsl/ast_to_hir.cpp
+++ b/src/compiler/glsl/ast_to_hir.cpp
@@ -4240,10 +4240,15 @@  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.
-       */
+
+      if ((strcmp("gl_ClipDistance", var->name) == 0) &&
+          earlier->data.mode != var->data.mode) {
+         _mesa_glsl_error(&loc, state,
+                          "redeclaration of '%s %s' with incorrect qualifiers '%s'.",
+                          mode_string(earlier),
+                          var->name,
+                          mode_string(var));
+      }
 
       const int size = var->type->array_size();
       check_builtin_array_max_size(var->name, size, loc, state);

Comments

Hello,

+ idr@freedesktop.org
and add missing bugzilla link
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108247

Regards,
Andrii.

On Thu, Oct 4, 2018 at 5:08 PM <asimiklit.work@gmail.com> wrote:

> From: Andrii Simiklit <andrii.simiklit@globallogic.com>
>
> GLSL 3.7 (Identifiers):
> "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'
> and leads to regression in clip-distance-out-values.shader_test
> but probably a fix should be made in the test.
>
> 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)
>
> Fixes: 8e6cb9fe51a2 "glsl: Refactor AST-to-HIR code handling variable
>                                          redeclarations"
> Signed-off-by: Andrii Simiklit <andrii.simiklit@globallogic.com>
> ---
>  src/compiler/glsl/ast_to_hir.cpp | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/src/compiler/glsl/ast_to_hir.cpp
> b/src/compiler/glsl/ast_to_hir.cpp
> index 93e7c8ec33..e26ae6b92a 100644
> --- a/src/compiler/glsl/ast_to_hir.cpp
> +++ b/src/compiler/glsl/ast_to_hir.cpp
> @@ -4240,10 +4240,15 @@ 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.
> -       */
> +
> +      if ((strcmp("gl_ClipDistance", var->name) == 0) &&
> +          earlier->data.mode != var->data.mode) {
> +         _mesa_glsl_error(&loc, state,
> +                          "redeclaration of '%s %s' with incorrect
> qualifiers '%s'.",
> +                          mode_string(earlier),
> +                          var->name,
> +                          mode_string(var));
> +      }
>
>        const int size = var->type->array_size();
>        check_builtin_array_max_size(var->name, size, loc, state);
> --
> 2.17.1
>
>
On 10/04/2018 07:08 AM, asimiklit.work@gmail.com wrote:
> From: Andrii Simiklit <andrii.simiklit@globallogic.com>
> 
> GLSL 3.7 (Identifiers):
> "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'
> and leads to regression in clip-distance-out-values.shader_test
> but probably a fix should be made in the test.

I believe clip-distance-out-values.shader_test is incorrect.  The redeclaration of gl_ClipDistance should have "out".  glslang rejects it with:

ERROR: 0:5: 'redeclaration' : cannot change qualification of gl_ClipDistance
ERROR: 1 compilation errors.  No code generated.

I think Mark and Clayton would prefer it if the fix to the test landed first.  That way the test never shows up as "fail" to them.

I'll send the fix for this with the new test cases that I mention below.

> 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)
> 
> Fixes: 8e6cb9fe51a2 "glsl: Refactor AST-to-HIR code handling variable
> 					 redeclarations"

While this is technically correct, I don't think we want to add a new compile error to the stable branches.  On the outside chance this breaks some application, I'd rather have it sit on master for a bit first.

> Signed-off-by: Andrii Simiklit <andrii.simiklit@globallogic.com>
> ---
>  src/compiler/glsl/ast_to_hir.cpp | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/src/compiler/glsl/ast_to_hir.cpp b/src/compiler/glsl/ast_to_hir.cpp
> index 93e7c8ec33..e26ae6b92a 100644
> --- a/src/compiler/glsl/ast_to_hir.cpp
> +++ b/src/compiler/glsl/ast_to_hir.cpp
> @@ -4240,10 +4240,15 @@ 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.
> -       */
> +
> +      if ((strcmp("gl_ClipDistance", var->name) == 0) &&

Hmm... this fixes the specific case mentioned in the old e-mail thread, but this could occur with any variable.  For example, I don't think this shader should compile, but it does:

    #version 110
    varying float x[];
    uniform float x[3];
    uniform int i;

    void main()
    {
        gl_Position = vec4(x[i]);
    }

Much to my surprise, glslang does not reject this shader.  It looks like glslang rejects anything that tries to change the storage qualifier on any built-in variable, but it seems to allow such changes on user-defined variables.  I think that's a bug in glslang.

Let's do this for now.

1. Change Mesa to reject any storage qualifier change on a built-in variable.  This will match glslang.  We can do this by bringing the code by the comment "Allow verbatim redeclarations of built-in variables. Not explicitly valid, but some applications do it." up before the big if-then-else tree.  Around line 4235 add something like:

   if (earlier->data.how_declared == ir_var_declared_implicitly) {
      /* 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 cannot change qualification of `%s'",
                          var->name);
      }
   }

I changed the error message to be more similar to glslang.  I like that wording, but I'm flexible.

We'll also want to remove the other tests for this (which will be redundant after the above change).

2. Add a bunch of piglit tests to make sure we match glslang.  I've got a start on this, and I'll CC you on the patches when I send them.

3. I'll submit a GLSL spec bug to make sure cases like my example above are intended to be illegal.

4. Depending on the outcome of #3, update Mesa to match.

> +          earlier->data.mode != var->data.mode) {
> +         _mesa_glsl_error(&loc, state,
> +                          "redeclaration of '%s %s' with incorrect qualifiers '%s'.",
> +                          mode_string(earlier),
> +                          var->name,
> +                          mode_string(var));
> +      }
>  
>        const int size = var->type->array_size();
>        check_builtin_array_max_size(var->name, size, loc, state);
On 10/05/2018 03:02 PM, Ian Romanick wrote:
> On 10/04/2018 07:08 AM, asimiklit.work@gmail.com wrote:
>> From: Andrii Simiklit <andrii.simiklit@globallogic.com>
>>
>> GLSL 3.7 (Identifiers):
>> "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'
>> and leads to regression in clip-distance-out-values.shader_test
>> but probably a fix should be made in the test.
> 
> I believe clip-distance-out-values.shader_test is incorrect.  The redeclaration of gl_ClipDistance should have "out".  glslang rejects it with:
> 
> ERROR: 0:5: 'redeclaration' : cannot change qualification of gl_ClipDistance
> ERROR: 1 compilation errors.  No code generated.
> 
> I think Mark and Clayton would prefer it if the fix to the test landed first.  That way the test never shows up as "fail" to them.
> 
> I'll send the fix for this with the new test cases that I mention below.
> 
>> 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)
>>
>> Fixes: 8e6cb9fe51a2 "glsl: Refactor AST-to-HIR code handling variable
>> 					 redeclarations"
> 
> While this is technically correct, I don't think we want to add a new compile error to the stable branches.  On the outside chance this breaks some application, I'd rather have it sit on master for a bit first.
> 
>> Signed-off-by: Andrii Simiklit <andrii.simiklit@globallogic.com>
>> ---
>>  src/compiler/glsl/ast_to_hir.cpp | 13 +++++++++----
>>  1 file changed, 9 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/compiler/glsl/ast_to_hir.cpp b/src/compiler/glsl/ast_to_hir.cpp
>> index 93e7c8ec33..e26ae6b92a 100644
>> --- a/src/compiler/glsl/ast_to_hir.cpp
>> +++ b/src/compiler/glsl/ast_to_hir.cpp
>> @@ -4240,10 +4240,15 @@ 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.
>> -       */
>> +
>> +      if ((strcmp("gl_ClipDistance", var->name) == 0) &&
> 
> Hmm... this fixes the specific case mentioned in the old e-mail thread, but this could occur with any variable.  For example, I don't think this shader should compile, but it does:
> 
>     #version 110
>     varying float x[];
>     uniform float x[3];
>     uniform int i;
> 
>     void main()
>     {
>         gl_Position = vec4(x[i]);
>     }
> 
> Much to my surprise, glslang does not reject this shader.  It looks like glslang rejects anything that tries to change the storage qualifier on any built-in variable, but it seems to allow such changes on user-defined variables.  I think that's a bug in glslang.
> 
> Let's do this for now.
> 
> 1. Change Mesa to reject any storage qualifier change on a built-in variable.  This will match glslang.  We can do this by bringing the code by the comment "Allow verbatim redeclarations of built-in variables. Not explicitly valid, but some applications do it." up before the big if-then-else tree.  Around line 4235 add something like:
> 
>    if (earlier->data.how_declared == ir_var_declared_implicitly) {
>       /* 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 cannot change qualification of `%s'",
>                           var->name);

Oops...

      } else if (earlier->type != var->type) {
         _mesa_glsl_error(&loc, state,
                          "redeclaration of `%s' has incorrect type",
                          var->name);

>       }
>    }
> 
> I changed the error message to be more similar to glslang.  I like that wording, but I'm flexible.
> 
> We'll also want to remove the other tests for this (which will be redundant after the above change).
> 
> 2. Add a bunch of piglit tests to make sure we match glslang.  I've got a start on this, and I'll CC you on the patches when I send them.
> 
> 3. I'll submit a GLSL spec bug to make sure cases like my example above are intended to be illegal.
> 
> 4. Depending on the outcome of #3, update Mesa to match.
> 
>> +          earlier->data.mode != var->data.mode) {
>> +         _mesa_glsl_error(&loc, state,
>> +                          "redeclaration of '%s %s' with incorrect qualifiers '%s'.",
>> +                          mode_string(earlier),
>> +                          var->name,
>> +                          mode_string(var));
>> +      }
>>  
>>        const int size = var->type->array_size();
>>        check_builtin_array_max_size(var->name, size, loc, state);
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
Hello,

Please find my comments below:

On Sat, Oct 6, 2018 at 1:07 AM Ian Romanick <idr@freedesktop.org> wrote:

> On 10/05/2018 03:02 PM, Ian Romanick wrote:
> > On 10/04/2018 07:08 AM, asimiklit.work@gmail.com wrote:
> >> From: Andrii Simiklit <andrii.simiklit@globallogic.com>
> >>
> >> GLSL 3.7 (Identifiers):
> >> "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'
> >> and leads to regression in clip-distance-out-values.shader_test
> >> but probably a fix should be made in the test.
> >
> > I believe clip-distance-out-values.shader_test is incorrect.  The
> redeclaration of gl_ClipDistance should have "out".  glslang rejects it
> with:
> >
> > ERROR: 0:5: 'redeclaration' : cannot change qualification of
> gl_ClipDistance
> > ERROR: 1 compilation errors.  No code generated.
> >
> > I think Mark and Clayton would prefer it if the fix to the test landed
> first.  That way the test never shows up as "fail" to them.
> >
> > I'll send the fix for this with the new test cases that I mention below.
> >
> >> 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)
> >>
> >> Fixes: 8e6cb9fe51a2 "glsl: Refactor AST-to-HIR code handling variable
> >>                                       redeclarations"
> >
> > While this is technically correct, I don't think we want to add a new
> compile error to the stable branches.  On the outside chance this breaks
> some application, I'd rather have it sit on master for a bit first.
> >
> >> Signed-off-by: Andrii Simiklit <andrii.simiklit@globallogic.com>
> >> ---
> >>  src/compiler/glsl/ast_to_hir.cpp | 13 +++++++++----
> >>  1 file changed, 9 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/src/compiler/glsl/ast_to_hir.cpp
> b/src/compiler/glsl/ast_to_hir.cpp
> >> index 93e7c8ec33..e26ae6b92a 100644
> >> --- a/src/compiler/glsl/ast_to_hir.cpp
> >> +++ b/src/compiler/glsl/ast_to_hir.cpp
> >> @@ -4240,10 +4240,15 @@ 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.
> >> -       */
> >> +
> >> +      if ((strcmp("gl_ClipDistance", var->name) == 0) &&
> >
> > Hmm... this fixes the specific case mentioned in the old e-mail thread,
> but this could occur with any variable.  For example, I don't think this
> shader should compile, but it does:
> >
> >     #version 110
> >     varying float x[];
> >     uniform float x[3];
> >     uniform int i;
> >
> >     void main()
> >     {
> >         gl_Position = vec4(x[i]);
> >     }
> >
> > Much to my surprise, glslang does not reject this shader.  It looks like
> glslang rejects anything that tries to change the storage qualifier on any
> built-in variable, but it seems to allow such changes on user-defined
> variables.  I think that's a bug in glslang.
> >
> > Let's do this for now.
> >
> > 1. Change Mesa to reject any storage qualifier change on a built-in
> variable.  This will match glslang.  We can do this by bringing the code by
> the comment "Allow verbatim redeclarations of built-in variables. Not
> explicitly valid, but some applications do it." up before the big
> if-then-else tree.  Around line 4235 add something like:
> >
> >    if (earlier->data.how_declared == ir_var_declared_implicitly) {
> >       /* 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 cannot change qualification of
> `%s'",
> >                           var->name);
>
> Oops...
>
>       } else if (earlier->type != var->type) {
>          _mesa_glsl_error(&loc, state,
>                           "redeclaration of `%s' has incorrect type",
>                           var->name);
>
> >       }
> >    }
>

I have tried to test this version on Intel CI.

https://gitlab.freedesktop.org/asimiklit/mesa/commit/9d4f6a97568e7e2720e43601697eaf90908b7aab

but it caused many errors:

https://mesa-ci.01.org/global_logic/builds/16/group/63a9f0ea7bb98050796b649e85481845

so I modified it in the following way:

- } else if (earlier->type != var->type) {
+ } else if (earlier->type->without_array() != var->type->without_array()) {

https://gitlab.freedesktop.org/asimiklit/mesa/commit/7f313c10640a72e37069f54baadaae1bc3d5a0a3

and it helped a bit:

https://mesa-ci.01.org/global_logic/builds/17/group/63a9f0ea7bb98050796b649e85481845

I have been investigating the remaining errors.
Some part of the errors refer to 'error: redeclaration of `gl_LastFragData'
with incorrect qualifiers'
(ex. https://mesa-ci.01.org/global_logic/builds/17/results/23218604 )
so probably we have to do some workaround for this variable
because it is added as 'ir_var_shader_out' but then
it is expected as 'ir_var_auto' in 'get_variable_being_redeclared' function.

I think about something like:
+   const bool qualifiersChangeIsRequired =
+                               (state->has_framebuffer_fetch() &&
+                                !state->is_version(130, 300) &&
+                                var->type->is_array() &&
+                                strcmp(var->name, "gl_LastFragData") == 0);
    if (earlier->data.how_declared == ir_var_declared_implicitly) {
       /* 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)) {
+            var->data.mode == ir_var_shader_in) &&
+          !qualifiersChangeIsRequired) {
          _mesa_glsl_error(&loc, state,
                           "redeclaration of `%s' with incorrect
qualifiers",
                           var->name);

What do you think about this workaround?

>
> > I changed the error message to be more similar to glslang.  I like that
> wording, but I'm flexible.
> >
> > We'll also want to remove the other tests for this (which will be
> redundant after the above change).
> >
> > 2. Add a bunch of piglit tests to make sure we match glslang.  I've got
> a start on this, and I'll CC you on the patches when I send them.
> >
> > 3. I'll submit a GLSL spec bug to make sure cases like my example above
> are intended to be illegal.
> >
> > 4. Depending on the outcome of #3, update Mesa to match.
> >
> >> +          earlier->data.mode != var->data.mode) {
> >> +         _mesa_glsl_error(&loc, state,
> >> +                          "redeclaration of '%s %s' with incorrect
> qualifiers '%s'.",
> >> +                          mode_string(earlier),
> >> +                          var->name,
> >> +                          mode_string(var));
> >> +      }
> >>
> >>        const int size = var->type->array_size();
> >>        check_builtin_array_max_size(var->name, size, loc, state);
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> >
>
>
Regards,
Andrii.
On 10/09/2018 06:52 AM, andrey simiklit wrote:
> Hello,
> 
> Please find my comments below:
> 
> On Sat, Oct 6, 2018 at 1:07 AM Ian Romanick <idr@freedesktop.org
> <mailto:idr@freedesktop.org>> wrote:
> 
>     On 10/05/2018 03:02 PM, Ian Romanick wrote:
>     > On 10/04/2018 07:08 AM, asimiklit.work@gmail.com
>     <mailto:asimiklit.work@gmail.com> wrote:
>     >> From: Andrii Simiklit <andrii.simiklit@globallogic.com
>     <mailto:andrii.simiklit@globallogic.com>>
>     >>
>     >> GLSL 3.7 (Identifiers):
>     >> "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'
>     >> and leads to regression in clip-distance-out-values.shader_test
>     >> but probably a fix should be made in the test.
>     >
>     > I believe clip-distance-out-values.shader_test is incorrect.  The
>     redeclaration of gl_ClipDistance should have "out".  glslang rejects
>     it with:
>     >
>     > ERROR: 0:5: 'redeclaration' : cannot change qualification of
>     gl_ClipDistance
>     > ERROR: 1 compilation errors.  No code generated.
>     >
>     > I think Mark and Clayton would prefer it if the fix to the test
>     landed first.  That way the test never shows up as "fail" to them.
>     >
>     > I'll send the fix for this with the new test cases that I mention
>     below.
>     >
>     >> 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)
>     >>
>     >> Fixes: 8e6cb9fe51a2 "glsl: Refactor AST-to-HIR code handling variable
>     >>                                       redeclarations"
>     >
>     > While this is technically correct, I don't think we want to add a
>     new compile error to the stable branches.  On the outside chance
>     this breaks some application, I'd rather have it sit on master for a
>     bit first.
>     >
>     >> Signed-off-by: Andrii Simiklit <andrii.simiklit@globallogic.com
>     <mailto:andrii.simiklit@globallogic.com>>
>     >> ---
>     >>  src/compiler/glsl/ast_to_hir.cpp | 13 +++++++++----
>     >>  1 file changed, 9 insertions(+), 4 deletions(-)
>     >>
>     >> diff --git a/src/compiler/glsl/ast_to_hir.cpp
>     b/src/compiler/glsl/ast_to_hir.cpp
>     >> index 93e7c8ec33..e26ae6b92a 100644
>     >> --- a/src/compiler/glsl/ast_to_hir.cpp
>     >> +++ b/src/compiler/glsl/ast_to_hir.cpp
>     >> @@ -4240,10 +4240,15 @@ 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.
>     >> -       */
>     >> +
>     >> +      if ((strcmp("gl_ClipDistance", var->name) == 0) &&
>     >
>     > Hmm... this fixes the specific case mentioned in the old e-mail
>     thread, but this could occur with any variable.  For example, I
>     don't think this shader should compile, but it does:
>     >
>     >     #version 110
>     >     varying float x[];
>     >     uniform float x[3];
>     >     uniform int i;
>     >
>     >     void main()
>     >     {
>     >         gl_Position = vec4(x[i]);
>     >     }
>     >
>     > Much to my surprise, glslang does not reject this shader.  It
>     looks like glslang rejects anything that tries to change the storage
>     qualifier on any built-in variable, but it seems to allow such
>     changes on user-defined variables.  I think that's a bug in glslang.
>     >
>     > Let's do this for now.
>     >
>     > 1. Change Mesa to reject any storage qualifier change on a
>     built-in variable.  This will match glslang.  We can do this by
>     bringing the code by the comment "Allow verbatim redeclarations of
>     built-in variables. Not explicitly valid, but some applications do
>     it." up before the big if-then-else tree.  Around line 4235 add
>     something like:
>     >
>     >    if (earlier->data.how_declared == ir_var_declared_implicitly) {
>     >       /* 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 cannot change
>     qualification of `%s'",
>     >                           var->name);
> 
>     Oops...
> 
>           } else if (earlier->type != var->type) {
>              _mesa_glsl_error(&loc, state,
>                               "redeclaration of `%s' has incorrect type",
>                               var->name);
> 
>     >       }
>     >    }
> 
> 
> I have tried to test this version on Intel CI.
> 
> https://gitlab.freedesktop.org/asimiklit/mesa/commit/9d4f6a97568e7e2720e43601697eaf90908b7aab
> 
> but it caused many errors:
> 
> https://mesa-ci.01.org/global_logic/builds/16/group/63a9f0ea7bb98050796b649e85481845
> 
> so I modified it in the following way:
> 
> - } else if (earlier->type != var->type) {
> + } else if (earlier->type->without_array() != var->type->without_array()) {
> 
> https://gitlab.freedesktop.org/asimiklit/mesa/commit/7f313c10640a72e37069f54baadaae1bc3d5a0a3
> 
> and it helped a bit:
> 
> https://mesa-ci.01.org/global_logic/builds/17/group/63a9f0ea7bb98050796b649e85481845

I poked at this a little bit too.  The earlier->type vs. var->type check
can't be moved before the if-statements.  The problem occurs when
earlier->type is an implicitly sized array (like gl_ClipDistance or
gl_TexCoord) and var->type explicitly sizes the array.  The types won't
(and should not) match.

I believe your modified test would allow shaders that resize built-in
arrays that have a fixed size (e.g., gl_FragData).

> I have been investigating the remaining errors.
> Some part of the errors refer to 'error: redeclaration of
> `gl_LastFragData' with incorrect qualifiers'
> (ex. https://mesa-ci.01.org/global_logic/builds/17/results/23218604 )
> so probably we have to do some workaround for this variable
> because it is added as 'ir_var_shader_out' but then
> it is expected as 'ir_var_auto' in 'get_variable_being_redeclared' function.
> 
> I think about something like:
> +   const bool qualifiersChangeIsRequired =
> +                               (state->has_framebuffer_fetch() &&
> +                                !state->is_version(130, 300) &&
> +                                var->type->is_array() &&
> +                                strcmp(var->name, "gl_LastFragData") == 0);
>     if (earlier->data.how_declared == ir_var_declared_implicitly) {
>        /* 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)) {
> +            var->data.mode == ir_var_shader_in) &&
> +          !qualifiersChangeIsRequired) {
>           _mesa_glsl_error(&loc, state,
>                            "redeclaration of `%s' with incorrect
> qualifiers",
>                            var->name);
> 
> What do you think about this workaround?

This basically moves an existing check earlier in the function.  I think
this is starting to turn into the old woman who swallowed a fly.  We're
adding a little more fix on to each fix until all that's left is chaos.
The fundamental problem is that my suggestion to move the type check
earlier was just wrong.

I simplified the fix a bit, and I added a couple refactors on top.  I
ran this through the CI, and it looks good.

https://cgit.freedesktop.org/~idr/mesa/log/?h=ast_to_hir-work

>     > I changed the error message to be more similar to glslang.  I like
>     that wording, but I'm flexible.
>     >
>     > We'll also want to remove the other tests for this (which will be
>     redundant after the above change).
>     >
>     > 2. Add a bunch of piglit tests to make sure we match glslang. 
>     I've got a start on this, and I'll CC you on the patches when I send
>     them.
>     >
>     > 3. I'll submit a GLSL spec bug to make sure cases like my example
>     above are intended to be illegal.
>     >
>     > 4. Depending on the outcome of #3, update Mesa to match.
>     >
>     >> +          earlier->data.mode != var->data.mode) {
>     >> +         _mesa_glsl_error(&loc, state,
>     >> +                          "redeclaration of '%s %s' with
>     incorrect qualifiers '%s'.",
>     >> +                          mode_string(earlier),
>     >> +                          var->name,
>     >> +                          mode_string(var));
>     >> +      }
>     >> 
>     >>        const int size = var->type->array_size();
>     >>        check_builtin_array_max_size(var->name, size, loc, state);
>     > _______________________________________________
>     > mesa-dev mailing list
>     > mesa-dev@lists.freedesktop.org <mailto:mesa-dev@lists.freedesktop.org>
>     > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>     >
>      
> 
> Regards,
> Andrii.