[Mesa-stable] [Mesa-dev] [PATCH v2] glsl: Prohibit illegal mixing of redeclarations inside/outside gl_PerVertex.

Ian Romanick idr at freedesktop.org
Wed Nov 20 11:26:36 PST 2013


On 11/19/2013 05:55 PM, Paul Berry wrote:
> From section 7.1 (Built-In Language Variables) of the GLSL 4.10
> spec:
> 
>     Also, if a built-in interface block is redeclared, no member of
>     the built-in declaration can be redeclared outside the block
>     redeclaration.
> 
> We have been regarding this text as a clarification to the behaviour
> established for gl_PerVertex by GLSL 1.50, so we apply it regardless
> of GLSL version.
> 
> This patch enforces the rule by adding a boolean to ir_variable to

But it's not a boolean. :)  Tri-state?

> track how the variable was declared: implicitly, normally, or in an
> interface block.
> 
> Fixes piglit tests:
> - gs-redeclares-pervertex-out-after-global-redeclaration.geom
> - vs-redeclares-pervertex-out-after-global-redeclaration.vert
> - gs-redeclares-pervertex-out-after-other-global-redeclaration.geom
> - vs-redeclares-pervertex-out-after-other-global-redeclaration.vert
> - gs-redeclares-pervertex-out-before-global-redeclaration
> - vs-redeclares-pervertex-out-before-global-redeclaration
> 
> Cc: "10.0" <mesa-stable at lists.freedesktop.org>
> 
> v2: Don't set "how_declared" redundantly in builtin_variables.cpp.
> Properly clone "how_declared".
> ---
>  src/glsl/ast_to_hir.cpp        | 20 ++++++++++++++++++++
>  src/glsl/builtin_variables.cpp |  1 +
>  src/glsl/ir.cpp                |  3 ++-
>  src/glsl/ir.h                  | 36 ++++++++++++++++++++++++++++++++++++
>  src/glsl/ir_clone.cpp          |  1 +
>  5 files changed, 60 insertions(+), 1 deletion(-)
> 
> diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp
> index 76b256c..0128047 100644
> --- a/src/glsl/ast_to_hir.cpp
> +++ b/src/glsl/ast_to_hir.cpp
> @@ -3355,6 +3355,15 @@ ast_declarator_list::hir(exec_list *instructions,
>        ir_variable *earlier =
>           get_variable_being_redeclared(var, decl->get_location(), state,
>                                         false /* allow_all_redeclarations */);
> +      if (earlier != NULL) {
> +         if (strncmp(var->name, "gl_", 3) == 0 &&
> +             earlier->how_declared == ir_var_declared_in_block) {
> +            _mesa_glsl_error(&loc, state,
> +                             "`%s' has already been redeclared using "
> +                             "gl_PerVertex", var->name);
> +         }
> +         earlier->how_declared = ir_var_declared_normally;
> +      }
>  
>        if (decl->initializer != NULL) {
>  	 result = process_initializer((earlier == NULL) ? var : earlier,
> @@ -5048,6 +5057,7 @@ ast_interface_block::hir(exec_list *instructions,
>              _mesa_glsl_error(&loc, state, "`%s' redeclared",
>                               this->instance_name);
>           }
> +         earlier->how_declared = ir_var_declared_normally;

Is this correct?  At this point redeclaring_per_vertex is true, so it
seems like this should be ir_var_declared_in_block.

>           earlier->type = var->type;
>           earlier->reinit_interface_type(block_type);
>           delete var;
> @@ -5078,7 +5088,11 @@ ast_interface_block::hir(exec_list *instructions,
>                 _mesa_glsl_error(&loc, state,
>                                  "redeclaration of gl_PerVertex can only "
>                                  "include built-in variables");
> +            } else if (earlier->how_declared == ir_var_declared_normally) {
> +               _mesa_glsl_error(&loc, state,
> +                                "`%s' has already been redeclared", var->name);
>              } else {
> +               earlier->how_declared = ir_var_declared_in_block;
>                 earlier->reinit_interface_type(block_type);
>              }
>              continue;
> @@ -5125,6 +5139,12 @@ ast_interface_block::hir(exec_list *instructions,
>              if (var != NULL &&
>                  var->get_interface_type() == earlier_per_vertex &&
>                  var->mode == var_mode) {
> +               if (var->how_declared == ir_var_declared_normally) {
> +                  _mesa_glsl_error(&loc, state,
> +                                   "redeclaration of gl_PerVertex cannot "
> +                                   "follow a redeclaration of `%s'",
> +                                   var->name);
> +               }
>                 state->symbols->disable_variable(var->name);
>                 var->remove();
>              }
> diff --git a/src/glsl/builtin_variables.cpp b/src/glsl/builtin_variables.cpp
> index 4d44104..d57324c 100644
> --- a/src/glsl/builtin_variables.cpp
> +++ b/src/glsl/builtin_variables.cpp
> @@ -434,6 +434,7 @@ builtin_variable_generator::add_variable(const char *name,
>                                           enum ir_variable_mode mode, int slot)
>  {
>     ir_variable *var = new(symtab) ir_variable(type, name, mode);
> +   var->how_declared = ir_var_declared_implicitly;
>  
>     switch (var->mode) {
>     case ir_var_auto:
> diff --git a/src/glsl/ir.cpp b/src/glsl/ir.cpp
> index 1b49736..ffff297 100644
> --- a/src/glsl/ir.cpp
> +++ b/src/glsl/ir.cpp
> @@ -1586,7 +1586,8 @@ ir_variable::ir_variable(const struct glsl_type *type, const char *name,
>  			 ir_variable_mode mode)
>     : max_array_access(0), max_ifc_array_access(NULL),
>       read_only(false), centroid(false), invariant(false),
> -        mode(mode), interpolation(INTERP_QUALIFIER_NONE), atomic()
> +     how_declared(ir_var_declared_normally), mode(mode),
> +     interpolation(INTERP_QUALIFIER_NONE), atomic()
>  {
>     this->ir_type = ir_type_variable;
>     this->type = type;
> diff --git a/src/glsl/ir.h b/src/glsl/ir.h
> index 7859702..4f775da 100644
> --- a/src/glsl/ir.h
> +++ b/src/glsl/ir.h
> @@ -294,6 +294,34 @@ enum ir_variable_mode {
>  };
>  
>  /**
> + * Enum keeping track of how a variable was declared.  For error checking of
> + * the gl_PerVertex redeclaration rules.
> + */
> +enum ir_var_declaration_type {
> +   /**
> +    * Normal declaration (for most variables, this means an explicit
> +    * declaration.  Exception: temporaries are always implicitly declared, but
> +    * they still use ir_var_declared_normally).
> +    *
> +    * Note: an ir_variable that represents a named interface block uses
> +    * ir_var_declared_normally.
> +    */
> +   ir_var_declared_normally = 0,
> +
> +   /**
> +    * Variable was explicitly declared (or re-declared) in an unnamed
> +    * interface block.
> +    */
> +   ir_var_declared_in_block,
> +
> +   /**
> +    * Variable is an implicitly declared built-in that has not been explicitly
> +    * re-declared by the shader.
> +    */
> +   ir_var_declared_implicitly,
> +};
> +
> +/**
>   * \brief Layout qualifiers for gl_FragDepth.
>   *
>   * The AMD/ARB_conservative_depth extensions allow gl_FragDepth to be redeclared
> @@ -526,6 +554,14 @@ public:
>     unsigned assigned:1;
>  
>     /**
> +    * Enum indicating how the variable was declared.  See
> +    * ir_var_declaration_type.
> +    *
> +    * This is used to detect certain kinds of illegal variable redeclarations.
> +    */
> +   unsigned how_declared:2;
> +
> +   /**
>      * Storage class of the variable.
>      *
>      * \sa ir_variable_mode
> diff --git a/src/glsl/ir_clone.cpp b/src/glsl/ir_clone.cpp
> index b0f173a..40ed33a 100644
> --- a/src/glsl/ir_clone.cpp
> +++ b/src/glsl/ir_clone.cpp
> @@ -68,6 +68,7 @@ ir_variable::clone(void *mem_ctx, struct hash_table *ht) const
>     var->has_initializer = this->has_initializer;
>     var->depth_layout = this->depth_layout;
>     var->assigned = this->assigned;
> +   var->how_declared = this->how_declared;
>     var->used = this->used;
>  
>     var->num_state_slots = this->num_state_slots;
> 



More information about the mesa-stable mailing list