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

[Timothy Arceri]

On 21/10/17 03:00, Ilia Mirkin wrote:
> 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.

This is where it's done. Without it you won't catch:

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

> 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
>