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

[Ilia Mirkin]

On Fri, Feb 12, 2016 at 5:53 PM, Timothy Arceri <t_arceri at yahoo.com.au> 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.

An alternative, as mentioned on IRC, is to stick it into
apply_explicit_location in the if (uniform) check. Or maybe somewhere
else entirely.

  -ilia