[Mesa-dev] [PATCH 3/3] glsl: be more strict on block qualifiers

[Kenneth Graunke]

On Saturday, July 30, 2016 4:38:06 PM PDT Timothy Arceri wrote:
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=96528
> ---
>  src/compiler/glsl/ast_to_hir.cpp | 61 ++++++++++++++++++++++++++++++++--------
>  1 file changed, 50 insertions(+), 11 deletions(-)
> 
> diff --git a/src/compiler/glsl/ast_to_hir.cpp b/src/compiler/glsl/ast_to_hir.cpp
> index 683c144..f2ed82a 100644
> --- a/src/compiler/glsl/ast_to_hir.cpp
> +++ b/src/compiler/glsl/ast_to_hir.cpp
> @@ -7054,13 +7054,58 @@ ast_interface_block::hir(exec_list *instructions,
>                         this->block_name);
>     }
>  
> -   if (!this->layout.flags.q.buffer &&
> -       this->layout.flags.q.std430) {
> -      _mesa_glsl_error(&loc, state,
> -                       "std430 storage block layout qualifier is supported "
> -                       "only for shader storage blocks");
> +   /* Validate layout qualifiers */

Maybe mention the table on page 62 of the GLSL 4.50 spec?

Oddly, it seems to indicate std430 should be supported for uniform
blocks as well...but the existing code here clearly disallowed them.

> +   ast_type_qualifier allowed_blk_qualifiers;
> +   allowed_blk_qualifiers.flags.i = 0;
> +   if (this->layout.flags.q.buffer || this->layout.flags.q.uniform) {
> +      allowed_blk_qualifiers.flags.q.shared = 1;
> +      allowed_blk_qualifiers.flags.q.packed = 1;
> +      allowed_blk_qualifiers.flags.q.std140 = 1;
> +      allowed_blk_qualifiers.flags.q.row_major = 1;
> +      allowed_blk_qualifiers.flags.q.column_major = 1;
> +      allowed_blk_qualifiers.flags.q.explicit_align = 1;
> +      allowed_blk_qualifiers.flags.q.explicit_binding = 1;
> +      if (this->layout.flags.q.buffer) {
> +         allowed_blk_qualifiers.flags.q.buffer = 1;
> +         allowed_blk_qualifiers.flags.q.std430 = 1;
> +         allowed_blk_qualifiers.flags.q.coherent = 1;
> +         allowed_blk_qualifiers.flags.q._volatile = 1;
> +         allowed_blk_qualifiers.flags.q.restrict_flag = 1;
> +         allowed_blk_qualifiers.flags.q.read_only = 1;
> +         allowed_blk_qualifiers.flags.q.write_only = 1;
> +      } else {
> +         allowed_blk_qualifiers.flags.q.uniform = 1;
> +      }
> +   } else {
> +      /* Interface block */
> +      assert(this->layout.flags.q.in || this->layout.flags.q.out);
> +
> +      allowed_blk_qualifiers.flags.q.explicit_location = 1;
> +      if (this->layout.flags.q.out) {
> +         allowed_blk_qualifiers.flags.q.out = 1;
> +         if (state->stage == MESA_SHADER_GEOMETRY ||
> +          state->stage == MESA_SHADER_TESS_CTRL ||
> +          state->stage == MESA_SHADER_TESS_EVAL ||
> +          state->stage == MESA_SHADER_VERTEX ) {
> +            allowed_blk_qualifiers.flags.q.explicit_xfb_offset = 1;
> +            allowed_blk_qualifiers.flags.q.explicit_xfb_buffer = 1;
> +            allowed_blk_qualifiers.flags.q.xfb_buffer = 1;
> +            allowed_blk_qualifiers.flags.q.explicit_xfb_stride = 1;
> +            allowed_blk_qualifiers.flags.q.xfb_stride = 1;
> +            if (state->stage == MESA_SHADER_GEOMETRY) {
> +               allowed_blk_qualifiers.flags.q.stream = 1;
> +               allowed_blk_qualifiers.flags.q.explicit_stream = 1;
> +            }

         if (state->stage == MESA_SHADER_TESS_CTRL) {
            allowed_blk_qualifiers.flags.q.patch = 1;
         }

> +         }
> +      } else {
> +         allowed_blk_qualifiers.flags.q.in = 1;

         if (state->stage == MESA_SHADER_TESS_EVAL) {
            allowed_blk_qualifiers.flags.q.patch = 1;
         }

With those changes, the series is:
Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>

> +      }
>     }
>  
> +   this->layout.validate_flags(&loc, state, allowed_blk_qualifiers,
> +                               "invalid qualifier for block",
> +                               this->block_name);
> +
>     /* The ast_interface_block has a list of ast_declarator_lists.  We
>      * need to turn those into ir_variables with an association
>      * with this uniform block.
> @@ -7116,12 +7161,6 @@ ast_interface_block::hir(exec_list *instructions,
>                         "Interface block sets both readonly and writeonly");
>     }
>  
> -   if (this->layout.flags.q.explicit_component) {
> -      _mesa_glsl_error(&loc, state, "component layout qualifier cannot be "
> -                       "applied to a matrix, a structure, a block, or an "
> -                       "array containing any of these.");
> -   }
> -
>     unsigned qual_stream;
>     if (!process_qualifier_constant(state, &loc, "stream", this->layout.stream,
>                                     &qual_stream) ||
> 

-------------- 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/20160730/346bb6d6/attachment.sig>