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

[Kenneth Graunke]

On Friday, February 5, 2016 1:08:19 PM PST Timothy Arceri wrote:
> Fixes:
> dEQP-GLES31.functional.shaders.arrays_of_arrays.invalid.empty_declaration_without_var_name_fragment
> dEQP-GLES31.functional.shaders.arrays_of_arrays.invalid.empty_declaration_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 :(

> -            _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");
>        }
>     }
>  
> 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160209/8f874c26/attachment.sig>