[Mesa-dev] [PATCH] glsl: validate arrays of arrays on empty type delclarations

[Timothy Arceri]

On Tue, 2016-02-09 at 00:50 -0800, Kenneth Graunke wrote:
> On Friday, February 5, 2016 1:08:19 PM PST Timothy Arceri wrote:
> > Fixes:
> > dEQP-
> > GLES31.functional.shaders.arrays_of_arrays.invalid.empty_declaratio
> > n_without_var_name_fragment
> > dEQP-
> > GLES31.functional.shaders.arrays_of_arrays.invalid.empty_declaratio
> > n_without_var_name_vertex
> > 
> > Cc: Ilia Mirkin <imirkin at alum.mit.edu>
> > ---
> >  src/compiler/glsl/ast_to_hir.cpp | 63 ++++++++++++++++++++++++--
> > --------------
> >  1 file changed, 38 insertions(+), 25 deletions(-)
> > 
> > diff --git a/src/compiler/glsl/ast_to_hir.cpp
> > b/src/compiler/glsl/ast_to_hir.cpp
> > index 6890172..5df3a85 100644
> > --- a/src/compiler/glsl/ast_to_hir.cpp
> > +++ b/src/compiler/glsl/ast_to_hir.cpp
> > @@ -4267,33 +4267,46 @@ ast_declarator_list::hir(exec_list
> > *instructions,
> >           _mesa_glsl_error(&loc, state,
> >                            "invalid type `%s' in empty
> > declaration",
> >                            type_name);
> > -      } else if (decl_type->base_type == GLSL_TYPE_ATOMIC_UINT) {
> > -         /* Empty atomic counter declarations are allowed and
> > useful
> > -          * to set the default offset qualifier.
> > -          */
> > -         return NULL;
> > -      } else if (this->type->qualifier.precision !=
> > ast_precision_none) {
> > -         if (this->type->specifier->structure != NULL) {
> > -            _mesa_glsl_error(&loc, state,
> > -                             "precision qualifiers can't be
> > applied "
> > -                             "to structures");
> > -         } else {
> > -            static const char *const precision_names[] = {
> > -               "highp",
> > -               "highp",
> > -               "mediump",
> > -               "lowp"
> > -            };
> > +      } else {
> > +         if (decl_type->base_type == GLSL_TYPE_ARRAY) {
> > +            /* From Section 4.12 (Empty Declarations) of the GLSL
> > 4.5 spec:
> > +             *
> > +             *    "The combinations of types and qualifiers that
> > cause
> > +             *    compile-time or link-time errors are the same
> > whether or not
> > +             *    the declaration is empty."
> > +             */
> > +            validate_array_dimensions(decl_type, state, &loc);
> > +         }
> 
> Hi Tim,
> 
> I'm a bit puzzled by the if ladders here.  I originally expected to
> see
> 
>    if (decl_type == NULL) {
>       ...
>    } else if (decl_type->base_type == GLSL_TYPE_ARRAY) {
>       ...
>    } else if (decl_type->base_type == GLSL_TYPE_ATOMIC_UINT) {
>       ...
>    } else if (this->type->qualifier.precision != ast_precision_none)
> {
>       ...
>    } else if (this->type->specifier->structure == NULL) {
>       ...
>    }
> 
> instead of everything but the first case indented a level, with two
> checks of decl_type->base_type without an else if.  I guess the way
> you structured it, the precision check happens for arrays, and the
> "empty declaration" warning would happen too?  Presumably at least
> the last one is useful?
> 
> Honestly, the precision check seems a bit weird and out of place here
> (even before your patch) - it gets skipped for ATOMIC_UINT (why?),
> and explicitly complains about structs, but not arrays?
> 
> It seems like what we really want is:
> 
>     if (decl_type == NULL) {
>        error: bogus type...
>     } else {
>         if (this->type->qualifier.precision != ast_precision_none) {
>            if (decl_type isn't glsl_type::float_type / int_type) {
>               error; precision qualifiers can only be applied to
>               float or int AFAICT...
>            } else {
>               warn: empty declaration with precision qualifier; wrong
>            }
>         }
> 
>         if (decl_type->base_type == GLSL_TYPE_ARRAY) {
>            ...validate the array things...
>         } else if (decl_type->base_type != OGLSL_TYPE_ATOMIC_UINT &&
>                    this->type->specifier->structure != NULL) {
>            warn: empty declaration
>         }
>     }
> 
> This would raise an error for precision qualifiers on arrays, too,
> which I think we want - and also for vec2/ivec2/uint/etc, which I
> also think we want (though outside the scope of your original patch).
> (Or, maybe forbidding vectors is done elsewhere?)
> 
> What do you think?  ast->hir code is rather messy :(

Yeah I was trying to make it work for the precision case  when we have
arrays and possibly to hit the structs warning too although I didn't
think about that case too much.

I'm probably missing something but the logic of your code looks the
same as mine just backwards. To me the code looked broken already,
there are many rules not checked for here and I was just trying to fix
the dEQP tests. IMO this stuff is only really usefull for passing
conformance tests.

With your change it now warns for empty atomics but the comment says
that they are actually useful so maybe we wouldn't want to warn. The
other thing is the atomic case (and maybe the struct cases) don't seem
to remove arrays from the type before doing their checks so they miss
arrays of atomics etc.

Anyway I've already pushed this so not sure if its worth trying to fix
up the rest of the problems or not.

Tim

> 
> > -            _mesa_glsl_warning(&loc, state,
> > -                               "empty declaration with precision
> > qualifier, "
> > -                               "to set the default precision, use
> > "
> > -                               "`precision %s %s;'",
> > -                               precision_names[this->type-
> > >qualifier.precision],
> > -                               type_name);
> > +         if (decl_type->base_type == GLSL_TYPE_ATOMIC_UINT) {
> > +            /* Empty atomic counter declarations are allowed and
> > useful
> > +             * to set the default offset qualifier.
> > +             */
> > +            return NULL;
> > +         } else if (this->type->qualifier.precision !=
> > ast_precision_none) {
> > +            if (this->type->specifier->structure != NULL) {
> > +               _mesa_glsl_error(&loc, state,
> > +                                "precision qualifiers can't be
> > applied "
> > +                                "to structures");
> > +            } else {
> > +               static const char *const precision_names[] = {
> > +                  "highp",
> > +                  "highp",
> > +                  "mediump",
> > +                  "lowp"
> > +               };
> > +
> > +               _mesa_glsl_warning(&loc, state,
> > +                                  "empty declaration with
> > precision "
> > +                                  "qualifier, to set the default
> > precision, "
> > +                                  "use `precision %s %s;'",
> > +                                  precision_names[this->type->
> > +                                     qualifier.precision],
> > +                                  type_name);
> > +            }
> > +         } else if (this->type->specifier->structure == NULL) {
> > +            _mesa_glsl_warning(&loc, state, "empty declaration");
> >           }
> > -      } else if (this->type->specifier->structure == NULL) {
> > -         _mesa_glsl_warning(&loc, state, "empty declaration");
> >        }
> >     }
> >  
> > 
>