[Mesa-dev] [PATCH V2] glsl: Validate aux storage qualifier combination with other qualifiers.

[Kenneth Graunke]

On 04/11/2014 06:21 PM, Chris Forbes wrote:
> We've been allowing `centroid` and `sample` in all kinds of weird places
> where they're not valid.
> 
> Insist that `sample` is combined with `in` or `out`;
> and that `centroid` is combined with `in`, `out`, or the deprecated
> `varying`.
> 
> V2: Validate this in a more sensible place. This does require an extra
> case for uniform blocks members and struct members, though, since they
> don't go through the normal path.
> 
> Signed-off-by: Chris Forbes <chrisf at ijw.co.nz>
> ---
>  src/glsl/ast_to_hir.cpp | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp
> index 1d15e62..6c7e00a 100644
> --- a/src/glsl/ast_to_hir.cpp
> +++ b/src/glsl/ast_to_hir.cpp
> @@ -2552,6 +2552,19 @@ apply_type_qualifier_to_variable(const struct ast_type_qualifier *qual,
>     const bool uses_deprecated_qualifier = qual->flags.q.attribute
>        || qual->flags.q.varying;
>  
> +
> +   /* Validate auxiliary storage qualifiers */

Spec citations would be great if you can find them.

> +   if (qual->flags.q.sample && (!is_varying_var(var, state->stage) || uses_deprecated_qualifier)) {
> +      _mesa_glsl_error(loc, state,
> +                       "sample qualifier may only be used with `in' or `out'");
> +   }
> +   if (qual->flags.q.centroid && !is_varying_var(var, state->stage)) {
> +      _mesa_glsl_error(loc, state,
> +                       "centroid qualifier may only be used with `in', "
> +                       "`out' or `varying'");
> +   }

This does a little more than the error message says - won't it prevent using
sample/centroid on, say, vertex shader inputs/fragment shader outputs?

If so, then it seems like you could drop a pile of other code:

      /* From section 4.3.4 of the GLSL 1.30 spec:
       *    "It is an error to use centroid in in a vertex shader."
       *
       * From section 4.3.4 of the GLSL ES 3.00 spec:
       *    "It is an error to use centroid in or interpolation qualifiers in
       *    a vertex shader input."
       */
      if (state->is_version(130, 300)
          && this->type->qualifier.flags.q.centroid
          && this->type->qualifier.flags.q.in
          && state->stage == MESA_SHADER_VERTEX) {

         _mesa_glsl_error(&loc, state,
                          "'centroid in' cannot be used in a vertex shader");
      }

      if (state->stage == MESA_SHADER_VERTEX
          && this->type->qualifier.flags.q.sample
          && this->type->qualifier.flags.q.in) {

         _mesa_glsl_error(&loc, state,
                        "'sample in' cannot be used in a vertex shader");
      }

      /* Section 4.3.6 of the GLSL 1.30 specification states:
       * "It is an error to use centroid out in a fragment shader."
       *
       * The GL_ARB_shading_language_420pack extension specification states:
       * "It is an error to use auxiliary storage qualifiers or interpolation
       *  qualifiers on an output in a fragment shader."
       */
      if (state->stage == MESA_SHADER_FRAGMENT &&
          this->type->qualifier.flags.q.out &&
          this->type->qualifier.has_auxiliary_storage()) {
         _mesa_glsl_error(&loc, state,
                          "auxiliary storage qualifiers cannot be used on "
                          "fragment shader outputs");
      }

> +
> +
>     /* Is the 'layout' keyword used with parameters that allow relaxed checking.
>      * Many implementations of GL_ARB_fragment_coord_conventions_enable and some
>      * implementations (only Mesa?) GL_ARB_explicit_attrib_location_enable
> @@ -4924,6 +4937,13 @@ ast_process_structure_or_interface_block(exec_list *instructions,
>                               "with uniform interface blocks");
>           }
>  

/* From the GLSL 4.20 specification, section 4.3 "Storage Qualifiers":
 * "[...] structure members do not use storage qualifiers."
 */

presumably this applies to auxiliary storage qualifiers too...it was the
best I could find.  Which, incidentally, makes me wonder...where do we check
for storage qualifiers?  Maybe do it in the same place?

> +         if ((qual->flags.q.uniform || !is_interface) &&
> +             qual->has_auxiliary_storage()) {
> +            _mesa_glsl_error(&loc, state,
> +                             "auxiliary storage qualifiers cannot be used "
> +                             "in uniform blocks or structures.");
> +         }
> +
>           if (field_type->is_matrix() ||
>               (field_type->is_array() && field_type->fields.array->is_matrix())) {
>              fields[i].row_major = block_row_major;
> 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: OpenPGP digital signature
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20140411/116d159e/attachment.sig>