[Mesa-dev] [PATCH] glsl: prevent qualifiers modification of predeclared variables

[Ian Romanick]

On 10/05/2018 03:02 PM, Ian Romanick wrote:
> On 10/04/2018 07:08 AM, asimiklit.work at gmail.com wrote:
>> From: Andrii Simiklit <andrii.simiklit at 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 at 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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>