[Mesa-dev] [PATCH] glsl: fix binding validation for interface blocks

[Timothy Arceri]

On Thu, 2015-05-28 at 07:07 +1000, Dave Airlie wrote:
> On 28 May 2015 at 06:51, Timothy Arceri <t_arceri at yahoo.com.au> wrote:
> > ---- Matt Turner wrote ----
> >
> >> On Wed, May 27, 2015 at 3:53 AM, Timothy Arceri <t_arceri at yahoo.com.au>
> >> wrote:
> >> > Cc: Kenneth Graunke <kenneth at whitecape.org>
> >> > ---
> >> >
> >> >  It looks like this may never have actually worked, at least there
> >> >  is no way it could have for inteface block arrays :)
> >> >
> >> >  No piglit regressions.
> >> >
> >> >  src/glsl/ast_to_hir.cpp | 22 ++++++++++++++--------
> >> >  1 file changed, 14 insertions(+), 8 deletions(-)
> >> >
> >> > diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp
> >> > index 0ab664e..861bbfe 100644
> >> > --- a/src/glsl/ast_to_hir.cpp
> >> > +++ b/src/glsl/ast_to_hir.cpp
> >> > @@ -2041,10 +2041,10 @@ validate_matrix_layout_for_type(struct
> >> > _mesa_glsl_parse_state *state,
> >> >  static bool
> >> >  validate_binding_qualifier(struct _mesa_glsl_parse_state *state,
> >> >                             YYLTYPE *loc,
> >> > -                           ir_variable *var,
> >> > +                           const glsl_type *type,
> >> >                             const ast_type_qualifier *qual)
> >> >  {
> >> > -   if (var->data.mode != ir_var_uniform) {
> >> > +   if (!qual->flags.q.uniform) {
> >> >        _mesa_glsl_error(loc, state,
> >> >                         "the \"binding\" qualifier only applies to
> >> > uniforms");
> >> >        return false;
> >> > @@ -2056,10 +2056,11 @@ validate_binding_qualifier(struct
> >> > _mesa_glsl_parse_state *state,
> >> >     }
> >> >
> >> >     const struct gl_context *const ctx = state->ctx;
> >> > -   unsigned elements = var->type->is_array() ? var->type->length : 1;
> >> > +   unsigned elements = type->is_array() ? type->length : 1;
> >> >     unsigned max_index = qual->binding + elements - 1;
> >> > +   const glsl_type *base_type = type->without_array();
> >> >
> >> > -   if (var->type->is_interface()) {
> >> > +   if (base_type->is_interface()) {
> >> >        /* UBOs.  From page 60 of the GLSL 4.20 specification:
> >> >         * "If the binding point for any uniform block instance is less
> >> > than zero,
> >> >         *  or greater than or equal to the implementation-dependent
> >> > maximum
> >> > @@ -2077,8 +2078,7 @@ validate_binding_qualifier(struct
> >> > _mesa_glsl_parse_state *state,
> >> >                            ctx->Const.MaxUniformBufferBindings);
> >> >           return false;
> >> >        }
> >> > -   } else if (var->type->is_sampler() ||
> >> > -              (var->type->is_array() &&
> >> > var->type->fields.array->is_sampler())) {
> >> > +   } else if (base_type->is_sampler()) {
> >> >        /* Samplers.  From page 63 of the GLSL 4.20 specification:
> >> >         * "If the binding is less than zero, or greater than or equal to
> >> > the
> >> >         *  implementation-dependent maximum supported number of units, a
> >> > @@ -2095,7 +2095,7 @@ validate_binding_qualifier(struct
> >> > _mesa_glsl_parse_state *state,
> >> >
> >> >           return false;
> >> >        }
> >> > -   } else if (var->type->contains_atomic()) {
> >> > +   } else if (base_type->contains_atomic()) {
> >> >        assert(ctx->Const.MaxAtomicBufferBindings <=
> >> > MAX_COMBINED_ATOMIC_BUFFERS);
> >> >        if (unsigned(qual->binding) >=
> >> > ctx->Const.MaxAtomicBufferBindings) {
> >> >           _mesa_glsl_error(loc, state, "layout(binding = %d) exceeds the
> >> > "
> >> > @@ -2651,7 +2651,7 @@ apply_type_qualifier_to_variable(const struct
> >> > ast_type_qualifier *qual,
> >> >     }
> >> >
> >> >     if (qual->flags.q.explicit_binding &&
> >> > -       validate_binding_qualifier(state, loc, var, qual)) {
> >> > +       validate_binding_qualifier(state, loc, var->type, qual)) {
> >> >        var->data.explicit_binding = true;
> >> >        var->data.binding = qual->binding;
> >> >     }
> >> > @@ -5658,6 +5658,8 @@ ast_interface_block::hir(exec_list *instructions,
> >> >                                          num_variables,
> >> >                                          packing,
> >> >                                          this->block_name);
> >> > +   if (this->layout.flags.q.explicit_binding)
> >> > +      validate_binding_qualifier(state, &loc, block_type,
> >> > &this->layout);
> >> >
> >> >     if (!state->symbols->add_interface(block_type->name, block_type,
> >> > var_mode)) {
> >> >        YYLTYPE loc = this->get_location();
> >> > @@ -5744,6 +5746,10 @@ ast_interface_block::hir(exec_list *instructions,
> >> >           const glsl_type *block_array_type =
> >> >              process_array_type(&loc, block_type, this->array_specifier,
> >> > state);
> >> >
> >> > +         if(this->layout.flags.q.explicit_binding)
> >>
> >> Space after if
> >
> > The space was intentional to separate validation code from from code that
> > creates the IR. If you still think it should go I can remove it but I think
> > it helps with code readability.
> 
> he meant the missing space between if and (
> 

oh right, thanks.

> Dave.