[Mesa-dev] [PATCH] glsl: reject explicit location on atomic counter uniforms

[Timothy Arceri]

On Fri, 2016-02-12 at 20:57 -0500, Ilia Mirkin wrote:
> On Fri, Feb 12, 2016 at 8:52 PM, Timothy Arceri <t_arceri at yahoo.com.a
> u> wrote:
> > On Fri, 2016-02-12 at 18:45 -0500, Ilia Mirkin wrote:
> > > On Fri, Feb 12, 2016 at 5:53 PM, Timothy Arceri <t_arceri at yahoo.c
> > > om.a
> > > u> wrote:
> > > > On Thu, 2016-02-11 at 20:10 -0500, Ilia Mirkin wrote:
> > > > > This fixes
> > > > > 
> > > > > dEQP-
> > > > > GLES31.functional.uniform_location.negative.atomic_fragment
> > > > > dEQP-
> > > > > GLES31.functional.uniform_location.negative.atomic_vertex
> > > > > 
> > > > > Both of which have lines like
> > > > > 
> > > > > layout(location = 3, binding = 0, offset = 0) uniform
> > > > > atomic_uint
> > > > > uni0;
> > > > > 
> > > > > The ARB_explicit_uniform_location spec makes a very
> > > > > tangential
> > > > > mention
> > > > > regarding atomic counters, but location isn't something that
> > > > > makes
> > > > > sense
> > > > > with them.
> > > > > 
> > > > > Signed-off-by: Ilia Mirkin <imirkin at alum.mit.edu>
> > > > > ---
> > > > > 
> > > > > Had no clue where to stick this check... this seemed like as
> > > > > good
> > > > > a
> > > > > place as any.
> > > > > 
> > > > >  src/compiler/glsl/ast_to_hir.cpp | 5 +++++
> > > > >  1 file changed, 5 insertions(+)
> > > > > 
> > > > > diff --git a/src/compiler/glsl/ast_to_hir.cpp
> > > > > b/src/compiler/glsl/ast_to_hir.cpp
> > > > > index dbeb5c0..9fce06b 100644
> > > > > --- a/src/compiler/glsl/ast_to_hir.cpp
> > > > > +++ b/src/compiler/glsl/ast_to_hir.cpp
> > > > > @@ -4179,6 +4179,11 @@ ast_declarator_list::hir(exec_list
> > > > > *instructions,
> > > > >              state->atomic_counter_offsets[qual_binding] =
> > > > > qual_offset;
> > > > >           }
> > > > >        }
> > > > 
> > > > Maybe we should just make this:
> > > >    else {
> > > >       _mesa_glsl_error(&loc, state, "invalid atomic counter
> > > > layout
> > > > qualifier");
> > > >    }
> > > 
> > > Nope, that doesn't work. In this case
> > > 
> > > layout(location = 3, binding = 0, offset = 0)
> > > 
> > > it goes into the if () case above, as these are all merged by the
> > > time
> > > it goes into hir. Also, it's legal to just have binding, in which
> > > case
> > > it'd go into the else, which we don't want either.
> > > 
> > > This has to be a standalone condition if I do it here.
> > 
> > Right. I corrected myself on IRC. IMO it should be done something
> > like
> > this.
> > 
> >    /* Valid atomic layout qualifiers */
> >    ast_type_qualifier atomic_layout_mask;
> >    atomic_layout_mask.flags.i = 0;
> >    atomic_layout_mask.flags.q.explicit_binding = 1;
> >    atomic_layout_mask.flags.q.explicit_offset = 1;
> >    atomic_layout_mask.flags.q.unifrom = 1 ; ???
> > 
> >    if ((qual->flags.i & ~atomic_layout_mask.flags.i) != 0)
> >       _mesa_glsl_error(&loc, state, "invalid atomic counter layout
> > qualifier");
> > 
> > As I don't see why we should be treating location as a special
> > case.
> 
> Ah OK. But this should be done here, or moved into
> ast_type::merge_qualifier or whereever it does the other masking
> logic?

It would be nice to have it in ast_type::merge_qualifier or maybe a
helper called from there but I don't think it knows what type its
dealing with. It just knows about the qualifier and I'm not sure how
easy it is to change that.

It would be nice to go in a helper somewhere, but I think its ok for
now to call it from this location.  


> 
>   -ilia