[Mesa-dev] [PATCH 3/3] glsl/linker: validate explicit locations for SSO programs

[Ilia Mirkin]

On Fri, Oct 20, 2017 at 11:55 AM, Timothy Arceri <tarceri at itsqueeze.com> wrote:
>
>
> On 21/10/17 00:35, Ilia Mirkin wrote:
>>
>> On Fri, Oct 20, 2017 at 6:46 AM, Iago Toral Quiroga <itoral at igalia.com>
>> wrote:
>>>
>>> ---
>>>   src/compiler/glsl/link_varyings.cpp | 53
>>> +++++++++++++++++++++++++++++++++++++
>>>   src/compiler/glsl/link_varyings.h   |  4 +++
>>>   src/compiler/glsl/linker.cpp        |  6 +++++
>>>   3 files changed, 63 insertions(+)
>>>
>>> diff --git a/src/compiler/glsl/link_varyings.cpp
>>> b/src/compiler/glsl/link_varyings.cpp
>>> index dffb4f98df..d7e407184d 100644
>>> --- a/src/compiler/glsl/link_varyings.cpp
>>> +++ b/src/compiler/glsl/link_varyings.cpp
>>> @@ -599,6 +599,59 @@ validate_explicit_variable_location(struct
>>> gl_context *ctx,
>>>   }
>>>
>>>   /**
>>> + * Validate explicit locations for SSO programs. For non-SSO programs
>>> this
>>> + * is alreadty done in cross_validate_outputs_to_inputs().
>>> + */
>>> +void
>>> +validate_sso_explicit_locations(struct gl_context *ctx,
>>> +                                struct gl_shader_program *prog)
>>> +{
>>> +   assert(prog->SeparateShader);
>>> +   struct explicit_location_info
>>> explicit_locations_in[MAX_VARYINGS_INCL_PATCH][4];
>>> +   struct explicit_location_info
>>> explicit_locations_out[MAX_VARYINGS_INCL_PATCH][4];
>>
>>
>> These comments may point to issues in earlier patches / overall logic,
>> but I'll still make them here. I don't have time to do a full review
>> of all the patches, unfortunately. Thanks for addressing my concerns
>> with your earlier patchset.
>>
>> This should be MAX_VARYINGS. The patch varyings are meant to be
>> aliased against the non-patch varyings, and their indices must be
>> assigned as such, otherwise you won't get the overlaps you're supposed
>> to.
>
>
> Are you sure this is really required?

https://github.com/KhronosGroup/OpenGL-API/issues/13

>> Furthermore, I haven't checked how your code works, but I found it was
>> easier to not have the [4]. None of the checks need to be
>> per-component (and in fact, you're supposed to raise an error when
>> components disagree on type-related things, except for VS inputs which
>> you skip here anyways). Just store the first value in the info, and
>> then if anything comes in counter to that, return an error. You can
>> see what I did in https://patchwork.freedesktop.org/patch/175959/. I
>> spent quite some time to make sure that the patch was correct, so if
>> you see any functional differences to what you've done, I'd recommend
>> considering why there are differences -- there shouldn't be any.
>
>
> As I keep pointing out your patch doesn't handle component aliasing.
> Dropping the [4] would mean we not longer check for this.

I'm 50% sure that something else checks that already. I'm talking about

layout (location=0, component=0) int a;
layout (location=0, component=1) float b;

being disallowed due to int/float mixing (location aliasing, I think
it's called, but it's been a month or two since I've read the spec).

  -ilia